[PATCH V3 0/2] spi: cqspi: Fix register reads in STIG Mode
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.
This series supercedes the previously sent: https://lore.kernel.org/u-boot/20221115114926.174351-1-d-gole@ti.com/
Logs demonstrating the usage and working of QSPI-NOR Flash (Cypress s25hs512t) can be found on the link below: https://gist.github.com/DhruvaG2000/11e7b4ee6a381be9d86b69b2bc2616e4
change log: ----------
v3: Improvements over the last 2 versions, I have added a bit mask to make sure nbytes dont overflow bit fields. Also minor improvements in wording of comments and commit log.
v2: add the setup ADDR bits patch because STIG read of registers wasn't present earlier.
v1: use STIG mode if reads are small.
Dhruva Gole (2): spi: cadence_qspi: setup ADDR Bits in cmd reads spi: cadence_qspi: use STIG mode for small reads
drivers/spi/cadence_qspi.c | 8 +++++++- drivers/spi/cadence_qspi_apb.c | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
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/cadence_qspi_apb.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index cfae5dcbda0e..45c1333e2d13 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -479,6 +479,19 @@ int cadence_qspi_apb_command_read(struct cadence_spi_priv *priv, /* 0 means 1 byte. */ reg |= (((rxlen - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK) << CQSPI_REG_CMDCTRL_RD_BYTES_LSB); + + /* setup ADDR BIT field */ + if (op->addr.nbytes) { + writel(op->addr.val, priv->regbase + CQSPI_REG_CMDADDRESS); + /* + * address bytes are zero indexed + */ + reg |= (((op->addr.nbytes - 1) & + CQSPI_REG_CMDCTRL_ADD_BYTES_MASK) << + CQSPI_REG_CMDCTRL_ADD_BYTES_LSB); + reg |= (0x1 << CQSPI_REG_CMDCTRL_ADDR_EN_LSB); + } + status = cadence_qspi_apb_exec_flash_cmd(reg_base, reg); if (status != 0) return status;
On 25/11/22 11:29AM, Dhruva Gole wrote:
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
Reviewed-by: Pratyush Yadav pratyush@kernel.org
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. This operates by default in a 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 registers 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/cadence_qspi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index ab0a681c8376..5506f63ef078 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -307,7 +307,13 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi, priv->is_decoded_cs);
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) mode = CQSPI_STIG_READ; else mode = CQSPI_READ;
On 25/11/22 11:29, 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. This operates by default in a 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 registers 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/cadence_qspi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index ab0a681c8376..5506f63ef078 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -307,7 +307,13 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi, priv->is_decoded_cs);
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) mode = CQSPI_STIG_READ;
For the series,
Tested-by: Vaishnav Achath vaishnav.a@ti.com
Erase-Write-Read back tested on s28hs512t, mt35xu512aba, mt25qu512a on J7200 and J721E platforms.
else mode = CQSPI_READ;
On 25/11/22 11:29AM, 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
I know where to find the useful information in this 12 thousand page long document since I used to work at TI. But other people might not find it so easy. It would be good to point to specific section numbers here to make this easier to understand and review. Also, this document does not contain the register map for the CQSPI controller. You should link that too.
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. This operates by default in a 32 bit mode causing it to always align all data to 4 bytes
Is this new in AM62x? I do not remember seeing this on J7 class devices. I remember being able to read all registers from Cypress S28 flash without having to do anything special.
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 registers 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/cadence_qspi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index ab0a681c8376..5506f63ef078 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -307,7 +307,13 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi, priv->is_decoded_cs);
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)
<= instead?
mode = CQSPI_STIG_READ; else mode = CQSPI_READ;-- 2.25.1
Hey Pratyush, Thanks for reviewing.
On 13/12/22 05:05, Pratyush Yadav wrote:
On 25/11/22 11:29AM, 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
I know where to find the useful information in this 12 thousand page long document since I used to work at TI. But other people might not find it so easy. It would be good to point to specific section numbers
I will try to be more specific about section numbers in future, however since this fix applies across any SoC's out there I thought not to go into too much detail just for AM625. I feared it may send a wrong message that this fix is ONLY and ONLY relevant to AM62x which might not necessarily be the case. Also, I have mentioned further below about it being "part of the Flash Subsystem (FSS)". Hence in my humble opinion should be enough to point which section to look into, but if someone really wanted to deep dive and see for themselves they still could go through the FSS part of the TRM.
here to make this easier to understand and review. Also, this document does not contain the register map for the CQSPI controller. You should link that too.
I believe that it does have the reg map for OSPI. Kindly refer to FSS_OSPI_0_config_reg and onwards, (Addresses 0FC4 0000h onwards)
It's under 12.4.6 Memory Interfaces Registers: FSS_OSPI_0 Registers
I am on revision: SPRUIV7A – MAY 2022 – REVISED NOVEMBER 2022
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. This operates by default in a 32 bit mode causing it to always align all data to 4 bytes
Is this new in AM62x? I do not remember seeing this on J7 class devices. I remember being able to read all registers from Cypress S28 flash without having to do anything special.
Taking a look at FSS0_0_SYSCONFIG Register (Offset = 4h) [reset = 0h ]
The Table 12-2293. SYSCONFIG Register Field Descriptions tells us,
OSPI_32B_DISABLE_MODE: Reset value = 0 and this means, 0: OSPI 32bit mode enabled by default,
So rather than complicating things to enable/ disable 32 bit mode all around, this fix should avoid major restructuring.
[...]
if (!op->addr.nbytes ||op->data.nbytes < CQSPI_STIG_DATA_LEN_MAX)<= instead?
This would be fine too, just wanted to ensure we enter this condition for small reads only. Registers are generally only 32 bit long (4Bytes) and CQSPI_STIG_DATA_LEN_MAX is 8Bytes so it's anyway more than a register read would really need.
mode = CQSPI_STIG_READ; else mode = CQSPI_READ;-- 2.25.1
Hi,
A Gentle reminder for this series.
On 25/11/22 11:29, Dhruva Gole wrote:
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.
This series supercedes the previously sent: https://lore.kernel.org/u-boot/20221115114926.174351-1-d-gole@ti.com/
Logs demonstrating the usage and working of QSPI-NOR Flash (Cypress s25hs512t) can be found on the link below: https://gist.github.com/DhruvaG2000/11e7b4ee6a381be9d86b69b2bc2616e4
[...] Dhruva Gole (2): spi: cadence_qspi: setup ADDR Bits in cmd reads spi: cadence_qspi: use STIG mode for small reads
drivers/spi/cadence_qspi.c | 8 +++++++- drivers/spi/cadence_qspi_apb.c | 13 +++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
It's an important fix because without this reading flash registers is a problem in some cases. I was unable to get one of the QSPI Flash from Cypress working just because STIG read of registers was not supported.
It would be great if we could get this merged for the coming release.
participants (3)
-
Dhruva Gole -
Pratyush Yadav -
Vaishnav Achath