<div dir="ltr"><div dir="ltr">Hi Daniel,<div><br></div><div>appreciate your comments and sorry for the late reply</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 29 Aug 2019 at 13:47, 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 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>
> 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 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 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) & 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></blockquote><div>the HWRNG generate byte of random data in a few milliseconds so it is better to use the sleep command. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +static int npcm_rng_read(struct hwrng *rng, void *buf, size_t max, bool 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 *)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", &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></blockquote><div> </div><div>But when the Kernel have PM configuration than the priv->rng.init is not set and </div><div><font face="arial, sans-serif"><span style="color:rgb(0,0,0)"><b>add_early_randomness</b></span><span style="color:rgb(0,0,0)"><b> </b>function is called. for the </span><span style="color:rgb(0,0,0)"><b>add_early_randomness</b></span><span style="color:rgb(0,0,0)"><b> </b>success</span></font></div><div><span style="color:rgb(0,0,0)"><font face="arial, sans-serif">the hwrng need to enabled in the probe.</font></span></div><div><br></div><div><span style="color:rgb(0,0,0)"><font face="arial, sans-serif">I will remove the ifdef and add local </font></span>variable <span style="color:rgb(0,0,0)"><font face="arial, sans-serif">instead (as Milton Miller suggested)</font></span></div><h3 class="gmail-iw" style="overflow:hidden;white-space:nowrap;font-size:0.75rem;font-weight:inherit;margin:inherit;text-overflow:ellipsis;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;letter-spacing:0.3px;color:rgb(95,99,104);line-height:20px"><br></h3><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> + ret = devm_hwrng_register(&pdev->dev, &priv->rng);<br>
> + if (ret) {<br>
> + dev_err(&pdev->dev, "Failed to register rng device: %d\n",<br>
> + ret);<br>
> + return ret;<br>
> + }<br>
> +<br>
> + dev_set_drvdata(&pdev->dev, priv);<br>
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 100);<br>
> + pm_runtime_use_autosuspend(&pdev->dev);<br>
> + pm_runtime_enable(&pdev->dev);<br>
> +<br>
> + dev_info(&pdev->dev, "Random Number Generator Probed\n");<br>
<br>
Does the user need to know this every time we boot? There are lots of<br>
debug tools we can bring to bear if they are worried the device<br>
isn't probing.<br>
<br>
<br>
Daniel.<br>
<br>
<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int npcm_rng_remove(struct platform_device *pdev)<br>
> +{<br>
> + struct npcm_rng *priv = platform_get_drvdata(pdev);<br>
> +<br>
> + hwrng_unregister(&priv->rng);<br>
> + pm_runtime_disable(&pdev->dev);<br>
> + pm_runtime_set_suspended(&pdev->dev);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +#ifdef CONFIG_PM<br>
> +static int npcm_rng_runtime_suspend(struct device *dev)<br>
> +{<br>
> + struct npcm_rng *priv = dev_get_drvdata(dev);<br>
> +<br>
> + npcm_rng_cleanup(&priv->rng);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int npcm_rng_runtime_resume(struct device *dev)<br>
> +{<br>
> + struct npcm_rng *priv = dev_get_drvdata(dev);<br>
> +<br>
> + return npcm_rng_init(&priv->rng);<br>
> +}<br>
> +#endif<br>
> +<br>
> +static const struct dev_pm_ops npcm_rng_pm_ops = {<br>
> + SET_RUNTIME_PM_OPS(npcm_rng_runtime_suspend,<br>
> + npcm_rng_runtime_resume, NULL)<br>
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,<br>
> + pm_runtime_force_resume)<br>
> +};<br>
> +<br>
> +static const struct of_device_id rng_dt_id[] = {<br>
> + { .compatible = "nuvoton,npcm750-rng", },<br>
> + {},<br>
> +};<br>
> +MODULE_DEVICE_TABLE(of, rng_dt_id);<br>
> +<br>
> +static struct platform_driver npcm_rng_driver = {<br>
> + .driver = {<br>
> + .name = "npcm-rng",<br>
> + .pm = &npcm_rng_pm_ops,<br>
> + .owner = THIS_MODULE,<br>
> + .of_match_table = of_match_ptr(rng_dt_id),<br>
> + },<br>
> + .probe = npcm_rng_probe,<br>
> + .remove = npcm_rng_remove,<br>
> +};<br>
> +<br>
> +module_platform_driver(npcm_rng_driver);<br>
> +<br>
> +MODULE_DESCRIPTION("Nuvoton NPCM Random Number Generator Driver");<br>
> +MODULE_AUTHOR("Tomer Maimon <<a href="mailto:tomer.maimon@nuvoton.com" target="_blank">tomer.maimon@nuvoton.com</a>>");<br>
> +MODULE_LICENSE("GPL v2");<br>
> -- <br>
> 2.18.0<br>
> <br></blockquote><div><br></div><div>Thanks a lot,</div><div><br></div><div>Tomer </div></div></div>