[PATCH] Add Ingenic JZ4780 hardware RNG driver

[PATCH] Add Ingenic JZ4780 hardware RNG driver

[PrasannaKumar Muralidharan]

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");
-- 
2.5.0

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Daniel Thompson]

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");
>

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[PrasannaKumar Muralidharan]

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.

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Corentin LABBE]

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

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[PrasannaKumar Muralidharan]

> 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.

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[LABBE Corentin]

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Daniel Thompson]

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Rob Herring]

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

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Jeffrey Walton]

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

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[PrasannaKumar Muralidharan]

> 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?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[Jeffrey Walton]

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

Re: [PATCH] Add Ingenic JZ4780 hardware RNG driver

[PrasannaKumar Muralidharan]

> __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.