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

[Zhengxun Li]

Hi Miquel,


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

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

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

> > +
> >       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