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

[Robin Gong <yibin.gong@xxxxxxx>]

On 2020/06/12 22:16 Mark Brown <broonie@xxxxxxxxxx> wrote: 
> On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> > On 2020/06/12 18:14 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 :)
> 
> Yes, looks good thanks!
> 
> > > 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.
> 
> It's not something that's going to come up a lot for most devices, it'd be a
> mapping failure due to running out of memory or something, but your point
> about that being possible is valid.
> 
> > > > 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?
> 
> No, I mean that the reason the DMA transfer fails may be something that
> happens after we've started putting things on the bus - the bit about FIFOs is
> just a random example of an error that could happen.
> 
Sorry Mark for that I can't get your point... The bus error such as data corrupt
seems not the spi core's business since it can only be caught in spi controller
driver or upper level such as mtd driver (spi-nor) which know what's the failure
happen at spi bus HW level or what's the correct data/message. In other words,
spi core can't detect such error by transfer_one().

> > > 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....
> 
> Well, this is free software and everything can be modified!  The other option
> would be framework changes in SPI that allowed us to indicate from the driver
> that an error occured before we started doing anything to the hardware (like
> happens here) through something like a special error code or splitting up the
> API.
Yes, but both assume spi controller driver could detect such dma failure before
dmaengine_prep_*(). Let's wait Vinod's comment for that if dmaengine_slave_config
could keep direction.
But despite of that case, do you think this patch is valid for transfer_one() failue
in dma and fallback to pio?