[PATCH 2/2] spi: cadence-quadspi: Use STIG mode for all ops with small payload
Pratyush Yadav
ptyadav at amazon.de
Mon Mar 27 17:50:42 CEST 2023
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 at ti.com>
> Signed-off-by: Dhruva Gole <d-gole at 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
More information about the U-Boot
mailing list