[PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

v1 => v2:
 - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
 - Fix 2099 => 2009 typo in commit log
 - Fix 'number of random numbers sourced' return value
 - Treat devm_clk_get()'s return value correctly
 - Check return value of clk_prepare_enable()
 - Use sysfs_streq() instead of manually stripping '\n' from sysfs

The main aim of this set is to allow users to access ST's hardware
random number generator.  It's a simple device, which only requires
a simple driver.

We're also taking the liberty to update some out of date HWRNG
documentation and making the sysfs interface a little easier to
use by ignoring any '\n' which may have been inadvertently passed.

Lee Jones (7):
  Documentation: hw_random: Fix device node name reference
    /dev/hw_random => /dev/hwrng
  hwrng: Kconfig: Fix device node name reference /dev/hw_random =>
    /dev/hwrng
  hwrng: core: Simplify RNG switching from sysfs
  hwrng: st: Provide DT bindings for ST's Random Number Generator
  hwrng: st: Add support for ST's HW Random Number Generator
  ARM: STi: STiH407: Enable the 2 HW Random Number Generators for
    STiH4{07,10}
  MAINTAINERS: Add ST's Random Number Generator to the ST entry

 Documentation/devicetree/bindings/rng/st,rng.txt |  15 +++
 Documentation/hw_random.txt                      |   8 +-
 MAINTAINERS                                      |   1 +
 arch/arm/boot/dts/stih407-family.dtsi            |  14 +++
 drivers/char/hw_random/Kconfig                   |  12 +-
 drivers/char/hw_random/Makefile                  |   1 +
 drivers/char/hw_random/core.c                    |   2 +-
 drivers/char/hw_random/st-rng.c                  | 144 +++++++++++++++++++++++
 8 files changed, 191 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rng/st,rng.txt
 create mode 100644 drivers/char/hw_random/st-rng.c

-- 
1.9.1

[PATCH v2 1/7] Documentation: hw_random: Fix device node name reference /dev/hw_random => /dev/hwrng

[Lee Jones]

In April 2009, commit d405640 ("Driver Core: misc: add node name support
for misc devices.") inadvertently changed the device node name from
/dev/hw_random to /dev/hwrng.  Since 6 years has passed since the change
it seems unpractical to change it back, as this node name is probably
considered ABI by now.  So instead, we'll just change the documentation
to match the current situation.

NB: It looks like rng-tools have already been updated.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/hw_random.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/hw_random.txt b/Documentation/hw_random.txt
index 026e237..fce1634 100644
--- a/Documentation/hw_random.txt
+++ b/Documentation/hw_random.txt
@@ -3,7 +3,7 @@ Introduction:
 	The hw_random framework is software that makes use of a
 	special hardware feature on your CPU or motherboard,
 	a Random Number Generator (RNG).  The software has two parts:
-	a core providing the /dev/hw_random character device and its
+	a core providing the /dev/hwrng character device and its
 	sysfs support, plus a hardware-specific driver that plugs
 	into that core.
 
@@ -14,7 +14,7 @@ Introduction:
 
 		http://sourceforge.net/projects/gkernel/
 
-	Those tools use /dev/hw_random to fill the kernel entropy pool,
+	Those tools use /dev/hwrng to fill the kernel entropy pool,
 	which is used internally and exported by the /dev/urandom and
 	/dev/random special files.
 
@@ -32,13 +32,13 @@ Theory of operation:
 	The rng-tools package uses such tests in "rngd", and lets you
 	run them by hand with a "rngtest" utility.
 
-	/dev/hw_random is char device major 10, minor 183.
+	/dev/hwrng is char device major 10, minor 183.
 
 	CLASS DEVICE.  There is a /sys/class/misc/hw_random node with
 	two unique attributes, "rng_available" and "rng_current".  The
 	"rng_available" attribute lists the hardware-specific drivers
 	available, while "rng_current" lists the one which is currently
-	connected to /dev/hw_random.  If your system has more than one
+	connected to /dev/hwrng.  If your system has more than one
 	RNG available, you may change the one used by writing a name from
 	the list in "rng_available" into "rng_current".
 
-- 
1.9.1

Re: [PATCH v2 1/7] Documentation: hw_random: Fix device node name reference /dev/hw_random => /dev/hwrng

[Kieran Bingham]

On 17 September 2015 at 14:45, Lee Jones <lee.jones@linaro.org> wrote:
> In April 2009, commit d405640 ("Driver Core: misc: add node name support
> for misc devices.") inadvertently changed the device node name from
> /dev/hw_random to /dev/hwrng.  Since 6 years has passed since the change
> it seems unpractical to change it back, as this node name is probably
> considered ABI by now.  So instead, we'll just change the documentation
> to match the current situation.
>
> NB: It looks like rng-tools have already been updated.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

I see the KConfig change went into a separate patch.
That's fine by me:

Acked-by: Kieran Bingham <kieranbingham@gmail.com>

> ---
>  Documentation/hw_random.txt | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/hw_random.txt b/Documentation/hw_random.txt
> index 026e237..fce1634 100644
> --- a/Documentation/hw_random.txt
> +++ b/Documentation/hw_random.txt
> @@ -3,7 +3,7 @@ Introduction:
>         The hw_random framework is software that makes use of a
>         special hardware feature on your CPU or motherboard,
>         a Random Number Generator (RNG).  The software has two parts:
> -       a core providing the /dev/hw_random character device and its
> +       a core providing the /dev/hwrng character device and its
>         sysfs support, plus a hardware-specific driver that plugs
>         into that core.
>
> @@ -14,7 +14,7 @@ Introduction:
>
>                 http://sourceforge.net/projects/gkernel/
>
> -       Those tools use /dev/hw_random to fill the kernel entropy pool,
> +       Those tools use /dev/hwrng to fill the kernel entropy pool,
>         which is used internally and exported by the /dev/urandom and
>         /dev/random special files.
>
> @@ -32,13 +32,13 @@ Theory of operation:
>         The rng-tools package uses such tests in "rngd", and lets you
>         run them by hand with a "rngtest" utility.
>
> -       /dev/hw_random is char device major 10, minor 183.
> +       /dev/hwrng is char device major 10, minor 183.
>
>         CLASS DEVICE.  There is a /sys/class/misc/hw_random node with
>         two unique attributes, "rng_available" and "rng_current".  The
>         "rng_available" attribute lists the hardware-specific drivers
>         available, while "rng_current" lists the one which is currently
> -       connected to /dev/hw_random.  If your system has more than one
> +       connected to /dev/hwrng.  If your system has more than one
>         RNG available, you may change the one used by writing a name from
>         the list in "rng_available" into "rng_current".
>
> --
> 1.9.1
>

[PATCH v2 2/7] hwrng: Kconfig: Fix device node name reference /dev/hw_random => /dev/hwrng

[Lee Jones]

In April 2009, commit d405640 ("Driver Core: misc: add node name support
for misc devices.") inadvertently changed the device node name from
/dev/hw_random to /dev/hwrng.  Since 6 years has passed since the change
it seems unpractical to change it back, as this node name is probably
considered ABI by now.  So instead, we'll just change the Kconfig help
to match the current situation.

NB: It looks like rng-tools have already been updated.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/char/hw_random/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index f48cf11..8998108 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -10,7 +10,7 @@ menuconfig HW_RANDOM
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called rng-core.  This provides a device
-	  that's usually called /dev/hw_random, and which exposes one
+	  that's usually called /dev/hwrng, and which exposes one
 	  of possibly several hardware random number generators.
 
 	  These hardware random number generators do not feed directly
-- 
1.9.1

Re: [PATCH v2 2/7] hwrng: Kconfig: Fix device node name reference /dev/hw_random => /dev/hwrng

[Kieran Bingham]

On 17 September 2015 at 14:45, Lee Jones <lee.jones@linaro.org> wrote:
> In April 2009, commit d405640 ("Driver Core: misc: add node name support
> for misc devices.") inadvertently changed the device node name from
> /dev/hw_random to /dev/hwrng.  Since 6 years has passed since the change
> it seems unpractical to change it back, as this node name is probably
> considered ABI by now.  So instead, we'll just change the Kconfig help
> to match the current situation.
>
> NB: It looks like rng-tools have already been updated.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Kieran Bingham <kieranbingham@gmail.com>

> ---
>  drivers/char/hw_random/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index f48cf11..8998108 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -10,7 +10,7 @@ menuconfig HW_RANDOM
>
>           To compile this driver as a module, choose M here: the
>           module will be called rng-core.  This provides a device
> -         that's usually called /dev/hw_random, and which exposes one
> +         that's usually called /dev/hwrng, and which exposes one
>           of possibly several hardware random number generators.
>
>           These hardware random number generators do not feed directly
> --
> 1.9.1
>

[PATCH v2 3/7] hwrng: core: Simplify RNG switching from sysfs

[Lee Jones]

If we attempt to use sysfs to change the current RNG in the usual
way i.e. issuing something like:

`echo 8a8a000.rng > /sys/devices/virtual/misc/hw_random/rng_current`

... it will fail because the code doesn't currently take the '\n'
into consideration.  Well, now it does.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/char/hw_random/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index da8faf7..2d4a969 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -323,7 +323,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 		return -ERESTARTSYS;
 	err = -ENODEV;
 	list_for_each_entry(rng, &rng_list, list) {
-		if (strcmp(rng->name, buf) == 0) {
+		if (sysfs_streq(rng->name, buf)) {
 			err = 0;
 			if (rng != current_rng)
 				err = set_current_rng(rng);
-- 
1.9.1

Re: [PATCH v2 3/7] hwrng: core: Simplify RNG switching from sysfs

[Peter Korsgaard]

>>>>> "Lee" == Lee Jones <lee.jones@linaro.org> writes:

 > If we attempt to use sysfs to change the current RNG in the usual
 > way i.e. issuing something like:

 > `echo 8a8a000.rng > /sys/devices/virtual/misc/hw_random/rng_current`

 > ... it will fail because the code doesn't currently take the '\n'
 > into consideration.  Well, now it does.

 > Signed-off-by: Lee Jones <lee.jones@linaro.org>
 > ---
 >  drivers/char/hw_random/core.c | 2 +-
 >  1 file changed, 1 insertion(+), 1 deletion(-)

 > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
 > index da8faf7..2d4a969 100644
 > --- a/drivers/char/hw_random/core.c
 > +++ b/drivers/char/hw_random/core.c
 > @@ -323,7 +323,7 @@ static ssize_t hwrng_attr_current_store(struct device *dev,
 >  		return -ERESTARTSYS;
 >  	err = -ENODEV;
 >  	list_for_each_entry(rng, &rng_list, list) {
 > -		if (strcmp(rng->name, buf) == 0) {
 > +		if (sysfs_streq(rng->name, buf)) {

Looks good.

Acked-by: Peter Korsgaard <peter@korsgaard.com>

-- 
Venlig hilsen,
Peter Korsgaard 

[PATCH v2 4/7] hwrng: st: Provide DT bindings for ST's Random Number Generator

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/rng/st,rng.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/st,rng.txt

diff --git a/Documentation/devicetree/bindings/rng/st,rng.txt b/Documentation/devicetree/bindings/rng/st,rng.txt
new file mode 100644
index 0000000..dbc64e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/st,rng.txt
@@ -0,0 +1,15 @@
+STMicroelectronics HW Random Number Generator
+----------------------------------------------
+
+Required parameters:
+compatible	: Should be "st,rng"
+reg		: Base address and size of IP's register map.
+clocks		: Phandle to device's clock (See: ../clocks/clock-bindings.txt)
+
+Example:
+
+rng@0xfee80000 {
+	compatible      = "st,rng";
+	reg		= <0xfee80000 0x1000>;
+	clocks          = <&clk_sysin>;
+}
-- 
1.9.1

Re: [PATCH v2 4/7] hwrng: st: Provide DT bindings for ST's Random Number Generator

[Stephen Boyd]

On 09/17/2015 06:45 AM, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/rng/st,rng.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/st,rng.txt
>
> diff --git a/Documentation/devicetree/bindings/rng/st,rng.txt b/Documentation/devicetree/bindings/rng/st,rng.txt
> new file mode 100644
> index 0000000..dbc64e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/st,rng.txt
> @@ -0,0 +1,15 @@
> +STMicroelectronics HW Random Number Generator
> +----------------------------------------------
> +
> +Required parameters:
> +compatible	: Should be "st,rng"
> +reg		: Base address and size of IP's register map.
> +clocks		: Phandle to device's clock (See: ../clocks/clock-bindings.txt)
> +
> +Example:
> +
> +rng@0xfee80000 {

Drop the 0x part please.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/7] hwrng: st: Provide DT bindings for ST's Random Number Generator

[Lee Jones]

On 18 September 2015 at 17:21, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/17/2015 06:45 AM, Lee Jones wrote:
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/rng/st,rng.txt | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/rng/st,rng.txt
>>
>> diff --git a/Documentation/devicetree/bindings/rng/st,rng.txt b/Documentation/devicetree/bindings/rng/st,rng.txt
>> new file mode 100644
>> index 0000000..dbc64e6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rng/st,rng.txt
>> @@ -0,0 +1,15 @@
>> +STMicroelectronics HW Random Number Generator
>> +----------------------------------------------
>> +
>> +Required parameters:
>> +compatible   : Should be "st,rng"
>> +reg          : Base address and size of IP's register map.
>> +clocks               : Phandle to device's clock (See: ../clocks/clock-bindings.txt)
>> +
>> +Example:
>> +
>> +rng@0xfee80000 {
>
> Drop the 0x part please.

Good spot.  Will fix as a subsequent patch.

-- 
Lee Jones
Linaro ST Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Lee Jones]

Signed-off-by: Pankaj Dev <pankaj.dev@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/char/hw_random/Kconfig  |  10 +++
 drivers/char/hw_random/Makefile |   1 +
 drivers/char/hw_random/st-rng.c | 144 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 drivers/char/hw_random/st-rng.c

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 8998108..ba5406b 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -346,6 +346,16 @@ config HW_RANDOM_MSM
 
 	  If unsure, say Y.
 
+config HW_RANDOM_ST
+	tristate "ST Microelectronics HW Random Number Generator support"
+	depends on HW_RANDOM && ARCH_STI
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator hardware found on STi series of SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called st-rng.
+
 config HW_RANDOM_XGENE
 	tristate "APM X-Gene True Random Number Generator (TRNG) support"
 	depends on HW_RANDOM && ARCH_XGENE
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 055bb01..8bcfb45 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -30,4 +30,5 @@ 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_ST) += st-rng.o
 obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
new file mode 100644
index 0000000..8c8a435
--- /dev/null
+++ b/drivers/char/hw_random/st-rng.c
@@ -0,0 +1,144 @@
+/*
+ * ST Random Number Generator Driver ST's Platforms
+ *
+ * Author: Pankaj Dev: <pankaj.dev@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
+ *
+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Registers */
+#define ST_RNG_STATUS_REG		0x20
+#define ST_RNG_DATA_REG			0x24
+
+/* Registers fields */
+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
+
+#define ST_RNG_FIFO_SIZE		8
+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
+
+/* Samples are available every 0.667us, which we round to 1us */
+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
+
+struct st_rng_data {
+	void __iomem	*base;
+	struct clk	*clk;
+	struct hwrng	ops;
+};
+
+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
+	u32 status;
+	int i;
+
+	if (max < sizeof(u16))
+		return -EINVAL;
+
+	/* Wait until FIFO is full - max 4uS*/
+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
+		if (status & ST_RNG_STATUS_FIFO_FULL)
+			break;
+		udelay(1);
+	}
+
+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
+		return 0;
+
+	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
+		*(u16 *)(data + i) =
+			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
+
+	return i;	/* No of bytes read */
+}
+
+static int st_rng_probe(struct platform_device *pdev)
+{
+	struct st_rng_data *ddata;
+	struct resource *res;
+	struct clk *clk;
+	void __iomem *base;
+	int ret;
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	ddata->ops.priv	= (unsigned long)ddata;
+	ddata->ops.read	= st_rng_read;
+	ddata->ops.name	= pdev->name;
+	ddata->base	= base;
+	ddata->clk	= clk;
+
+	dev_set_drvdata(&pdev->dev, ddata);
+
+	ret = hwrng_register(&ddata->ops);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register HW RNG\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
+
+	return 0;
+}
+
+static int st_rng_remove(struct platform_device *pdev)
+{
+	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
+
+	hwrng_unregister(&ddata->ops);
+
+	clk_disable_unprepare(ddata->clk);
+
+	return 0;
+}
+
+static const struct of_device_id st_rng_match[] = {
+	{ .compatible = "st,rng" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st_rng_match);
+
+static struct platform_driver st_rng_driver = {
+	.driver = {
+		.name = "st-hwrandom",
+		.of_match_table = of_match_ptr(st_rng_match),
+	},
+	.probe = st_rng_probe,
+	.remove = st_rng_remove
+};
+
+module_platform_driver(st_rng_driver);
+
+MODULE_AUTHOR("Pankaj Dev <pankaj.dev@st.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Kieran Bingham]

On 17 September 2015 at 14:45, Lee Jones <lee.jones@linaro.org> wrote:
> Signed-off-by: Pankaj Dev <pankaj.dev@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Comments addressed, Also LGTM.
Acked-by: Kieran Bingham <kieranbingham@gmail.com>


> ---
>  drivers/char/hw_random/Kconfig  |  10 +++
>  drivers/char/hw_random/Makefile |   1 +
>  drivers/char/hw_random/st-rng.c | 144 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 drivers/char/hw_random/st-rng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 8998108..ba5406b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -346,6 +346,16 @@ config HW_RANDOM_MSM
>
>           If unsure, say Y.
>
> +config HW_RANDOM_ST
> +       tristate "ST Microelectronics HW Random Number Generator support"
> +       depends on HW_RANDOM && ARCH_STI
> +       ---help---
> +         This driver provides kernel-side support for the Random Number
> +         Generator hardware found on STi series of SoCs.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called st-rng.
> +
>  config HW_RANDOM_XGENE
>         tristate "APM X-Gene True Random Number Generator (TRNG) support"
>         depends on HW_RANDOM && ARCH_XGENE
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 055bb01..8bcfb45 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -30,4 +30,5 @@ 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_ST) += st-rng.o
>  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> new file mode 100644
> index 0000000..8c8a435
> --- /dev/null
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -0,0 +1,144 @@
> +/*
> + * ST Random Number Generator Driver ST's Platforms
> + *
> + * Author: Pankaj Dev: <pankaj.dev@st.com>
> + *         Lee Jones <lee.jones@linaro.org>
> + *
> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Registers */
> +#define ST_RNG_STATUS_REG              0x20
> +#define ST_RNG_DATA_REG                        0x24
> +
> +/* Registers fields */
> +#define ST_RNG_STATUS_BAD_SEQUENCE     BIT(0)
> +#define ST_RNG_STATUS_BAD_ALTERNANCE   BIT(1)
> +#define ST_RNG_STATUS_FIFO_FULL                BIT(5)
> +
> +#define ST_RNG_FIFO_SIZE               8
> +#define ST_RNG_SAMPLE_SIZE             2 /* 2 Byte (16bit) samples */
> +
> +/* Samples are available every 0.667us, which we round to 1us */
> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> +
> +struct st_rng_data {
> +       void __iomem    *base;
> +       struct clk      *clk;
> +       struct hwrng    ops;
> +};
> +
> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +       struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> +       u32 status;
> +       int i;
> +
> +       if (max < sizeof(u16))
> +               return -EINVAL;
> +
> +       /* Wait until FIFO is full - max 4uS*/
> +       for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> +               status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> +               if (status & ST_RNG_STATUS_FIFO_FULL)
> +                       break;
> +               udelay(1);
> +       }
> +
> +       if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> +               return 0;
> +
> +       for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> +               *(u16 *)(data + i) =
> +                       readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> +
> +       return i;       /* No of bytes read */
> +}
> +
> +static int st_rng_probe(struct platform_device *pdev)
> +{
> +       struct st_rng_data *ddata;
> +       struct resource *res;
> +       struct clk *clk;
> +       void __iomem *base;
> +       int ret;
> +
> +       ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> +       if (!ddata)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +
> +       ret = clk_prepare_enable(clk);
> +       if (ret)
> +               return ret;
> +
> +       ddata->ops.priv = (unsigned long)ddata;
> +       ddata->ops.read = st_rng_read;
> +       ddata->ops.name = pdev->name;
> +       ddata->base     = base;
> +       ddata->clk      = clk;
> +
> +       dev_set_drvdata(&pdev->dev, ddata);
> +
> +       ret = hwrng_register(&ddata->ops);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register HW RNG\n");
> +               return ret;
> +       }
> +
> +       dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> +
> +       return 0;
> +}
> +
> +static int st_rng_remove(struct platform_device *pdev)
> +{
> +       struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> +
> +       hwrng_unregister(&ddata->ops);
> +
> +       clk_disable_unprepare(ddata->clk);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id st_rng_match[] = {
> +       { .compatible = "st,rng" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, st_rng_match);
> +
> +static struct platform_driver st_rng_driver = {
> +       .driver = {
> +               .name = "st-hwrandom",
> +               .of_match_table = of_match_ptr(st_rng_match),
> +       },
> +       .probe = st_rng_probe,
> +       .remove = st_rng_remove
> +};
> +
> +module_platform_driver(st_rng_driver);
> +
> +MODULE_AUTHOR("Pankaj Dev <pankaj.dev@st.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>

Re: [PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Daniel Thompson]

Hi Lee

Late but...

On 17/09/15 14:45, Lee Jones wrote:
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 055bb01..8bcfb45 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -30,4 +30,5 @@ 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_ST) += st-rng.o
>   obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> new file mode 100644
> index 0000000..8c8a435
> --- /dev/null
> +++ b/drivers/char/hw_random/st-rng.c
> @@ -0,0 +1,144 @@
> +/*
> + * ST Random Number Generator Driver ST's Platforms
> + *
> + * Author: Pankaj Dev: <pankaj.dev@st.com>
> + *         Lee Jones <lee.jones@linaro.org>
> + *
> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Registers */
> +#define ST_RNG_STATUS_REG		0x20
> +#define ST_RNG_DATA_REG			0x24
> +
> +/* Registers fields */
> +#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> +#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> +#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> +
> +#define ST_RNG_FIFO_SIZE		8
> +#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> +
> +/* Samples are available every 0.667us, which we round to 1us */
> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> +
> +struct st_rng_data {
> +	void __iomem	*base;
> +	struct clk	*clk;
> +	struct hwrng	ops;
> +};
> +
> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> +	u32 status;
> +	int i;
> +
> +	if (max < sizeof(u16))
> +		return -EINVAL;
> +
> +	/* Wait until FIFO is full - max 4uS*/
> +	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> +		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> +		if (status & ST_RNG_STATUS_FIFO_FULL)
> +			break;
> +		udelay(1);

How much bandwidth does using udelay() cost? I think it could be >10% 
compared to a tighter polling loop.


> +	}
> +
> +	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> +		return 0;

Isn't a timeout an error condition?


> +
> +	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> +		*(u16 *)(data + i) =
> +			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> +
> +	return i;	/* No of bytes read */
> +}
> +
> +static int st_rng_probe(struct platform_device *pdev)
> +{
> +	struct st_rng_data *ddata;
> +	struct resource *res;
> +	struct clk *clk;
> +	void __iomem *base;
> +	int ret;
> +
> +	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	ddata->ops.priv	= (unsigned long)ddata;
> +	ddata->ops.read	= st_rng_read;
> +	ddata->ops.name	= pdev->name;
> +	ddata->base	= base;
> +	ddata->clk	= clk;
> +
> +	dev_set_drvdata(&pdev->dev, ddata);
> +
> +	ret = hwrng_register(&ddata->ops);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register HW RNG\n");

Why shout about this particular error but not any others? Perhaps just 
rely on the driver core to report the error here?


> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> +
> +	return 0;
> +}
> +
> +static int st_rng_remove(struct platform_device *pdev)
> +{
> +	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> +
> +	hwrng_unregister(&ddata->ops);
> +
> +	clk_disable_unprepare(ddata->clk);

This mismatches the error paths in the probe function (there is no 
cleanup of clock counts in probe function).


Daniel.

Re: [PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Lee Jones]

On Mon, 05 Oct 2015, Daniel Thompson wrote:
> Late but...

That's okay.  Fixup patches can always be submitted.

We have forever. :)

> On 17/09/15 14:45, Lee Jones wrote:
> >diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> >index 055bb01..8bcfb45 100644
> >--- a/drivers/char/hw_random/Makefile
> >+++ b/drivers/char/hw_random/Makefile
> >@@ -30,4 +30,5 @@ 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_ST) += st-rng.o
> >  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> >diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> >new file mode 100644
> >index 0000000..8c8a435
> >--- /dev/null
> >+++ b/drivers/char/hw_random/st-rng.c
> >@@ -0,0 +1,144 @@
> >+/*
> >+ * ST Random Number Generator Driver ST's Platforms
> >+ *
> >+ * Author: Pankaj Dev: <pankaj.dev@st.com>
> >+ *         Lee Jones <lee.jones@linaro.org>
> >+ *
> >+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License version 2 as
> >+ * published by the Free Software Foundation.
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/delay.h>
> >+#include <linux/hw_random.h>
> >+#include <linux/io.h>
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+
> >+/* Registers */
> >+#define ST_RNG_STATUS_REG		0x20
> >+#define ST_RNG_DATA_REG			0x24
> >+
> >+/* Registers fields */
> >+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> >+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> >+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> >+
> >+#define ST_RNG_FIFO_SIZE		8
> >+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> >+
> >+/* Samples are available every 0.667us, which we round to 1us */
> >+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> >+
> >+struct st_rng_data {
> >+	void __iomem	*base;
> >+	struct clk	*clk;
> >+	struct hwrng	ops;
> >+};
> >+
> >+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >+{
> >+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> >+	u32 status;
> >+	int i;
> >+
> >+	if (max < sizeof(u16))
> >+		return -EINVAL;
> >+
> >+	/* Wait until FIFO is full - max 4uS*/
> >+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> >+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> >+		if (status & ST_RNG_STATUS_FIFO_FULL)
> >+			break;
> >+		udelay(1);
> 
> How much bandwidth does using udelay() cost? I think it could be
> >10% compared to a tighter polling loop.

Samples are only available every 0.7uS and we only do this for every
4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
over that tiny amount of time?  It's an understandable point if we
were talking about milliseconds, but a single microsecond?

> >+	}
> >+
> >+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> >+		return 0;
> 
> Isn't a timeout an error condition?

Yes, which is why we're returning 0.  In this context 0 == 'no data'.
This will be converted to -EAGAIN if the caller did not request
NONBLOCKING.

> >+
> >+	for (i = 0; i < ST_RNG_FIFO_SIZE && i < max; i += 2)
> >+		*(u16 *)(data + i) =
> >+			readl_relaxed(ddata->base + ST_RNG_DATA_REG);
> >+
> >+	return i;	/* No of bytes read */
> >+}
> >+
> >+static int st_rng_probe(struct platform_device *pdev)
> >+{
> >+	struct st_rng_data *ddata;
> >+	struct resource *res;
> >+	struct clk *clk;
> >+	void __iomem *base;
> >+	int ret;
> >+
> >+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
> >+	if (!ddata)
> >+		return -ENOMEM;
> >+
> >+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+	base = devm_ioremap_resource(&pdev->dev, res);
> >+	if (IS_ERR(base))
> >+		return PTR_ERR(base);
> >+
> >+	clk = devm_clk_get(&pdev->dev, NULL);
> >+	if (IS_ERR(clk))
> >+		return PTR_ERR(clk);
> >+
> >+	ret = clk_prepare_enable(clk);
> >+	if (ret)
> >+		return ret;
> >+
> >+	ddata->ops.priv	= (unsigned long)ddata;
> >+	ddata->ops.read	= st_rng_read;
> >+	ddata->ops.name	= pdev->name;
> >+	ddata->base	= base;
> >+	ddata->clk	= clk;
> >+
> >+	dev_set_drvdata(&pdev->dev, ddata);
> >+
> >+	ret = hwrng_register(&ddata->ops);
> >+	if (ret) {
> >+		dev_err(&pdev->dev, "Failed to register HW RNG\n");
> 
> Why shout about this particular error but not any others? Perhaps
> just rely on the driver core to report the error here?

I have omitted error prints from subsystem calls which do all the
shouting required.  Unfortunately the HWRNG is somewhat stuck in the
past in a number of ways; a lack of subsystem level shouting being one
of them.

> >+		return ret;
> >+	}
> >+
> >+	dev_info(&pdev->dev, "Successfully registered HW RNG\n");
> >+
> >+	return 0;
> >+}
> >+
> >+static int st_rng_remove(struct platform_device *pdev)
> >+{
> >+	struct st_rng_data *ddata = dev_get_drvdata(&pdev->dev);
> >+
> >+	hwrng_unregister(&ddata->ops);
> >+
> >+	clk_disable_unprepare(ddata->clk);
> 
> This mismatches the error paths in the probe function (there is no
> cleanup of clock counts in probe function).

Good catch.  I am missing a clk_disable_unprepare() in the
hwrng_register() failure patch.  Will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Daniel Thompson]

On 05/10/15 13:11, Lee Jones wrote:
> On Mon, 05 Oct 2015, Daniel Thompson wrote:
>> Late but...
>
> That's okay.  Fixup patches can always be submitted.
>
> We have forever. :)
>
>> On 17/09/15 14:45, Lee Jones wrote:
>>> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
>>> index 055bb01..8bcfb45 100644
>>> --- a/drivers/char/hw_random/Makefile
>>> +++ b/drivers/char/hw_random/Makefile
>>> @@ -30,4 +30,5 @@ 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_ST) += st-rng.o
>>>   obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
>>> diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
>>> new file mode 100644
>>> index 0000000..8c8a435
>>> --- /dev/null
>>> +++ b/drivers/char/hw_random/st-rng.c
>>> @@ -0,0 +1,144 @@
>>> +/*
>>> + * ST Random Number Generator Driver ST's Platforms
>>> + *
>>> + * Author: Pankaj Dev: <pankaj.dev@st.com>
>>> + *         Lee Jones <lee.jones@linaro.org>
>>> + *
>>> + * Copyright (C) 2015 STMicroelectronics (R&D) Limited
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/hw_random.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +
>>> +/* Registers */
>>> +#define ST_RNG_STATUS_REG		0x20
>>> +#define ST_RNG_DATA_REG			0x24
>>> +
>>> +/* Registers fields */
>>> +#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
>>> +#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
>>> +#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
>>> +
>>> +#define ST_RNG_FIFO_SIZE		8
>>> +#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
>>> +
>>> +/* Samples are available every 0.667us, which we round to 1us */
>>> +#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
>>> +
>>> +struct st_rng_data {
>>> +	void __iomem	*base;
>>> +	struct clk	*clk;
>>> +	struct hwrng	ops;
>>> +};
>>> +
>>> +static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>>> +{
>>> +	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
>>> +	u32 status;
>>> +	int i;
>>> +
>>> +	if (max < sizeof(u16))
>>> +		return -EINVAL;
>>> +
>>> +	/* Wait until FIFO is full - max 4uS*/
>>> +	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
>>> +		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
>>> +		if (status & ST_RNG_STATUS_FIFO_FULL)
>>> +			break;
>>> +		udelay(1);
>>
>> How much bandwidth does using udelay() cost? I think it could be
>>> 10% compared to a tighter polling loop.
>
> Samples are only available every 0.7uS and we only do this for every
> 4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
> over that tiny amount of time?  It's an understandable point if we
> were talking about milliseconds, but a single microsecond?

This code is called in a tight loop so we're not talking about a 
*single* microsecond! We are are talking about about one microsecond in 
every five[1] and yes, I think we should care about that.

It takes 2.67uS for the FIFO to come ready so with a polling interval of 
1uS + loop overhead so I would fully expect on average to "waste" 0.5uS 
each time st_rng_read() is called (in a tight loop).


[1]... point three recurring  ;-)


>>> +	}
>>> +
>>> +	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
>>> +		return 0;
>>
>> Isn't a timeout an error condition?
>
> Yes, which is why we're returning 0.  In this context 0 == 'no data'.
> This will be converted to -EAGAIN if the caller did not request
> NONBLOCKING.

I took the view that hitting the timeout means the hardware is broken. 
Is it likely that the next time we call it there will be data ready? If 
it is should it be trusted?


Daniel.

Re: [PATCH v2 5/7] hwrng: st: Add support for ST's HW Random Number Generator

[Lee Jones]

On Mon, 05 Oct 2015, Daniel Thompson wrote:
> On 05/10/15 13:11, Lee Jones wrote:
> >On Mon, 05 Oct 2015, Daniel Thompson wrote:
> >>Late but...
> >
> >That's okay.  Fixup patches can always be submitted.
> >
> >We have forever. :)
> >
> >>On 17/09/15 14:45, Lee Jones wrote:
> >>>diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> >>>index 055bb01..8bcfb45 100644
> >>>--- a/drivers/char/hw_random/Makefile
> >>>+++ b/drivers/char/hw_random/Makefile
> >>>@@ -30,4 +30,5 @@ 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_ST) += st-rng.o
> >>>  obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o
> >>>diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
> >>>new file mode 100644
> >>>index 0000000..8c8a435
> >>>--- /dev/null
> >>>+++ b/drivers/char/hw_random/st-rng.c
> >>>@@ -0,0 +1,144 @@
> >>>+/*
> >>>+ * ST Random Number Generator Driver ST's Platforms
> >>>+ *
> >>>+ * Author: Pankaj Dev: <pankaj.dev@st.com>
> >>>+ *         Lee Jones <lee.jones@linaro.org>
> >>>+ *
> >>>+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
> >>>+ *
> >>>+ * This program is free software; you can redistribute it and/or modify
> >>>+ * it under the terms of the GNU General Public License version 2 as
> >>>+ * published by the Free Software Foundation.
> >>>+ */
> >>>+
> >>>+#include <linux/clk.h>
> >>>+#include <linux/delay.h>
> >>>+#include <linux/hw_random.h>
> >>>+#include <linux/io.h>
> >>>+#include <linux/module.h>
> >>>+#include <linux/of.h>
> >>>+#include <linux/platform_device.h>
> >>>+#include <linux/slab.h>
> >>>+
> >>>+/* Registers */
> >>>+#define ST_RNG_STATUS_REG		0x20
> >>>+#define ST_RNG_DATA_REG			0x24
> >>>+
> >>>+/* Registers fields */
> >>>+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
> >>>+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
> >>>+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
> >>>+
> >>>+#define ST_RNG_FIFO_SIZE		8
> >>>+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
> >>>+
> >>>+/* Samples are available every 0.667us, which we round to 1us */
> >>>+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
> >>>+
> >>>+struct st_rng_data {
> >>>+	void __iomem	*base;
> >>>+	struct clk	*clk;
> >>>+	struct hwrng	ops;
> >>>+};
> >>>+
> >>>+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >>>+{
> >>>+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
> >>>+	u32 status;
> >>>+	int i;
> >>>+
> >>>+	if (max < sizeof(u16))
> >>>+		return -EINVAL;
> >>>+
> >>>+	/* Wait until FIFO is full - max 4uS*/
> >>>+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
> >>>+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
> >>>+		if (status & ST_RNG_STATUS_FIFO_FULL)
> >>>+			break;
> >>>+		udelay(1);
> >>
> >>How much bandwidth does using udelay() cost? I think it could be
> >>>10% compared to a tighter polling loop.
> >
> >Samples are only available every 0.7uS and we only do this for every
> >4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
> >over that tiny amount of time?  It's an understandable point if we
> >were talking about milliseconds, but a single microsecond?
> 
> This code is called in a tight loop so we're not talking about a
> *single* microsecond! We are are talking about about one microsecond
> in every five[1] and yes, I think we should care about that.
> 
> It takes 2.67uS for the FIFO to come ready so with a polling
> interval of 1uS + loop overhead so I would fully expect on average
> to "waste" 0.5uS each time st_rng_read() is called (in a tight
> loop).
> 
> [1]... point three recurring  ;-)

