On Fri, May 22, 2020 at 08:03:25PM +0800, Feng Tang wrote:

On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote:

quoted

On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote:

quoted

Hi Serge,

On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote:

quoted

quoted

quoted

quoted

quoted

+	dw_spi_dma_wait_rx_done(dws);

I can understand the problem about TX, but I don't see how RX
will get hurt, can you elaborate more? thanks

- Feng

Your question is correct. You are right with your hypothesis. Ideally upon the
dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
commit log signifies the error being mostly related with Tx FIFO. But
practically there are many reasons why Rx FIFO might be left with data:
DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
messed something up), controller hanging up, and so on. It's better to catch
an error at this stage while propagating it up to the SPI device drivers.
Especially seeing the wait-check implementation doesn't gives us much of the
execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
we make sure that all the data has been fetched and we may freely get the
buffers back to the client driver.

I see your point about checking RX. But I still don't think checking
RX FIFO level is the right way to detect error. Some data left in
RX FIFO doesn't always mean a error, say for some case if there is
20 words in RX FIFO, and the driver starts a DMA request for 16
words, then after a sucessful DMA transaction, there are 4 words
left without any error.

Neither Tx nor Rx FIFO should be left with any data after transaction is
finished. If they are then something has been wrong.

See, every SPI transfer starts with FIFO clearance since we disable/enable the
SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is the
SSIENR register description: "It enables and disables all SPI Controller operations.
When disabled, all serial transfers are halted immediately. Transmit and receive
FIFO buffers are cleared when the device is disabled. It is impossible to program
some of the SPI Controller control registers when enabled"

No mater whether we start DMA request or perform the normal IRQ-based PIO, we
request as much data as we need and neither Tx nor Rx FIFO are supposed to
be left with any data after the request is finished. If data is left, then
either we didn't push all of the necessary data to the SPI bus, or we didn't
pull all the data from the FIFO, and this could have happened only due to some
component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
device driver should be notified about the problem.

Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The
former in dma case means the dma hw/driver has done its job, and spi hw/driver
hasn't done its job of pushing out the data to spi slave devices,

Agreed.

quoted

while the
latter means the spi hw/driver has done its job, while the dma hw/driver hasn't.

In this particular case agreed, that the data left in the Rx FIFO means DMA
hw/driver hasn't done its work right. Though SPI hw could be also a reason of
the data left in FIFO (though this only a theoretical consideration).

Right, that's why I was initially very curious about this RX FIFO thing,
and if possible, please give some details in commit log about the data
left in TX FIFO problem, which will help future developers when they
met simliar bugs.

Ok. I'll add a more descriptive patch log.

And I'm fine with adding the rx check, no matter the problem is in
dma side or spi side.

quoted

quoted

And the code is called inside the dma rx channel callback, which means the
dma driver is saying "hey, I've done my job", but apparently it hasn't if
there is data left.

Right, either it hasn't, or the DMA engine claimed it has, but still is doing
something (asynchronously or something, depending on the hardware implementation),
or it think it has, but in fact it hasn't due to whatever problem happened
(software/hardware/etc.). In anyway we have to at least check whether it's
really done with fetching data and to be on a safe side give it some time to
make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is
having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer()
means a certain error.

quoted

As for the wait time

+	nents = dw_readl(dws, DW_SPI_RXFLR);
+	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
+	     BITS_PER_BYTE;

Using this formula for checking TX makes sense, but it doesn't for RX.
Because the time of pushing data in TX FIFO to spi device depends on
the clk, but the time of transferring RX FIFO to memory is up to
the DMA controller and peripheral bus. 

On this I agree with you. That formulae doesn't describe exactly the time left
before the Rx FIFO gets empty. But at least it provides an upper limit on the
time needed for the peripheral bus to fetch the data from FIFO. If for some
reason the internal APB bus is slower than the SPI bus, then the hardware
engineers screwed, since the CPU/DMA won't keep up with pulling data from Rx
FIFO on time so the FIFO may get overflown. Though in this case CPU/DMA won't
be able to push data to the Tx FIFO fast enough to cause the Rx FIFO overflown,
so the problem might be unnoticeable until we enable the EEPROM-read or Rx-only
modes of the DW APB SSI controller. Anyway I am pretty much sure all the systems
have the internal bus much faster than the external SPI bus.

Getting back to the formulae. I was thinking of how to make it better and here
is what we can do. We can't predict neither the DMA controller performance,
nor the performance of its driver. In this case we have no choice but to add
some assumption to clarify the task. Let's assume that the reason why Rx FIFO is
non-empty is that even though we are at the DMA completion callback, but the
DMA controller is still fetching data in background (any other reason might be
related with a bug, so we'll detect it here anyway). In this case we need to
give it a time to finish its work. As far as I can see the DW_apb_ssi interface
doesn't use PREADY APB signal, which means the IO access cycle will take 4
reference clock periods for each read and write accesses. Thus taking all of
these into account we can create the next formulae to measure the time needed to
read all the data from the Rx FIFO:

-	ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
-	     BITS_PER_BYTE;
+	ns = (NSEC_PER_SEC / dws->max_freq) * nents * 4;

By doing several busy-wait loop iteration we'll cover the DMA controller and
it's driver possible latency. 

Feng, does it now makes sense for you now? If so, I'll replace the delay
calculation formulae in the patch.

Frankly I don't have a good idea, if it really happens which means
something is abnormal, explicitly waiting for some micro-seconds may
also be acceptable?

Well, If we can estimate the real delay, then it will be more preferred solution.
Hard-coding a single number is only an option if there isn't any other way.

quoted

quoted

Also for the

+	while (dw_spi_dma_rx_busy(dws) && retry--)
+		ndelay(ns);
+

the rx busy bit is cleared after this rx/tx checking, and it should
be always true at this point. Am I mis-reading the code?

Sorry I don't get your logic here. I am not checking the Rx busy bit here,
but the Rx FIFO non-empty bit. Also SR register bits aren't cleared on read,
so the status bits are left pending until the reason is cleared. In our case
until Rx FIFO gets empty, which will happen eventually either at the point of
all data finally being extracted from it or when the controller is disabled
by means of the SSIENR register.

I did misread the code, I thought it is checking the busy bits, sorry
for that. Though the dw_spi_dma_rx_busy() name is a little confusing,
as checking the emptiness of RX FIFO is not dma bound.

dw_spi_dma_* is a common prefix for all methods implemented in this module.
As I said having the Rx FIFO non-empty could mean that DMA in fact busy reading
data from the Rx FIFO or a bug or etc. Also the naming correlates with the
dw_spi_dma_tx_busy() method, which also doesn't mean DMA is busy with doing
something, but SPI Tx engine is busy with pushing data out to the SPI bus.

-Sergey

Thanks,
Feng