| Message ID | 20210713125743.1540-2-a-nandan@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 9cb2ff11171264d10be7ea9e31d9ee5d49ba84a5 |
| 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...
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.
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 7a00346ff9b9..a2e1f4ce8b3e 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -797,19 +797,20 @@ static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata, reg = cqspi_calc_rdreg(f_pdata); writel(reg, reg_base + CQSPI_REG_RD_INSTR); - if (f_pdata->dtr) { - /* - * Some flashes like the cypress Semper flash expect a 4-byte - * dummy address with the Read SR command in DTR mode, but this - * controller does not support sending address with the Read SR - * command. So, disable write completion polling on the - * controller's side. spi-nor will take care of polling the - * status register. - */ - reg = readl(reg_base + CQSPI_REG_WR_COMPLETION_CTRL); - reg |= CQSPI_REG_WR_DISABLE_AUTO_POLL; - writel(reg, reg_base + CQSPI_REG_WR_COMPLETION_CTRL); - } + /* + * SPI NAND flashes require the address of the status register to be + * passed in the Read SR command. Also, some SPI NOR flashes like the + * cypress Semper flash expect a 4-byte dummy address in the Read SR + * command in DTR mode. + * + * But this controller does not support address phase in the Read SR + * command when doing auto-HW polling. So, disable write completion + * polling on the controller's side. spinand and spi-nor will take + * care of polling the status register. + */ + reg = readl(reg_base + CQSPI_REG_WR_COMPLETION_CTRL); + reg |= CQSPI_REG_WR_DISABLE_AUTO_POLL; + writel(reg, reg_base + CQSPI_REG_WR_COMPLETION_CTRL); reg = readl(reg_base + CQSPI_REG_SIZE); reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
cadence-quadspi has a builtin Auto-HW polling funtionality using which it keep tracks of completion of write operations. When Auto-HW polling is enabled, it automatically initiates status register read operation, until the flash clears its busy bit. 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. Signed-off-by: Apurva Nandan <a-nandan@ti.com> --- drivers/spi/spi-cadence-quadspi.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)