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