[PATCH 0/2] hwrng: bcm2835: add reset support

[PATCH 0/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

Álvaro Fernández Rojas (2):
  dt-bindings: rng: bcm2835: document reset support
  hwrng: bcm2835: add reset support

 .../devicetree/bindings/rng/brcm,bcm2835.yaml          |  5 +++++
 drivers/char/hw_random/bcm2835-rng.c                   | 10 ++++++++++
 2 files changed, 15 insertions(+)

-- 
2.20.1

[PATCH 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v2: document reset support.

 Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
index c147900f9041..dba70764b7d0 100644
--- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
+++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
@@ -31,6 +31,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -58,4 +61,6 @@ examples:
 
         clocks = <&periph_clk 18>;
         clock-names = "ipsec";
+
+        resets = <&periph_rst 4>;
     };
-- 
2.20.1

Re: [PATCH 1/2] dt-bindings: rng: bcm2835: document reset support

[Florian Fainelli]

On 2/23/2021 8:01 AM, Álvaro Fernández Rojas wrote:
> Some devices may need to perform a reset before using the RNG, such as the
> BCM6368.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Since the reset is unique to the 6368, you may want to make the property
mandatory for the 6368 compatible string and optional otherwise.
-- 
Florian

Re: [PATCH 1/2] dt-bindings: rng: bcm2835: document reset support

[Scott Branden]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On 2021-02-23 8:36 a.m., Florian Fainelli wrote:
> 
> 
> On 2/23/2021 8:01 AM, Álvaro Fernández Rojas wrote:
>> Some devices may need to perform a reset before using the RNG, such as the
>> BCM6368.
>>
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> 
> Since the reset is unique to the 6368, you may want to make the property
> mandatory for the 6368 compatible string and optional otherwise.
> 
Perhaps the reset could be done at an earlier boot stage as well and then the
reset would even be optional on 6368?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Hello Scott,

El 23/02/2021 a las 18:17, Scott Branden escribió:
> On 2021-02-23 8:36 a.m., Florian Fainelli wrote:
>>
>>
>> On 2/23/2021 8:01 AM, Álvaro Fernández Rojas wrote:
>>> Some devices may need to perform a reset before using the RNG, such as the
>>> BCM6368.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>
>> Since the reset is unique to the 6368, you may want to make the property
>> mandatory for the 6368 compatible string and optional otherwise.
>>
> Perhaps the reset could be done at an earlier boot stage as well and then the
> reset would even be optional on 6368?
> 

No, this isn't possible on bmips, which is device tree only.
However, it's how is done in bcm63xx, which is why it wasn't needed before.

Best regards,
Álvaro.

[PATCH 2/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

BCM6368 devices need to reset the in order to generate true random numbers.
This is what BCM6368 produces without a reset:
root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
rngtest 6.10
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: 20000032
rngtest: FIPS 140-2 successes: 0
rngtest: FIPS 140-2 failures: 1000
rngtest: FIPS 140-2(2001-10-10) Monobit: 2
rngtest: FIPS 140-2(2001-10-10) Poker: 1000
rngtest: FIPS 140-2(2001-10-10) Runs: 1000
rngtest: FIPS 140-2(2001-10-10) Long run: 30
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
rngtest: Program run time: 1336176 microseconds

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v2: no changes.

 drivers/char/hw_random/bcm2835-rng.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c
index 1a7c43b43c6b..1b93a896d8e8 100644
--- a/drivers/char/hw_random/bcm2835-rng.c
+++ b/drivers/char/hw_random/bcm2835-rng.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #define RNG_CTRL	0x0
 #define RNG_STATUS	0x4
@@ -32,6 +33,7 @@ struct bcm2835_rng_priv {
 	void __iomem *base;
 	bool mask_interrupts;
 	struct clk *clk;
+	struct reset_control *reset;
 };
 
 static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
@@ -94,6 +96,10 @@ static int bcm2835_rng_init(struct hwrng *rng)
 			return ret;
 	}
 
+	ret = reset_control_reset(priv->reset);
+	if (ret)
+		return ret;
+
 	if (priv->mask_interrupts) {
 		/* mask the interrupt */
 		val = rng_readl(priv, RNG_INT_MASK);
@@ -159,6 +165,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
 	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return PTR_ERR(priv->reset);
+
 	priv->rng.name = pdev->name;
 	priv->rng.init = bcm2835_rng_init;
 	priv->rng.read = bcm2835_rng_read;
-- 
2.20.1

[PATCH v4 0/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

v4: fix documentation, add reset_control_rearm().
v3: make resets required if brcm,bcm6368-rng.
v2: document reset support.

Álvaro Fernández Rojas (2):
  dt-bindings: rng: bcm2835: document reset support
  hwrng: bcm2835: add reset support

 .../devicetree/bindings/rng/brcm,bcm2835.yaml      | 14 ++++++++++++++
 drivers/char/hw_random/bcm2835-rng.c               | 12 ++++++++++++
 2 files changed, 26 insertions(+)

-- 
2.20.1

[PATCH v4 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v4: pass dt_binding_check.
 v3: make resets required if brcm,bcm6368-rng.
 v2: document reset support.

 .../devicetree/bindings/rng/brcm,bcm2835.yaml      | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
index c147900f9041..9a74b5af1dbd 100644
--- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
+++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
@@ -31,12 +31,24 @@ properties:
   interrupts:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
 
 additionalProperties: false
 
+if:
+  properties:
+    compatible:
+      enum:
+        - brcm,bcm6368-rng
+then:
+  required:
+    - resets
+
 examples:
   - |
     rng@7e104000 {
@@ -58,4 +70,6 @@ examples:
 
         clocks = <&periph_clk 18>;
         clock-names = "ipsec";
+
+        resets = <&periph_rst 4>;
     };
-- 
2.20.1

[PATCH v4 2/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

BCM6368 devices need to reset the in order to generate true random numbers.
This is what BCM6368 produces without a reset:
root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
rngtest 6.10
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: 20000032
rngtest: FIPS 140-2 successes: 0
rngtest: FIPS 140-2 failures: 1000
rngtest: FIPS 140-2(2001-10-10) Monobit: 2
rngtest: FIPS 140-2(2001-10-10) Poker: 1000
rngtest: FIPS 140-2(2001-10-10) Runs: 1000
rngtest: FIPS 140-2(2001-10-10) Long run: 30
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
rngtest: Program run time: 1336176 microseconds

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v4: add reset_control_rearm().
 v3: no changes.
 v2: no changes.

 drivers/char/hw_random/bcm2835-rng.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c
index 1a7c43b43c6b..92658edaff22 100644
--- a/drivers/char/hw_random/bcm2835-rng.c
+++ b/drivers/char/hw_random/bcm2835-rng.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #define RNG_CTRL	0x0
 #define RNG_STATUS	0x4
@@ -32,6 +33,7 @@ struct bcm2835_rng_priv {
 	void __iomem *base;
 	bool mask_interrupts;
 	struct clk *clk;
+	struct reset_control *reset;
 };
 
 static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
@@ -94,6 +96,10 @@ static int bcm2835_rng_init(struct hwrng *rng)
 			return ret;
 	}
 
+	ret = reset_control_reset(priv->reset);
+	if (ret)
+		return ret;
+
 	if (priv->mask_interrupts) {
 		/* mask the interrupt */
 		val = rng_readl(priv, RNG_INT_MASK);
@@ -115,6 +121,8 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
 	/* disable rng hardware */
 	rng_writel(priv, 0, RNG_CTRL);
 
+	reset_control_rearm(priv->reset);
+
 	if (!IS_ERR(priv->clk))
 		clk_disable_unprepare(priv->clk);
 }
@@ -159,6 +167,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
 	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return PTR_ERR(priv->reset);
+
 	priv->rng.name = pdev->name;
 	priv->rng.init = bcm2835_rng_init;
 	priv->rng.read = bcm2835_rng_read;
-- 
2.20.1

Re: [PATCH v4 2/2] hwrng: bcm2835: add reset support

[Philipp Zabel]

Hi Álvaro,

On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
[...]
> @@ -115,6 +121,8 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
>  	/* disable rng hardware */
>  	rng_writel(priv, 0, RNG_CTRL);
>  
> +	reset_control_rearm(priv->reset);
> +
>  	if (!IS_ERR(priv->clk))
>  		clk_disable_unprepare(priv->clk);
>  }
> @@ -159,6 +167,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
>  	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
>  
> +	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(priv->reset))
> +		return PTR_ERR(priv->reset);
> +
>  	priv->rng.name = pdev->name;
>  	priv->rng.init = bcm2835_rng_init;
>  	priv->rng.read = bcm2835_rng_read;

That doesn't seem right. reset_control_rearm() doesn't do anything if
the reset control is exclusive. Either the reset control should be
requested as shared, or the _rearm should be removed.

regards
Philipp

Re: [PATCH v4 2/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

Hi Philipp,

> El 3 mar 2021, a las 14:52, Philipp Zabel <p.zabel@pengutronix.de> escribió:
> 
> Hi Álvaro,
> 
> On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
> [...]
>> @@ -115,6 +121,8 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
>> 	/* disable rng hardware */
>> 	rng_writel(priv, 0, RNG_CTRL);
>> 
>> +	reset_control_rearm(priv->reset);
>> +
>> 	if (!IS_ERR(priv->clk))
>> 		clk_disable_unprepare(priv->clk);
>> }
>> @@ -159,6 +167,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
>> 	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
>> 		return -EPROBE_DEFER;
>> 
>> +	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
>> +	if (IS_ERR(priv->reset))
>> +		return PTR_ERR(priv->reset);
>> +
>> 	priv->rng.name = pdev->name;
>> 	priv->rng.init = bcm2835_rng_init;
>> 	priv->rng.read = bcm2835_rng_read;
> 
> That doesn't seem right. reset_control_rearm() doesn't do anything if
> the reset control is exclusive. Either the reset control should be
> requested as shared, or the _rearm should be removed.

In only added reset_control_rearm() because Florian requested it…
I think it’s not needed, so we can use v3, since it was the only change between v3 and v4...

> 
> regards
> Philipp

Best regards,
Álvaro.

Re: [PATCH v4 2/2] hwrng: bcm2835: add reset support

[Florian Fainelli]

On 3/3/21 6:06 AM, Álvaro Fernández Rojas wrote:
> Hi Philipp,
> 
>> El 3 mar 2021, a las 14:52, Philipp Zabel <p.zabel@pengutronix.de> escribió:
>>
>> Hi Álvaro,
>>
>> On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
>> [...]
>>> @@ -115,6 +121,8 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
>>> 	/* disable rng hardware */
>>> 	rng_writel(priv, 0, RNG_CTRL);
>>>
>>> +	reset_control_rearm(priv->reset);
>>> +
>>> 	if (!IS_ERR(priv->clk))
>>> 		clk_disable_unprepare(priv->clk);
>>> }
>>> @@ -159,6 +167,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
>>> 	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
>>> 		return -EPROBE_DEFER;
>>>
>>> +	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
>>> +	if (IS_ERR(priv->reset))
>>> +		return PTR_ERR(priv->reset);
>>> +
>>> 	priv->rng.name = pdev->name;
>>> 	priv->rng.init = bcm2835_rng_init;
>>> 	priv->rng.read = bcm2835_rng_read;
>>
>> That doesn't seem right. reset_control_rearm() doesn't do anything if
>> the reset control is exclusive. Either the reset control should be
>> requested as shared, or the _rearm should be removed.
> 
> In only added reset_control_rearm() because Florian requested it…
> I think it’s not needed, so we can use v3, since it was the only change between v3 and v4...

Not the first time I am confused by the reset API not sure if I will
ever get it one day, so apologies for suggesting something incorrect here.
-- 
Florian

[PATCH v3 0/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

v3: make resets required if brcm,bcm6368-rng.
v2: document reset support.

Álvaro Fernández Rojas (2):
  dt-bindings: rng: bcm2835: document reset support
  hwrng: bcm2835: add reset support

 .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
 drivers/char/hw_random/bcm2835-rng.c            | 10 ++++++++++
 2 files changed, 27 insertions(+)

-- 
2.20.1

[PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Some devices may need to perform a reset before using the RNG, such as the
BCM6368.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v3: make resets required if brcm,bcm6368-rng.
 v2: document reset support.

 .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
index c147900f9041..11c23e1f6988 100644
--- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
+++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
@@ -37,6 +37,21 @@ required:
 
 additionalProperties: false
 
+if:
+  properties:
+    compatible:
+      enum:
+        - brcm,bcm6368-rng
+then:
+  properties:
+    resets:
+      maxItems: 1
+  required:
+    - resets
+else:
+  properties:
+    resets: false
+
 examples:
   - |
     rng@7e104000 {
@@ -58,4 +73,6 @@ examples:
 
         clocks = <&periph_clk 18>;
         clock-names = "ipsec";
+
+        resets = <&periph_rst 4>;
     };
-- 
2.20.1

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

On Tue, 23 Feb 2021 18:00:05 +0100, Álvaro Fernández Rojas wrote:
> Some devices may need to perform a reset before using the RNG, such as the
> BCM6368.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  v3: make resets required if brcm,bcm6368-rng.
>  v2: document reset support.
> 
>  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.example.dt.yaml: rng@10004180: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml

See https://patchwork.ozlabs.org/patch/1443582

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Hi Rob,

> El 23 feb 2021, a las 20:34, Rob Herring <robh@kernel.org> escribió:
> 
> On Tue, 23 Feb 2021 18:00:05 +0100, Álvaro Fernández Rojas wrote:
>> Some devices may need to perform a reset before using the RNG, such as the
>> BCM6368.
>> 
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>> v3: make resets required if brcm,bcm6368-rng.
>> v2: document reset support.
>> 
>> .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.example.dt.yaml: rng@10004180: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml

I don’t get what’s wrong here...

> 
> See https://patchwork.ozlabs.org/patch/1443582
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 

Best regards,
Álvaro.

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

Hi Alvaro,

On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
> Some devices may need to perform a reset before using the RNG, such as the
> BCM6368.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  v3: make resets required if brcm,bcm6368-rng.
>  v2: document reset support.
> 
>  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> index c147900f9041..11c23e1f6988 100644
> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> @@ -37,6 +37,21 @@ required:
>  
> 
>  additionalProperties: false

I can't claim I fully understand all the meta stuff in shemas, so I generally
just follow the patterns already available out there. That said, shoudln't this
be at the end, just before the examples? Maybe the cause of that odd warning
you got there?

> +if:
> +  properties:
> +    compatible:
> +      enum:
> +        - brcm,bcm6368-rng
> +then:
> +  properties:
> +    resets:
> +      maxItems: 1
> +  required:
> +    - resets

I belive you can't really make a property required when the bindings for
'brcm,bcm6368-rng' were already defined. This will break the schema for those
otherwise correct devicetrees.

> +else:
> +  properties:
> +    resets: false
> +
>  examples:
>    - |
>      rng@7e104000 {
> @@ -58,4 +73,6 @@ examples:
>  
> 
>          clocks = <&periph_clk 18>;
>          clock-names = "ipsec";
> +
> +        resets = <&periph_rst 4>;
>      };

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

> El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió:
> 
> Hi Alvaro,
> 
> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
>> Some devices may need to perform a reset before using the RNG, such as the
>> BCM6368.
>> 
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>>  v3: make resets required if brcm,bcm6368-rng.
>>  v2: document reset support.
>> 
>>  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>> index c147900f9041..11c23e1f6988 100644
>> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>> @@ -37,6 +37,21 @@ required:
>>  
>> 
>>  additionalProperties: false
> 
> I can't claim I fully understand all the meta stuff in shemas, so I generally
> just follow the patterns already available out there.

Well, that makes two of us :).

> That said, shoudln't this be at the end, just before the examples?

I don’t know but I can move it there ¯\_(ツ)_/¯

> Maybe the cause of that odd warning
> you got there?

Which odd warning?
I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls):
make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml

> 
>> +if:
>> +  properties:
>> +    compatible:
>> +      enum:
>> +        - brcm,bcm6368-rng
>> +then:
>> +  properties:
>> +    resets:
>> +      maxItems: 1
>> +  required:
>> +    - resets
> 
> I belive you can't really make a property required when the bindings for
> 'brcm,bcm6368-rng' were already defined. This will break the schema for those
> otherwise correct devicetrees.

Why not?
Wouldn’t just be required for brcm,bcm6368-rng?

Anyway, I can omit this, since it would be the same for clocks and those aren’t required either.

> 
>> +else:
>> +  properties:
>> +    resets: false
>> +
>>  examples:
>>    - |
>>      rng@7e104000 {
>> @@ -58,4 +73,6 @@ examples:
>>  
>> 
>>          clocks = <&periph_clk 18>;
>>          clock-names = "ipsec";
>> +
>> +        resets = <&periph_rst 4>;
>>      };
> 
> Regards,
> Nicolas

Best regards,
Álvaro.

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2832 bytes --]

