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> <20190910112922.xqho33smx6zsmank@holly.lan> In-Reply-To: <20190910112922.xqho33smx6zsmank@holly.lan> From: Tomer Maimon Date: Tue, 10 Sep 2019 14:59:57 +0300 Message-ID: Subject: Re: [PATCH v1 2/2] hwrng: npcm: add NPCM RNG driver Content-Type: multipart/alternative; boundary="000000000000424307059231845f" 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: --000000000000424307059231845f Content-Type: text/plain; charset="UTF-8" On Tue, 10 Sep 2019 at 14:29, Daniel Thompson wrote: > On Tue, Sep 10, 2019 at 01:52:35PM +0300, Tomer Maimon wrote: > > 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() ). > > I ended up reading my mail out of order and replied to the v2 patch. > > The question is *why* the driver doesn't resume properly when adding > early randomness! I think it is because the hwrng_register() is being > called before PM runtime is enabled. > > probably, but I am not sure it will be right to enable the PM runtime before hwrng_register() is called. I will double check it. > > Daniel. > Thanks, Tomer --000000000000424307059231845f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, 10 Sep 2019 at 14:29, Daniel = Thompson <daniel.thompson@= linaro.org> wrote:
On Tue, Sep 10, 2019 at 01:52:35PM +0300, Tomer Maimon wrote: > Hi Daniel,
>
> Thanks for your prompt reply,
>
>
>
> On Mon, 9 Sep 2019 at 18:10, Daniel Thompson <daniel.thompson@linaro.org&g= t;
> 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 <
> > d= aniel.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 <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/np= cm-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 corporat= ion.
> > > > > +
> > > > > +#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=A0GENMASK(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 (suc= h as
> > > > milliseconds) since the timeout is effectively the long= est time the
> > > > hardware can take to generate 4 bytes.
> > > >
> > > > > +
> > > > > +#define to_npcm_rng(p)=C2=A0 =C2=A0 =C2=A0 =C2=A0= container_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(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base += NPCM_RNGCS_REG);
> > > > > +=C2=A0 =C2=A0 =C2=A0val |=3D NPCM_RNG_ENABLE;
> > > > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + N= PCM_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(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base += NPCM_RNGCS_REG);
> > > > > +=C2=A0 =C2=A0 =C2=A0val &=3D ~NPCM_RNG_ENABLE= ;
> > > > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + N= PCM_RNGCS_REG);
> > > > > +}
> > > > > +
> > > > > +static bool npcm_rng_wait_ready(struct hwrng *rng= , bool wait)
> > > > > +{
> > > > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_= npcm_rng(rng);
> > > > > +=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_RNGCS_REG) &
> > NPCM_RNG_DATA_VALID;
> > > > > +=C2=A0 =C2=A0 =C2=A0while ((ready =3D=3D 0) &= & (timeout_cnt < NPCM_RNG_TIMEOUT_POLL)) {
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0u= sleep_range(500, 1000);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r= eady =3D readl(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=A0t= imeout_cnt++;
> > > > > +=C2=A0 =C2=A0 =C2=A0}
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0return !!ready;
> > > > > +}
> > > >
> > > > This looks like an open-coded version of readl_poll_tim= eout()... better
> > > > to use the library function.
> > > >
> > > > Also the sleep looks a bit long to me. What is the gene= ration rate of
> > > > the peripheral? Most RNG drivers have short intervals b= etween 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)
> > > > > +{
> > > > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_= npcm_rng(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0int retval =3D 0;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_get_sync((struct d= evice *)priv->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=A0i= f (!npcm_rng_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=A0r= etval +=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0b= uf +=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m= ax -=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0}
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_mark_last_busy((st= ruct device *)priv->rng.priv);
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_put_sync_autosuspe= nd((struct device
> > *)priv->rng.priv);
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0return retval || !wait ? retv= al : -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(&pd= ev->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=A0r= eturn -ENOMEM;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0res =3D platform_get_resource= (pdev, IORESOURCE_MEM, 0);
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->base =3D devm_iorema= p_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=A0r= eturn 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_rn= g_init;
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.cleanup =3D npcm= _rng_cleanup;
> > > > > +#endif
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.read =3D npcm_rn= g_read;
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.priv =3D (unsign= ed long)&pdev->dev;
> > > > > +=C2=A0 =C2=A0 =C2=A0if (of_property_read_u32(pdev= ->dev.of_node, "quality",
> > &quality))
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p= riv->rng.quality =3D 1000;
> > > > > +=C2=A0 =C2=A0 =C2=A0else
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p= riv->rng.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_RNG_ENABLE,
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 priv-&g= t;base + NPCM_RNGCS_REG);
> > > > > +#endif
> > > >
> > > > If this initialization was moved to npcm_rng_init() the= n 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_earl= y_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 curren= tly set
> > NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being calle= d by
> >
>
> Sorry maybe I miss understood, but when the=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=A0 writel(NPCM_RNG_CLK_SET_25MHZ, priv->bas= e + NPCM_RNGCS_REG);
> #else (*CONFIG_PM enabled*)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_EN= ABLE,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->base + = NPCM_RNGCS_REG);
> #endif
>
> And the hwrng needed to be enabled to run *add_early_randomness *funct= ion
> 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() ).

I ended up reading my mail out of order and replied to the v2 patch.

The question is *why* the driver doesn't resume properly when adding early randomness! I think it is because the hwrng_register() is being
called before PM runtime is enabled.


probably, but I am not sure it will be= right to enable the PM runtime before=C2=A0 hwrng_register() is called.

I will double check it.

Daniel.

Thanks,

Tomer=C2=A0
--000000000000424307059231845f-- 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::341; helo=mail-ot1-x341.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="NNfPbbZB"; dkim-atps=neutral Received: from mail-ot1-x341.google.com (mail-ot1-x341.google.com [IPv6:2607:f8b0:4864:20::341]) (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 46SNdt0GbdzDsMN for ; Tue, 10 Sep 2019 21:50:57 +1000 (AEST) Received: by mail-ot1-x341.google.com with SMTP id b2so18053583otq.10 for ; Tue, 10 Sep 2019 04:50:57 -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=GYIia8uCIiJUVALiJSYwnd+aMivWPw4F+FI/mAlUETM=; b=NNfPbbZByytYdL7hE8lbS6XiZMiiR9JnX22cIt1RuSvsbaVSzTOci0qAFyAe9bSrBa W4czLtUOxpq/11XPqiU6agH6LPm4awGloY4hbXk6YbbrHy3xJUOxPm5/ag7owarjVcU7 ou3GChuI1vSxW3rXSNyJuZiwxKmpZa3HQSOG9LrR9Z5EWcmUv04fO5DfWwHCLVrQZbw+ VfIZMgB4IsCYVF63n+y5jXcl4zvAdRpicf/Qecvg4RZR7ns0Ap8xdbpFNbRiPoeWotd/ OjXs/vWM/tG1UM9FqHW0K8cNhUV3mOt5l4FtimsKf2FGu7MozrVy04a4YAJELzhRT5o+ GMnw== 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=GYIia8uCIiJUVALiJSYwnd+aMivWPw4F+FI/mAlUETM=; b=G6MNXHohpE2Jy1btgryVRDJV/bZRDifoU8uSAQ7TZnL9bB4xLAAdO0PtRz6hgaoj2+ VXJEMtoJ5PQ+lDQVcnr9OQo0LfwSS6gaKSUjl6wLgQDE0pm8h4oNlJTYjtviIb+F4S0U /Va8BuBCxNTue/KARbt/RxioSvIuQ9wt1Uk09L9Z+loTLdAkMr5j+j7yH0BahUEMcgIH 0Dw0Ijk09z4VFw32mt7AZLizX67MV2mwj23YnfyhiYS1e9G4U3ccaTPXf070Dx4+KnqJ 8rQysjmo7A9OmHsePE15scK5Tg7dtwU3zc0nah6eymJHS6xm2h4jWb4fTJDtsAMfc6WD n8fA== X-Gm-Message-State: APjAAAW9dXemZ1iGlIlvEZYQsD4eyRkN96FGUI/T1/gIwxDDjToxozN5 KwT75vgof5faMD71tOIT5GjG2K06wGzzuDChzk2rexCdftY= X-Google-Smtp-Source: APXvYqxoNHuVQjCCZjp45N9ShgXtNya5jd5d8znLzFcjN5AmTsP0SEGRco8qabJB0Ij6k1Vln49R5gdAvUDLrA6u8U0= X-Received: by 2002:a05:6830:1d4e:: with SMTP id p14mr2322802oth.292.1568116253539; Tue, 10 Sep 2019 04:50:53 -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> <20190910112922.xqho33smx6zsmank@holly.lan> In-Reply-To: <20190910112922.xqho33smx6zsmank@holly.lan> From: Tomer Maimon Date: Tue, 10 Sep 2019 14:59:57 +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="000000000000424307059231845f" 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 11:50:58 -0000 --000000000000424307059231845f Content-Type: text/plain; charset="UTF-8" On Tue, 10 Sep 2019 at 14:29, Daniel Thompson wrote: > On Tue, Sep 10, 2019 at 01:52:35PM +0300, Tomer Maimon wrote: > > 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() ). > > I ended up reading my mail out of order and replied to the v2 patch. > > The question is *why* the driver doesn't resume properly when adding > early randomness! I think it is because the hwrng_register() is being > called before PM runtime is enabled. > > probably, but I am not sure it will be right to enable the PM runtime before hwrng_register() is called. I will double check it. > > Daniel. > Thanks, Tomer --000000000000424307059231845f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, 10 Sep 2019 at 14:29, Daniel = Thompson <daniel.thompson@= linaro.org> wrote:
On Tue, Sep 10, 2019 at 01:52:35PM +0300, Tomer Maimon wrote: > Hi Daniel,
>
> Thanks for your prompt reply,
>
>
>
> On Mon, 9 Sep 2019 at 18:10, Daniel Thompson <daniel.thompson@linaro.org&g= t;
> 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 <
> > d= aniel.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 <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/np= cm-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 corporat= ion.
> > > > > +
> > > > > +#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=A0GENMASK(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 (suc= h as
> > > > milliseconds) since the timeout is effectively the long= est time the
> > > > hardware can take to generate 4 bytes.
> > > >
> > > > > +
> > > > > +#define to_npcm_rng(p)=C2=A0 =C2=A0 =C2=A0 =C2=A0= container_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(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base += NPCM_RNGCS_REG);
> > > > > +=C2=A0 =C2=A0 =C2=A0val |=3D NPCM_RNG_ENABLE;
> > > > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + N= PCM_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(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0u32 val;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0val =3D readl(priv->base += NPCM_RNGCS_REG);
> > > > > +=C2=A0 =C2=A0 =C2=A0val &=3D ~NPCM_RNG_ENABLE= ;
> > > > > +=C2=A0 =C2=A0 =C2=A0writel(val, priv->base + N= PCM_RNGCS_REG);
> > > > > +}
> > > > > +
> > > > > +static bool npcm_rng_wait_ready(struct hwrng *rng= , bool wait)
> > > > > +{
> > > > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_= npcm_rng(rng);
> > > > > +=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_RNGCS_REG) &
> > NPCM_RNG_DATA_VALID;
> > > > > +=C2=A0 =C2=A0 =C2=A0while ((ready =3D=3D 0) &= & (timeout_cnt < NPCM_RNG_TIMEOUT_POLL)) {
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0u= sleep_range(500, 1000);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r= eady =3D readl(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=A0t= imeout_cnt++;
> > > > > +=C2=A0 =C2=A0 =C2=A0}
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0return !!ready;
> > > > > +}
> > > >
> > > > This looks like an open-coded version of readl_poll_tim= eout()... better
> > > > to use the library function.
> > > >
> > > > Also the sleep looks a bit long to me. What is the gene= ration rate of
> > > > the peripheral? Most RNG drivers have short intervals b= etween 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)
> > > > > +{
> > > > > +=C2=A0 =C2=A0 =C2=A0struct npcm_rng *priv =3D to_= npcm_rng(rng);
> > > > > +=C2=A0 =C2=A0 =C2=A0int retval =3D 0;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_get_sync((struct d= evice *)priv->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=A0i= f (!npcm_rng_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=A0r= etval +=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0b= uf +=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0m= ax -=3D sizeof(u32);
> > > > > +=C2=A0 =C2=A0 =C2=A0}
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_mark_last_busy((st= ruct device *)priv->rng.priv);
> > > > > +=C2=A0 =C2=A0 =C2=A0pm_runtime_put_sync_autosuspe= nd((struct device
> > *)priv->rng.priv);
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0return retval || !wait ? retv= al : -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(&pd= ev->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=A0r= eturn -ENOMEM;
> > > > > +
> > > > > +=C2=A0 =C2=A0 =C2=A0res =3D platform_get_resource= (pdev, IORESOURCE_MEM, 0);
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->base =3D devm_iorema= p_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=A0r= eturn 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_rn= g_init;
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.cleanup =3D npcm= _rng_cleanup;
> > > > > +#endif
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.read =3D npcm_rn= g_read;
> > > > > +=C2=A0 =C2=A0 =C2=A0priv->rng.priv =3D (unsign= ed long)&pdev->dev;
> > > > > +=C2=A0 =C2=A0 =C2=A0if (of_property_read_u32(pdev= ->dev.of_node, "quality",
> > &quality))
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p= riv->rng.quality =3D 1000;
> > > > > +=C2=A0 =C2=A0 =C2=A0else
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p= riv->rng.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_RNG_ENABLE,
> > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 priv-&g= t;base + NPCM_RNGCS_REG);
> > > > > +#endif
> > > >
> > > > If this initialization was moved to npcm_rng_init() the= n 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_earl= y_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 curren= tly set
> > NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being calle= d by
> >
>
> Sorry maybe I miss understood, but when the=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=A0 writel(NPCM_RNG_CLK_SET_25MHZ, priv->bas= e + NPCM_RNGCS_REG);
> #else (*CONFIG_PM enabled*)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_EN= ABLE,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0priv->base + = NPCM_RNGCS_REG);
> #endif
>
> And the hwrng needed to be enabled to run *add_early_randomness *funct= ion
> 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() ).

I ended up reading my mail out of order and replied to the v2 patch.

The question is *why* the driver doesn't resume properly when adding early randomness! I think it is because the hwrng_register() is being
called before PM runtime is enabled.


probably, but I am not sure it will be= right to enable the PM runtime before=C2=A0 hwrng_register() is called.

I will double check it.

Daniel.

Thanks,

Tomer=C2=A0
--000000000000424307059231845f--