`time dd if=/dev/hwrng of=/dev/null bs=1 count=10M`

/* Current code, inc delay */
Run 1: real 0m17.996s
Run 2: real 0m17.991s
Run 3: real 0m17.996s
Run 4: real 0m18.000s
Run 5: real 0m18.000s
Total       0m89.983s

/* Tight loop, no delay */
Run 1: real 0m18.011s
Run 2: real 0m17.995s
Run 3: real 0m18.005s
Run 4: real 0m18.020s
Run 5: real 0m17.990s
Total       0m90.021s

(89.983 / 90.021) * 100 = 99.958%

0.042% saving.

Not quite the >10% predicted.  I'd say that's negligible. 

> >>>+	}
> >>>+
> >>>+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
> >>>+		return 0;
> >>
> >>Isn't a timeout an error condition?
> >
> >Yes, which is why we're returning 0.  In this context 0 == 'no data'.
> >This will be converted to -EAGAIN if the caller did not request
> >NONBLOCKING.
> 
> I took the view that hitting the timeout means the hardware is
> broken. Is it likely that the next time we call it there will be
> data ready? If it is should it be trusted?

>From experience gained whilst testing, I can say that when returning
an error the HNG breaks and the user receives an error.  If instead we
return 0, we get to have another go and random numbers again pour
out.  Perhaps we're just not waiting long enough?  Either way, the
current implementation works real well.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 6/7] ARM: STi: STiH407: Enable the 2 HW Random Number Generators for STiH4{07,10}

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 838b812..9452b42 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -565,5 +565,19 @@
 						  <&phy_port2 PHY_TYPE_USB3>;
 			};
 		};
