Re: [PATCH v5 3/8] stm32mp1: rng: Add a driver for random number generator(rng) device

27 Dec 2019

      On Fri, 27 Dec 2019 at 18:25, Heinrich Schuchardt xypron.glpk@gmx.de
wrote:
...
On 12/27/19 1:42 PM, Heinrich Schuchardt wrote:
...
On 12/27/19 12:19 PM, Sughosh Ganu wrote:
...
On Fri, 27 Dec 2019 at 13:21, Heinrich Schuchardt <xypron.glpk@gmx.de
mailto:xypron.glpk@gmx.de> wrote:
On 12/26/19 6:25 PM, Sughosh Ganu wrote:
 > Add a driver for the rng device found on stm32mp1 platforms. The
 > driver provides a routine for reading the random number seed

from the
     > hardware device.
     >
     > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
    mailto:sughosh.ganu@linaro.org>
     > Reviewed-by: Patrice Chotard <patrice.chotard@st.com
    mailto:patrice.chotard@st.com>
     > Acked-by: Patrick Delaunay <patrick.delaunay@st.com
    mailto:patrick.delaunay@st.com>
     > ---
     > Changes since V4:
     > * Return number of bytes read on a successful read, and a -ve
    value on
     >    error.
     >
     >   drivers/rng/Kconfig        |   7 ++
     >   drivers/rng/Makefile       |   1 +
     >   drivers/rng/stm32mp1_rng.c | 165
    +++++++++++++++++++++++++++++++++++++++++++++
     >   3 files changed, 173 insertions(+)
     >   create mode 100644 drivers/rng/stm32mp1_rng.c
     >
     > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
     > index dd44cc0..c9c4751 100644
     > --- a/drivers/rng/Kconfig
     > +++ b/drivers/rng/Kconfig
     > @@ -5,3 +5,10 @@ config DM_RNG
     >         Enable driver model for random number generator(rng)
devices.
     >         This interface is used to initialise the rng device and
to
...
...
 >         read the random seed from the device.
 > +
 > +config RNG_STM32MP1
 > +     bool "Enable random number generator for STM32MP1"
 > +     depends on ARCH_STM32MP && DM_RNG
 > +     default n
 > +     help
 > +       Enable STM32MP1 rng driver.
 > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
 > index 311705b..699beb3 100644
 > --- a/drivers/rng/Makefile
 > +++ b/drivers/rng/Makefile
 > @@ -4,3 +4,4 @@
 >   #
 >
 >   obj-$(CONFIG_DM_RNG) += rng-uclass.o
 > +obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
 > diff --git a/drivers/rng/stm32mp1_rng.c

b/drivers/rng/stm32mp1_rng.c
     > new file mode 100644
     > index 0000000..56ad848
     > --- /dev/null
     > +++ b/drivers/rng/stm32mp1_rng.c
     > @@ -0,0 +1,165 @@
     > +// SPDX-License-Identifier: GPL-2.0-or-later
     > +/*
     > + * Copyright (c) 2019, Linaro Limited
     > + */
     > +
     > +#include <common.h>
     > +#include <clk.h>
     > +#include <dm.h>
     > +#include <reset.h>
     > +#include <rng.h>
     > +
     > +#include <asm/io.h>
     > +#include <linux/iopoll.h>
     > +#include <linux/kernel.h>
     > +
     > +#define RNG_CR 0x00
     > +#define RNG_CR_RNGEN BIT(2)
     > +#define RNG_CR_CED BIT(5)
     > +
     > +#define RNG_SR 0x04
     > +#define RNG_SR_SEIS BIT(6)
     > +#define RNG_SR_CEIS BIT(5)
     > +#define RNG_SR_SECS BIT(2)
     > +#define RNG_SR_DRDY BIT(0)
     > +
     > +#define RNG_DR 0x08
     > +
     > +struct stm32_rng_platdata {
     > +     fdt_addr_t base;
     > +     struct clk clk;
     > +     struct reset_ctl rst;
     > +};
     > +
     > +static int stm32_rng_read(struct udevice *dev, void *data,
    size_t len)
     > +{
     > +     int retval = 0, i, nbytes;
     > +     u32 sr, count, reg;
     > +     size_t increment;
     > +     struct stm32_rng_platdata *pdata = dev_get_platdata(dev);
     > +
     > +     /*
     > +      * Only INT_MAX number of bytes can be returned. If more
     > +      * bytes need to be read, the caller must do it in a loop.
     > +      */
     > +     if (len > INT_MAX)
     > +             len = INT_MAX;
     > +
     > +     nbytes = len;
     > +     while (len > 0) {
     > +             retval = readl_poll_timeout(pdata->base + RNG_SR,
sr,
     > +                                         sr & RNG_SR_DRDY,
10000);
     > +             if (retval)
     > +                     return retval;
     > +
     > +             if (sr & (RNG_SR_SEIS | RNG_SR_SECS)) {
     > +                     /* As per SoC TRM */
     > +                     clrbits_le32(pdata->base + RNG_SR,
    RNG_SR_SEIS);
     > +                     for (i = 0; i < 12; i++)
     > +                             readl(pdata->base + RNG_DR);
     > +                     if (readl(pdata->base + RNG_SR) &
    RNG_SR_SEIS) {
     > +                             printf("RNG Noise");
     > +                             return -EIO;
The SEIS bit indicates a seed error. According to the RM0090

Reference
    manual for the STM32F429/439 you should clear the SEIS bit and set
the
    RNGEN bit to restart the RNG.
https://www.st.com/content/ccc/resource/technical/document/reference_manual/...,
...
...
page 768.

So why do you return an error code here? What do you expect the

caller
    to do?
I am referring to the stm32mp157 trm, Noise source error detection, pg
1939.
Could you, please, provide a link to the document for stm32mp157.
RM0436 Reference manual STM32MP157 advanced Arm®-based 32-bit MPUs
https://www.st.com/content/ccc/resource/technical/document/reference_manual/...
chapter 37.3.7 - "Error management" requires the following error handling:

Clear the SEIS bit by writing it to “0”.
Read out 12 words from the RNG_DR register, and discard each of them

in order to clean the pipeline.
3. Confirm that SEIS is still cleared. Random number generation is back
to normal
So the same error handling can be used for both SoCs.
Which is what the current logic is doing. In case, the SEIS bit is still
set after reading the 12 words, the driver returns an error, else it
continues.
-sughosh