On Thu, 2021-03-04 at 13:18 +0100, Álvaro Fernández Rojas wrote:
> 
> > El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió:
> > 
> > Hi Alvaro,
> > 
> > On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
> > > Some devices may need to perform a reset before using the RNG, such as the
> > > BCM6368.
> > > 
> > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > > ---
> > >  v3: make resets required if brcm,bcm6368-rng.
> > >  v2: document reset support.
> > > 
> > >  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > > index c147900f9041..11c23e1f6988 100644
> > > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > > @@ -37,6 +37,21 @@ required:
> > >  
> > > 
> > > 
> > >  additionalProperties: false
> > 
> > I can't claim I fully understand all the meta stuff in shemas, so I generally
> > just follow the patterns already available out there.
> 
> Well, that makes two of us :).
> 
> > That said, shoudln't this be at the end, just before the examples?
> 
> I don’t know but I can move it there ¯\_(ツ)_/¯

Yes please do. See commit 7f464532b05 ("dt-bindings: Add missing
'additionalProperties: false'") which expands on why it is needed, and why it
should be at the end.

> > Maybe the cause of that odd warning
> > you got there?
> 
> Which odd warning?

The one pointed out by Rob Herring's script, which I can reproduce too BTW. On
the other hand I can't really tell what's wrong right away.

> I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls):
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> 
> > 
> > > +if:
> > > +  properties:
> > > +    compatible:
> > > +      enum:
> > > +        - brcm,bcm6368-rng
> > > +then:
> > > +  properties:
> > > +    resets:
> > > +      maxItems: 1
> > > +  required:
> > > +    - resets
> > 
> > I belive you can't really make a property required when the bindings for
> > 'brcm,bcm6368-rng' were already defined. This will break the schema for those
> > otherwise correct devicetrees.
> 
> Why not?
> Wouldn’t just be required for brcm,bcm6368-rng?

Well, do all 'brcm,bcm6368-rng' users absolutely need the reset controller? If
so, the original binding is wrong, which should be mentioned and a 'Fixes:' tag
should be added to the commit message. Otherwise, the requirement is optional.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Álvaro Fernández Rojas]

Hi Nicolas,

> El 4 mar 2021, a las 14:30, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió:
> 
> On Thu, 2021-03-04 at 13:18 +0100, Álvaro Fernández Rojas wrote:
>> 
>>> El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió:
>>> 
>>> Hi Alvaro,
>>> 
>>> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
>>>> Some devices may need to perform a reset before using the RNG, such as the
>>>> BCM6368.
>>>> 
>>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>>> ---
>>>>  v3: make resets required if brcm,bcm6368-rng.
>>>>  v2: document reset support.
>>>> 
>>>>  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
>>>>  1 file changed, 17 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> index c147900f9041..11c23e1f6988 100644
>>>> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>>>> @@ -37,6 +37,21 @@ required:
>>>>  
>>>> 
>>>> 
>>>>  additionalProperties: false
>>> 
>>> I can't claim I fully understand all the meta stuff in shemas, so I generally
>>> just follow the patterns already available out there.
>> 
>> Well, that makes two of us :).
>> 
>>> That said, shoudln't this be at the end, just before the examples?
>> 
>> I don’t know but I can move it there ¯\_(ツ)_/¯
> 
> Yes please do. See commit 7f464532b05 ("dt-bindings: Add missing
> 'additionalProperties: false'") which expands on why it is needed, and why it
> should be at the end.
> 
>>> Maybe the cause of that odd warning
>>> you got there?
>> 
>> Which odd warning?
> 
> The one pointed out by Rob Herring's script, which I can reproduce too BTW. On
> the other hand I can't really tell what's wrong right away.

Well, I can’t reproduce that locally and I don’t know what’s wrong either, I think that the best option is to remove the full if block.

> 
>> I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls):
>> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
>> 
>>> 
>>>> +if:
>>>> +  properties:
>>>> +    compatible:
>>>> +      enum:
>>>> +        - brcm,bcm6368-rng
>>>> +then:
>>>> +  properties:
>>>> +    resets:
>>>> +      maxItems: 1
>>>> +  required:
>>>> +    - resets
>>> 
>>> I belive you can't really make a property required when the bindings for
>>> 'brcm,bcm6368-rng' were already defined. This will break the schema for those
>>> otherwise correct devicetrees.
>> 
>> Why not?
>> Wouldn’t just be required for brcm,bcm6368-rng?
> 
> Well, do all 'brcm,bcm6368-rng' users absolutely need the reset controller? If
> so, the original binding is wrong, which should be mentioned and a 'Fixes:' tag
> should be added to the commit message. Otherwise, the requirement is optional.

