| Message ID | 20210713125743.1540-1-a-nandan@ti.com |
|---|---|
| Headers | show |
| Series | spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations | expand |
On Tue, Jul 13, 2021 at 12:57:41PM +0000, Apurva Nandan wrote: > cadence-quadspi controller doesn't allow an address phase when > auto-polling the busy bit on the status register. Unlike SPI NOR > flashes, SPI NAND flashes do require the address of status register > when polling the busy bit using the read register operation. As > Auto-HW polling is enabled by default, cadence-quadspi returns a > timeout for every write operation after an indefinite amount of > polling on SPI NAND flashes. > Disable Auto-HW polling completely as the spi-nor core, spinand core, > etc. take care of polling the busy bit on their own. Would it not be better to only disable this on NAND rather than disabling it completely?
On 13-Jul-21 11:55 PM, Mark Brown wrote: > On Tue, Jul 13, 2021 at 12:57:41PM +0000, Apurva Nandan wrote: > >> cadence-quadspi controller doesn't allow an address phase when >> auto-polling the busy bit on the status register. Unlike SPI NOR >> flashes, SPI NAND flashes do require the address of status register >> when polling the busy bit using the read register operation. As >> Auto-HW polling is enabled by default, cadence-quadspi returns a >> timeout for every write operation after an indefinite amount of >> polling on SPI NAND flashes. > >> Disable Auto-HW polling completely as the spi-nor core, spinand core, >> etc. take care of polling the busy bit on their own. > > Would it not be better to only disable this on NAND rather than > disabling it completely? > I am not sure how it is possible currently in the controller, could you please suggest a way? Also, should we have this logic of checking flash device type in the cadence-quadspi controller? SPI controller should be generic to all flash cores right? In my opinion, it shouldn't harm as spi-nor core doesn't depend on HW polling anyways and auto-HW polling is a minor overhead. Regards, Apurva Nandan
On Wed, Jul 14, 2021 at 06:52:12PM +0530, Nandan, Apurva wrote: > On 13-Jul-21 11:55 PM, Mark Brown wrote: > > On Tue, Jul 13, 2021 at 12:57:41PM +0000, Apurva Nandan wrote: > >> cadence-quadspi controller doesn't allow an address phase when > >> auto-polling the busy bit on the status register. Unlike SPI NOR > > Would it not be better to only disable this on NAND rather than > > disabling it completely? > I am not sure how it is possible currently in the controller, could you > please suggest a way? Also, should we have this logic of checking flash > device type in the cadence-quadspi controller? SPI controller should be > generic to all flash cores right? Surely the controller can tell if an address phase (or other unsupported feature) is present? > In my opinion, it shouldn't harm as spi-nor core doesn't depend on HW > polling anyways and auto-HW polling is a minor overhead. Flash stuff seems to quite often end up happening when the system is heavily loaded for other reasons, it's much more of an issue with things that are done more with PIO but still seems useful to avoid having to poll in software, either it'll reduce CPU load or reduce latency and increase throughput.
On 14-Jul-21 9:58 PM, Mark Brown wrote: > On Wed, Jul 14, 2021 at 06:52:12PM +0530, Nandan, Apurva wrote: >> On 13-Jul-21 11:55 PM, Mark Brown wrote: >>> On Tue, Jul 13, 2021 at 12:57:41PM +0000, Apurva Nandan wrote: > >>>> cadence-quadspi controller doesn't allow an address phase when >>>> auto-polling the busy bit on the status register. Unlike SPI NOR > >>> Would it not be better to only disable this on NAND rather than >>> disabling it completely? > >> I am not sure how it is possible currently in the controller, could you >> please suggest a way? Also, should we have this logic of checking flash >> device type in the cadence-quadspi controller? SPI controller should be >> generic to all flash cores right? > > Surely the controller can tell if an address phase (or other unsupported > feature) is present? > Yeah sure, understood. >> In my opinion, it shouldn't harm as spi-nor core doesn't depend on HW >> polling anyways and auto-HW polling is a minor overhead. > > Flash stuff seems to quite often end up happening when the system is > heavily loaded for other reasons, it's much more of an issue with things > that are done more with PIO but still seems useful to avoid having to > poll in software, either it'll reduce CPU load or reduce latency and > increase throughput. > Yes, got the point. Will amend it. Thanks, Apurva Nandan
On 14-Jul-21 11:21 PM, Apurva Nandan wrote: > > > On 14-Jul-21 9:58 PM, Mark Brown wrote: >> On Wed, Jul 14, 2021 at 06:52:12PM +0530, Nandan, Apurva wrote: >>> On 13-Jul-21 11:55 PM, Mark Brown wrote: >>>> On Tue, Jul 13, 2021 at 12:57:41PM +0000, Apurva Nandan wrote: >> >>>>> cadence-quadspi controller doesn't allow an address phase when >>>>> auto-polling the busy bit on the status register. Unlike SPI NOR >> >>>> Would it not be better to only disable this on NAND rather than >>>> disabling it completely? >> >>> I am not sure how it is possible currently in the controller, could you >>> please suggest a way? Also, should we have this logic of checking flash >>> device type in the cadence-quadspi controller? SPI controller should be >>> generic to all flash cores right? >> >> Surely the controller can tell if an address phase (or other unsupported >> feature) is present? >> > > Yeah sure, understood. > There are issues in this, I noticed it when tried to implement. So, the controller driver can't tell if an address phase is present, as it is just dealing with write page/reg operation and auto HW poll operation (whose address phase we are concerned with) isn't visible to it (as it is running solely on hardware). Now, whether the poll instruction should have an address phase or not depends on the connected flash chip, which the controller wouldn't be aware of as it only takes in a spimem op from the flash cores for execution. Hence, it can't disable auto HW polling by checking the the address phase and passing any flag information for this from flash cores would be inappropriate. More to this, not just address phase but any kind of variation in the read register operation would result in polling failure. Suppose, SPI-NOR flash is in QuadSPI/QPI mode, should the controller send poll instruction in 4s-4s-4s, 1s-4s-4s, or 1s-1s-1s mode? Some flashes keep it in 1s-1s-1s mode others keep it in 4s-4s-4s i.e it varies. For example, Winbond W25Q256FV SPI-NOR requires 4s-4s-4s read reg op when in QPI mode but it requires 1s-1s-1s read reg op when using QuadSPI instead of QPI mode. There can be other variations as well e.g. Gigadevice GD25LB256E requires 8 "don't care" bytes after command phase in QPI mode above 140MHz. Any SPI operation that is going underneath the visibility of flash core can can problems. I agree offloading the status polling process to controller HW is beneficial but on the other hand it restricts the flash on having a fixed type of polling operation. This would reduce the number of flash devices it will support (out of the box). What should be the right way out for this situation? >>> In my opinion, it shouldn't harm as spi-nor core doesn't depend on HW >>> polling anyways and auto-HW polling is a minor overhead. >> >> Flash stuff seems to quite often end up happening when the system is >> heavily loaded for other reasons, it's much more of an issue with things >> that are done more with PIO but still seems useful to avoid having to >> poll in software, either it'll reduce CPU load or reduce latency and >> increase throughput. >> > > Yes, got the point. Will amend it. > > Thanks, > Apurva Nandan > Thanks, Apurva Nandan
On Thu, Jul 15, 2021 at 09:57:51PM +0530, Apurva Nandan wrote: > Now, whether the poll instruction should have an address phase or not > depends on the connected flash chip, which the controller wouldn't be > aware of as it only takes in a spimem op from the flash cores for execution. ... > More to this, not just address phase but any kind of variation in the > read register operation would result in polling failure. That seems like something that should be fixed since it means that no controller will be able to support a feature like this - it needs to get enough information passed to it to allow it to poll correctly. > Any SPI operation that is going underneath the visibility of flash core > can can problems. I agree offloading the status polling process to > controller HW is beneficial but on the other hand it restricts the flash > on having a fixed type of polling operation. This would reduce the > number of flash devices it will support (out of the box). > What should be the right way out for this situation? One idea would be to have something that takes both the operation itself and the operation that's used to poll for status (with expected result), the controller can then check the poll operation and either tell the core it's not supported or go ahead and do the polling. Or simpler just a separate poll operation which is fully specified enough. Not actually looked at the code to see how tasteful that is though...
On 15/07/21 05:41PM, Mark Brown wrote: > On Thu, Jul 15, 2021 at 09:57:51PM +0530, Apurva Nandan wrote: > > > Now, whether the poll instruction should have an address phase or not > > depends on the connected flash chip, which the controller wouldn't be > > aware of as it only takes in a spimem op from the flash cores for execution. > > ... > > > More to this, not just address phase but any kind of variation in the > > read register operation would result in polling failure. > > That seems like something that should be fixed since it means that no > controller will be able to support a feature like this - it needs to get > enough information passed to it to allow it to poll correctly. Right. > > > Any SPI operation that is going underneath the visibility of flash core > > can can problems. I agree offloading the status polling process to > > controller HW is beneficial but on the other hand it restricts the flash > > on having a fixed type of polling operation. This would reduce the > > number of flash devices it will support (out of the box). > > What should be the right way out for this situation? > > One idea would be to have something that takes both the operation itself > and the operation that's used to poll for status (with expected result), > the controller can then check the poll operation and either tell the > core it's not supported or go ahead and do the polling. Or simpler just > a separate poll operation which is fully specified enough. We do have the new spi_mem_poll_status() API that does this somewhat. But this is not very useful for this controller since it you can't issue the auto polling on demand. It only happens when you perform a write. One option is: spi_mem_exec_op_with_poll(mem, op, poll_op, mask, match, timeout); But then the problem is how to tell the caller whether the poll actually happened or not. The other option I see is: ret = spi_mem_set_autopoll_op(mem, poll_op, mask, match, ...); spi_mem_exec_op(mem, op); if (ret == -EOPNOTSUPP) poll_status(); When spi_mem_set_autopoll_op() is called, the controller driver can check if it can autopoll with this op. It can configure its autopoll feature based on this, and can provide feedback to the caller about whether they will then have to poll themselves, or it has already been done for them. I like the latter option more. I think the question we need to answer first is whether doing all this is worth the hassle. Are there enough controllers with this auto polling feature to make it worth the bother? > > Not actually looked at the code to see how tasteful that is though... -- Regards, Pratyush Yadav Texas Instruments Inc.
On Fri, Jul 16, 2021 at 12:06:29AM +0530, Pratyush Yadav wrote: > On 15/07/21 05:41PM, Mark Brown wrote: > > On Thu, Jul 15, 2021 at 09:57:51PM +0530, Apurva Nandan wrote: > But then the problem is how to tell the caller whether the poll actually > happened or not. The other option I see is: > ret = spi_mem_set_autopoll_op(mem, poll_op, mask, match, ...); > spi_mem_exec_op(mem, op); > > if (ret == -EOPNOTSUPP) > poll_status(); > When spi_mem_set_autopoll_op() is called, the controller driver can > check if it can autopoll with this op. It can configure its autopoll > feature based on this, and can provide feedback to the caller about > whether they will then have to poll themselves, or it has already been > done for them. I was more thinking about just having polling be a separate operation entirely, but you're right that a controller might integrate polling with the actual operation so that won't do and we need something more like you suggest. > I like the latter option more. Yes. > I think the question we need to answer first is whether doing all this > is worth the hassle. Are there enough controllers with this auto polling > feature to make it worth the bother? If we build it they will come! :P But yes, that's definitely a concern.
On Tue, 13 Jul 2021 12:57:40 +0000, Apurva Nandan wrote: > This series proposes fixes for cadence-quadspi controller for the > following issues with SPI NAND flashes: > > - Due to auto-HW polling without address phase, the cadence-quadspi > controller timeouts when performing any write operation on SPI NAND > flash. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: cadence-quadspi: Disable Auto-HW polling commit: 9cb2ff11171264d10be7ea9e31d9ee5d49ba84a5 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark