Re: [PATCH] spi: xilinx: use FIFO occupancy register to determine buffer size

[Michal Simek <michal.simek@amd.com>]

+Amit,

On 6/11/26 09:10, lars.poeschel.linux@edag.com wrote:
> From: Lars Pöschel <lars.poeschel@edag.com>
> 
> The method the driver uses to determine the size of the FIFO has a
> problem. What it currently does is this:
> It stops the SPI hardware and writes to the TX FIFO register until TX
> FIFO FULL asserts in the status register. But the hardware does not only
> have the FIFO, it also seems to have a shift register. This can be seen,

I don't think you should guess here


> when writing a byte to the FIFO (while the SPI hardware is stopped,) the
> TX FIFO EMPTY is still empty. So, if we have a FIFO size of 16 for
> example, the current method returns a 17.




> This is a problem, at least when using the driver in irq mode. The same
> size determined for the TX FIFO is also assumed for the RX FIFO. When an
> SPI transaction wants to write the amount of the FIFO size or more
> bytes, the following happens, let's assume 16 bytes FIFO size:

s/let's/for example/


> The driver stops the SPI hardware and writes 17 bytes to the TX FIFO and
> starts the SPI hardware and goes sleep.
> The hardware then shifts out 17 bytes (FIFO + shift register) and
> simultaneously reads bytes into the RX FIFO, but it only has 16 places,
> so it looses one byte. Then TX FIFO empty asserts, wakes the driver
> again, which has a fast path and reads 16 bytes from the RX FIFO, but
> before reading the last 17th byte (which is lost) it does this:
> 
> 	sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
> 	if (!(sr & XSPI_SR_RX_EMPTY_MASK)) {
> 		xilinx_spi_rx(xspi);
> 		rx_words--;
> 	}
> 
> It reads the status register and checks if the RX FIFO is not empty.
> But it is empty in our case. So this check spins in a while loop
> forever locking the driver.
> 
> The new method for determining the FIFO size is to read the SPITFOR (TX
> FIFO occupancy register, value = occupancy - 1) after filling the TX
> FIFO to obtain the true FIFO storage depth, which also equals the RX
> FIFO depth. In non-FIFO configurations (n_words == 1) the register does
> not exist; return 1 directly in that case.
> 

missing fix tag.

> Signed-off-by: Lars Pöschel <lars.poeschel@edag.com>
> ---
>   drivers/spi/spi-xilinx.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
> index 9f065d4e27d1..2d31e30fc4eb 100644
> --- a/drivers/spi/spi-xilinx.c
> +++ b/drivers/spi/spi-xilinx.c
> @@ -54,6 +54,7 @@
>   #define XSPI_RXD_OFFSET		0x6c	/* Data Receive Register */
>   
>   #define XSPI_SSR_OFFSET		0x70	/* 32-bit Slave Select Register */
> +#define XSPI_TFOR_OFFSET	0x74	/* Transmit FIFO Occupancy Register */
>   
>   /* Register definitions as per "OPB IPIF (v3.01c) Product Specification", DS414
>    * IPIF registers are 32 bit
> @@ -377,7 +378,9 @@ static int xilinx_spi_find_buffer_size(struct xilinx_spi *xspi)
>   		n_words++;
>   	} while (!(sr & XSPI_SR_TX_FULL_MASK));
>   
> -	return n_words;
> +	if (n_words == 1)
> +		return 1;
> +	return xspi->read_fn(xspi->regs + XSPI_TFOR_OFFSET) + 1;

Based on pg153
Exists only when FIFO Depth is set to 16 or 256

Based on ds570
This register does not exist if C_FIFO_EXIST = 0

It means this is not going to work with all HW configurations.

Pretty much you wanted to use this register to find out how big is the fifo
but I think in this case you should be just fixing logic around +-1 caused by 
n_words++ in a loop.

What about to change logic to be like this? It is clear and it is without any +1 
logic and no fifo is handled separately?

         while (1) { 
  

                 xspi->write_fn(0, xspi->regs + XSPI_TXD_OFFSET); 
  

                 sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET); 
  

                 if (sr & XSPI_SR_TX_FULL_MASK) 
  

                         break; 
  

                 n_words++; 
  

         } 
  

  
  

         /* Handle NO FIFO case separately */ 
  

         if (!n_words) 
  

                 return 1; 
  

  
  

         return n_words;

Thanks,
Michal