Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA

[Serge Semin]

On Thu, May 21, 2020 at 10:55:20PM +0800, Feng Tang wrote:
> Hi Serge,
> 
> On Thu, May 21, 2020 at 02:47:36PM +0300, Serge Semin wrote:
> > Hello Feng,
> > 
> > On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> > > Hi Serge,
> > > 
> > > On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> > 
> > [nip]
> > 
> > > >  /*
> > > >   * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> > > >   * channel will clear a corresponding bit.
> > > > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > > >  {
> > > >  	struct dw_spi *dws = arg;
> > > >  
> > > > +	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.

-Sergey

> 
> Thanks,
> Feng
> 
> > 
> > -Sergey