[PATCH 0/2] spi: cadence_qspi: Fixes for DTR ops and improve STIG support

[PATCH 0/2] spi: cadence_qspi: Fixes for DTR ops and improve STIG support

[Dhruva Gole]

This series aims to address some critical bugs in the cadence qspi
driver like the need to Flush the CMDCTRL reg after the execution due to
a hardware limitation and also fixes the check conditions for DTR ops.

Logs with sf read and update commands:
https://gist.github.com/DhruvaG2000/874b3eb683bba92f25d7fe49d09f76fa

Dhruva Gole (2):
  spi: cadence-quadspi: Fix check condition for DTR ops
  spi: cadence-quadspi: Use STIG mode for all ops with small payload

 drivers/spi/cadence_qspi.c     | 16 ++++++----
 drivers/spi/cadence_qspi_apb.c | 53 ++++++++++++++++++++--------------
 2 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.25.1

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

[Dhruva Gole]

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,
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);
+
 	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
 		    !op->data.dtr;
 
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);
 
 	ret = cadence_qspi_buswidth_to_inst_type(op->cmd.buswidth);
 	if (ret < 0)
-- 
2.25.1

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

[Pratyush Yadav]

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

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

[Dhruva Gole]

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 ||
-		    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)
 			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);
 	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;
-	}
-
 	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;
-	}
-
-	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;
+
 	if (txlen) {
 		/* writing data = yes */
 		reg |= (0x1 << CQSPI_REG_CMDCTRL_WR_EN_LSB);
-- 
2.25.1

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

[Pratyush Yadav]

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