[PATCH v1 RFC 0/2] introduce fallback to pio in spi core

[PATCH v1 RFC 0/2] introduce fallback to pio in spi core

[Robin Gong]

Introudce fallback pio way at spi core level so that the tx/rx buffer
mapped by spi core for dma could be unmap and fallback pio again. Hence no
need the below patch where 'dma_sync_sg_for_devic' to sync fresh data back
into 'device' memory and sync to 'cpu' again in spi core.
https://www.spinics.net/lists/arm-kernel/msg812236.html

If you want to use this fallback feature, please set 'master->flags' with
SPI_MASTER_FALLBACK and add 'master->fallback' checking in your can_dma()
to ensure it can fallback to pio.

Robin Gong (2):
  spi: introduce fallback to pio
  spi: spi-imx: add fallback feature

 drivers/spi/spi-imx.c   | 23 +++++------------------
 drivers/spi/spi.c       | 11 +++++++++++
 include/linux/spi/spi.h |  4 ++++
 3 files changed, 20 insertions(+), 18 deletions(-)

-- 
2.7.4

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

[Robin Gong]

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

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/spi/spi.c       | 11 +++++++++++
 include/linux/spi/spi.h |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8158e28..3bf860c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -982,6 +982,8 @@ static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg)
 		spi_unmap_buf(ctlr, tx_dev, &xfer->tx_sg, DMA_TO_DEVICE);
 	}
 
+	ctlr->cur_msg_mapped = false;
+
 	return 0;
 }
 #else /* !CONFIG_HAS_DMA */
@@ -1234,8 +1236,16 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 		if (xfer->tx_buf || xfer->rx_buf) {
 			reinit_completion(&ctlr->xfer_completion);
 
+fallback_pio:
 			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;
+				}
+
 				SPI_STATISTICS_INCREMENT_FIELD(statm,
 							       errors);
 				SPI_STATISTICS_INCREMENT_FIELD(stats,
@@ -1693,6 +1703,7 @@ void spi_finalize_current_message(struct spi_controller *ctlr)
 	spin_lock_irqsave(&ctlr->queue_lock, flags);
 	ctlr->cur_msg = NULL;
 	ctlr->cur_msg_prepared = false;
+	ctlr->fallback = false;
 	kthread_queue_work(&ctlr->kworker, &ctlr->pump_messages);
 	spin_unlock_irqrestore(&ctlr->queue_lock, flags);
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index aac57b5..95291fe4 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -506,6 +506,8 @@ struct spi_controller {
 #define SPI_CONTROLLER_MUST_TX		BIT(4)	/* requires tx */
 
 #define SPI_MASTER_GPIO_SS		BIT(5)	/* GPIO CS must select slave */
+/* fallback to PIO if DMA failed */
+#define SPI_CONTROLLER_FALLBACK		BIT(6)
 
 	/* flag indicating this is an SPI slave controller */
 	bool			slave;
@@ -602,6 +604,7 @@ struct spi_controller {
 	bool				auto_runtime_pm;
 	bool                            cur_msg_prepared;
 	bool				cur_msg_mapped;
+	bool				fallback;
 	struct completion               xfer_completion;
 	size_t				max_dma_len;
 
@@ -1517,6 +1520,7 @@ of_find_spi_device_by_node(struct device_node *node)
 #define SPI_MASTER_NO_TX		SPI_CONTROLLER_NO_TX
 #define SPI_MASTER_MUST_RX		SPI_CONTROLLER_MUST_RX
 #define SPI_MASTER_MUST_TX		SPI_CONTROLLER_MUST_TX
+#define SPI_MASTER_FALLBACK		SPI_CONTROLLER_FALLBACK
 
 #define spi_master_get_devdata(_ctlr)	spi_controller_get_devdata(_ctlr)
 #define spi_master_set_devdata(_ctlr, _data)	\
-- 
2.7.4

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

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...

>  			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 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

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...

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2104 bytes --]

On Fri, Jun 12, 2020 at 02:18:32AM +0000, Robin Gong wrote:
> On 2020/06/11 21: 41 Mark Brown <broonie@kernel.org> 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.

> > 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?

> > ...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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> wrote: 
> On Fri, Jun 12, 2020 at 02:18:32AM +0000, Robin Gong wrote:
> > On 2020/06/11 21: 41 Mark Brown <broonie@kernel.org> 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.  
 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> 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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/12 22:16 Mark Brown <broonie@kernel.org> wrote: 
> On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> > On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> 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?

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

[Vinod Koul]

On 14-06-20, 13:04, Robin Gong wrote:
> On 2020/06/12 22:16 Mark Brown <broonie@kernel.org> wrote: 
> > On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> > > On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> 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

> But despite of that case, do you think this patch is valid for transfer_one() failue
> in dma and fallback to pio?

-- 
~Vinod

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

[Robin Gong]

On 2020/06/15 15:20 Vinod Koul <vkoul@kernel.org> wrote:
> On 14-06-20, 13:04, Robin Gong wrote:
> > On 2020/06/12 22:16 Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> > > > On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> 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

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

[Vinod Koul]

Hi Robin,

On 15-06-20, 08:59, Robin Gong wrote:
> On 2020/06/15 15:20 Vinod Koul <vkoul@kernel.org> wrote:

> > > 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. 

Can you use .device_alloc_chan_resources for that? This is where all
the resource allocation for a channel should happen...

> 	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).

I think that is wrong expectation, dmaengine_slave_config should pass
the slave_config to driver and nothing else. The relevant action should
be taken in respective prep_ calls here, so that should be fixed as well

-- 
~Vinod

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

[Robin Gong]

On 2020/06/15 Vinod Koul <vkoul@kernel.org> wrote:
> Hi Robin,
> 
> On 15-06-20, 08:59, Robin Gong wrote:
> > 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.
> 
> Can you use .device_alloc_chan_resources for that? This is where all the
> resource allocation for a channel should happen...
But many client driver request dma channel(device_alloc_chan_resources)
in driver probe phase instead of later transfer startup phase.

> > 	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).
> 
> I think that is wrong expectation, dmaengine_slave_config should pass the
> slave_config to driver and nothing else. The relevant action should be taken in
> respective prep_ calls here, so that should be fixed as well
Got it, thanks for your clarification.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On Sun, Jun 14, 2020 at 01:04:57PM +0000, Robin Gong wrote:
> On 2020/06/12 22:16 Mark Brown <broonie@kernel.org> wrote: 
> > On Fri, Jun 12, 2020 at 01:48:41PM +0000, Robin Gong wrote:
> > > On 2020/06/12 18:14 Mark Brown <broonie@kernel.org> wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

