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

[Serge Semin]

On Wed, Nov 18, 2020 at 04:41:27AM +0000, Damien Le Moal wrote:
> 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.

v3.22a spec says "greater than or equal to the depth of the FIFO" for
TXFTLR and "greater than the depth of the FIFO" - for RXFTLR. In my
case both of the formulations make sense. Seeing the semantic of the
registers is different and recollecting how vague vendors can describe
the essence of regs, in both cases FIFO depth would be detected as
(MAX_value + 1). That is obvious for TXFTLR (in v3.22a formulation),
but isn't for RXFTLR. In the later case we need to keep in mind that
the controller internally increments (RXFTLR.RFT + 1) then compares it
with RXFLR.RXTFL. Most likely the RXFTLR description text means that
incremented value when 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." but not the actual value written into the register.

Regarding DW APB SSI v4 spec. I don't really know why the description
has changed for TXFTLR. I also don't have the v4 spec to better
understand what they really meant there. But if the TXFTLR/RXFTLR
registers semantic has changed, then I'd start to dig in the aspects
of that change and whether it affects the FIFO-depth calculation
algorithm...

Anyway regarding your fix. Please see the next two commits, which have
already attempted to introduce a change proposed by you, but then
reverted it back:
d297933cc7fc ("spi: dw: Fix detecting FIFO depth")
9d239d353c31 ("spi: dw: revisit FIFO size detection again")

I don't think Andy was wrong reverting the fix back, especially seeing
the current FIFO-depth detection algorithm implementation is correct
for both types of my controllers (with 8 and 64 words depths). I know
with what parameters the controllers have been synthesized and the
detected depths match them.

> 
> 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.

In accordance with the registers semantic we shall never write a
FIFO-depth value into them and actually we aren't able to do that.
First of all indeed there is no point in setting the FIFO-depth value
into the TXFTLR because that will cause the TXE-IRQ being constantly
generated (because the level of the Tx FIFO will be always less than
or equal to the FIFO-depth). There is no point in setting the
FIFO-depth into the RXFTLR because that will effectively disable the
RXF-IRQ (the level of the Rx FIFO will be never greater than
FIFO-depth + 1). Secondly the TXFTLR/RXFTLR registers width have been
defined as: TX_ABW-1:0 and RX_ABW-1:0 unlike the FIFO level registers
TXFLR/RXFLR: TX_ABW:0 and RX_ABW:0 . So we just literally can't set a
value wider than the register is.

> 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.

Of course we shouldn't reject a possibility of having a HW bug here,
but that is always a last resort and needs to be firmly proved.
In your case we may assume one of two causes of the problem:
1) There is the depths mismatch.
   But how come you still get the RXFLR to be filled with data of
   greater length than we think the Rx FIFO depth is?.. Anyway the
   weird behaviour still can be explained by the non-standard
   configuration especially if the depth isn't power of 2, that
   Synopsys might haven't even tested it, etc.
2) There is a HW bug, due to which the RXO interrupt is generated even
   though there is no actual overrun.
Let's try to prove or disprove them.

Let's assume that there is indeed the depths mismatch here:
(SSI_TX_FIFO_DEPTH = 32) != (SSI_RX_FIFO_DEPTH = 31), and for some
reason (due to for instance the TXFTLR/RXFTLR semantic change or
the HW bug or etc) we can't detect the Rx FIFO depth by the algorithm
in the driver. If so we can't rely on the depth detection loop and
need to try to prove the depths mismatch somehow else. It would be
also better to exclude the driver code from the problem equation...

Since we are sure the Rx FIFO depth must be smaller than the Tx FIFO
depth (otherwise you wouldn't be getting the Rx FIFO overflows), then
we can just try to manually execute a small SPI transfer of the Tx FIFO
depth length. If the mismatch takes place or an HW bug with false RXO,
then we'll get the Rx FIFO overflow flag set.

I've created a small shell-script (you can find it in the attachment)
which runs a single SPI transfer of the passed length by manually
setting a DW APB SSI controller up with help of the devmem utility.
In our case the length is supposed to be equal to the FIFO depth.
Here is what happens if I run it on my hardware (depth equals to 64
and the controller registers physical address is 0x1f04e000):

root at msbt2:~# ./check_mismatch.sh 64 0x1f04e000
1. Tx FIFO lvl 0, Rx FIFO lvl 0, RISR 0x00000000
1. Tx FIFO lvl 64, Rx FIFO lvl 0, RISR 0x00000000
2. Tx FIFO lvl 0, Rx FIFO lvl 64, RISR 0x00000011

See, after sending and receiving all the data the status register
detects the normal TXE and RXF interrupts. But if our assumptions are
correct in your case it shall be something like 0x00000019 also
presenting the RXO interrupt.

* Note AFAIU you might need to fix the script a bit to set the DFS
* field at the correct place of CTRLR0...

If after running the attached script you get the RXO status set, then
indeed there is either the depths mismatch or there is a HW bug. If
the TXFTLR/RXFTLR semantic hasn't changed in spec v4, then the
mismatch or a HW bug don't let us to detect the Rx FIFO depth by means
of the algorithm implemented in the driver. Then we have no choice but
to manually set the FIFO depth for the K210 SPI controller.

> 
> 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.

The log you've sent indeed looks weird. I'd one more time performed
the next steps:
1) Study the spec paying special attention to the TXFTLR/RXFTLR and
   TXFLR/RXFLR registers descriptions. Mostly we need to make sure
   that nothing has changed since the older controller revisions and
   what the driver implements is correct for your controller.
2) Try to find an internal doc with the DW APB SSI IP-core parameters
   initialized during the controller synthesize. Try to find the
   SSI_TX_FIFO_DEPTH/SSI_RX_FIFO_DEPTH parameters value.
3) Try to find an errata doc for your controller revision and make
   sure it doesn't have anything about the problem you are getting
   here.
4) Run the attached script to make sure that the problem isn't
   connected with a bug in the driver, but either with the depths
   mismatch or with an HW bug.

> 
> 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.

So if none of the steps above gives us an answer, then we need to dig
dipper into the driver code and we missing something. If 2) - 4) proves the
depths mismatch or a HW bug, then you can patch the spi-dw-mmio.c code to set
a custom fifo_len for the K210 SPI controller.

Anyway I don't think a quirk flag would be a good solution here because
the problem seems very specific to your controller...

-Sergey

> 
> -- 
> Damien Le Moal
> Western Digital
-------------- next part --------------
A non-text attachment was scrubbed...
Name: check_mismatch.sh
Type: application/x-sh
Size: 1735 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20201118/9ce78219/attachment.sh>