| Message ID | 1591880310-1813-2-git-send-email-yibin.gong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | introduce fallback to pio in spi core | expand |
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.
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...
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.
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.
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.
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?
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?
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
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
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.
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.
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.
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 */
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.
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; + } +
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.
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; }
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.
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.
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...
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?
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 :)
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) \
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(+)