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:   Fri, 1 Apr 2022 15:36:06 +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: <20220401100606.iz52jbrdcz6pd5sg@ti.com>
References: <20220331110819.133392-1-matthias.schiffer@ew.tq-group.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220331110819.133392-1-matthias.schiffer@ew.tq-group.com>
Precedence: bulk

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.