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-nan...@ti.com> > Signed-off-by: Dhruva Gole <d-g...@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); -- 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