* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 15:35 [PATCH] Add Ingenic JZ4780 hardware RNG driver PrasannaKumar Muralidharan
@ 2016-08-17 16:01 ` Daniel Thompson
2016-08-17 16:13 ` PrasannaKumar Muralidharan
2016-08-17 19:06 ` Corentin LABBE
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Daniel Thompson @ 2016-08-17 16:01 UTC (permalink / raw)
To: PrasannaKumar Muralidharan, mpm, herbert, robh+dt, mark.rutland,
ralf, davem, geert, akpm, gregkh, mchehab, linux,
boris.brezillon, harvey.hunt, alex.smith, lee.jones, f.fainelli,
kieran, krzk, joshua.henderson, yendapally.reddy, narmstrong,
wangkefeng.wang, chunkeey, noltari, linus.walleij, pankaj.dev,
mathieu.poirier, linux-crypto, devicetree, linux-kernel,
linux-mips
On 17/08/16 16:35, PrasannaKumar Muralidharan wrote:
> This patch adds support for hardware random number generator present in
> JZ4780 SoC.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> ---
> .../devicetree/bindings/rng/ingenic,jz4780-rng.txt | 12 +++
> MAINTAINERS | 5 +
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 7 +-
> drivers/char/hw_random/Kconfig | 14 +++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/jz4780-rng.c | 105 +++++++++++++++++++++
> 6 files changed, 143 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> create mode 100644 drivers/char/hw_random/jz4780-rng.c
>
> diff --git a/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> new file mode 100644
> index 0000000..03abf56
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> @@ -0,0 +1,12 @@
> +Ingenic jz4780 RNG driver
> +
> +Required properties:
> +- compatible : Should be "ingenic,jz4780-rng"
> +- reg : Specifies base physical address and size of the registers.
> +
> +Example:
> +
> +rng: rng@100000D8 {
> + compatible = "ingenic,jz4780-rng";
> + reg = <0x100000D8 0x8>;
> +};
Device tree bindings should be a separate patch. See
Documentation/devicetree/bindings/submitting-patches.txt .
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08e9efe..c0c66eb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6002,6 +6002,11 @@ M: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> S: Maintained
> F: drivers/dma/dma-jz4780.c
>
> +INGENIC JZ4780 HW RNG Driver
> +M: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> +S: Maintained
> +F: drivers/char/hw_random/jz4780-rng.c
> +
> INTEGRITY MEASUREMENT ARCHITECTURE (IMA)
> M: Mimi Zohar <zohar@linux.vnet.ibm.com>
> M: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index b868b42..f11d139 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -36,7 +36,7 @@
>
> cgu: jz4780-cgu@10000000 {
> compatible = "ingenic,jz4780-cgu";
> - reg = <0x10000000 0x100>;
> + reg = <0x10000000 0xD8>;
> clocks = <&ext>, <&rtc>;
> clock-names = "ext", "rtc";
> @@ -44,6 +44,11 @@
> #clock-cells = <1>;
> };
>
> + rng: jz4780-rng@100000D8 {
> + compatible = "ingenic,jz4780-rng";
> + reg = <0x100000D8 0x8>;
> + };
> +
> uart0: serial@10030000 {
> compatible = "ingenic,jz4780-uart";
> reg = <0x10030000 0x100>;
Updates to device tree files are also normally sent as a separate patch
later in the series than the driver itself (at least they are in arm land).
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 56ad5a59..c336fe8 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -294,6 +294,20 @@ config HW_RANDOM_POWERNV
>
> If unsure, say Y.
>
> +config HW_RANDOM_JZ4780
> + tristate "JZ4780 HW random number generator support"
> + depends on MACH_INGENIC
> + depends on HAS_IOMEM
> + default HW_RANDOM
> + ---help---
> + This driver provides kernel-side support for the Random Number
> + Generator hardware found on JZ4780 SOCs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called jz4780-rng.
> +
> + If unsure, say Y.
> +
Shouldn't this be inserted either in alphabetic order (which not
applicable for this file) or at the end of the file?
> config HW_RANDOM_EXYNOS
> tristate "EXYNOS HW random number generator support"
> depends on ARCH_EXYNOS || COMPILE_TEST
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 04bb0b0..a155066 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
> obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
> obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
> obj-$(CONFIG_HW_RANDOM_HISI) += hisi-rng.o
> +obj-$(CONFIG_HW_RANDOM_JZ4780) += jz4780-rng.o
This looks like it inserts into the makefile in a gratuitously different
order than the one in the KConfig file.
> 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
> diff --git a/drivers/char/hw_random/jz4780-rng.c b/drivers/char/hw_random/jz4780-rng.c
> new file mode 100644
> index 0000000..c9d2cde
> --- /dev/null
> +++ b/drivers/char/hw_random/jz4780-rng.c
> @@ -0,0 +1,105 @@
> +/*
> + * jz4780-rng.c - Random Number Generator driver for J4780
> + *
> + * Copyright 2016 (C) PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/hw_random.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +
> +#define REG_RNG_CTRL 0x0
> +#define REG_RNG_DATA 0x4
> +
> +struct jz4780_rng {
> + struct device *dev;
> + struct hwrng rng;
> + void __iomem *mem;
> +};
> +
> +static u32 jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
> +{
> + return readl(rng->mem + offset);
> +}
> +
> +static void jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
> +{
> + writel(val, rng->mem + offset);
> +}
> +
> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
> + rng);
> + u32 *data = buf;
> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
> + return 4;
> +}
> +
> +static int jz4780_rng_probe(struct platform_device *pdev)
> +{
> + struct jz4780_rng *jz4780_rng;
> + struct resource *res;
> + resource_size_t size;
> + int ret;
> +
> + jz4780_rng = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_rng),
> + GFP_KERNEL);
> + if (!jz4780_rng)
> + return -ENOMEM;
> +
> + jz4780_rng->dev = &pdev->dev;
> + jz4780_rng->rng.name = "jz4780";
> + jz4780_rng->rng.read = jz4780_rng_read;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
If platform_get_resource() returns NULL I think this code will crash.
> + size = resource_size(res);
> +
> + jz4780_rng->mem = devm_ioremap(&pdev->dev, res->start, size);
> + if (IS_ERR(jz4780_rng->mem))
> + return PTR_ERR(jz4780_rng->mem);
> +
> + platform_set_drvdata(pdev, jz4780_rng);
> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
> + ret = hwrng_register(&jz4780_rng->rng);
> + return ret;
> +}
> +
> +static int jz4780_rng_remove(struct platform_device *pdev)
> +{
> + struct jz4780_rng *jz4780_rng = platform_get_drvdata(pdev);
> +
> + jz4780_rng_writel(jz4780_rng, 0, REG_RNG_CTRL);
> + hwrng_unregister(&jz4780_rng->rng);
These two are in same order as probe() function. I would expect them to
be reversed (so hardware cannot be shutdown until we have unregistered it).
> +
> + return 0;
> +}
> +
> +static const struct of_device_id jz4780_rng_dt_match[] = {
> + { .compatible = "ingenic,jz4780-rng", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_rng_dt_match);
> +
> +static struct platform_driver jz4780_rng_driver = {
> + .driver = {
> + .name = "jz4780-rng",
> + .of_match_table = jz4780_rng_dt_match,
> + },
> + .probe = jz4780_rng_probe,
> + .remove = jz4780_rng_remove,
> +};
> +module_platform_driver(jz4780_rng_driver);
> +
> +MODULE_DESCRIPTION("Ingenic JZ4780 H/W Random Number Generator driver");
> +MODULE_AUTHOR("PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 16:01 ` Daniel Thompson
@ 2016-08-17 16:13 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 12+ messages in thread
From: PrasannaKumar Muralidharan @ 2016-08-17 16:13 UTC (permalink / raw)
To: Daniel Thompson
Cc: mpm, herbert, robh+dt, mark.rutland, Ralf Baechle, davem, geert,
Andrew Morton, Greg KH, mchehab, linux, boris.brezillon,
harvey.hunt, alex.smith, lee.jones, Florian Fainelli, kieran,
krzk, joshua.henderson, yendapally.reddy, narmstrong,
wangkefeng.wang, chunkeey, noltari, linus.walleij, pankaj.dev,
mathieu.poirier, linux-crypto, devicetree, linux-kernel,
linux-mips
On 17 August 2016 at 21:31, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 17/08/16 16:35, PrasannaKumar Muralidharan wrote:
>>
>> This patch adds support for hardware random number generator present in
>> JZ4780 SoC.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>> ---
>> .../devicetree/bindings/rng/ingenic,jz4780-rng.txt | 12 +++
>> MAINTAINERS | 5 +
>> arch/mips/boot/dts/ingenic/jz4780.dtsi | 7 +-
>> drivers/char/hw_random/Kconfig | 14 +++
>> drivers/char/hw_random/Makefile | 1 +
>> drivers/char/hw_random/jz4780-rng.c | 105
>> +++++++++++++++++++++
>> 6 files changed, 143 insertions(+), 1 deletion(-)
>> create mode 100644
>> Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
>> create mode 100644 drivers/char/hw_random/jz4780-rng.c
>>
>> diff --git a/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
>> b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
>> new file mode 100644
>> index 0000000..03abf56
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
>> @@ -0,0 +1,12 @@
>> +Ingenic jz4780 RNG driver
>> +
>> +Required properties:
>> +- compatible : Should be "ingenic,jz4780-rng"
>> +- reg : Specifies base physical address and size of the registers.
>> +
>> +Example:
>> +
>> +rng: rng@100000D8 {
>> + compatible = "ingenic,jz4780-rng";
>> + reg = <0x100000D8 0x8>;
>> +};
>
>
> Device tree bindings should be a separate patch. See
> Documentation/devicetree/bindings/submitting-patches.txt .
Sure, Will submit it as a separate patch.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 08e9efe..c0c66eb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6002,6 +6002,11 @@ M: Zubair Lutfullah Kakakhel
>> <Zubair.Kakakhel@imgtec.com>
>> S: Maintained
>> F: drivers/dma/dma-jz4780.c
>>
>> +INGENIC JZ4780 HW RNG Driver
>> +M: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>> +S: Maintained
>> +F: drivers/char/hw_random/jz4780-rng.c
>> +
>> INTEGRITY MEASUREMENT ARCHITECTURE (IMA)
>> M: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> M: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index b868b42..f11d139 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -36,7 +36,7 @@
>>
>> cgu: jz4780-cgu@10000000 {
>> compatible = "ingenic,jz4780-cgu";
>> - reg = <0x10000000 0x100>;
>> + reg = <0x10000000 0xD8>;
>> clocks = <&ext>, <&rtc>;
>> clock-names = "ext", "rtc";
>> @@ -44,6 +44,11 @@
>> #clock-cells = <1>;
>> };
>>
>> + rng: jz4780-rng@100000D8 {
>> + compatible = "ingenic,jz4780-rng";
>> + reg = <0x100000D8 0x8>;
>> + };
>> +
>> uart0: serial@10030000 {
>> compatible = "ingenic,jz4780-uart";
>> reg = <0x10030000 0x100>;
>
>
> Updates to device tree files are also normally sent as a separate patch
> later in the series than the driver itself (at least they are in arm land).
Will follow the convention.
>> diff --git a/drivers/char/hw_random/Kconfig
>> b/drivers/char/hw_random/Kconfig
>> index 56ad5a59..c336fe8 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -294,6 +294,20 @@ config HW_RANDOM_POWERNV
>>
>> If unsure, say Y.
>>
>> +config HW_RANDOM_JZ4780
>> + tristate "JZ4780 HW random number generator support"
>> + depends on MACH_INGENIC
>> + depends on HAS_IOMEM
>> + default HW_RANDOM
>> + ---help---
>> + This driver provides kernel-side support for the Random Number
>> + Generator hardware found on JZ4780 SOCs.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called jz4780-rng.
>> +
>> + If unsure, say Y.
>> +
>
>
> Shouldn't this be inserted either in alphabetic order (which not applicable
> for this file) or at the end of the file?
I missed it. Will take into account while sending next revision.
>> config HW_RANDOM_EXYNOS
>> tristate "EXYNOS HW random number generator support"
>> depends on ARCH_EXYNOS || COMPILE_TEST
>> diff --git a/drivers/char/hw_random/Makefile
>> b/drivers/char/hw_random/Makefile
>> index 04bb0b0..a155066 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
>> obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
>> obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
>> obj-$(CONFIG_HW_RANDOM_HISI) += hisi-rng.o
>> +obj-$(CONFIG_HW_RANDOM_JZ4780) += jz4780-rng.o
>
>
> This looks like it inserts into the makefile in a gratuitously different
> order than the one in the KConfig file.
Will arrange both alphabetically. They will be in sync then.
>> 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
>> diff --git a/drivers/char/hw_random/jz4780-rng.c
>> b/drivers/char/hw_random/jz4780-rng.c
>> new file mode 100644
>> index 0000000..c9d2cde
>> --- /dev/null
>> +++ b/drivers/char/hw_random/jz4780-rng.c
>> @@ -0,0 +1,105 @@
>> +/*
>> + * jz4780-rng.c - Random Number Generator driver for J4780
>> + *
>> + * Copyright 2016 (C) PrasannaKumar Muralidharan
>> <prasannatsmkumar@gmail.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/hw_random.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/err.h>
>> +
>> +#define REG_RNG_CTRL 0x0
>> +#define REG_RNG_DATA 0x4
>> +
>> +struct jz4780_rng {
>> + struct device *dev;
>> + struct hwrng rng;
>> + void __iomem *mem;
>> +};
>> +
>> +static u32 jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
>> +{
>> + return readl(rng->mem + offset);
>> +}
>> +
>> +static void jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32
>> offset)
>> +{
>> + writel(val, rng->mem + offset);
>> +}
>> +
>> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool
>> wait)
>> +{
>> + struct jz4780_rng *jz4780_rng = container_of(rng, struct
>> jz4780_rng,
>> + rng);
>> + u32 *data = buf;
>> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
>> + return 4;
>> +}
>> +
>> +static int jz4780_rng_probe(struct platform_device *pdev)
>> +{
>> + struct jz4780_rng *jz4780_rng;
>> + struct resource *res;
>> + resource_size_t size;
>> + int ret;
>> +
>> + jz4780_rng = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_rng),
>> + GFP_KERNEL);
>> + if (!jz4780_rng)
>> + return -ENOMEM;
>> +
>> + jz4780_rng->dev = &pdev->dev;
>> + jz4780_rng->rng.name = "jz4780";
>> + jz4780_rng->rng.read = jz4780_rng_read;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
>
> If platform_get_resource() returns NULL I think this code will crash.
Missed it. Will fix the issue.
>> + size = resource_size(res);
>> +
>> + jz4780_rng->mem = devm_ioremap(&pdev->dev, res->start, size);
>> + if (IS_ERR(jz4780_rng->mem))
>> + return PTR_ERR(jz4780_rng->mem);
>> +
>> + platform_set_drvdata(pdev, jz4780_rng);
>> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
>> + ret = hwrng_register(&jz4780_rng->rng);
>> + return ret;
>> +}
>> +
>> +static int jz4780_rng_remove(struct platform_device *pdev)
>> +{
>> + struct jz4780_rng *jz4780_rng = platform_get_drvdata(pdev);
>> +
>> + jz4780_rng_writel(jz4780_rng, 0, REG_RNG_CTRL);
>> + hwrng_unregister(&jz4780_rng->rng);
>
>
> These two are in same order as probe() function. I would expect them to be
> reversed (so hardware cannot be shutdown until we have unregistered it).
True. I will change it.
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id jz4780_rng_dt_match[] = {
>> + { .compatible = "ingenic,jz4780-rng", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, jz4780_rng_dt_match);
>> +
>> +static struct platform_driver jz4780_rng_driver = {
>> + .driver = {
>> + .name = "jz4780-rng",
>> + .of_match_table = jz4780_rng_dt_match,
>> + },
>> + .probe = jz4780_rng_probe,
>> + .remove = jz4780_rng_remove,
>> +};
>> +module_platform_driver(jz4780_rng_driver);
>> +
>> +MODULE_DESCRIPTION("Ingenic JZ4780 H/W Random Number Generator driver");
>> +MODULE_AUTHOR("PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>");
>> +MODULE_LICENSE("GPL");
>>
>
Thanks for the review, appreciate your time.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 15:35 [PATCH] Add Ingenic JZ4780 hardware RNG driver PrasannaKumar Muralidharan
2016-08-17 16:01 ` Daniel Thompson
@ 2016-08-17 19:06 ` Corentin LABBE
2016-08-18 5:14 ` PrasannaKumar Muralidharan
2016-08-19 0:59 ` Rob Herring
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Corentin LABBE @ 2016-08-17 19:06 UTC (permalink / raw)
To: PrasannaKumar Muralidharan, mpm, herbert, robh+dt, mark.rutland,
ralf, davem, geert, akpm, gregkh, mchehab, linux,
boris.brezillon, harvey.hunt, alex.smith, daniel.thompson,
lee.jones, f.fainelli, kieran, krzk, joshua.henderson,
yendapally.reddy, narmstrong, wangkefeng.wang, chunkeey, noltari,
linus.walleij, pankaj.dev, mathieu.poirier, linux-crypto,
devicetree, linux-kernel, linux-mips
Hello
I have just some minor comments below
> diff --git a/drivers/char/hw_random/jz4780-rng.c b/drivers/char/hw_random/jz4780-rng.c
> new file mode 100644
> index 0000000..c9d2cde
> --- /dev/null
> +++ b/drivers/char/hw_random/jz4780-rng.c
> @@ -0,0 +1,105 @@
> +/*
> + * jz4780-rng.c - Random Number Generator driver for J4780
> + *
> + * Copyright 2016 (C) PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/hw_random.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
You could sort them by alphabetical order
> +
> +#define REG_RNG_CTRL 0x0
> +#define REG_RNG_DATA 0x4
> +
> +struct jz4780_rng {
> + struct device *dev;
> + struct hwrng rng;
> + void __iomem *mem;
> +};
> +
> +static u32 jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
> +{
> + return readl(rng->mem + offset);
> +}
> +
> +static void jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
> +{
> + writel(val, rng->mem + offset);
> +}
> +
> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
> + rng);
> + u32 *data = buf;
> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
> + return 4;
> +}
If max is less than 4, its bad
> +
> +static int jz4780_rng_probe(struct platform_device *pdev)
> +{
> + struct jz4780_rng *jz4780_rng;
> + struct resource *res;
> + resource_size_t size;
> + int ret;
> +
> + jz4780_rng = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_rng),
> + GFP_KERNEL);
You could write sizeof(*js480_rng)
> + if (!jz4780_rng)
> + return -ENOMEM;
> +
> + jz4780_rng->dev = &pdev->dev;
> + jz4780_rng->rng.name = "jz4780";
> + jz4780_rng->rng.read = jz4780_rng_read;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + size = resource_size(res);
> +
> + jz4780_rng->mem = devm_ioremap(&pdev->dev, res->start, size);
You could save code by using devm_ioremap_resource (don't need size)
> + if (IS_ERR(jz4780_rng->mem))
> + return PTR_ERR(jz4780_rng->mem);
> +
> + platform_set_drvdata(pdev, jz4780_rng);
> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
> + ret = hwrng_register(&jz4780_rng->rng);
> +
> + return ret;
> +}
You could write directly return hwrng_register(..)
Regards
LABBE Corentin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 19:06 ` Corentin LABBE
@ 2016-08-18 5:14 ` PrasannaKumar Muralidharan
2016-08-18 11:53 ` LABBE Corentin
0 siblings, 1 reply; 12+ messages in thread
From: PrasannaKumar Muralidharan @ 2016-08-18 5:14 UTC (permalink / raw)
To: Corentin LABBE
Cc: mpm, Herbert Xu, robh+dt, mark.rutland, Ralf Baechle, davem,
geert, Andrew Morton, Greg KH, mchehab, Guenter Roeck,
boris.brezillon, harvey.hunt, alex.smith, Daniel Thompson,
Lee Jones, Florian Fainelli, kieran, Krzysztof Kozlowski,
joshua.henderson, yendapally.reddy, narmstrong, wangkefeng.wang,
Christian Lamparter, Álvaro Fernández Rojas,
Linus Walleij, pankaj.dev, Mathieu Poirier, linux-crypto,
devicetree, linux-kernel, linux-mips
> I have just some minor comments below
Appreciate your review.
>> diff --git a/drivers/char/hw_random/jz4780-rng.c b/drivers/char/hw_random/jz4780-rng.c
>> new file mode 100644
>> index 0000000..c9d2cde
>> --- /dev/null
>> +++ b/drivers/char/hw_random/jz4780-rng.c
>> @@ -0,0 +1,105 @@
>> +/*
>> + * jz4780-rng.c - Random Number Generator driver for J4780
>> + *
>> + * Copyright 2016 (C) PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/hw_random.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/err.h>
>
> You could sort them by alphabetical order
Sure, will do.
>> +
>> +#define REG_RNG_CTRL 0x0
>> +#define REG_RNG_DATA 0x4
>> +
>> +struct jz4780_rng {
>> + struct device *dev;
>> + struct hwrng rng;
>> + void __iomem *mem;
>> +};
>> +
>> +static u32 jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
>> +{
>> + return readl(rng->mem + offset);
>> +}
>> +
>> +static void jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
>> +{
>> + writel(val, rng->mem + offset);
>> +}
>> +
>> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
>> +{
>> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
>> + rng);
>> + u32 *data = buf;
>> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
>> + return 4;
>> +}
>
> If max is less than 4, its bad
Data will be 4 bytes.
>> +
>> +static int jz4780_rng_probe(struct platform_device *pdev)
>> +{
>> + struct jz4780_rng *jz4780_rng;
>> + struct resource *res;
>> + resource_size_t size;
>> + int ret;
>> +
>> + jz4780_rng = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_rng),
>> + GFP_KERNEL);
>
> You could write sizeof(*js480_rng)
Will do.
>> + if (!jz4780_rng)
>> + return -ENOMEM;
>> +
>> + jz4780_rng->dev = &pdev->dev;
>> + jz4780_rng->rng.name = "jz4780";
>> + jz4780_rng->rng.read = jz4780_rng_read;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + size = resource_size(res);
>> +
>> + jz4780_rng->mem = devm_ioremap(&pdev->dev, res->start, size);
> You could save code by using devm_ioremap_resource (don't need size)
Will do.
>> + if (IS_ERR(jz4780_rng->mem))
>> + return PTR_ERR(jz4780_rng->mem);
>> +
>> + platform_set_drvdata(pdev, jz4780_rng);
>> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
>> + ret = hwrng_register(&jz4780_rng->rng);
>> +
>> + return ret;
>> +}
> You could write directly return hwrng_register(..)
Will do.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-18 5:14 ` PrasannaKumar Muralidharan
@ 2016-08-18 11:53 ` LABBE Corentin
2016-08-18 12:19 ` Daniel Thompson
0 siblings, 1 reply; 12+ messages in thread
From: LABBE Corentin @ 2016-08-18 11:53 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: mpm, Herbert Xu, robh+dt, mark.rutland, Ralf Baechle, davem,
geert, Andrew Morton, Greg KH, mchehab, Guenter Roeck,
boris.brezillon, harvey.hunt, alex.smith, Daniel Thompson,
Lee Jones, Florian Fainelli, kieran, Krzysztof Kozlowski,
joshua.henderson, yendapally.reddy, narmstrong, wangkefeng.wang,
Christian Lamparter, Álvaro Fernández Rojas,
Linus Walleij, pankaj.dev, Mathieu Poirier, linux-crypto,
devicetree, linux-kernel, linux-mips
On Thu, Aug 18, 2016 at 10:44:18AM +0530, PrasannaKumar Muralidharan wrote:
> >> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> >> +{
> >> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
> >> + rng);
> >> + u32 *data = buf;
> >> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
> >> + return 4;
> >> +}
> >
> > If max is less than 4, its bad
>
> Data will be 4 bytes.
>
No, according to comment in include/linux/hw_random.h "drivers can fill up to max bytes of data"
So you cannot write more than max bytes without risking buffer overflow.
And if max > 4, hwrng client need to recall your read function.
The better example I found is tpm_get_random() in drivers/char/tpm/tpm-interface.c for handling both problem.
Regards
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-18 11:53 ` LABBE Corentin
@ 2016-08-18 12:19 ` Daniel Thompson
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Thompson @ 2016-08-18 12:19 UTC (permalink / raw)
To: LABBE Corentin, PrasannaKumar Muralidharan
Cc: mpm, Herbert Xu, robh+dt, mark.rutland, Ralf Baechle, davem,
geert, Andrew Morton, Greg KH, mchehab, Guenter Roeck,
boris.brezillon, harvey.hunt, alex.smith, Lee Jones,
Florian Fainelli, kieran, Krzysztof Kozlowski, joshua.henderson,
yendapally.reddy, narmstrong, wangkefeng.wang,
Christian Lamparter, Álvaro Fernández Rojas,
Linus Walleij, pankaj.dev, Mathieu Poirier, linux-crypto,
devicetree, linux-kernel, linux-mips
On 18/08/16 12:53, LABBE Corentin wrote:
> On Thu, Aug 18, 2016 at 10:44:18AM +0530, PrasannaKumar Muralidharan wrote:
>>>> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
>>>> +{
>>>> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
>>>> + rng);
>>>> + u32 *data = buf;
>>>> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
>>>> + return 4;
>>>> +}
>>>
>>> If max is less than 4, its bad
>>
>> Data will be 4 bytes.
>>
>
> No, according to comment in include/linux/hw_random.h "drivers can fill up to max bytes of data"
> So you cannot write more than max bytes without risking buffer overflow.
>
> And if max > 4, hwrng client need to recall your read function.
> The better example I found is tpm_get_random() in drivers/char/tpm/tpm-interface.c for handling both problem.
Right now the core code will never actually ask a RNG driver for <4
bytes so perhaps it would be better to update the comment in
include/linux/hw_random.h !
For devices with 32-bit RNG registers the extra code to handle a special
case that doesn't actually exist is a waste.
There are 14 drivers in drivers/char/hw_random that support the ->read()
interface but only three of these actually support max == 1 (existing
accepted behavior varies between return 0, return 2, return 4 and return
-EIO).
Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 15:35 [PATCH] Add Ingenic JZ4780 hardware RNG driver PrasannaKumar Muralidharan
2016-08-17 16:01 ` Daniel Thompson
2016-08-17 19:06 ` Corentin LABBE
@ 2016-08-19 0:59 ` Rob Herring
2016-08-19 9:47 ` Jeffrey Walton
2016-08-19 10:55 ` Jeffrey Walton
4 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2016-08-19 0:59 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: mpm, herbert, mark.rutland, ralf, davem, geert, akpm, gregkh,
mchehab, linux, boris.brezillon, harvey.hunt, alex.smith,
daniel.thompson, lee.jones, f.fainelli, kieran, krzk,
joshua.henderson, yendapally.reddy, narmstrong, wangkefeng.wang,
chunkeey, noltari, linus.walleij, pankaj.dev, mathieu.poirier,
linux-crypto, devicetree, linux-kernel, linux-mips
On Wed, Aug 17, 2016 at 09:05:51PM +0530, PrasannaKumar Muralidharan wrote:
> This patch adds support for hardware random number generator present in
> JZ4780 SoC.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> ---
> .../devicetree/bindings/rng/ingenic,jz4780-rng.txt | 12 +++
Acked-by: Rob Herring <robh@kernel.org>
> MAINTAINERS | 5 +
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 7 +-
> drivers/char/hw_random/Kconfig | 14 +++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/jz4780-rng.c | 105 +++++++++++++++++++++
> 6 files changed, 143 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> create mode 100644 drivers/char/hw_random/jz4780-rng.c
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 15:35 [PATCH] Add Ingenic JZ4780 hardware RNG driver PrasannaKumar Muralidharan
` (2 preceding siblings ...)
2016-08-19 0:59 ` Rob Herring
@ 2016-08-19 9:47 ` Jeffrey Walton
2016-08-19 12:54 ` PrasannaKumar Muralidharan
2016-08-19 10:55 ` Jeffrey Walton
4 siblings, 1 reply; 12+ messages in thread
From: Jeffrey Walton @ 2016-08-19 9:47 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: linux-crypto, devicetree, linux-kernel, linux-mips
On Wed, Aug 17, 2016 at 11:35 AM, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> This patch adds support for hardware random number generator present in
> JZ4780 SoC.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> ---
> .../devicetree/bindings/rng/ingenic,jz4780-rng.txt | 12 +++
> MAINTAINERS | 5 +
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 7 +-
> drivers/char/hw_random/Kconfig | 14 +++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/jz4780-rng.c | 105 +++++++++++++++++++++
> 6 files changed, 143 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> create mode 100644 drivers/char/hw_random/jz4780-rng.c
>
> diff --git a/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> new file mode 100644
> index 0000000..03abf56
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
> @@ -0,0 +1,12 @@
> +Ingenic jz4780 RNG driver
> +
> +Required properties:
> +- compatible : Should be "ingenic,jz4780-rng"
> +- reg : Specifies base physical address and size of the registers.
> +
> +Example:
> +
> +rng: rng@100000D8 {
> + compatible = "ingenic,jz4780-rng";
> + reg = <0x100000D8 0x8>;
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08e9efe..c0c66eb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6002,6 +6002,11 @@ M: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> S: Maintained
> F: drivers/dma/dma-jz4780.c
>
> +INGENIC JZ4780 HW RNG Driver
> +M: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> +S: Maintained
> +F: drivers/char/hw_random/jz4780-rng.c
> +
> INTEGRITY MEASUREMENT ARCHITECTURE (IMA)
> M: Mimi Zohar <zohar@linux.vnet.ibm.com>
> M: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index b868b42..f11d139 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -36,7 +36,7 @@
>
> cgu: jz4780-cgu@10000000 {
> compatible = "ingenic,jz4780-cgu";
> - reg = <0x10000000 0x100>;
> + reg = <0x10000000 0xD8>;
>
> clocks = <&ext>, <&rtc>;
> clock-names = "ext", "rtc";
> @@ -44,6 +44,11 @@
> #clock-cells = <1>;
> };
>
> + rng: jz4780-rng@100000D8 {
> + compatible = "ingenic,jz4780-rng";
> + reg = <0x100000D8 0x8>;
> + };
> +
> uart0: serial@10030000 {
> compatible = "ingenic,jz4780-uart";
> reg = <0x10030000 0x100>;
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 56ad5a59..c336fe8 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -294,6 +294,20 @@ config HW_RANDOM_POWERNV
>
> If unsure, say Y.
>
> +config HW_RANDOM_JZ4780
> + tristate "JZ4780 HW random number generator support"
> + depends on MACH_INGENIC
> + depends on HAS_IOMEM
> + default HW_RANDOM
> + ---help---
> + This driver provides kernel-side support for the Random Number
> + Generator hardware found on JZ4780 SOCs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called jz4780-rng.
> +
> + If unsure, say Y.
> +
> config HW_RANDOM_EXYNOS
> tristate "EXYNOS HW random number generator support"
> depends on ARCH_EXYNOS || COMPILE_TEST
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 04bb0b0..a155066 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
> obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
> obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
> obj-$(CONFIG_HW_RANDOM_HISI) += hisi-rng.o
> +obj-$(CONFIG_HW_RANDOM_JZ4780) += jz4780-rng.o
> 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
> diff --git a/drivers/char/hw_random/jz4780-rng.c b/drivers/char/hw_random/jz4780-rng.c
> new file mode 100644
> index 0000000..c9d2cde
> --- /dev/null
> +++ b/drivers/char/hw_random/jz4780-rng.c
> @@ -0,0 +1,105 @@
> +/*
> + * jz4780-rng.c - Random Number Generator driver for J4780
> + *
> + * Copyright 2016 (C) PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/hw_random.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +
> +#define REG_RNG_CTRL 0x0
> +#define REG_RNG_DATA 0x4
> +
> +struct jz4780_rng {
> + struct device *dev;
> + struct hwrng rng;
> + void __iomem *mem;
> +};
> +
> +static u32 jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
> +{
> + return readl(rng->mem + offset);
> +}
> +
> +static void jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
> +{
> + writel(val, rng->mem + offset);
> +}
> +
> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
> + rng);
> + u32 *data = buf;
> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
> + return 4;
> +}
> +
> +static int jz4780_rng_probe(struct platform_device *pdev)
> +{
> + struct jz4780_rng *jz4780_rng;
> + struct resource *res;
> + resource_size_t size;
> + int ret;
> +
> + jz4780_rng = devm_kzalloc(&pdev->dev, sizeof(struct jz4780_rng),
> + GFP_KERNEL);
> + if (!jz4780_rng)
> + return -ENOMEM;
> +
> + jz4780_rng->dev = &pdev->dev;
> + jz4780_rng->rng.name = "jz4780";
> + jz4780_rng->rng.read = jz4780_rng_read;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + size = resource_size(res);
> +
> + jz4780_rng->mem = devm_ioremap(&pdev->dev, res->start, size);
> + if (IS_ERR(jz4780_rng->mem))
> + return PTR_ERR(jz4780_rng->mem);
> +
> + platform_set_drvdata(pdev, jz4780_rng);
> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
> + ret = hwrng_register(&jz4780_rng->rng);
> +
> + return ret;
> +}
> +
> +static int jz4780_rng_remove(struct platform_device *pdev)
> +{
> + struct jz4780_rng *jz4780_rng = platform_get_drvdata(pdev);
> +
> + jz4780_rng_writel(jz4780_rng, 0, REG_RNG_CTRL);
> + hwrng_unregister(&jz4780_rng->rng);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id jz4780_rng_dt_match[] = {
> + { .compatible = "ingenic,jz4780-rng", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_rng_dt_match);
> +
> +static struct platform_driver jz4780_rng_driver = {
> + .driver = {
> + .name = "jz4780-rng",
> + .of_match_table = jz4780_rng_dt_match,
> + },
> + .probe = jz4780_rng_probe,
> + .remove = jz4780_rng_remove,
> +};
> +module_platform_driver(jz4780_rng_driver);
> +
> +MODULE_DESCRIPTION("Ingenic JZ4780 H/W Random Number Generator driver");
> +MODULE_AUTHOR("PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>");
> +MODULE_LICENSE("GPL");
> --
Please forgive my ignorance Prasanna...
For the JZ4780 I have, there are two registers in play. The first is
the control register which enables/disables the RNG. The control
register is named ERNG. The second register is the data register, and
it produces the random stream. The data register is named RNG. ERNG is
located at 0x100000D8 and RNG is located at 0x100000DC. This kind of
confuses me because I don't see where 0x100000D8 is ever added to
those values (maybe its in the descriptor?):
+#define REG_RNG_CTRL 0x0
+#define REG_RNG_DATA 0x4
Also, testing with a userland PoC for the device, you have to throttle
reads from RNG register. If reads occur with a 0 delay, then the
random value appears fixed. If the delay is too small, then you can
watch random values being shifted-in in a barrel like fashion.
Unfortunately, the manual did not discuss how long to wait for a value
to be ready. I found spinning in a loop for 5000 was too small and
witnessed the shifting; while spinning in a loop for 10000 avoided the
shift observation. I don't what number of JIFFIES that translates to.
Finally, from looking at the native Ingenic driver (which was not very
impressive), they enabled/disabled the RNG register on demand. There
was also a [possible related] note in the manual about not applying
VCC for over a second. I can only say "possibly related" because I was
not sure if the register was part of the controller they were
discussing. The userland PoC worked fine when enabling/disabling the
RNG register. So I'm not sure about this (from jz4780_rng_probe):
+ platform_set_drvdata(pdev, jz4780_rng);
+ jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
+ ret = hwrng_register(&jz4780_rng->rng);
And this (from jz4780_rng_remove):
+ jz4780_rng_writel(jz4780_rng, 0, REG_RNG_CTRL);
+ hwrng_unregister(&jz4780_rng->rng);
Anyway, I hope that helps you avoid some land mines (if they are present).
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-19 9:47 ` Jeffrey Walton
@ 2016-08-19 12:54 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 12+ messages in thread
From: PrasannaKumar Muralidharan @ 2016-08-19 12:54 UTC (permalink / raw)
To: noloader; +Cc: linux-crypto, devicetree, linux-kernel, linux-mips
> Please forgive my ignorance Prasanna...
>
> For the JZ4780 I have, there are two registers in play. The first is
> the control register which enables/disables the RNG. The control
> register is named ERNG. The second register is the data register, and
> it produces the random stream. The data register is named RNG. ERNG is
> located at 0x100000D8 and RNG is located at 0x100000DC. This kind of
> confuses me because I don't see where 0x100000D8 is ever added to
> those values (maybe its in the descriptor?):
>
> +#define REG_RNG_CTRL 0x0
> +#define REG_RNG_DATA 0x4
The base address 0x100000D8 is defined in jz4780.dtsi file.
REG_RNG_CTRL and REG_RNG_DATA are offsets. In jz4780_rng_readl and
jz4780_rng_writel functions the ioremap'd base address is added with
offset.
> Also, testing with a userland PoC for the device, you have to throttle
> reads from RNG register. If reads occur with a 0 delay, then the
> random value appears fixed. If the delay is too small, then you can
> watch random values being shifted-in in a barrel like fashion.
> Unfortunately, the manual did not discuss how long to wait for a value
> to be ready. I found spinning in a loop for 5000 was too small and
> witnessed the shifting; while spinning in a loop for 10000 avoided the
> shift observation. I don't what number of JIFFIES that translates to.
I can calculate the speed and make sure the delay is met in the
driver. Thanks a lot for providing this info.
> Finally, from looking at the native Ingenic driver (which was not very
> impressive), they enabled/disabled the RNG register on demand. There
> was also a [possible related] note in the manual about not applying
> VCC for over a second. I can only say "possibly related" because I was
> not sure if the register was part of the controller they were
> discussing. The userland PoC worked fine when enabling/disabling the
> RNG register. So I'm not sure about this (from jz4780_rng_probe):
>
> + platform_set_drvdata(pdev, jz4780_rng);
> + jz4780_rng_writel(jz4780_rng, 1, REG_RNG_CTRL);
> + ret = hwrng_register(&jz4780_rng->rng);
>
> And this (from jz4780_rng_remove):
>
> + jz4780_rng_writel(jz4780_rng, 0, REG_RNG_CTRL);
> + hwrng_unregister(&jz4780_rng->rng);
>
> Anyway, I hope that helps you avoid some land mines (if they are present).
Looking at JZ4780 Programmers manual I could not find any info about
VCC. Can you point me to it?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-17 15:35 [PATCH] Add Ingenic JZ4780 hardware RNG driver PrasannaKumar Muralidharan
` (3 preceding siblings ...)
2016-08-19 9:47 ` Jeffrey Walton
@ 2016-08-19 10:55 ` Jeffrey Walton
2016-08-19 13:09 ` PrasannaKumar Muralidharan
4 siblings, 1 reply; 12+ messages in thread
From: Jeffrey Walton @ 2016-08-19 10:55 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: linux-crypto, devicetree, linux-kernel, linux-mips
On Wed, Aug 17, 2016 at 11:35 AM, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> This patch adds support for hardware random number generator present in
> JZ4780 SoC.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> ---
> ...
> +static int jz4780_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct jz4780_rng *jz4780_rng = container_of(rng, struct jz4780_rng,
> + rng);
> + u32 *data = buf;
> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
> + return 4;
> +}
My bad, I should have spotted this earlier....
i686, x86_64 and some ARM will sometimes define a macro indicating
unaligned data access is allowed. For example, see
__ARM_FEATURE_UNALIGNED (cf.,
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0774f/chr1383660321827.html)
. MIPSEL does not define such a macro.
# MIPS ci20 creator with GCC 4.6
$ gcc -march=native -dM -E - </dev/null | grep -i align
#define __BIGGEST_ALIGNMENT__ 8
If the MIPS CPU does not tolerate unaligned data access, then the
following could SIGBUS:
> + u32 *data = buf;
> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
If GCC emits code that uses the MIPS unaligned load and store
instructions, then there's probably going to be a performance penalty.
Regardless of what the CPU tolerates, I believe unaligned data access
is undefined behavior in C/C++. I believe you should memcpy the value
into the buffer.
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver
2016-08-19 10:55 ` Jeffrey Walton
@ 2016-08-19 13:09 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 12+ messages in thread
From: PrasannaKumar Muralidharan @ 2016-08-19 13:09 UTC (permalink / raw)
To: noloader; +Cc: linux-crypto, devicetree, linux-kernel, linux-mips
> __ARM_FEATURE_UNALIGNED (cf.,
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0774f/chr1383660321827.html)
> . MIPSEL does not define such a macro.
>
> # MIPS ci20 creator with GCC 4.6
> $ gcc -march=native -dM -E - </dev/null | grep -i align
> #define __BIGGEST_ALIGNMENT__ 8
>
> If the MIPS CPU does not tolerate unaligned data access, then the
> following could SIGBUS:
>
>> + u32 *data = buf;
>> + *data = jz4780_rng_readl(jz4780_rng, REG_RNG_DATA);
>
> If GCC emits code that uses the MIPS unaligned load and store
> instructions, then there's probably going to be a performance penalty.
>
> Regardless of what the CPU tolerates, I believe unaligned data access
> is undefined behavior in C/C++. I believe you should memcpy the value
> into the buffer.
I am not sure whether this is required. 'buf' is part of a structure
so I guess it is properly aligned by padding. Not sure though, will
look about this.
^ permalink raw reply [flat|nested] 12+ messages in thread