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

[Pratyush Yadav <p.yadav@xxxxxx>]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.