This series of patchsets contains the Broadcom Random Number Generator
driver and device tree binding documentation.
Changes from v1:
added \n to 2 dev_err messages
Scott Branden (2):
hwrng: iproc-rng200 - Add device tree bindings
hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver
.../bindings/hwrng/brcm,iproc-rng200.txt | 12 ++
drivers/char/hw_random/Kconfig | 13 ++
drivers/char/hw_random/Makefile | 1 +
drivers/char/hw_random/iproc-rng200.c | 239 +++++++++++++++++++++
4 files changed, 265 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwrng/brcm,iproc-rng200.txt
create mode 100644 drivers/char/hw_random/iproc-rng200.c
--
2.3.0
Documents the IPROC random number generator device tree bindings
used in some Broadcom chipsets.
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
.../devicetree/bindings/hwrng/brcm,iproc-rng200.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwrng/brcm,iproc-rng200.txt
diff --git a/Documentation/devicetree/bindings/hwrng/brcm,iproc-rng200.txt b/Documentation/devicetree/bindings/hwrng/brcm,iproc-rng200.txt
new file mode 100644
index 0000000..e25a456
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwrng/brcm,iproc-rng200.txt
@@ -0,0 +1,12 @@
+HWRNG support for the iproc-rng200 driver
+
+Required properties:
+- compatible : "brcm,iproc-rng200"
+- reg : base address and size of control register block
+
+Example:
+
+rng {
+ compatible = "brcm,iproc-rng200";
+ reg = <0x18032000 0x28>;
+};
--
2.3.0
This adds a driver for random number generator present on Broadcom
IPROC devices.
Reviewed-by: Ray Jui <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
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,
+};
+
+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;
+
+ /* 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;
+ }
+
+ rng_base = devm_ioremap_resource(dev, res);
+ if (!rng_base) {
+ dev_err(dev, "failed to remap rng regs\n");
+ return -ENODEV;
+ }
+
+ 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);
+
+ /* 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);
+
+ return 0;
+}
+
+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
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 <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> ---
> 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
On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
> This adds a driver for random number generator present on Broadcom
> IPROC devices.
>
> Reviewed-by: Ray Jui <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
The driver looks reasonable overall, I have just one question about
something that sticks out:
> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) {
...
> +
> + /* Are there any random numbers available? */
> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) &
> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) {
...
> + } else {
> + if (!wait)
> + /* Cannot wait, return immediately */
> + return max - num_remaining;
> +
> + /* Can wait, give others chance to run */
> + cpu_relax();
> + }
> + }
> +
It looks like you do a busy-loop around cpu_relax here if asked to wait.
Is this intentional? I would normally expect either cond_resched() or
some msleep() instead.
Arnd
Response inline.
On 15-02-25 11:17 AM, Arnd Bergmann wrote:
> On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
>> This adds a driver for random number generator present on Broadcom
>> IPROC devices.
>>
>> Reviewed-by: Ray Jui <[email protected]>
>> Signed-off-by: Scott Branden <[email protected]>
>
> The driver looks reasonable overall, I have just one question about
> something that sticks out:
>
>> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) {
> ...
>> +
>> + /* Are there any random numbers available? */
>> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) &
>> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) {
> ...
>> + } else {
>> + if (!wait)
>> + /* Cannot wait, return immediately */
>> + return max - num_remaining;
>> +
>> + /* Can wait, give others chance to run */
>> + cpu_relax();
>> + }
>> + }
>> +
>
> It looks like you do a busy-loop around cpu_relax here if asked to wait.
> Is this intentional? I would normally expect either cond_resched() or
> some msleep() instead.
This code was following examples of other open source drivers - bcm2835
and exynos both use cpu_relax. I'll have to look into this more to
understand.
>
> Arnd
>
On Thursday 26 February 2015 11:37:20 Scott Branden wrote:
> Response inline.
>
> On 15-02-25 11:17 AM, Arnd Bergmann wrote:
> > On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
> >> This adds a driver for random number generator present on Broadcom
> >> IPROC devices.
> >>
> >> Reviewed-by: Ray Jui <[email protected]>
> >> Signed-off-by: Scott Branden <[email protected]>
> >
> > The driver looks reasonable overall, I have just one question about
> > something that sticks out:
> >
> >> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) {
> > ...
> >> +
> >> + /* Are there any random numbers available? */
> >> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) &
> >> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) {
> > ...
> >> + } else {
> >> + if (!wait)
> >> + /* Cannot wait, return immediately */
> >> + return max - num_remaining;
> >> +
> >> + /* Can wait, give others chance to run */
> >> + cpu_relax();
> >> + }
> >> + }
> >> +
> >
> > It looks like you do a busy-loop around cpu_relax here if asked to wait.
> > Is this intentional? I would normally expect either cond_resched() or
> > some msleep() instead.
>
> This code was following examples of other open source drivers - bcm2835
> and exynos both use cpu_relax. I'll have to look into this more to
> understand.
>
The majority of the driver apparently use udelay(10) to wait, which is
not much better but at least consistent. The cpu_relax() call probably
gives better throughput.
I don't understand why none of the drivers actually attempts to
msleep(), but that may be because the delay is much too long.
Can you find out what the expected latency is for new data to
become available on your hardware?
Arnd
Hi Arnd,
Latency is 32 us for 32bits of data - commented inline. What delay call
do you recommend in this case?
On 15-02-26 12:15 PM, Arnd Bergmann wrote:
> On Thursday 26 February 2015 11:37:20 Scott Branden wrote:
>> Response inline.
>>
>> On 15-02-25 11:17 AM, Arnd Bergmann wrote:
>>> On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
>>>> This adds a driver for random number generator present on Broadcom
>>>> IPROC devices.
>>>>
>>>> Reviewed-by: Ray Jui <[email protected]>
>>>> Signed-off-by: Scott Branden <[email protected]>
>>>
>>> The driver looks reasonable overall, I have just one question about
>>> something that sticks out:
>>>
>>>> + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) {
>>> ...
>>>> +
>>>> + /* Are there any random numbers available? */
>>>> + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) &
>>>> + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) {
>>> ...
>>>> + } else {
>>>> + if (!wait)
>>>> + /* Cannot wait, return immediately */
>>>> + return max - num_remaining;
>>>> +
>>>> + /* Can wait, give others chance to run */
>>>> + cpu_relax();
>>>> + }
>>>> + }
>>>> +
>>>
>>> It looks like you do a busy-loop around cpu_relax here if asked to wait.
>>> Is this intentional? I would normally expect either cond_resched() or
>>> some msleep() instead.
>>
>> This code was following examples of other open source drivers - bcm2835
>> and exynos both use cpu_relax. I'll have to look into this more to
>> understand.
>>
>
> The majority of the driver apparently use udelay(10) to wait, which is
> not much better but at least consistent. The cpu_relax() call probably
> gives better throughput.
>
> I don't understand why none of the drivers actually attempts to
> msleep(), but that may be because the delay is much too long.
>
> Can you find out what the expected latency is for new data to
> become available on your hardware?
RNG generates at a nominal 1 Mbps. So to generate 32 bits of data takes
approximately 32 us.
>
> Arnd
>
On Thursday 26 February 2015 14:26:02 Scott Branden wrote:
> On 15-02-26 12:15 PM, Arnd Bergmann wrote:
> >> On 15-02-25 11:17 AM, Arnd Bergmann wrote:
> >>> On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
> >> This code was following examples of other open source drivers - bcm2835
> >> and exynos both use cpu_relax. I'll have to look into this more to
> >> understand.
> >>
> >
> > The majority of the driver apparently use udelay(10) to wait, which is
> > not much better but at least consistent. The cpu_relax() call probably
> > gives better throughput.
> >
> > I don't understand why none of the drivers actually attempts to
> > msleep(), but that may be because the delay is much too long.
> >
> > Can you find out what the expected latency is for new data to
> > become available on your hardware?
> RNG generates at a nominal 1 Mbps. So to generate 32 bits of data takes
> approximately 32 us.
The udelay(10) that the other drivers have seems about appropriate then,
and we can independently think of a way to refine the interface.
Please add a comment that explains the rate. Also, is there some kind
of FIFO present in the hwrng device? If it can store close to 1ms work
of data (1000 bits), you can just use an msleep(1) to wait for the
pool to recover.
Another option would be to use usleep_range() with the exact amount
of time to wait for, the lower bound being the minimum number of
bytes asked for and the fifo size, the upper bound being the fifo
size.
Arnd
On 15-02-27 01:14 AM, Arnd Bergmann wrote:
> On Thursday 26 February 2015 14:26:02 Scott Branden wrote:
>> On 15-02-26 12:15 PM, Arnd Bergmann wrote:
>>>> On 15-02-25 11:17 AM, Arnd Bergmann wrote:
>>>>> On Wednesday 25 February 2015 10:16:24 Scott Branden wrote:
>>>> This code was following examples of other open source drivers - bcm2835
>>>> and exynos both use cpu_relax. I'll have to look into this more to
>>>> understand.
>>>>
>>>
>>> The majority of the driver apparently use udelay(10) to wait, which is
>>> not much better but at least consistent. The cpu_relax() call probably
>>> gives better throughput.
>>>
>>> I don't understand why none of the drivers actually attempts to
>>> msleep(), but that may be because the delay is much too long.
>>>
>>> Can you find out what the expected latency is for new data to
>>> become available on your hardware?
>> RNG generates at a nominal 1 Mbps. So to generate 32 bits of data takes
>> approximately 32 us.
>
> The udelay(10) that the other drivers have seems about appropriate then,
> and we can independently think of a way to refine the interface.
> Please add a comment that explains the rate. Also, is there some kind
> of FIFO present in the hwrng device? If it can store close to 1ms work
> of data (1000 bits), you can just use an msleep(1) to wait for the
> pool to recover.
FIFO is 512 bits. I will look as to whether we can live with 1/2
throughput.
>
> Another option would be to use usleep_range() with the exact amount
> of time to wait for, the lower bound being the minimum number of
> bytes asked for and the fifo size, the upper bound being the fifo
> size.
>
> Arnd
>
On Saturday 28 February 2015 08:01:11 Scott Branden wrote:
> > The udelay(10) that the other drivers have seems about appropriate then,
> > and we can independently think of a way to refine the interface.
> > Please add a comment that explains the rate. Also, is there some kind
> > of FIFO present in the hwrng device? If it can store close to 1ms work
> > of data (1000 bits), you can just use an msleep(1) to wait for the
> > pool to recover.
> FIFO is 512 bits. I will look as to whether we can live with 1/2
> throughput.
In that case, I think usleep_range(min(len * 8, 500), 500)) would be
a good compromise: it waits at most until the fifo is full, but might
return earlier if enough bits are available to fulfill the request.
Arnd