[PATCH] spi: atmel: poll short PIO transfers

[PATCH] spi: atmel: poll short PIO transfers

[Jens Emil Schulz Østergaard]

Every PIO transfer enables the controller interrupt and sleeps in
wait_for_completion_timeout() until the interrupt handler has drained the
receive data. For a transfer of a few bytes, the interrupt, the
completion and the two context switches take several times longer than
the transfer on the wire. Devices that do their register IO over SPI,
such as SPI attached Ethernet switches, issue one message of a few short
transfers per register access, so this overhead dominates their register
access time.

Busy-wait on the status register instead of enabling the interrupt when
the transfer is estimated to take less than polling_limit_us, 30 us by
default, as spi-imx does. The estimate uses the effective clock rate and
the word delay. Setting the module parameter to 0 disables polling.

Mask the receive and overrun interrupts before a polled transfer is
started. An earlier interrupt driven transfer can leave them enabled:
the overrun interrupt always stays enabled, and the receive interrupt
also stays enabled after an overrun or a timeout. The interrupt handler
would then consume the status the poll waits for. Like the interrupt
handler, the poll only reads received data while bytes are outstanding.

Signed-off-by: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
---
 drivers/spi/spi-atmel.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index ffc1292b08c1..25a0a3acfc4e 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -234,6 +234,11 @@
  */
 #define DMA_MIN_BYTES	16
 
