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

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

On 6/12/26 12:52, 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 has a shift register which can hold a byte. This
> can be seen, 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 a
> SPI transaction wants to write the amount of the FIFO size or more
> bytes, the following happens, for example with 16 bytes FIFO size:
> 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.
> 
> This patch fixes the logic to determine the FIFO size.
> 
> Fixes: 4c9a761402d7 ("spi/xilinx: Simplify spi_fill_tx_fifo")
> Signed-off-by: Lars Pöschel <lars.poeschel@edag.com>
> ---
>   drivers/spi/spi-xilinx.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
> index 9f065d4e27d1..b95485710e2f 100644
> --- a/drivers/spi/spi-xilinx.c
> +++ b/drivers/spi/spi-xilinx.c
> @@ -371,11 +371,18 @@ static int xilinx_spi_find_buffer_size(struct xilinx_spi *xspi)
>   		xspi->regs + XIPIF_V123B_RESETR_OFFSET);
>   
>   	/* Fill the Tx FIFO with as many words as possible */
> -	do {
> +	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++;
> -	} while (!(sr & XSPI_SR_TX_FULL_MASK));
> +	}
> +
> +	/* Handle the NO FIFO case separately */
> +	if (!n_words)
> +		return 1;
>   
>   	return n_words;
>   }

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal