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

[PATCH 0/3] 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. Also, there's nothing we can do in parallel while self
test or initial seeding are running.

Martin Kaiser (3):
  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 to wait for end of seeding

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

-- 
2.39.2

[PATCH 1/3] 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.

Active waiting is no disadvantage here. The self test is run during
probe, there's nothing we could do in parallel at this time.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/char/hw_random/imx-rngc.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index e4b385b01b11..85207535fd12 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,21 +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, msecs_to_jiffies(RNGC_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, 1000, RNGC_TIMEOUT * 1000);
+	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

[PATCH 2/3] 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, 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 85207535fd12..d2df468fd460 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

[PATCH 3/3] hwrng: imx-rngc - use polling to wait for end of seeding

[Martin Kaiser]

Use polling to wait until the imx-rngc is properly seeded.

We do this only in the init function when the imx-rngc becomes active.
Polling is ok at this time, there's nothing else we could do while
we're waiting.

We can now remove the code for the interrupt and the completion.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/char/hw_random/imx-rngc.c | 81 ++++---------------------------
 1 file changed, 9 insertions(+), 72 deletions(-)

diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
index d2df468fd460..7ab9aada72d0 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,56 +128,32 @@ 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);
-	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, msecs_to_jiffies(RNGC_TIMEOUT));
-		if (!ret) {
-			ret = -ETIMEDOUT;
-			goto err;
-		}
+		ret = readl_poll_timeout(rngc->base + RNGC_STATUS, status,
+					 status & RNGC_STATUS_SEED_DONE, 1000, RNGC_TIMEOUT * 1000);
+		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
@@ -202,23 +163,7 @@ 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;
-
-err:
-	imx_rngc_irq_mask_clear(rngc);
-	return ret;
-}
-
-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)
@@ -254,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;
@@ -267,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

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

[Alexander Stein]

Am Dienstag, 22. August 2023, 17:25:51 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.
> 
> Active waiting is no disadvantage here. The self test is run during
> probe, there's nothing we could do in parallel at this time.

If this driver is built-in you are stalling the boot process while polling, 
no? Unless probe_type = PROBE_PREFER_ASYNCHRONOUS is set of course.

Best regards,
Alexander

> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/char/hw_random/imx-rngc.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/hw_random/imx-rngc.c
> b/drivers/char/hw_random/imx-rngc.c index e4b385b01b11..85207535fd12 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,21 +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,
> msecs_to_jiffies(RNGC_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, 1000, 
RNGC_TIMEOUT * 1000);
> +	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/

Re: [PATCH 3/3] hwrng: imx-rngc - use polling to wait for end of seeding

[Alexander Stein]

Am Dienstag, 22. August 2023, 17:25:53 CEST schrieb Martin Kaiser:
> Use polling to wait until the imx-rngc is properly seeded.

What is the benefit of burning CPU cycles while waiting for hardware?

> We do this only in the init function when the imx-rngc becomes active.
> Polling is ok at this time, there's nothing else we could do while
> we're waiting.
> 
> We can now remove the code for the interrupt and the completion.

Please split the change to polling and IRQ removal into two patches.

> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/char/hw_random/imx-rngc.c | 81 ++++---------------------------
>  1 file changed, 9 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/char/hw_random/imx-rngc.c
> b/drivers/char/hw_random/imx-rngc.c index d2df468fd460..7ab9aada72d0 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,56 +128,32 @@ 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);
> -	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,
> msecs_to_jiffies(RNGC_TIMEOUT)); -		if (!ret) {
> -			ret = -ETIMEDOUT;
> -			goto err;
> -		}
> +		ret = readl_poll_timeout(rngc->base + RNGC_STATUS, status,
> +					 status & 
RNGC_STATUS_SEED_DONE, 1000, RNGC_TIMEOUT * 1000);

So you want to poll for up to 3s?

Best regards,
Alexander

> +		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
> @@ -202,23 +163,7 @@ 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;
> -
> -err:
> -	imx_rngc_irq_mask_clear(rngc);
> -	return ret;
> -}
> -
> -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)
> @@ -254,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;
> @@ -267,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)


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

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

[Martin Kaiser]

Hi Alexander,

thanks for reviewing the patches.

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

> Am Dienstag, 22. August 2023, 17:25:51 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.

> > Active waiting is no disadvantage here. The self test is run during
> > probe, there's nothing we could do in parallel at this time.

> If this driver is built-in you are stalling the boot process while
> polling, no?

According to the manual, "The self test takes approximately 29,000
cycles to complete." On my system, the rngb peripheral clock runs at
66.5MHz, i.e. the self test takes 436us.

A poll interval of 480us and a timeout of 1.5ms are more appropriate
than the current values (1ms poll interval, 3sec timeout). I'll fix this
in v2.

If it is unacceptable for a probe function to run for 1.5ms, we could
probably move the self test to the init function. It's called when the
rng is selected as the active rng.

> Unless probe_type = PROBE_PREFER_ASYNCHRONOUS is set of
> course.

This does not work for drivers that use module_platform_driver_probe.

We could switch to module_platform_driver. I'd prefer fixing the timing
or moving the self test to init, though.

Best regards,
Martin

Re: [PATCH 3/3] hwrng: imx-rngc - use polling to wait for end of seeding

[Martin Kaiser]

Hi Alexander,

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

> Am Dienstag, 22. August 2023, 17:25:53 CEST schrieb Martin Kaiser:
> > Use polling to wait until the imx-rngc is properly seeded.

> What is the benefit of burning CPU cycles while waiting for hardware?

it seems to me that

readl_poll_timeout
   readx_poll_timeout
      usleep_range
         usleep_range_state
            schedule_hrtimeout_range

will not wait in the foreground and burn CPU cycles.

The comment for schedule_hrtimeout_range says

 * Make the current task sleep until the given expiry time has
 * elapsed.

> > We do this only in the init function when the imx-rngc becomes active.
> > Polling is ok at this time, there's nothing else we could do while
> > we're waiting.

> > We can now remove the code for the interrupt and the completion.

> Please split the change to polling and IRQ removal into two patches.

Good point. Will do this in v2.

> RNGC_STATUS_SEED_DONE, 1000, RNGC_TIMEOUT * 1000);
> So you want to poll for up to 3s?

According to the manual, "The initial seed takes approximately 2,000,000
clock cycles." With a 66.5MHz clock, this is approx 30ms. So that should
be

RNGC_STATUS_SEED_DONE, 20000, 100000);

The comment for readx_poll_timeout suggests <= 20ms for the poll
interval.

Best regards,
Martin