| Message ID | 20190909123840.154745-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | hwrng: npcm: add NPCM RNG driver support | expand |
On Mon, Sep 09, 2019 at 03:38:40PM +0300, Tomer Maimon wrote: > Add Nuvoton NPCM BMC Random Number Generator(RNG) driver. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/char/hw_random/Kconfig | 13 ++ > drivers/char/hw_random/Makefile | 1 + > drivers/char/hw_random/npcm-rng.c | 203 ++++++++++++++++++++++++++++++ > 3 files changed, 217 insertions(+) > create mode 100644 drivers/char/hw_random/npcm-rng.c > > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig > index 59f25286befe..87a1c30e7958 100644 > --- a/drivers/char/hw_random/Kconfig > +++ b/drivers/char/hw_random/Kconfig > @@ -440,6 +440,19 @@ config HW_RANDOM_OPTEE > > If unsure, say Y. > > +config HW_RANDOM_NPCM > + tristate "NPCM Random Number Generator support" > + depends on ARCH_NPCM || COMPILE_TEST > + default HW_RANDOM > + help > + This driver provides support for the Random Number > + Generator hardware available in Nuvoton NPCM SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called npcm-rng. > + > + If unsure, say Y. > + > endif # HW_RANDOM > > config UML_RANDOM > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile > index 7c9ef4a7667f..17b6d4e6d591 100644 > --- a/drivers/char/hw_random/Makefile > +++ b/drivers/char/hw_random/Makefile > @@ -39,3 +39,4 @@ obj-$(CONFIG_HW_RANDOM_MTK) += mtk-rng.o > obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o > obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o > obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o > +obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o > diff --git a/drivers/char/hw_random/npcm-rng.c b/drivers/char/hw_random/npcm-rng.c > new file mode 100644 > index 000000000000..3ed396474563 > --- /dev/null > +++ b/drivers/char/hw_random/npcm-rng.c > @@ -0,0 +1,203 @@ > +// 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 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_USEC 20000 > +#define NPCM_RNG_POLL_USEC 1000 > + > +#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 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; > + int ready; > + > + pm_runtime_get_sync((struct device *)priv->rng.priv); > + > + while (max >= sizeof(u32)) { > + ready = readl(priv->base + NPCM_RNGCS_REG) & > + NPCM_RNG_DATA_VALID; > + if (!ready) { > + if (wait) { > + if (readl_poll_timeout(priv->base + NPCM_RNGCS_REG, > + ready, > + ready & NPCM_RNG_DATA_VALID, > + NPCM_RNG_POLL_USEC, > + NPCM_RNG_TIMEOUT_USEC)) > + break; > + } else { > + break; > + } This would read easier without breaking on the else clause: if (!wait) break; if (readl_poll_timeout(...)) break; > + } > + > + *(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; > + bool pm_dis = false; > + 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 > + pm_dis = true; > + 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); > + if (pm_dis) > + 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); This still doesn't seem right and its not simply because pm_dis is an obfuscated way to write IS_ENABLED(CONFIG_PM). I'd like to understand why the call to pm_runtime_get_sync() isn't resulting in the device resume callback running... it is simply because the hwrng_register() happens before the pm_runtime_enable() ? Daniel. > + > + ret = devm_hwrng_register(&pdev->dev, &priv->rng); > + if (ret) { > + dev_err(&pdev->dev, "Failed to register rng device: %d\n", > + ret); > + return ret; > + } > + > + dev_set_drvdata(&pdev->dev, priv); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 100); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + > + return 0; > +} > + > +static int npcm_rng_remove(struct platform_device *pdev) > +{ > + struct npcm_rng *priv = platform_get_drvdata(pdev); > + > + hwrng_unregister(&priv->rng); > + pm_runtime_disable(&pdev->dev); > + pm_runtime_set_suspended(&pdev->dev); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int npcm_rng_runtime_suspend(struct device *dev) > +{ > + struct npcm_rng *priv = dev_get_drvdata(dev); > + > + npcm_rng_cleanup(&priv->rng); > + > + return 0; > +} > + > +static int npcm_rng_runtime_resume(struct device *dev) > +{ > + struct npcm_rng *priv = dev_get_drvdata(dev); > + > + return npcm_rng_init(&priv->rng); > +} > +#endif > + > +static const struct dev_pm_ops npcm_rng_pm_ops = { > + SET_RUNTIME_PM_OPS(npcm_rng_runtime_suspend, > + npcm_rng_runtime_resume, NULL) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) > +}; > + > +static const struct of_device_id rng_dt_id[] = { > + { .compatible = "nuvoton,npcm750-rng", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, rng_dt_id); > + > +static struct platform_driver npcm_rng_driver = { > + .driver = { > + .name = "npcm-rng", > + .pm = &npcm_rng_pm_ops, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(rng_dt_id), > + }, > + .probe = npcm_rng_probe, > + .remove = npcm_rng_remove, > +}; > + > +module_platform_driver(npcm_rng_driver); > + > +MODULE_DESCRIPTION("Nuvoton NPCM Random Number Generator Driver"); > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.18.0 >
On Tue, Sep 10, 2019 at 08:53:13PM +0000, Milton Miller II wrote: > On September 9, 2019 around 7:40AM in somet timezone, Tomer Maimon wrote: > >+#define NPCM_RNG_TIMEOUT_USEC 20000 > >+#define NPCM_RNG_POLL_USEC 1000 > > ... > > >+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 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; > >+ int ready; > >+ > >+ pm_runtime_get_sync((struct device *)priv->rng.priv); > >+ > >+ while (max >= sizeof(u32)) { > >+ ready = readl(priv->base + NPCM_RNGCS_REG) & > >+ NPCM_RNG_DATA_VALID; > >+ if (!ready) { > >+ if (wait) { > >+ if (readl_poll_timeout(priv->base + NPCM_RNGCS_REG, > >+ ready, > >+ ready & NPCM_RNG_DATA_VALID, > >+ NPCM_RNG_POLL_USEC, > >+ NPCM_RNG_TIMEOUT_USEC)) > >+ break; > >+ } else { > >+ break; > > This break is too far from the condition and deeply nested to follow. > > And looking further, readl_poll_timeout will read and check the condition before > calling usleep, so the the initial readl and check is redundant > > Rearrange to make wait determine if you call readl_poll_timeout or > readl / compare / break. > > >+ } > >+ } > >+ > >+ *(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; > >+ bool pm_dis = false; > >+ 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 > >+ pm_dis = true; > >+ priv->rng.init = npcm_rng_init; > >+ priv->rng.cleanup = npcm_rng_cleanup; > >+#endif > > if you move this down you can use one if (ENABLED_CONFIG_PM) {} > > >+ 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); > >+ if (pm_dis) > >+ 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); > > wait ... if we know the whole value here, why read/modify/write the value > in the init and cleanup hook? Save the io read and write the known value > ... define the value to be written for clarity between enable/disable if > needed > > > > >+ > >+ ret = devm_hwrng_register(&pdev->dev, &priv->rng); > >+ if (ret) { > >+ dev_err(&pdev->dev, "Failed to register rng device: %d\n", > >+ ret); > > need to disable if CONFIG_PM ? > > >+ return ret; > >+ } > >+ > >+ dev_set_drvdata(&pdev->dev, priv); > > This should probably be before the register. > > >+ pm_runtime_set_autosuspend_delay(&pdev->dev, 100); > > So every 100ms power off, and if userspace does a read we > will poll every 1ms for upto 20ms. > > If userspace says try once a second with -ENODELAY so no wait, > it never gets data. I didn't follow this. In the time before the device is suspended it should have generated data and this can be sent to the userspace. Providing the suspend delay is longer than the buffer size of the hardware then there won't necessarily be performance problems because the device is "full" when it is suspended. Of course if the hardware loses state when it is suspended then the driver would need extra code on the PM paths to preserve the data... Daniel.