+
+		rng10: rng@08a89000 {
+			compatible      = "st,rng";
+			reg		= <0x08a89000 0x1000>;
+			clocks          = <&clk_sysin>;
+			status		= "okay";
+		};
+
+		rng11: rng@08a8a000 {
+			compatible      = "st,rng";
+			reg		= <0x08a8a000 0x1000>;
+			clocks          = <&clk_sysin>;
+			status		= "okay";
+		};
 	};
 };
-- 
1.9.1

Re: [STLinux Kernel] [PATCH v2 6/7] ARM: STi: STiH407: Enable the 2 HW Random Number Generators for STiH4{07, 10}

[Maxime Coquelin]

Hi Lee, Herbert,

On 09/17/2015 03:45 PM, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   arch/arm/boot/dts/stih407-family.dtsi | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index 838b812..9452b42 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -565,5 +565,19 @@
>   						  <&phy_port2 PHY_TYPE_USB3>;
>   			};
>   		};
> +
> +		rng10: rng@08a89000 {
> +			compatible      = "st,rng";
> +			reg		= <0x08a89000 0x1000>;
> +			clocks          = <&clk_sysin>;
> +			status		= "okay";
> +		};
> +
> +		rng11: rng@08a8a000 {
> +			compatible      = "st,rng";
> +			reg		= <0x08a8a000 0x1000>;
> +			clocks          = <&clk_sysin>;
> +			status		= "okay";
> +		};
>   	};
>   };

