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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

zhengxunli.mxic@gmail.com wrote on Wed, 17 Nov 2021 17:30:52 +0800:

> Hi Miquel,
> 
> 
> > Hello,
> >
> > zhengxunli.mxic@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@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(){
	_enable() { if (<useful> && supported) use_continuous_read = true; }
	loop {
		read();
	}
	_disable() { use_continuous_read = false; }
}

read(){
	_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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/