[PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Matthias Schiffer]

cqspi_set_protocol() only set the data width, but ignored the command
and address width (except for 8-8-8 DTR ops), leading to corruption of
all transfers using 1-X-X or X-X-X ops. Fix by setting the other two
widths as well.

While we're at it, simplify the code a bit by replacing the
CQSPI_INST_TYPE_* constants with ilog2().

Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-4-4
read and write operations.

Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/spi/spi-cadence-quadspi.c | 46 ++++++++-----------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index b0c9f62ccefb..616ada891974 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -19,6 +19,7 @@
 #include <linux/iopoll.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of.h>
@@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
 #define CQSPI_TIMEOUT_MS			500
 #define CQSPI_READ_TIMEOUT_MS			10
 
-/* Instruction type */
-#define CQSPI_INST_TYPE_SINGLE			0
-#define CQSPI_INST_TYPE_DUAL			1
-#define CQSPI_INST_TYPE_QUAD			2
-#define CQSPI_INST_TYPE_OCTAL			3
-
 #define CQSPI_DUMMY_CLKS_PER_BYTE		8
 #define CQSPI_DUMMY_BYTES_MAX			4
 #define CQSPI_DUMMY_CLKS_MAX			31
@@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr)
 static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
 			      const struct spi_mem_op *op)
 {
-	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
-	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
-	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
-
 	/*
 	 * 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
@@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
 		       (!op->addr.nbytes || op->addr.dtr) &&
 		       (!op->data.nbytes || op->data.dtr);
 
-	switch (op->data.buswidth) {
-	case 0:
-		break;
-	case 1:
-		f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
-		break;
-	case 2:
-		f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
-		break;
-	case 4:
-		f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
-		break;
-	case 8:
-		f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
-		break;
-	default:
-		return -EINVAL;
-	}
+	f_pdata->inst_width = 0;
+	if (op->cmd.buswidth)
+		f_pdata->inst_width = ilog2(op->cmd.buswidth);
+
+	f_pdata->addr_width = 0;
+	if (op->addr.buswidth)
+		f_pdata->addr_width = ilog2(op->addr.buswidth);
+
+	f_pdata->data_width = 0;
+	if (op->data.buswidth)
+		f_pdata->data_width = ilog2(op->data.buswidth);
 
 	/* Right now we only support 8-8-8 DTR mode. */
 	if (f_pdata->dtr) {
 		switch (op->cmd.buswidth) {
 		case 0:
-			break;
 		case 8:
-			f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL;
 			break;
 		default:
 			return -EINVAL;
@@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
 
 		switch (op->addr.buswidth) {
 		case 0:
-			break;
 		case 8:
-			f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL;
 			break;
 		default:
 			return -EINVAL;
@@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
 
 		switch (op->data.buswidth) {
 		case 0:
-			break;
 		case 8:
-			f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
 			break;
 		default:
 			return -EINVAL;
-- 
2.25.1

Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Pratyush Yadav]

Hi Matthias,

On 31/03/22 01:08PM, Matthias Schiffer wrote:
> cqspi_set_protocol() only set the data width, but ignored the command
> and address width (except for 8-8-8 DTR ops), leading to corruption of
> all transfers using 1-X-X or X-X-X ops. Fix by setting the other two
> widths as well.
> 
> While we're at it, simplify the code a bit by replacing the
> CQSPI_INST_TYPE_* constants with ilog2().
> 
> Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-4-4
> read and write operations.
> 
> Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework")

I think a fixes tag is wrong here. The old driver did not support 1-X-X 
modes either. So you are not fixing anything, you are adding a new 
feature. I don't think we should backport this patch to stable.

> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 46 ++++++++-----------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index b0c9f62ccefb..616ada891974 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -19,6 +19,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of.h>
> @@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
>  #define CQSPI_TIMEOUT_MS			500
>  #define CQSPI_READ_TIMEOUT_MS			10
>  
> -/* Instruction type */
> -#define CQSPI_INST_TYPE_SINGLE			0
> -#define CQSPI_INST_TYPE_DUAL			1
> -#define CQSPI_INST_TYPE_QUAD			2
> -#define CQSPI_INST_TYPE_OCTAL			3
> -
>  #define CQSPI_DUMMY_CLKS_PER_BYTE		8
>  #define CQSPI_DUMMY_BYTES_MAX			4
>  #define CQSPI_DUMMY_CLKS_MAX			31
> @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr)
>  static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
>  			      const struct spi_mem_op *op)
>  {
> -	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> -	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> -	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> -
>  	/*
>  	 * 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
> @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
>  		       (!op->addr.nbytes || op->addr.dtr) &&
>  		       (!op->data.nbytes || op->data.dtr);
>  
> -	switch (op->data.buswidth) {
> -	case 0:
> -		break;
> -	case 1:
> -		f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> -		break;
> -	case 2:
> -		f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> -		break;
> -	case 4:
> -		f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> -		break;
> -	case 8:
> -		f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	f_pdata->inst_width = 0;
> +	if (op->cmd.buswidth)
> +		f_pdata->inst_width = ilog2(op->cmd.buswidth);
> +
> +	f_pdata->addr_width = 0;
> +	if (op->addr.buswidth)
> +		f_pdata->addr_width = ilog2(op->addr.buswidth);
> +
> +	f_pdata->data_width = 0;
> +	if (op->data.buswidth)
> +		f_pdata->data_width = ilog2(op->data.buswidth);

Honestly, I think we should get rid of cqspi_set_protocol() entirely. I 
see no need to store f_pdata->{instr,addr,data}_width since we 
recalculate those for each op execution anyway. So why not just use the 
spi_mem_op to get those values directly and be rid of all this mess?

>  
>  	/* Right now we only support 8-8-8 DTR mode. */
>  	if (f_pdata->dtr) {
>  		switch (op->cmd.buswidth) {
>  		case 0:
> -			break;
>  		case 8:
> -			f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL;
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
>  
>  		switch (op->addr.buswidth) {
>  		case 0:
> -			break;
>  		case 8:
> -			f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL;
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
>  
>  		switch (op->data.buswidth) {
>  		case 0:
> -			break;
>  		case 8:
> -			f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
>  			break;
>  		default:
>  			return -EINVAL;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Matthias Schiffer]

On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote:
> Hi Matthias,
> 
> On 31/03/22 01:08PM, Matthias Schiffer wrote:
> > cqspi_set_protocol() only set the data width, but ignored the
> > command
> > and address width (except for 8-8-8 DTR ops), leading to corruption
> > of
> > all transfers using 1-X-X or X-X-X ops. Fix by setting the other
> > two
> > widths as well.
> > 
> > While we're at it, simplify the code a bit by replacing the
> > CQSPI_INST_TYPE_* constants with ilog2().
> > 
> > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-
> > 4-4
> > read and write operations.
> > 
> > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use
> > spi-mem framework")
> 
> I think a fixes tag is wrong here. The old driver did not support 1-
> X-X 
> modes either. So you are not fixing anything, you are adding a new 
> feature. I don't think we should backport this patch to stable.


Giving a precise fixes tag is a bit difficult. The referenced commit
made the driver (accidentally) accept commands like 1-4-4 without
handing them correctly, causing data corruption for flashs that support
these modes. The data corruption is fixed by my patch.

As the change was unintended, one option would be to split this patch
into two parts: One fix patch that makes cqspi_set_protocol() -EINVAL
again for all commands that are not 1-1-X, and one feature patch that
adds actual support for these commands.

My thought process was that making these commands work correctly can't
break anything that is not already broken in current stable kernels.
But if you prefer the minimal change, I can send a v2 that splits the
patch.

Regards,
Matthias



> 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > >
> > ---
> >  drivers/spi/spi-cadence-quadspi.c | 46 ++++++++-------------------
> > ----
> >  1 file changed, 12 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-
> > cadence-quadspi.c
> > index b0c9f62ccefb..616ada891974 100644
> > --- a/drivers/spi/spi-cadence-quadspi.c
> > +++ b/drivers/spi/spi-cadence-quadspi.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/iopoll.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/kernel.h>
> > +#include <linux/log2.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of.h>
> > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
> >  #define CQSPI_TIMEOUT_MS			500
> >  #define CQSPI_READ_TIMEOUT_MS			10
> >  
> > -/* Instruction type */
> > -#define CQSPI_INST_TYPE_SINGLE			0
> > -#define CQSPI_INST_TYPE_DUAL			1
> > -#define CQSPI_INST_TYPE_QUAD			2
> > -#define CQSPI_INST_TYPE_OCTAL			3
> > -
> >  #define CQSPI_DUMMY_CLKS_PER_BYTE		8
> >  #define CQSPI_DUMMY_BYTES_MAX			4
> >  #define CQSPI_DUMMY_CLKS_MAX			31
> > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const
> > struct spi_mem_op *op, bool dtr)
> >  static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
> >  			      const struct spi_mem_op *op)
> >  {
> > -	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> > -	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> > -	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > -
> >  	/*
> >  	 * 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
> > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct
> > cqspi_flash_pdata *f_pdata,
> >  		       (!op->addr.nbytes || op->addr.dtr) &&
> >  		       (!op->data.nbytes || op->data.dtr);
> >  
> > -	switch (op->data.buswidth) {
> > -	case 0:
> > -		break;
> > -	case 1:
> > -		f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > -		break;
> > -	case 2:
> > -		f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> > -		break;
> > -	case 4:
> > -		f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> > -		break;
> > -	case 8:
> > -		f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > -		break;
> > -	default:
> > -		return -EINVAL;
> > -	}
> > +	f_pdata->inst_width = 0;
> > +	if (op->cmd.buswidth)
> > +		f_pdata->inst_width = ilog2(op->cmd.buswidth);
> > +
> > +	f_pdata->addr_width = 0;
> > +	if (op->addr.buswidth)
> > +		f_pdata->addr_width = ilog2(op->addr.buswidth);
> > +
> > +	f_pdata->data_width = 0;
> > +	if (op->data.buswidth)
> > +		f_pdata->data_width = ilog2(op->data.buswidth);
> 
> Honestly, I think we should get rid of cqspi_set_protocol() entirely.
> I 
> see no need to store f_pdata->{instr,addr,data}_width since we 
> recalculate those for each op execution anyway. So why not just use
> the 
> spi_mem_op to get those values directly and be rid of all this mess?
> 
> >  
> >  	/* Right now we only support 8-8-8 DTR mode. */
> >  	if (f_pdata->dtr) {
> >  		switch (op->cmd.buswidth) {
> >  		case 0:
> > -			break;
> >  		case 8:
> > -			f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL;
> >  			break;
> >  		default:
> >  			return -EINVAL;
> > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct
> > cqspi_flash_pdata *f_pdata,
> >  
> >  		switch (op->addr.buswidth) {
> >  		case 0:
> > -			break;
> >  		case 8:
> > -			f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL;
> >  			break;
> >  		default:
> >  			return -EINVAL;
> > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct
> > cqspi_flash_pdata *f_pdata,
> >  
> >  		switch (op->data.buswidth) {
> >  		case 0:
> > -			break;
> >  		case 8:
> > -			f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> >  			break;
> >  		default:
> >  			return -EINVAL;
> > -- 
> > 2.25.1
> > 

Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Pratyush Yadav]

On 01/04/22 12:20PM, Matthias Schiffer wrote:
> On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote:
> > Hi Matthias,
> > 
> > On 31/03/22 01:08PM, Matthias Schiffer wrote:
> > > cqspi_set_protocol() only set the data width, but ignored the
> > > command
> > > and address width (except for 8-8-8 DTR ops), leading to corruption
> > > of
> > > all transfers using 1-X-X or X-X-X ops. Fix by setting the other
> > > two
> > > widths as well.
> > > 
> > > While we're at it, simplify the code a bit by replacing the
> > > CQSPI_INST_TYPE_* constants with ilog2().
> > > 
> > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-
> > > 4-4
> > > read and write operations.
> > > 
> > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use
> > > spi-mem framework")
> > 
> > I think a fixes tag is wrong here. The old driver did not support 1-
> > X-X 
> > modes either. So you are not fixing anything, you are adding a new 
> > feature. I don't think we should backport this patch to stable.
> 
> 
> Giving a precise fixes tag is a bit difficult. The referenced commit
> made the driver (accidentally) accept commands like 1-4-4 without
> handing them correctly, causing data corruption for flashs that support
> these modes. The data corruption is fixed by my patch.

Ah, you're right. I missed the fact that the original driver explicitly 
checked for 1-1-1, 1-1-4, and 1-1-8, and returned an error for the rest. 
So your patch does indeed fix a bug.

> 
> As the change was unintended, one option would be to split this patch
> into two parts: One fix patch that makes cqspi_set_protocol() -EINVAL
> again for all commands that are not 1-1-X, and one feature patch that
> adds actual support for these commands.
> 
> My thought process was that making these commands work correctly can't
> break anything that is not already broken in current stable kernels.
> But if you prefer the minimal change, I can send a v2 that splits the
> patch.

Yes, but as I commented below, I would prefer you rework the driver to 
drop cqspi_set_protocol() entirely. For doing that the 2 patch approach 
would work best so we don't end up getting the new code backported to 
stable as well.

> 
> Regards,
> Matthias
> 
> 
> 
> > 
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > > >
> > > ---
> > >  drivers/spi/spi-cadence-quadspi.c | 46 ++++++++-------------------
> > > ----
> > >  1 file changed, 12 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-
> > > cadence-quadspi.c
> > > index b0c9f62ccefb..616ada891974 100644
> > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/iopoll.h>
> > >  #include <linux/jiffies.h>
> > >  #include <linux/kernel.h>
> > > +#include <linux/log2.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/of.h>
> > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
> > >  #define CQSPI_TIMEOUT_MS			500
> > >  #define CQSPI_READ_TIMEOUT_MS			10
> > >  
> > > -/* Instruction type */
> > > -#define CQSPI_INST_TYPE_SINGLE			0
> > > -#define CQSPI_INST_TYPE_DUAL			1
> > > -#define CQSPI_INST_TYPE_QUAD			2
> > > -#define CQSPI_INST_TYPE_OCTAL			3
> > > -
> > >  #define CQSPI_DUMMY_CLKS_PER_BYTE		8
> > >  #define CQSPI_DUMMY_BYTES_MAX			4
> > >  #define CQSPI_DUMMY_CLKS_MAX			31
> > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const
> > > struct spi_mem_op *op, bool dtr)
> > >  static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
> > >  			      const struct spi_mem_op *op)
> > >  {
> > > -	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> > > -	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> > > -	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > -
> > >  	/*
> > >  	 * 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
> > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > >  		       (!op->addr.nbytes || op->addr.dtr) &&
> > >  		       (!op->data.nbytes || op->data.dtr);
> > >  
> > > -	switch (op->data.buswidth) {
> > > -	case 0:
> > > -		break;
> > > -	case 1:
> > > -		f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > -		break;
> > > -	case 2:
> > > -		f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> > > -		break;
> > > -	case 4:
> > > -		f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> > > -		break;
> > > -	case 8:
> > > -		f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > > -		break;
> > > -	default:
> > > -		return -EINVAL;
> > > -	}
> > > +	f_pdata->inst_width = 0;
> > > +	if (op->cmd.buswidth)
> > > +		f_pdata->inst_width = ilog2(op->cmd.buswidth);
> > > +
> > > +	f_pdata->addr_width = 0;
> > > +	if (op->addr.buswidth)
> > > +		f_pdata->addr_width = ilog2(op->addr.buswidth);
> > > +
> > > +	f_pdata->data_width = 0;
> > > +	if (op->data.buswidth)
> > > +		f_pdata->data_width = ilog2(op->data.buswidth);
> > 
> > Honestly, I think we should get rid of cqspi_set_protocol() entirely.
> > I 
> > see no need to store f_pdata->{instr,addr,data}_width since we 
> > recalculate those for each op execution anyway. So why not just use
> > the 
> > spi_mem_op to get those values directly and be rid of all this mess?
> > 
> > >  
> > >  	/* Right now we only support 8-8-8 DTR mode. */
> > >  	if (f_pdata->dtr) {
> > >  		switch (op->cmd.buswidth) {
> > >  		case 0:
> > > -			break;
> > >  		case 8:
> > > -			f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL;
> > >  			break;
> > >  		default:
> > >  			return -EINVAL;
> > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > >  
> > >  		switch (op->addr.buswidth) {
> > >  		case 0:
> > > -			break;
> > >  		case 8:
> > > -			f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL;
> > >  			break;
> > >  		default:
> > >  			return -EINVAL;
> > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > >  
> > >  		switch (op->data.buswidth) {
> > >  		case 0:
> > > -			break;
> > >  		case 8:
> > > -			f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > >  			break;
> > >  		default:
> > >  			return -EINVAL;
> > > -- 
> > > 2.25.1
> > > 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Matthias Schiffer]

On Wed, 2022-04-06 at 01:07 +0530, Pratyush Yadav wrote:
> On 01/04/22 12:20PM, Matthias Schiffer wrote:
> > On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote:
> > > Hi Matthias,
> > > 
> > > On 31/03/22 01:08PM, Matthias Schiffer wrote:
> > > > cqspi_set_protocol() only set the data width, but ignored the
> > > > command
> > > > and address width (except for 8-8-8 DTR ops), leading to
> > > > corruption
> > > > of
> > > > all transfers using 1-X-X or X-X-X ops. Fix by setting the
> > > > other
> > > > two
> > > > widths as well.
> > > > 
> > > > While we're at it, simplify the code a bit by replacing the
> > > > CQSPI_INST_TYPE_* constants with ilog2().
> > > > 
> > > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash
> > > > with 1-
> > > > 4-4
> > > > read and write operations.
> > > > 
> > > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to
> > > > use
> > > > spi-mem framework")
> > > 
> > > I think a fixes tag is wrong here. The old driver did not support
> > > 1-
> > > X-X 
> > > modes either. So you are not fixing anything, you are adding a
> > > new 
> > > feature. I don't think we should backport this patch to stable.
> > 
> > Giving a precise fixes tag is a bit difficult. The referenced
> > commit
> > made the driver (accidentally) accept commands like 1-4-4 without
> > handing them correctly, causing data corruption for flashs that
> > support
> > these modes. The data corruption is fixed by my patch.
> 
> Ah, you're right. I missed the fact that the original driver
> explicitly 
> checked for 1-1-1, 1-1-4, and 1-1-8, and returned an error for the
> rest. 
> So your patch does indeed fix a bug.
> 
> > As the change was unintended, one option would be to split this
> > patch
> > into two parts: One fix patch that makes cqspi_set_protocol()
> > -EINVAL
> > again for all commands that are not 1-1-X, and one feature patch
> > that
> > adds actual support for these commands.
> > 
> > My thought process was that making these commands work correctly
> > can't
> > break anything that is not already broken in current stable
> > kernels.
> > But if you prefer the minimal change, I can send a v2 that splits
> > the
> > patch.
> 
> Yes, but as I commented below, I would prefer you rework the driver
> to 
> drop cqspi_set_protocol() entirely. For doing that the 2 patch
> approach 
> would work best so we don't end up getting the new code backported
> to 
> stable as well.

Okay, I tried to split my patch, however I found
that cqspi_set_protocol() is not the right place to check for
unsupported operations at all - if the driver ever reaches a path where
cqspi_set_protocol() returns -EINVAL, something has already gone wrong
(and the attemption operation will fail altogether rather than falling
back to a slower command).

I think that these checks - including the check that only 8-8-8 is
supported with DTR - should actually happen in cqspi_supports_mem_op().
Does that sound right?

Regards,
Matthias



> 
> > Regards,
> > Matthias
> > 
> > 
> > 
> > > > Signed-off-by: Matthias Schiffer <
> > > > matthias.schiffer@ew.tq-group.com
> > > > ---
> > > >  drivers/spi/spi-cadence-quadspi.c | 46 ++++++++---------------
> > > > ----
> > > > ----
> > > >  1 file changed, 12 insertions(+), 34 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-cadence-quadspi.c
> > > > b/drivers/spi/spi-
> > > > cadence-quadspi.c
> > > > index b0c9f62ccefb..616ada891974 100644
> > > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/iopoll.h>
> > > >  #include <linux/jiffies.h>
> > > >  #include <linux/kernel.h>
> > > > +#include <linux/log2.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of_device.h>
> > > >  #include <linux/of.h>
> > > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
> > > >  #define CQSPI_TIMEOUT_MS			500
> > > >  #define CQSPI_READ_TIMEOUT_MS			10
> > > >  
> > > > -/* Instruction type */
> > > > -#define CQSPI_INST_TYPE_SINGLE			0
> > > > -#define CQSPI_INST_TYPE_DUAL			1
> > > > -#define CQSPI_INST_TYPE_QUAD			2
> > > > -#define CQSPI_INST_TYPE_OCTAL			3
> > > > -
> > > >  #define CQSPI_DUMMY_CLKS_PER_BYTE		8
> > > >  #define CQSPI_DUMMY_BYTES_MAX			4
> > > >  #define CQSPI_DUMMY_CLKS_MAX			31
> > > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const
> > > > struct spi_mem_op *op, bool dtr)
> > > >  static int cqspi_set_protocol(struct cqspi_flash_pdata
> > > > *f_pdata,
> > > >  			      const struct spi_mem_op *op)
> > > >  {
> > > > -	f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> > > > -	f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> > > > -	f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > > -
> > > >  	/*
> > > >  	 * 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
> > > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct
> > > > cqspi_flash_pdata *f_pdata,
> > > >  		       (!op->addr.nbytes || op->addr.dtr) &&
> > > >  		       (!op->data.nbytes || op->data.dtr);
> > > >  
> > > > -	switch (op->data.buswidth) {
> > > > -	case 0:
> > > > -		break;
> > > > -	case 1:
> > > > -		f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > > -		break;
> > > > -	case 2:
> > > > -		f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> > > > -		break;
> > > > -	case 4:
> > > > -		f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> > > > -		break;
> > > > -	case 8:
> > > > -		f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > > > -		break;
> > > > -	default:
> > > > -		return -EINVAL;
> > > > -	}
> > > > +	f_pdata->inst_width = 0;
> > > > +	if (op->cmd.buswidth)
> > > > +		f_pdata->inst_width = ilog2(op->cmd.buswidth);
> > > > +
> > > > +	f_pdata->addr_width = 0;
> > > > +	if (op->addr.buswidth)
> > > > +		f_pdata->addr_width = ilog2(op->addr.buswidth);
> > > > +
> > > > +	f_pdata->data_width = 0;
> > > > +	if (op->data.buswidth)
> > > > +		f_pdata->data_width = ilog2(op->data.buswidth);
> > > 
> > > Honestly, I think we should get rid of cqspi_set_protocol()
> > > entirely.
> > > I 
> > > see no need to store f_pdata->{instr,addr,data}_width since we 
> > > recalculate those for each op execution anyway. So why not just
> > > use
> > > the 
> > > spi_mem_op to get those values directly and be rid of all this
> > > mess?
> > > 
> > > >  
> > > >  	/* Right now we only support 8-8-8 DTR mode. */
> > > >  	if (f_pdata->dtr) {
> > > >  		switch (op->cmd.buswidth) {
> > > >  		case 0:
> > > > -			break;
> > > >  		case 8:
> > > > -			f_pdata->inst_width =
> > > > CQSPI_INST_TYPE_OCTAL;
> > > >  			break;
> > > >  		default:
> > > >  			return -EINVAL;
> > > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct
> > > > cqspi_flash_pdata *f_pdata,
> > > >  
> > > >  		switch (op->addr.buswidth) {
> > > >  		case 0:
> > > > -			break;
> > > >  		case 8:
> > > > -			f_pdata->addr_width =
> > > > CQSPI_INST_TYPE_OCTAL;
> > > >  			break;
> > > >  		default:
> > > >  			return -EINVAL;
> > > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct
> > > > cqspi_flash_pdata *f_pdata,
> > > >  
> > > >  		switch (op->data.buswidth) {
> > > >  		case 0:
> > > > -			break;
> > > >  		case 8:
> > > > -			f_pdata->data_width =
> > > > CQSPI_INST_TYPE_OCTAL;
> > > >  			break;
> > > >  		default:
> > > >  			return -EINVAL;
> > > > -- 
> > > > 2.25.1
> > > > 

Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Mark Brown]

On Thu, 31 Mar 2022 13:08:19 +0200, Matthias Schiffer wrote:
> cqspi_set_protocol() only set the data width, but ignored the command
> and address width (except for 8-8-8 DTR ops), leading to corruption of
> all transfers using 1-X-X or X-X-X ops. Fix by setting the other two
> widths as well.
> 
> While we're at it, simplify the code a bit by replacing the
> CQSPI_INST_TYPE_* constants with ilog2().
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations
      commit: 97e4827d775faa9a32b5e1a97959c69dd77d17a3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark