Re: [PATCH 2/2] spi: cadence-quadspi: Use STIG mode for all ops with small payload

[Pratyush Yadav <ptyadav@amazon.de>]

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

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