All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-24 22:21 ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-24 22:21 UTC (permalink / raw)
  To: u-boot

Add support for the hardware random number generator of Amlogic SOCs.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/rng/Kconfig     |   8 +++
 drivers/rng/Makefile    |   1 +
 drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/rng/meson-rng.c

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index c1aa43b823..edb6152bb9 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -8,6 +8,14 @@ config DM_RNG

 if DM_RNG

+config RNG_MESON
+	bool "Amlogic Meson Random Number Generator support"
+	depends on ARCH_MESON
+	default y
+	help
+	  Enable support for hardware random number generator
+	  of Amlogic Meson SoCs.
+
 config RNG_SANDBOX
 	bool "Sandbox random number generator"
 	depends on SANDBOX
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 3517005541..6a8a66779b 100644
--- a/drivers/rng/Makefile
+++ b/drivers/rng/Makefile
@@ -4,5 +4,6 @@
 #

 obj-$(CONFIG_DM_RNG) += rng-uclass.o
+obj-$(CONFIG_RNG_MESON) += meson-rng.o
 obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
 obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
new file mode 100644
index 0000000000..4b81a62353
--- /dev/null
+++ b/drivers/rng/meson-rng.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * Driver for Amlogic hardware random number generator
+ */
+
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <rng.h>
+#include <asm/io.h>
+
+struct meson_rng_platdata {
+	fdt_addr_t base;
+	struct clk clk;
+};
+
+/**
+ * meson_rng_read() - fill buffer with random bytes
+ *
+ * @buffer:	buffer to receive data
+ * @size:	size of buffer
+ *
+ * Return:	0
+ */
+static int meson_rng_read(struct udevice *dev, void *data, size_t len)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	char *buffer = (char *)data;
+
+	while (len) {
+		u32 rand = readl(pdata->base);
+		size_t step;
+
+		if (len >= 4)
+			step = 4;
+		else
+			step = len;
+		memcpy(buffer, &rand, step);
+		buffer += step;
+		len -= step;
+	}
+	return 0;
+}
+
+/**
+ * meson_rng_probe() - probe rng device
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_probe(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	int err;
+
+	err = clk_enable(&pdata->clk);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/**
+ * meson_rng_remove() - deinitialize rng device
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_remove(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+
+	return clk_disable(&pdata->clk);
+}
+
+/**
+ * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform data
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_ofdata_to_platdata(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	int err;
+
+	pdata->base = dev_read_addr(dev);
+	if (!pdata->base)
+		return -ENODEV;
+
+	err = clk_get_by_name(dev, "core", &pdata->clk);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static const struct dm_rng_ops meson_rng_ops = {
+	.read = meson_rng_read,
+};
+
+static const struct udevice_id meson_rng_match[] = {
+	{
+		.compatible = "amlogic,meson-rng",
+	},
+	{},
+};
+
+U_BOOT_DRIVER(meson_rng) = {
+	.name = "meson-rng",
+	.id = UCLASS_RNG,
+	.of_match = meson_rng_match,
+	.ops = &meson_rng_ops,
+	.probe = meson_rng_probe,
+	.remove = meson_rng_remove,
+	.platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
+	.ofdata_to_platdata = meson_rng_ofdata_to_platdata,
+};
--
2.20.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-24 22:21 ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-24 22:21 UTC (permalink / raw)
  To: Sughosh Ganu; +Cc: Neil Armstrong, u-boot, u-boot-amlogic, Heinrich Schuchardt

Add support for the hardware random number generator of Amlogic SOCs.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/rng/Kconfig     |   8 +++
 drivers/rng/Makefile    |   1 +
 drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drivers/rng/meson-rng.c

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index c1aa43b823..edb6152bb9 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -8,6 +8,14 @@ config DM_RNG

 if DM_RNG

+config RNG_MESON
+	bool "Amlogic Meson Random Number Generator support"
+	depends on ARCH_MESON
+	default y
+	help
+	  Enable support for hardware random number generator
+	  of Amlogic Meson SoCs.
+
 config RNG_SANDBOX
 	bool "Sandbox random number generator"
 	depends on SANDBOX
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 3517005541..6a8a66779b 100644
--- a/drivers/rng/Makefile
+++ b/drivers/rng/Makefile
@@ -4,5 +4,6 @@
 #

 obj-$(CONFIG_DM_RNG) += rng-uclass.o
+obj-$(CONFIG_RNG_MESON) += meson-rng.o
 obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
 obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
new file mode 100644
index 0000000000..4b81a62353
--- /dev/null
+++ b/drivers/rng/meson-rng.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * Driver for Amlogic hardware random number generator
+ */
+
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <rng.h>
+#include <asm/io.h>
+
+struct meson_rng_platdata {
+	fdt_addr_t base;
+	struct clk clk;
+};
+
+/**
+ * meson_rng_read() - fill buffer with random bytes
+ *
+ * @buffer:	buffer to receive data
+ * @size:	size of buffer
+ *
+ * Return:	0
+ */
+static int meson_rng_read(struct udevice *dev, void *data, size_t len)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	char *buffer = (char *)data;
+
+	while (len) {
+		u32 rand = readl(pdata->base);
+		size_t step;
+
+		if (len >= 4)
+			step = 4;
+		else
+			step = len;
+		memcpy(buffer, &rand, step);
+		buffer += step;
+		len -= step;
+	}
+	return 0;
+}
+
+/**
+ * meson_rng_probe() - probe rng device
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_probe(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	int err;
+
+	err = clk_enable(&pdata->clk);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+/**
+ * meson_rng_remove() - deinitialize rng device
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_remove(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+
+	return clk_disable(&pdata->clk);
+}
+
+/**
+ * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform data
+ *
+ * @dev:	device
+ * Return:	0 if ok
+ */
+static int meson_rng_ofdata_to_platdata(struct udevice *dev)
+{
+	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
+	int err;
+
+	pdata->base = dev_read_addr(dev);
+	if (!pdata->base)
+		return -ENODEV;
+
+	err = clk_get_by_name(dev, "core", &pdata->clk);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static const struct dm_rng_ops meson_rng_ops = {
+	.read = meson_rng_read,
+};
+
+static const struct udevice_id meson_rng_match[] = {
+	{
+		.compatible = "amlogic,meson-rng",
+	},
+	{},
+};
+
+U_BOOT_DRIVER(meson_rng) = {
+	.name = "meson-rng",
+	.id = UCLASS_RNG,
+	.of_match = meson_rng_match,
+	.ops = &meson_rng_ops,
+	.probe = meson_rng_probe,
+	.remove = meson_rng_remove,
+	.platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
+	.ofdata_to_platdata = meson_rng_ofdata_to_platdata,
+};
--
2.20.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-24 22:21 ` Heinrich Schuchardt
@ 2020-02-26 10:19   ` Neil Armstrong
  -1 siblings, 0 replies; 14+ messages in thread
From: Neil Armstrong @ 2020-02-26 10:19 UTC (permalink / raw)
  To: u-boot

Hi,

Le 24/02/2020 ? 23:21, Heinrich Schuchardt a ?crit :
> Add support for the hardware random number generator of Amlogic SOCs.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/rng/Kconfig     |   8 +++
>  drivers/rng/Makefile    |   1 +
>  drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/rng/meson-rng.c
> 
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c1aa43b823..edb6152bb9 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -8,6 +8,14 @@ config DM_RNG
> 
>  if DM_RNG
> 
> +config RNG_MESON
> +	bool "Amlogic Meson Random Number Generator support"
> +	depends on ARCH_MESON
> +	default y
> +	help
> +	  Enable support for hardware random number generator
> +	  of Amlogic Meson SoCs.
> +
>  config RNG_SANDBOX
>  	bool "Sandbox random number generator"
>  	depends on SANDBOX
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 3517005541..6a8a66779b 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -4,5 +4,6 @@
>  #
> 
>  obj-$(CONFIG_DM_RNG) += rng-uclass.o
> +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>  obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> new file mode 100644
> index 0000000000..4b81a62353
> --- /dev/null
> +++ b/drivers/rng/meson-rng.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * Driver for Amlogic hardware random number generator
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <asm/io.h>
> +
> +struct meson_rng_platdata {
> +	fdt_addr_t base;
> +	struct clk clk;
> +};
> +
> +/**
> + * meson_rng_read() - fill buffer with random bytes
> + *
> + * @buffer:	buffer to receive data
> + * @size:	size of buffer
> + *
> + * Return:	0
> + */
> +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	char *buffer = (char *)data;
> +
> +	while (len) {
> +		u32 rand = readl(pdata->base);
> +		size_t step;
> +
> +		if (len >= 4)
> +			step = 4;
> +		else
> +			step = len;
> +		memcpy(buffer, &rand, step);
> +		buffer += step;
> +		len -= step;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * meson_rng_probe() - probe rng device
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_probe(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	int err;
> +
> +	err = clk_enable(&pdata->clk);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +/**
> + * meson_rng_remove() - deinitialize rng device
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_remove(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	return clk_disable(&pdata->clk);
> +}
> +
> +/**
> + * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform data
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	int err;
> +
> +	pdata->base = dev_read_addr(dev);
> +	if (!pdata->base)
> +		return -ENODEV;
> +
> +	err = clk_get_by_name(dev, "core", &pdata->clk);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops meson_rng_ops = {
> +	.read = meson_rng_read,
> +};
> +
> +static const struct udevice_id meson_rng_match[] = {
> +	{
> +		.compatible = "amlogic,meson-rng",
> +	},
> +	{},
> +};
> +
> +U_BOOT_DRIVER(meson_rng) = {
> +	.name = "meson-rng",
> +	.id = UCLASS_RNG,
> +	.of_match = meson_rng_match,
> +	.ops = &meson_rng_ops,
> +	.probe = meson_rng_probe,
> +	.remove = meson_rng_remove,
> +	.platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
> +	.ofdata_to_platdata = meson_rng_ofdata_to_platdata,
> +};
> --
> 2.20.1
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks, looks fine.

I suspect this will help adding EFI RNG data for Linux early random seed, right ?

Is there a RNG maintainer or should I apply it myself ?

Neil

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 10:19   ` Neil Armstrong
  0 siblings, 0 replies; 14+ messages in thread
From: Neil Armstrong @ 2020-02-26 10:19 UTC (permalink / raw)
  To: Heinrich Schuchardt, Sughosh Ganu; +Cc: u-boot, u-boot-amlogic

Hi,

Le 24/02/2020 à 23:21, Heinrich Schuchardt a écrit :
> Add support for the hardware random number generator of Amlogic SOCs.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/rng/Kconfig     |   8 +++
>  drivers/rng/Makefile    |   1 +
>  drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/rng/meson-rng.c
> 
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c1aa43b823..edb6152bb9 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -8,6 +8,14 @@ config DM_RNG
> 
>  if DM_RNG
> 
> +config RNG_MESON
> +	bool "Amlogic Meson Random Number Generator support"
> +	depends on ARCH_MESON
> +	default y
> +	help
> +	  Enable support for hardware random number generator
> +	  of Amlogic Meson SoCs.
> +
>  config RNG_SANDBOX
>  	bool "Sandbox random number generator"
>  	depends on SANDBOX
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 3517005541..6a8a66779b 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -4,5 +4,6 @@
>  #
> 
>  obj-$(CONFIG_DM_RNG) += rng-uclass.o
> +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>  obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> new file mode 100644
> index 0000000000..4b81a62353
> --- /dev/null
> +++ b/drivers/rng/meson-rng.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * Driver for Amlogic hardware random number generator
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <asm/io.h>
> +
> +struct meson_rng_platdata {
> +	fdt_addr_t base;
> +	struct clk clk;
> +};
> +
> +/**
> + * meson_rng_read() - fill buffer with random bytes
> + *
> + * @buffer:	buffer to receive data
> + * @size:	size of buffer
> + *
> + * Return:	0
> + */
> +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	char *buffer = (char *)data;
> +
> +	while (len) {
> +		u32 rand = readl(pdata->base);
> +		size_t step;
> +
> +		if (len >= 4)
> +			step = 4;
> +		else
> +			step = len;
> +		memcpy(buffer, &rand, step);
> +		buffer += step;
> +		len -= step;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * meson_rng_probe() - probe rng device
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_probe(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	int err;
> +
> +	err = clk_enable(&pdata->clk);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +/**
> + * meson_rng_remove() - deinitialize rng device
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_remove(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	return clk_disable(&pdata->clk);
> +}
> +
> +/**
> + * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform data
> + *
> + * @dev:	device
> + * Return:	0 if ok
> + */
> +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +	int err;
> +
> +	pdata->base = dev_read_addr(dev);
> +	if (!pdata->base)
> +		return -ENODEV;
> +
> +	err = clk_get_by_name(dev, "core", &pdata->clk);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops meson_rng_ops = {
> +	.read = meson_rng_read,
> +};
> +
> +static const struct udevice_id meson_rng_match[] = {
> +	{
> +		.compatible = "amlogic,meson-rng",
> +	},
> +	{},
> +};
> +
> +U_BOOT_DRIVER(meson_rng) = {
> +	.name = "meson-rng",
> +	.id = UCLASS_RNG,
> +	.of_match = meson_rng_match,
> +	.ops = &meson_rng_ops,
> +	.probe = meson_rng_probe,
> +	.remove = meson_rng_remove,
> +	.platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
> +	.ofdata_to_platdata = meson_rng_ofdata_to_platdata,
> +};
> --
> 2.20.1
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks, looks fine.

I suspect this will help adding EFI RNG data for Linux early random seed, right ?

Is there a RNG maintainer or should I apply it myself ?

Neil

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-26 10:19   ` Neil Armstrong
@ 2020-02-26 10:38     ` Heinrich Schuchardt
  -1 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 10:38 UTC (permalink / raw)
  To: u-boot



On 2/26/20 11:19 AM, Neil Armstrong wrote:
> Hi,
>
> Le 24/02/2020 ? 23:21, Heinrich Schuchardt a ?crit :
>> Add support for the hardware random number generator of Amlogic SOCs.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
<snip />
>>
>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks for reviewing.

>
> Thanks, looks fine.
>
> I suspect this will help adding EFI RNG data for Linux early random seed, right ?

Yes, we need this for the EFI_RNG_PROTOCOL.

>
> Is there a RNG maintainer or should I apply it myself ?


Sughosh is the maintainer for RNG.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 10:38     ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 10:38 UTC (permalink / raw)
  To: Neil Armstrong, Sughosh Ganu; +Cc: u-boot, u-boot-amlogic



On 2/26/20 11:19 AM, Neil Armstrong wrote:
> Hi,
>
> Le 24/02/2020 à 23:21, Heinrich Schuchardt a écrit :
>> Add support for the hardware random number generator of Amlogic SOCs.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
<snip />
>>
>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks for reviewing.

>
> Thanks, looks fine.
>
> I suspect this will help adding EFI RNG data for Linux early random seed, right ?

Yes, we need this for the EFI_RNG_PROTOCOL.

>
> Is there a RNG maintainer or should I apply it myself ?


Sughosh is the maintainer for RNG.


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-24 22:21 ` Heinrich Schuchardt
@ 2020-02-26 10:51   ` Sughosh Ganu
  -1 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2020-02-26 10:51 UTC (permalink / raw)
  To: u-boot

On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> Add support for the hardware random number generator of Amlogic SOCs.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/rng/Kconfig     |   8 +++
>  drivers/rng/Makefile    |   1 +
>  drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/rng/meson-rng.c
>
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c1aa43b823..edb6152bb9 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -8,6 +8,14 @@ config DM_RNG
>
>  if DM_RNG
>
> +config RNG_MESON
> +       bool "Amlogic Meson Random Number Generator support"
> +       depends on ARCH_MESON
> +       default y
> +       help
> +         Enable support for hardware random number generator
> +         of Amlogic Meson SoCs.
> +
>  config RNG_SANDBOX
>         bool "Sandbox random number generator"
>         depends on SANDBOX
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 3517005541..6a8a66779b 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -4,5 +4,6 @@
>  #
>
>  obj-$(CONFIG_DM_RNG) += rng-uclass.o
> +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>  obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> new file mode 100644
> index 0000000000..4b81a62353
> --- /dev/null
> +++ b/drivers/rng/meson-rng.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * Driver for Amlogic hardware random number generator
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <asm/io.h>
> +
> +struct meson_rng_platdata {
> +       fdt_addr_t base;
> +       struct clk clk;
> +};
> +
> +/**
> + * meson_rng_read() - fill buffer with random bytes
> + *
> + * @buffer:    buffer to receive data
> + * @size:      size of buffer
> + *
> + * Return:     0
> + */
> +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       char *buffer = (char *)data;
> +
> +       while (len) {
> +               u32 rand = readl(pdata->base);
> +               size_t step;
>

I believe declaration of variables in the middle of the function is frowned
upon. Can be moved to the start of the function.

Other than that,
Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>

-sughosh



> +
> +               if (len >= 4)
> +                       step = 4;
> +               else
> +                       step = len;
> +               memcpy(buffer, &rand, step);
> +               buffer += step;
> +               len -= step;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * meson_rng_probe() - probe rng device
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_probe(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       int err;
> +
> +       err = clk_enable(&pdata->clk);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +/**
> + * meson_rng_remove() - deinitialize rng device
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_remove(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +       return clk_disable(&pdata->clk);
> +}
> +
> +/**
> + * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform
> data
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       int err;
> +
> +       pdata->base = dev_read_addr(dev);
> +       if (!pdata->base)
> +               return -ENODEV;
> +
> +       err = clk_get_by_name(dev, "core", &pdata->clk);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +static const struct dm_rng_ops meson_rng_ops = {
> +       .read = meson_rng_read,
> +};
> +
> +static const struct udevice_id meson_rng_match[] = {
> +       {
> +               .compatible = "amlogic,meson-rng",
> +       },
> +       {},
> +};
> +
> +U_BOOT_DRIVER(meson_rng) = {
> +       .name = "meson-rng",
> +       .id = UCLASS_RNG,
> +       .of_match = meson_rng_match,
> +       .ops = &meson_rng_ops,
> +       .probe = meson_rng_probe,
> +       .remove = meson_rng_remove,
> +       .platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
> +       .ofdata_to_platdata = meson_rng_ofdata_to_platdata,
> +};
> --
> 2.20.1
>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 10:51   ` Sughosh Ganu
  0 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2020-02-26 10:51 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Neil Armstrong, U-Boot Mailing List, u-boot-amlogic

[-- Attachment #1: Type: text/plain, Size: 4809 bytes --]

On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> Add support for the hardware random number generator of Amlogic SOCs.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/rng/Kconfig     |   8 +++
>  drivers/rng/Makefile    |   1 +
>  drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>  create mode 100644 drivers/rng/meson-rng.c
>
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c1aa43b823..edb6152bb9 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -8,6 +8,14 @@ config DM_RNG
>
>  if DM_RNG
>
> +config RNG_MESON
> +       bool "Amlogic Meson Random Number Generator support"
> +       depends on ARCH_MESON
> +       default y
> +       help
> +         Enable support for hardware random number generator
> +         of Amlogic Meson SoCs.
> +
>  config RNG_SANDBOX
>         bool "Sandbox random number generator"
>         depends on SANDBOX
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 3517005541..6a8a66779b 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -4,5 +4,6 @@
>  #
>
>  obj-$(CONFIG_DM_RNG) += rng-uclass.o
> +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>  obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> new file mode 100644
> index 0000000000..4b81a62353
> --- /dev/null
> +++ b/drivers/rng/meson-rng.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * Driver for Amlogic hardware random number generator
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <asm/io.h>
> +
> +struct meson_rng_platdata {
> +       fdt_addr_t base;
> +       struct clk clk;
> +};
> +
> +/**
> + * meson_rng_read() - fill buffer with random bytes
> + *
> + * @buffer:    buffer to receive data
> + * @size:      size of buffer
> + *
> + * Return:     0
> + */
> +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       char *buffer = (char *)data;
> +
> +       while (len) {
> +               u32 rand = readl(pdata->base);
> +               size_t step;
>

I believe declaration of variables in the middle of the function is frowned
upon. Can be moved to the start of the function.

Other than that,
Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>

-sughosh



> +
> +               if (len >= 4)
> +                       step = 4;
> +               else
> +                       step = len;
> +               memcpy(buffer, &rand, step);
> +               buffer += step;
> +               len -= step;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * meson_rng_probe() - probe rng device
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_probe(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       int err;
> +
> +       err = clk_enable(&pdata->clk);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +/**
> + * meson_rng_remove() - deinitialize rng device
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_remove(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +       return clk_disable(&pdata->clk);
> +}
> +
> +/**
> + * meson_rng_ofdata_to_platdata() - transfer device tree data to plaform
> data
> + *
> + * @dev:       device
> + * Return:     0 if ok
> + */
> +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> +       int err;
> +
> +       pdata->base = dev_read_addr(dev);
> +       if (!pdata->base)
> +               return -ENODEV;
> +
> +       err = clk_get_by_name(dev, "core", &pdata->clk);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +static const struct dm_rng_ops meson_rng_ops = {
> +       .read = meson_rng_read,
> +};
> +
> +static const struct udevice_id meson_rng_match[] = {
> +       {
> +               .compatible = "amlogic,meson-rng",
> +       },
> +       {},
> +};
> +
> +U_BOOT_DRIVER(meson_rng) = {
> +       .name = "meson-rng",
> +       .id = UCLASS_RNG,
> +       .of_match = meson_rng_match,
> +       .ops = &meson_rng_ops,
> +       .probe = meson_rng_probe,
> +       .remove = meson_rng_remove,
> +       .platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
> +       .ofdata_to_platdata = meson_rng_ofdata_to_platdata,
> +};
> --
> 2.20.1
>
>

[-- Attachment #2: Type: text/html, Size: 6481 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-26 10:51   ` Sughosh Ganu
@ 2020-02-26 17:59     ` Heinrich Schuchardt
  -1 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 17:59 UTC (permalink / raw)
  To: u-boot

On 2/26/20 11:51 AM, Sughosh Ganu wrote:
>
> On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     Add support for the hardware random number generator of Amlogic SOCs.
>
>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>>
>     ---
>      ?drivers/rng/Kconfig? ? ?|? ?8 +++
>      ?drivers/rng/Makefile? ? |? ?1 +
>      ?drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>      ?3 files changed, 129 insertions(+)
>      ?create mode 100644 drivers/rng/meson-rng.c
>
>     diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
>     index c1aa43b823..edb6152bb9 100644
>     --- a/drivers/rng/Kconfig
>     +++ b/drivers/rng/Kconfig
>     @@ -8,6 +8,14 @@ config DM_RNG
>
>      ?if DM_RNG
>
>     +config RNG_MESON
>     +? ? ? ?bool "Amlogic Meson Random Number Generator support"
>     +? ? ? ?depends on ARCH_MESON
>     +? ? ? ?default y
>     +? ? ? ?help
>     +? ? ? ? ?Enable support for hardware random number generator
>     +? ? ? ? ?of Amlogic Meson SoCs.
>     +
>      ?config RNG_SANDBOX
>      ? ? ? ? bool "Sandbox random number generator"
>      ? ? ? ? depends on SANDBOX
>     diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
>     index 3517005541..6a8a66779b 100644
>     --- a/drivers/rng/Makefile
>     +++ b/drivers/rng/Makefile
>     @@ -4,5 +4,6 @@
>      ?#
>
>      ?obj-$(CONFIG_DM_RNG) += rng-uclass.o
>     +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>      ?obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>      ?obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>     diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
>     new file mode 100644
>     index 0000000000..4b81a62353
>     --- /dev/null
>     +++ b/drivers/rng/meson-rng.c
>     @@ -0,0 +1,120 @@
>     +// SPDX-License-Identifier: GPL-2.0-or-later
>     +/*
>     + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>>
>     + *
>     + * Driver for Amlogic hardware random number generator
>     + */
>     +
>     +#include <common.h>
>     +#include <clk.h>
>     +#include <dm.h>
>     +#include <rng.h>
>     +#include <asm/io.h>
>     +
>     +struct meson_rng_platdata {
>     +? ? ? ?fdt_addr_t base;
>     +? ? ? ?struct clk clk;
>     +};
>     +
>     +/**
>     + * meson_rng_read() - fill buffer with random bytes
>     + *
>     + * @buffer:? ? buffer to receive data
>     + * @size:? ? ? size of buffer
>     + *
>     + * Return:? ? ?0
>     + */
>     +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
>     +{
>     +? ? ? ?struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +? ? ? ?char *buffer = (char *)data;
>     +
>     +? ? ? ?while (len) {
>     +? ? ? ? ? ? ? ?u32 rand = readl(pdata->base);
>     +? ? ? ? ? ? ? ?size_t step;
>
>
> I believe declaration of variables in the middle of the function is
> frowned upon. Can be moved to the start of the function.

rand is defined at the top of the smallest possible code block
delineated by braces.

C compilers before C99 required you to put definitions at the top of
functions. This led to the unsafe coding practice to reuse the same
variable for different purposes in the same function. Currently we are
using -std=gnu11.

U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
could reduce the scope of a variable.

Neither the U-Boot coding style guide
https://www.denx.de/wiki/U-Boot/CodingStyle
nor the Linux kernel coding style guide
https://www.kernel.org/doc/html/v4.10/process/coding-style.html
recommend such unsafe practice.

If you look at other parts of the U-Boot code, you will find that in
lots of places variables are defined in the smallest possible code block
(e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
carry a lot of code lines not following this rule.

But anyway if you still want it in a different way, I will resubmit.

Best regards

Heinrich

>
> Other than that,
> Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org
> <mailto:sughosh.ganu@linaro.org>>
>
> -sughosh
>
>     +
>     +? ? ? ? ? ? ? ?if (len >= 4)
>     +? ? ? ? ? ? ? ? ? ? ? ?step = 4;
>     +? ? ? ? ? ? ? ?else
>     +? ? ? ? ? ? ? ? ? ? ? ?step = len;
>     +? ? ? ? ? ? ? ?memcpy(buffer, &rand, step);
>     +? ? ? ? ? ? ? ?buffer += step;
>     +? ? ? ? ? ? ? ?len -= step;
>     +? ? ? ?}
>     +? ? ? ?return 0;
>     +}
>     +
>     +/**
>     + * meson_rng_probe() - probe rng device
>     + *
>     + * @dev:? ? ? ?device
>     + * Return:? ? ?0 if ok
>     + */
>     +static int meson_rng_probe(struct udevice *dev)
>     +{
>     +? ? ? ?struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +? ? ? ?int err;
>     +
>     +? ? ? ?err = clk_enable(&pdata->clk);
>     +? ? ? ?if (err)
>     +? ? ? ? ? ? ? ?return err;
>     +
>     +? ? ? ?return 0;
>     +}
>     +
>     +/**
>     + * meson_rng_remove() - deinitialize rng device
>     + *
>     + * @dev:? ? ? ?device
>     + * Return:? ? ?0 if ok
>     + */
>     +static int meson_rng_remove(struct udevice *dev)
>     +{
>     +? ? ? ?struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +
>     +? ? ? ?return clk_disable(&pdata->clk);
>     +}
>     +
>     +/**
>     + * meson_rng_ofdata_to_platdata() - transfer device tree data to
>     plaform data
>     + *
>     + * @dev:? ? ? ?device
>     + * Return:? ? ?0 if ok
>     + */
>     +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
>     +{
>     +? ? ? ?struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +? ? ? ?int err;
>     +
>     +? ? ? ?pdata->base = dev_read_addr(dev);
>     +? ? ? ?if (!pdata->base)
>     +? ? ? ? ? ? ? ?return -ENODEV;
>     +
>     +? ? ? ?err = clk_get_by_name(dev, "core", &pdata->clk);
>     +? ? ? ?if (err)
>     +? ? ? ? ? ? ? ?return err;
>     +
>     +? ? ? ?return 0;
>     +}
>     +
>     +static const struct dm_rng_ops meson_rng_ops = {
>     +? ? ? ?.read = meson_rng_read,
>     +};
>     +
>     +static const struct udevice_id meson_rng_match[] = {
>     +? ? ? ?{
>     +? ? ? ? ? ? ? ?.compatible = "amlogic,meson-rng",
>     +? ? ? ?},
>     +? ? ? ?{},
>     +};
>     +
>     +U_BOOT_DRIVER(meson_rng) = {
>     +? ? ? ?.name = "meson-rng",
>     +? ? ? ?.id = UCLASS_RNG,
>     +? ? ? ?.of_match = meson_rng_match,
>     +? ? ? ?.ops = &meson_rng_ops,
>     +? ? ? ?.probe = meson_rng_probe,
>     +? ? ? ?.remove = meson_rng_remove,
>     +? ? ? ?.platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
>     +? ? ? ?.ofdata_to_platdata = meson_rng_ofdata_to_platdata,
>     +};
>     --
>     2.20.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 17:59     ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 17:59 UTC (permalink / raw)
  To: Sughosh Ganu; +Cc: Neil Armstrong, U-Boot Mailing List, u-boot-amlogic

On 2/26/20 11:51 AM, Sughosh Ganu wrote:
>
> On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     Add support for the hardware random number generator of Amlogic SOCs.
>
>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>>
>     ---
>       drivers/rng/Kconfig     |   8 +++
>       drivers/rng/Makefile    |   1 +
>       drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++
>       3 files changed, 129 insertions(+)
>       create mode 100644 drivers/rng/meson-rng.c
>
>     diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
>     index c1aa43b823..edb6152bb9 100644
>     --- a/drivers/rng/Kconfig
>     +++ b/drivers/rng/Kconfig
>     @@ -8,6 +8,14 @@ config DM_RNG
>
>       if DM_RNG
>
>     +config RNG_MESON
>     +       bool "Amlogic Meson Random Number Generator support"
>     +       depends on ARCH_MESON
>     +       default y
>     +       help
>     +         Enable support for hardware random number generator
>     +         of Amlogic Meson SoCs.
>     +
>       config RNG_SANDBOX
>              bool "Sandbox random number generator"
>              depends on SANDBOX
>     diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
>     index 3517005541..6a8a66779b 100644
>     --- a/drivers/rng/Makefile
>     +++ b/drivers/rng/Makefile
>     @@ -4,5 +4,6 @@
>       #
>
>       obj-$(CONFIG_DM_RNG) += rng-uclass.o
>     +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>       obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>       obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>     diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
>     new file mode 100644
>     index 0000000000..4b81a62353
>     --- /dev/null
>     +++ b/drivers/rng/meson-rng.c
>     @@ -0,0 +1,120 @@
>     +// SPDX-License-Identifier: GPL-2.0-or-later
>     +/*
>     + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>>
>     + *
>     + * Driver for Amlogic hardware random number generator
>     + */
>     +
>     +#include <common.h>
>     +#include <clk.h>
>     +#include <dm.h>
>     +#include <rng.h>
>     +#include <asm/io.h>
>     +
>     +struct meson_rng_platdata {
>     +       fdt_addr_t base;
>     +       struct clk clk;
>     +};
>     +
>     +/**
>     + * meson_rng_read() - fill buffer with random bytes
>     + *
>     + * @buffer:    buffer to receive data
>     + * @size:      size of buffer
>     + *
>     + * Return:     0
>     + */
>     +static int meson_rng_read(struct udevice *dev, void *data, size_t len)
>     +{
>     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +       char *buffer = (char *)data;
>     +
>     +       while (len) {
>     +               u32 rand = readl(pdata->base);
>     +               size_t step;
>
>
> I believe declaration of variables in the middle of the function is
> frowned upon. Can be moved to the start of the function.

rand is defined at the top of the smallest possible code block
delineated by braces.

C compilers before C99 required you to put definitions at the top of
functions. This led to the unsafe coding practice to reuse the same
variable for different purposes in the same function. Currently we are
using -std=gnu11.

U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
could reduce the scope of a variable.

Neither the U-Boot coding style guide
https://www.denx.de/wiki/U-Boot/CodingStyle
nor the Linux kernel coding style guide
https://www.kernel.org/doc/html/v4.10/process/coding-style.html
recommend such unsafe practice.

If you look at other parts of the U-Boot code, you will find that in
lots of places variables are defined in the smallest possible code block
(e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
carry a lot of code lines not following this rule.

But anyway if you still want it in a different way, I will resubmit.

Best regards

Heinrich

>
> Other than that,
> Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org
> <mailto:sughosh.ganu@linaro.org>>
>
> -sughosh
>
>     +
>     +               if (len >= 4)
>     +                       step = 4;
>     +               else
>     +                       step = len;
>     +               memcpy(buffer, &rand, step);
>     +               buffer += step;
>     +               len -= step;
>     +       }
>     +       return 0;
>     +}
>     +
>     +/**
>     + * meson_rng_probe() - probe rng device
>     + *
>     + * @dev:       device
>     + * Return:     0 if ok
>     + */
>     +static int meson_rng_probe(struct udevice *dev)
>     +{
>     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +       int err;
>     +
>     +       err = clk_enable(&pdata->clk);
>     +       if (err)
>     +               return err;
>     +
>     +       return 0;
>     +}
>     +
>     +/**
>     + * meson_rng_remove() - deinitialize rng device
>     + *
>     + * @dev:       device
>     + * Return:     0 if ok
>     + */
>     +static int meson_rng_remove(struct udevice *dev)
>     +{
>     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +
>     +       return clk_disable(&pdata->clk);
>     +}
>     +
>     +/**
>     + * meson_rng_ofdata_to_platdata() - transfer device tree data to
>     plaform data
>     + *
>     + * @dev:       device
>     + * Return:     0 if ok
>     + */
>     +static int meson_rng_ofdata_to_platdata(struct udevice *dev)
>     +{
>     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>     +       int err;
>     +
>     +       pdata->base = dev_read_addr(dev);
>     +       if (!pdata->base)
>     +               return -ENODEV;
>     +
>     +       err = clk_get_by_name(dev, "core", &pdata->clk);
>     +       if (err)
>     +               return err;
>     +
>     +       return 0;
>     +}
>     +
>     +static const struct dm_rng_ops meson_rng_ops = {
>     +       .read = meson_rng_read,
>     +};
>     +
>     +static const struct udevice_id meson_rng_match[] = {
>     +       {
>     +               .compatible = "amlogic,meson-rng",
>     +       },
>     +       {},
>     +};
>     +
>     +U_BOOT_DRIVER(meson_rng) = {
>     +       .name = "meson-rng",
>     +       .id = UCLASS_RNG,
>     +       .of_match = meson_rng_match,
>     +       .ops = &meson_rng_ops,
>     +       .probe = meson_rng_probe,
>     +       .remove = meson_rng_remove,
>     +       .platdata_auto_alloc_size = sizeof(struct meson_rng_platdata),
>     +       .ofdata_to_platdata = meson_rng_ofdata_to_platdata,
>     +};
>     --
>     2.20.1
>


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-26 17:59     ` Heinrich Schuchardt
@ 2020-02-26 19:32       ` Sughosh Ganu
  -1 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2020-02-26 19:32 UTC (permalink / raw)
  To: u-boot

On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 2/26/20 11:51 AM, Sughosh Ganu wrote:
> >
> > On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     Add support for the hardware random number generator of Amlogic SOCs.
> >
> >     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
> >     <mailto:xypron.glpk@gmx.de>>
> >     ---
> >       drivers/rng/Kconfig     |   8 +++
> >       drivers/rng/Makefile    |   1 +
> >       drivers/rng/meson-rng.c | 120
> ++++++++++++++++++++++++++++++++++++++++
> >       3 files changed, 129 insertions(+)
> >       create mode 100644 drivers/rng/meson-rng.c
> >
> >     diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> >     index c1aa43b823..edb6152bb9 100644
> >     --- a/drivers/rng/Kconfig
> >     +++ b/drivers/rng/Kconfig
> >     @@ -8,6 +8,14 @@ config DM_RNG
> >
> >       if DM_RNG
> >
> >     +config RNG_MESON
> >     +       bool "Amlogic Meson Random Number Generator support"
> >     +       depends on ARCH_MESON
> >     +       default y
> >     +       help
> >     +         Enable support for hardware random number generator
> >     +         of Amlogic Meson SoCs.
> >     +
> >       config RNG_SANDBOX
> >              bool "Sandbox random number generator"
> >              depends on SANDBOX
> >     diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> >     index 3517005541..6a8a66779b 100644
> >     --- a/drivers/rng/Makefile
> >     +++ b/drivers/rng/Makefile
> >     @@ -4,5 +4,6 @@
> >       #
> >
> >       obj-$(CONFIG_DM_RNG) += rng-uclass.o
> >     +obj-$(CONFIG_RNG_MESON) += meson-rng.o
> >       obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
> >       obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> >     diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> >     new file mode 100644
> >     index 0000000000..4b81a62353
> >     --- /dev/null
> >     +++ b/drivers/rng/meson-rng.c
> >     @@ -0,0 +1,120 @@
> >     +// SPDX-License-Identifier: GPL-2.0-or-later
> >     +/*
> >     + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
> >     <mailto:xypron.glpk@gmx.de>>
> >     + *
> >     + * Driver for Amlogic hardware random number generator
> >     + */
> >     +
> >     +#include <common.h>
> >     +#include <clk.h>
> >     +#include <dm.h>
> >     +#include <rng.h>
> >     +#include <asm/io.h>
> >     +
> >     +struct meson_rng_platdata {
> >     +       fdt_addr_t base;
> >     +       struct clk clk;
> >     +};
> >     +
> >     +/**
> >     + * meson_rng_read() - fill buffer with random bytes
> >     + *
> >     + * @buffer:    buffer to receive data
> >     + * @size:      size of buffer
> >     + *
> >     + * Return:     0
> >     + */
> >     +static int meson_rng_read(struct udevice *dev, void *data, size_t
> len)
> >     +{
> >     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> >     +       char *buffer = (char *)data;
> >     +
> >     +       while (len) {
> >     +               u32 rand = readl(pdata->base);
> >     +               size_t step;
> >
> >
> > I believe declaration of variables in the middle of the function is
> > frowned upon. Can be moved to the start of the function.
>
> rand is defined at the top of the smallest possible code block
> delineated by braces.
>
> C compilers before C99 required you to put definitions at the top of
> functions. This led to the unsafe coding practice to reuse the same
> variable for different purposes in the same function. Currently we are
> using -std=gnu11.
>
> U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
> could reduce the scope of a variable.
>
> Neither the U-Boot coding style guide
> https://www.denx.de/wiki/U-Boot/CodingStyle
> nor the Linux kernel coding style guide
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> recommend such unsafe practice.
>

Yes, I had checked that this is not mentioned in the coding style guides.
But i have seen Wolfgang mention this on multiple occasions as part of his
review comments earlier, one example being [1].


>
> If you look at other parts of the U-Boot code, you will find that in
> lots of places variables are defined in the smallest possible code block
> (e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
> carry a lot of code lines not following this rule.
>
> But anyway if you still want it in a different way, I will resubmit.
>

I don't have a strong opinion on this. I was simply stating what I thought
to be a coding style in u-boot based on some earlier review comments that I
have seen.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2020-January/397651.html

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 19:32       ` Sughosh Ganu
  0 siblings, 0 replies; 14+ messages in thread
From: Sughosh Ganu @ 2020-02-26 19:32 UTC (permalink / raw)
  To: Heinrich Schuchardt; +Cc: Neil Armstrong, U-Boot Mailing List, u-boot-amlogic

[-- Attachment #1: Type: text/plain, Size: 4732 bytes --]

On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 2/26/20 11:51 AM, Sughosh Ganu wrote:
> >
> > On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     Add support for the hardware random number generator of Amlogic SOCs.
> >
> >     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
> >     <mailto:xypron.glpk@gmx.de>>
> >     ---
> >       drivers/rng/Kconfig     |   8 +++
> >       drivers/rng/Makefile    |   1 +
> >       drivers/rng/meson-rng.c | 120
> ++++++++++++++++++++++++++++++++++++++++
> >       3 files changed, 129 insertions(+)
> >       create mode 100644 drivers/rng/meson-rng.c
> >
> >     diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> >     index c1aa43b823..edb6152bb9 100644
> >     --- a/drivers/rng/Kconfig
> >     +++ b/drivers/rng/Kconfig
> >     @@ -8,6 +8,14 @@ config DM_RNG
> >
> >       if DM_RNG
> >
> >     +config RNG_MESON
> >     +       bool "Amlogic Meson Random Number Generator support"
> >     +       depends on ARCH_MESON
> >     +       default y
> >     +       help
> >     +         Enable support for hardware random number generator
> >     +         of Amlogic Meson SoCs.
> >     +
> >       config RNG_SANDBOX
> >              bool "Sandbox random number generator"
> >              depends on SANDBOX
> >     diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> >     index 3517005541..6a8a66779b 100644
> >     --- a/drivers/rng/Makefile
> >     +++ b/drivers/rng/Makefile
> >     @@ -4,5 +4,6 @@
> >       #
> >
> >       obj-$(CONFIG_DM_RNG) += rng-uclass.o
> >     +obj-$(CONFIG_RNG_MESON) += meson-rng.o
> >       obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
> >       obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> >     diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
> >     new file mode 100644
> >     index 0000000000..4b81a62353
> >     --- /dev/null
> >     +++ b/drivers/rng/meson-rng.c
> >     @@ -0,0 +1,120 @@
> >     +// SPDX-License-Identifier: GPL-2.0-or-later
> >     +/*
> >     + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
> >     <mailto:xypron.glpk@gmx.de>>
> >     + *
> >     + * Driver for Amlogic hardware random number generator
> >     + */
> >     +
> >     +#include <common.h>
> >     +#include <clk.h>
> >     +#include <dm.h>
> >     +#include <rng.h>
> >     +#include <asm/io.h>
> >     +
> >     +struct meson_rng_platdata {
> >     +       fdt_addr_t base;
> >     +       struct clk clk;
> >     +};
> >     +
> >     +/**
> >     + * meson_rng_read() - fill buffer with random bytes
> >     + *
> >     + * @buffer:    buffer to receive data
> >     + * @size:      size of buffer
> >     + *
> >     + * Return:     0
> >     + */
> >     +static int meson_rng_read(struct udevice *dev, void *data, size_t
> len)
> >     +{
> >     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
> >     +       char *buffer = (char *)data;
> >     +
> >     +       while (len) {
> >     +               u32 rand = readl(pdata->base);
> >     +               size_t step;
> >
> >
> > I believe declaration of variables in the middle of the function is
> > frowned upon. Can be moved to the start of the function.
>
> rand is defined at the top of the smallest possible code block
> delineated by braces.
>
> C compilers before C99 required you to put definitions at the top of
> functions. This led to the unsafe coding practice to reuse the same
> variable for different purposes in the same function. Currently we are
> using -std=gnu11.
>
> U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
> could reduce the scope of a variable.
>
> Neither the U-Boot coding style guide
> https://www.denx.de/wiki/U-Boot/CodingStyle
> nor the Linux kernel coding style guide
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> recommend such unsafe practice.
>

Yes, I had checked that this is not mentioned in the coding style guides.
But i have seen Wolfgang mention this on multiple occasions as part of his
review comments earlier, one example being [1].


>
> If you look at other parts of the U-Boot code, you will find that in
> lots of places variables are defined in the smallest possible code block
> (e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
> carry a lot of code lines not following this rule.
>
> But anyway if you still want it in a different way, I will resubmit.
>

I don't have a strong opinion on this. I was simply stating what I thought
to be a coding style in u-boot based on some earlier review comments that I
have seen.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2020-January/397651.html

[-- Attachment #2: Type: text/html, Size: 6928 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
  2020-02-26 19:32       ` Sughosh Ganu
@ 2020-02-26 20:18         ` Heinrich Schuchardt
  -1 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 20:18 UTC (permalink / raw)
  To: u-boot

On 2/26/20 8:32 PM, Sughosh Ganu wrote:
>
>
> On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 2/26/20 11:51 AM, Sughosh Ganu wrote:
>      >
>      > On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt
>     <xypron.glpk at gmx.de <mailto:xypron.glpk@gmx.de>
>      > <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk@gmx.de>>> wrote:
>      >
>      >? ? ?Add support for the hardware random number generator of
>     Amlogic SOCs.
>      >
>      >? ? ?Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>
>      >? ? ?<mailto:xypron.glpk at gmx.de <mailto:xypron.glpk@gmx.de>>>
>      >? ? ?---
>      >? ? ? ?drivers/rng/Kconfig? ? ?|? ?8 +++
>      >? ? ? ?drivers/rng/Makefile? ? |? ?1 +
>      >? ? ? ?drivers/rng/meson-rng.c | 120
>     ++++++++++++++++++++++++++++++++++++++++
>      >? ? ? ?3 files changed, 129 insertions(+)
>      >? ? ? ?create mode 100644 drivers/rng/meson-rng.c
>      >
>      >? ? ?diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
>      >? ? ?index c1aa43b823..edb6152bb9 100644
>      >? ? ?--- a/drivers/rng/Kconfig
>      >? ? ?+++ b/drivers/rng/Kconfig
>      >? ? ?@@ -8,6 +8,14 @@ config DM_RNG
>      >
>      >? ? ? ?if DM_RNG
>      >
>      >? ? ?+config RNG_MESON
>      >? ? ?+? ? ? ?bool "Amlogic Meson Random Number Generator support"
>      >? ? ?+? ? ? ?depends on ARCH_MESON
>      >? ? ?+? ? ? ?default y
>      >? ? ?+? ? ? ?help
>      >? ? ?+? ? ? ? ?Enable support for hardware random number generator
>      >? ? ?+? ? ? ? ?of Amlogic Meson SoCs.
>      >? ? ?+
>      >? ? ? ?config RNG_SANDBOX
>      >? ? ? ? ? ? ? bool "Sandbox random number generator"
>      >? ? ? ? ? ? ? depends on SANDBOX
>      >? ? ?diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
>      >? ? ?index 3517005541..6a8a66779b 100644
>      >? ? ?--- a/drivers/rng/Makefile
>      >? ? ?+++ b/drivers/rng/Makefile
>      >? ? ?@@ -4,5 +4,6 @@
>      >? ? ? ?#
>      >
>      >? ? ? ?obj-$(CONFIG_DM_RNG) += rng-uclass.o
>      >? ? ?+obj-$(CONFIG_RNG_MESON) += meson-rng.o
>      >? ? ? ?obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>      >? ? ? ?obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>      >? ? ?diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
>      >? ? ?new file mode 100644
>      >? ? ?index 0000000000..4b81a62353
>      >? ? ?--- /dev/null
>      >? ? ?+++ b/drivers/rng/meson-rng.c
>      >? ? ?@@ -0,0 +1,120 @@
>      >? ? ?+// SPDX-License-Identifier: GPL-2.0-or-later
>      >? ? ?+/*
>      >? ? ?+ * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>
>      >? ? ?<mailto:xypron.glpk at gmx.de <mailto:xypron.glpk@gmx.de>>>
>      >? ? ?+ *
>      >? ? ?+ * Driver for Amlogic hardware random number generator
>      >? ? ?+ */
>      >? ? ?+
>      >? ? ?+#include <common.h>
>      >? ? ?+#include <clk.h>
>      >? ? ?+#include <dm.h>
>      >? ? ?+#include <rng.h>
>      >? ? ?+#include <asm/io.h>
>      >? ? ?+
>      >? ? ?+struct meson_rng_platdata {
>      >? ? ?+? ? ? ?fdt_addr_t base;
>      >? ? ?+? ? ? ?struct clk clk;
>      >? ? ?+};
>      >? ? ?+
>      >? ? ?+/**
>      >? ? ?+ * meson_rng_read() - fill buffer with random bytes
>      >? ? ?+ *
>      >? ? ?+ * @buffer:? ? buffer to receive data
>      >? ? ?+ * @size:? ? ? size of buffer
>      >? ? ?+ *
>      >? ? ?+ * Return:? ? ?0
>      >? ? ?+ */
>      >? ? ?+static int meson_rng_read(struct udevice *dev, void *data,
>     size_t len)
>      >? ? ?+{
>      >? ? ?+? ? ? ?struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>      >? ? ?+? ? ? ?char *buffer = (char *)data;
>      >? ? ?+
>      >? ? ?+? ? ? ?while (len) {
>      >? ? ?+? ? ? ? ? ? ? ?u32 rand = readl(pdata->base);
>      >? ? ?+? ? ? ? ? ? ? ?size_t step;
>      >
>      >
>      > I believe declaration of variables in the middle of the function is
>      > frowned upon. Can be moved to the start of the function.
>
>     rand is defined at the top of the smallest possible code block
>     delineated by braces.
>
>     C compilers before C99 required you to put definitions at the top of
>     functions. This led to the unsafe coding practice to reuse the same
>     variable for different purposes in the same function. Currently we are
>     using -std=gnu11.
>
>     U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
>     could reduce the scope of a variable.
>
>     Neither the U-Boot coding style guide
>     https://www.denx.de/wiki/U-Boot/CodingStyle
>     nor the Linux kernel coding style guide
>     https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>     recommend such unsafe practice.
>
>
> Yes, I had checked that this is not mentioned in the coding style
> guides. But i have seen Wolfgang mention this on multiple occasions as
> part of his review comments earlier, one example being [1].

https://lists.denx.de/pipermail/u-boot/2020-January/397651.html
has a variable definition which is not at the start of code block
delineated by {} but in the middle of the code block. I agree with
Wolfgang that this is not advisable.

Best regards

Heinrich

>
>
>     If you look at other parts of the U-Boot code, you will find that in
>     lots of places variables are defined in the smallest possible code block
>     (e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
>     carry a lot of code lines not following this rule.
>
>     But anyway if you still want it in a different way, I will resubmit.
>
>
> I don't have a strong opinion on this. I was simply stating what I
> thought to be a coding style in u-boot based on some earlier review
> comments that I have seen.
>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2020-January/397651.html

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver
@ 2020-02-26 20:18         ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-02-26 20:18 UTC (permalink / raw)
  To: Sughosh Ganu; +Cc: Neil Armstrong, U-Boot Mailing List, u-boot-amlogic

On 2/26/20 8:32 PM, Sughosh Ganu wrote:
>
>
> On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 2/26/20 11:51 AM, Sughosh Ganu wrote:
>      >
>      > On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt
>     <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>
>      > <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>> wrote:
>      >
>      >     Add support for the hardware random number generator of
>     Amlogic SOCs.
>      >
>      >     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>
>      >     <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>>
>      >     ---
>      >       drivers/rng/Kconfig     |   8 +++
>      >       drivers/rng/Makefile    |   1 +
>      >       drivers/rng/meson-rng.c | 120
>     ++++++++++++++++++++++++++++++++++++++++
>      >       3 files changed, 129 insertions(+)
>      >       create mode 100644 drivers/rng/meson-rng.c
>      >
>      >     diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
>      >     index c1aa43b823..edb6152bb9 100644
>      >     --- a/drivers/rng/Kconfig
>      >     +++ b/drivers/rng/Kconfig
>      >     @@ -8,6 +8,14 @@ config DM_RNG
>      >
>      >       if DM_RNG
>      >
>      >     +config RNG_MESON
>      >     +       bool "Amlogic Meson Random Number Generator support"
>      >     +       depends on ARCH_MESON
>      >     +       default y
>      >     +       help
>      >     +         Enable support for hardware random number generator
>      >     +         of Amlogic Meson SoCs.
>      >     +
>      >       config RNG_SANDBOX
>      >              bool "Sandbox random number generator"
>      >              depends on SANDBOX
>      >     diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
>      >     index 3517005541..6a8a66779b 100644
>      >     --- a/drivers/rng/Makefile
>      >     +++ b/drivers/rng/Makefile
>      >     @@ -4,5 +4,6 @@
>      >       #
>      >
>      >       obj-$(CONFIG_DM_RNG) += rng-uclass.o
>      >     +obj-$(CONFIG_RNG_MESON) += meson-rng.o
>      >       obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o
>      >       obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>      >     diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c
>      >     new file mode 100644
>      >     index 0000000000..4b81a62353
>      >     --- /dev/null
>      >     +++ b/drivers/rng/meson-rng.c
>      >     @@ -0,0 +1,120 @@
>      >     +// SPDX-License-Identifier: GPL-2.0-or-later
>      >     +/*
>      >     + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de
>     <mailto:xypron.glpk@gmx.de>
>      >     <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>>
>      >     + *
>      >     + * Driver for Amlogic hardware random number generator
>      >     + */
>      >     +
>      >     +#include <common.h>
>      >     +#include <clk.h>
>      >     +#include <dm.h>
>      >     +#include <rng.h>
>      >     +#include <asm/io.h>
>      >     +
>      >     +struct meson_rng_platdata {
>      >     +       fdt_addr_t base;
>      >     +       struct clk clk;
>      >     +};
>      >     +
>      >     +/**
>      >     + * meson_rng_read() - fill buffer with random bytes
>      >     + *
>      >     + * @buffer:    buffer to receive data
>      >     + * @size:      size of buffer
>      >     + *
>      >     + * Return:     0
>      >     + */
>      >     +static int meson_rng_read(struct udevice *dev, void *data,
>     size_t len)
>      >     +{
>      >     +       struct meson_rng_platdata *pdata = dev_get_platdata(dev);
>      >     +       char *buffer = (char *)data;
>      >     +
>      >     +       while (len) {
>      >     +               u32 rand = readl(pdata->base);
>      >     +               size_t step;
>      >
>      >
>      > I believe declaration of variables in the middle of the function is
>      > frowned upon. Can be moved to the start of the function.
>
>     rand is defined at the top of the smallest possible code block
>     delineated by braces.
>
>     C compilers before C99 required you to put definitions at the top of
>     functions. This led to the unsafe coding practice to reuse the same
>     variable for different purposes in the same function. Currently we are
>     using -std=gnu11.
>
>     U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you
>     could reduce the scope of a variable.
>
>     Neither the U-Boot coding style guide
>     https://www.denx.de/wiki/U-Boot/CodingStyle
>     nor the Linux kernel coding style guide
>     https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>     recommend such unsafe practice.
>
>
> Yes, I had checked that this is not mentioned in the coding style
> guides. But i have seen Wolfgang mention this on multiple occasions as
> part of his review comments earlier, one example being [1].

https://lists.denx.de/pipermail/u-boot/2020-January/397651.html
has a variable definition which is not at the start of code block
delineated by {} but in the middle of the code block. I agree with
Wolfgang that this is not advisable.

Best regards

Heinrich

>
>
>     If you look at other parts of the U-Boot code, you will find that in
>     lots of places variables are defined in the smallest possible code block
>     (e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still
>     carry a lot of code lines not following this rule.
>
>     But anyway if you still want it in a different way, I will resubmit.
>
>
> I don't have a strong opinion on this. I was simply stating what I
> thought to be a coding style in u-boot based on some earlier review
> comments that I have seen.
>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2020-January/397651.html


^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-02-26 20:18 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24 22:21 [PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver Heinrich Schuchardt
2020-02-24 22:21 ` Heinrich Schuchardt
2020-02-26 10:19 ` Neil Armstrong
2020-02-26 10:19   ` Neil Armstrong
2020-02-26 10:38   ` Heinrich Schuchardt
2020-02-26 10:38     ` Heinrich Schuchardt
2020-02-26 10:51 ` Sughosh Ganu
2020-02-26 10:51   ` Sughosh Ganu
2020-02-26 17:59   ` Heinrich Schuchardt
2020-02-26 17:59     ` Heinrich Schuchardt
2020-02-26 19:32     ` Sughosh Ganu
2020-02-26 19:32       ` Sughosh Ganu
2020-02-26 20:18       ` Heinrich Schuchardt
2020-02-26 20:18         ` Heinrich Schuchardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.