linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver
@ 2023-01-25  8:10 Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion Dhruva Gole
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Dhruva Gole @ 2023-01-25  8:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	Dhruva Gole, linux-mtd, linux-arm-kernel, Takahiro.Kuwano,
	Pratyush Yadav, Pratyush Yadav

* Reset the CMD_CTRL Register, without which read/writes in STIG mode
were failing in some cases. The issue came to light while using STIG
Mode for small reads.
* Also add a flag that can allow us to do direct reads but distinguish
direct writes, thus enabling us to disable writes in DAC mode in some
cases that require it. (Like to write to some connected Flash registers)
* Fix register reads in STIG mode and also use STIG mode while reading flash
registers.
Currently if you try to read a register while in STIG mode there is no
support for ADDR and thus naturally a register never gets read from the
flash.

This patch series has been tested on a TI AM625-SK-EVM with both a quad
spi nor flash (s25hs) and OSPI NOR Flash (s28hs).

Output of ltp-ddt test, "DD_RW_ERASESIZE_UBIFS" run with s25hs512t flash:
...
[    2.334068] spi-nor spi0.0: s25hs512t (65536 Kbytes)
[    2.339185] 7 fixed-partitions partitions found on MTD device
fc40000.spi.0
[    2.346158] Creating 7 MTD partitions on "fc40000.spi.0":
[    2.351555] 0x000000000000-0x000000080000 : "ospi.tiboot3"
[    2.358344] 0x000000080000-0x000000280000 : "ospi.tispl"
[    2.364788] 0x000000280000-0x000000680000 : "ospi.u-boot"
[    2.371311] 0x000000680000-0x0000006c0000 : "ospi.env"
[    2.377519] 0x0000006c0000-0x000000700000 : "ospi.env.backup"
[    2.384419] 0x000000800000-0x000003fc0000 : "ospi.rootfs"
[    2.390890] 0x000003fc0000-0x000004000000 : "ospi.phypattern"
..snip..
Test Start Time: Wed Jan 11 21:14:31 2023
-----------------------------------------
Testcase                                           Result     Exit Value
--------                                           ------     ----------
OSPI_S_FUNC_DD_RW_ERASESIZE_UBIFS                  PASS       0

-----------------------------------------------
Total Tests: 1
Total Skipped Tests: 0
Total Failures: 0
Kernel Version: 6.2.0-rc1-00040-g700d796a94e0-dirty
Machine Architecture: aarch64
Hostname: am62xx-evm
...

Previous version link:
https://lore.kernel.org/linux-spi/20230104062604.1556763-1-d-gole@ti.com/

Changelog:

* This version addresses concerns from Vaishnav where he was seeing issues
while testing on different platforms.
* Also has additional patches that fix the issues that came to light
while using STIG Mode.

To: Mark Brown <broonie@kernel.org>
Cc: Vaishnav Achath <vaishnav.a@ti.com>
Cc: Pratyush Yadav <pratyush@kernel.org>
Cc: Vignesh Raghavendra <vigneshr@ti.com>

Dhruva Gole (4):
  spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
  spi: cadence-quadspi: Add flag for direct mode writes
  spi: cadence-quadspi: setup ADDR Bits in cmd reads
  spi: cadence-quadspi: use STIG mode for small reads

 drivers/spi/spi-cadence-quadspi.c | 42 ++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

--
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
  2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
@ 2023-01-25  8:10 ` Dhruva Gole
  2023-01-27 15:16   ` Pratyush Yadav
  2023-01-25  8:10 ` [PATCH v2 2/4] spi: cadence-quadspi: Add flag for direct mode writes Dhruva Gole
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Dhruva Gole @ 2023-01-25  8:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	Dhruva Gole, linux-mtd, linux-arm-kernel, Takahiro.Kuwano,
	Pratyush Yadav

If one leaves the CQSPI_REG_CMDCTRL in an unclean state this may cause
issues in future command reads. This issue came to light when some flash
reads in STIG mode were coming back dirty.

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 676313e1bdad..6030da942c6e 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -549,6 +549,9 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
 		memcpy(rxbuf, &reg, read_len);
 	}

+	/* Reset CMD_CTRL Reg once command read completes */
+	writel(0, reg_base + CQSPI_REG_CMDCTRL);
+
 	return 0;
 }

@@ -613,7 +616,12 @@ static int cqspi_command_write(struct cqspi_flash_pdata *f_pdata,
 		}
 	}

-	return cqspi_exec_flash_cmd(cqspi, reg);
+	ret = cqspi_exec_flash_cmd(cqspi, reg);
+
+	/* Reset CMD_CTRL Reg once command write completes */
+	writel(0, reg_base + CQSPI_REG_CMDCTRL);
+
+	return ret;
 }

 static int cqspi_read_setup(struct cqspi_flash_pdata *f_pdata,
--
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/4] spi: cadence-quadspi: Add flag for direct mode writes
  2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion Dhruva Gole
@ 2023-01-25  8:10 ` Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 3/4] spi: cadence-quadspi: setup ADDR Bits in cmd reads Dhruva Gole
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Dhruva Gole @ 2023-01-25  8:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	Dhruva Gole, linux-mtd, linux-arm-kernel, Takahiro.Kuwano,
	Pratyush Yadav

Create new flag inorder to avoid playing with use_direct_mode
flag currently being used throughout the driver.
Disable DAC write if auto polling is disabled or CQSPI_DISABLE_DAC_MODE
is set.

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 6030da942c6e..4bbf6e3ad34a 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -84,6 +84,7 @@ struct cqspi_st {
 	u32			trigger_address;
 	u32			wr_delay;
 	bool			use_direct_mode;
+	bool			use_direct_mode_wr;
 	struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
 	bool			use_dma_read;
 	u32			pd_dev_id;
@@ -945,6 +946,12 @@ static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata,
 		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);