Patch applied to STi tree (sti-dt-for-v4.4).

Thanks,
Maxime

[PATCH v2 7/7] MAINTAINERS: Add ST's Random Number Generator to the ST entry

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fd60784..b084d69 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1502,6 +1502,7 @@ W:	http://www.stlinux.com
 S:	Maintained
 F:	arch/arm/mach-sti/
 F:	arch/arm/boot/dts/sti*
+F:	drivers/char/hw_random/st-rng.c
 F:	drivers/clocksource/arm_global_timer.c
 F:	drivers/i2c/busses/i2c-st.c
 F:	drivers/media/rc/st_rc.c
-- 
1.9.1

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Thu, Sep 17, 2015 at 02:45:50PM +0100, Lee Jones wrote:
> v1 => v2:
>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
>  - Fix 2099 => 2009 typo in commit log
>  - Fix 'number of random numbers sourced' return value
>  - Treat devm_clk_get()'s return value correctly
>  - Check return value of clk_prepare_enable()
>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On 18 September 2015 at 15:07, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Sep 17, 2015 at 02:45:50PM +0100, Lee Jones wrote:
>> v1 => v2:
>>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
>>  - Fix 2099 => 2009 typo in commit log
>>  - Fix 'number of random numbers sourced' return value
>>  - Treat devm_clk_get()'s return value correctly
>>  - Check return value of clk_prepare_enable()
>>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
>
> All applied.  Thanks.

