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

[Feng Tang <feng.tang@intel.com>]

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.

Thanks,
Feng

> 
> -Sergey