+		/*
+		 * DAC mode require auto polling as flash needs to be polled
+		 * for write completion in case of bubble in SPI transaction
+		 * due to slow CPU/DMA master.
+		 */
+		cqspi->use_direct_mode_wr = false;
 	}

 	reg = readl(reg_base + CQSPI_REG_SIZE);
@@ -1230,7 +1237,7 @@ static ssize_t cqspi_write(struct cqspi_flash_pdata *f_pdata,
 	 * data.
 	 */
 	if (!op->cmd.dtr && cqspi->use_direct_mode &&
-	    ((to + len) <= cqspi->ahb_size)) {
+	    cqspi->use_direct_mode_wr && ((to + len) <= cqspi->ahb_size)) {
 		memcpy_toio(cqspi->ahb_base + to, buf, len);
 		return cqspi_wait_idle(cqspi);
 	}
@@ -1700,8 +1707,10 @@ static int cqspi_probe(struct platform_device *pdev)
 						cqspi->master_ref_clk_hz);
 		if (ddata->hwcaps_mask & CQSPI_SUPPORTS_OCTAL)
 			master->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
-		if (!(ddata->quirks & CQSPI_DISABLE_DAC_MODE))
+		if (!(ddata->quirks & CQSPI_DISABLE_DAC_MODE)) {
 			cqspi->use_direct_mode = true;
+			cqspi->use_direct_mode_wr = true;
+		}
 		if (ddata->quirks & CQSPI_SUPPORT_EXTERNAL_DMA)
 			cqspi->use_dma_read = true;
 		if (ddata->quirks & CQSPI_NO_SUPPORT_WR_COMPLETION)
--
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/4] spi: cadence-quadspi: setup ADDR Bits in cmd reads
  2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 2/4] spi: cadence-quadspi: Add flag for direct mode writes Dhruva Gole