Just to be clear.  Which patches have you applied?

> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-- 
Lee Jones
Linaro ST Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Fri, Sep 18, 2015 at 03:53:50PM +0100, Lee Jones wrote:
> On 18 September 2015 at 15:07, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Thu, Sep 17, 2015 at 02:45:50PM +0100, Lee Jones wrote:
> >> v1 => v2:
> >>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
> >>  - Fix 2099 => 2009 typo in commit log
> >>  - Fix 'number of random numbers sourced' return value
> >>  - Treat devm_clk_get()'s return value correctly
> >>  - Check return value of clk_prepare_enable()
> >>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
> >
> > All applied.  Thanks.
> 
> Just to be clear.  Which patches have you applied?

All of your patches.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Fri, 18 Sep 2015, Herbert Xu wrote:
> On Fri, Sep 18, 2015 at 03:53:50PM +0100, Lee Jones wrote:
> > On 18 September 2015 at 15:07, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > On Thu, Sep 17, 2015 at 02:45:50PM +0100, Lee Jones wrote:
> > >> v1 => v2:
> > >>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
> > >>  - Fix 2099 => 2009 typo in commit log
> > >>  - Fix 'number of random numbers sourced' return value
> > >>  - Treat devm_clk_get()'s return value correctly
> > >>  - Check return value of clk_prepare_enable()
> > >>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
> > >
> > > All applied.  Thanks.
> > 
> > Just to be clear.  Which patches have you applied?
> 
> All of your patches.

