hwrng: bcm2835: add reset support

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

Signed-off-by Álvaro Fernández Rojas <[email protected]>

Quoted 4 lines · Á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 <[email protected]>

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

Quoted 6 lines · Florian Fainelli

Quoted 3 lines · Álvaro Fernández Rojas

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

Quoted 5 lines

Quoted 1 line

Signed-off-by Álvaro Fernández Rojas <[email protected]>

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?

drivers/char/hw_random/bcm2835-rng.c +2 -0

@@ -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);
 }

Hi Álvaro,

On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
[...]

Quoted 13 lines

drivers/char/hw_random/bcm2835-rng.c +2 -0

@@ -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;
 

drivers/char/hw_random/bcm2835-rng.c static int bcm2835_rng_probe(struct platform_device *pdev) · line 176

Quoted 7 lines

drivers/char/hw_random/bcm2835-rng.c +4 -0

+	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

Hi Philipp,

Quoted 19 lines · Philipp Zabel

Hi Álvaro,

On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
[...]

Quoted 13 lines

drivers/char/hw_random/bcm2835-rng.c +2 -0

@@ -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;
 

Quoted 11 lines

drivers/char/hw_random/bcm2835-rng.c static int bcm2835_rng_probe(struct platform_device *pdev) · line 176

Quoted 7 lines

drivers/char/hw_random/bcm2835-rng.c +4 -0

+	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...

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

Signed-off-by Álvaro Fernández Rojas <[email protected]>

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

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

Quoted 11 lines · Á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 <[email protected]>

---
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.

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

Signed-off-by Álvaro Fernández Rojas <[email protected]>

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

 .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++

 1 file changed, 17 insertions(+)

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

index c147900f9041..11c23e1f6988 100644
@@ -37,6 +37,21 @@ required:

 additionalProperties: false

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +4 -0

+else:
+  properties:
+    resets: false
+

 examples:
   - |
     rng@7e104000 {

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

@@ -58,4 +73,6 @@ examples:

         clocks = <&periph_clk 18>;
         clock-names = "ipsec";

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +2 -0

+
+        resets = <&periph_rst 4>;

     };

Hi Alvaro,

Quoted 18 lines · Á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 <[email protected]>

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

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

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

index c147900f9041..11c23e1f6988 100644
@@ -37,6 +37,21 @@ required:
 
 

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 38

Quoted 1 line

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

 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?

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 50

Quoted 12 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +11 -0

+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.

Quoted 11 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +4 -0

+else:
+  properties:
+    resets: false
+
 examples:
   - |
     rng@7e104000 {

@@ -58,4 +73,6 @@ examples:
 
 

Quoted 5 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +2 -0

         clocks = <&periph_clk 18>;
         clock-names = "ipsec";
+
+        resets = <&periph_rst 4>;

     };

Regards,
Nicolas

Quoted 25 lines · Nicolas Saenz Julienne

Hi Alvaro,

Quoted 19 lines · Á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 <[email protected]>

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

 .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++

 1 file changed, 17 insertions(+)

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

index c147900f9041..11c23e1f6988 100644
@@ -37,6 +37,21 @@ required:

Quoted 4 lines

Quoted 1 line

 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 :).

Quoted 1 line

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

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

Quoted 2 lines

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

Quoted 16 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 50

Quoted 12 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +11 -0

+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?

Quoted 30 lines · Álvaro Fernández Rojas

Quoted 25 lines · Nicolas Saenz Julienne

Hi Alvaro,

Quoted 19 lines · Á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 <[email protected]>

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

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

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

index c147900f9041..11c23e1f6988 100644
@@ -37,6 +37,21 @@ required:
 
 
 

Quoted 4 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 38

Quoted 1 line

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

 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.

Quoted 5 lines

Well, that makes two of us :).

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 21 lines

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

Quoted 16 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 50

Quoted 12 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +11 -0

+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.

Quoted 2 lines

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

Hi Alvaro,

Quoted 18 lines · Á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 <[email protected]>

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

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

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

index c147900f9041..11c23e1f6988 100644
@@ -37,6 +37,21 @@ required:
 
 

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 38

Quoted 1 line

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +0 -0

 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?

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml required: · line 50

Quoted 12 lines

Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +11 -0

+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.

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 <[email protected]>

---
v3: no changes.
v2: no changes.

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

drivers/char/hw_random/bcm2835-rng.c +2 -0

index 1a7c43b43c6b..1b93a896d8e8 100644
@@ -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)