<div dir="ltr"><div dir="ltr">Hi Milton,<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 Sat, 31 Aug 2019 at 01:47, Milton Miller II <<a href="mailto:miltonm@us.ibm.com">miltonm@us.ibm.com</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 August 28th around 11:28AM in some timezone, Tomer Maimon wrote<br>
<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/Kconfig<br>
>b/drivers/char/hw_random/Kconfig<br>
>index 59f25286befe..87a1c30e7958 100644<br>
>--- a/drivers/char/hw_random/Kconfig<br>
>+++ b/drivers/char/hw_random/Kconfig<br>
>@@ -440,6 +440,19 @@ config HW_RANDOM_OPTEE<br>
> <br>
> If unsure, say Y.<br>
> <br>
>+config HW_RANDOM_NPCM<br>
>+ tristate "NPCM Random Number Generator support"<br>
>+ depends on ARCH_NPCM || COMPILE_TEST<br>
>+ default HW_RANDOM<br>
>+ help<br>
>+ This driver provides support for the Random Number<br>
>+ Generator hardware available in Nuvoton NPCM SoCs.<br>
>+<br>
>+ To compile this driver as a module, choose M here: the<br>
>+ module will be called npcm-rng.<br>
>+<br>
>+ If unsure, say Y.<br>
>+<br>
> endif # HW_RANDOM<br>
> <br>
> config UML_RANDOM<br>
>diff --git a/drivers/char/hw_random/Makefile<br>
>b/drivers/char/hw_random/Makefile<br>
>index 7c9ef4a7667f..17b6d4e6d591 100644<br>
>--- a/drivers/char/hw_random/Makefile<br>
>+++ b/drivers/char/hw_random/Makefile<br>
>@@ -39,3 +39,4 @@ obj-$(CONFIG_HW_RANDOM_MTK) += mtk-rng.o<br>
> obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o<br>
> obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o<br>
> obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o<br>
>+obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o<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 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>
>+#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>
<br>
You should honor the wait paramter here.<br>
<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>
>+static int npcm_rng_read(struct hwrng *rng, void *buf, size_t max,<br>
>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>
>+ *(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>
So you are doing pm get/put around each rng data sample.</blockquote><div>Do you mean every read? from the core it minimum 32 bytes.</div><div>only when <span style="font-family:arial,sans-serif;color:rgb(0,0,0)"><b>add_early_randomness</b></span><span style="font-family:arial,sans-serif;color:rgb(0,0,0)"><b> </b>function is called the read is 16 bytes.</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Have you characterized the time to enable to the time to get a sample<br>
and compared to the pm runtime sync parameters? <br></blockquote><div>Enabling the hwrng module take much less than generating the random data, especially when the read is minimum 16 bytes</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Do you get any data if you set non-blocking wait above?<br></blockquote><div>yes, if the user need only one byte. but if he will need more than a byte it will fail. </div><div>Do you think I should remove this option because the core read minimum of 16 bytes?</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>
>+}<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>
<br>
so npcm_rng_init and npcm_rng_cleanup are unused if !CONFIG_PM. No warnings?<br></blockquote><div>No </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>
I am assuming these are safe to always set and the clock will<br>
bein range?<br></blockquote><div>Yes </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Did you test without CONFIG_PM ? Looks like the ifndev should be<br>
ifdef otherwise the enable will never be set.<br></blockquote><div>Yes, I have test it.</div><div>The hwrng should be enabled when the PM configuration is set before the </div><div><span style="font-family:arial,sans-serif;color:rgb(0,0,0)"><b>add_early_randomness</b></span><span style="font-family:arial,sans-serif;color:rgb(0,0,0)"><b> </b>function is called in the hwrng core.</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Can you use a local variable for this value that is chosen by<br>
the config instead of ifdef'ing the code?<br>
<br>
<br>
<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>
>+ 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>
><br>
<br></blockquote><div><br></div><div>Thanks a lot,</div><div><br></div><div>Tomer </div></div></div>