I think it's okay for you to take all but patch 6.

Patch 6 is an ARM patch and needs to go into ARM SoC via
STMicroelectronics STi tree.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Fri, Sep 18, 2015 at 04:51:12PM +0100, Lee Jones wrote:
>
> I think it's okay for you to take all but patch 6.
> 
> Patch 6 is an ARM patch and needs to go into ARM SoC via
> STMicroelectronics STi tree.

In future please don't send me patches that you don't want me to
merge in the series.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Sat, 19 Sep 2015, Herbert Xu wrote:
> On Fri, Sep 18, 2015 at 04:51:12PM +0100, Lee Jones wrote:
> >
> > I think it's okay for you to take all but patch 6.
> > 
> > Patch 6 is an ARM patch and needs to go into ARM SoC via
> > STMicroelectronics STi tree.
> 
> In future please don't send me patches that you don't want me to
> merge in the series.

That's not how it works.  It's helpful, more often than not, to submit
the entire set to each maintainer concerned so they can keep up with
the general conversation.  By only sending specific patches to
maintainers you essentially blinker them to the bigger picture.

As a maintainer you should _know_ that you can't apply patches from
other subsystems without appropriate Acks.  I'm sure you'd take
exception to another maintainer who started accepting patches for
subsystems you are responsible for.  This works both ways.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Sat, Sep 19, 2015 at 10:21:45AM +0100, Lee Jones wrote:
>
> That's not how it works.  It's helpful, more often than not, to submit
> the entire set to each maintainer concerned so they can keep up with
> the general conversation.  By only sending specific patches to
> maintainers you essentially blinker them to the bigger picture.
> 
> As a maintainer you should _know_ that you can't apply patches from
> other subsystems without appropriate Acks.  I'm sure you'd take
> exception to another maintainer who started accepting patches for
> subsystems you are responsible for.  This works both ways.

