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

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

Hi Zhengxun,

zhengxunli.mxic@gmail.com wrote on Wed, 12 Jan 2022 17:49:33 +0800:

> Hi Miquel,
> 
> Sorry for the late reply.
> 
> > >
> > >  
> > > > 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(){  
> 
> In this case, do you mean spinand_mtd_read() ?

I don't remember the details, it was a while ago. I don't think I meant
spinand_mtd_read() given that below I refer to it as do_io(), but try
to find the best way to implement such a logic and we'll see.

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


Thanks,
Miquèl

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