@ 2023-01-25  8:10 ` Dhruva Gole
  2023-01-25  8:10 ` [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads Dhruva Gole
  2023-02-14 21:10 ` [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Mark Brown
  4 siblings, 0 replies; 16+ messages in thread
From: Dhruva Gole @ 2023-01-25  8:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	Dhruva Gole, linux-mtd, linux-arm-kernel, Takahiro.Kuwano,
	Pratyush Yadav

Setup the Addr bit field while issuing register reads in STIG mode. This
is needed for example flashes like cypress define in their transaction
table that to read any register there is 1 cmd byte and a few more address
bytes trailing the cmd byte. Absence of addr bytes will obviously fail
to read correct data from flash register that maybe requested by flash
driver because the controller doesn't even specify which address of the
flash register the read is being requested from.

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 4bbf6e3ad34a..a6d0f1b0bb49 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -532,6 +532,17 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
 	/* 0 means 1 byte. */
 	reg |= (((n_rx - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK)
 		<< CQSPI_REG_CMDCTRL_RD_BYTES_LSB);
+
+	/* setup ADDR BIT field */
+	if (op->addr.nbytes) {
+		reg |= (0x1 << CQSPI_REG_CMDCTRL_ADDR_EN_LSB);
+		reg |= ((op->addr.nbytes - 1) &
+			CQSPI_REG_CMDCTRL_ADD_BYTES_MASK)
+			<< CQSPI_REG_CMDCTRL_ADD_BYTES_LSB;
+
+		writel(op->addr.val, reg_base + CQSPI_REG_CMDADDRESS);
+	}
+
 	status = cqspi_exec_flash_cmd(cqspi, reg);
 	if (status)
 		return status;
--
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
                   ` (2 preceding siblings ...)
  2023-01-25  8:10 ` [PATCH v2 3/4] spi: cadence-quadspi: setup ADDR Bits in cmd reads Dhruva Gole
@ 2023-01-25  8:10 ` Dhruva Gole
  2023-04-26  2:34   ` Yoshitaka Ikeda
  2023-02-14 21:10 ` [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Mark Brown
  4 siblings, 1 reply; 16+ messages in thread
From: Dhruva Gole @ 2023-01-25  8:10 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	Dhruva Gole, linux-mtd, linux-arm-kernel, Takahiro.Kuwano,
	Pratyush Yadav

Fix the issue where some flash chips like cypress S25HS256T return the
value of the same register over and over in DAC mode.

For example in the TI K3-AM62x Processors refer [0] Technical Reference
Manual there is a layer of digital logic in front of the QSPI/OSPI
Drive when used in DAC mode. This is part of the Flash Subsystem (FSS)
which provides access to external Flash devices.

The FSS0_0_SYSCONFIG Register (Offset = 4h) has a BIT Field for
OSPI_32B_DISABLE_MODE which has a Reset value = 0. This means, OSPI 32bit
mode enabled by default.

Thus, by default controller operates in 32 bit mode causing it to always
align all data to 4 bytes from a 4byte aligned address. In some flash
chips like cypress for example if we try to read some regs in DAC mode
then it keeps sending the value of the first register that was requested
and inorder to read the next reg, we have to stop and re-initiate a new
transaction.

This causes wrong register values to be read than what is desired when
registers are read in DAC mode. Hence if the data.nbytes is very less
then prefer STIG mode for such small reads.

[0] https://www.ti.com/lit/ug/spruiv7a/spruiv7a.pdf

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index a6d0f1b0bb49..2954c06a7f57 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1359,7 +1359,13 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 	cqspi_configure(f_pdata, mem->spi->max_speed_hz);

 	if (op->data.dir == SPI_MEM_DATA_IN && op->data.buf.in) {
-		if (!op->addr.nbytes)
+	/*
+	 * Performing reads in DAC mode forces to read minimum 4 bytes
+	 * which is unsupported on some flash devices during register
+	 * reads, prefer STIG mode for such small reads.
+	 */
+		if (!op->addr.nbytes ||
+		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)
 			return cqspi_command_read(f_pdata, op);

 		return cqspi_read(f_pdata, op);
--
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
  2023-01-25  8:10 ` [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion Dhruva Gole
@ 2023-01-27 15:16   ` Pratyush Yadav
  2023-02-07 13:06     ` Gole, Dhruva
  0 siblings, 1 reply; 16+ messages in thread
From: Pratyush Yadav @ 2023-01-27 15:16 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: Mark Brown, linux-spi, linux-kernel, Vignesh Raghavendra,
	Vaishnav Achath, linux-mtd, linux-arm-kernel, Takahiro.Kuwano


Hi,

On Wed, Jan 25 2023, Dhruva Gole wrote:

> If one leaves the CQSPI_REG_CMDCTRL in an unclean state this may cause
> issues in future command reads. This issue came to light when some flash
> reads in STIG mode were coming back dirty.

Can you explain in more detail what you mean by "reads coming back
dirty"? Because I don't see any clear reason why not resetting the
register would break anything. We re-create the register value from the
scratch on the next read anyway, and as soon as you writel() that, the
old fields get thrown away anyway.

>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 676313e1bdad..6030da942c6e 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -549,6 +549,9 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
>                 memcpy(rxbuf, &reg, read_len);
>         }
>
> +       /* Reset CMD_CTRL Reg once command read completes */
> +       writel(0, reg_base + CQSPI_REG_CMDCTRL);
> +
>         return 0;
>  }
>
> @@ -613,7 +616,12 @@ static int cqspi_command_write(struct cqspi_flash_pdata *f_pdata,
>                 }
>         }
>
> -       return cqspi_exec_flash_cmd(cqspi, reg);
> +       ret = cqspi_exec_flash_cmd(cqspi, reg);
> +
> +       /* Reset CMD_CTRL Reg once command write completes */
> +       writel(0, reg_base + CQSPI_REG_CMDCTRL);
> +
> +       return ret;
>  }
>
>  static int cqspi_read_setup(struct cqspi_flash_pdata *f_pdata,
> --
> 2.25.1
>

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
  2023-01-27 15:16   ` Pratyush Yadav
@ 2023-02-07 13:06     ` Gole, Dhruva
  0 siblings, 0 replies; 16+ messages in thread
From: Gole, Dhruva @ 2023-02-07 13:06 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: Mark Brown, linux-spi, linux-kernel, Vignesh Raghavendra,
	Vaishnav Achath, linux-mtd, linux-arm-kernel, Takahiro.Kuwano

Thanks for taking a look Pratyush!

On 1/27/2023 8:46 PM, Pratyush Yadav wrote:
> Hi,
>
> On Wed, Jan 25 2023, Dhruva Gole wrote:
>
>> If one leaves the CQSPI_REG_CMDCTRL in an unclean state this may cause
>> issues in future command reads. This issue came to light when some flash
>> reads in STIG mode were coming back dirty.
> Can you explain in more detail what you mean by "reads coming back
> dirty"? Because I don't see any clear reason why not resetting the
> register would break anything. We re-create the register value from the
> scratch on the next read anyway, and as soon as you writel() that, the
> old fields get thrown away anyway.
There's a hardware bug due to which clearing of this register is necessary.
> [...]

-- 
Regards,
Dhruva Gole <d-gole@ti.com>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver
  2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
                   ` (3 preceding siblings ...)
  2023-01-25  8:10 ` [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads Dhruva Gole
@ 2023-02-14 21:10 ` Mark Brown
  4 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2023-02-14 21:10 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Pratyush Yadav

On Wed, 25 Jan 2023 13:40:19 +0530, Dhruva Gole wrote:
> * Reset the CMD_CTRL Register, without which read/writes in STIG mode
> were failing in some cases. The issue came to light while using STIG
> Mode for small reads.
> * Also add a flag that can allow us to do direct reads but distinguish
> direct writes, thus enabling us to disable writes in DAC mode in some
> cases that require it. (Like to write to some connected Flash registers)
> * Fix register reads in STIG mode and also use STIG mode while reading flash
> registers.
> Currently if you try to read a register while in STIG mode there is no
> support for ADDR and thus naturally a register never gets read from the
> flash.
> 
> [...]

Applied to

   broonie/spi.git for-next

Thanks!

[1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
      commit: d4f43a2d05faf7febb839edb2e9e8f85dfb9d2d2
[2/4] spi: cadence-quadspi: Add flag for direct mode writes
      commit: e8c51b164355c1d519a4b8ad0873f131035d26b7
[3/4] spi: cadence-quadspi: setup ADDR Bits in cmd reads
      commit: a8674ae02db232927385c2d0a063e10c0118f5ca
[4/4] spi: cadence-quadspi: use STIG mode for small reads
      commit: d403fb6e76bf854ef0f7d84e797e51b9494788e0

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-01-25  8:10 ` [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads Dhruva Gole
@ 2023-04-26  2:34   ` Yoshitaka Ikeda
  2023-04-26  7:39     ` Dhruva Gole
  0 siblings, 1 reply; 16+ messages in thread
From: Yoshitaka Ikeda @ 2023-04-26  2:34 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown, Yoshitaka Ikeda

Hi Dhruva,

An error occurred in the following environments where this patch was applied, and the flash could not be accessed.
However, after reverting to this patch, it is possible to access it.

- Environment
  - OS : Linux 6.3
  - SoC : Cyclone V
  - Flash : MT25QL512A

- Error at startup
  - Kernel log
[ 0.980290] spi-nor spi0.0: found mt25ql512a, expected n25q512a
[ 1.485140] cadence-qspi ff705000.flash: Flash command execution timed out.
[ 1.490792] spi-nor spi0.0: operation failed with -110
[ 1.494654] spi-nor spi0.0: mt25ql512a (65536 Kbytes)

- Error at access
  - Access command and log
# hexdump -Cv /dev/mtdblock0
hexdump: /dev/mtdblock0: Input/output error

  - Kernel log
[ 2124.201193] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[ 2124.201229] spi-nor spi0.0: operation failed with -110
[ 2124.201256] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 4 prio class 2
[ 2124.711241] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[ 2124.711276] spi-nor spi0.0: operation failed with -110
[ 2124.711302] I/O error, dev mtdblock0, sector 8 op 0x0:(READ) flags 0x80700 phys_seg 3 prio class 2
[ 2125.221193] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[ 2125.221230] spi-nor spi0.0: operation failed with -110
[ 2125.221256] I/O error, dev mtdblock0, sector 16 op 0x0:(READ) flags 0x80700 phys_seg 2 prio class 2
[ 2125.731237] cadence-qspi ff705000.flash:. QSPI is still busy after 500ms timeout.
[ 2125.731270] spi-nor spi0.0: operation failed with -110
[ 2125.731296] I/O error, dev mtdblock0, sector 24 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
[ 2126.241190] cadence-qspi ff705000.flash:. QSPI is still busy after 500ms timeout.
[ 2126.241224] spi-nor spi0.0: operation failed with -110
[ 2126.241251] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2
[ 2126.241274] Buffer I/O error on dev mtdblock0, logical block 0, async page read


regards,
Yoshitaka Ikeda


-----Original Message-----
From: Dhruva Gole <d-gole@ti.com> 
Sent: Wednesday, January 25, 2023 5:10 PM
To: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Vignesh Raghavendra <vigneshr@ti.com>; Vaishnav Achath <vaishnav.a@ti.com>; Dhruva Gole <d-gole@ti.com>; linux-mtd@lists.infradead.org; linux-arm-kernel@lists.infradead.org; Takahiro.Kuwano@infineon.com; Pratyush Yadav <ptyadav@amazon.de>
Subject: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads

Fix the issue where some flash chips like cypress S25HS256T return the value of the same register over and over in DAC mode.

For example in the TI K3-AM62x Processors refer [0] Technical Reference Manual there is a layer of digital logic in front of the QSPI/OSPI Drive when used in DAC mode. This is part of the Flash Subsystem (FSS) which provides access to external Flash devices.

The FSS0_0_SYSCONFIG Register (Offset = 4h) has a BIT Field for OSPI_32B_DISABLE_MODE which has a Reset value = 0. This means, OSPI 32bit mode enabled by default.

Thus, by default controller operates in 32 bit mode causing it to always align all data to 4 bytes from a 4byte aligned address. In some flash chips like cypress for example if we try to read some regs in DAC mode then it keeps sending the value of the first register that was requested and inorder to read the next reg, we have to stop and re-initiate a new transaction.

This causes wrong register values to be read than what is desired when registers are read in DAC mode. Hence if the data.nbytes is very less then prefer STIG mode for such small reads.

[0] https://www.ti.com/lit/ug/spruiv7a/spruiv7a.pdf

Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index a6d0f1b0bb49..2954c06a7f57 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1359,7 +1359,13 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 	cqspi_configure(f_pdata, mem->spi->max_speed_hz);

 	if (op->data.dir == SPI_MEM_DATA_IN && op->data.buf.in) {
-		if (!op->addr.nbytes)
+	/*
+	 * Performing reads in DAC mode forces to read minimum 4 bytes
+	 * which is unsupported on some flash devices during register
+	 * reads, prefer STIG mode for such small reads.
+	 */
+		if (!op->addr.nbytes ||
+		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)
 			return cqspi_command_read(f_pdata, op);

 		return cqspi_read(f_pdata, op);
--
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-04-26  2:34   ` Yoshitaka Ikeda
@ 2023-04-26  7:39     ` Dhruva Gole
  2023-04-27  0:41       ` Yoshitaka Ikeda
  0 siblings, 1 reply; 16+ messages in thread
From: Dhruva Gole @ 2023-04-26  7:39 UTC (permalink / raw)
  To: Yoshitaka Ikeda
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown

Hi Yoshitaka,

On 26/04/23 08:04, Yoshitaka Ikeda wrote:
> Hi Dhruva,
> 
> An error occurred in the following environments where this patch was applied, and the flash could not be accessed.
> However, after reverting to this patch, it is possible to access it.
> 
> - Environment
>    - OS : Linux 6.3
>    - SoC : Cyclone V

Please can you send me the register fields information for the
CQSPI controller used in this device?
I wanted to verify if atall there were any mismatch between the
controller I have tested with vs your SOC's controller.

>    - Flash : MT25QL512A
> 
> - Error at startup
>    - Kernel log
> [ 0.980290] spi-nor spi0.0: found mt25ql512a, expected n25q512a
> [ 1.485140] cadence-qspi ff705000.flash: Flash command execution timed out.
> [ 1.490792] spi-nor spi0.0: operation failed with -110
> [ 1.494654] spi-nor spi0.0: mt25ql512a (65536 Kbytes)
> 
> - Error at access
>    - Access command and log
> # hexdump -Cv /dev/mtdblock0
> hexdump: /dev/mtdblock0: Input/output error
> 
>    - Kernel log
> [ 2124.201193] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
> [ 2124.201229] spi-nor spi0.0: operation failed with -110
> [ 2124.201256] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 4 prio class 2
> [ 2124.711241] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
> [ 2124.711276] spi-nor spi0.0: operation failed with -110
> [ 2124.711302] I/O error, dev mtdblock0, sector 8 op 0x0:(READ) flags 0x80700 phys_seg 3 prio class 2
> [ 2125.221193] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
> [ 2125.221230] spi-nor spi0.0: operation failed with -110
> [ 2125.221256] I/O error, dev mtdblock0, sector 16 op 0x0:(READ) flags 0x80700 phys_seg 2 prio class 2
> [ 2125.731237] cadence-qspi ff705000.flash:. QSPI is still busy after 500ms timeout.
> [ 2125.731270] spi-nor spi0.0: operation failed with -110
> [ 2125.731296] I/O error, dev mtdblock0, sector 24 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
> [ 2126.241190] cadence-qspi ff705000.flash:. QSPI is still busy after 500ms timeout.
> [ 2126.241224] spi-nor spi0.0: operation failed with -110
> [ 2126.241251] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2
> [ 2126.241274] Buffer I/O error on dev mtdblock0, logical block 0, async page read
> 
> 
> regards,
> Yoshitaka Ikeda
> 
> 
> [..snip..]

It maybe that STIG mode may have some different way of configuring/using
on your SoC.

However at this point it's only a guess and I will wait till I have the
exact information on the controller specs being used in this device
along with the register descriptions.

-- 
Thanks and Regards,
Dhruva Gole

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-04-26  7:39     ` Dhruva Gole
@ 2023-04-27  0:41       ` Yoshitaka Ikeda
  2023-04-27 13:25         ` Dhruva Gole
  0 siblings, 1 reply; 16+ messages in thread
From: Yoshitaka Ikeda @ 2023-04-27  0:41 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown, Yoshitaka Ikeda

Hi Dhruva,

> Please can you send me the register fields information for the CQSPI
> controller used in this device?
> I wanted to verify if atall there were any mismatch between the controller I
> have tested with vs your SOC's controller.

You can find it in the pdf that can be obtained by pressing the download button in the upper right corner of the following website.
- https://www.intel.com/content/www/us/en/docs/programmable/683126/15-0/introduction.html

The description is as follows:
- 15. Quad SPI Flash
  - Quad SPI Flash Controller Address Map and Register Definitions

-- 
Thanks and Regards,
Yoshitaka Ikeda


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-04-27  0:41       ` Yoshitaka Ikeda
@ 2023-04-27 13:25         ` Dhruva Gole
  2023-05-08  0:36           ` Yoshitaka Ikeda
  0 siblings, 1 reply; 16+ messages in thread
From: Dhruva Gole @ 2023-04-27 13:25 UTC (permalink / raw)
  To: Yoshitaka Ikeda
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown

Hi,

On 27/04/23 06:11, Yoshitaka Ikeda wrote:
> Hi Dhruva,
> 
>> Please can you send me the register fields information for the CQSPI
>> controller used in this device?
>> I wanted to verify if atall there were any mismatch between the controller I
>> have tested with vs your SOC's controller.
> 
> You can find it in the pdf that can be obtained by pressing the download button in the upper right corner of the following website.
> - https://www.intel.com/content/www/us/en/docs/programmable/683126/15-0/introduction.html

Thanks for sharing, I went through and couldn't really find any major
changes at the controller level.
So I am wondering if some specific sequence of flash operations on your 
device is exposing some issues in the driver's STIG reads.


Please can you share some logs with the following patch:
I am trying to see a pattern that may be causing issues.
I am unable to reproduce this on my end at the moment.

...
diff --git a/drivers/spi/spi-cadence-quadspi.c 
b/drivers/spi/spi-cadence-quadspi.c
index 6ddb2dfc0f00..5fa71dbd37ef 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1390,6 +1390,14 @@ static int cqspi_exec_mem_op(struct spi_mem *mem, 
const struct spi_mem_op *op)
         int ret;

         ret = cqspi_mem_process(mem, op);
+       /* debug */
+       printk("**********spi_mem_op dump**************");
+       printk("addr: nbytes:0x%X , buswidth 0x%X, dtr 0x%X, val 
0x%llX", op->addr.nbytes, op->addr.buswidth, op->addr.dtr, op->addr.val);
+       printk("cmd: nbytes:0x%X , buswidth 0x%X, dtr 0x%X, opcode 
0x%X", op->cmd.nbytes, op->cmd.buswidth, op->cmd.dtr, op->cmd.opcode);
+       printk("data: nbytes:0x%X , buswidth 0x%X, dtr 0x%X, data dir 
0x%X", op->data.nbytes, op->data.buswidth, op->data.dtr, op->data.dir);
+
+       printk("***************************************");
+       /* ---------------------------- */
         if (ret)
                 dev_err(&mem->spi->dev, "operation failed with %d\n", ret);


...

-- 
Thanks and Regards,
Dhruva Gole

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-04-27 13:25         ` Dhruva Gole
@ 2023-05-08  0:36           ` Yoshitaka Ikeda
  2023-05-08  5:29             ` Dhruva Gole
  0 siblings, 1 reply; 16+ messages in thread
From: Yoshitaka Ikeda @ 2023-05-08  0:36 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown

Hi Dhruva,

Sorry for the late reply as I was on a long vacation.

> Thanks for sharing, I went through and couldn't really find any major changes
> at the controller level.
> So I am wondering if some specific sequence of flash operations on your
> device is exposing some issues in the driver's STIG reads.

Thank you for looking into this.

> Please can you share some logs with the following patch:
> I am trying to see a pattern that may be causing issues.
> I am unable to reproduce this on my end at the moment.

The logs obtained with the patch applied are as follows:

- Error at startup
  - Kernel log
[    0.980598] **********spi_mem_op dump**************
[    0.980613] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
[    0.984223] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
[    0.988656] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
[    0.993362] ***************************************
[    0.998329] spi-nor spi0.0: found mt25ql512a, expected n25q512a
[    1.006574] **********spi_mem_op dump**************
[    1.006583] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
[    1.010150] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
[    1.014596] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
[    1.019285] ***************************************
[    1.524271] cadence-qspi ff705000.flash: Flash command execution timed out.
[    1.533483] **********spi_mem_op dump**************
[    1.533489] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10
[    1.537055] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
[    1.541579] data: nbytes:0x8 , buswidth 0x1, dtr 0x0, data dir 0x1
[    1.546266] ***************************************
[    1.551123] spi-nor spi0.0: operation failed with -110
[    1.558531] spi-nor spi0.0: mt25ql512a (65536 Kbytes)

- Error at access
  - Access command and log
# hexdump -Cv /dev/mtdblock0
hexdump: /dev/mtdblock0: Input/output error

  - Kernel log
[   75.401297] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[   75.401337] **********spi_mem_op dump**************
[   75.401343] addr: nbytes:0x4 , buswidth 0x1, dtr 0x0, val 0x0
[   75.401354] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0xC
[   75.401364] data: nbytes:0x200 , buswidth 0x1, dtr 0x0, data dir 0x1
[   75.401373] ***************************************
[   75.401382] spi-nor spi0.0: operation failed with -110
[   75.401407] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 4 prio class 2
[   75.911339] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[   75.911372] **********spi_mem_op dump**************
[   75.911379] addr: nbytes:0x4 , buswidth 0x1, dtr 0x0, val 0x1000
[   75.911390] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0xC
[   75.911399] data: nbytes:0x200 , buswidth 0x1, dtr 0x0, data dir 0x1
[   75.911409] ***************************************
[   75.911417] spi-nor spi0.0: operation failed with -110
[   75.911443] I/O error, dev mtdblock0, sector 8 op 0x0:(READ) flags 0x80700 phys_seg 3 prio class 2
[   76.421333] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[   76.421365] **********spi_mem_op dump**************
[   76.421371] addr: nbytes:0x4 , buswidth 0x1, dtr 0x0, val 0x2000
[   76.421383] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0xC
[   76.421393] data: nbytes:0x200 , buswidth 0x1, dtr 0x0, data dir 0x1
[   76.421403] ***************************************
[   76.421411] spi-nor spi0.0: operation failed with -110
[   76.421436] I/O error, dev mtdblock0, sector 16 op 0x0:(READ) flags 0x80700 phys_seg 2 prio class 2
[   76.931330] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[   76.931365] **********spi_mem_op dump**************
[   76.931371] addr: nbytes:0x4 , buswidth 0x1, dtr 0x0, val 0x3000
[   76.931382] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0xC
[   76.931392] data: nbytes:0x200 , buswidth 0x1, dtr 0x0, data dir 0x1
[   76.931401] ***************************************
[   76.931410] spi-nor spi0.0: operation failed with -110
[   76.931437] I/O error, dev mtdblock0, sector 24 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
[   77.441297] cadence-qspi ff705000.flash: QSPI is still busy after 500ms timeout.
[   77.441330] **********spi_mem_op dump**************
[   77.441337] addr: nbytes:0x4 , buswidth 0x1, dtr 0x0, val 0x0
[   77.441347] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0xC
[   77.441357] data: nbytes:0x200 , buswidth 0x1, dtr 0x0, data dir 0x1
[   77.441367] ***************************************
[   77.441375] spi-nor spi0.0: operation failed with -110
[   77.441401] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2
[   77.441423] Buffer I/O error on dev mtdblock0, logical block 0, async page read

-- 
Thanks and Regards,
Yoshitaka Ikeda

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-05-08  0:36           ` Yoshitaka Ikeda
@ 2023-05-08  5:29             ` Dhruva Gole
  2023-05-08  7:44               ` Yoshitaka Ikeda
  0 siblings, 1 reply; 16+ messages in thread
From: Dhruva Gole @ 2023-05-08  5:29 UTC (permalink / raw)
  To: Yoshitaka Ikeda
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown



On 08/05/23 06:06, Yoshitaka Ikeda wrote:
> Hi Dhruva,
> 
> Sorry for the late reply as I was on a long vacation.
> 
>> Thanks for sharing, I went through and couldn't really find any major changes
>> at the controller level.
>> So I am wondering if some specific sequence of flash operations on your
>> device is exposing some issues in the driver's STIG reads.
> 
> Thank you for looking into this.
> 
>> Please can you share some logs with the following patch:
>> I am trying to see a pattern that may be causing issues.
>> I am unable to reproduce this on my end at the moment.
> 
> The logs obtained with the patch applied are as follows:

Thanks for sharing the logs. It has made me concerned about something,

> 
> - Error at startup
>    - Kernel log
> [    0.980598] **********spi_mem_op dump**************
> [    0.980613] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
> [    0.984223] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
> [    0.988656] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    0.993362] ***************************************
> [    0.998329] spi-nor spi0.0: found mt25ql512a, expected n25q512a
> [    1.006574] **********spi_mem_op dump**************
> [    1.006583] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
> [    1.010150] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> [    1.014596] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    1.019285] ***************************************
> [    1.524271] cadence-qspi ff705000.flash: Flash command execution timed out.

This print message is from cqspi_exec_flash_cmd. This function should
only be called from cqspi_command_read/write .

However, from spi_mem_op dump that you have provided above,
where addr.nbytes is 3 and data.nbytes is 0x10 (which is > 8)
it should never have entered the cqspi_command_read function.

> [    1.533483] **********spi_mem_op dump**************
> [    1.533489] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10
> [    1.537055] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> [    1.541579] data: nbytes:0x8 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    1.546266] ***************************************
> [    1.551123] spi-nor spi0.0: operation failed with -110
> [    1.558531] spi-nor spi0.0: mt25ql512a (65536 Kbytes)
> 

Anything after the Flash command execution timed out step seems
irrelevant because at that point the controller is in some weird state 
that it never again comes out of.


Please can you share the exact output of uname -a where you observe this
error?

I am now wondering why atall would the following conditions be satisfied
in your case:

if (!op->addr.nbytes || op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)

either addr.nbytes needs to be 0, which it isn't.
or data has to be <= 8 (which again doesn't seem to be the case here)

So it should never enter the above "if" chunk.

My logs for reference:
https://gist.github.com/DhruvaG2000/7185a84de5757e4988f93478f6b75289


Are you carrying any sort of local patches? Can you make sure that the
CQSPI_STIG_DATA_LEN_MAX is 8 in your case too?
Any other delta from linux-next that I am using in the logs I gave
above?

-- 
Thanks and Regards,
Dhruva Gole

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-05-08  5:29             ` Dhruva Gole
@ 2023-05-08  7:44               ` Yoshitaka Ikeda
  2023-05-30  2:56                 ` Yoshitaka Ikeda
  0 siblings, 1 reply; 16+ messages in thread
From: Yoshitaka Ikeda @ 2023-05-08  7:44 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown

Hi Dhruva,

> > - Error at startup
> >    - Kernel log
> > [    0.980598] **********spi_mem_op dump**************
> > [    0.980613] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
> > [    0.984223] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
> > [    0.988656] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
> > [    0.993362] ***************************************
> > [    0.998329] spi-nor spi0.0: found mt25ql512a, expected n25q512a
> > [    1.006574] **********spi_mem_op dump**************
> > [    1.006583] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
> > [    1.010150] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> > [    1.014596] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
> > [    1.019285] ***************************************
> > [    1.524271] cadence-qspi ff705000.flash: Flash command execution
> timed out.
> 
> This print message is from cqspi_exec_flash_cmd. This function should only
> be called from cqspi_command_read/write .
> 
> However, from spi_mem_op dump that you have provided above, where
> addr.nbytes is 3 and data.nbytes is 0x10 (which is > 8) it should never have
> entered the cqspi_command_read function.

The location of the log output is after the call to cqspi_mem_process(), so I assume it is called with the following data.

[ 1.533483] **********spi_mem_op dump**************
[ 1.533489] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10
[ 1.537055] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
[ 1.541579] data: nbytes:0x8 , buswidth 0x1, dtr 0x0, data dir 0x1
[ 1.546266] ***************************************

Thus, the condition is met and cqspi_command_read() is called.
We have also applied the following patch and confirmed that it is called.

- patch
```
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 64b6a460d739..8d3681e1f35c 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1365,15 +1365,21 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 	 * reads, prefer STIG mode for such small reads.
 	 */
 		if (!op->addr.nbytes ||
-		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)
+		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX) {
+			printk("----- %d:%s() (1) -----\n", __LINE__, __func__);
 			return cqspi_command_read(f_pdata, op);
+		}
 
+		printk("----- %d:%s() (2) -----\n", __LINE__, __func__);
 		return cqspi_read(f_pdata, op);
 	}
 
-	if (!op->addr.nbytes || !op->data.buf.out)
+	if (!op->addr.nbytes || !op->data.buf.out) {
+		printk("----- %d:%s() (3) -----\n", __LINE__, __func__);
 		return cqspi_command_write(f_pdata, op);
+	}
 
+	printk("----- %d:%s() (4) -----\n", __LINE__, __func__);
 	return cqspi_write(f_pdata, op);
 }

```

- log
```
[    0.786600] ----- 1369:cqspi_mem_process() (1) -----
[    0.790312] **********spi_mem_op dump**************
[    0.790319] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
[    0.793885] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
[    0.798325] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
[    0.803013] ***************************************
[    0.807892] spi-nor spi0.0: found mt25ql512a, expected n25q512a
[    0.816062] ----- 1373:cqspi_mem_process() (2) -----
[    0.819775] **********spi_mem_op dump**************
[    0.819782] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
[    0.823348] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
[    0.827792] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
[    0.832480] ***************************************
[    0.837442] ----- 1369:cqspi_mem_process() (1) -----
[    1.344671] cadence-qspi ff705000.flash: Flash command execution timed out.
[    1.350328] **********spi_mem_op dump**************
[    1.350334] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10
[    1.353901] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
[    1.358427] data: nbytes:0x8 , buswidth 0x1, dtr 0x0, data dir 0x1
[    1.363114] ***************************************
[    1.367981] spi-nor spi0.0: operation failed with -110
[    1.375376] spi-nor spi0.0: mt25ql512a (65536 Kbytes)
```

> Please can you share the exact output of uname -a where you observe this
> error?

My environment has shortened the output of uname -a, so it may not be useful.
Instead, I will give you information about the kernel I am using.
It is the following (v6.3):
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=457391b0380335d5e9a5babdec90ac53928b23b4

> Are you carrying any sort of local patches? Can you make sure that the
> CQSPI_STIG_DATA_LEN_MAX is 8 in your case too?

There is no local patch and CQSPI_STIG_DATA_LEN_MAX remains 8.

-- 
Thanks and Regards,
Yoshitaka Ikeda

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads
  2023-05-08  7:44               ` Yoshitaka Ikeda
@ 2023-05-30  2:56                 ` Yoshitaka Ikeda
  0 siblings, 0 replies; 16+ messages in thread
From: Yoshitaka Ikeda @ 2023-05-30  2:56 UTC (permalink / raw)
  To: Dhruva Gole
  Cc: linux-spi, linux-kernel, Vignesh Raghavendra, Vaishnav Achath,
	linux-mtd, linux-arm-kernel, Takahiro.Kuwano, Pratyush Yadav,
	Mark Brown

Hi Dhruva,

The patch described below was able to avoid this problem.
However, we could not find any material to determine if it was correct.


Dear all,

Is there anyone else in a similar environment?


```
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 8f36e1306e16..4a9535e6c86a 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1365,7 +1365,9 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 	 * reads, prefer STIG mode for such small reads.
 	 */
 		if (!op->addr.nbytes ||
-		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)
+		    (op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX &&
+			!of_device_is_compatible(cqspi->pdev->dev.of_node,
+					    "intel,socfpga-qspi")))
 			return cqspi_command_read(f_pdata, op);
 
 		return cqspi_read(f_pdata, op);
```

Thanks and Regards,
Yoshitaka Ikeda



> -----Original Message-----
> From: Yoshitaka Ikeda <ikeda@nskint.co.jp>
> Sent: Monday, May 8, 2023 4:44 PM
> To: Dhruva Gole <d-gole@ti.com>
> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; Vignesh
> Raghavendra <vigneshr@ti.com>; Vaishnav Achath <vaishnav.a@ti.com>;
> linux-mtd@lists.infradead.org; linux-arm-kernel@lists.infradead.org;
> Takahiro.Kuwano@infineon.com; Pratyush Yadav <ptyadav@amazon.de>;
> Mark Brown <broonie@kernel.org>
> Subject: RE: [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small
> reads
> 
> Hi Dhruva,
> 
> > > - Error at startup
> > >    - Kernel log
> > > [    0.980598] **********spi_mem_op dump**************
> > > [    0.980613] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
> > > [    0.984223] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
> > > [    0.988656] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
> > > [    0.993362] ***************************************
> > > [    0.998329] spi-nor spi0.0: found mt25ql512a, expected n25q512a
> > > [    1.006574] **********spi_mem_op dump**************
> > > [    1.006583] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
> > > [    1.010150] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> > > [    1.014596] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
> > > [    1.019285] ***************************************
> > > [    1.524271] cadence-qspi ff705000.flash: Flash command execution
> > timed out.
> >
> > This print message is from cqspi_exec_flash_cmd. This function should
> > only be called from cqspi_command_read/write .
> >
> > However, from spi_mem_op dump that you have provided above, where
> > addr.nbytes is 3 and data.nbytes is 0x10 (which is > 8) it should
> > never have entered the cqspi_command_read function.
> 
> The location of the log output is after the call to cqspi_mem_process(), so I
> assume it is called with the following data.
> 
> [ 1.533483] **********spi_mem_op dump************** [ 1.533489] addr:
> nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10 [ 1.537055] cmd: nbytes:0x1 ,
> buswidth 0x1, dtr 0x0, opcode 0x5A [ 1.541579] data: nbytes:0x8 , buswidth 0x1,
> dtr 0x0, data dir 0x1 [ 1.546266] ***************************************
> 
> Thus, the condition is met and cqspi_command_read() is called.
> We have also applied the following patch and confirmed that it is called.
> 
> - patch
> ```
> diff --git a/drivers/spi/spi-cadence-quadspi.c
> b/drivers/spi/spi-cadence-quadspi.c
> index 64b6a460d739..8d3681e1f35c 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1365,15 +1365,21 @@ static int cqspi_mem_process(struct spi_mem
> *mem, const struct spi_mem_op *op)
>  	 * reads, prefer STIG mode for such small reads.
>  	 */
>  		if (!op->addr.nbytes ||
> -		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX)
> +		    op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX) {
> +			printk("----- %d:%s() (1) -----\n", __LINE__,
> __func__);
>  			return cqspi_command_read(f_pdata, op);
> +		}
> 
> +		printk("----- %d:%s() (2) -----\n", __LINE__, __func__);
>  		return cqspi_read(f_pdata, op);
>  	}
> 
> -	if (!op->addr.nbytes || !op->data.buf.out)
> +	if (!op->addr.nbytes || !op->data.buf.out) {
> +		printk("----- %d:%s() (3) -----\n", __LINE__, __func__);
>  		return cqspi_command_write(f_pdata, op);
> +	}
> 
> +	printk("----- %d:%s() (4) -----\n", __LINE__, __func__);
>  	return cqspi_write(f_pdata, op);
>  }
> 
> ```
> 
> - log
> ```
> [    0.786600] ----- 1369:cqspi_mem_process() (1) -----
> [    0.790312] **********spi_mem_op dump**************
> [    0.790319] addr: nbytes:0x0 , buswidth 0x0, dtr 0x0, val 0x0
> [    0.793885] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x9F
> [    0.798325] data: nbytes:0x6 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    0.803013] ***************************************
> [    0.807892] spi-nor spi0.0: found mt25ql512a, expected n25q512a
> [    0.816062] ----- 1373:cqspi_mem_process() (2) -----
> [    0.819775] **********spi_mem_op dump**************
> [    0.819782] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x0
> [    0.823348] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> [    0.827792] data: nbytes:0x10 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    0.832480] ***************************************
> [    0.837442] ----- 1369:cqspi_mem_process() (1) -----
> [    1.344671] cadence-qspi ff705000.flash: Flash command execution timed
> out.
> [    1.350328] **********spi_mem_op dump**************
> [    1.350334] addr: nbytes:0x3 , buswidth 0x1, dtr 0x0, val 0x10
> [    1.353901] cmd: nbytes:0x1 , buswidth 0x1, dtr 0x0, opcode 0x5A
> [    1.358427] data: nbytes:0x8 , buswidth 0x1, dtr 0x0, data dir 0x1
> [    1.363114] ***************************************
> [    1.367981] spi-nor spi0.0: operation failed with -110
> [    1.375376] spi-nor spi0.0: mt25ql512a (65536 Kbytes)
> ```
> 
> > Please can you share the exact output of uname -a where you observe
> > this error?
> 
> My environment has shortened the output of uname -a, so it may not be useful.
> Instead, I will give you information about the kernel I am using.
> It is the following (v6.3):
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id
> =457391b0380335d5e9a5babdec90ac53928b23b4
> 
> > Are you carrying any sort of local patches? Can you make sure that the
> > CQSPI_STIG_DATA_LEN_MAX is 8 in your case too?
> 
> There is no local patch and CQSPI_STIG_DATA_LEN_MAX remains 8.
> 
> --
> Thanks and Regards,
> Yoshitaka Ikeda

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-05-30  2:56 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-25  8:10 [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Dhruva Gole
2023-01-25  8:10 ` [PATCH v2 1/4] spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion Dhruva Gole
2023-01-27 15:16   ` Pratyush Yadav
2023-02-07 13:06     ` Gole, Dhruva
2023-01-25  8:10 ` [PATCH v2 2/4] spi: cadence-quadspi: Add flag for direct mode writes Dhruva Gole
2023-01-25  8:10 ` [PATCH v2 3/4] spi: cadence-quadspi: setup ADDR Bits in cmd reads Dhruva Gole
2023-01-25  8:10 ` [PATCH v2 4/4] spi: cadence-quadspi: use STIG mode for small reads Dhruva Gole
2023-04-26  2:34   ` Yoshitaka Ikeda
2023-04-26  7:39     ` Dhruva Gole
2023-04-27  0:41       ` Yoshitaka Ikeda
2023-04-27 13:25         ` Dhruva Gole
2023-05-08  0:36           ` Yoshitaka Ikeda
2023-05-08  5:29             ` Dhruva Gole
2023-05-08  7:44               ` Yoshitaka Ikeda
2023-05-30  2:56                 ` Yoshitaka Ikeda
2023-02-14 21:10 ` [PATCH v2 0/4] STIG Mode Fixes for spi-cadence-qspi driver Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).