[PATCH 0/2] Add support for Qualcomm's PRNG

[PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

This patch set adds hardware RNG driver wich is used to control the
Qualcomm's PRNG hardware block.
The first patch document the DT bindings needed to sucessfuly probe
the driver and the second patch adds the driver.

Comments are welecome!

Stanimir Varbanov (2):
  ARM: DT: msm: Add Qualcomm's PRNG driver binding document
  hwrng: msm: Add PRNG support for MSM SoC's

 .../devicetree/bindings/rng/qcom,prng.txt          |  17 ++
 drivers/char/hw_random/Kconfig                     |  12 ++
 drivers/char/hw_random/Makefile                    |   1 +
 drivers/char/hw_random/msm-rng.c                   | 211 +++++++++++++++++++++
 4 files changed, 241 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/qcom,prng.txt
 create mode 100644 drivers/char/hw_random/msm-rng.c

-- 
1.8.3.1

[PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

This patch set adds hardware RNG driver wich is used to control the
Qualcomm's PRNG hardware block.
The first patch document the DT bindings needed to sucessfuly probe
the driver and the second patch adds the driver.

Comments are welecome!

Stanimir Varbanov (2):
  ARM: DT: msm: Add Qualcomm's PRNG driver binding document
  hwrng: msm: Add PRNG support for MSM SoC's

 .../devicetree/bindings/rng/qcom,prng.txt          |  17 ++
 drivers/char/hw_random/Kconfig                     |  12 ++
 drivers/char/hw_random/Makefile                    |   1 +
 drivers/char/hw_random/msm-rng.c                   | 211 +++++++++++++++++++++
 4 files changed, 241 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/qcom,prng.txt
 create mode 100644 drivers/char/hw_random/msm-rng.c

-- 
1.8.3.1

[PATCH 1/2] ARM: DT: msm: Add Qualcomm's PRNG driver binding document

[Stanimir Varbanov]

This adds Qualcomm PRNG driver device tree binding documentation
to use as an example in dts trees.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 Documentation/devicetree/bindings/rng/qcom,prng.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/qcom,prng.txt

diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
new file mode 100644
index 000000000000..92be00085ab1
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
@@ -0,0 +1,17 @@
+Qualcomm MSM pseudo random number generator.
+
+Required properties:
+
+- compatible  : should be "qcom,prng"
+- reg         : specifies base physical address and size of the registers map
+- clocks      : phandle to clock-controller plus clock-specifier pair
+- clock-names : "prng" clocks all registers, FIFO and circuits in PRNG IP block
+
+Example:
+
+	rng {
+		compatible = "qcom,prng";
+		reg = <0xf9bff000 0x200>;
+		clocks = <&clock GCC_PRNG_AHB_CLK>;
+		clock-names = "prng";
+	};
-- 
1.8.3.1

[PATCH 1/2] ARM: DT: msm: Add Qualcomm's PRNG driver binding document

[Stanimir Varbanov]

This adds Qualcomm PRNG driver device tree binding documentation
to use as an example in dts trees.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 Documentation/devicetree/bindings/rng/qcom,prng.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/qcom,prng.txt

diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
new file mode 100644
index 000000000000..92be00085ab1
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
@@ -0,0 +1,17 @@
+Qualcomm MSM pseudo random number generator.
+
+Required properties:
+
+- compatible  : should be "qcom,prng"
+- reg         : specifies base physical address and size of the registers map
+- clocks      : phandle to clock-controller plus clock-specifier pair
+- clock-names : "prng" clocks all registers, FIFO and circuits in PRNG IP block
+
+Example:
+
+	rng {
+		compatible = "qcom,prng";
+		reg = <0xf9bff000 0x200>;
+		clocks = <&clock GCC_PRNG_AHB_CLK>;
+		clock-names = "prng";
+	};
-- 
1.8.3.1

[PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

This adds a driver for hardware random number generator present
on Qualcomm MSM SoC's.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/char/hw_random/Kconfig   |  12 +++
 drivers/char/hw_random/Makefile  |   1 +
 drivers/char/hw_random/msm-rng.c | 211 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/char/hw_random/msm-rng.c

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0aa9d91daef5..d902330cef43 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -314,3 +314,15 @@ config HW_RANDOM_TPM
 	  module will be called tpm-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_MSM
+        tristate "Qualcomm MSM Random Number Generator support"
+        depends on HW_RANDOM && ARCH_MSM && HAVE_CLK
+        ---help---
+          This driver provides kernel-side support for the Random Number
+          Generator hardware found on Qualcomm MSM SoCs.
+
+          To compile this driver as a module, choose M here. the
+          module will be called msm-rng.
+
+          If unsure, say Y.
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index bed467c9300e..441a0a7705a2 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -27,3 +27,4 @@ obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-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_MSM) += msm-rng.o
diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
new file mode 100644
index 000000000000..c4545d2da949
--- /dev/null
+++ b/drivers/char/hw_random/msm-rng.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright (c) 2011-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+/* Device specific register offsets */
+#define PRNG_DATA_OUT		0x0000
+#define PRNG_STATUS		0x0004
+#define PRNG_LFSR_CFG		0x0100
+#define PRNG_CONFIG		0x0104
+
+/* Device specific register masks and config values */
+#define PRNG_LFSR_CFG_MASK	0x0000ffff
+#define PRNG_LFSR_CFG_CLOCKS	0x0000dddd
+#define PRNG_CONFIG_MASK	0x00000002
+#define PRNG_CONFIG_HW_ENABLE	BIT(1)
+#define PRNG_STATUS_DATA_AVAIL	BIT(0)
+
+#define MAX_HW_FIFO_DEPTH	16
+#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
+#define WORD_SZ			4
+
+struct msm_rng {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static int msm_rng_enable(struct msm_rng *rng, int enable)
+{
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(rng->clk);
+	if (ret)
+		return ret;
+
+	if (enable) {
+		/* Enable PRNG only if it is not already enabled */
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		if (val & PRNG_CONFIG_HW_ENABLE)
+			goto already_enabled;
+
+		/* PRNG is not enabled */
+		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
+		val &= ~PRNG_LFSR_CFG_MASK;
+		val |= PRNG_LFSR_CFG_CLOCKS;
+		writel(val, rng->base + PRNG_LFSR_CFG);
+
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		val &= ~PRNG_CONFIG_MASK;
+		val |= PRNG_CONFIG_HW_ENABLE;
+		writel(val, rng->base + PRNG_CONFIG);
+	} else {
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		val &= ~PRNG_CONFIG_MASK;
+		writel(val, rng->base + PRNG_CONFIG);
+	}
+
+already_enabled:
+	clk_disable_unprepare(rng->clk);
+	return 0;
+}
+
+static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+	size_t currsize = 0;
+	u32 *retdata = data;
+	size_t maxsize;
+	int ret;
+	u32 val;
+
+	/* calculate max size bytes to transfer back to caller */
+	maxsize = min_t(size_t, MAX_HW_FIFO_SIZE, max);
+
+	/* no room for word data */
+	if (maxsize < WORD_SZ)
+		return 0;
+
+	ret = clk_prepare_enable(rng->clk);
+	if (ret)
+		return ret;
+
+	/* read random data from hardware */
+	do {
+		val = readl_relaxed(rng->base + PRNG_STATUS);
+		if (!(val & PRNG_STATUS_DATA_AVAIL))
+			break;
+
+		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
+		if (!val)
+			break;
+
+		*retdata++ = val;
+		currsize += WORD_SZ;
+
+		/* make sure we stay on 32bit boundary */
+		if ((maxsize - currsize) < WORD_SZ)
+			break;
+	} while (currsize < maxsize);
+
+	clk_disable_unprepare(rng->clk);
+
+	return currsize;
+}
+
+static int msm_rng_init(struct hwrng *hwrng)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+
+	return msm_rng_enable(rng, 1);
+}
+
+static void msm_rng_cleanup(struct hwrng *hwrng)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+
+	msm_rng_enable(rng, 0);
+}
+
+static struct hwrng msm_rng_ops = {
+	.name = KBUILD_MODNAME,
+	.init = msm_rng_init,
+	.cleanup = msm_rng_cleanup,
+	.read = msm_rng_read,
+};
+
+static int msm_rng_probe(struct platform_device *pdev)
+{
+	struct msm_rng *rng;
+	struct device_node *np;
+	struct resource res;
+	int ret;
+
+	np = of_node_get(pdev->dev.of_node);
+	if (!np)
+		return -ENODEV;
+
+	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
+	if (!rng) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret)
+		goto err_exit;
+
+	rng->base = devm_ioremap_resource(&pdev->dev, &res);
+	if (IS_ERR(rng->base)) {
+		ret = PTR_ERR(rng->base);
+		goto err_exit;
+	}
+
+	rng->clk = devm_clk_get(&pdev->dev, "prng");
+	if (IS_ERR(rng->clk)) {
+		ret = PTR_ERR(rng->clk);
+		goto err_exit;
+	}
+
+	msm_rng_ops.priv = (unsigned long) rng;
+	ret = hwrng_register(&msm_rng_ops);
+	if (ret)
+		dev_err(&pdev->dev, "failed to register hwrng\n");
+
+err_exit:
+	of_node_put(np);
+	return ret;
+}
+
+static int msm_rng_remove(struct platform_device *pdev)
+{
+	hwrng_unregister(&msm_rng_ops);
+	return 0;
+}
+
+static struct of_device_id msm_rng_of_match[] = {
+	{ .compatible = "qcom,prng", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, msm_rng_of_match);
+
+static struct platform_driver msm_rng_driver = {
+	.probe = msm_rng_probe,
+	.remove = msm_rng_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(msm_rng_of_match),
+	}
+};
+module_platform_driver(msm_rng_driver);
+
+MODULE_AUTHOR("The Linux Foundation");
+MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

[PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

This adds a driver for hardware random number generator present
on Qualcomm MSM SoC's.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
---
 drivers/char/hw_random/Kconfig   |  12 +++
 drivers/char/hw_random/Makefile  |   1 +
 drivers/char/hw_random/msm-rng.c | 211 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/char/hw_random/msm-rng.c

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0aa9d91daef5..d902330cef43 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -314,3 +314,15 @@ config HW_RANDOM_TPM
 	  module will be called tpm-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_MSM
+        tristate "Qualcomm MSM Random Number Generator support"
+        depends on HW_RANDOM && ARCH_MSM && HAVE_CLK
+        ---help---
+          This driver provides kernel-side support for the Random Number
+          Generator hardware found on Qualcomm MSM SoCs.
+
+          To compile this driver as a module, choose M here. the
+          module will be called msm-rng.
+
+          If unsure, say Y.
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index bed467c9300e..441a0a7705a2 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -27,3 +27,4 @@ obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-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_MSM) += msm-rng.o
diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
new file mode 100644
index 000000000000..c4545d2da949
--- /dev/null
+++ b/drivers/char/hw_random/msm-rng.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright (c) 2011-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#include <linux/clk.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+/* Device specific register offsets */
+#define PRNG_DATA_OUT		0x0000
+#define PRNG_STATUS		0x0004
+#define PRNG_LFSR_CFG		0x0100
+#define PRNG_CONFIG		0x0104
+
+/* Device specific register masks and config values */
+#define PRNG_LFSR_CFG_MASK	0x0000ffff
+#define PRNG_LFSR_CFG_CLOCKS	0x0000dddd
+#define PRNG_CONFIG_MASK	0x00000002
+#define PRNG_CONFIG_HW_ENABLE	BIT(1)
+#define PRNG_STATUS_DATA_AVAIL	BIT(0)
+
+#define MAX_HW_FIFO_DEPTH	16
+#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
+#define WORD_SZ			4
+
+struct msm_rng {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static int msm_rng_enable(struct msm_rng *rng, int enable)
+{
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(rng->clk);
+	if (ret)
+		return ret;
+
+	if (enable) {
+		/* Enable PRNG only if it is not already enabled */
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		if (val & PRNG_CONFIG_HW_ENABLE)
+			goto already_enabled;
+
+		/* PRNG is not enabled */
+		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
+		val &= ~PRNG_LFSR_CFG_MASK;
+		val |= PRNG_LFSR_CFG_CLOCKS;
+		writel(val, rng->base + PRNG_LFSR_CFG);
+
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		val &= ~PRNG_CONFIG_MASK;
+		val |= PRNG_CONFIG_HW_ENABLE;
+		writel(val, rng->base + PRNG_CONFIG);
+	} else {
+		val = readl_relaxed(rng->base + PRNG_CONFIG);
+		val &= ~PRNG_CONFIG_MASK;
+		writel(val, rng->base + PRNG_CONFIG);
+	}
+
+already_enabled:
+	clk_disable_unprepare(rng->clk);
+	return 0;
+}
+
+static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+	size_t currsize = 0;
+	u32 *retdata = data;
+	size_t maxsize;
+	int ret;
+	u32 val;
+
+	/* calculate max size bytes to transfer back to caller */
+	maxsize = min_t(size_t, MAX_HW_FIFO_SIZE, max);
+
+	/* no room for word data */
+	if (maxsize < WORD_SZ)
+		return 0;
+
+	ret = clk_prepare_enable(rng->clk);
+	if (ret)
+		return ret;
+
+	/* read random data from hardware */
+	do {
+		val = readl_relaxed(rng->base + PRNG_STATUS);
+		if (!(val & PRNG_STATUS_DATA_AVAIL))
+			break;
+
+		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
+		if (!val)
+			break;
+
+		*retdata++ = val;
+		currsize += WORD_SZ;
+
+		/* make sure we stay on 32bit boundary */
+		if ((maxsize - currsize) < WORD_SZ)
+			break;
+	} while (currsize < maxsize);
+
+	clk_disable_unprepare(rng->clk);
+
+	return currsize;
+}
+
+static int msm_rng_init(struct hwrng *hwrng)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+
+	return msm_rng_enable(rng, 1);
+}
+
+static void msm_rng_cleanup(struct hwrng *hwrng)
+{
+	struct msm_rng *rng = (struct msm_rng *)hwrng->priv;
+
+	msm_rng_enable(rng, 0);
+}
+
+static struct hwrng msm_rng_ops = {
+	.name = KBUILD_MODNAME,
+	.init = msm_rng_init,
+	.cleanup = msm_rng_cleanup,
+	.read = msm_rng_read,
+};
+
+static int msm_rng_probe(struct platform_device *pdev)
+{
+	struct msm_rng *rng;
+	struct device_node *np;
+	struct resource res;
+	int ret;
+
+	np = of_node_get(pdev->dev.of_node);
+	if (!np)
+		return -ENODEV;
+
+	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
+	if (!rng) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret)
+		goto err_exit;
+
+	rng->base = devm_ioremap_resource(&pdev->dev, &res);
+	if (IS_ERR(rng->base)) {
+		ret = PTR_ERR(rng->base);
+		goto err_exit;
+	}
+
+	rng->clk = devm_clk_get(&pdev->dev, "prng");
+	if (IS_ERR(rng->clk)) {
+		ret = PTR_ERR(rng->clk);
+		goto err_exit;
+	}
+
+	msm_rng_ops.priv = (unsigned long) rng;
+	ret = hwrng_register(&msm_rng_ops);
+	if (ret)
+		dev_err(&pdev->dev, "failed to register hwrng\n");
+
+err_exit:
+	of_node_put(np);
+	return ret;
+}
+
+static int msm_rng_remove(struct platform_device *pdev)
+{
+	hwrng_unregister(&msm_rng_ops);
+	return 0;
+}
+
+static struct of_device_id msm_rng_of_match[] = {
+	{ .compatible = "qcom,prng", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, msm_rng_of_match);
+
+static struct platform_driver msm_rng_driver = {
+	.probe = msm_rng_probe,
+	.remove = msm_rng_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(msm_rng_of_match),
+	}
+};
+module_platform_driver(msm_rng_driver);
+
+MODULE_AUTHOR("The Linux Foundation");
+MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stephen Boyd]