No you are mistaken.  You should only put patches which have
dependencies on each other in a series.  If the patches can be
applied independently of each other there is no need to have
them in a single series.

Obviously if they can go into different trees then they cannot
have dependencies.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Sun, 20 Sep 2015, Herbert Xu wrote:

> On Sat, Sep 19, 2015 at 10:21:45AM +0100, Lee Jones wrote:
> >
> > That's not how it works.  It's helpful, more often than not, to submit
> > the entire set to each maintainer concerned so they can keep up with
> > the general conversation.  By only sending specific patches to
> > maintainers you essentially blinker them to the bigger picture.
> > 
> > As a maintainer you should _know_ that you can't apply patches from
> > other subsystems without appropriate Acks.  I'm sure you'd take
> > exception to another maintainer who started accepting patches for
> > subsystems you are responsible for.  This works both ways.
> 
> No you are mistaken.  You should only put patches which have
> dependencies on each other in a series.  If the patches can be
> applied independently of each other there is no need to have
> them in a single series.

That's just not true.  I've explained why it's important for everyone
involved to see the bigger picture.  Let me use this set in an
example.  The patches can (and should) be applied separately, but they
are heavily entwined.  Let's say I only sent the ARM patch to Maxime
(the STi Maintainer) and only sent you the driver and the binding
document.  There's a chance Maxime could apply the DTS changes prior
to a proper review of the bindings.  Granted, one way round this would
be to place the DTS changes into a holding-pen until the binding has
been accepted, but this method is highly impractical and puts
unnecessary burden on the contributor.

There are 1000's of examples where all parties need to see reviews on
other, related but not dependant, parts of a set.  For many of the
sets I review it's critical for me what else is going on in related
diffs.  I guess for the subsystems you maintain it's less of an issue,
but still, it _is_ how people tend to submit code and there is no good
reason for you to dictate otherwise.

> Obviously if they can go into different trees then they cannot
> have dependencies.
> 
> Cheers,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Fri, 18 Sep 2015, Lee Jones wrote:
> On Fri, 18 Sep 2015, Herbert Xu wrote:
> > On Fri, Sep 18, 2015 at 03:53:50PM +0100, Lee Jones wrote:
> > > On 18 September 2015 at 15:07, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > > > On Thu, Sep 17, 2015 at 02:45:50PM +0100, Lee Jones wrote:
> > > >> v1 => v2:
> > > >>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
> > > >>  - Fix 2099 => 2009 typo in commit log
> > > >>  - Fix 'number of random numbers sourced' return value
> > > >>  - Treat devm_clk_get()'s return value correctly
> > > >>  - Check return value of clk_prepare_enable()
> > > >>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
> > > >
> > > > All applied.  Thanks.
> > > 
> > > Just to be clear.  Which patches have you applied?
> > 
> > All of your patches.
> 
> I think it's okay for you to take all but patch 6.
> 
> Patch 6 is an ARM patch and needs to go into ARM SoC via
> STMicroelectronics STi tree.

Hi Herbert,

I see that your tree is 8 days old, so this may have been resolved
already, but would you be kind enough to ensure you remove the 6th
(ARM) patch from your repo please?  I wouldn't want it to cause
conflicts and for Maxime and yourself to get shouted at by Linus.

Much appreciated.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Tue, Sep 29, 2015 at 03:29:32PM +0100, Lee Jones wrote:
> 
> I see that your tree is 8 days old, so this may have been resolved
> already, but would you be kind enough to ensure you remove the 6th
> (ARM) patch from your repo please?  I wouldn't want it to cause
> conflicts and for Maxime and yourself to get shouted at by Linus.

I prefer not to merge patches that cannot be tested.  Without
the DT bits in patch 6 the other five patches are useless.  So
I think patch 6 should be applied together with the other five
which add the driver.

Of course if Linus wants me to revert patch 6 in case of any
potential conflicts with Maxime's tree I'll do that.  Linus?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Wed, 30 Sep 2015, Herbert Xu wrote:
> On Tue, Sep 29, 2015 at 03:29:32PM +0100, Lee Jones wrote:
> > 
> > I see that your tree is 8 days old, so this may have been resolved
> > already, but would you be kind enough to ensure you remove the 6th
> > (ARM) patch from your repo please?  I wouldn't want it to cause
> > conflicts and for Maxime and yourself to get shouted at by Linus.
> 
> I prefer not to merge patches that cannot be tested.  Without
> the DT bits in patch 6 the other five patches are useless.  So
> I think patch 6 should be applied together with the other five
> which add the driver.

