[PATCH v2 0/6] hwrng: imx-rngc - use polling instead of interrupt

[PATCH v2 0/6] hwrng: imx-rngc - use polling instead of interrupt

[Martin Kaiser]

Use polling and wait actively for the self test and the initial seeding
of the rngc to complete. This is much simpler than using an interrupt
and a completion.

v2:
- set reasonable timeouts
- separate commit for the removal of interrupt handling
- readl_poll_timeout does not wait in the foreground

Martin Kaiser (6):
  hwrng: imx-rngc - reasonable timeout for selftest
  hwrng: imx-rngc - reasonable timeout for initial seed
  hwrng: imx-rngc - use polling to detect end of self test
  hwrng: imx-rngc - read status register for error checks
  hwrng: imx-rngc - use polling for initial seed
  hwrng: imx-rngc - remove interrupt handler

 drivers/char/hw_random/imx-rngc.c | 100 ++++++------------------------
 1 file changed, 18 insertions(+), 82 deletions(-)

-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/6] hwrng: imx-rngc - reasonable timeout for selftest

[Martin Kaiser]

Set a more reasonable timeout for the rngc selftest.

According to the reference manual, "The self test takes approximately
29,000 cycles to complete." With the rngc peripheral clock running at
66.5 MHz, this would be 436us. Let's use 1.5ms insteaf of 3sec for the
timeout.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
- adjust timeouts before we switch to polling

 drivers/char/hw_random/imx-rngc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index e4b385b01b11..6024c923b67d 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -53,6 +53,7 @@
 
 #define RNGC_TIMEOUT  3000 /* 3 sec */
 
+#define RNGC_SELFTEST_TIMEOUT 1500 /* us */
 
 static bool self_test = true;
 module_param(self_test, bool, 0);
@@ -110,7 +111,8 @@ static int imx_rngc_self_test(struct imx_rngc *rngc)
 	cmd = readl(rngc->base + RNGC_COMMAND);
 	writel(cmd | RNGC_CMD_SELF_TEST, rngc->base + RNGC_COMMAND);
 
-	ret = wait_for_completion_timeout(&rngc->rng_op_done, msecs_to_jiffies(RNGC_TIMEOUT));
+	ret = wait_for_completion_timeout(&rngc->rng_op_done,
+					  usecs_to_jiffies(RNGC_SELFTEST_TIMEOUT));
 	imx_rngc_irq_mask_clear(rngc);
 	if (!ret)
 		return -ETIMEDOUT;
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/6] hwrng: imx-rngc - reasonable timeout for initial seed

[Martin Kaiser]

Set a more reasonable timeout for calculating the initial seed.

The reference manuals says that "The initial seed takes approximately
2,000,000 clock cycles." If the rngc peripheral clock runs at 66.5MHz,
this is approx. 30ms.

A timeout of 100ms is more appropriate that the current value of 3
seconds. We define the timeout in us to simplify the transition to
readl_poll_timeout.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
- adjust timeouts before we switch to polling

 drivers/char/hw_random/imx-rngc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index 6024c923b67d..8ff3d46674fd 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -51,9 +51,8 @@
 
 #define RNGC_ERROR_STATUS_STAT_ERR	0x00000008
 
-#define RNGC_TIMEOUT  3000 /* 3 sec */
-
-#define RNGC_SELFTEST_TIMEOUT 1500 /* us */
+#define RNGC_SELFTEST_TIMEOUT   1500 /* us */
+#define RNGC_SEED_TIMEOUT     100000 /* us */
 
 static bool self_test = true;
 module_param(self_test, bool, 0);
@@ -184,7 +183,8 @@ static int imx_rngc_init(struct hwrng *rng)
 		cmd = readl(rngc->base + RNGC_COMMAND);
 		writel(cmd | RNGC_CMD_SEED, rngc->base + RNGC_COMMAND);
 
-		ret = wait_for_completion_timeout(&rngc->rng_op_done, msecs_to_jiffies(RNGC_TIMEOUT));
+		ret = wait_for_completion_timeout(&rngc->rng_op_done,
+						  usecs_to_jiffies(RNGC_SEED_TIMEOUT));
 		if (!ret) {
 			ret = -ETIMEDOUT;
 			goto err;
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/6] hwrng: imx-rngc - use polling to detect end of self test

[Martin Kaiser]

Use polling to detect the end of the rngc self test. This is much simpler
than using an interrupt and a completion.