+static unsigned int polling_limit_us = 30;
+module_param(polling_limit_us, uint, 0664);
+MODULE_PARM_DESC(polling_limit_us,
+		 "maximum time in us to run a transfer in polling mode");
+
 #define AUTOSUSPEND_TIMEOUT	2000
 
 struct atmel_spi_caps {
@@ -274,6 +279,8 @@ struct atmel_spi {
 
 	bool			use_dma;
 	bool			use_pdc;
+	bool			poll;
+	unsigned int		poll_limit_us;
 
 	bool			keep_cs;
 
@@ -688,6 +695,9 @@ static void atmel_spi_next_xfer_single(struct spi_controller *host,
 
 	dev_vdbg(host->dev.parent, "atmel_spi_next_xfer_pio\n");
 
+	if (as->poll)
+		spi_writel(as, IDR, SPI_BIT(RDRF) | SPI_BIT(OVRES));
+
 	/* Make sure data is not remaining in RDR */
 	spi_readl(as, RDR);
 	while (spi_readl(as, SR) & SPI_BIT(RDRF)) {
@@ -706,7 +716,8 @@ static void atmel_spi_next_xfer_single(struct spi_controller *host,
 		xfer->bits_per_word);
 
 	/* Enable relevant interrupts */
-	spi_writel(as, IER, SPI_BIT(RDRF) | SPI_BIT(OVRES));
+	if (!as->poll)
+		spi_writel(as, IER, SPI_BIT(RDRF) | SPI_BIT(OVRES));
 }
 
 /*
@@ -725,6 +736,9 @@ static void atmel_spi_next_xfer_fifo(struct spi_controller *host,
 
 	dev_vdbg(host->dev.parent, "atmel_spi_next_xfer_fifo\n");
 
+	if (as->poll)
+		spi_writel(as, IDR, SPI_BIT(RXFTHF) | SPI_BIT(OVRES));
+
 	/* Compute the number of data to transfer in the current iteration */
 	current_remaining_data = ((xfer->bits_per_word > 8) ?
 				  ((u32)as->current_remaining_bytes >> 1) :
@@ -776,7 +790,8 @@ static void atmel_spi_next_xfer_fifo(struct spi_controller *host,
 	 * Enable RX FIFO Threshold Flag interrupt to be notified about
 	 * transfer completion.
 	 */
-	spi_writel(as, IER, SPI_BIT(RXFTHF) | SPI_BIT(OVRES));
+	if (!as->poll)
+		spi_writel(as, IER, SPI_BIT(RXFTHF) | SPI_BIT(OVRES));
 }
 
 /*
@@ -1375,6 +1390,58 @@ static void atmel_spi_set_cs(struct spi_device *spi, bool enable)
 
 }
 
+static bool atmel_spi_use_poll(struct atmel_spi *as,
+			       struct spi_transfer *xfer)
+{
+	u32 hz = xfer->effective_speed_hz ?: xfer->speed_hz;
+	unsigned int limit = as->poll_limit_us;
+	int word_delay_ns;
+	u32 words;
+	u64 us;
+
+	if (!limit || as->use_pdc || atmel_spi_use_dma(as, xfer) || !hz)
+		return false;
+
+	us = div_u64((u64)xfer->len * BITS_PER_BYTE * USEC_PER_SEC, hz);
+
+	word_delay_ns = spi_delay_to_ns(&xfer->word_delay, xfer);
+	if (word_delay_ns > 0) {
+		words = xfer->len / DIV_ROUND_UP(xfer->bits_per_word,
+						 BITS_PER_BYTE);
+		us += div_u64((u64)words * word_delay_ns, NSEC_PER_USEC);
+	}
+
+	return us < limit;
+}
+
+static int atmel_spi_poll_xfer(struct atmel_spi *as,
+			       struct spi_transfer *xfer)
+{
+	u32 done = as->fifo_size ? SPI_BIT(RXFTHF) : SPI_BIT(RDRF);
+	u32 status;
+	int ret;
+
+	ret = readl_poll_timeout(as->regs + SPI_SR, status,
+				 status & (done | SPI_BIT(OVRES)),
+				 0, (u64)as->poll_limit_us * 10);
+	if (ret) {
+		dev_err(&as->pdev->dev, "spi transfer timeout\n");
+		return -EIO;
+	}
+
+	if (status & SPI_BIT(OVRES)) {
+		dev_warn(&as->pdev->dev, "overrun\n");
+		return -EIO;
+	}
+
+	atmel_spi_lock(as);
+	if (as->current_remaining_bytes)
+		atmel_spi_pump_pio_data(as, xfer);
+	atmel_spi_unlock(as);
+
+	return 0;
+}
+
 static int atmel_spi_one_transfer(struct spi_controller *host,
 					struct spi_device *spi,
 					struct spi_transfer *xfer)
@@ -1412,6 +1479,8 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 	as->done_status = 0;
 	as->current_transfer = xfer;
 	as->current_remaining_bytes = xfer->len;
+	as->poll_limit_us = READ_ONCE(polling_limit_us);
+	as->poll = atmel_spi_use_poll(as, xfer);
 	while (as->current_remaining_bytes) {
 		reinit_completion(&as->xfer_completion);
 
@@ -1437,6 +1506,13 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
 			atmel_spi_lock(as);
 			atmel_spi_next_xfer_pio(host, xfer);
 			atmel_spi_unlock(as);
+
+			if (as->poll) {
+				as->done_status = atmel_spi_poll_xfer(as, xfer);
+				if (as->done_status)
+					break;
+				continue;
+			}
 		}
 
 		dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));

---
base-commit: 2bc7eb398cd0f89fd2331d5a1a8fe7ed17addd35
change-id: 20261112-spi-atmel-poll-short-pio-transfers-bd8fcb9c19bb

Best regards,
-- 
Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

Re: [PATCH] spi: atmel: poll short PIO transfers

[Mark Brown]

> Every PIO transfer enables the controller interrupt and sleeps in
> wait_for_completion_timeout() until the interrupt handler has drained the
> receive data. For a transfer of a few bytes, the interrupt, the
> completion and the two context switches take several times longer than
> the transfer on the wire. Devices that do their register IO over SPI,
> such as SPI attached Ethernet switches, issue one message of a few short
> transfers per register access, so this overhead dominates their register
> access time.

> Busy-wait on the status register instead of enabling the interrupt when
> the transfer is estimated to take less than polling_limit_us, 30 us by
> default, as spi-imx does. The estimate uses the effective clock rate and

How big is the FIFO?

> the word delay. Setting the module parameter to 0 disables polling.

If this is dubious enough to require the module parameter we shouldn't
be doing this.  Most likely it's perfectly sensible and we should just
do it as standard, other drivers don't make this optional.  If we do
have a parameter it needs to be documented.

> @@ -725,6 +736,9 @@ static void atmel_spi_next_xfer_fifo(struct spi_controller *host,
>  
>  	dev_vdbg(host->dev.parent, "atmel_spi_next_xfer_fifo\n");
>  
> +	if (as->poll)
> +		spi_writel(as, IDR, SPI_BIT(RXFTHF) | SPI_BIT(OVRES));

We should ensure there's no interrupt handler running or that could run
in future, especially with PREEMPT_RT which puts everything into a
thread.  If we timed out a transfer then an interrupt could be
delivered while we're polling and try to also process things.  A lock
while updating the flag and checking the flag before doing anything in
the interrupt would probably do the trick.

-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] spi: atmel: poll short PIO transfers

[Jens Emil Schulz Østergaard]

On Thu Nov 12, 2026 at 9:12 PM CET, Mark Brown wrote:
>> Every PIO transfer enables the controller interrupt and sleeps in
>> wait_for_completion_timeout() until the interrupt handler has drained the
>> receive data. For a transfer of a few bytes, the interrupt, the
>> completion and the two context switches take several times longer than
>> the transfer on the wire. Devices that do their register IO over SPI,
>> such as SPI attached Ethernet switches, issue one message of a few short
>> transfers per register access, so this overhead dominates their register
>> access time.
>
>> Busy-wait on the status register instead of enabling the interrupt when
>> the transfer is estimated to take less than polling_limit_us, 30 us by
>> default, as spi-imx does. The estimate uses the effective clock rate and
>
> How big is the FIFO?
>

It depends on the SoC. The SPI in lan966x, lan969x and sama7 has a 32
data FIFO, sam9x60 and sam9x7 have 16, and sama5d2 has 16 or 32 depending
on the instance. The older parts (sam9x5, sama5d3, sama5d4 and earlier)
have none and take one interrupt per word.

>> the word delay. Setting the module parameter to 0 disables polling.
>
> If this is dubious enough to require the module parameter we shouldn't
> be doing this.  Most likely it's perfectly sensible and we should just
> do it as standard, other drivers don't make this optional.  If we do
> have a parameter it needs to be documented.
>

I will drop the parameter in the next version and poll below a fixed 30
us.

>> @@ -725,6 +736,9 @@ static void atmel_spi_next_xfer_fifo(struct spi_controller *host,
>>  
>>  	dev_vdbg(host->dev.parent, "atmel_spi_next_xfer_fifo\n");
>>  
>> +	if (as->poll)
>> +		spi_writel(as, IDR, SPI_BIT(RXFTHF) | SPI_BIT(OVRES));
>
> We should ensure there's no interrupt handler running or that could run
> in future, especially with PREEMPT_RT which puts everything into a
> thread.  If we timed out a transfer then an interrupt could be
> delivered while we're polling and try to also process things.  A lock
> while updating the flag and checking the flag before doing anything in
> the interrupt would probably do the trick.

Done as you suggest in v2. The poll flag is set and the interrupts are
masked in the same locked section before the transfer starts. The PIO
interrupt handler takes the lock before it reads the status register and
returns IRQ_NONE without touching the controller while the flag is set.

Thanks,
Emil