[05/32] spi: dw: Introduce DW_SPI_CAP_POLL_NODELAY

Message ID 20201107081420.60325-6-damien.lemoal@wdc.com
State New
Headers show
Series
  • [01/32] of: Fix property supplier parsing
Related show

Commit Message

Damien Le Moal Nov. 7, 2020, 8:13 a.m.
On slow systems, i.e. systems with a slow CPU resulting in slow context
switches, calling spi_delay_exec() when executing polled transfers
using dw_spi_poll_transfer() can lead to RX FIFO overflows. Allow
platforms to opt out of delayed polling by introducing the
DW_SPI_CAP_POLL_NODELAY DW SPI capability flag to disable
the execution of spi_delay_exec() in dw_spi_poll_transfer().

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/spi/spi-dw-core.c | 12 ++++++++----
 drivers/spi/spi-dw.h      |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Mark Brown Nov. 9, 2020, 2:03 p.m. | #1
On Sat, Nov 07, 2020 at 05:13:53PM +0900, Damien Le Moal wrote:
> On slow systems, i.e. systems with a slow CPU resulting in slow context
> switches, calling spi_delay_exec() when executing polled transfers
> using dw_spi_poll_transfer() can lead to RX FIFO overflows. Allow
> platforms to opt out of delayed polling by introducing the
> DW_SPI_CAP_POLL_NODELAY DW SPI capability flag to disable
> the execution of spi_delay_exec() in dw_spi_poll_transfer().

This feels like it should be done based on something like a cutoff for
sufficiently small delays rather than with a capability - it's tuning,
not a property of the system.
Serge Semin Nov. 9, 2020, 8:45 p.m. | #2
On Sat, Nov 07, 2020 at 05:13:53PM +0900, Damien Le Moal wrote:
> On slow systems, i.e. systems with a slow CPU resulting in slow context
> switches, calling spi_delay_exec() when executing polled transfers
> using dw_spi_poll_transfer() can lead to RX FIFO overflows. Allow
> platforms to opt out of delayed polling by introducing the
> DW_SPI_CAP_POLL_NODELAY DW SPI capability flag to disable
> the execution of spi_delay_exec() in dw_spi_poll_transfer().

Please, have a more thorough problem review. Rx FIFO shouldn't
overflow even for the CPUs with slow context switch. If it does, then
most likely there is a bug in the code. So it's not a good idea to
work it around by introducing a dts-property in any case.

Just to note in case of our hardware no matter what CPU frequency I
specified (it can be varied from 200MHz to 1500MHz), there have never
been seen the Rx FIFO overflow error even with heavy background tasks
executed. I am really puzzled why some context switch causes the error
in your case...

As I said in another thread, some logical MMC-SPI drivers errors may
happen if the native CS is utilized...

On the other hand, if you have a DMA-engine utilized together with
your controller and the Tx DMA-channel is tuned to work faster than
the Rx DMA-channel, then the Rx FIFO overflow will eventually happen.
So replacing the DMA-based transfers with the poll- or IRQ-based ones
shall indeed solve the problem. But the better solution would be to
balance the DMA-channels priority if your DMA-controller is capable of
doing that.

-Sergey

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/spi/spi-dw-core.c | 12 ++++++++----
>  drivers/spi/spi-dw.h      |  1 +
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index c2ef1d8d46d5..16a6fd569145 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -385,14 +385,18 @@ static int dw_spi_poll_transfer(struct dw_spi *dws,
>  	u16 nbits;
>  	int ret;
>  
> -	delay.unit = SPI_DELAY_UNIT_SCK;
> -	nbits = dws->n_bytes * BITS_PER_BYTE;
> +	if (!(dws->caps & DW_SPI_CAP_POLL_NODELAY)) {
> +		delay.unit = SPI_DELAY_UNIT_SCK;
> +		nbits = dws->n_bytes * BITS_PER_BYTE;
> +	}
>  
>  	do {
>  		dw_writer(dws);
>  
> -		delay.value = nbits * (dws->rx_len - dws->tx_len);
> -		spi_delay_exec(&delay, transfer);
> +		if (!(dws->caps & DW_SPI_CAP_POLL_NODELAY)) {
> +			delay.value = nbits * (dws->rx_len - dws->tx_len);
> +			spi_delay_exec(&delay, transfer);
> +		}
>  
>  		dw_reader(dws);
>  
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index 48a11a51a407..25f6372b993a 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -130,6 +130,7 @@ enum dw_ssi_type {
>  #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
>  #define DW_SPI_CAP_DWC_SSI		BIT(2)
>  #define DW_SPI_CAP_DFS_32		BIT(3)
> +#define DW_SPI_CAP_POLL_NODELAY		BIT(4)
>  
>  /* Slave spi_transfer/spi_mem_op related */
>  struct dw_spi_cfg {
> -- 
> 2.28.0
>

Patch

diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
index c2ef1d8d46d5..16a6fd569145 100644
--- a/drivers/spi/spi-dw-core.c
+++ b/drivers/spi/spi-dw-core.c
@@ -385,14 +385,18 @@  static int dw_spi_poll_transfer(struct dw_spi *dws,
 	u16 nbits;
 	int ret;
 
-	delay.unit = SPI_DELAY_UNIT_SCK;
-	nbits = dws->n_bytes * BITS_PER_BYTE;
+	if (!(dws->caps & DW_SPI_CAP_POLL_NODELAY)) {
+		delay.unit = SPI_DELAY_UNIT_SCK;
+		nbits = dws->n_bytes * BITS_PER_BYTE;
+	}
 
 	do {
 		dw_writer(dws);
 
-		delay.value = nbits * (dws->rx_len - dws->tx_len);
-		spi_delay_exec(&delay, transfer);
+		if (!(dws->caps & DW_SPI_CAP_POLL_NODELAY)) {
+			delay.value = nbits * (dws->rx_len - dws->tx_len);
+			spi_delay_exec(&delay, transfer);
+		}
 
 		dw_reader(dws);
 
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 48a11a51a407..25f6372b993a 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -130,6 +130,7 @@  enum dw_ssi_type {
 #define DW_SPI_CAP_KEEMBAY_MST		BIT(1)
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS_32		BIT(3)
+#define DW_SPI_CAP_POLL_NODELAY		BIT(4)
 
 /* Slave spi_transfer/spi_mem_op related */
 struct dw_spi_cfg {