[v2,5/7] hwrng: st: Add support for ST's HW Random Number Generator

Message ID 1442497557-9271-6-git-send-email-lee.jones@linaro.org
State Accepted
Commit 4a4da53c408c9e1e545b60d2b07635d08a949c99
Headers show

Commit Message

Lee Jones Sept. 17, 2015, 1:45 p.m.
Signed-off-by: Pankaj Dev <pankaj.dev@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/char/hw_random/Kconfig  |  10 +++
 drivers/char/hw_random/Makefile |   1 +
 drivers/char/hw_random/st-rng.c | 144 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 drivers/char/hw_random/st-rng.c

Comments

Kieran Bingham Sept. 18, 2015, 10:44 a.m. | #1
On 17 September 2015 at 14:45, Lee Jones <lee.jones@linaro.org> wrote:
> Signed-off-by: Pankaj Dev <pankaj.dev@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Comments addressed, Also LGTM.
Acked-by: Kieran Bingham <kieranbingham@gmail.com>


> ---
>  drivers/char/hw_random/Kconfig  |  10 +++
>  drivers/char/hw_random/Makefile |   1 +
>  drivers/char/hw_random/st-rng.c | 144 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 drivers/char/hw_random/st-rng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 8998108..ba5406b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -346,6 +346,16 @@ config HW_RANDOM_MSM
>
>           If unsure, say Y.
>
> +config HW_RANDOM_ST
> +       tristate "ST Microelectronics HW Random Number Generator support"
> +       depends on HW_RANDOM && ARCH_STI
> +       ---help---
> +         This driver provides kernel-side support for the Random Number
> +         Generator hardware found on STi series of SoCs.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called st-rng.
> +
>  config HW_RANDOM_XGENE
>         tristate "APM X-Gene True Random Number Generator (TRNG) support"
>         depends on HW_RANDOM && ARCH_XGENE
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 055bb01..8bcfb45 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
>  obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
>  obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
>  obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> +obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
>  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> new file mode 100644
> index 0000000..8c8a435
> --- /dev/null
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -0,0 +1,144 @@
> +/*
> + * ST Random Number Generator Driver ST's Platforms
> + *
> + * Author: Pankaj Dev: <pankaj.dev@st.com>
> + *         Lee Jones <lee.jones@linaro.org>
> + *
> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Registers */
> +#define ST_RNG_STATUS_REG              0x20
> +#define ST_RNG_DATA_REG                        0x24
> +
> +/* Registers fields */
> +#define ST_RNG_STATUS_BAD_SEQUENCE     BIT(0)
> +#define ST_RNG_STATUS_BAD_ALTERNANCE   BIT(1)
> +#define ST_RNG_STATUS_FIFO_FULL                BIT(5)
> +
> +#define ST_RNG_FIFO_SIZE               8
> +#define ST_RNG_SAMPLE_SIZE             2 /* 2 Byte (16bit) samples */
> +
> +/* Samples are available every 0.667us, which we round to 1us */
> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> +
> +struct st_rng_data {
> +       void __iomem    *base;
> +       struct clk      *clk;
> +       struct hwrng    ops;
> +};
> +
> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +       struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> +       u32 status;
> +       int i;
> +
> +       if (max < sizeof(u16))
> +               return -EINVAL;
> +
> +       /* Wait until FIFO is full - max 4uS*/
> +       for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> +               status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> +               if (status & ST_RNG_STATUS_FIFO_FULL)
> +                       break;
> +               udelay(1);
> +       }
> +
> +       if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> +               return 0;
> +
> +       for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> +               *(u16 *)(data + i) =
> +                       readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> +
> +       return i;       /* No of bytes read */
> +}
> +
> +static int st_rng_probe(struct platform_device *pdev)
> +{
> +       struct st_rng_data *ddata;
> +       struct resource *res;
> +       struct clk *clk;
> +       void __iomem *base;
> +       int ret;
> +
> +       ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> +       if (!ddata)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               return ret;
> +
> +       ddata->ops.priv = (unsigned long)ddata;
> +       ddata->ops.read = st_rng_read;
> +       ddata->ops.name = pdev->name;
> +       ddata->base     = base;
> +       ddata->clk      = clk;
> +
> +       dev_set_drvdata(&pdev->dev, ddata);
> +
> +       ret = hwrng_register(&ddata->ops);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register HW RNG\n");
> +               return ret;
> +       }
> +
> +       dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> +
> +       return 0;
> +}
> +
> +static int st_rng_remove(struct platform_device *pdev)
> +{
> +       struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> +
> +       hwrng_unregister(&ddata->ops);
> +
> +       clk_disable_unprepare(ddata->clk);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id st_rng_match[] = {
> +       { .compatible = "st,rng" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, st_rng_match);
> +
> +static struct platform_driver st_rng_driver = {
> +       .driver = {
> +               .name = "st-hwrandom",
> +               .of_match_table = of_match_ptr(st_rng_match),
> +       },
> +       .probe = st_rng_probe,
> +       .remove = st_rng_remove
> +};
> +
> +module_platform_driver(st_rng_driver);
> +
> +MODULE_AUTHOR("Pankaj Dev <pankaj.dev@st.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Thompson Oct. 5, 2015, 10:44 a.m. | #2
Hi Lee

Late but...

On 17/09/15 14:45, Lee Jones wrote:
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 055bb01..8bcfb45 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
>   obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
>   obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
>   obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> +obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
>   obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> new file mode 100644
> index 0000000..8c8a435
> --- /dev/null
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -0,0 +1,144 @@
> +/*
> + * ST Random Number Generator Driver ST's Platforms
> + *
> + * Author: Pankaj Dev: <pankaj.dev@st.com>
> + *         Lee Jones <lee.jones@linaro.org>
> + *
> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Registers */
> +#define ST_RNG_STATUS_REG		0x20
> +#define ST_RNG_DATA_REG			0x24
> +
> +/* Registers fields */
> +#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> +#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> +#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> +
> +#define ST_RNG_FIFO_SIZE		8
> +#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> +
> +/* Samples are available every 0.667us, which we round to 1us */
> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> +
> +struct st_rng_data {
> +	void __iomem	*base;
> +	struct clk	*clk;
> +	struct hwrng	ops;
> +};
> +
> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> +	u32 status;
> +	int i;
> +
> +	if (max < sizeof(u16))
> +		return -EINVAL;
> +
> +	/* Wait until FIFO is full - max 4uS*/
> +	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> +		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> +		if (status & ST_RNG_STATUS_FIFO_FULL)
> +			break;
> +		udelay(1);

How much bandwidth does using udelay() cost? I think it could be >10% 
compared to a tighter polling loop.


> +	}
> +
> +	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> +		return 0;

Isn't a timeout an error condition?


> +
> +	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> +		*(u16 *)(data + i) =
> +			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> +
> +	return i;	/* No of bytes read */
> +}
> +
> +static int st_rng_probe(struct platform_device *pdev)
> +{
> +	struct st_rng_data *ddata;
> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *base;
> +	int ret;
> +
> +	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	ddata->ops.priv	= (unsigned long)ddata;
> +	ddata->ops.read	= st_rng_read;
> +	ddata->ops.name	= pdev->name;
> +	ddata->base	= base;
> +	ddata->clk	= clk;
> +
> +	dev_set_drvdata(&pdev->dev, ddata);
> +
> +	ret = hwrng_register(&ddata->ops);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register HW RNG\n");

Why shout about this particular error but not any others? Perhaps just 
rely on the driver core to report the error here?


> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> +
> +	return 0;
> +}
> +
> +static int st_rng_remove(struct platform_device *pdev)
> +{
> +	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> +
> +	hwrng_unregister(&ddata->ops);
> +
> +	clk_disable_unprepare(ddata->clk);

This mismatches the error paths in the probe function (there is no 
cleanup of clock counts in probe function).


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Oct. 5, 2015, 12:11 p.m. | #3
On Mon, 05 Oct 2015, Daniel Thompson wrote:
> Late but...

That's okay.  Fixup patches can always be submitted.

We have forever. :)

> On 17/09/15 14:45, Lee Jones wrote:
> >diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> >index 055bb01..8bcfb45 100644
> >--- a/drivers/char/hw_random/Makefile
> >+++ b/drivers/char/hw_random/Makefile
> >@@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
> >  obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
> >  obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
> >  obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> >+obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
> >  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> >diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> >new file mode 100644
> >index 0000000..8c8a435
> >--- /dev/null
> >+++ b/drivers/char/hw_random/st-rng.c
> >@@ -0,0 +1,144 @@
> >+/*
> >+ * ST Random Number Generator Driver ST's Platforms
> >+ *
> >+ * Author: Pankaj Dev: <pankaj.dev@st.com>
> >+ *         Lee Jones <lee.jones@linaro.org>
> >+ *
> >+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License version 2 as
> >+ * published by the Free Software Foundation.
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/delay.h>
> >+#include <linux/hw_random.h>
> >+#include <linux/io.h>
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+
> >+/* Registers */
> >+#define ST_RNG_STATUS_REG		0x20
> >+#define ST_RNG_DATA_REG			0x24
> >+
> >+/* Registers fields */
> >+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> >+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> >+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> >+
> >+#define ST_RNG_FIFO_SIZE		8
> >+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> >+
> >+/* Samples are available every 0.667us, which we round to 1us */
> >+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> >+
> >+struct st_rng_data {
> >+	void __iomem	*base;
> >+	struct clk	*clk;
> >+	struct hwrng	ops;
> >+};
> >+
> >+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >+{
> >+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> >+	u32 status;
> >+	int i;
> >+
> >+	if (max < sizeof(u16))
> >+		return -EINVAL;
> >+
> >+	/* Wait until FIFO is full - max 4uS*/
> >+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> >+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> >+		if (status & ST_RNG_STATUS_FIFO_FULL)
> >+			break;
> >+		udelay(1);
> 
> How much bandwidth does using udelay() cost? I think it could be
> >10% compared to a tighter polling loop.

Samples are only available every 0.7uS and we only do this for every
4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
over that tiny amount of time?  It's an understandable point if we
were talking about milliseconds, but a single microsecond?

> >+	}
> >+
> >+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> >+		return 0;
> 
> Isn't a timeout an error condition?

Yes, which is why we're returning 0.  In this context 0 == 'no data'.
This will be converted to -EAGAIN if the caller did not request
NONBLOCKING.

> >+
> >+	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> >+		*(u16 *)(data + i) =
> >+			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> >+
> >+	return i;	/* No of bytes read */
> >+}
> >+
> >+static int st_rng_probe(struct platform_device *pdev)
> >+{
> >+	struct st_rng_data *ddata;
> >+	struct resource *res;
> >+	struct clk *clk;
> >+	void __iomem *base;
> >+	int ret;
> >+
> >+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> >+	if (!ddata)
> >+		return -ENOMEM;
> >+
> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+	base = devm_ioremap_resource(&pdev->dev, res);
> >+	if (IS_ERR(base))
> >+		return PTR_ERR(base);
> >+
> >+	clk = devm_clk_get(&pdev->dev, NULL);
> >+	if (IS_ERR(clk))
> >+		return PTR_ERR(clk);
> >+
> >+	ret = clk_prepare_enable(clk);
> >+	if (ret)
> >+		return ret;
> >+
> >+	ddata->ops.priv	= (unsigned long)ddata;
> >+	ddata->ops.read	= st_rng_read;
> >+	ddata->ops.name	= pdev->name;
> >+	ddata->base	= base;
> >+	ddata->clk	= clk;
> >+
> >+	dev_set_drvdata(&pdev->dev, ddata);
> >+
> >+	ret = hwrng_register(&ddata->ops);
> >+	if (ret) {
> >+		dev_err(&pdev->dev, "Failed to register HW RNG\n");
> 
> Why shout about this particular error but not any others? Perhaps
> just rely on the driver core to report the error here?

I have omitted error prints from subsystem calls which do all the
shouting required.  Unfortunately the HWRNG is somewhat stuck in the
past in a number of ways; a lack of subsystem level shouting being one
of them.

> >+		return ret;
> >+	}
> >+
> >+	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> >+
> >+	return 0;
> >+}
> >+
> >+static int st_rng_remove(struct platform_device *pdev)
> >+{
> >+	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> >+
> >+	hwrng_unregister(&ddata->ops);
> >+
> >+	clk_disable_unprepare(ddata->clk);
> 
> This mismatches the error paths in the probe function (there is no
> cleanup of clock counts in probe function).

Good catch.  I am missing a clk_disable_unprepare() in the
hwrng_register() failure patch.  Will fix.
Daniel Thompson Oct. 5, 2015, 12:54 p.m. | #4
On 05/10/15 13:11, Lee Jones wrote:
> On Mon, 05 Oct 2015, Daniel Thompson wrote:
>> Late but...
>
> That's okay.  Fixup patches can always be submitted.
>
> We have forever. :)
>
>> On 17/09/15 14:45, Lee Jones wrote:
>>> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
>>> index 055bb01..8bcfb45 100644
>>> --- a/drivers/char/hw_random/Makefile
>>> +++ b/drivers/char/hw_random/Makefile
>>> @@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
>>>   obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
>>>   obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
>>>   obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
>>> +obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
>>>   obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
>>> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
>>> new file mode 100644
>>> index 0000000..8c8a435
>>> --- /dev/null
>>> +++ b/drivers/char/hw_random/st-rng.c
>>> @@ -0,0 +1,144 @@
>>> +/*
>>> + * ST Random Number Generator Driver ST's Platforms
>>> + *
>>> + * Author: Pankaj Dev: <pankaj.dev@st.com>
>>> + *         Lee Jones <lee.jones@linaro.org>
>>> + *
>>> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/hw_random.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +
>>> +/* Registers */
>>> +#define ST_RNG_STATUS_REG		0x20
>>> +#define ST_RNG_DATA_REG			0x24
>>> +
>>> +/* Registers fields */
>>> +#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
>>> +#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
>>> +#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
>>> +
>>> +#define ST_RNG_FIFO_SIZE		8
>>> +#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
>>> +
>>> +/* Samples are available every 0.667us, which we round to 1us */
>>> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
>>> +
>>> +struct st_rng_data {
>>> +	void __iomem	*base;
>>> +	struct clk	*clk;
>>> +	struct hwrng	ops;
>>> +};
>>> +
>>> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>>> +{
>>> +	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
>>> +	u32 status;
>>> +	int i;
>>> +
>>> +	if (max < sizeof(u16))
>>> +		return -EINVAL;
>>> +
>>> +	/* Wait until FIFO is full - max 4uS*/
>>> +	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
>>> +		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
>>> +		if (status & ST_RNG_STATUS_FIFO_FULL)
>>> +			break;
>>> +		udelay(1);
>>
>> How much bandwidth does using udelay() cost? I think it could be
>>> 10% compared to a tighter polling loop.
>
> Samples are only available every 0.7uS and we only do this for every
> 4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
> over that tiny amount of time?  It's an understandable point if we
> were talking about milliseconds, but a single microsecond?

This code is called in a tight loop so we're not talking about a 
*single* microsecond! We are are talking about about one microsecond in 
every five[1] and yes, I think we should care about that.

It takes 2.67uS for the FIFO to come ready so with a polling interval of 
1uS + loop overhead so I would fully expect on average to "waste" 0.5uS 
each time st_rng_read() is called (in a tight loop).


[1]... point three recurring  ;-)