The selftest should take approx. 450us. Keep the overhead to a minimum
by polling every 500us. (We've already lowered the timeout to 1.5ms.)

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
- use shorter timeout and polling interval

 drivers/char/hw_random/imx-rngc.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index 8ff3d46674fd..09523936d2af 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -17,6 +17,7 @@
 #include <linux/hw_random.h>
 #include <linux/completion.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/bitfield.h>
 
 #define RNGC_VER_ID			0x0000
@@ -101,22 +102,19 @@ static inline void imx_rngc_irq_unmask(struct imx_rngc *rngc)
 
 static int imx_rngc_self_test(struct imx_rngc *rngc)
 {
-	u32 cmd;
+	u32 cmd, status;
 	int ret;
 
-	imx_rngc_irq_unmask(rngc);
-
 	/* run self test */
 	cmd = readl(rngc->base + RNGC_COMMAND);
 	writel(cmd | RNGC_CMD_SELF_TEST, rngc->base + RNGC_COMMAND);
 
-	ret = wait_for_completion_timeout(&rngc->rng_op_done,
-					  usecs_to_jiffies(RNGC_SELFTEST_TIMEOUT));
-	imx_rngc_irq_mask_clear(rngc);
-	if (!ret)
-		return -ETIMEDOUT;
+	ret = readl_poll_timeout(rngc->base + RNGC_STATUS, status,
+				 status & RNGC_STATUS_ST_DONE, 500, RNGC_SELFTEST_TIMEOUT);
+	if (ret < 0)
+		return ret;
 
-	return rngc->err_reg ? -EIO : 0;
+	return readl(rngc->base + RNGC_ERROR) ? -EIO : 0;
 }
 
 static int imx_rngc_read(struct hwrng *rng, void *data, size_t max, bool wait)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] hwrng: imx-rngc - use polling to detect end of self test

[Alexander Stein]

Hi Martin,

thanks for splitting the series into single patches.

Am Donnerstag, 24. August 2023, 21:20:56 CEST schrieb Martin Kaiser:
> Use polling to detect the end of the rngc self test. This is much simpler
> than using an interrupt and a completion.

I'm still not convinced that using polling is simpler. By using 
readl_poll_timeout() you will also get an interrupt, the timer one. Why 
exactly is using polling much (!) simpler?

> The selftest should take approx. 450us. Keep the overhead to a minimum
> by polling every 500us. (We've already lowered the timeout to 1.5ms.)

I suppose these times only hold true for a specific peripheral clock 
frequency. Is it guaranteed that this frequency is fixed?
For using IRQ it's simpler, there is no guessing: you return once the self 
test finished. The timeout is identical anyway.

Best regards,
Alexander

> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
> v2:
> - use shorter timeout and polling interval
> 
>  drivers/char/hw_random/imx-rngc.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/hw_random/imx-rngc.c
> b/drivers/char/hw_random/imx-rngc.c index 8ff3d46674fd..09523936d2af 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -17,6 +17,7 @@
>  #include <linux/hw_random.h>
>  #include <linux/completion.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/bitfield.h>
> 
>  #define RNGC_VER_ID			0x0000
> @@ -101,22 +102,19 @@ static inline void imx_rngc_irq_unmask(struct imx_rngc
> *rngc)
> 
>  static int imx_rngc_self_test(struct imx_rngc *rngc)
>  {
> -	u32 cmd;
> +	u32 cmd, status;
>  	int ret;
> 
> -	imx_rngc_irq_unmask(rngc);
> -
>  	/* run self test */
>  	cmd = readl(rngc->base + RNGC_COMMAND);
>  	writel(cmd | RNGC_CMD_SELF_TEST, rngc->base + RNGC_COMMAND);
> 
> -	ret = wait_for_completion_timeout(&rngc->rng_op_done,
> -					  
usecs_to_jiffies(RNGC_SELFTEST_TIMEOUT));
> -	imx_rngc_irq_mask_clear(rngc);
> -	if (!ret)
> -		return -ETIMEDOUT;
> +	ret = readl_poll_timeout(rngc->base + RNGC_STATUS, status,
> +				 status & RNGC_STATUS_ST_DONE, 500, 
RNGC_SELFTEST_TIMEOUT);
> +	if (ret < 0)
> +		return ret;
> 
> -	return rngc->err_reg ? -EIO : 0;
> +	return readl(rngc->base + RNGC_ERROR) ? -EIO : 0;
>  }
> 
>  static int imx_rngc_read(struct hwrng *rng, void *data, size_t max, bool
> wait)


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/6] hwrng: imx-rngc - use polling to detect end of self test

[Martin Kaiser]

Hi Alexander,

Alexander Stein (alexander.stein@ew.tq-group.com) wrote:

> I'm still not convinced that using polling is simpler. By using 
> readl_poll_timeout() you will also get an interrupt, the timer one. Why 
> exactly is using polling much (!) simpler?

it requires much less code in the driver.

> > The selftest should take approx. 450us. Keep the overhead to a minimum
> > by polling every 500us. (We've already lowered the timeout to 1.5ms.)

> I suppose these times only hold true for a specific peripheral clock 
> frequency. Is it guaranteed that this frequency is fixed?

Good point. The lowest possible peripheral clock frequency is half of
what I used for the calculations, i.e. 33.25MHz. That would double the
durations. Should be ok for the selftest. But for the initial seed, we'd
get into a region where readl_poll_timeout (usleep_range) does no longer
make sense.

> For using IRQ it's simpler, there is no guessing: you return once the self 
> test finished. The timeout is identical anyway.

I've looked at other callers of readl_poll_timeout. It seems that none
of them is called in a driver's probe function or uses an overall
timeout of 200ms.

I'll keep the interrupt + completion and resubmit the patches for
adjusting the timeouts.

Thanks,
Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/6] hwrng: imx-rngc - read status register for error checks

[Martin Kaiser]

The error bit in the status register of the imx-rngc is set for any kind
of error. Details about the error can be read from the bits in the error
status register.

In the imx_rngc_self_test function, we just need the info if there was an
error or not. We can check the status register, there's no need to read
the error status register.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/char/hw_random/imx-rngc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index 09523936d2af..c2582673427d 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -114,7 +114,7 @@ static int imx_rngc_self_test(struct imx_rngc *rngc)
 	if (ret < 0)
 		return ret;
 
-	return readl(rngc->base + RNGC_ERROR) ? -EIO : 0;
+	return (status & RNGC_STATUS_ERROR) ? -EIO : 0;
 }
 
 static int imx_rngc_read(struct hwrng *rng, void *data, size_t max, bool wait)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/6] hwrng: imx-rngc - use polling for initial seed

[Martin Kaiser]

Use polling instead of interrupt + completion to wait until the initial
seed was calculated. This should take about 30ms.

Call readl_poll_timeout and set the poll interval to the recommended
maximum of 20ms. readl_poll_timeout uses usleep_range, this is based on
a hrtimer. The task will we put to sleep while waiting, this does not
burn CPU cycles by waiting in the foreground.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
- use shorter timeout and polling interval
- readl_poll_timeout does not wait in the foreground

 drivers/char/hw_random/imx-rngc.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index c2582673427d..3a3f0f923bed 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -166,34 +166,29 @@ static irqreturn_t imx_rngc_irq(int irq, void *priv)
 static int imx_rngc_init(struct hwrng *rng)
 {
 	struct imx_rngc *rngc = container_of(rng, struct imx_rngc, rng);
-	u32 cmd, ctrl;
+	u32 cmd, ctrl, status, err_reg;
 	int ret;
 
 	/* clear error */
 	cmd = readl(rngc->base + RNGC_COMMAND);
 	writel(cmd | RNGC_CMD_CLR_ERR, rngc->base + RNGC_COMMAND);
 
-	imx_rngc_irq_unmask(rngc);
-
 	/* create seed, repeat while there is some statistical error */
 	do {
 		/* seed creation */
 		cmd = readl(rngc->base + RNGC_COMMAND);
 		writel(cmd | RNGC_CMD_SEED, rngc->base + RNGC_COMMAND);
 
-		ret = wait_for_completion_timeout(&rngc->rng_op_done,
-						  usecs_to_jiffies(RNGC_SEED_TIMEOUT));
-		if (!ret) {
-			ret = -ETIMEDOUT;
-			goto err;
-		}
+		ret = readl_poll_timeout(rngc->base + RNGC_STATUS, status,
+					 status & RNGC_STATUS_SEED_DONE, 20000, RNGC_SEED_TIMEOUT);
+		if (ret < 0)
+			return ret;
 
-	} while (rngc->err_reg == RNGC_ERROR_STATUS_STAT_ERR);
+		err_reg = readl(rngc->base + RNGC_ERROR);
+	} while (err_reg == RNGC_ERROR_STATUS_STAT_ERR);
 
-	if (rngc->err_reg) {
-		ret = -EIO;
-		goto err;
-	}
+	if (err_reg)
+		return -EIO;
 
 	/*
 	 * enable automatic seeding, the rngc creates a new seed automatically
@@ -209,10 +204,6 @@ static int imx_rngc_init(struct hwrng *rng)
 	 * we mask the interrupt ourselves if we return an error
 	 */
 	return 0;
