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

[PATCH v2 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.

v1 series: https://lore.kernel.org/linux-spi/20210713125743.1540-1-a-nandan@ti.com/

Changes in v2:
- [PATCH v2 1/2]: Same as v1. This patch has been already applied to
  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
  commit 9cb2ff111712 ("spi: cadence-quadspi: Disable Auto-HW polling")

- [PATCH v2 2/2]: Add new comments to explain the DTR check conditions

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 | 48 ++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH v2 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>
Link: https://lore.kernel.org/r/20210713125743.1540-2-a-nandan@ti.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 This patch has already been applied to
 https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
 [1/2] spi: cadence-quadspi: Disable Auto-HW polling
       commit: 9cb2ff11171264d10be7ea9e31d9ee5d49ba84a5
 Reposting it for sake of completeness of the v2 series.

 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 d62d69dd72b9..a2de23516553 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -800,19 +800,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

[PATCH v2 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 | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index a2de23516553..1cec1c179a94 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -325,7 +325,15 @@ 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;
+
+	/*
+	 * For an op to be DTR, cmd phase along with every other non-empty
+	 * phase should have dtr field set to 1. If an op phase has zero
+	 * nbytes, ignore its dtr field; otherwise, check its dtr field.
+	 */
+	f_pdata->dtr = op->cmd.dtr &&
+		       (!op->addr.nbytes || op->addr.dtr) &&
+		       (!op->data.nbytes || op->data.dtr);
 
 	switch (op->data.buswidth) {
 	case 0:
@@ -1228,8 +1236,15 @@ 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 required for converting nbytes into ncycles.
+	 * Also, don't check the dtr field of the op phase having zero nbytes.
+	 */
+	all_true = op->cmd.dtr &&
+		   (!op->addr.nbytes || op->addr.dtr) &&
+		   (!op->dummy.nbytes || op->dummy.dtr) &&
+		   (!op->data.nbytes || op->data.dtr);
+
 	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
 		    !op->data.dtr;
 
-- 
2.17.1

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

[Nandan, Apurva]

On 17-Jul-21 4:55 AM, Apurva Nandan wrote:
> 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 | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index a2de23516553..1cec1c179a94 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -325,7 +325,15 @@ 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;
> +
> +	/*
> +	 * For an op to be DTR, cmd phase along with every other non-empty
> +	 * phase should have dtr field set to 1. If an op phase has zero
> +	 * nbytes, ignore its dtr field; otherwise, check its dtr field.
> +	 */
> +	f_pdata->dtr = op->cmd.dtr &&
> +		       (!op->addr.nbytes || op->addr.dtr) &&
> +		       (!op->data.nbytes || op->data.dtr);
>  
>  	switch (op->data.buswidth) {
>  	case 0:
> @@ -1228,8 +1236,15 @@ 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 required for converting nbytes into ncycles.
> +	 * Also, don't check the dtr field of the op phase having zero nbytes.
> +	 */
> +	all_true = op->cmd.dtr &&
> +		   (!op->addr.nbytes || op->addr.dtr) &&
> +		   (!op->dummy.nbytes || op->dummy.dtr) &&
> +		   (!op->data.nbytes || op->data.dtr);
> +
>  	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
>  		    !op->data.dtr;
>  
> 

Hi Mark,

Could you please have a look, I fixed the comments as you suggested.

Thanks and regards,
Apurva Nandan

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

[Mark Brown]

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

On Wed, Jul 21, 2021 at 08:23:30PM +0530, Nandan, Apurva wrote:

> Could you please have a look, I fixed the comments as you suggested.

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

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

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

[Mark Brown]

On Fri, 16 Jul 2021 23:25:01 +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!

[2/2] spi: cadence-quadspi: Fix check condition for DTR ops
      commit: 0395be967b067d99494113d78470574e86a02ed4

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