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

[Jean Pihet]

Hi Pratyush,

On Mon, Nov 30, 2020 at 1:37 PM Pratyush Yadav <p.yadav at 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 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?

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