> > 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().

If we see an error in transfer_one() it could be from anything, we've no
idea what happened on the bus - the controller may have got part way
through the transfer before failing.

> But despite of that case, do you think this patch is valid for transfer_one() failue
> in dma and fallback to pio?

No, not unless we know that nothing went out on the bus.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/15 20:36 Mark Brown <broonie@kernel.org> wrote: 
> On Sun, Jun 14, 2020 at 01:04:57PM +0000, Robin Gong wrote:
> 
> Please delete unneeded context from mails when replying.  Doing this makes
> it much easier to find your reply in the message, helping ensure it won't be
> missed by people scrolling through the irrelevant quoted material.
Okay, thanks Mark.
> 
> > > 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().
> 
> If we see an error in transfer_one() it could be from anything, we've no idea
> what happened on the bus - the controller may have got part way through the
> transfer before failing.
Then how about choosing specific error code for such dma not ready case where
nothing went out on the bus neither? 

> 
> > But despite of that case, do you think this patch is valid for
> > transfer_one() failue in dma and fallback to pio?
> 
> No, not unless we know that nothing went out on the bus.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Mon, Jun 15, 2020 at 01:35:01PM +0000, Robin Gong wrote:
> On 2020/06/15 20:36 Mark Brown <broonie@kernel.org> wrote: 

> > If we see an error in transfer_one() it could be from anything, we've no idea
> > what happened on the bus - the controller may have got part way through the
> > transfer before failing.

> Then how about choosing specific error code for such dma not ready case where
> nothing went out on the bus neither? 