On 10/03/13 07:52, Stanimir Varbanov wrote:
> +#define PRNG_CONFIG_MASK	0x00000002
> +#define PRNG_CONFIG_HW_ENABLE	BIT(1)

These two are the same so please drop the PRNG_CONFIG_MASK define and
just use the PRNG_CONFIG_HW_ENABLE define.

> +#define PRNG_STATUS_DATA_AVAIL	BIT(0)
> +
> +#define MAX_HW_FIFO_DEPTH	16
> +#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
> +#define WORD_SZ			4
> +
> +struct msm_rng {
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static int msm_rng_enable(struct msm_rng *rng, int enable)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(rng->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (enable) {
> +		/* Enable PRNG only if it is not already enabled */
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		if (val & PRNG_CONFIG_HW_ENABLE)
> +			goto already_enabled;
> +
> +		/* PRNG is not enabled */
> +		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
> +		val &= ~PRNG_LFSR_CFG_MASK;
> +		val |= PRNG_LFSR_CFG_CLOCKS;
> +		writel(val, rng->base + PRNG_LFSR_CFG);
> +
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		val &= ~PRNG_CONFIG_MASK;
> +		val |= PRNG_CONFIG_HW_ENABLE;
> +		writel(val, rng->base + PRNG_CONFIG);

This could just be

		val = readl_relaxed(rng->base + PRNG_CONFIG);
		val |= PRNG_CONFIG_HW_ENABLE;
		writel(val, rng->base + PRNG_CONFIG);


> +	} else {
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		val &= ~PRNG_CONFIG_MASK;
> +		writel(val, rng->base + PRNG_CONFIG);
> +	}
> +
> +already_enabled:
> +	clk_disable_unprepare(rng->clk);
> +	return 0;
> +}
> +
[...]
> +static int msm_rng_probe(struct platform_device *pdev)
> +{
> +	struct msm_rng *rng;
> +	struct device_node *np;
> +	struct resource res;
> +	int ret;
> +
> +	np = of_node_get(pdev->dev.of_node);
> +	if (!np)
> +		return -ENODEV;

This is unnecessary.

> +
> +	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> +	if (!rng) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret)
> +		goto err_exit;

We should just do

  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  rng->base = devm_ioremap_resource(&pdev->dev, res);
  if (IS_ERR(rng->base))
            return PTR_ERR(rng->base);

> +
> +	rng->base = devm_ioremap_resource(&pdev->dev, &res);
> +	if (IS_ERR(rng->base)) {
> +		ret = PTR_ERR(rng->base);
> +		goto err_exit;
> +	}
> +
> +	rng->clk = devm_clk_get(&pdev->dev, "prng");
> +	if (IS_ERR(rng->clk)) {
> +		ret = PTR_ERR(rng->clk);
> +		goto err_exit;
> +	}
> +
> +	msm_rng_ops.priv = (unsigned long) rng;
> +	ret = hwrng_register(&msm_rng_ops);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to register hwrng\n");
> +
> +err_exit:

Doing all that should make this goto exit path unnecessary.

> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int msm_rng_remove(struct platform_device *pdev)
> +{
> +	hwrng_unregister(&msm_rng_ops);
> +	return 0;
> +}
> +
> +static struct of_device_id msm_rng_of_match[] = {

const?

> +	{ .compatible = "qcom,prng", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, msm_rng_of_match);
> +
> +static struct platform_driver msm_rng_driver = {
> +	.probe = msm_rng_probe,
> +	.remove = msm_rng_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(msm_rng_of_match),
> +	}
> +};
> +module_platform_driver(msm_rng_driver);
> +
> +MODULE_AUTHOR("The Linux Foundation");
> +MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
> +MODULE_LICENSE("GPL v2");


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stephen Boyd]

On 10/03/13 07:52, Stanimir Varbanov wrote:
> +#define PRNG_CONFIG_MASK	0x00000002
> +#define PRNG_CONFIG_HW_ENABLE	BIT(1)

These two are the same so please drop the PRNG_CONFIG_MASK define and
just use the PRNG_CONFIG_HW_ENABLE define.