I’m not sure about this...

> 
> Regards,
> Nicolas

Best regards,
Álvaro.

Re: [PATCH v3 1/2] dt-bindings: rng: bcm2835: document reset support

[Rob Herring]

On Thu, Mar 4, 2021 at 6:07 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Alvaro,
>
> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote:
> > Some devices may need to perform a reset before using the RNG, such as the
> > BCM6368.
> >
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > ---
> >  v3: make resets required if brcm,bcm6368-rng.
> >  v2: document reset support.
> >
> >  .../devicetree/bindings/rng/brcm,bcm2835.yaml   | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > index c147900f9041..11c23e1f6988 100644
> > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml
> > @@ -37,6 +37,21 @@ required:
> >
> >
> >  additionalProperties: false
>
> I can't claim I fully understand all the meta stuff in shemas, so I generally
> just follow the patterns already available out there. That said, shoudln't this
> be at the end, just before the examples? Maybe the cause of that odd warning
> you got there?

Yes, 'resets' needs to be defined under 'properties' as
additionalProperties can't 'see' into if/then schemas. The top level
needs to define everything and then if/then schema just add additional
constraints.

>
> > +if:
> > +  properties:
> > +    compatible:
> > +      enum:
> > +        - brcm,bcm6368-rng
> > +then:
> > +  properties:
> > +    resets:
> > +      maxItems: 1
> > +  required:
> > +    - resets
>
> I belive you can't really make a property required when the bindings for
> 'brcm,bcm6368-rng' were already defined. This will break the schema for those
> otherwise correct devicetrees.

