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

[Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>]

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