-
-err:
-	imx_rngc_irq_mask_clear(rngc);
-	return ret;
 }
 
 static void imx_rngc_cleanup(struct hwrng *rng)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/6] hwrng: imx-rngc - remove interrupt handler

[Martin Kaiser]

Remove the interrupt handler and the code for its installation and
cleanup.

We use readl_poll_timeout now for the selftest and the initial seed.
There are no more users of the interrupt.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
- separate commit for removing irq code

 drivers/char/hw_random/imx-rngc.c | 55 -------------------------------
 1 file changed, 55 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index 3a3f0f923bed..7fe09c59ce19 100644
--- a/drivers/char/hw_random/imx-rngc.c
+++ b/drivers/char/hw_random/imx-rngc.c
@@ -63,12 +63,6 @@ struct imx_rngc {
 	struct clk		*clk;
 	void __iomem		*base;
 	struct hwrng		rng;
-	struct completion	rng_op_done;
-	/*
-	 * err_reg is written only by the irq handler and read only
-	 * when interrupts are masked, we need no spinlock
-	 */
-	u32			err_reg;
 };
 
 
@@ -91,15 +85,6 @@ static inline void imx_rngc_irq_mask_clear(struct imx_rngc *rngc)
 	writel(cmd, rngc->base + RNGC_COMMAND);
 }
 
-static inline void imx_rngc_irq_unmask(struct imx_rngc *rngc)
-{
-	u32 ctrl;
-
-	ctrl = readl(rngc->base + RNGC_CONTROL);
-	ctrl &= ~(RNGC_CTRL_MASK_DONE | RNGC_CTRL_MASK_ERROR);
-	writel(ctrl, rngc->base + RNGC_CONTROL);
-}
-
 static int imx_rngc_self_test(struct imx_rngc *rngc)
 {
 	u32 cmd, status;
@@ -143,26 +128,6 @@ static int imx_rngc_read(struct hwrng *rng, void *data, size_t max, bool wait)
 	return retval ? retval : -EIO;
 }
 
-static irqreturn_t imx_rngc_irq(int irq, void *priv)
-{
-	struct imx_rngc *rngc = (struct imx_rngc *)priv;
-	u32 status;
-
-	/*
-	 * clearing the interrupt will also clear the error register
-	 * read error and status before clearing
-	 */
-	status = readl(rngc->base + RNGC_STATUS);
-	rngc->err_reg = readl(rngc->base + RNGC_ERROR);
-
-	imx_rngc_irq_mask_clear(rngc);
-
-	if (status & (RNGC_STATUS_SEED_DONE | RNGC_STATUS_ST_DONE))
-		complete(&rngc->rng_op_done);
-
-	return IRQ_HANDLED;
-}
-
 static int imx_rngc_init(struct hwrng *rng)
 {
 	struct imx_rngc *rngc = container_of(rng, struct imx_rngc, rng);
@@ -198,21 +163,9 @@ static int imx_rngc_init(struct hwrng *rng)
 	ctrl |= RNGC_CTRL_AUTO_SEED;
 	writel(ctrl, rngc->base + RNGC_CONTROL);
 
-	/*
-	 * if initialisation was successful, we keep the interrupt
-	 * unmasked until imx_rngc_cleanup is called
-	 * we mask the interrupt ourselves if we return an error
-	 */
 	return 0;
 }
 
-static void imx_rngc_cleanup(struct hwrng *rng)
-{
-	struct imx_rngc *rngc = container_of(rng, struct imx_rngc, rng);
-
-	imx_rngc_irq_mask_clear(rngc);
-}
-
 static int __init imx_rngc_probe(struct platform_device *pdev)
 {
 	struct imx_rngc *rngc;
@@ -246,12 +199,9 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
 	if (rng_type != RNGC_TYPE_RNGC && rng_type != RNGC_TYPE_RNGB)
 		return -ENODEV;
 
-	init_completion(&rngc->rng_op_done);
-
 	rngc->rng.name = pdev->name;
 	rngc->rng.init = imx_rngc_init;
 	rngc->rng.read = imx_rngc_read;
-	rngc->rng.cleanup = imx_rngc_cleanup;
 	rngc->rng.quality = 19;
 
 	rngc->dev = &pdev->dev;
@@ -259,11 +209,6 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
 
 	imx_rngc_irq_mask_clear(rngc);
 
-	ret = devm_request_irq(&pdev->dev,
-			irq, imx_rngc_irq, 0, pdev->name, (void *)rngc);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret, "Can't get interrupt working.\n");
-
 	if (self_test) {
 		ret = imx_rngc_self_test(rngc);
 		if (ret)
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel