Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 6 Apr 2022 01:07:21 +0530
From:   Pratyush Yadav <p.yadav@ti.com>
To:     Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
CC:     Mark Brown <broonie@kernel.org>,
        Tudor Ambarus <tudor.ambarus@microchip.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Ramuthevar Vadivel Murugan 
        <vadivel.muruganx.ramuthevar@linux.intel.com>,
        <linux-spi@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X
 operations
Message-ID: <20220405193721.5jf3umfn3dvv6fxd@ti.com>
References: <20220331110819.133392-1-matthias.schiffer@ew.tq-group.com>
 <20220401100606.iz52jbrdcz6pd5sg@ti.com>
 <6b2bfa6614fbb9339b94a191ab933a2c25b8b4d7.camel@ew.tq-group.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6b2bfa6614fbb9339b94a191ab933a2c25b8b4d7.camel@ew.tq-group.com>
Precedence: bulk

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.