* Re: [PATCH] spi: xilinx: use FIFO occupancy register to determine buffer size
[not found] ` <e6af23c7-1ba9-4acc-af16-394fde75b4da@edag.com>
@ 2026-06-11 11:46 ` Michal Simek
0 siblings, 0 replies; 2+ messages in thread
From: Michal Simek @ 2026-06-11 11:46 UTC (permalink / raw)
To: Lars Pöschel, Mark Brown, linux-spi, linux-arm-kernel,
linux-kernel
Cc: Mahapatra, Amit Kumar, lars.poeschel
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
^ permalink raw reply [flat|nested] 2+ messages in thread