RE: [PATCH v9 RESEND 01/13] spi: imx: add dma_sync_sg_for_device after fallback from dma

[Robin Gong]

On 2020/06/08 23:32 Mark Brown <broonie@xxxxxxxxxx> wrote: 
> On Mon, Jun 08, 2020 at 03:08:45PM +0000, Robin Gong wrote:
> 
> > > > +	if (transfer->rx_sg.sgl) {
> > > > +		struct device *rx_dev = spi->controller->dma_rx->device->dev;
> > > > +
> > > > +		dma_sync_sg_for_device(rx_dev, transfer->rx_sg.sgl,
> > > > +				       transfer->rx_sg.nents, DMA_TO_DEVICE);
> > > > +	}
> > > > +
> 
> > > This is confusing - why are we DMA mapping to the device after doing
> > > a PIO transfer?
> 
> > 'transfer->rx_sg.sgl' condition check that's the case fallback PIO
> > after DMA transfer failed. But the spi core still think the buffer
> > should be in 'device' while spi driver touch it by PIO(CPU), so sync it back to
> device to ensure all received data flush to DDR.
> 
> So we sync it back to the device so that we can then do another sync to CPU?
Yes, spi.c will sync to CPU again in spi_unmap_buf() after transfer done finally.
Otherwise, the fresh received data by CPU in this fallback case may be invalidated
by spi.c, which led to the data corrupt on Matthias's side.

> TBH I'm a bit surprised that there's a requirement that we explicitly undo a
> sync and that a redundant double sync in the same direction might be an issue
Considering DMA transfer may be failed(for example, sdma firmware may not be
updated as ERR009165 depends on), we'd better fallback to PIO to ensure no any
function break here. Thus should clean fresh rx data from cache into external memory
as real 'device' received by DMA. Understood a bit confusing here, but that way could
be avoided by any code changing in spi.c. Or make some code changes in spi.c to cancel
spi_unmap_buf() in such fallback case?

> but I've not had a need to care so I'm perfectly prepared to believe there is.
> 
> At the very least this needs a comment.
Okay, I'll add comment here in next.