| Message ID | 20201129103948.158293-1-jean.pihet@newoldbits.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | [1/3] mtd: spi nor: add support for dual and quad bit transfers | expand |
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@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 >
Hi Pratyush, On Mon, Nov 30, 2020 at 1:37 PM Pratyush Yadav <p.yadav@ti.com> wrote: > > 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@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? Ok I will rework this that way. The goal is to enable bigger flashes (>64MB) and to optimize the transfer speed. I will add a cover letter to the patch series to better describe the goals. > > > 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? Yes exactly. The TI QSPI controller can only mmap 64MB. > > 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. Ok let me rework this. > > > 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 Thanks for reviewing! BR, Jean
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; + } + 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; 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 */ }; /**
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@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(-)