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

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

On 6/11/26 13:36, Lars Pöschel wrote:
> On 6/11/26 10:52 Michal Simek wrote:
>> +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
> 
> Ok, I will change that.
> 
>>> 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/
> 
> Ok.
> 
>>> 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.
> 
> Ok, I will add one.
> 
>>> 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.
> 
> I think it will work with all HW configurations, even with no FIFO configured.
> You are right, TFOR does not exist in non-FIFO configurations, but then you will 
> take the
> 
>      if (n_words ==1)
>          return 1;
> 
> path and exit before reading this register. And then according to PG153, Table 
> 2-6, Tx_Full bit:
> "Note: When FIFOs do not exist, this bit is set High when an AXI write to the 
> transmit register has been made (this option is available only in standard SPI 
> mode). This bit is cleared when the SPI transfer is completed."
> So n_words should be 1 in this case and I think this should still work.
> 
>> 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;
> 
> If you still think, this is more clear or better readable, then I can change it 
> like this. Let me know!

I understand that without FIFO you should never reach that code but I still 
think it is better not to deal with it. Pretty much if you want to wire this 
register I think dt binding should be extended to cover this optional feature.
That's why I think it is better not to introduce it.

Thanks,
Michal