> +#define PRNG_STATUS_DATA_AVAIL	BIT(0)
> +
> +#define MAX_HW_FIFO_DEPTH	16
> +#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
> +#define WORD_SZ			4
> +
> +struct msm_rng {
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static int msm_rng_enable(struct msm_rng *rng, int enable)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(rng->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (enable) {
> +		/* Enable PRNG only if it is not already enabled */
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		if (val & PRNG_CONFIG_HW_ENABLE)
> +			goto already_enabled;
> +
> +		/* PRNG is not enabled */
> +		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
> +		val &= ~PRNG_LFSR_CFG_MASK;
> +		val |= PRNG_LFSR_CFG_CLOCKS;
> +		writel(val, rng->base + PRNG_LFSR_CFG);
> +
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		val &= ~PRNG_CONFIG_MASK;
> +		val |= PRNG_CONFIG_HW_ENABLE;
> +		writel(val, rng->base + PRNG_CONFIG);

This could just be

		val = readl_relaxed(rng->base + PRNG_CONFIG);
		val |= PRNG_CONFIG_HW_ENABLE;
		writel(val, rng->base + PRNG_CONFIG);


> +	} else {
> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
> +		val &= ~PRNG_CONFIG_MASK;
> +		writel(val, rng->base + PRNG_CONFIG);
> +	}
> +
> +already_enabled:
> +	clk_disable_unprepare(rng->clk);
> +	return 0;
> +}
> +
[...]
> +static int msm_rng_probe(struct platform_device *pdev)
> +{
> +	struct msm_rng *rng;
> +	struct device_node *np;
> +	struct resource res;
> +	int ret;
> +
> +	np = of_node_get(pdev->dev.of_node);
> +	if (!np)
> +		return -ENODEV;

This is unnecessary.

> +
> +	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> +	if (!rng) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret)
> +		goto err_exit;

We should just do

  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  rng->base = devm_ioremap_resource(&pdev->dev, res);
  if (IS_ERR(rng->base))
            return PTR_ERR(rng->base);

> +
> +	rng->base = devm_ioremap_resource(&pdev->dev, &res);
> +	if (IS_ERR(rng->base)) {
> +		ret = PTR_ERR(rng->base);
> +		goto err_exit;
> +	}
> +
> +	rng->clk = devm_clk_get(&pdev->dev, "prng");
> +	if (IS_ERR(rng->clk)) {
> +		ret = PTR_ERR(rng->clk);
> +		goto err_exit;
> +	}
> +
> +	msm_rng_ops.priv = (unsigned long) rng;
> +	ret = hwrng_register(&msm_rng_ops);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to register hwrng\n");
> +
> +err_exit:

Doing all that should make this goto exit path unnecessary.

> +	of_node_put(np);
> +	return ret;
> +}
> +
> +static int msm_rng_remove(struct platform_device *pdev)
> +{
> +	hwrng_unregister(&msm_rng_ops);
> +	return 0;
> +}
> +
> +static struct of_device_id msm_rng_of_match[] = {

const?

> +	{ .compatible = "qcom,prng", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, msm_rng_of_match);
> +
> +static struct platform_driver msm_rng_driver = {
> +	.probe = msm_rng_probe,
> +	.remove = msm_rng_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(msm_rng_of_match),
> +	}
> +};
> +module_platform_driver(msm_rng_driver);
> +
> +MODULE_AUTHOR("The Linux Foundation");
> +MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
> +MODULE_LICENSE("GPL v2");


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

Hi Stephen,

Thanks for the quick review!

On 10/03/2013 10:25 PM, Stephen Boyd wrote:
> On 10/03/13 07:52, Stanimir Varbanov wrote:
>> +#define PRNG_CONFIG_MASK	0x00000002
>> +#define PRNG_CONFIG_HW_ENABLE	BIT(1)
> 
> These two are the same so please drop the PRNG_CONFIG_MASK define and
> just use the PRNG_CONFIG_HW_ENABLE define.
> 

OK I will drop the mask and rework the code related to it.

>> +#define PRNG_STATUS_DATA_AVAIL	BIT(0)
>> +
>> +#define MAX_HW_FIFO_DEPTH	16
>> +#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
>> +#define WORD_SZ			4
>> +
>> +struct msm_rng {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +};
>> +
>> +static int msm_rng_enable(struct msm_rng *rng, int enable)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(rng->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (enable) {
>> +		/* Enable PRNG only if it is not already enabled */
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		if (val & PRNG_CONFIG_HW_ENABLE)
>> +			goto already_enabled;
>> +
>> +		/* PRNG is not enabled */
>> +		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
>> +		val &= ~PRNG_LFSR_CFG_MASK;
>> +		val |= PRNG_LFSR_CFG_CLOCKS;
>> +		writel(val, rng->base + PRNG_LFSR_CFG);
>> +
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		val &= ~PRNG_CONFIG_MASK;
>> +		val |= PRNG_CONFIG_HW_ENABLE;
>> +		writel(val, rng->base + PRNG_CONFIG);
> 
> This could just be
> 
> 		val = readl_relaxed(rng->base + PRNG_CONFIG);
> 		val |= PRNG_CONFIG_HW_ENABLE;
> 		writel(val, rng->base + PRNG_CONFIG);
> 
> 
>> +	} else {
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		val &= ~PRNG_CONFIG_MASK;
>> +		writel(val, rng->base + PRNG_CONFIG);
>> +	}
>> +
>> +already_enabled:
>> +	clk_disable_unprepare(rng->clk);
>> +	return 0;
>> +}
>> +
> [...]
>> +static int msm_rng_probe(struct platform_device *pdev)
>> +{
>> +	struct msm_rng *rng;
>> +	struct device_node *np;
>> +	struct resource res;
>> +	int ret;
>> +
>> +	np = of_node_get(pdev->dev.of_node);
>> +	if (!np)
>> +		return -ENODEV;
> 
> This is unnecessary.

I used this call because CONFIG_OF_DYNAMIC could be enabled at some
time. Isn't that possible? I saw that of_node_get|put is used in .probe
on few places in drivers.

> 
>> +
>> +	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
>> +	if (!rng) {
>> +		ret = -ENOMEM;
>> +		goto err_exit;
>> +	}
>> +
>> +	ret = of_address_to_resource(np, 0, &res);
>> +	if (ret)
>> +		goto err_exit;
> 
> We should just do
> 
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   rng->base = devm_ioremap_resource(&pdev->dev, res);
>   if (IS_ERR(rng->base))
>             return PTR_ERR(rng->base);
> 
>> +
>> +	rng->base = devm_ioremap_resource(&pdev->dev, &res);
>> +	if (IS_ERR(rng->base)) {
>> +		ret = PTR_ERR(rng->base);
>> +		goto err_exit;
>> +	}
>> +
>> +	rng->clk = devm_clk_get(&pdev->dev, "prng");
>> +	if (IS_ERR(rng->clk)) {
>> +		ret = PTR_ERR(rng->clk);
>> +		goto err_exit;
>> +	}
>> +
>> +	msm_rng_ops.priv = (unsigned long) rng;
>> +	ret = hwrng_register(&msm_rng_ops);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "failed to register hwrng\n");
>> +
>> +err_exit:
> 
> Doing all that should make this goto exit path unnecessary.
> 
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +
>> +static int msm_rng_remove(struct platform_device *pdev)
>> +{
>> +	hwrng_unregister(&msm_rng_ops);
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id msm_rng_of_match[] = {
> 
> const?
> 
>> +	{ .compatible = "qcom,prng", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, msm_rng_of_match);
>> +
>> +static struct platform_driver msm_rng_driver = {
>> +	.probe = msm_rng_probe,
>> +	.remove = msm_rng_remove,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = of_match_ptr(msm_rng_of_match),
>> +	}
>> +};
>> +module_platform_driver(msm_rng_driver);
>> +
>> +MODULE_AUTHOR("The Linux Foundation");
>> +MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
>> +MODULE_LICENSE("GPL v2");
> 
> 

regards,
Stan

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

Hi Stephen,

Thanks for the quick review!

On 10/03/2013 10:25 PM, Stephen Boyd wrote:
> On 10/03/13 07:52, Stanimir Varbanov wrote:
>> +#define PRNG_CONFIG_MASK	0x00000002
>> +#define PRNG_CONFIG_HW_ENABLE	BIT(1)
> 
> These two are the same so please drop the PRNG_CONFIG_MASK define and
> just use the PRNG_CONFIG_HW_ENABLE define.
> 

OK I will drop the mask and rework the code related to it.

>> +#define PRNG_STATUS_DATA_AVAIL	BIT(0)
>> +
>> +#define MAX_HW_FIFO_DEPTH	16
>> +#define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
>> +#define WORD_SZ			4
>> +
>> +struct msm_rng {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +};
>> +
>> +static int msm_rng_enable(struct msm_rng *rng, int enable)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(rng->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (enable) {
>> +		/* Enable PRNG only if it is not already enabled */
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		if (val & PRNG_CONFIG_HW_ENABLE)
>> +			goto already_enabled;
>> +
>> +		/* PRNG is not enabled */
>> +		val = readl_relaxed(rng->base + PRNG_LFSR_CFG);
>> +		val &= ~PRNG_LFSR_CFG_MASK;
>> +		val |= PRNG_LFSR_CFG_CLOCKS;
>> +		writel(val, rng->base + PRNG_LFSR_CFG);
>> +
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		val &= ~PRNG_CONFIG_MASK;
>> +		val |= PRNG_CONFIG_HW_ENABLE;
>> +		writel(val, rng->base + PRNG_CONFIG);
> 
> This could just be
> 
> 		val = readl_relaxed(rng->base + PRNG_CONFIG);
> 		val |= PRNG_CONFIG_HW_ENABLE;
> 		writel(val, rng->base + PRNG_CONFIG);
> 
> 
>> +	} else {
>> +		val = readl_relaxed(rng->base + PRNG_CONFIG);
>> +		val &= ~PRNG_CONFIG_MASK;
>> +		writel(val, rng->base + PRNG_CONFIG);
>> +	}
>> +
>> +already_enabled:
>> +	clk_disable_unprepare(rng->clk);
>> +	return 0;
>> +}
>> +
> [...]
>> +static int msm_rng_probe(struct platform_device *pdev)
>> +{
>> +	struct msm_rng *rng;
>> +	struct device_node *np;
>> +	struct resource res;
>> +	int ret;
>> +
>> +	np = of_node_get(pdev->dev.of_node);
>> +	if (!np)
>> +		return -ENODEV;
> 
> This is unnecessary.

I used this call because CONFIG_OF_DYNAMIC could be enabled at some
time. Isn't that possible? I saw that of_node_get|put is used in .probe
on few places in drivers.

> 
>> +
>> +	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
>> +	if (!rng) {
>> +		ret = -ENOMEM;
>> +		goto err_exit;
>> +	}
>> +
>> +	ret = of_address_to_resource(np, 0, &res);
>> +	if (ret)
>> +		goto err_exit;
> 
> We should just do
> 
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   rng->base = devm_ioremap_resource(&pdev->dev, res);
>   if (IS_ERR(rng->base))
>             return PTR_ERR(rng->base);
> 
>> +
>> +	rng->base = devm_ioremap_resource(&pdev->dev, &res);
>> +	if (IS_ERR(rng->base)) {
>> +		ret = PTR_ERR(rng->base);
>> +		goto err_exit;
>> +	}
>> +
>> +	rng->clk = devm_clk_get(&pdev->dev, "prng");
>> +	if (IS_ERR(rng->clk)) {
>> +		ret = PTR_ERR(rng->clk);
>> +		goto err_exit;
>> +	}
>> +
>> +	msm_rng_ops.priv = (unsigned long) rng;
>> +	ret = hwrng_register(&msm_rng_ops);
>> +	if (ret)
>> +		dev_err(&pdev->dev, "failed to register hwrng\n");
>> +
>> +err_exit:
> 
> Doing all that should make this goto exit path unnecessary.
> 
>> +	of_node_put(np);
>> +	return ret;
>> +}
>> +
>> +static int msm_rng_remove(struct platform_device *pdev)
>> +{
>> +	hwrng_unregister(&msm_rng_ops);
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id msm_rng_of_match[] = {
> 
> const?
> 
>> +	{ .compatible = "qcom,prng", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, msm_rng_of_match);
>> +
>> +static struct platform_driver msm_rng_driver = {
>> +	.probe = msm_rng_probe,
>> +	.remove = msm_rng_remove,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = of_match_ptr(msm_rng_of_match),
>> +	}
>> +};
>> +module_platform_driver(msm_rng_driver);
>> +
>> +MODULE_AUTHOR("The Linux Foundation");
>> +MODULE_DESCRIPTION("Qualcomm MSM random number generator driver");
>> +MODULE_LICENSE("GPL v2");
> 
> 

regards,
Stan

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stephen Boyd]

On 10/04/13 09:31, Stanimir Varbanov wrote:
>
>>> +static int msm_rng_probe(struct platform_device *pdev)
>>> +{
>>> +	struct msm_rng *rng;
>>> +	struct device_node *np;
>>> +	struct resource res;
>>> +	int ret;
>>> +
>>> +	np = of_node_get(pdev->dev.of_node);
>>> +	if (!np)
>>> +		return -ENODEV;
>> This is unnecessary.
> I used this call because CONFIG_OF_DYNAMIC could be enabled at some
> time. Isn't that possible? I saw that of_node_get|put is used in .probe
> on few places in drivers.

So far we aren't selecting that config on ARM.

If you look at of_device_alloc() you'll see

    dev->dev.of_node = of_node_get(np);

so any platform devices created from of_platform_populate won't have
their of_node go away.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stephen Boyd]

On 10/04/13 09:31, Stanimir Varbanov wrote:
>
>>> +static int msm_rng_probe(struct platform_device *pdev)
>>> +{
>>> +	struct msm_rng *rng;
>>> +	struct device_node *np;
>>> +	struct resource res;
>>> +	int ret;
>>> +
>>> +	np = of_node_get(pdev->dev.of_node);
>>> +	if (!np)
>>> +		return -ENODEV;
>> This is unnecessary.
> I used this call because CONFIG_OF_DYNAMIC could be enabled at some
> time. Isn't that possible? I saw that of_node_get|put is used in .probe
> on few places in drivers.

So far we aren't selecting that config on ARM.

If you look at of_device_alloc() you'll see

    dev->dev.of_node = of_node_get(np);

so any platform devices created from of_platform_populate won't have
their of_node go away.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

Hi Stephen,

On 10/04/2013 07:37 PM, Stephen Boyd wrote:
> On 10/04/13 09:31, Stanimir Varbanov wrote:
>>
>>>> +static int msm_rng_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct msm_rng *rng;
>>>> +	struct device_node *np;
>>>> +	struct resource res;
>>>> +	int ret;
>>>> +
>>>> +	np = of_node_get(pdev->dev.of_node);
>>>> +	if (!np)
>>>> +		return -ENODEV;
>>> This is unnecessary.
>> I used this call because CONFIG_OF_DYNAMIC could be enabled at some
>> time. Isn't that possible? I saw that of_node_get|put is used in .probe
>> on few places in drivers.
> 
> So far we aren't selecting that config on ARM.
> 
> If you look at of_device_alloc() you'll see
> 
>     dev->dev.of_node = of_node_get(np);
> 
> so any platform devices created from of_platform_populate won't have
> their of_node go away.

Thanks for the pointers, it makes sense. I'll remove the calls to
of_node_get|put.

regards,
Stan

Re: [PATCH 2/2] hwrng: msm: Add PRNG support for MSM SoC's

[Stanimir Varbanov]

Hi Stephen,

On 10/04/2013 07:37 PM, Stephen Boyd wrote:
> On 10/04/13 09:31, Stanimir Varbanov wrote:
>>
>>>> +static int msm_rng_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct msm_rng *rng;
>>>> +	struct device_node *np;
>>>> +	struct resource res;
>>>> +	int ret;
>>>> +
>>>> +	np = of_node_get(pdev->dev.of_node);
>>>> +	if (!np)
>>>> +		return -ENODEV;
>>> This is unnecessary.
>> I used this call because CONFIG_OF_DYNAMIC could be enabled at some
>> time. Isn't that possible? I saw that of_node_get|put is used in .probe
>> on few places in drivers.
> 
> So far we aren't selecting that config on ARM.
> 
> If you look at of_device_alloc() you'll see
> 
>     dev->dev.of_node = of_node_get(np);
> 
> so any platform devices created from of_platform_populate won't have
> their of_node go away.

Thanks for the pointers, it makes sense. I'll remove the calls to
of_node_get|put.

regards,
Stan

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Theodore Ts'o]

On Thu, Oct 03, 2013 at 05:52:33PM +0300, Stanimir Varbanov wrote:
> This patch set adds hardware RNG driver wich is used to control the
> Qualcomm's PRNG hardware block.
> The first patch document the DT bindings needed to sucessfuly probe
> the driver and the second patch adds the driver.

Is this really a PRNG (pseudo-random number generator)?  What are the
guarantees which Qualcomm is providing for the PRNG?  If it's really a
PRNG such as AES(i++, NSA_KEY), then kudo to Qualcomm for being
honest.

If it is supposed to be (or at least claimed to be) a secure random
number generator ala RDRAND suitable for use in cryptographic
applications, calling it a PRNG is going to be a bit misleading.

Cheers,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Theodore Ts'o]

On Thu, Oct 03, 2013 at 05:52:33PM +0300, Stanimir Varbanov wrote:
> This patch set adds hardware RNG driver wich is used to control the
> Qualcomm's PRNG hardware block.
> The first patch document the DT bindings needed to sucessfuly probe
> the driver and the second patch adds the driver.

Is this really a PRNG (pseudo-random number generator)?  What are the
guarantees which Qualcomm is providing for the PRNG?  If it's really a
PRNG such as AES(i++, NSA_KEY), then kudo to Qualcomm for being
honest.

If it is supposed to be (or at least claimed to be) a secure random
number generator ala RDRAND suitable for use in cryptographic
applications, calling it a PRNG is going to be a bit misleading.

Cheers,

						- Ted

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

Hi Ted,

On 10/03/2013 07:51 PM, Theodore Ts'o wrote:
> On Thu, Oct 03, 2013 at 05:52:33PM +0300, Stanimir Varbanov wrote:
>> This patch set adds hardware RNG driver wich is used to control the
>> Qualcomm's PRNG hardware block.
>> The first patch document the DT bindings needed to sucessfuly probe
>> the driver and the second patch adds the driver.
> 
> Is this really a PRNG (pseudo-random number generator)?  What are the
> guarantees which Qualcomm is providing for the PRNG?  If it's really a
> PRNG such as AES(i++, NSA_KEY), then kudo to Qualcomm for being
> honest.
> 
> If it is supposed to be (or at least claimed to be) a secure random
> number generator ala RDRAND suitable for use in cryptographic
> applications, calling it a PRNG is going to be a bit misleading.

I guess that it should follow NIST 800-90 recommendation, but I'm not
aware what DRBG mechanism is used.

To be honest I really don't know the hardware implementation details. I
put PRNG abbreviation in the cover letter just because I saw that
defines for register offsets are prefixed with PRNG_*. I could rename
all occurrences of PRNG to RNG. Is that will be enough to avoid confusions?

regadrs,
Stan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

Hi Ted,

On 10/03/2013 07:51 PM, Theodore Ts'o wrote:
> On Thu, Oct 03, 2013 at 05:52:33PM +0300, Stanimir Varbanov wrote:
>> This patch set adds hardware RNG driver wich is used to control the
>> Qualcomm's PRNG hardware block.
>> The first patch document the DT bindings needed to sucessfuly probe
>> the driver and the second patch adds the driver.
> 
> Is this really a PRNG (pseudo-random number generator)?  What are the
> guarantees which Qualcomm is providing for the PRNG?  If it's really a
> PRNG such as AES(i++, NSA_KEY), then kudo to Qualcomm for being
> honest.
> 
> If it is supposed to be (or at least claimed to be) a secure random
> number generator ala RDRAND suitable for use in cryptographic
> applications, calling it a PRNG is going to be a bit misleading.

I guess that it should follow NIST 800-90 recommendation, but I'm not
aware what DRBG mechanism is used.

To be honest I really don't know the hardware implementation details. I
put PRNG abbreviation in the cover letter just because I saw that
defines for register offsets are prefixed with PRNG_*. I could rename
all occurrences of PRNG to RNG. Is that will be enough to avoid confusions?

regadrs,
Stan

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Theodore Ts'o]

On Fri, Oct 04, 2013 at 07:23:50PM +0300, Stanimir Varbanov wrote:
> I guess that it should follow NIST 800-90 recommendation, but I'm not
> aware what DRBG mechanism is used.
> 
> To be honest I really don't know the hardware implementation details. I
> put PRNG abbreviation in the cover letter just because I saw that
> defines for register offsets are prefixed with PRNG_*. I could rename
> all occurrences of PRNG to RNG. Is that will be enough to avoid confusions?

If that's what the Qualcomm documentation uses, maybe we should stick
with it, and add some explanatory comments.  Is there any
documentation for this block that is public that you can either send
me a a pointer to?

Thanks,

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Theodore Ts'o]

On Fri, Oct 04, 2013 at 07:23:50PM +0300, Stanimir Varbanov wrote:
> I guess that it should follow NIST 800-90 recommendation, but I'm not
> aware what DRBG mechanism is used.
> 
> To be honest I really don't know the hardware implementation details. I
> put PRNG abbreviation in the cover letter just because I saw that
> defines for register offsets are prefixed with PRNG_*. I could rename
> all occurrences of PRNG to RNG. Is that will be enough to avoid confusions?

If that's what the Qualcomm documentation uses, maybe we should stick
with it, and add some explanatory comments.  Is there any
documentation for this block that is public that you can either send
me a a pointer to?

Thanks,

					- Ted

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

Hi Ted,

On 10/04/2013 09:10 PM, Theodore Ts'o wrote:
> On Fri, Oct 04, 2013 at 07:23:50PM +0300, Stanimir Varbanov wrote:
>> I guess that it should follow NIST 800-90 recommendation, but I'm not
>> aware what DRBG mechanism is used.
>>
>> To be honest I really don't know the hardware implementation details. I
>> put PRNG abbreviation in the cover letter just because I saw that
>> defines for register offsets are prefixed with PRNG_*. I could rename
>> all occurrences of PRNG to RNG. Is that will be enough to avoid confusions?
> 
> If that's what the Qualcomm documentation uses, maybe we should stick
> with it, and add some explanatory comments.  Is there any
> documentation for this block that is public that you can either send
> me a a pointer to?

No, there is no public documentation for the block. Here is the driver
documentation which I used as a base [1].

My guess was that - if it is PRNG (got from hardware description link
above) than according to wiki [2] it is also known as a deterministic
random bit generator (DRBG). The recommendation for RNG using DRBG is
NIST 800-90.

Of course I could be wrong, so I can add a comment that this is just a
guess and we shouldn't over-reliance on this.

regards,
Stan

[1]
https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/Documentation/arm/msm/msm_rng-driver.txt?h=jb_3.2.1

[2] http://en.wikipedia.org/wiki/Pseudorandom_number_generator

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[H. Peter Anvin]

On 10/09/2013 07:46 AM, Stanimir Varbanov wrote:
> 
> No, there is no public documentation for the block. Here is the driver
> documentation which I used as a base [1].
> 
> My guess was that - if it is PRNG (got from hardware description link
> above) than according to wiki [2] it is also known as a deterministic
> random bit generator (DRBG). The recommendation for RNG using DRBG is
> NIST 800-90.
> 
> Of course I could be wrong, so I can add a comment that this is just a
> guess and we shouldn't over-reliance on this.
> 

There needs to be an architecturally guaranteed lower bound on the
entropic content for this to be at all useful.  However, the hwrandom
interface is currently expecting fully entropic output (which is almost
certainly bogus... consider the PowerPC random number generator[1]) and
so using it for a PRNG output is directly wrong.  This is part of why
RDRAND support is implemented directly in rngd so that we can do the
required cryptographic data reduction to produce fully entropic output.

	-hpa



[1] which has a known first-order bias which they "correct" for by
XORing two datums together in a very simple data reduction step.
However, if their random source has bias it is extremely likely it also
has nonzero correlations, which require stronger reductions.  It would
make a lot more sense to feed this data into the random pools but
derated at a lower entropy level.  This would be useful for RDRAND as well.

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Theodore Ts'o]

On Wed, Oct 09, 2013 at 08:07:35AM -0700, H. Peter Anvin wrote:
> There needs to be an architecturally guaranteed lower bound on the
> entropic content for this to be at all useful.  However, the hwrandom
> interface is currently expecting fully entropic output (which is almost
> certainly bogus... consider the PowerPC random number generator[1]) and
> so using it for a PRNG output is directly wrong.  

You can specify as a command-line argument (-H) to rngd the entropy
per bit of input data.  So if you think an entropy source isn't great,
but has some uncertainty, you could do pass to rngd "-H 0.5" or maybe
even "-H 0.1".

Maybe it would be nice to have /dev/hwrandom export the quality of its
output, but the reality is that most hardware devices don't document
or export via some programmatic interface how well or how poorly their
hwrng really might be.  And even if they did, most people, who don't
have access to scanning electronic microscopes and nanometer probes,
and the ability to decrypt, reverse engineer, and decompile firmware,
couldn't know for sure whether or not to believe the claims of the
hardware or the hardware manufacturer anyway.

							- Ted

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[H. Peter Anvin]

On 10/09/2013 09:03 AM, Theodore Ts'o wrote:
> On Wed, Oct 09, 2013 at 08:07:35AM -0700, H. Peter Anvin wrote:
>> There needs to be an architecturally guaranteed lower bound on the
>> entropic content for this to be at all useful.  However, the hwrandom
>> interface is currently expecting fully entropic output (which is almost
>> certainly bogus... consider the PowerPC random number generator[1]) and
>> so using it for a PRNG output is directly wrong.  
> 
> You can specify as a command-line argument (-H) to rngd the entropy
> per bit of input data.  So if you think an entropy source isn't great,
> but has some uncertainty, you could do pass to rngd "-H 0.5" or maybe
> even "-H 0.1".
> 
> Maybe it would be nice to have /dev/hwrandom export the quality of its
> output, but the reality is that most hardware devices don't document
> or export via some programmatic interface how well or how poorly their
> hwrng really might be.  And even if they did, most people, who don't
> have access to scanning electronic microscopes and nanometer probes,
> and the ability to decrypt, reverse engineer, and decompile firmware,
> couldn't know for sure whether or not to believe the claims of the
> hardware or the hardware manufacturer anyway.
> 

There is no -H option in upstream rngd.  It might be in the Debian fork,
but the Debian fork has serious other problems.  I don't understand how
that would work with the FIPS tests in rngd, unless of course the FIPS
tests are so weak they are pointless anyway (they are

/dev/hwrng should definitely export it rather than having to have the
*user* enter that data... the user has even less opportunity than the
vendor and driver maintainer to get this even remotely right, I fear.

It is really problematic when there isn't even anything to hang things
off, and perhaps a real question is if we even should have drivers for
devices where there isn't any kind of documentation given, or if we
should derate them substantially.

The RDRAND code in rngd already has cryptographic (AES) data reduction
and we might want to extend that code to be able to derate other data
sources.

Or we should just move this into a kernel thread and let the pool do that...

	-hpa

Re: rngd (was: [PATCH 0/2] Add support for Qualcomm's PRNG)

[Clemens Ladisch]

H. Peter Anvin wrote:
> On 10/09/2013 09:03 AM, Theodore Ts'o wrote:
>> You can specify as a command-line argument (-H) to rngd the entropy
>> per bit of input data.
>
> There is no -H option in upstream rngd.  It might be in the Debian fork,
> but the Debian fork has serious other problems.

What problems?  I have been thinking about adding another entropy source
to rngd, and was wondering which fork to use, or if it would make sense
to merge them.  Are there any features of the Debian fork that should
not be ported to upstream?

> I don't understand how that would work with the FIPS tests in rngd,
> unless of course the FIPS tests are so weak they are pointless anyway

Most of the FIPS tests assume that the bits are independently generated
(the two other tests check for correlations in 4/32-bit groups).  None
of these tests make sense if the bit stream is the output of an AES
conditioner.  For RDRAND, it might be useful to check that we don't
accidentally get a series of zeros or something like that, but otherwise
we have to trust the built-in tests that Intel claims the hardware is
doing before conditioning.

As it happens, the 2002-12-03 change notice of FIPS 140-2 dropped the
RNG tests.


For the entropy source I've been thinking about (captured audio
samples), the FIPS tests would make sense only if done independently on
each bit in the sample (e.g., with 24-bit samples, there would be 24
parallel bit streams, most of which wouldn't be random).  Additional
tests to check for correlations between the bits in a sample would be
useful, too.

What I'm trying to say with all this is that self-tests must be
customized for each entropy source.


Regards,
Clemens

Re: rngd

[H. Peter Anvin]

On 10/10/2013 12:46 AM, Clemens Ladisch wrote:
> H. Peter Anvin wrote:
>> On 10/09/2013 09:03 AM, Theodore Ts'o wrote:
>>> You can specify as a command-line argument (-H) to rngd the entropy
>>> per bit of input data.
>>
>> There is no -H option in upstream rngd.  It might be in the Debian fork,
>> but the Debian fork has serious other problems.
> 
> What problems?  I have been thinking about adding another entropy source
> to rngd, and was wondering which fork to use, or if it would make sense
> to merge them.  Are there any features of the Debian fork that should
> not be ported to upstream?
> 

Mainly the maintainer isn't merging in fixes from upstream, apparently
because he has misunderstood their function.

>> I don't understand how that would work with the FIPS tests in rngd,
>> unless of course the FIPS tests are so weak they are pointless anyway
> 
> Most of the FIPS tests assume that the bits are independently generated
> (the two other tests check for correlations in 4/32-bit groups).  None
> of these tests make sense if the bit stream is the output of an AES
> conditioner.  For RDRAND, it might be useful to check that we don't
> accidentally get a series of zeros or something like that, but otherwise
> we have to trust the built-in tests that Intel claims the hardware is
> doing before conditioning.
> 
> As it happens, the 2002-12-03 change notice of FIPS 140-2 dropped the
> RNG tests.
> 
> For the entropy source I've been thinking about (captured audio
> samples), the FIPS tests would make sense only if done independently on
> each bit in the sample (e.g., with 24-bit samples, there would be 24
> parallel bit streams, most of which wouldn't be random).  Additional
> tests to check for correlations between the bits in a sample would be
> useful, too.
> 
> What I'm trying to say with all this is that self-tests must be
> customized for each entropy source.
> 

Yes.  I don't think the FIPS tests make any sense at all (up to and
including rngd 3 they would eventually kill rngd, because it only
allowed for a fixed number of false positives.)

	-hpa

Re: rngd

[Theodore Ts'o]

On Thu, Oct 10, 2013 at 08:08:41AM -0700, H. Peter Anvin wrote:
> 
> Mainly the maintainer isn't merging in fixes from upstream, apparently
> because he has misunderstood their function.

>From the README file from the Debian fork:

rng-tools unofficial-mt is a living reminder to myself to not modify upstream
code without sending the changes upstream at every step.  Suddenly, you have a
mass of changes too big to send upstream, and yet you find yourself without the
energy to break them into smallish patches to submit upstream (i.e. to
"unfork").

> > What I'm trying to say with all this is that self-tests must be
> > customized for each entropy source.
> 
> Yes.  I don't think the FIPS tests make any sense at all (up to and
> including rngd 3 they would eventually kill rngd, because it only
> allowed for a fixed number of false positives.)

... and to the extent that output is crypto-whitened in the hardware,
with no way of disabling the whitening, it's actually kind of hopeless
to do any kind of testing.

One of the reasons why I wanted to keep this functionality in
userspace, as opposed to moving this into a kernel thread, is because
I was hoping someone would create hardware which did not do hardware
whitening, and userspace could do proper quality checking of the
entropy source, and data reduction as necessary, all in the an open
and auditable way.  If we are doing the those more heavyweight tests,
it's not a clear it makes sense to put all of this in the kernel.

     	   	    	  	       - Ted

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Paul Mackerras]

On Wed, Oct 09, 2013 at 08:07:35AM -0700, H. Peter Anvin wrote:

> consider the PowerPC random number generator[1]) and

[snip]

> [1] which has a known first-order bias which they "correct" for by
> XORing two datums together in a very simple data reduction step.

65 actually, not two.

> However, if their random source has bias it is extremely likely it also
> has nonzero correlations, which require stronger reductions.  It would

The correlations are essentially zero, by design, and experiment
confirms it.  Did you see my mail on the kvm list where I explained
how it works?

Paul.

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[H. Peter Anvin]

On 10/10/2013 03:41 AM, Paul Mackerras wrote:
> On Wed, Oct 09, 2013 at 08:07:35AM -0700, H. Peter Anvin wrote:
> 
>> consider the PowerPC random number generator[1]) and
> 
> [snip]
> 
>> [1] which has a known first-order bias which they "correct" for by
>> XORing two datums together in a very simple data reduction step.
> 
> 65 actually, not two.
> 
>> However, if their random source has bias it is extremely likely it also
>> has nonzero correlations, which require stronger reductions.  It would
> 
> The correlations are essentially zero, by design, and experiment
> confirms it.  Did you see my mail on the kvm list where I explained
> how it works?
> 

No, sorry... I got a bit of detached discussion as part of benh talking
about KVM and randomness (for the record, I'm all for better randomness
on all platforms.)

Either way, XORing samples is a pretty inefficient (both in terms of
anticorrelation and in terms of entropy efficiency) form of data
reduction/conditioning.  It would still be better to feed the output
into the pool with a 65x derating.

	-hpa

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Stanimir Varbanov]

Hi Ted, Peter,

On 10/09/2013 06:07 PM, H. Peter Anvin wrote:
> On 10/09/2013 07:46 AM, Stanimir Varbanov wrote:
>>
>> No, there is no public documentation for the block. Here is the driver
>> documentation which I used as a base [1].
>>
>> My guess was that - if it is PRNG (got from hardware description link
>> above) than according to wiki [2] it is also known as a deterministic
>> random bit generator (DRBG). The recommendation for RNG using DRBG is
>> NIST 800-90.
>>
>> Of course I could be wrong, so I can add a comment that this is just a
>> guess and we shouldn't over-reliance on this.
>>
> 
> There needs to be an architecturally guaranteed lower bound on the
> entropic content for this to be at all useful.  However, the hwrandom
> interface is currently expecting fully entropic output (which is almost
> certainly bogus... consider the PowerPC random number generator[1]) and
> so using it for a PRNG output is directly wrong.  This is part of why
> RDRAND support is implemented directly in rngd so that we can do the
> required cryptographic data reduction to produce fully entropic output.

