[PATCH 1/2] spi: cadence-quadspi: Fix check condition for DTR ops
Pratyush Yadav
ptyadav at amazon.de
Mon Mar 27 17:19:10 CEST 2023
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 at ti.com>
> Signed-off-by: Dhruva Gole <d-gole at 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
More information about the U-Boot
mailing list