1. Patchew
  2. linux
  3. View series

[PATCH] spi: use udelay() for short polling delays

Peter Collingbourne posted 1 patch 1 week ago 
drivers/spi/spi-mem.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
[PATCH] spi: use udelay() for short polling delays
Posted by Peter Collingbourne 1 week ago 
A short polling delay, such as the delay of 5us
(SPINAND_READ_POLL_DELAY_US) provided by the SPI NAND driver,
can become a 1/HZ (order of ms) delay caused by the usleep_range()
call in read_poll_timeout(), significantly reducing SPI NAND access
performance. Fix it by implementing the polling delay with udelay()
(via read_poll_timeout_atomic()) if it is short enough, matching how
the initial delay is handled.

Fixes: c955a0cc8a28 ("spi: spi-mem: add automatic poll status functions")
Signed-off-by: Peter Collingbourne <peter@pcc.me.uk>
---
 drivers/spi/spi-mem.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index a09371a075d2..914e35e51cea 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -1005,10 +1005,19 @@ int spi_mem_poll_status(struct spi_mem *mem,
 			usleep_range((initial_delay_us >> 2) + 1,
 				     initial_delay_us);
 
-		ret = read_poll_timeout(spi_mem_read_status, read_status_ret,
-					(read_status_ret || ((status) & mask) == match),
-					polling_delay_us, timeout_ms * 1000, false, mem,
-					op, &status);
+		if (polling_delay_us < 10)
+			ret = read_poll_timeout_atomic(
+				spi_mem_read_status, read_status_ret,
+				(read_status_ret || ((status)&mask) == match),
+				polling_delay_us, timeout_ms * 1000, false, mem,
+				op, &status);
+		else
+			ret = read_poll_timeout(
+				spi_mem_read_status, read_status_ret,
+				(read_status_ret || ((status)&mask) == match),
+				polling_delay_us, timeout_ms * 1000, false, mem,
+				op, &status);
+
 		if (read_status_ret)
 			return read_status_ret;
 	}
-- 
2.54.0
Re: [PATCH] spi: use udelay() for short polling delays
Posted by Mark Brown 6 days, 19 hours ago 
On Sat, May 16, 2026 at 11:25:54PM -0700, Peter Collingbourne wrote:

> +		if (polling_delay_us < 10)
> +			ret = read_poll_timeout_atomic(
> +				spi_mem_read_status, read_status_ret,
> +				(read_status_ret || ((status)&mask) == match),
> +				polling_delay_us, timeout_ms * 1000, false, mem,
> +				op, &status);
> +		else
> +			ret = read_poll_timeout(
> +				spi_mem_read_status, read_status_ret,
> +				(read_status_ret || ((status)&mask) == match),
> +				polling_delay_us, timeout_ms * 1000, false, mem,
> +				op, &status);

This seems like something that should be fixed in read_poll_timeout()
rather than open coded in a user, similarly to how fsleep() is there for
sleep functions.
Re: [PATCH] spi: use udelay() for short polling delays
Posted by Peter Collingbourne 5 days, 19 hours ago 
On Mon, May 18, 2026 at 3:46 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Sat, May 16, 2026 at 11:25:54PM -0700, Peter Collingbourne wrote:
>
> > +             if (polling_delay_us < 10)
> > +                     ret = read_poll_timeout_atomic(
> > +                             spi_mem_read_status, read_status_ret,
> > +                             (read_status_ret || ((status)&mask) == match),
> > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > +                             op, &status);
> > +             else
> > +                     ret = read_poll_timeout(
> > +                             spi_mem_read_status, read_status_ret,
> > +                             (read_status_ret || ((status)&mask) == match),
> > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > +                             op, &status);
>
> This seems like something that should be fixed in read_poll_timeout()
> rather than open coded in a user, similarly to how fsleep() is there for
> sleep functions.

Sounds reasonable, sent v2 with the fix moved to read_poll_timeout().

Peter
Re: [PATCH] spi: use udelay() for short polling delays
Posted by David Laight 1 week ago 
On Sat, 16 May 2026 23:25:54 -0700
Peter Collingbourne <peter@pcc.me.uk> wrote:

> A short polling delay, such as the delay of 5us
> (SPINAND_READ_POLL_DELAY_US) provided by the SPI NAND driver,
> can become a 1/HZ (order of ms) delay caused by the usleep_range()
> call in read_poll_timeout(), significantly reducing SPI NAND access
> performance. Fix it by implementing the polling delay with udelay()
> (via read_poll_timeout_atomic()) if it is short enough, matching how
> the initial delay is handled.
> 
> Fixes: c955a0cc8a28 ("spi: spi-mem: add automatic poll status functions")
> Signed-off-by: Peter Collingbourne <peter@pcc.me.uk>
> ---
>  drivers/spi/spi-mem.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index a09371a075d2..914e35e51cea 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -1005,10 +1005,19 @@ int spi_mem_poll_status(struct spi_mem *mem,
>  			usleep_range((initial_delay_us >> 2) + 1,
>  				     initial_delay_us);
>  
> -		ret = read_poll_timeout(spi_mem_read_status, read_status_ret,
> -					(read_status_ret || ((status) & mask) == match),
> -					polling_delay_us, timeout_ms * 1000, false, mem,
> -					op, &status);
> +		if (polling_delay_us < 10)
> +			ret = read_poll_timeout_atomic(
> +				spi_mem_read_status, read_status_ret,
> +				(read_status_ret || ((status)&mask) == match),
> +				polling_delay_us, timeout_ms * 1000, false, mem,
> +				op, &status);
> +		else
> +			ret = read_poll_timeout(
> +				spi_mem_read_status, read_status_ret,
> +				(read_status_ret || ((status)&mask) == match),
> +				polling_delay_us, timeout_ms * 1000, false, mem,
> +				op, &status);
> +

That looks rather sub-optional.
Even if the interval is short you want to drop to sleeps if the device isn't
responding.
(Or this code is used for erases and you are doing a full device erase.)

I doubt you want to spin for more than 1ms.

-David


>  		if (read_status_ret)
>  			return read_status_ret;
>  	}
Re: [PATCH] spi: use udelay() for short polling delays
Posted by Peter Collingbourne 1 week ago 
On Sun, May 17, 2026 at 7:02 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Sat, 16 May 2026 23:25:54 -0700
> Peter Collingbourne <peter@pcc.me.uk> wrote:
>
> > A short polling delay, such as the delay of 5us
> > (SPINAND_READ_POLL_DELAY_US) provided by the SPI NAND driver,
> > can become a 1/HZ (order of ms) delay caused by the usleep_range()
> > call in read_poll_timeout(), significantly reducing SPI NAND access
> > performance. Fix it by implementing the polling delay with udelay()
> > (via read_poll_timeout_atomic()) if it is short enough, matching how
> > the initial delay is handled.
> >
> > Fixes: c955a0cc8a28 ("spi: spi-mem: add automatic poll status functions")
> > Signed-off-by: Peter Collingbourne <peter@pcc.me.uk>
> > ---
> >  drivers/spi/spi-mem.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index a09371a075d2..914e35e51cea 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -1005,10 +1005,19 @@ int spi_mem_poll_status(struct spi_mem *mem,
> >                       usleep_range((initial_delay_us >> 2) + 1,
> >                                    initial_delay_us);
> >
> > -             ret = read_poll_timeout(spi_mem_read_status, read_status_ret,
> > -                                     (read_status_ret || ((status) & mask) == match),
> > -                                     polling_delay_us, timeout_ms * 1000, false, mem,
> > -                                     op, &status);
> > +             if (polling_delay_us < 10)
> > +                     ret = read_poll_timeout_atomic(
> > +                             spi_mem_read_status, read_status_ret,
> > +                             (read_status_ret || ((status)&mask) == match),
> > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > +                             op, &status);
> > +             else
> > +                     ret = read_poll_timeout(
> > +                             spi_mem_read_status, read_status_ret,
> > +                             (read_status_ret || ((status)&mask) == match),
> > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > +                             op, &status);
> > +
>
> That looks rather sub-optional.
> Even if the interval is short you want to drop to sleeps if the device isn't
> responding.
> (Or this code is used for erases and you are doing a full device erase.)
>
> I doubt you want to spin for more than 1ms.
>
> -David

In include/linux/mtd/spinand.h we have:

#define SPINAND_READ_POLL_DELAY_US 5
#define SPINAND_RESET_POLL_DELAY_US 5
#define SPINAND_WRITE_POLL_DELAY_US 15
#define SPINAND_ERASE_POLL_DELAY_US 50

So we will only delay here for reads and resets.

I would generally expect drivers to arrange for spi_mem_read_status()
to sleep using wait_for_completion*() where possible, so we shouldn't
normally be actually spinning here.

For example, the generic driver:

spi_mem_read_status()
-> spi_mem_exec_op()
  -> spi_sync()
    -> __spi_sync()
      -> __spi_transfer_message_noqueue()
        -> __spi_pump_transfer_message()
          -> spi_transfer_one_message() [via ctlr->transfer_one_message]
            -> spi_transfer_wait()
              -> wait_for_completion_timeout()

For spi-mt65xx:

spi_mem_read_status()
-> spi_mem_exec_op()
  -> mtk_spi_mem_exec_op() [via ctlr->mem_ops->exec_op]
    -> wait_for_completion_timeout()

Peter
Re: [PATCH] spi: use udelay() for short polling delays
Posted by David Laight 6 days, 20 hours ago 
On Sun, 17 May 2026 16:15:01 -0700
Peter Collingbourne <peter@pcc.me.uk> wrote:

> On Sun, May 17, 2026 at 7:02 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Sat, 16 May 2026 23:25:54 -0700
> > Peter Collingbourne <peter@pcc.me.uk> wrote:
> >  
> > > A short polling delay, such as the delay of 5us
> > > (SPINAND_READ_POLL_DELAY_US) provided by the SPI NAND driver,
> > > can become a 1/HZ (order of ms) delay caused by the usleep_range()
> > > call in read_poll_timeout(), significantly reducing SPI NAND access
> > > performance. Fix it by implementing the polling delay with udelay()
> > > (via read_poll_timeout_atomic()) if it is short enough, matching how
> > > the initial delay is handled.
> > >
> > > Fixes: c955a0cc8a28 ("spi: spi-mem: add automatic poll status functions")
> > > Signed-off-by: Peter Collingbourne <peter@pcc.me.uk>
> > > ---
> > >  drivers/spi/spi-mem.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > > index a09371a075d2..914e35e51cea 100644
> > > --- a/drivers/spi/spi-mem.c
> > > +++ b/drivers/spi/spi-mem.c
> > > @@ -1005,10 +1005,19 @@ int spi_mem_poll_status(struct spi_mem *mem,
> > >                       usleep_range((initial_delay_us >> 2) + 1,
> > >                                    initial_delay_us);
> > >
> > > -             ret = read_poll_timeout(spi_mem_read_status, read_status_ret,
> > > -                                     (read_status_ret || ((status) & mask) == match),
> > > -                                     polling_delay_us, timeout_ms * 1000, false, mem,
> > > -                                     op, &status);
> > > +             if (polling_delay_us < 10)
> > > +                     ret = read_poll_timeout_atomic(
> > > +                             spi_mem_read_status, read_status_ret,
> > > +                             (read_status_ret || ((status)&mask) == match),
> > > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > > +                             op, &status);
> > > +             else
> > > +                     ret = read_poll_timeout(
> > > +                             spi_mem_read_status, read_status_ret,
> > > +                             (read_status_ret || ((status)&mask) == match),
> > > +                             polling_delay_us, timeout_ms * 1000, false, mem,
> > > +                             op, &status);
> > > +  
> >
> > That looks rather sub-optional.
> > Even if the interval is short you want to drop to sleeps if the device isn't
> > responding.
> > (Or this code is used for erases and you are doing a full device erase.)
> >
> > I doubt you want to spin for more than 1ms.
> >
> > -David  
> 
> In include/linux/mtd/spinand.h we have:
> 
> #define SPINAND_READ_POLL_DELAY_US 5
> #define SPINAND_RESET_POLL_DELAY_US 5
> #define SPINAND_WRITE_POLL_DELAY_US 15
> #define SPINAND_ERASE_POLL_DELAY_US 50
> 
> So we will only delay here for reads and resets.

The timeout_ms values are equally relevant.
If anything goes wrong you don't want to be spinning for a long time
(and the timeout in the polling/atomic case is likely to be significantly
longer than you might expect - it only counts time in delay_us()).

So sort of want something like the following - but it is too hacky!
	int spins = polling_delay_us < 10 ? 10 : 0;
#define usleep_range(min, max) (--spins < 0 ? usleep_range(min, max) : udelay(max))
	ret = read_poll_timeout(....);
#undef usleep_range

-- David

> 
> I would generally expect drivers to arrange for spi_mem_read_status()
> to sleep using wait_for_completion*() where possible, so we shouldn't
> normally be actually spinning here.
> 
> For example, the generic driver:
> 
> spi_mem_read_status()
> -> spi_mem_exec_op()
>   -> spi_sync()
>     -> __spi_sync()
>       -> __spi_transfer_message_noqueue()
>         -> __spi_pump_transfer_message()
>           -> spi_transfer_one_message() [via ctlr->transfer_one_message]
>             -> spi_transfer_wait()
>               -> wait_for_completion_timeout()  
> 
> For spi-mt65xx:
> 
> spi_mem_read_status()
> -> spi_mem_exec_op()
>   -> mtk_spi_mem_exec_op() [via ctlr->mem_ops->exec_op]
>     -> wait_for_completion_timeout()  
> 
> Peter
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.