<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 10 Sep 2019 at 14:29, Daniel Thompson <<a href="mailto:daniel.thompson@linaro.org">daniel.thompson@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Sep 10, 2019 at 01:52:35PM +0300, Tomer Maimon wrote:<br>
> Hi Daniel,<br>
> <br>
> Thanks for your prompt reply,<br>
> <br>
> <br>
> <br>
> On Mon, 9 Sep 2019 at 18:10, Daniel Thompson <<a href="mailto:daniel.thompson@linaro.org" target="_blank">daniel.thompson@linaro.org</a>><br>
> wrote:<br>
> <br>
> > On Mon, Sep 09, 2019 at 05:31:30PM +0300, Tomer Maimon wrote:<br>
> > > Hi Daniel,<br>
> > ><br>
> > > appreciate your comments and sorry for the late reply<br>
> > ><br>
> > > On Thu, 29 Aug 2019 at 13:47, Daniel Thompson <<br>
> > <a href="mailto:daniel.thompson@linaro.org" target="_blank">daniel.thompson@linaro.org</a>><br>
> > > wrote:<br>
> > ><br>
> > > > On Wed, Aug 28, 2019 at 07:26:17PM +0300, Tomer Maimon wrote:<br>
> > > > > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver.<br>
> > > > ><br>
> > > > > Signed-off-by: Tomer Maimon <<a href="mailto:tmaimon77@gmail.com" target="_blank">tmaimon77@gmail.com</a>><br>
> > > > > ---<br>
> > > > > drivers/char/hw_random/Kconfig | 13 ++<br>
> > > > > drivers/char/hw_random/Makefile | 1 +<br>
> > > > > drivers/char/hw_random/npcm-rng.c | 207<br>
> > ++++++++++++++++++++++++++++++<br>
> > > > > 3 files changed, 221 insertions(+)<br>
> > > > > create mode 100644 drivers/char/hw_random/npcm-rng.c<br>
> > > > ><br>
> > > > > diff --git a/drivers/char/hw_random/npcm-rng.c<br>
> > > > b/drivers/char/hw_random/npcm-rng.c<br>
> > > > > new file mode 100644<br>
> > > > > index 000000000000..5b4b1b6cb362<br>
> > > > > --- /dev/null<br>
> > > > > +++ b/drivers/char/hw_random/npcm-rng.c<br>
> > > > > @@ -0,0 +1,207 @@<br>
> > > > > +// SPDX-License-Identifier: GPL-2.0<br>
> > > > > +// Copyright (c) 2019 Nuvoton Technology corporation.<br>
> > > > > +<br>
> > > > > +#include <linux/kernel.h><br>
> > > > > +#include <linux/module.h><br>
> > > > > +#include <linux/io.h><br>
> > > > > +#include <linux/iopoll.h><br>
> > > > > +#include <linux/init.h><br>
> > > > > +#include <linux/random.h><br>
> > > > > +#include <linux/err.h><br>
> > > > > +#include <linux/platform_device.h><br>
> > > > > +#include <linux/hw_random.h><br>
> > > > > +#include <linux/delay.h><br>
> > > > > +#include <linux/of_irq.h><br>
> > > > > +#include <linux/pm_runtime.h><br>
> > > > > +<br>
> > > > > +#define NPCM_RNGCS_REG 0x00 /* Control and status<br>
> > > > register */<br>
> > > > > +#define NPCM_RNGD_REG 0x04 /* Data register */<br>
> > > > > +#define NPCM_RNGMODE_REG 0x08 /* Mode register */<br>
> > > > > +<br>
> > > > > +#define NPCM_RNG_CLK_SET_25MHZ GENMASK(4, 3) /* 20-25 MHz */<br>
> > > > > +#define NPCM_RNG_DATA_VALID BIT(1)<br>
> > > > > +#define NPCM_RNG_ENABLE BIT(0)<br>
> > > > > +#define NPCM_RNG_M1ROSEL BIT(1)<br>
> > > > > +<br>
> > > > > +#define NPCM_RNG_TIMEOUT_POLL 20<br>
> > > ><br>
> > > > Might be better to define this in real-world units (such as<br>
> > > > milliseconds) since the timeout is effectively the longest time the<br>
> > > > hardware can take to generate 4 bytes.<br>
> > > ><br>
> > > > > +<br>
> > > > > +#define to_npcm_rng(p) container_of(p, struct npcm_rng, rng)<br>
> > > > > +<br>
> > > > > +struct npcm_rng {<br>
> > > > > + void __iomem *base;<br>
> > > > > + struct hwrng rng;<br>
> > > > > +};<br>
> > > > > +<br>
> > > > > +static int npcm_rng_init(struct hwrng *rng)<br>
> > > > > +{<br>
> > > > > + struct npcm_rng *priv = to_npcm_rng(rng);<br>
> > > > > + u32 val;<br>
> > > > > +<br>
> > > > > + val = readl(priv->base + NPCM_RNGCS_REG);<br>
> > > > > + val |= NPCM_RNG_ENABLE;<br>
> > > > > + writel(val, priv->base + NPCM_RNGCS_REG);<br>
> > > > > +<br>
> > > > > + return 0;<br>
> > > > > +}<br>
> > > > > +<br>
> > > > > +static void npcm_rng_cleanup(struct hwrng *rng)<br>
> > > > > +{<br>
> > > > > + struct npcm_rng *priv = to_npcm_rng(rng);<br>
> > > > > + u32 val;<br>
> > > > > +<br>
> > > > > + val = readl(priv->base + NPCM_RNGCS_REG);<br>
> > > > > + val &= ~NPCM_RNG_ENABLE;<br>
> > > > > + writel(val, priv->base + NPCM_RNGCS_REG);<br>
> > > > > +}<br>
> > > > > +<br>
> > > > > +static bool npcm_rng_wait_ready(struct hwrng *rng, bool wait)<br>
> > > > > +{<br>
> > > > > + struct npcm_rng *priv = to_npcm_rng(rng);<br>
> > > > > + int timeout_cnt = 0;<br>
> > > > > + int ready;<br>
> > > > > +<br>
> > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) &<br>
> > NPCM_RNG_DATA_VALID;<br>
> > > > > + while ((ready == 0) && (timeout_cnt < NPCM_RNG_TIMEOUT_POLL)) {<br>
> > > > > + usleep_range(500, 1000);<br>
> > > > > + ready = readl(priv->base + NPCM_RNGCS_REG) &<br>
> > > > > + NPCM_RNG_DATA_VALID;<br>
> > > > > + timeout_cnt++;<br>
> > > > > + }<br>
> > > > > +<br>
> > > > > + return !!ready;<br>
> > > > > +}<br>
> > > ><br>
> > > > This looks like an open-coded version of readl_poll_timeout()... better<br>
> > > > to use the library function.<br>
> > > ><br>
> > > > Also the sleep looks a bit long to me. What is the generation rate of<br>
> > > > the peripheral? Most RNG drivers have short intervals between data<br>
> > > > generation so they use delays rather than sleeps (a.k.a.<br>
> > > > readl_poll_timeout_atomic() ).<br>
> > ><br>
> > > the HWRNG generate byte of random data in a few milliseconds so it is<br>
> > > better to use the sleep command.<br>
> ><br>
> > That's fine, just use readl_poll_timeout() then.<br>
> ><br>
> ><br>
> > > > > +<br>
> > > > > +static int npcm_rng_read(struct hwrng *rng, void *buf, size_t max,<br>
> > bool<br>
> > > > wait)<br>
> > > > > +{<br>
> > > > > + struct npcm_rng *priv = to_npcm_rng(rng);<br>
> > > > > + int retval = 0;<br>
> > > > > +<br>
> > > > > + pm_runtime_get_sync((struct device *)priv->rng.priv);<br>
> > > > > +<br>
> > > > > + while (max >= sizeof(u32)) {<br>
> > > > > + if (!npcm_rng_wait_ready(rng, wait))<br>
> > > > > + break;<br>
> > > ><br>
> > > > The code as currently written does not honour the wait parameter (e.g.<br>
> > > > it sleeps even when wait is false).<br>
> > > ><br>
> > > ><br>
> > > > > +<br>
> > > > > + *(u32 *)buf = readl(priv->base + NPCM_RNGD_REG);<br>
> > > > > + retval += sizeof(u32);<br>
> > > > > + buf += sizeof(u32);<br>
> > > > > + max -= sizeof(u32);<br>
> > > > > + }<br>
> > > > > +<br>
> > > > > + pm_runtime_mark_last_busy((struct device *)priv->rng.priv);<br>
> > > > > + pm_runtime_put_sync_autosuspend((struct device<br>
> > *)priv->rng.priv);<br>
> > > > > +<br>
> > > > > + return retval || !wait ? retval : -EIO;<br>
> > > > > +}<br>
> > > > > +<br>
> > > > > +static int npcm_rng_probe(struct platform_device *pdev)<br>
> > > > > +{<br>
> > > > > + struct npcm_rng *priv;<br>
> > > > > + struct resource *res;<br>
> > > > > + u32 quality;<br>
> > > > > + int ret;<br>
> > > > > +<br>
> > > > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);<br>
> > > > > + if (!priv)<br>
> > > > > + return -ENOMEM;<br>
> > > > > +<br>
> > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);<br>
> > > > > + priv->base = devm_ioremap_resource(&pdev->dev, res);<br>
> > > > > + if (IS_ERR(priv->base))<br>
> > > > > + return PTR_ERR(priv->base);<br>
> > > > > +<br>
> > > > > + priv-><a href="http://rng.name" rel="noreferrer" target="_blank">rng.name</a> = pdev->name;<br>
> > > > > +#ifndef CONFIG_PM<br>
> > > > > + priv->rng.init = npcm_rng_init;<br>
> > > > > + priv->rng.cleanup = npcm_rng_cleanup;<br>
> > > > > +#endif<br>
> > > > > + priv->rng.read = npcm_rng_read;<br>
> > > > > + priv->rng.priv = (unsigned long)&pdev->dev;<br>
> > > > > + if (of_property_read_u32(pdev->dev.of_node, "quality",<br>
> > &quality))<br>
> > > > > + priv->rng.quality = 1000;<br>
> > > > > + else<br>
> > > > > + priv->rng.quality = quality;<br>
> > > > > +<br>
> > > > > + writel(NPCM_RNG_M1ROSEL, priv->base + NPCM_RNGMODE_REG);<br>
> > > > > +#ifndef CONFIG_PM<br>
> > > > > + writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG);<br>
> > > > > +#else<br>
> > > > > + writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE,<br>
> > > > > + priv->base + NPCM_RNGCS_REG);<br>
> > > > > +#endif<br>
> > > ><br>
> > > > If this initialization was moved to npcm_rng_init() then there would be<br>
> > > > no need for the additional ifdefing. It would also get rid of the<br>
> > > > (potentially slow) readl calls on the PM wakeup path.<br>
> > > ><br>
> > ><br>
> > > But when the Kernel have PM configuration than the priv->rng.init is not<br>
> > > set and<br>
> > > *add_early_randomness* function is called. for the *add_early_randomness*<br>
> > > success<br>
> > > the hwrng need to enabled in the probe.<br>
> ><br>
> > Sorry but I don't understand this reply.<br>
> ><br>
> > When CONFIG_PM is enabled then the probe function does not currently set<br>
> > NPCM_RNG_ENABLE; instead is relies on npcm_rng_init() being called by<br>
> ><br>
> <br>
> Sorry maybe I miss understood, but when the CONFIG_PM enabled so the<br>
> NPCM_RNG_ENABLE sets (the code use ifndef and not ifdef)<br>
> *#ifndef CONFIG_PM*<br>
> writel(NPCM_RNG_CLK_SET_25MHZ, priv->base + NPCM_RNGCS_REG);<br>
> #else (*CONFIG_PM enabled*)<br>
> writel(NPCM_RNG_CLK_SET_25MHZ | NPCM_RNG_ENABLE,<br>
> priv->base + NPCM_RNGCS_REG);<br>
> #endif<br>
> <br>
> And the hwrng needed to be enabled to run *add_early_randomness *function<br>
> successfully.<br>
> <br>
> If the hwrng driver will relay on PM logic to enable the hwrng will be<br>
> disable when *add_early_randomness *function is called.<br>
> <br>
> the PM logic (as part of pm_runtime_get_sync() ).<br>
<br>
I ended up reading my mail out of order and replied to the v2 patch.<br>
<br>
The question is *why* the driver doesn't resume properly when adding<br>
early randomness! I think it is because the hwrng_register() is being<br>
called before PM runtime is enabled.<br>
<br></blockquote><div><br></div><div>probably, but I am not sure it will be right to enable the PM runtime before hwrng_register() is called.</div><div><br></div><div>I will double check it.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Daniel.<br></blockquote><div><br></div><div>Thanks,</div><div><br></div><div>Tomer </div></div></div>