[PATCH v2 0/3] hwrng: st: Couple of improvements

[PATCH v2 0/3] hwrng: st: Couple of improvements

[Lee Jones]

This set contains a minor documentation fix, greater clarification
with how the FIFO depth/size is represented and finally a fix to
prevent early timeout during data acquisition.

v1 => v2:
 - Remove phantom bugfix (FIFO really is 4 deep, not 8)
 - Double timeout value to account for inconsistent call to udelay()
   - Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>

Lee Jones (3):
  hwrng: st: dt: Fix trivial typo in node address
  hwrng: st: Use real-world device timings for timeout
  hwrng: st: Improve FIFO size/depth description

 Documentation/devicetree/bindings/rng/st,rng.txt |  2 +-
 drivers/char/hw_random/st-rng.c                  | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] hwrng: st: dt: Fix trivial typo in node address

[Lee Jones]

DT nodes should not append their addresses with '0x'.

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/rng/st,rng.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rng/st,rng.txt b/Documentation/devicetree/bindings/rng/st,rng.txt
index dbc64e6..35734bc 100644
--- a/Documentation/devicetree/bindings/rng/st,rng.txt
+++ b/Documentation/devicetree/bindings/rng/st,rng.txt
@@ -8,7 +8,7 @@ clocks		: Phandle to device's clock (See: ../clocks/clock-bindings.txt)
 
 Example:
 
-rng@0xfee80000 {
+rng@fee80000 {
 	compatible      = "st,rng";
 	reg		= <0xfee80000 0x1000>;
 	clocks          = <&clk_sysin>;
-- 
1.9.1

[PATCH v2 2/3] hwrng: st: Use real-world device timings for timeout

[Lee Jones]

Samples are documented to be available every 0.667us, so in theory
the 8 sample deep FIFO should take 5.336us to fill.  However, during
thorough testing, it became apparent that filling the FIFO actually
takes closer to 12us.

Also take into consideration that udelay() can behave oddly i.e. not
delay for as long as requested.

  Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>:

    "IIRC, Linus recommends a x2 factor on delays, especially
     timeouts generated by these functions.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/char/hw_random/st-rng.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
index 8c8a435..17f0a09 100644
--- a/drivers/char/hw_random/st-rng.c
+++ b/drivers/char/hw_random/st-rng.c
@@ -32,8 +32,14 @@
 #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))
+/*
+ * Samples are documented to be available every 0.667us, so in theory
+ * the 4 sample deep FIFO should take 2.668us to fill.  However, during
+ * thorough testing, it became apparent that filling the FIFO actually
+ * takes closer to 12us.  We then multiply by 2 in order to account for
+ * the lack of udelay()'s reliability, suggested by Russell King.
+ */
+#define ST_RNG_FILL_FIFO_TIMEOUT	(12 * 2)
 
 struct st_rng_data {
 	void __iomem	*base;
-- 
1.9.1

[PATCH v2 3/3] hwrng: st: Improve FIFO size/depth description

[Lee Jones]

The original representation of FIFO size in the driver coupled with the
ambiguity in the documentation meant that it was easy to confuse readers.
This lead to a false positive BUG-find and subsequently time wastage
debugging this phantom issue.

Hopefully this patch can prevent future readers from falling into the
same trap.

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

diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
index 17f0a09..1d35363 100644
--- a/drivers/char/hw_random/st-rng.c
+++ b/drivers/char/hw_random/st-rng.c
@@ -29,8 +29,9 @@
 #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 */
+#define ST_RNG_FIFO_DEPTH		4
+#define ST_RNG_FIFO_SIZE		(ST_RNG_FIFO_DEPTH * ST_RNG_SAMPLE_SIZE)
 
 /*
  * Samples are documented to be available every 0.667us, so in theory
-- 
1.9.1

Re: [PATCH v2 0/3] hwrng: st: Couple of improvements

[Herbert Xu]

On Wed, Oct 07, 2015 at 01:23:26PM +0100, Lee Jones wrote:
> This set contains a minor documentation fix, greater clarification
> with how the FIFO depth/size is represented and finally a fix to
> prevent early timeout during data acquisition.
> 
> v1 => v2:
>  - Remove phantom bugfix (FIFO really is 4 deep, not 8)
>  - Double timeout value to account for inconsistent call to udelay()
>    - Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>

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/3] hwrng: st: Couple of improvements

[Lee Jones]

On Thu, 08 Oct 2015, Herbert Xu wrote:

> On Wed, Oct 07, 2015 at 01:23:26PM +0100, Lee Jones wrote:
> > This set contains a minor documentation fix, greater clarification
> > with how the FIFO depth/size is represented and finally a fix to
> > prevent early timeout during data acquisition.
> > 
> > v1 => v2:
> >  - Remove phantom bugfix (FIFO really is 4 deep, not 8)
> >  - Double timeout value to account for inconsistent call to udelay()
> >    - Suggested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> All applied.  Thanks.

Thanks Herbert.

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