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

[Robin Gong <yibin.gong@xxxxxxx>]

On 2020/06/15 15:20 Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> On 14-06-20, 13:04, Robin Gong wrote:
> > 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.
> 
> The direction is already in the prep_ call, so sending in dmaengine_slave_config
> is not required, pls pass it in the prep_ call
Hi Vinod,
	Is there any way to let the device driver to know dma controller is ready
(in sdma case is sdma firmware loaded or not)before prep_call? Hence, spi core
could map dma buffer or not. Prep_call is too late for spi core since the buffers
have been already mapped. 

	From my view, seems dmaengine_slave_config is the only one...Further,
sdma need direction in dmaengine_slave_config phase, because currently
what's the tx/rx script used on sdma channel is decided not only peripheral_type
but also direction. For example, spi tx dma is running ram script to workaround
ecspi ERR009165 while rx dma is running rom script, so only spi tx dma channel
depends on sdma firmware loaded(now that could be detect by ' load_address
< 0' in sdma_load_context() and prep_ call finally).
   I knew direction is deprecated in dmaengine_slave_config, but that's really
very useful for sdma to check if firmware loaded and spi core could get it earlier
before prep_call(fallback to PIO if dma is not ready).
 
> 
> > But despite of that case, do you think this patch is valid for
> > transfer_one() failue in dma and fallback to pio?
> 
> --
> ~Vinod