| Message ID | 20230824192059.1569591-4-martin@kaiser.cx (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | hwrng: imx-rngc - use polling instead of interrupt | expand |
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)
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
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)
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(-)