On Thu, Mar 23 2023, Dhruva Gole 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,

Right.

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 | 9 ++++++++- 2 files changed, 17 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);
  

Looks good!

•   all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
                !op->data.dtr;
  

Since we treat DTR as "do not care" when the phase does not exist, you should check for that here as well. What if someone _sets_ DTR for a field with nbytes == 0? This check would fail.

Since no one reasonable should do this, I will not insist upon this. Fine by me if you don't do this. Your choice.

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 21fe2e655c5f..2b04b58124a5 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -120,7 +120,14 @@ 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.
  

•    */
  

•   priv->dtr = op->cmd.dtr &&
  

•               (!op->addr.nbytes || op->addr.dtr) &&
  

•               (!op->data.nbytes || op->data.dtr);
  

Why not check dummy? Since supports_op() already checks that _all_ or _none_ of the fields are DTR, you can get away with just checking for op->cmd.dtr here (do add a comment here or it can be a bit confusing).

BTW, I think it would be better if you get rid of cadence_qspi_set_protocol() entirely. I see no point in carrying the state around. Wherever you use priv->dtr or priv->inst_width, etc. you also have access to the spi_mem_op. You can derive that information from the op. Something to fix when you have some free time on your hands. Will make the driver a bit simpler.

    ret = cadence_qspi_buswidth_to_inst_type(op->cmd.buswidth);
    if (ret < 0)

-- 2.25.1

On Thu, Mar 23 2023, Dhruva Gole wrote:

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.

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 | 44 ++++++++++++++++++---------------- 2 files changed, 26 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 ||
  

What if someone sends us a command that has no address phase but reads more than CQSPI_STIG_DATA_LEN_MAX bytes? You would happily send it to cadence_qspi_apb_read_setup(), which would then do reg |= (op->addr.nbytes - 1) causing an underflow and having corrputing the register.

Since there is no way to execute a command with no address phase and data bytes > CQSPI_STIG_DATA_LEN_MAX, you should add a check for this in the supports_op() hook.

•               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)
  

Same here.

                    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 2b04b58124a5..25b5fc292e07 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -374,6 +374,8 @@ 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);
  

Do this in a separate patch with its own explanation please.

    return 0;

}

@@ -460,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;
  

•   }
  

Ok.

    if (priv->dtr)
            opcode = op->cmd.opcode >> 8;
    else

@@ -547,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;
  

•   }
  

You should explain why you are removing this.

•   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
  

@@ -574,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;
  

Ok.

    if (txlen) {
            /* writing data = yes */
            reg |= (0x1 << CQSPI_REG_CMDCTRL_WR_EN_LSB);