[PATCH V2 0/3] spi: cadence_qspi: Fixes for DTR ops and improve STIG support
This series aims to address some critical bugs in the cadence qspi driver like the need to Flush the CMDCTRL reg after the execution due to a hardware limitation and also fixes the check conditions for DTR ops.
previously posted: https://lore.kernel.org/u-boot/20230323164408.1043725-1-d-gole@ti.com/
Changelog: * Preserve Apurva's authorship * Split the cmdctrl reg clearing patch into a seperate one * Address few other comments from Pratyush on the previous series
Logs (on AM62x SK EVM):
... U-Boot 2023.04-rc5-00589-g1a9171bfbc66 (Apr 12 2023 - 16:16:19 +0530)
SoC: AM62X SR1.0 HS-FS Model: Texas Instruments AM625 SK DRAM: 2 GiB Core: 50 devices, 20 uclasses, devicetree: separate MMC: mmc@fa10000: 0, mmc@fa00000: 1, mmc@fa20000: 2 Loading Environment from nowhere... OK In: serial@2800000 Out: serial@2800000 Err: serial@2800000 Net: eth0: ethernet@8000000port@1 Hit any key to stop autoboot: 0 => => sf probe SF: Detected s28hs512t with page size 256 Bytes, erase size 256 KiB, total 64 MiB => random $loadaddr 0x200000 2097152 bytes filled with random data => sf update $loadaddr 0x0 0x200000 device 0 offset 0x0, size 0x200000 2097152 bytes written, 0 bytes skipped in 13.365s, speed 160643 B/s => sf read 0x90000000 0x0 0x200000 device 0 offset 0x0, size 0x200000 SF: 2097152 bytes @ 0x0 Read: OK => cmp.b $loadaddr 0x90000000 0x200000 Total of 2097152 byte(s) were the same
local git log: ... 1a9171bfbc66 (HEAD -> next) spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion f60c2702c9df spi: cadence-quadspi: Use STIG mode for all ops with small payload f84f1b610f90 spi: cadence-quadspi: Fix check condition for DTR ops a25dcda452bf (origin/next) Revert "disk: Use a helper function to reduce duplication" ...
Apurva Nandan (2): spi: cadence-quadspi: Fix check condition for DTR ops spi: cadence-quadspi: Use STIG mode for all ops with small payload
Dhruva Gole (1): spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
drivers/spi/cadence_qspi.c | 16 +++++++--- drivers/spi/cadence_qspi_apb.c | 56 +++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 26 deletions(-)
From: Apurva Nandan a-nandan@ti.com
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() 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 Signed-off-by: Dhruva Gole d-gole@ti.com --- drivers/spi/cadence_qspi.c | 11 +++++++++-- drivers/spi/cadence_qspi_apb.c | 11 ++++++++++- 2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index c7f10c501320..a858a62888e4 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -362,8 +362,15 @@ static bool cadence_spi_mem_supports_op(struct spi_slave *slave, { 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;
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 21fe2e655c5f..dfcdeff80545 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -120,7 +120,16 @@ static int cadence_qspi_set_protocol(struct cadence_spi_priv *priv, { int ret;
- priv->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. + * Also, dummy checks not performed here Since supports_op() + * already checks that all or none of the fields are DTR. + */ + priv->dtr = op->cmd.dtr && + (!op->addr.nbytes || op->addr.dtr) && + (!op->data.nbytes || op->data.dtr);
ret = cadence_qspi_buswidth_to_inst_type(op->cmd.buswidth); if (ret < 0)
From: Apurva Nandan a-nandan@ti.com
OSPI controller supports all types of op variants in STIG mode, only limitation being that the data payload should be less than 8 bytes when not using memory banks.
STIG mode is more stable for operations that send small data payload and is more efficient than using DMA for few bytes of memory accesses. It overcomes the limitation of minimum 4 bytes read from flash into RAM seen in DAC mode.
Use STIG mode for all read and write operations that require data input/output of less than 8 bytes from the flash, and thereby support all four phases, cmd/address/dummy/data, through OSPI STIG.
Also, remove the reorder address chunk in apb_command_write since we now setup ADDR BIT field that does the same job in a cleaner way.
Signed-off-by: Apurva Nandan a-nandan@ti.com Signed-off-by: Dhruva Gole d-gole@ti.com --- drivers/spi/cadence_qspi.c | 5 ++-- drivers/spi/cadence_qspi_apb.c | 42 ++++++++++++++++++---------------- 2 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index a858a62888e4..f931e4cf3e2f 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -312,13 +312,12 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi, * 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) + if (op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX) mode = CQSPI_STIG_READ; else mode = CQSPI_READ; } else { - if (!op->addr.nbytes || !op->data.buf.out) + if (op->data.nbytes <= CQSPI_STIG_DATA_LEN_MAX) mode = CQSPI_STIG_WRITE; else mode = CQSPI_WRITE; diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index dfcdeff80545..4c055a05807e 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -462,11 +462,6 @@ int cadence_qspi_apb_command_read(struct cadence_spi_priv *priv, unsigned int dummy_clk; u8 opcode;
- if (rxlen > CQSPI_STIG_DATA_LEN_MAX || !rxbuf) { - printf("QSPI: Invalid input arguments rxlen %u\n", rxlen); - return -EINVAL; - } - if (priv->dtr) opcode = op->cmd.opcode >> 8; else @@ -549,26 +544,12 @@ int cadence_qspi_apb_command_write(struct cadence_spi_priv *priv, unsigned int reg = 0; unsigned int wr_data; unsigned int wr_len; + unsigned int dummy_clk; unsigned int txlen = op->data.nbytes; const void *txbuf = op->data.buf.out; void *reg_base = priv->regbase; - u32 addr; u8 opcode;
- /* Reorder address to SPI bus order if only transferring address */ - if (!txlen) { - addr = cpu_to_be32(op->addr.val); - if (op->addr.nbytes == 3) - addr >>= 8; - txbuf = &addr; - txlen = op->addr.nbytes; - } - - if (txlen > CQSPI_STIG_DATA_LEN_MAX) { - printf("QSPI: Invalid input arguments txlen %u\n", txlen); - return -EINVAL; - } - if (priv->dtr) opcode = op->cmd.opcode >> 8; else @@ -576,6 +557,27 @@ int cadence_qspi_apb_command_write(struct cadence_spi_priv *priv,
reg |= opcode << CQSPI_REG_CMDCTRL_OPCODE_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); + } + + /* Set up dummy cycles. */ + dummy_clk = cadence_qspi_calc_dummy(op, priv->dtr); + if (dummy_clk > CQSPI_DUMMY_CLKS_MAX) + return -EOPNOTSUPP; + + if (dummy_clk) + reg |= (dummy_clk & CQSPI_REG_CMDCTRL_DUMMY_MASK) + << CQSPI_REG_CMDCTRL_DUMMY_LSB; + if (txlen) { /* writing data = yes */ reg |= (0x1 << CQSPI_REG_CMDCTRL_WR_EN_LSB);
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.
Co-developed-by: Apurva Nandan a-nandan@ti.com Signed-off-by: Apurva Nandan a-nandan@ti.com Signed-off-by: Dhruva Gole d-gole@ti.com --- drivers/spi/cadence_qspi_apb.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 4c055a05807e..9ce2c0f254f3 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -376,6 +376,9 @@ int cadence_qspi_apb_exec_flash_cmd(void *reg_base, unsigned int reg) if (!cadence_qspi_wait_idle(reg_base)) return -EIO;
+ /* Flush the CMDCTRL reg after the execution */ + writel(0, reg_base + CQSPI_REG_CMDCTRL); + return 0; }
On Wed, Apr 12, 2023 at 4:29 PM Dhruva Gole d-gole@ti.com wrote:
This series aims to address some critical bugs in the cadence qspi driver like the need to Flush the CMDCTRL reg after the execution due to a hardware limitation and also fixes the check conditions for DTR ops.
previously posted: https://lore.kernel.org/u-boot/20230323164408.1043725-1-d-gole@ti.com/
Changelog:
- Preserve Apurva's authorship
- Split the cmdctrl reg clearing patch into a seperate one
- Address few other comments from Pratyush on the previous series
Logs (on AM62x SK EVM):
... U-Boot 2023.04-rc5-00589-g1a9171bfbc66 (Apr 12 2023 - 16:16:19 +0530)
SoC: AM62X SR1.0 HS-FS Model: Texas Instruments AM625 SK DRAM: 2 GiB Core: 50 devices, 20 uclasses, devicetree: separate MMC: mmc@fa10000: 0, mmc@fa00000: 1, mmc@fa20000: 2 Loading Environment from nowhere... OK In: serial@2800000 Out: serial@2800000 Err: serial@2800000 Net: eth0: ethernet@8000000port@1 Hit any key to stop autoboot: 0 => => sf probe SF: Detected s28hs512t with page size 256 Bytes, erase size 256 KiB, total 64 MiB => random $loadaddr 0x200000 2097152 bytes filled with random data => sf update $loadaddr 0x0 0x200000 device 0 offset 0x0, size 0x200000 2097152 bytes written, 0 bytes skipped in 13.365s, speed 160643 B/s => sf read 0x90000000 0x0 0x200000 device 0 offset 0x0, size 0x200000 SF: 2097152 bytes @ 0x0 Read: OK => cmp.b $loadaddr 0x90000000 0x200000 Total of 2097152 byte(s) were the same
local git log: ... 1a9171bfbc66 (HEAD -> next) spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion f60c2702c9df spi: cadence-quadspi: Use STIG mode for all ops with small payload f84f1b610f90 spi: cadence-quadspi: Fix check condition for DTR ops a25dcda452bf (origin/next) Revert "disk: Use a helper function to reduce duplication" ...
Apurva Nandan (2): spi: cadence-quadspi: Fix check condition for DTR ops spi: cadence-quadspi: Use STIG mode for all ops with small payload
Dhruva Gole (1): spi: cadence-quadspi: Reset CMD_CTRL Reg on cmd r/w completion
drivers/spi/cadence_qspi.c | 16 +++++++--- drivers/spi/cadence_qspi_apb.c | 56 +++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 26 deletions(-)
Applied to u-boot-spi/master
participants (2)
-
Dhruva Gole -
Jagan Teki