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

From: Robin Gong <yibin.gong@nxp.com>
To: Mark Brown <broonie@kernel.org>
Cc: "shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"matthias.schiffer@ew.tq-group.com" 
	<matthias.schiffer@ew.tq-group.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v1 RFC 1/2] spi: introduce fallback to pio
Date: Fri, 12 Jun 2020 02:18:32 +0000	[thread overview]
Message-ID: <VE1PR04MB66383245FAD2AE33CFEA76F789810@VE1PR04MB6638.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20200611134042.GG4671@sirena.org.uk>

On 2020/06/11 21: 41 Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 11, 2020 at 08:58:29PM +0800, Robin Gong wrote:
> > Add SPI_CONTROLLER_FALLBACK to fallback to pio mode in case dma
> > transfer failed.
> > If spi client driver want to enable this feature please set
> > master->flags with SPI_MASTER_FALLBACK and add master->fallback
> > checking in its can_dma() as spi-imx.c
> 
> 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.
> 
> >  			ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
> >  			if (ret < 0) {
> > +				if (ctlr->cur_msg_mapped &&
> > +				   (ctlr->flags & SPI_CONTROLLER_FALLBACK)) {
> > +					__spi_unmap_msg(ctlr, msg);
> > +					ctlr->fallback = true;
> > +					goto fallback_pio;
> > +				}
> 
> ...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?    
> 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...