Re: [PATCH 04/32] spi: dw: Introduce polling device tree property

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On Tue, 2020-11-17 at 21:26 +0300, Serge Semin wrote:
[...]
> > Found the bug :)
> > The fix is:
> > 
> > diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> > index e3b76e40ed73..b7538093c7ef 100644
> > --- a/drivers/spi/spi-dw-core.c
> > +++ b/drivers/spi/spi-dw-core.c
> > @@ -828,7 +828,7 @@ static void spi_hw_init(struct device *dev, struct dw_spi
> > *dws)
> >                 }
> >                 dw_writel(dws, DW_SPI_TXFTLR, 0);
> >  
> > 
> > 
> > 
> > -               dws->fifo_len = (fifo == 1) ? 0 : fifo;
> > +               dws->fifo_len = (fifo == 1) ? 0 : fifo - 1;
> >                 dev_dbg(dev, "Detected FIFO size: %u bytes\n", dws->fifo_len);
> >         }
> > 
> > Basically, fifo_len is off by one, one too large and that causes the RX FIFO
> > overflow error. The loop above breaks when the written fifo value does not
> > match the read one, which means that the last correct one is at step fifo - 1.
> > 
> > I realized that by tracing the transfers RX first, then TX in
> > dw_spi_transfer_handler().I did not see anything wrong with tx_max() and
> > rx_max(), all the numbers always added up correctly either up to transfer len
> > or to fifo_len, as they should. It looked all good. But then I realized that RX
> > FIFO errors would trigger 100% of the time for:
> > 1) transfers larger than fifo size (32 in my case)
> > 2) FIFO slots used for TX + RX adding up to 32
> > After some tweaking, found the above. Since that bug should be affecting all
> > dw-apb spi devices, not sure why it does not manifest itself more often.
> > 
> > With the above fix, the SD card is now running flawlessly at 1.2MB/s with
> > maximum SPI frequency, zero errors no matter how hard I hit it with traffic.
> 
> Hm, what you've found is the clue to getting where the problem lies,
> but I don't see fifo_len being calculated incorrectly in my HW. In my
> case it equals to 64 and 8 bytes for two different controllers. Those
> are the correct SSI_TX_FIFO_DEPTH/SSI_RX_FIFO_DEPTH parameters values
> our controllers have been synthesized with.
> 
> But I've just realized that DW APB SSI controller can be synthesized
> with Rx and Tx FIFO having different depths. (Synopsys, really, what
> scenario did you imagine to provide such configuration?..). Anyway is
> it possible that you've got a controller which (most likely by
> mistake) has been synthesized with Rx_fifo_len != Tx_fifo_len? If so
> then that will explain the problem you are having, but everyone else
> isn't. The algorithm thinks that both FIFOs length match and equals to
> the Tx FIFO length. If Rx FIFO length is smaller even with a single
> byte, you'll end up with occasional overflows.
> 
> Note if you don't have a manual with the parameters selected for your
> IP-core, you can just fix the fifo_len detection loop by replacing the
> TXFTLR with RXFTLR. Then just print the detected Rx and Tx FIFO
> lengths out. If they don't match, then, bingo, that's the root cause
> of the problem.

Just checked: TX and RX fifo depth match and the maximum size I can see in both
RXFTLR and TXFTLR is 31, when the fifo for loop stops with fifo=32. I checked
the Synopsis DW apb_ssi v4 specs and for both RXFTLR and TXFTLR, it says:

"If you attempt to set this value greater than the depth of the FIFO,
this field is not written and retains its current value."

So for a FIFO max depth of 32, as the K210 SoC documents (very poor spec sheet,
but that information is written), the loop should stop for fifo=33 with the
registers indicating 32. My fix should be correct.

However (I think this may be the catch), the spec also says:

"This register is sized to the number of address bits needed to access the
FIFO."

So for a fifo depth of 32, the register would be 5 bits only, preventing it
from ever indicating 32. I think that this spec clause takes precedence over
the previous one, and for a fifo max depth that is a power of 2 (which I assume
is the case on most synthesis of the device), the detection loop actually
works. But it would be buggy (off by one) for any other value of the fifo max
depth that is not a power of 2.

If the above is correct, and my SoC spec sheet is also correct, then all I can
think of now is a HW bug. Because no matter what I do or how I look at it, the
RX FIFO overflow always happen when the sum of TX bytes sent and RX bytes left
to receive equals exactly 32. Sending 32B on TX fifo does nto seem to cause any
problem, so the TX fifo seems to indeed be 32B deep. But on the RX side, it
really look like 31 is the value to use.

Here is a trace for a 64B transfer (errors happen only for transfers larger
than 32 B):

IRQ(1):
[    1.062978] spi_master spi1: ## RX: used 0/0, len 64 -> rx 0
[    1.068533] spi_master spi1: ## TX: used 0/0, room 32, len 64, gap 32 -> tx
32

IRQ(2):
[    1.076052] spi_master spi1: ## RX: used 16/15, len 64 -> rx 15
[    1.081647] spi_master spi1: ## TX: used 0/17, room 32, len 32, gap 15 -> tx
15

IRQ(3):
[    1.088932] spi_master spi1: RX FIFO overflow detected
[    1.094053] spi_master spi1: ## TX/RX used 0/32, len 17/49

Each pair of line correspond to one execution of dw_spi_transfer_handler() on
an IRQ occurrence. The first line is what rx_max() sees when dw_reader() is
called, the second line is what tx_max() sees when dw_writer() is executed. The
"used x/y" part of the messages shows TXFLR/RXFLR values.

(1) After setup of the transfer and enabling the controller, IRQ(1) occurs,
nothing to receive yet, TX fifo all empty, 32 B are written. All expected. OK.
(2) More than fifo_len / 2 - 1 (as RXFTLR is set in dw_spi_irq_setup) become
available and IRQ(2) trigger, 15 Bytes are received. When dw_writer() runs, it
sees the rxtxgap at 15B (17B still to receive from the previous 32B written),
so writes only 15B. All good, again expected. Note that when dw_writer() runs,
the remaining 17B to be received are already available, but that is likely due
to the delay from the pr_info() message print.
(3) Next IRQ(3) is the error, showing that all TX bytes have been processed and
that the RX fifo is full with 32B.

If the RX fifo max depth is indeed 32, I do not understand why the overflow
error is triggered at step (3). There are no more than 32B that can possibly be
received. Putting back the "drive MOSI lne high" patch does not change
anything. Same behavior.

I am out of ideas at this point and can only think that I am facing a HW bug
that needs a quirk somewhere.

Thoughts ? Do you think it is OK to add a quirk flag for this SoC to have
fifo_len reduced by one ? Adding a DT property to manually force a value for
fifo_len would work too.

-- 
Damien Le Moal
Western Digital