From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20190828162617.237398-1-tmaimon77@gmail.com> <20190828162617.237398-3-tmaimon77@gmail.com> <20190829104721.tnjk3bqt3cq6iagr@holly.lan> <20190909151033.f3inbbas4duzsmh5@holly.lan> In-Reply-To: <20190909151033.f3inbbas4duzsmh5@holly.lan> From: Tomer Maimon Date: Tue, 10 Sep 2019 13:52:35 +0300 Message-ID: Subject: Re: [PATCH v1 2/2] hwrng: npcm: add NPCM RNG driver Content-Type: multipart/alternative; boundary="00000000000049a5e8059230937d" To: Daniel Thompson Cc: mpm@selenic.com, herbert@gondor.apana.org.au, Arnd Bergmann , Greg KH , Rob Herring , Mark Rutland , Avi Fishman , Tali Perry , Patrick Venture , Nancy Yuen , Benjamin Fair , sumit.garg@linaro.org, jens.wiklander@linaro.org, vkoul@kernel.org, Thomas Gleixner , Joel Stanley , devicetree , Linux Kernel Mailing List , linux-crypto@vger.kernel.org, OpenBMC Maillist List-ID: --00000000000049a5e8059230937d Content-Type: text/plain; charset="UTF-8" Hi Daniel, Thanks for your prompt reply, On Mon, 9 Sep 2019 at 18:10, Daniel Thompson wrote: > On Mon, Sep 09, 2019 at 05:31:30PM +0300, Tomer Maimon wrote: > > Hi Daniel, > > > > appreciate your comments and sorry for the late reply > > > > On Thu, 29 Aug 2019 at 13:47, Daniel Thompson < > daniel.thompson@linaro.org> > > wrote: > > > > > On Wed, Aug 28, 2019 at 07:26:17PM +0300, Tomer Maimon wrote: > > > > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver. > > > > > > > > Signed-off-by: Tomer Maimon > > > > --- > > > > drivers/char/hw_random/Kconfig | 13 ++ > > > > drivers/char/hw_random/Makefile | 1 + > > > > drivers/char/hw_random/npcm-rng.c | 207 > ++++++++++++++++++++++++++++++ > > > > 3 files changed, 221 insertions(+) > > > > create mode 100644 drivers/char/hw_random/npcm-rng.c > > > > > > > > diff --git a/drivers/char/hw_random/npcm-rng.c > > > b/drivers/char/hw_random/npcm-rng.c > > > > new file mode 100644 > > > > index 000000000000..5b4b1b6cb362 > > > > --- /dev/null > > > > +++ b/drivers/char/hw_random/npcm-rng.c > > > > @@ -0,0 +1,207 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +// Copyright (c) 2019 Nuvoton Technology corporation. > > > > + > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#define NPCM_RNGCS_REG 0x00 /* Control and status > > > register */ > > > > +#define NPCM_RNGD_REG 0x04 /* Data register */ > > > > +#define NPCM_RNGMODE_REG 0x08 /* Mode register */ > > > > + > > > > +#define NPCM_RNG_CLK_SET_25MHZ GENMASK(4, 3) /* 20-25 MHz */ > > > > +#define NPCM_RNG_DATA_VALID BIT(1) > > > > +#define NPCM_RNG_ENABLE BIT(0) > > > > +#define NPCM_RNG_M1ROSEL BIT(1) > > > > + > > > > +#define NPCM_RNG_TIMEOUT_POLL 20 > > > > > > Might be better to define this in real-world units (such as > > > milliseconds) since the timeout is effectively the longest time the > > > hardware can take to generate 4 bytes. > > > > > > > + > > > > +#define to_npcm_rng(p) container_of(p, struct npcm_rng, rng) > > > > + > > > > +struct npcm_rng { > > > > + void __iomem *base; > > > > + struct hwrng rng; > > > > +}; > > > > + > > > > +static int npcm_rng_init(struct hwrng *rng) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + u32 val; > > > > + > > > > + val = readl(priv->base + NPCM_RNGCS_REG); > > > > + val |= NPCM_RNG_ENABLE; > > > > + writel(val, priv->base + NPCM_RNGCS_REG); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void npcm_rng_cleanup(struct hwrng *rng) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + u32 val; > > > > + > > > > + val = readl(priv->base + NPCM_RNGCS_REG); > > > > + val &= ~NPCM_RNG_ENABLE; > > > > + writel(val, priv->base + NPCM_RNGCS_REG); > > > > +} > > > > + > > > > +static bool npcm_rng_wait_ready(struct hwrng *rng, bool wait) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + int timeout_cnt = 0; > > > > + int ready; > > > > + > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) & > NPCM_RNG_DATA_VALID; > > > > + while ((ready == 0) && (timeout_cnt < NPCM_RNG_TIMEOUT_POLL)) { > > > > + usleep_range(500, 1000); > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) & > > > > + NPCM_RNG_DATA_VALID; > > > > + timeout_cnt++; > > > > + } > > > > + > > > > + return !!ready; > > > > +} > > > > > > This looks like an open-coded version of readl_poll_timeout()... better > > > to use the library function. > > > > > > Also the sleep looks a bit long to me. What is the generation rate of > > > the peripheral? Most RNG drivers have short intervals between data > > > generation so they use delays rather than sleeps (a.k.a. > > > readl_poll_timeout_atomic() ). > > > > the HWRNG generate byte of random data in a few milliseconds so it is > > better to use the sleep command. > > That's fine, just use readl_poll_timeout() then. > > > > > > + > > > > +static int npcm_rng_read(struct hwrng *rng, void *buf, size_t max, > bool > > > wait) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + int retval = 0; > > > > + > > > > + pm_runtime_get_sync((struct device *)priv->rng.priv); > > > > + > > > > + while (max >= sizeof(u32)) { > > > > + if (!npcm_rng_wait_ready(rng, wait)) > > > > + break; > > > > > > The code as currently written does not honour the wait parameter (e.g. > > > it sleeps even when wait is false). > > > > > > > > > > + > > > > + *(u32 *)buf = readl(priv->base + NPCM_RNGD_REG); > > > > + retval += sizeof(u32); > > > > + buf += sizeof(u32); > > > > + max -= sizeof(u32); > > > > + } > > > > + > > > > + pm_runtime_mark_last_busy((struct device *)priv->rng.priv); > > > > + pm_runtime_put_sync_autosuspend((struct device > *)priv->rng.priv); > > > > + > > > > + return retval || !wait ? retval : -EIO; > > > > +} > > > > + > > > > +static int npcm_rng_probe(struct platform_device *pdev) > > > > +{ > > > > + struct npcm_rng *priv; > > > > + struct resource *res; > > > > + u32 quality; > > > > + int ret; > > > > + > > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > > > + if (!priv) > > > > + return -ENOMEM; > > > > + > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + priv->base = devm_ioremap_resource(&pdev->dev, res); > > > > + if (IS_ERR(priv->base)) > > > > + return PTR_ERR(priv->base); > > > > + > > > > + priv->rng.name = pdev->name; > > > > +#ifndef CONFIG_PM > > > > + priv->rng.init = npcm_rng_init; > > > > + priv->rng.cleanup = npcm_rng_cleanup; > > > > +#endif > > > > + priv->rng.read = npcm_rng_read; > > > > + priv->rng.priv = (unsigned long)&pdev->dev; > > > > + if (of_property_read_u32(pdev->dev.of_node, "quality", > &quality)) > > > > + priv->rng.quality = 1000; > > > > + else > > > > + priv->rng.quality = quality; > > > > + > > > > + writel(NPCM_RNG_M1ROSEL, priv->base + NPCM_RNGMODE_REG); > > > > +#ifndef CONFIG_PM > > > > + writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG); > > > > +#else > > > > + writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE, > > > > + priv->base + NPCM_RNGCS_REG); > > > > +#endif > > > > > > If this initialization was moved to npcm_rng_init() then there would be > > > no need for the additional ifdefing. It would also get rid of the > > > (potentially slow) readl calls on the PM wakeup path. > > > > > > > But when the Kernel have PM configuration than the priv->rng.init is not > > set and > > *add_early_randomness* function is called. for the *add_early_randomness* > > success > > the hwrng need to enabled in the probe. > > Sorry but I don't understand this reply. > > When CONFIG_PM is enabled then the probe function does not currently set > NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being called by > Sorry maybe I miss understood, but when the CONFIG_PM enabled so the NPCM_RNG_ENABLE sets (the code use ifndef and not ifdef) *#ifndef CONFIG_PM* writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG); #else (*CONFIG_PM enabled*) writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE, priv->base + NPCM_RNGCS_REG); #endif And the hwrng needed to be enabled to run *add_early_randomness *function successfully. If the hwrng driver will relay on PM logic to enable the hwrng will be disable when *add_early_randomness *function is called. the PM logic (as part of pm_runtime_get_sync() ). > > Given the code *already* relies on npcm_rng_init() being called by the > PM logic why does it matter if additional init is put there? > > > Daniel. > Thanks, Tomer --00000000000049a5e8059230937d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Dani= el,