>>> +	}
>>> +
>>> +	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
>>> +		return 0;
>>
>> Isn't a timeout an error condition?
>
> Yes, which is why we're returning 0.  In this context 0 == 'no data'.
> This will be converted to -EAGAIN if the caller did not request
> NONBLOCKING.

I took the view that hitting the timeout means the hardware is broken. 
Is it likely that the next time we call it there will be data ready? If 
it is should it be trusted?


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Oct. 5, 2015, 2:52 p.m. | #5
On Mon, 05 Oct 2015, Daniel Thompson wrote:
> On 05/10/15 13:11, Lee Jones wrote:
> >On Mon, 05 Oct 2015, Daniel Thompson wrote:
> >>Late but...
> >
> >That's okay.  Fixup patches can always be submitted.
> >
> >We have forever. :)
> >
> >>On 17/09/15 14:45, Lee Jones wrote:
> >>>diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> >>>index 055bb01..8bcfb45 100644
> >>>--- a/drivers/char/hw_random/Makefile
> >>>+++ b/drivers/char/hw_random/Makefile
> >>>@@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
> >>>  obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
> >>>  obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
> >>>  obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> >>>+obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
> >>>  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> >>>diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> >>>new file mode 100644
> >>>index 0000000..8c8a435
> >>>--- /dev/null
> >>>+++ b/drivers/char/hw_random/st-rng.c
> >>>@@ -0,0 +1,144 @@
> >>>+/*
> >>>+ * ST Random Number Generator Driver ST's Platforms
> >>>+ *
> >>>+ * Author: Pankaj Dev: <pankaj.dev@st.com>
> >>>+ *         Lee Jones <lee.jones@linaro.org>
> >>>+ *
> >>>+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> >>>+ *
> >>>+ * This program is free software; you can redistribute it and/or modify
> >>>+ * it under the terms of the GNU General Public License version 2 as
> >>>+ * published by the Free Software Foundation.
> >>>+ */
> >>>+
> >>>+#include <linux/clk.h>
> >>>+#include <linux/delay.h>
> >>>+#include <linux/hw_random.h>
> >>>+#include <linux/io.h>
> >>>+#include <linux/module.h>
> >>>+#include <linux/of.h>
> >>>+#include <linux/platform_device.h>
> >>>+#include <linux/slab.h>
> >>>+
> >>>+/* Registers */
> >>>+#define ST_RNG_STATUS_REG		0x20
> >>>+#define ST_RNG_DATA_REG			0x24
> >>>+
> >>>+/* Registers fields */
> >>>+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> >>>+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> >>>+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> >>>+
> >>>+#define ST_RNG_FIFO_SIZE		8
> >>>+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> >>>+
> >>>+/* Samples are available every 0.667us, which we round to 1us */
> >>>+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> >>>+
> >>>+struct st_rng_data {
> >>>+	void __iomem	*base;
> >>>+	struct clk	*clk;
> >>>+	struct hwrng	ops;
> >>>+};
> >>>+
> >>>+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >>>+{
> >>>+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> >>>+	u32 status;
> >>>+	int i;
> >>>+
> >>>+	if (max < sizeof(u16))
> >>>+		return -EINVAL;
> >>>+
> >>>+	/* Wait until FIFO is full - max 4uS*/
> >>>+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> >>>+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> >>>+		if (status & ST_RNG_STATUS_FIFO_FULL)
> >>>+			break;
> >>>+		udelay(1);
> >>
> >>How much bandwidth does using udelay() cost? I think it could be
> >>>10% compared to a tighter polling loop.
> >
> >Samples are only available every 0.7uS and we only do this for every
> >4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
> >over that tiny amount of time?  It's an understandable point if we
> >were talking about milliseconds, but a single microsecond?
> 
> This code is called in a tight loop so we're not talking about a
> *single* microsecond! We are are talking about about one microsecond
> in every five[1] and yes, I think we should care about that.
> 
> It takes 2.67uS for the FIFO to come ready so with a polling
> interval of 1uS + loop overhead so I would fully expect on average
> to "waste" 0.5uS each time st_rng_read() is called (in a tight
> loop).
> 
> [1]... point three recurring  ;-)

