[PATCH 0/2] spi: cqspi: Fix register reads in STIG Mode

[PATCH 0/2] spi: cqspi: Fix register reads in STIG Mode

[Dhruva Gole]

Intent of these patches is to 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.

Logs demonstrating the usage and working of QSPI-NOR Flash (Cypress
s25hs512t) on a modified AM625 SK EVM can be found on the link below:
https://gist.github.com/DhruvaG2000/a9b90d3d9c60edd3b2d8a360d869a00b

A series very similar to this was also sent to u-boot and the latest
revision can be viewed here:
[PATCH V4 0/2] spi: cqspi: Fix register reads in STIG Mode
https://lore.kernel.org/u-boot/20230103063112.1165898-1-d-gole@ti.com/


Dhruva Gole (2):
  spi: cadence-quadspi: setup ADDR Bits in cmd reads
  spi: cadence-quadspi: use STIG mode for small reads

 drivers/spi/spi-cadence-quadspi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] spi: cadence-quadspi: setup ADDR Bits in cmd reads

[Dhruva Gole]

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 676313e1bdad..8c7938776cfc 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -531,6 +531,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

[PATCH 2/2] spi: cadence-quadspi: use STIG mode for small reads

[Dhruva Gole]

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 8c7938776cfc..f5188dc52db6 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1344,7 +1344,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

Re: [PATCH 2/2] spi: cadence-quadspi: use STIG mode for small reads

[Vaishnav Achath]

Hi Dhruva,

On 04/01/23 11:56, Dhruva Gole wrote:
> 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 8c7938776cfc..f5188dc52db6 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1344,7 +1344,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);
>  

I am seeing issues while testing after applying this series,

On J7200 EVM,
[    2.164655] spi-nor spi7.0: Failed to parse optional parameter table: ff81
[    2.171644] spi-nor spi7.0: s28hs512t (65536 Kbytes)

On J721E EVM,
[    5.565961] spi-nor spi7.0: mt35xu512aba (0 Kbytes)
[    5.732084] spi-nor spi8.0: mt25qu512a (81753 Kbytes)

In all the three cases above, the behavior is normal without these patches.

>  		return cqspi_read(f_pdata, op);

-- 
Regards,
Vaishnav