[PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers

[Pratyush Yadav]

Hi Jean,

On 29/11/20 11:39AM, Jean Pihet wrote:
> Use the flags field of the SPI slave struct to pass the dual and quad
> read properties, from the SPI NOR layer to the low level driver.
> Tested with TI QSPI in 1, 2 and 4 bits modes.
> 
> Signed-off-by: Jean Pihet <jean.pihet at newoldbits.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 34 +++++++++++++++++++++++++++++++---
>  drivers/spi/spi-mem.c          |  8 +++++++-
>  include/spi.h                  |  2 ++
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e16b0e1462..54569b3cba 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -94,22 +94,50 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  	/* convert the dummy cycles to the number of bytes */
>  	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
>  
> +	/* Check for dual and quad I/O read */
> +	switch (op.cmd.opcode) {
> +	case SPINOR_OP_READ_1_1_2:
> +	case SPINOR_OP_READ_1_2_2:
> +	case SPINOR_OP_READ_1_1_2_4B:
> +	case SPINOR_OP_READ_1_2_2_4B:
> +	case SPINOR_OP_READ_1_2_2_DTR:
> +	case SPINOR_OP_READ_1_2_2_DTR_4B:
> +		nor->spi->flags |= SPI_XFER_DUAL_READ;
> +		break;
> +	case SPINOR_OP_READ_1_1_4:
> +	case SPINOR_OP_READ_1_4_4:
> +	case SPINOR_OP_READ_1_1_4_4B:
> +	case SPINOR_OP_READ_1_4_4_4B:
> +	case SPINOR_OP_READ_1_4_4_DTR:
> +	case SPINOR_OP_READ_1_4_4_DTR_4B:
> +		nor->spi->flags |= SPI_XFER_QUAD_READ;
> +		break;
> +	default:
> +		break;
> +	}
> +

NACK. What if a flash uses some other opcode for dual or quad reads?

Why not use op->data.buswidth in ti_qspi_exec_mem_op() to enable dual or 
quad mode? In fact, ti_qspi_setup_mmap_read() already checks this and 
enables dual or quad mode. What problem does this patch solve then?

>  	while (remaining) {
>  		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
>  		ret = spi_mem_adjust_op_size(nor->spi, &op);
>  		if (ret)
> -			return ret;
> +			goto out;
>  
>  		ret = spi_mem_exec_op(nor->spi, &op);
>  		if (ret)
> -			return ret;
> +			goto out;
>  
>  		op.addr.val += op.data.nbytes;
>  		remaining -= op.data.nbytes;
>  		op.data.buf.in += op.data.nbytes;
>  	}
>  
> -	return len;
> +	ret = len;
> +
> +out:
> +	/* Reset dual and quad I/O read flags for upcoming transactions */
> +	nor->spi->flags &= ~(SPI_XFER_DUAL_READ | SPI_XFER_QUAD_READ);
> +
> +	return ret;
>  }
>  
>  static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index c095ae9505..24e38ea95e 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -387,9 +387,15 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>  		return ret;
>  
>  	/* 2nd transfer: rx or tx data path */
> +	flag = SPI_XFER_END;
> +	/* Check for dual and quad I/O reads */
> +	if (slave->flags & SPI_XFER_DUAL_READ)
> +		flag |= SPI_XFER_DUAL_READ;
> +	if (slave->flags & SPI_XFER_QUAD_READ)
> +		flag |= SPI_XFER_QUAD_READ;

Ah, so you are targeting the "legacy" path. Seeing that ti_qspi does 
support the SPI MEM path, this would be executed when the read exceeds 
priv->mmap_size, correct?

In any case, I don't see why you need slave->flags. Why can't you set 
these flags by checking op->data.buswidth and op->data.dir? This would 
make your fix transparent to SPI NOR, which IMO is a much better idea.

>  	if (tx_buf || rx_buf) {
>  		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
> -			       rx_buf, SPI_XFER_END);
> +			       rx_buf, flag);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/spi.h b/include/spi.h
> index ef8c1f6692..cf5f05e526 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -146,6 +146,8 @@ struct spi_slave {
>  #define SPI_XFER_BEGIN		BIT(0)	/* Assert CS before transfer */
>  #define SPI_XFER_END		BIT(1)	/* Deassert CS after transfer */
>  #define SPI_XFER_ONCE		(SPI_XFER_BEGIN | SPI_XFER_END)
> +#define SPI_XFER_DUAL_READ	BIT(2)  /* Dual I/O read */
> +#define SPI_XFER_QUAD_READ	BIT(3)  /* Quad I/O read */
>  };
>  
>  /**
> -- 
> 2.26.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India