`time dd if=/dev/hwrng of=/dev/null bs=1 count=10M`

/* Current code, inc delay */
Run 1: real 0m17.996s
Run 2: real 0m17.991s
Run 3: real 0m17.996s
Run 4: real 0m18.000s
Run 5: real 0m18.000s
Total       0m89.983s

/* Tight loop, no delay */
Run 1: real 0m18.011s
Run 2: real 0m17.995s
Run 3: real 0m18.005s
Run 4: real 0m18.020s
Run 5: real 0m17.990s
Total       0m90.021s

(89.983 / 90.021) * 100 = 99.958%

0.042% saving.

Not quite the >10% predicted.  I'd say that's negligible. 

> >>>+	}
> >>>+
> >>>+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> >>>+		return 0;
> >>
> >>Isn't a timeout an error condition?
> >
> >Yes, which is why we're returning 0.  In this context 0 == 'no data'.
> >This will be converted to -EAGAIN if the caller did not request
> >NONBLOCKING.
> 
> I took the view that hitting the timeout means the hardware is
> broken. Is it likely that the next time we call it there will be
> data ready? If it is should it be trusted?

From experience gained whilst testing, I can say that when returning
an error the HNG breaks and the user receives an error.  If instead we
return 0, we get to have another go and random numbers again pour
out.  Perhaps we're just not waiting long enough?  Either way, the
current implementation works real well.

