Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver

From: Dmitry Torokhov
Date: Wed Feb 25 2015 - 13:43:49 EST

Next message: Guenter Roeck: "Re: [PATCH v5 0/6] hwmon: thermal: Odroid U3: Provide support for Odroid U3 fan"
Previous message: Denys Vlasenko: "Re: [PATCH 2/3 v3] x86: entry_64.S: always allocate complete "struct pt_regs""
In reply to: Scott Branden: "[PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver"
Next in thread: Arnd Bergmann: "Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Scott,

On Wed, Feb 25, 2015 at 10:16:24AM -0800, Scott Branden wrote:
> This adds a driver for random number generator present on Broadcom
> IPROC devices.
>
> Reviewed-by: Ray Jui <rjui@xxxxxxxxxxxx>
> Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx>
> ---
> drivers/char/hw_random/Kconfig | 13 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/iproc-rng200.c | 239 ++++++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+)
> create mode 100644 drivers/char/hw_random/iproc-rng200.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index de57b38..f48cf11 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -101,6 +101,19 @@ config HW_RANDOM_BCM2835
>
> If unsure, say Y.
>
> +config HW_RANDOM_IPROC_RNG200
> + tristate "Broadcom iProc RNG200 support"
> + depends on ARCH_BCM_IPROC
> + default HW_RANDOM
> + ---help---
> + This driver provides kernel-side support for the RNG200
> + hardware found on the Broadcom iProc SoCs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called iproc-rng200
> +
> + If unsure, say Y.
> +
> config HW_RANDOM_GEODE
> tristate "AMD Geode HW Random Number Generator support"
> depends on X86_32 && PCI
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 0b4cd57..055bb01 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
> obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
> obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
> obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
> +obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
> obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> diff --git a/drivers/char/hw_random/iproc-rng200.c b/drivers/char/hw_random/iproc-rng200.c
> new file mode 100644
> index 0000000..4643aa9
> --- /dev/null
> +++ b/drivers/char/hw_random/iproc-rng200.c
> @@ -0,0 +1,239 @@
> +/*
> +* Copyright (C) 2014 Broadcom Corporation
> +*
> +* This program is free software; you can redistribute it and/or
> +* modify it under the terms of the GNU General Public License as
> +* published by the Free Software Foundation version 2.
> +*
> +* This program is distributed "as is" WITHOUT ANY WARRANTY of any
> +* kind, whether express or implied; without even the implied warranty
> +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +* GNU General Public License for more details.
> +*/
> +/*
> + * DESCRIPTION: The Broadcom iProc RNG200 Driver
> + */
> +
> +#include <linux/hw_random.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +
> +
> +/* Registers */
> +#define RNG_CTRL_OFFSET 0x00
> +#define RNG_CTRL_RNG_RBGEN_MASK 0x00001FFF
> +#define RNG_CTRL_RNG_RBGEN_ENABLE 0x00000001
> +#define RNG_CTRL_RNG_RBGEN_DISABLE 0x00000000
> +
> +#define RNG_SOFT_RESET_OFFSET 0x04
> +#define RNG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001
> +#define RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001
> +#define RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000
> +
> +#define RBG_SOFT_RESET_OFFSET 0x08
> +#define RBG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001
> +#define RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001
> +#define RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000
> +
> +#define RNG_INT_STATUS_OFFSET 0x18
> +#define RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK 0x80000000
> +#define RNG_INT_STATUS_STARTUP_TRANSITIONS_MET_IRQ_MASK 0x00020000
> +#define RNG_INT_STATUS_NIST_FAIL_IRQ_MASK 0x00000020
> +#define RNG_INT_STATUS_TOTAL_BITS_COUNT_IRQ_MASK 0x00000001
> +
> +#define RNG_FIFO_DATA_OFFSET 0x20
> +
> +#define RNG_FIFO_COUNT_OFFSET 0x24
> +#define RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK 0x000000FF
> +
> +static void iproc_rng200_restart(void __iomem *rng_base)
> +{
> + uint32_t val;
> +
> + /* Disable RBG */
> + val = ioread32(rng_base + RNG_CTRL_OFFSET);
> + val &= ~RNG_CTRL_RNG_RBGEN_MASK;
> + val |= RNG_CTRL_RNG_RBGEN_DISABLE;
> + iowrite32(val, rng_base + RNG_CTRL_OFFSET);
> +
> + /* Clear all interrupt status */
> + iowrite32(0xFFFFFFFFUL, rng_base + RNG_INT_STATUS_OFFSET);
> +
> + /* Reset RNG and RBG */
> + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET);
> + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK;
> + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE;
> + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET);
> +
> + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET);
> + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK;
> + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE;
> + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET);
> +
> + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET);
> + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK;
> + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR;
> + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET);
> +
> + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET);
> + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK;
> + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR;
> + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET);
> +
> + /* Enable RBG */
> + val = ioread32(rng_base + RNG_CTRL_OFFSET);
> + val &= ~RNG_CTRL_RNG_RBGEN_MASK;
> + val |= RNG_CTRL_RNG_RBGEN_ENABLE;
> + iowrite32(val, rng_base + RNG_CTRL_OFFSET);
> +}
> +
> +static int iproc_rng200_read(struct hwrng *rng, void *buf, size_t max,
> + bool wait)
> +{
> + uint32_t status = 0;
> + uint32_t num_remaining = max;
> +
> + #define MAX_RESETS_PER_READ 1
> + uint32_t num_resets = 0;
> +
> + #define MAX_IDLE_TIME (1 * HZ)
> + unsigned long idle_endtime = jiffies + MAX_IDLE_TIME;
> +
> + /* Retrieve HW RNG registers base address. */
> + void __iomem *rng_base = (void __iomem *)rng->priv;
> +
> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) {
> +
> + /* Is RNG sane? If not, reset it. */
> + status = ioread32(rng_base + RNG_INT_STATUS_OFFSET);
> + if ((status & (RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK |
> + RNG_INT_STATUS_NIST_FAIL_IRQ_MASK)) != 0) {
> +
> + if (num_resets >= MAX_RESETS_PER_READ)
> + return max - num_remaining;
> +
> + iproc_rng200_restart(rng_base);
> + num_resets++;
> + }
> +
> + /* Are there any random numbers available? */
> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) &
> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) {
> +
> + if (num_remaining >= sizeof(uint32_t)) {
> + /* Buffer has room to store entire word */
> + *(uint32_t *)buf = ioread32(rng_base +
> + RNG_FIFO_DATA_OFFSET);
> + buf += sizeof(uint32_t);
> + num_remaining -= sizeof(uint32_t);
> + } else {
> + /* Buffer can only store partial word */
> + uint32_t rnd_number = ioread32(rng_base +
> + RNG_FIFO_DATA_OFFSET);
> + memcpy(buf, &rnd_number, num_remaining);
> + buf += num_remaining;
> + num_remaining = 0;
> + }
> +
> + /* Reset the IDLE timeout */
> + idle_endtime = jiffies + MAX_IDLE_TIME;
> + } else {
> + if (!wait)
> + /* Cannot wait, return immediately */
> + return max - num_remaining;
> +
> + /* Can wait, give others chance to run */
> + cpu_relax();
> + }
> + }
> +
> + return max - num_remaining;
> +}
> +
> +static struct hwrng iproc_rng200_ops = {
> + .name = "iproc-rng200",
> + .read = iproc_rng200_read,
> +};

I would prefer if we allocated a driver-private structure that wraps
hwrng instead of using statically allocated singleton as I wonder how it
will behave if I bind/unbind the device several times.

> +
> +static int iproc_rng200_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *rng_base = 0;
> + uint32_t val = 0;
> + int err = 0;

There is no need to initialize all variables as it suppressed "used but
uninitialized" warnings from the compiler. Imagine someone adding a new
init operation and when they handle failure they forget to adjust error
code so it stays 0: for upper layers we'd signal success even though
our device/driver are half-bound.

> +
> + /* Map peripheral */
> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + if (!res) {
> + dev_err(dev, "failed to get rng resources\n");
> + return -ENODEV;

If we have platform device but it is missing parts of necessary data it
should be -EINVAL.

> + }
> +
> + rng_base = devm_ioremap_resource(dev, res);
> + if (!rng_base) {

devm_ioremap_resource() returns ERR_PTR-encoded result, you need to
check with IS_ERR().

> + dev_err(dev, "failed to remap rng regs\n");
> + return -ENODEV;

return PTR_ERR(rng_base);

> + }

There is no clock for RNG?

> +
> + iproc_rng200_ops.priv = (unsigned long)rng_base;
> +
> + /* Setup RNG. */
> + val = ioread32(rng_base + RNG_CTRL_OFFSET);
> + val &= ~RNG_CTRL_RNG_RBGEN_MASK;
> + val |= RNG_CTRL_RNG_RBGEN_ENABLE;
> + iowrite32(val, rng_base + RNG_CTRL_OFFSET);

That should be in [new] iproc_rng200_init().

> +
> + /* Register driver */
> + err = hwrng_register(&iproc_rng200_ops);
> + if (err) {
> + dev_err(dev, "hwrng registration failed\n");
> + return err;
> + }
> + dev_info(dev, "hwrng registered\n");
> +
> + return 0;
> +}
> +
> +static int iproc_rng200_remove(struct platform_device *pdev)
> +{
> + uint32_t val = 0;
> + void __iomem *rng_base = (void __iomem *)iproc_rng200_ops.priv;
> +
> + /* Unregister driver */
> + hwrng_unregister(&iproc_rng200_ops);
> +
> + /* Disable RNG hardware */
> + val = ioread32(rng_base + RNG_CTRL_OFFSET);
> + val &= ~RNG_CTRL_RNG_RBGEN_MASK;
> + val |= RNG_CTRL_RNG_RBGEN_DISABLE;
> + iowrite32(val, rng_base + RNG_CTRL_OFFSET);

That should be in [new] iproc_rng200_cleanup().

> +
> + return 0;
> +}
> +

#ifdef CONFIG_OF

maybe?

> +static const struct of_device_id iproc_rng200_of_match[] = {
> + { .compatible = "brcm,iproc-rng200", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, iproc_rng200_of_match);
> +
> +static struct platform_driver iproc_rng200_driver = {
> + .driver = {
> + .name = "iproc-rng200",
> + .of_match_table = iproc_rng200_of_match,
> + },
> + .probe = iproc_rng200_probe,
> + .remove = iproc_rng200_remove,
> +};
> +module_platform_driver(iproc_rng200_driver);
> +
> +MODULE_AUTHOR("Broadcom");
> +MODULE_DESCRIPTION("iProc RNG200 Random Number Generator driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.3.0
>

Thanks.

--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Guenter Roeck: "Re: [PATCH v5 0/6] hwmon: thermal: Odroid U3: Provide support for Odroid U3 fan"
Previous message: Denys Vlasenko: "Re: [PATCH 2/3 v3] x86: entry_64.S: always allocate complete "struct pt_regs""
In reply to: Scott Branden: "[PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver"
Next in thread: Arnd Bergmann: "Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]