[PATCH 3/4] mtd: spinand: Add support continuous read operation

[Zhengxun Li]

Hi Miquel,

Sorry for the late reply.

> >
> >
> > > Hello,
> > >
> > > zhengxunli.mxic at gmail.com wrote on Fri,  8 Oct 2021 14:57:58 +0800:
> > >
> > > > The patch adds a continuous read start flag to support continuous
> > > > read operations. The continuous read operation only issues a page
> > > > read command (13h), issues multiple read commands from the cache
> > > > (03h/0Bh/3Bh/6Bh/BBh/EBh) to read continuous address data, and
> > > > finally issues an exit continuous read command (63h) to terminate
> > > > this continuous read operation.
> > > >
> > > > Since the continuous read mode can only read the entire page of data
> > > > (2KB)
> > >
> > > Remove this size, it is highly unlikely that all SPI NAND devices will
> > > ever be restricted to 2kiB right?
> >
> > Okay.
> >
> > > > and cannot read the oob data,
> > >
> > > This is something that you seem to skip to check in your series.
> >
> > In fact, only ECC-Free SPI-NAND support continuous read mode now.
> >
> > > > the dynamic change mode is added
> > > > to enable continuous read mode and disable continuous read mode in
> > > > spinand_mtd_read to avoid writing and erasing operation is abnormal.
> > > >
> > > > Signed-off-by: Zhengxun <zhengxunli.mxic at gmail.com>
> > > > ---
> > > >  drivers/mtd/nand/spi/core.c | 38 +++++++++++++++++++++++++++++---------
> > > >  include/linux/mtd/spinand.h |  2 ++
> > > >  2 files changed, 31 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > > > index 0d9632f..0369453 100644
> > > > --- a/drivers/mtd/nand/spi/core.c
> > > > +++ b/drivers/mtd/nand/spi/core.c
> > > > @@ -195,6 +195,8 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
> > > >
> > > >  static int spinand_continuous_read_enable(struct spinand_device *spinand)
> > > >  {
> > > > +     spinand->cont_read_start = false;
> > >
> > > I really don't like the "= false" in the "read_enable" hook. Why not
> > > just checking directly in mtd_read and drop that boolean ?
> >
> > Okay, I will delet it.
> >
> > > > +
> > > >       if (!(spinand->flags & SPINAND_HAS_CONT_READ_BIT))
> > > >               return 0;
> > > >
> > > > @@ -598,16 +600,22 @@ static int spinand_read_page(struct spinand_device *spinand,
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > > -     ret = spinand_load_page_op(spinand, req);
> > > > -     if (ret)
> > > > -             return ret;
> > > > +     if (!spinand->cont_read_start) {
> > >
> > > I don't get this check. This condition will always be true. You can
> > > drop it.
> >
> > This condition is help to avoid issue page read
> > command again. The continuous read mode help
> > SPI-NAND prevent always issue page read(13h)
> > command in continuous address.
>
> Yes I understand what you try to achieve but I believe I overlooked at
> that section.
>
> I believe we should have something just a little bit more clean like:
>
> mtd_io(){

In this case, do you mean spinand_mtd_read() ?

>         _enable() { if (<useful> && supported) use_continuous_read = true; }
>         loop {
>                 read();
>         }
>         _disable() { use_continuous_read = false; }
> }
>
> read(){

And the same, do you mean spinand_read_page() ?

>         _enter() { if (use_continuous_read) enter(); }
>         do_io();
>         _exit() { if (use_continuous_read) exit(); }
>
> >
> > > >
> > > > -     ret = spinand_wait(spinand,
> > > > -                        SPINAND_READ_INITIAL_DELAY_US,
> > > > -                        SPINAND_READ_POLL_DELAY_US,
> > > > -                        &status);
> > > > -     if (ret < 0)
> > > > -             return ret;
> > > > +             ret = spinand_load_page_op(spinand, req);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +
> > > > +             ret = spinand_wait(spinand,
> > > > +                                SPINAND_READ_INITIAL_DELAY_US,
> > > > +                                SPINAND_READ_POLL_DELAY_US,
> > > > +                                &status);
> > > > +             if (ret < 0)
> > > > +                     return ret;
> > > > +
> > > > +             if (spinand->flags & SPINAND_HAS_CONT_READ_BIT)
> > > > +                     spinand->cont_read_start = true;
> > > > +     }
> > > >
> > > >       spinand_ondie_ecc_save_status(nand, status);
> > > >
> > > > @@ -667,6 +675,10 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> > > >
> > > >       mutex_lock(&spinand->lock);
> > > >
> > > > +     ret = spinand_continuous_read_enable(spinand);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > >       nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) {
> > > >               if (disable_ecc)
> > > >                       iter.req.mode = MTD_OPS_RAW;
> > > > @@ -689,6 +701,14 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> > > >               ops->oobretlen += iter.req.ooblen;
> > > >       }
> > > >
> > > > +     ret = spinand_continuous_read_exit(spinand);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = spinand_continuous_read_disable(spinand);
> > > > +     if (ret)
> > > > +             return ret;
> > >
> > > The asymmetry here looks strange. Where do we actually enter the
> > > continuous read mode?
> >
> > In this series, each read always enter the continuous read mode.
>
> This is definitely something to improve. You need to benchmark a little
> bit and try to read 1, 2, 3, 4,... pages until we are sure that
> enabling this and the overall penalty is backed by the additional
> performances.
>
> >
> > >
> > > Do you have any indicators that this change improves the performances?
> > > It would be good to share them in the commit log.
> >
> > I will share the performances of continuous read mode.
>
> Yes please.
>
> >
> > > > +
> > > >       mutex_unlock(&spinand->lock);
> > > >
> > > >       if (ecc_failed && !ret)
> > > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > > > index e044aba..c2a41a3 100644
> > > > --- a/include/linux/mtd/spinand.h
> > > > +++ b/include/linux/mtd/spinand.h
> > > > @@ -422,6 +422,7 @@ struct spinand_dirmap {
> > > >   *           because the spi-mem interface explicitly requests that buffers
> > > >   *           passed in spi_mem_op be DMA-able, so we can't based the bufs on
> > > >   *           the stack
> > > > + * @cont_read_start: record the continuous read status
> > > >   * @manufacturer: SPI NAND manufacturer information
> > > >   * @priv: manufacturer private data
> > > >   */
> > > > @@ -450,6 +451,7 @@ struct spinand_device {
> > > >       u8 *databuf;
> > > >       u8 *oobbuf;
> > > >       u8 *scratchbuf;
> > > > +     bool cont_read_start;
> > > >       const struct spinand_manufacturer *manufacturer;
> > > >       void *priv;
> > > >  };
> > >
> > >
> > > Thanks,
> > > Miquèl
> >
> > All in all, the continuous read mode can improve
> > the read performance of continuous addresses
> > and avoid re-issuing page read commands through
> > each page.
> >
> > Do you have any suggestions for this series?
> >
> > Thanks,
> > Zhengxun
>
>
> Thanks,
> Miquèl

Thanks,
Zhengxun