[PATCH 3/4] mtd: spinand: Add support continuous read operation
Miquel Raynal
miquel.raynal at bootlin.com
Wed Jan 26 02:40:38 PST 2022
Hi Zhengxun,
zhengxunli.mxic at gmail.com wrote on Wed, 12 Jan 2022 17:49:33 +0800:
> 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() ?
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
More information about the linux-mtd
mailing list