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

[Heinrich Schuchardt]

On 2/26/20 8:32 PM, Sughosh Ganu wrote:
>
>
> On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk at gmx.de
> <mailto: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>
>      > <mailto: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>
>      >     <mailto: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>
>      >     <mailto: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].

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