[PATCH 1/1] drivers/rng: add Amlogic hardware RNG driver

[Sughosh Ganu]

On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk at 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 at gmx.de>> wrote:
> >
> >     Add support for the hardware random number generator of Amlogic SOCs.
> >
> >     Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de
> >     <mailto:xypron.glpk at 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 at gmx.de
> >     <mailto:xypron.glpk at 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