I ran the rngtest with following command line:

# cat /dev/hw_random | rngtest -c 100000

Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions.  There is
NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE.

rngtest: starting FIPS tests...
rngtest: bits received from input: 2000000032
rngtest: FIPS 140-2 successes: 99925
rngtest: FIPS 140-2 failures: 75
rngtest: FIPS 140-2(2001-10-10) Monobit: 10
rngtest: FIPS 140-2(2001-10-10) Poker: 9
rngtest: FIPS 140-2(2001-10-10) Runs: 20
rngtest: FIPS 140-2(2001-10-10) Long run: 38
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=1.267; avg=53.222; max=2384.186)Mibits/s
rngtest: FIPS tests speed: (min=3.016; avg=48.847; max=49.931)Mibits/s
rngtest: Program run time: 75191914 microseconds

Could you guys comment those results?

regards,
Stan

Re: [PATCH 0/2] Add support for Qualcomm's PRNG

[Clemens Ladisch]

Stanimir Varbanov wrote:
> I ran the rngtest with following command line:
>
> # cat /dev/hw_random | rngtest -c 100000
> ...
> rngtest: bits received from input: 2000000032
> rngtest: FIPS 140-2 successes: 99925
> rngtest: FIPS 140-2 failures: 75
> ...
>
> Could you guys comment those results?

These tests are expected to fail a few times even for perfectly random
data (think monkeys/Shakespeare).  An entropy source will be permanently
disabled by rngd only if the failure ratio is worse than 1 in 1000.


Regards,
Clemens