Re: [PATCH 1/2] spi: cadence-quadspi: Fix check condition for DTR ops

[Pratyush Yadav <ptyadav@amazon.de>]

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
>

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