That's crazy talk.  If all subsystem maintainers abide by this rule
there would be chaos.  We'd either need to send pull-requests to each
other for every set which crossed a subsystems boundary, or 1000's of
merge conflicts would ensue at merge time.

The (sensible) rule we normally stick to is; as long as there isn't
a _build_ dependency, then the patches should filter though their
respective trees; _functional_ dependencies have nothing to do with
us as maintainers.  Another chaos preventing rule we abide by is; thou
shalt not apply patches belonging to other maintainer's subsystems
without the appropriate Ack-by and a subsequent "you may take this
though your tree" and/or "please send me an immutable pull-request".

> Of course if Linus wants me to revert patch 6 in case of any
> potential conflicts with Maxime's tree I'll do that.  Linus?

Why bother Linus?  The whole purpose of this is to _not_ pi$$ him
off.  This stuff is common sense.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Herbert Xu]

On Wed, Sep 30, 2015 at 03:15:39PM +0100, Lee Jones wrote:
>
> > I prefer not to merge patches that cannot be tested.  Without
> > the DT bits in patch 6 the other five patches are useless.  So
> > I think patch 6 should be applied together with the other five
> > which add the driver.
> 
> That's crazy talk.  If all subsystem maintainers abide by this rule
> there would be chaos.  We'd either need to send pull-requests to each
> other for every set which crossed a subsystems boundary, or 1000's of
> merge conflicts would ensue at merge time.
> 
> The (sensible) rule we normally stick to is; as long as there isn't
> a _build_ dependency, then the patches should filter though their
> respective trees; _functional_ dependencies have nothing to do with
> us as maintainers.  Another chaos preventing rule we abide by is; thou
> shalt not apply patches belonging to other maintainer's subsystems
> without the appropriate Ack-by and a subsequent "you may take this
> though your tree" and/or "please send me an immutable pull-request".

So you want the series to be merged in two parts via two different
trees where neither can be tested? That sounds crazy to me.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [STLinux Kernel] [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Maxime Coquelin]

On 09/30/2015 04:28 PM, Herbert Xu wrote:
> On Wed, Sep 30, 2015 at 03:15:39PM +0100, Lee Jones wrote:
>>> I prefer not to merge patches that cannot be tested.  Without
>>> the DT bits in patch 6 the other five patches are useless.  So
>>> I think patch 6 should be applied together with the other five
>>> which add the driver.
>> That's crazy talk.  If all subsystem maintainers abide by this rule
>> there would be chaos.  We'd either need to send pull-requests to each
>> other for every set which crossed a subsystems boundary, or 1000's of
>> merge conflicts would ensue at merge time.
>>
>> The (sensible) rule we normally stick to is; as long as there isn't
>> a _build_ dependency, then the patches should filter though their
>> respective trees; _functional_ dependencies have nothing to do with
>> us as maintainers.  Another chaos preventing rule we abide by is; thou
>> shalt not apply patches belonging to other maintainer's subsystems
>> without the appropriate Ack-by and a subsequent "you may take this
>> though your tree" and/or "please send me an immutable pull-request".
> So you want the series to be merged in two parts via two different
> trees where neither can be tested? That sounds crazy to me.
>

Yes, that's what we want, and that's how people work usually.
I will repeat what Lee was saying, what we have to ensure as maintainer 
is that our tree is building.
We will be able to test it with linux-next.

Regards,
Maxime

Re: [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Wed, 30 Sep 2015, Herbert Xu wrote:
> On Wed, Sep 30, 2015 at 03:15:39PM +0100, Lee Jones wrote:
> >
> > > I prefer not to merge patches that cannot be tested.  Without
> > > the DT bits in patch 6 the other five patches are useless.  So
> > > I think patch 6 should be applied together with the other five
> > > which add the driver.
> > 
> > That's crazy talk.  If all subsystem maintainers abide by this rule
> > there would be chaos.  We'd either need to send pull-requests to each
> > other for every set which crossed a subsystems boundary, or 1000's of
> > merge conflicts would ensue at merge time.
> > 
> > The (sensible) rule we normally stick to is; as long as there isn't
> > a _build_ dependency, then the patches should filter though their
> > respective trees; _functional_ dependencies have nothing to do with
> > us as maintainers.  Another chaos preventing rule we abide by is; thou
> > shalt not apply patches belonging to other maintainer's subsystems
> > without the appropriate Ack-by and a subsequent "you may take this
> > though your tree" and/or "please send me an immutable pull-request".
> 
> So you want the series to be merged in two parts via two different
> trees where neither can be tested? That sounds crazy to me.

Who is going to checkout the HWRNG tree and run-test it on it's own on
all of the required hardware?  No one.  Agreed, subsystem trees should
be bisectably (new word? :D) buildable as per my first rule above, but
that's it.  Per-subsystem repos are not designed to be tested for
full-functionality orthogonally, that's the point of Stephen's -next
tree.

Please take my other points into consideration too.  The kernel would
be unmainatinable if we all stuck to your rule.  No-one else has that
rule, and for good reason.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Peter Griffin]

Hi Lee,

On Thu, 17 Sep 2015, Lee Jones wrote:

> v1 => v2:
>  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
>  - Fix 2099 => 2009 typo in commit log
>  - Fix 'number of random numbers sourced' return value
>  - Treat devm_clk_get()'s return value correctly
>  - Check return value of clk_prepare_enable()
>  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
> 
> The main aim of this set is to allow users to access ST's hardware
> random number generator.  It's a simple device, which only requires
> a simple driver.
> 
> We're also taking the liberty to update some out of date HWRNG
> documentation and making the sysfs interface a little easier to
> use by ignoring any '\n' which may have been inadvertently passed.

Looks good, for the series: -

Acked-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter.

Re: [STLinux Kernel] [PATCH v2 0/7] hwrng: Add support for STMicroelectronics' RNG IP

[Lee Jones]

On Tue, 29 Sep 2015, Peter Griffin wrote:
> On Thu, 17 Sep 2015, Lee Jones wrote:
> 
> > v1 => v2:
> >  - New patch: Also fixing /dev/hw_random => /dev/hwrng in Kconfig
> >  - Fix 2099 => 2009 typo in commit log
> >  - Fix 'number of random numbers sourced' return value
> >  - Treat devm_clk_get()'s return value correctly
> >  - Check return value of clk_prepare_enable()
> >  - Use sysfs_streq() instead of manually stripping '\n' from sysfs
> > 
> > The main aim of this set is to allow users to access ST's hardware
> > random number generator.  It's a simple device, which only requires
> > a simple driver.
> > 
> > We're also taking the liberty to update some out of date HWRNG
> > documentation and making the sysfs interface a little easier to
> > use by ignoring any '\n' which may have been inadvertently passed.
> 
> Looks good, for the series: -
> 
> Acked-by: Peter Griffin <peter.griffin@linaro.org>

Thanks, but Herbert already applied the series.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog