RE: [PATCH v1 RFC 1/2] spi: introduce fallback to pio

[Robin Gong <yibin.gong@xxxxxxx>]

On 2020/06/12 18:14 Mark Brown <broonie@xxxxxxxxxx> wrote: 
> On Fri, Jun 12, 2020 at 02:18:32AM +0000, Robin Gong wrote:
> > On 2020/06/11 21: 41 Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> Please look at the formatting of your e-mails - they're really hard to read.  The
> line length is over 80 columns and there's no breaks between paragraphs.
Sorry for that, seems my outlook format issue, hope it's ok now this time :)

> 
> > > If we were going to do this I don't see why we'd have a flag for
> > > this rather than just doing it unconditionally but...
> 
> > What do you mean flag here, 'master->flags' or SPI_MASTER_FALLBACK?
> 'master->flags'
> > could let client fallback to PIO finally and spi core clear this flag
> > once this transfer done, so that DMA could be tried again in the next transfer.
> Client could enable this feature by choosing SPI_MASTER_FALLBACK freely
> without any impact on others.
> 
> SPI_MASTER_FALLBACK.  If this works why would any driver not enable the
> flag?
Just make sure little impact if it's not good enough and potential issue may
come out after it's merged into mainline. TBH, I'm not sure if it has taken
care all in spi core. Besides, I don't know if other spi client need this feature.

> 
> > > ...I don't think this can work sensibly - this is going to try PIO
> > > if there's *any* error.  We might have had some sort of issue during
> > > the transfer for example so have some noise on the bus.  Like I said
> > > on a prior version of this I really
> 
> > Any error happen in DMA could fallback to PIO , seems a nice to have,
> because it could
> > give chance to run in PIO which is more reliable. But if there is also error in
> PIO, thus may loop here, it's better adding limit try times here?
> 
> An error doesn't mean nothing happened on the bus, an error could for
> example also be something like a FIFO overrun which corrupts data.
Do you mean fallback to PIO may cause FIFO overrun since some latency
involved so that this patch seems not useful as expected?

> 
> > > think that we need to be figuring out if the DMA controller can
> > > support the transaction before we even map the buffer for it, having
> > > the controller just randomly fail underneath the consumer just does not
> sound robust.
> 
> > But dmaengine_prep_slave_sg still may return failure even if anything
> > about DMA is ok before spi transfer start, such as dma description
> > malloc failure. This patch seems could make spi a bit robust...
> 
> It *could* but only in extreme situations, and again this isn't just handling
> errors from failure to prepare the hardware but also anything that happens
> after it.
Okay, understood your point. You prefer to some interface provided by dma
engine before dmaengine_prep_slave_sg so that can_dma() can know if
this dma channel is ready indeed. But unfortunately, seems there is no one....

dmaengine_slave_config is the best one I think probably do that, but the
'direction' is deprecated and Vinod may remove it in the future. That's
important for sdma since sdma need to know the load_address of script which
is running on this channel by checking 'direction', then, could know if
the script is ready or not(ram script not ready if sdma firmware not loaded, but
rom script should be ready always). But now, as it's 'deprecated', until
dmaengine_prep_* phase sdma could know what's the load_address of script
running on this dma channel and if it's ready or not(firmware loaded or not).
commit 107d06441b70 ("dmaengine: imx-sdma: remove dma_slave_config direction usage and leave sdma_event_enable() ")

Hi @Vinod Koul, sorry for pulling you, Could we keep 'direction' in dma_slave_config?
This's useful for checking if the script used on channel is ready or not in
dmaengine_slave_config phase so that can easily for spi or other drivers decide to
start dma rather than the last dmaengine_prep_* phase where dma buffers have
been already map in spi core.