Patch

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 8998108..ba5406b 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -346,6 +346,16 @@  config HW_RANDOM_MSM
 
 	  If unsure, say Y.
 
+config HW_RANDOM_ST
+	tristate "ST Microelectronics HW Random Number Generator support"
+	depends on HW_RANDOM && ARCH_STI
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator hardware found on STi series of SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called st-rng.
+
 config HW_RANDOM_XGENE
 	tristate "APM X-Gene True Random Number Generator (TRNG) support"
 	depends on HW_RANDOM && ARCH_XGENE
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 055bb01..8bcfb45 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -30,4 +30,5 @@  obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
 obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
 obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
+obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
 obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
new file mode 100644
index 0000000..8c8a435
--- /dev/null
+++ b/drivers/char/hw_random/st-rng.c
@@ -0,0 +1,144 @@ 
+/*
+ * ST Random Number Generator Driver ST's Platforms
+ *
+ * Author: Pankaj Dev: <pankaj.dev@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
+ *
+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Registers */
+#define ST_RNG_STATUS_REG		0x20
+#define ST_RNG_DATA_REG			0x24
+
+/* Registers fields */
+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
+
+#define ST_RNG_FIFO_SIZE		8
+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
+
+/* Samples are available every 0.667us, which we round to 1us */
+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
+
+struct st_rng_data {
+	void __iomem	*base;
+	struct clk	*clk;
+	struct hwrng	ops;
+};
+
+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
+	u32 status;
+	int i;
+
+	if (max < sizeof(u16))
+		return -EINVAL;
+
+	/* Wait until FIFO is full - max 4uS*/
+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
+		if (status & ST_RNG_STATUS_FIFO_FULL)
+			break;
+		udelay(1);
+	}
+
+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
+		return 0;
+
+	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
+		*(u16 *)(data + i) =
+			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
+
+	return i;	/* No of bytes read */
+}
+
+static int st_rng_probe(struct platform_device *pdev)
+{
+	struct st_rng_data *ddata;
+	struct resource *res;
+	struct clk *clk;
+	void __iomem *base;
+	int ret;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	ddata->ops.priv	= (unsigned long)ddata;
+	ddata->ops.read	= st_rng_read;
+	ddata->ops.name	= pdev->name;
+	ddata->base	= base;
+	ddata->clk	= clk;
+
+	dev_set_drvdata(&pdev->dev, ddata);
+
+	ret = hwrng_register(&ddata->ops);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register HW RNG\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
+
+	return 0;
+}
+
+static int st_rng_remove(struct platform_device *pdev)
+{
+	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
+
+	hwrng_unregister(&ddata->ops);
+
+	clk_disable_unprepare(ddata->clk);
+
+	return 0;
+}
+
+static const struct of_device_id st_rng_match[] = {
+	{ .compatible = "st,rng" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_rng_match);
+
+static struct platform_driver st_rng_driver = {
+	.driver = {
+		.name = "st-hwrandom",
+		.of_match_table = of_match_ptr(st_rng_match),
+	},
+	.probe = st_rng_probe,
+	.remove = st_rng_remove
+};
+
+module_platform_driver(st_rng_driver);
+
+MODULE_AUTHOR("Pankaj Dev <pankaj.dev@st.com>");
+MODULE_LICENSE("GPL v2");