Right, unless not having the property meant the device never would have worked.

Rob

[PATCH v3 2/2] hwrng: bcm2835: add reset support

[Álvaro Fernández Rojas]

BCM6368 devices need to reset the in order to generate true random numbers.
This is what BCM6368 produces without a reset:
root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
rngtest 6.10
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: 20000032
rngtest: FIPS 140-2 successes: 0
rngtest: FIPS 140-2 failures: 1000
rngtest: FIPS 140-2(2001-10-10) Monobit: 2
rngtest: FIPS 140-2(2001-10-10) Poker: 1000
rngtest: FIPS 140-2(2001-10-10) Runs: 1000
rngtest: FIPS 140-2(2001-10-10) Long run: 30
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
rngtest: Program run time: 1336176 microseconds
cat: write error: Broken pipe

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 v3: no changes.
 v2: no changes.

 drivers/char/hw_random/bcm2835-rng.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c
index 1a7c43b43c6b..1b93a896d8e8 100644
--- a/drivers/char/hw_random/bcm2835-rng.c
+++ b/drivers/char/hw_random/bcm2835-rng.c
@@ -13,6 +13,7 @@
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #define RNG_CTRL	0x0
 #define RNG_STATUS	0x4
@@ -32,6 +33,7 @@ struct bcm2835_rng_priv {
 	void __iomem *base;
 	bool mask_interrupts;
 	struct clk *clk;
+	struct reset_control *reset;
 };
 
 static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
@@ -94,6 +96,10 @@ static int bcm2835_rng_init(struct hwrng *rng)
 			return ret;
 	}
 
+	ret = reset_control_reset(priv->reset);
+	if (ret)
+		return ret;
+
 	if (priv->mask_interrupts) {
 		/* mask the interrupt */
 		val = rng_readl(priv, RNG_INT_MASK);
@@ -159,6 +165,10 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
 	if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
+	priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(priv->reset))
+		return PTR_ERR(priv->reset);
+
 	priv->rng.name = pdev->name;
 	priv->rng.init = bcm2835_rng_init;
 	priv->rng.read = bcm2835_rng_read;
-- 
2.20.1

Re: [PATCH v3 2/2] hwrng: bcm2835: add reset support

[Florian Fainelli]

On 2/23/2021 9:00 AM, Álvaro Fernández Rojas wrote:
> BCM6368 devices need to reset the in order to generate true random numbers.
> This is what BCM6368 produces without a reset:
> root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
> rngtest 6.10
> 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: 20000032
> rngtest: FIPS 140-2 successes: 0
> rngtest: FIPS 140-2 failures: 1000
> rngtest: FIPS 140-2(2001-10-10) Monobit: 2
> rngtest: FIPS 140-2(2001-10-10) Poker: 1000
> rngtest: FIPS 140-2(2001-10-10) Runs: 1000
> rngtest: FIPS 140-2(2001-10-10) Long run: 30
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
> rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
> rngtest: Program run time: 1336176 microseconds
> cat: write error: Broken pipe
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  v3: no changes.
>  v2: no changes.
> 
>  drivers/char/hw_random/bcm2835-rng.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/char/hw_random/bcm2835-rng.c b/drivers/char/hw_random/bcm2835-rng.c
> index 1a7c43b43c6b..1b93a896d8e8 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c
> @@ -13,6 +13,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
>  #include <linux/clk.h>
> +#include <linux/reset.h>
>  
>  #define RNG_CTRL	0x0
>  #define RNG_STATUS	0x4
> @@ -32,6 +33,7 @@ struct bcm2835_rng_priv {
>  	void __iomem *base;
>  	bool mask_interrupts;
>  	struct clk *clk;
> +	struct reset_control *reset;
>  };
>  
>  static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
> @@ -94,6 +96,10 @@ static int bcm2835_rng_init(struct hwrng *rng)
>  			return ret;
>  	}
>  
> +	ret = reset_control_reset(priv->reset);
> +	if (ret)
> +		return ret;

For symmetry, the remove path should call reset_control_rearm(),
assuming that .reset() is what you want and not .deassert()/assert().
-- 
Florian