Yes, that's what I suggested.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/15 21:39 Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jun 15, 2020 at 01:35:01PM +0000, Robin Gong wrote:
> > Then how about choosing specific error code for such dma not ready
> > case where nothing went out on the bus neither?
> 
> Yes, that's what I suggested.
Seems not easy to find a suitable error value, how about EBADR which
sounds like no any available dma_async_tx_descriptor got by calling dmaengine_prep_slave_sg? 
#define EBADR           53      /* Invalid request descriptor */

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

On Mon, Jun 15, 2020 at 02:18:54PM +0000, Robin Gong wrote:
> On 2020/06/15 21:39 Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jun 15, 2020 at 01:35:01PM +0000, Robin Gong wrote:

> > > Then how about choosing specific error code for such dma not ready
> > > case where nothing went out on the bus neither?

> > Yes, that's what I suggested.

> Seems not easy to find a suitable error value, how about EBADR which
> sounds like no any available dma_async_tx_descriptor got by calling dmaengine_prep_slave_sg? 

> #define EBADR           53      /* Invalid request descriptor */

We could also pass in a flag that could be set separately to the error
code to indicate that nothing had happened to the hardware yet.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/15 22:36 Mark Brown <broonie@kernel.org> wrote: 
> On Mon, Jun 15, 2020 at 02:18:54PM +0000, Robin Gong wrote:
> > Seems not easy to find a suitable error value, how about EBADR which
> > sounds like no any available dma_async_tx_descriptor got by calling
> dmaengine_prep_slave_sg?
> 
> > #define EBADR           53      /* Invalid request descriptor */
> 
> We could also pass in a flag that could be set separately to the error code to
> indicate that nothing had happened to the hardware yet.
Do you mean spi-imx.c checking 'ctlr->flags' before return such error code?
Or just like below done in spi.c.
+fallback_pio:
                        ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
                        if (ret < 0) {
+                               if (ret == - EBADR && ctlr->cur_msg_mapped &&
+                                  (ctlr->flags & SPI_CONTROLLER_FALLBACK)) {
+                                       __spi_unmap_msg(ctlr, msg);
+                                       ctlr->fallback = true;
+                                       goto fallback_pio;
+                               }
+

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

On Mon, Jun 15, 2020 at 02:53:29PM +0000, Robin Gong wrote:
> On 2020/06/15 22:36 Mark Brown <broonie@kernel.org> wrote: 

> > We could also pass in a flag that could be set separately to the error code to
> > indicate that nothing had happened to the hardware yet.

> Do you mean spi-imx.c checking 'ctlr->flags' before return such error code?
> Or just like below done in spi.c.

No, I mean passing in an additional argument which can provide richer
data than trying to smash things into the return value.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/15 22:56 Mark Brown <broonie@kernel.org> wrote: 
> On Mon, Jun 15, 2020 at 02:53:29PM +0000, Robin Gong wrote: 
> > Do you mean spi-imx.c checking 'ctlr->flags' before return such error code?
> > Or just like below done in spi.c.
> 
> No, I mean passing in an additional argument which can provide richer data
> than trying to smash things into the return value.
Okay, how about adding this additional argument in struct spi_transfer like below?
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 95291fe4..7c19099 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -943,6 +943,9 @@ struct spi_transfer {
        bool            timestamped;

        struct list_head transfer_list;
+
+#define        SPI_TRANS_DMA_PREP_FAIL BIT(3)  /* prepare dma failed */
+       u16             flags;
 };

While spi core checking it as below after it set in spi-imx.c:
fallback_pio:
        ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
        if (ret < 0) {
                 if (ctlr->cur_msg_mapped &&
                   (xfer->flags & SPI_TRANS_DMA_PREP_FAIL)) {
                   __spi_unmap_msg(ctlr, msg);
                   ctlr->fallback = true;
                   xfer->flags &= ~SPI_TRANS_DMA_PREP_FAIL;
                   goto fallback_pio;
                 }

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Tue, Jun 16, 2020 at 02:03:40AM +0000, Robin Gong wrote:
> On 2020/06/15 22:56 Mark Brown <broonie@kernel.org> wrote: 

>         struct list_head transfer_list;
> +
> +#define        SPI_TRANS_DMA_PREP_FAIL BIT(3)  /* prepare dma failed */
> +       u16             flags;

I'd just make this a generic flag for failures before we start
interacting with the hardware rather than specifically this one error
case.  Otherwise this looks fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/16 Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jun 16, 2020 at 02:03:40AM +0000, Robin Gong wrote:
> >         struct list_head transfer_list;
> > +
> > +#define        SPI_TRANS_DMA_PREP_FAIL BIT(3)  /* prepare dma
> failed */
> > +       u16             flags;
> 
> I'd just make this a generic flag for failures before we start interacting with the
> hardware rather than specifically this one error case.  Otherwise this looks
> fine.
So rename to SPI_TRANS_DMA_FAIL? I think at least DMA is MUST for fallback
case...

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On Tue, Jun 16, 2020 at 10:13:08AM +0000, Robin Gong wrote:
> On 2020/06/16 Mark Brown <broonie@kernel.org> wrote:

> > I'd just make this a generic flag for failures before we start interacting with the
> > hardware rather than specifically this one error case.  Otherwise this looks
> > fine.

> So rename to SPI_TRANS_DMA_FAIL? I think at least DMA is MUST for fallback
> case...

This is not purely for DMA, it's just about the failure having occurred
before the transfer started.  How about _FAIL_NO_START?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Robin Gong]

On 2020/06/16 18:27 Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jun 16, 2020 at 10:13:08AM +0000, Robin Gong wrote:
> > So rename to SPI_TRANS_DMA_FAIL? I think at least DMA is MUST for
> > fallback case...
> 
> This is not purely for DMA, it's just about the failure having occurred before the
> transfer started.  How about _FAIL_NO_START?
Okay, I'll use SPI_TRANS_FAIL_NO_START in v2. Thanks Mark for your kind support :)

[PATCH v1 RFC 2/2] spi: spi-imx: add fallback feature

[Robin Gong]

Add fallback pio feature for dma transfer failed. Since another whole
pio transfer including setup_transfer will be issued by spi core, no
need to restore jobs like commit bcd8e7761ec9 ("spi: imx: fallback to PIO
if dma setup failure").

Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
 drivers/spi/spi-imx.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index b7a85e3..afd31d7 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -224,7 +224,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
 
-	if (!use_dma)
+	if (!use_dma || master->fallback)
 		return false;
 
 	if (!master->dma_rx)
@@ -1302,7 +1302,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	master->can_dma = spi_imx_can_dma;
 	master->max_dma_len = MAX_SDMA_BD_BYTES;
 	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
-					 SPI_MASTER_MUST_TX;
+					 SPI_MASTER_MUST_TX |
+					 SPI_MASTER_FALLBACK;
 
 	return 0;
 err:
@@ -1507,7 +1508,6 @@ static int spi_imx_transfer(struct spi_device *spi,
 				struct spi_transfer *transfer)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
-	int ret;
 
 	/* flush rxfifo before transfer */
 	while (spi_imx->devtype_data->rx_available(spi_imx))
@@ -1516,21 +1516,8 @@ static int spi_imx_transfer(struct spi_device *spi,
 	if (spi_imx->slave_mode)
 		return spi_imx_pio_transfer_slave(spi, transfer);
 
-	/*
-	 * fallback PIO mode if dma setup error happen, for example sdma
-	 * firmware may not be updated as ERR009165 required.
-	 */
-	if (spi_imx->usedma) {
-		ret = spi_imx_dma_transfer(spi_imx, transfer);
-		if (ret != -EINVAL)
-			return ret;
-
-		spi_imx->devtype_data->disable_dma(spi_imx);
-
-		spi_imx->usedma = false;
-		spi_imx->dynamic_burst = spi_imx->devtype_data->dynamic_burst;
-		dev_dbg(&spi->dev, "Fallback to PIO mode\n");
-	}
+	if (spi_imx->usedma)
+		return spi_imx_dma_transfer(spi_imx, transfer);
 
 	return spi_imx_pio_transfer(spi, transfer);
 }
-- 
2.7.4