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

[Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>]

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