Thanks for your prompt reply,



On Mon, 9 Sep 2019 at 18:10, Daniel Thompson <daniel.thompson@linaro.org> wrote:<= br>
On Mon, Sep 09, = 2019 at 05:31:30PM +0300, Tomer Maimon wrote:
> Hi Daniel,
>
> appreciate your comments and sorry for the late reply
>
> On Thu, 29 Aug 2019 at 13:47, Daniel Thompson <daniel.thompson@linaro.org&= gt;
> wrote:
>
> > On Wed, Aug 28, 2019 at 07:26:17PM +0300, Tomer Maimon wrote:
> > > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver. > > >
> > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > > ---
> > >=C2=A0 drivers/char/hw_random/Kconfig=C2=A0 =C2=A0 |=C2=A0 13= ++
> > >=C2=A0 drivers/char/hw_random/Makefile=C2=A0 =C2=A0|=C2=A0 = =C2=A01 +
> > >=C2=A0 drivers/char/hw_random/npcm-rng.c | 207 ++++++++++++++= ++++++++++++++++
> > >=C2=A0 3 files changed, 221 insertions(+)
> > >=C2=A0 create mode 100644 drivers/char/hw_random/npcm-rng.c > > >
> > > diff --git a/drivers/char/hw_random/npcm-rng.c
> > b/drivers/char/hw_random/npcm-rng.c
> > > new file mode 100644
> > > index 000000000000..5b4b1b6cb362
> > > --- /dev/null
> > > +++ b/drivers/char/hw_random/npcm-rng.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2019 Nuvoton Technology corporation.
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/init.h>
> > > +#include <linux/random.h>
> > > +#include <linux/err.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/hw_random.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +#define NPCM_RNGCS_REG=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A00x00=C2=A0 =C2=A0 /* Control and status
> > register */
> > > +#define NPCM_RNGD_REG=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 0x04=C2=A0 =C2=A0 /* Data register */
> > > +#define NPCM_RNGMODE_REG=C2=A0 =C2=A0 =C2=A00x08=C2=A0 =C2= =A0 /* Mode register */
> > > +
> > > +#define NPCM_RNG_CLK_SET_25MHZ=C2=A0 =C2=A0 =C2=A0 =C2=A0GE= NMASK(4, 3) /* 20-25 MHz */
> > > +#define NPCM_RNG_DATA_VALID=C2=A0 BIT(1)
> > > +#define NPCM_RNG_ENABLE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 BIT(0)
> > > +#define NPCM_RNG_M1ROSEL=C2=A0 =C2=A0 =C2=A0BIT(1)
> > > +
> > > +#define NPCM_RNG_TIMEOUT_POLL=C2=A0 =C2=A0 =C2=A0 =C2=A0 20=
> >
> > Might be better to define this in real-world units (such as
> > milliseconds) since the timeout is effectively the longest time t= he
> > hardware can take to generate 4 bytes.
> >
> > > +
> > > +#define to_npcm_rng(p)=C2=A0 =C2=A0 =C2=A0 =C2=A0container_= of(p, struct npcm_rng, rng)
> > > +
> > > +struct npcm_rng {
> > > +=C2=A0 =C2=A0 =C2=A0void __iomem *base;
> > > +=C2=A0 =C2=A0 =C2=A0struct hwrng rng;
> > > +};
> > > +
> > > +static int npcm_rng_init(struct hwrng *rng)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base + NPCM_RNGC= S_REG);
> > > +=C2=A0 =C2=A0 =C2=A0val |=3D NPCM_RNG_ENABLE;
> > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + NPCM_RNGCS_= REG);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return 0;
> > > +}
> > > +
> > > +static void npcm_rng_cleanup(struct hwrng *rng)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base + NPCM_RNGC= S_REG);
> > > +=C2=A0 =C2=A0 =C2=A0val &=3D ~NPCM_RNG_ENABLE;
> > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + NPCM_RNGCS_= REG);
> > > +}
> > > +
> > > +static bool npcm_rng_wait_ready(struct hwrng *rng, bool wai= t)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0int timeout_cnt =3D 0;
> > > +=C2=A0 =C2=A0 =C2=A0int ready;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0ready =3D readl(priv->base + NPCM_RN= GCS_REG) & NPCM_RNG_DATA_VALID;
> > > +=C2=A0 =C2=A0 =C2=A0while ((ready =3D=3D 0) && (tim= eout_cnt < NPCM_RNG_TIMEOUT_POLL)) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0usleep_rang= e(500, 1000);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ready =3D r= eadl(priv->base + NPCM_RNGCS_REG) &
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0NPCM_RNG_DATA_VALID;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0timeout_cnt= ++;
> > > +=C2=A0 =C2=A0 =C2=A0}
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return !!ready;
> > > +}
> >
> > This looks like an open-coded version of readl_poll_timeout()... = better
> > to use the library function.
> >
> > Also the sleep looks a bit long to me. What is the generation rat= e of
> > the peripheral? Most RNG drivers have short intervals between dat= a
> > generation so they use delays rather than sleeps (a.k.a.
> > readl_poll_timeout_atomic() ).
>
> the HWRNG generate byte of random data in a few milliseconds so it is<= br> > better to use the sleep command.

That's fine, just use readl_poll_timeout() then.


> > > +
> > > +static int npcm_rng_read(struct hwrng *rng, void *buf, size= _t max, bool
> > wait)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0int retval =3D 0;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_get_sync((struct device *)pr= iv->rng.priv);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0while (max >=3D sizeof(u32)) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!npcm_r= ng_wait_ready(rng, wait))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0break;
> >
> > The code as currently written does not honour the wait parameter = (e.g.
> > it sleeps even when wait is false).
> >
> >
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(u32 *)buf= =3D readl(priv->base + NPCM_RNGD_REG);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0retval +=3D= sizeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0buf +=3D si= zeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0max -=3D si= zeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0}
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_mark_last_busy((struct devic= e *)priv->rng.priv);
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_put_sync_autosuspend((struct= device *)priv->rng.priv);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return retval || !wait ? retval : -EIO;=
> > > +}
> > > +
> > > +static int npcm_rng_probe(struct platform_device *pdev)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv;
> > > +=C2=A0 =C2=A0 =C2=A0struct resource *res;
> > > +=C2=A0 =C2=A0 =C2=A0u32 quality;
> > > +=C2=A0 =C2=A0 =C2=A0int ret;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0priv =3D devm_kzalloc(&pdev->dev= , sizeof(*priv), GFP_KERNEL);
> > > +=C2=A0 =C2=A0 =C2=A0if (!priv)
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENO= MEM;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0res =3D platform_get_resource(pdev, IOR= ESOURCE_MEM, 0);
> > > +=C2=A0 =C2=A0 =C2=A0priv->base =3D devm_ioremap_resource= (&pdev->dev, res);
> > > +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(priv->base))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_= ERR(priv->base);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.name =3D pdev->name;
> > > +#ifndef CONFIG_PM
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.init =3D npcm_rng_init; > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.cleanup =3D npcm_rng_clean= up;
> > > +#endif
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.read =3D npcm_rng_read; > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.priv =3D (unsigned long)&a= mp;pdev->dev;
> > > +=C2=A0 =C2=A0 =C2=A0if (of_property_read_u32(pdev->dev.o= f_node, "quality", &quality))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->rn= g.quality =3D 1000;
> > > +=C2=A0 =C2=A0 =C2=A0else
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->rn= g.quality =3D quality;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_M1ROSEL, priv->base = + NPCM_RNGMODE_REG);
> > > +#ifndef CONFIG_PM
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ, priv->= ;base + NPCM_RNGCS_REG);
> > > +#else
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RN= G_ENABLE,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 priv->base + N= PCM_RNGCS_REG);
> > > +#endif
> >
> > If this initialization was moved to npcm_rng_init() then there wo= uld be
> > no need for the additional ifdefing. It would also get rid of the=
> > (potentially slow) readl calls on the PM wakeup path.
> >
>
> But when the Kernel have PM configuration than the priv->rng.init i= s not
> set and
> *add_early_randomness* function is called. for the *add_early_randomne= ss*
> success
> the hwrng need to enabled in the probe.

Sorry but I don't understand this reply.

When CONFIG_PM is enabled then the probe function does not currently set NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being called by
=C2=A0
Sorry maybe I miss understood, but when th= e=C2=A0 CONFIG_PM enabled so the NPCM_RNG_ENABLE sets (the code use ifndef = and not ifdef)
#ifndef CONFIG_PM
=C2=A0 =C2=A0 =C2=A0 = =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG);
#e= lse (CONFIG_PM enabled)
=C2=A0 =C2=A0 =C2=A0 =C2=A0writel(NPCM_RN= G_CLK_SET_25MHZ | NPCM_RNG_ENABLE,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 priv->base + NPCM_RNGCS_REG);
#endif=C2=A0

And the hwrng needed to be enabled to run=C2=A0add_earl= y_randomness function successfully.

If the hwr= ng driver will relay on PM logic to enable the hwrng will be disable when= =C2=A0add_early_randomness function is called.=C2=A0

<= /div>
the PM logic (as part of pm_runtime_get_sync() ).

Given the code *already* relies on npcm_rng_init() being called by the
PM logic why does it matter if additional init is put there?


Daniel.

Thanks,

Tomer=C2=A0
--00000000000049a5e8059230937d-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gmail.com (client-ip=2607:f8b0:4864:20::344; helo=mail-ot1-x344.google.com; envelope-from=tmaimon77@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="MTFxgu0c"; dkim-atps=neutral Received: from mail-ot1-x344.google.com (mail-ot1-x344.google.com [IPv6:2607:f8b0:4864:20::344]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 46SM890WNszDsGD for ; Tue, 10 Sep 2019 20:43:34 +1000 (AEST) Received: by mail-ot1-x344.google.com with SMTP id g16so2612058otp.8 for ; Tue, 10 Sep 2019 03:43:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=e4qpf9RwTSC8g32++XjeIesX488ki0t90aZuHQKdRUQ=; b=MTFxgu0c1/YkW/1kHguduXcEC9ZxrE/n1eFy+esVJNAklCOpHwap4QVIepEiLA2Ca7 cybHO9/sg0Lbf+QesLgSvf32LS3wbbDkxpopYun8K4GMDxtGhktduXNal2dTHukQfax3 cDxnbx0uLUCXmDrUgPvOa9VYhQ4b5AP3JroaHi3XSOSPLraPanPGFH7hT+gRfB3YWXqE O12XzybacgSgHdr/H8RWC2iqdn/4OMiySP27/BrBW2ng9K0s4BMoU1M4KF3Y7SOVewtN TtPYPhdHZ+/kvKezbRjyAYC7TeNS6nVhD09IxyaxHKtc+gvri7+hZJhLw/EwrEJnNSm8 TUDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=e4qpf9RwTSC8g32++XjeIesX488ki0t90aZuHQKdRUQ=; b=JqULHnDbLAOVJdGGbdigmhP7kdH+90c73+tRkGJ8KIuTv7qTZStFOLbN5qILtEvYpS hR9YAaITeuz18jZZhRxjzTQzv0z1esR9JUJKiT2BiJXLRCnPdMY6tfC7YK7XZ4aIIsaQ Q/YuhF7ek9k5m4kxTTd1k/EQ8F25q71ZHPIlxFfuqxOY1xsYyFF+uJpWkJKFKVRQzHlm c1PDnhTP/dIvFmQP6i2rJNiZmZRFAnb/gQ1ixEo/zqlqyfIuQdr4vMoEKpx/RM/bkbDQ 49Q/0eqp/OhNGKc2+nrMfGmEbxfATQJBLJkOYWmseyxtzgygplqQJsP+6atfQYnqzK85 uK5w== X-Gm-Message-State: APjAAAVC443LJCWK9Z67bSwjac7Q7oN8gzPs62MAGThM8DYP+uhstOzS a9/JCao17Z/7S0EJd8IglX5EOqmrpKM1veB72+w= X-Google-Smtp-Source: APXvYqwnLFnRYvtEUDmh2PXOpcd9MWvS4Crnitn7BmWapkl3cGTq3zuuRX0ISEln9HG9yOeWro1KufVDaFb2J4hPKZg= X-Received: by 2002:a05:6830:1d4e:: with SMTP id p14mr1981599oth.292.1568112210714; Tue, 10 Sep 2019 03:43:30 -0700 (PDT) MIME-Version: 1.0 References: <20190828162617.237398-1-tmaimon77@gmail.com> <20190828162617.237398-3-tmaimon77@gmail.com> <20190829104721.tnjk3bqt3cq6iagr@holly.lan> <20190909151033.f3inbbas4duzsmh5@holly.lan> In-Reply-To: <20190909151033.f3inbbas4duzsmh5@holly.lan> From: Tomer Maimon Date: Tue, 10 Sep 2019 13:52:35 +0300 Message-ID: Subject: Re: [PATCH v1 2/2] hwrng: npcm: add NPCM RNG driver To: Daniel Thompson Cc: mpm@selenic.com, herbert@gondor.apana.org.au, Arnd Bergmann , Greg KH , Rob Herring , Mark Rutland , Avi Fishman , Tali Perry , Patrick Venture , Nancy Yuen , Benjamin Fair , sumit.garg@linaro.org, jens.wiklander@linaro.org, vkoul@kernel.org, Thomas Gleixner , Joel Stanley , devicetree , Linux Kernel Mailing List , linux-crypto@vger.kernel.org, OpenBMC Maillist Content-Type: multipart/alternative; boundary="00000000000049a5e8059230937d" X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Sep 2019 10:43:38 -0000 --00000000000049a5e8059230937d Content-Type: text/plain; charset="UTF-8" Hi Daniel, Thanks for your prompt reply, On Mon, 9 Sep 2019 at 18:10, Daniel Thompson wrote: > On Mon, Sep 09, 2019 at 05:31:30PM +0300, Tomer Maimon wrote: > > Hi Daniel, > > > > appreciate your comments and sorry for the late reply > > > > On Thu, 29 Aug 2019 at 13:47, Daniel Thompson < > daniel.thompson@linaro.org> > > wrote: > > > > > On Wed, Aug 28, 2019 at 07:26:17PM +0300, Tomer Maimon wrote: > > > > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver. > > > > > > > > Signed-off-by: Tomer Maimon > > > > --- > > > > drivers/char/hw_random/Kconfig | 13 ++ > > > > drivers/char/hw_random/Makefile | 1 + > > > > drivers/char/hw_random/npcm-rng.c | 207 > ++++++++++++++++++++++++++++++ > > > > 3 files changed, 221 insertions(+) > > > > create mode 100644 drivers/char/hw_random/npcm-rng.c > > > > > > > > diff --git a/drivers/char/hw_random/npcm-rng.c > > > b/drivers/char/hw_random/npcm-rng.c > > > > new file mode 100644 > > > > index 000000000000..5b4b1b6cb362 > > > > --- /dev/null > > > > +++ b/drivers/char/hw_random/npcm-rng.c > > > > @@ -0,0 +1,207 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +// Copyright (c) 2019 Nuvoton Technology corporation. > > > > + > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#define NPCM_RNGCS_REG 0x00 /* Control and status > > > register */ > > > > +#define NPCM_RNGD_REG 0x04 /* Data register */ > > > > +#define NPCM_RNGMODE_REG 0x08 /* Mode register */ > > > > + > > > > +#define NPCM_RNG_CLK_SET_25MHZ GENMASK(4, 3) /* 20-25 MHz */ > > > > +#define NPCM_RNG_DATA_VALID BIT(1) > > > > +#define NPCM_RNG_ENABLE BIT(0) > > > > +#define NPCM_RNG_M1ROSEL BIT(1) > > > > + > > > > +#define NPCM_RNG_TIMEOUT_POLL 20 > > > > > > Might be better to define this in real-world units (such as > > > milliseconds) since the timeout is effectively the longest time the > > > hardware can take to generate 4 bytes. > > > > > > > + > > > > +#define to_npcm_rng(p) container_of(p, struct npcm_rng, rng) > > > > + > > > > +struct npcm_rng { > > > > + void __iomem *base; > > > > + struct hwrng rng; > > > > +}; > > > > + > > > > +static int npcm_rng_init(struct hwrng *rng) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + u32 val; > > > > + > > > > + val = readl(priv->base + NPCM_RNGCS_REG); > > > > + val |= NPCM_RNG_ENABLE; > > > > + writel(val, priv->base + NPCM_RNGCS_REG); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void npcm_rng_cleanup(struct hwrng *rng) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + u32 val; > > > > + > > > > + val = readl(priv->base + NPCM_RNGCS_REG); > > > > + val &= ~NPCM_RNG_ENABLE; > > > > + writel(val, priv->base + NPCM_RNGCS_REG); > > > > +} > > > > + > > > > +static bool npcm_rng_wait_ready(struct hwrng *rng, bool wait) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + int timeout_cnt = 0; > > > > + int ready; > > > > + > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) & > NPCM_RNG_DATA_VALID; > > > > + while ((ready == 0) && (timeout_cnt < NPCM_RNG_TIMEOUT_POLL)) { > > > > + usleep_range(500, 1000); > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) & > > > > + NPCM_RNG_DATA_VALID; > > > > + timeout_cnt++; > > > > + } > > > > + > > > > + return !!ready; > > > > +} > > > > > > This looks like an open-coded version of readl_poll_timeout()... better > > > to use the library function. > > > > > > Also the sleep looks a bit long to me. What is the generation rate of > > > the peripheral? Most RNG drivers have short intervals between data > > > generation so they use delays rather than sleeps (a.k.a. > > > readl_poll_timeout_atomic() ). > > > > the HWRNG generate byte of random data in a few milliseconds so it is > > better to use the sleep command. > > That's fine, just use readl_poll_timeout() then. > > > > > > + > > > > +static int npcm_rng_read(struct hwrng *rng, void *buf, size_t max, > bool > > > wait) > > > > +{ > > > > + struct npcm_rng *priv = to_npcm_rng(rng); > > > > + int retval = 0; > > > > + > > > > + pm_runtime_get_sync((struct device *)priv->rng.priv); > > > > + > > > > + while (max >= sizeof(u32)) { > > > > + if (!npcm_rng_wait_ready(rng, wait)) > > > > + break; > > > > > > The code as currently written does not honour the wait parameter (e.g. > > > it sleeps even when wait is false). > > > > > > > > > > + > > > > + *(u32 *)buf = readl(priv->base + NPCM_RNGD_REG); > > > > + retval += sizeof(u32); > > > > + buf += sizeof(u32); > > > > + max -= sizeof(u32); > > > > + } > > > > + > > > > + pm_runtime_mark_last_busy((struct device *)priv->rng.priv); > > > > + pm_runtime_put_sync_autosuspend((struct device > *)priv->rng.priv); > > > > + > > > > + return retval || !wait ? retval : -EIO; > > > > +} > > > > + > > > > +static int npcm_rng_probe(struct platform_device *pdev) > > > > +{ > > > > + struct npcm_rng *priv; > > > > + struct resource *res; > > > > + u32 quality; > > > > + int ret; > > > > + > > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > > > + if (!priv) > > > > + return -ENOMEM; > > > > + > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + priv->base = devm_ioremap_resource(&pdev->dev, res); > > > > + if (IS_ERR(priv->base)) > > > > + return PTR_ERR(priv->base); > > > > + > > > > + priv->rng.name = pdev->name; > > > > +#ifndef CONFIG_PM > > > > + priv->rng.init = npcm_rng_init; > > > > + priv->rng.cleanup = npcm_rng_cleanup; > > > > +#endif > > > > + priv->rng.read = npcm_rng_read; > > > > + priv->rng.priv = (unsigned long)&pdev->dev; > > > > + if (of_property_read_u32(pdev->dev.of_node, "quality", > &quality)) > > > > + priv->rng.quality = 1000; > > > > + else > > > > + priv->rng.quality = quality; > > > > + > > > > + writel(NPCM_RNG_M1ROSEL, priv->base + NPCM_RNGMODE_REG); > > > > +#ifndef CONFIG_PM > > > > + writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG); > > > > +#else > > > > + writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE, > > > > + priv->base + NPCM_RNGCS_REG); > > > > +#endif > > > > > > If this initialization was moved to npcm_rng_init() then there would be > > > no need for the additional ifdefing. It would also get rid of the > > > (potentially slow) readl calls on the PM wakeup path. > > > > > > > But when the Kernel have PM configuration than the priv->rng.init is not > > set and > > *add_early_randomness* function is called. for the *add_early_randomness* > > success > > the hwrng need to enabled in the probe. > > Sorry but I don't understand this reply. > > When CONFIG_PM is enabled then the probe function does not currently set > NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being called by > Sorry maybe I miss understood, but when the CONFIG_PM enabled so the NPCM_RNG_ENABLE sets (the code use ifndef and not ifdef) *#ifndef CONFIG_PM* writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG); #else (*CONFIG_PM enabled*) writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE, priv->base + NPCM_RNGCS_REG); #endif And the hwrng needed to be enabled to run *add_early_randomness *function successfully. If the hwrng driver will relay on PM logic to enable the hwrng will be disable when *add_early_randomness *function is called. the PM logic (as part of pm_runtime_get_sync() ). > > Given the code *already* relies on npcm_rng_init() being called by the > PM logic why does it matter if additional init is put there? > > > Daniel. > Thanks, Tomer --00000000000049a5e8059230937d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Dani= el,

Thanks for your prompt reply,



On Mon, 9 Sep 2019 at 18:10, Daniel Thompson <daniel.thompson@linaro.org> wrote:<= br>
On Mon, Sep 09, = 2019 at 05:31:30PM +0300, Tomer Maimon wrote:
> Hi Daniel,
>
> appreciate your comments and sorry for the late reply
>
> On Thu, 29 Aug 2019 at 13:47, Daniel Thompson <daniel.thompson@linaro.org&= gt;
> wrote:
>
> > On Wed, Aug 28, 2019 at 07:26:17PM +0300, Tomer Maimon wrote:
> > > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver. > > >
> > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > > ---
> > >=C2=A0 drivers/char/hw_random/Kconfig=C2=A0 =C2=A0 |=C2=A0 13= ++
> > >=C2=A0 drivers/char/hw_random/Makefile=C2=A0 =C2=A0|=C2=A0 = =C2=A01 +
> > >=C2=A0 drivers/char/hw_random/npcm-rng.c | 207 ++++++++++++++= ++++++++++++++++
> > >=C2=A0 3 files changed, 221 insertions(+)
> > >=C2=A0 create mode 100644 drivers/char/hw_random/npcm-rng.c > > >
> > > diff --git a/drivers/char/hw_random/npcm-rng.c
> > b/drivers/char/hw_random/npcm-rng.c
> > > new file mode 100644
> > > index 000000000000..5b4b1b6cb362
> > > --- /dev/null
> > > +++ b/drivers/char/hw_random/npcm-rng.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2019 Nuvoton Technology corporation.
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/init.h>
> > > +#include <linux/random.h>
> > > +#include <linux/err.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/hw_random.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +#define NPCM_RNGCS_REG=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A00x00=C2=A0 =C2=A0 /* Control and status
> > register */
> > > +#define NPCM_RNGD_REG=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 0x04=C2=A0 =C2=A0 /* Data register */
> > > +#define NPCM_RNGMODE_REG=C2=A0 =C2=A0 =C2=A00x08=C2=A0 =C2= =A0 /* Mode register */
> > > +
> > > +#define NPCM_RNG_CLK_SET_25MHZ=C2=A0 =C2=A0 =C2=A0 =C2=A0GE= NMASK(4, 3) /* 20-25 MHz */
> > > +#define NPCM_RNG_DATA_VALID=C2=A0 BIT(1)
> > > +#define NPCM_RNG_ENABLE=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 BIT(0)
> > > +#define NPCM_RNG_M1ROSEL=C2=A0 =C2=A0 =C2=A0BIT(1)
> > > +
> > > +#define NPCM_RNG_TIMEOUT_POLL=C2=A0 =C2=A0 =C2=A0 =C2=A0 20=
> >
> > Might be better to define this in real-world units (such as
> > milliseconds) since the timeout is effectively the longest time t= he
> > hardware can take to generate 4 bytes.
> >
> > > +
> > > +#define to_npcm_rng(p)=C2=A0 =C2=A0 =C2=A0 =C2=A0container_= of(p, struct npcm_rng, rng)
> > > +
> > > +struct npcm_rng {
> > > +=C2=A0 =C2=A0 =C2=A0void __iomem *base;
> > > +=C2=A0 =C2=A0 =C2=A0struct hwrng rng;
> > > +};
> > > +
> > > +static int npcm_rng_init(struct hwrng *rng)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base + NPCM_RNGC= S_REG);
> > > +=C2=A0 =C2=A0 =C2=A0val |=3D NPCM_RNG_ENABLE;
> > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + NPCM_RNGCS_= REG);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return 0;
> > > +}
> > > +
> > > +static void npcm_rng_cleanup(struct hwrng *rng)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base + NPCM_RNGC= S_REG);
> > > +=C2=A0 =C2=A0 =C2=A0val &=3D ~NPCM_RNG_ENABLE;
> > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + NPCM_RNGCS_= REG);
> > > +}
> > > +
> > > +static bool npcm_rng_wait_ready(struct hwrng *rng, bool wai= t)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0int timeout_cnt =3D 0;
> > > +=C2=A0 =C2=A0 =C2=A0int ready;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0ready =3D readl(priv->base + NPCM_RN= GCS_REG) & NPCM_RNG_DATA_VALID;
> > > +=C2=A0 =C2=A0 =C2=A0while ((ready =3D=3D 0) && (tim= eout_cnt < NPCM_RNG_TIMEOUT_POLL)) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0usleep_rang= e(500, 1000);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ready =3D r= eadl(priv->base + NPCM_RNGCS_REG) &
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0NPCM_RNG_DATA_VALID;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0timeout_cnt= ++;
> > > +=C2=A0 =C2=A0 =C2=A0}
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return !!ready;
> > > +}
> >
> > This looks like an open-coded version of readl_poll_timeout()... = better
> > to use the library function.
> >
> > Also the sleep looks a bit long to me. What is the generation rat= e of
> > the peripheral? Most RNG drivers have short intervals between dat= a
> > generation so they use delays rather than sleeps (a.k.a.
> > readl_poll_timeout_atomic() ).
>
> the HWRNG generate byte of random data in a few milliseconds so it is<= br> > better to use the sleep command.

That's fine, just use readl_poll_timeout() then.


> > > +
> > > +static int npcm_rng_read(struct hwrng *rng, void *buf, size= _t max, bool
> > wait)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_npcm_rng(r= ng);
> > > +=C2=A0 =C2=A0 =C2=A0int retval =3D 0;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_get_sync((struct device *)pr= iv->rng.priv);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0while (max >=3D sizeof(u32)) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!npcm_r= ng_wait_ready(rng, wait))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0break;
> >
> > The code as currently written does not honour the wait parameter = (e.g.
> > it sleeps even when wait is false).
> >
> >
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*(u32 *)buf= =3D readl(priv->base + NPCM_RNGD_REG);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0retval +=3D= sizeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0buf +=3D si= zeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0max -=3D si= zeof(u32);
> > > +=C2=A0 =C2=A0 =C2=A0}
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_mark_last_busy((struct devic= e *)priv->rng.priv);
> > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_put_sync_autosuspend((struct= device *)priv->rng.priv);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0return retval || !wait ? retval : -EIO;=
> > > +}
> > > +
> > > +static int npcm_rng_probe(struct platform_device *pdev)
> > > +{
> > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv;
> > > +=C2=A0 =C2=A0 =C2=A0struct resource *res;
> > > +=C2=A0 =C2=A0 =C2=A0u32 quality;
> > > +=C2=A0 =C2=A0 =C2=A0int ret;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0priv =3D devm_kzalloc(&pdev->dev= , sizeof(*priv), GFP_KERNEL);
> > > +=C2=A0 =C2=A0 =C2=A0if (!priv)
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENO= MEM;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0res =3D platform_get_resource(pdev, IOR= ESOURCE_MEM, 0);
> > > +=C2=A0 =C2=A0 =C2=A0priv->base =3D devm_ioremap_resource= (&pdev->dev, res);
> > > +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(priv->base))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_= ERR(priv->base);
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.name =3D pdev->name;
> > > +#ifndef CONFIG_PM
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.init =3D npcm_rng_init; > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.cleanup =3D npcm_rng_clean= up;
> > > +#endif
> > > +=C2=A0 =C2=A0 =C2=A0priv->rng.read =3D npcm_rng_read; > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.priv =3D (unsigned long)&a= mp;pdev->dev;
> > > +=C2=A0 =C2=A0 =C2=A0if (of_property_read_u32(pdev->dev.o= f_node, "quality", &quality))
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->rn= g.quality =3D 1000;
> > > +=C2=A0 =C2=A0 =C2=A0else
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->rn= g.quality =3D quality;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_M1ROSEL, priv->base = + NPCM_RNGMODE_REG);
> > > +#ifndef CONFIG_PM
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ, priv->= ;base + NPCM_RNGCS_REG);
> > > +#else
> > > +=C2=A0 =C2=A0 =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RN= G_ENABLE,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 priv->base + N= PCM_RNGCS_REG);
> > > +#endif
> >
> > If this initialization was moved to npcm_rng_init() then there wo= uld be
> > no need for the additional ifdefing. It would also get rid of the=
> > (potentially slow) readl calls on the PM wakeup path.
> >
>
> But when the Kernel have PM configuration than the priv->rng.init i= s not
> set and
> *add_early_randomness* function is called. for the *add_early_randomne= ss*
> success
> the hwrng need to enabled in the probe.

Sorry but I don't understand this reply.

When CONFIG_PM is enabled then the probe function does not currently set NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being called by
=C2=A0
Sorry maybe I miss understood, but when th= e=C2=A0 CONFIG_PM enabled so the NPCM_RNG_ENABLE sets (the code use ifndef = and not ifdef)
#ifndef CONFIG_PM
=C2=A0 =C2=A0 =C2=A0 = =C2=A0writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG);
#e= lse (CONFIG_PM enabled)
=C2=A0 =C2=A0 =C2=A0 =C2=A0writel(NPCM_RN= G_CLK_SET_25MHZ | NPCM_RNG_ENABLE,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 priv->base + NPCM_RNGCS_REG);
#endif=C2=A0

And the hwrng needed to be enabled to run=C2=A0add_earl= y_randomness function successfully.

If the hwr= ng driver will relay on PM logic to enable the hwrng will be disable when= =C2=A0add_early_randomness function is called.=C2=A0

<= /div>
the PM logic (as part of pm_runtime_get_sync() ).

Given the code *already* relies on npcm_rng_init() being called by the
PM logic why does it matter if additional init is put there?


Daniel.

Thanks,

Tomer=C2=A0
--00000000000049a5e8059230937d--