[PATCH 0/2] spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations

[PATCH 0/2] spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations

[Apurva Nandan]

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

- When checking for DTR spi_mem_op, cadence-quadspi doesn't ignore a
  zero length phase in the SPI instruction, resulting in false negatives.

This series has been tested on TI J721e EVM with the Winbond W35N01JW
flash.

Apurva Nandan (2):
  spi: cadence-quadspi: Disable Auto-HW polling
  spi: cadence-quadspi: Fix check condition for DTR ops

 drivers/spi/spi-cadence-quadspi.c | 39 ++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Apurva Nandan]

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

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;
-- 
2.17.1

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Mark Brown]

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

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?

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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Nandan, Apurva]

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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Mark Brown]

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

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.

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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Apurva Nandan]

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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Mark Brown]

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

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

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

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Pratyush Yadav]

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.

Re: [PATCH 1/2] spi: cadence-quadspi: Disable Auto-HW polling

[Mark Brown]

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

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.

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

[PATCH 2/2] spi: cadence-quadspi: Fix check condition for DTR ops

[Apurva Nandan]

buswidth and dtr fields in spi_mem_op are only valid when the
corresponding spi_mem_op phase has a non-zero length. For example,
SPI NAND core doesn't set buswidth when using SPI_MEM_OP_NO_ADDR
phase.

Fix the dtr checks in set_protocol() and suppports_mem_op() to
ignore empty spi_mem_op phases, as checking for dtr field in
empty phase will result in false negatives.

Signed-off-by: Apurva Nandan <a-nandan@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index a2e1f4ce8b3e..a884678e8dff 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -322,7 +322,9 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
 	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
 	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
 	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
-	f_pdata->dtr = op->data.dtr && op->cmd.dtr && op->addr.dtr;
+	f_pdata->dtr = op->cmd.dtr &&
+		       (op->addr.dtr || !op->addr.nbytes) &&
+		       (op->data.dtr || !op->data.nbytes);
 
 	switch (op->data.buswidth) {
 	case 0:
@@ -1225,8 +1227,12 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
 {
 	bool all_true, all_false;
 
-	all_true = op->cmd.dtr && op->addr.dtr && op->dummy.dtr &&
-		   op->data.dtr;
+	/* op->dummy.dtr is checked when converting nbytes into ncycles.*/
+	all_true = op->cmd.dtr &&
+		   (op->addr.dtr || !op->addr.nbytes) &&
+		   (op->dummy.dtr || !op->dummy.nbytes) &&
+		   (op->data.dtr || !op->data.nbytes);
+
 	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
 		    !op->data.dtr;
 
-- 
2.17.1

Re: [PATCH 2/2] spi: cadence-quadspi: Fix check condition for DTR ops

[Mark Brown]

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

On Tue, Jul 13, 2021 at 12:57:42PM +0000, Apurva Nandan wrote:

> +	f_pdata->dtr = op->cmd.dtr &&
> +		       (op->addr.dtr || !op->addr.nbytes) &&
> +		       (op->data.dtr || !op->data.nbytes);

I'm not sure anyone reading this code is going to figure out what it's
doing without thinking about it, the combination of writing the bytes
check with a !, putting it after the check for .dtr and not having any
comments is a bit obscure.  Something like

	(op->addr.nbytes && op.addr.dtr)

might be a bit clearer, or a comment explicitly spelling it out.

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

Re: [EXTERNAL] Re: [PATCH 2/2] spi: cadence-quadspi: Fix check condition for DTR ops

[Nandan, Apurva]

On 14-Jul-21 12:09 AM, Mark Brown wrote:
> On Tue, Jul 13, 2021 at 12:57:42PM +0000, Apurva Nandan wrote:
> 
>> +	f_pdata->dtr = op->cmd.dtr &&
>> +		       (op->addr.dtr || !op->addr.nbytes) &&
>> +		       (op->data.dtr || !op->data.nbytes);
> 
> I'm not sure anyone reading this code is going to figure out what it's
> doing without thinking about it, the combination of writing the bytes
> check with a !, putting it after the check for .dtr and not having any
> comments is a bit obscure.  Something like
> 
> 	(op->addr.nbytes && op.addr.dtr)
> 
> might be a bit clearer, or a comment explicitly spelling it out.
> 

Okay, I will add a comment explaining it, as other logic (with &&) won't
deliver the behavior we expect.

The logic it implements is: for an op to be dtr, if any phase has
non-zero bytes, it must have dtr field set as true. So, it does p
implies q: p->q = (!p || q) i.e. if phase has non-zero bytes (p) then
check its dtr field (q). If all phases are empty, then follow what
op.cmd.dtr says.

Regards,
Apurva Nandan

Re: (subset) [PATCH 0/2] spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations

[Mark Brown]

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