From: liao jaime <jaimeliao.tw@gmail.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: linux-mtd@lists.infradead.org, jaimeliao@mxic.com.tw
Subject: Re: [PATCH v1 3/4] mtd: spinand: Add support continuous read operation
Date: Wed, 8 Feb 2023 19:14:55 +0800 [thread overview]
Message-ID: <CAAQoYR=4SeCLVzAa14t8UOJXK0ntymRdsvdb0DO6LcJWtiz5tg@mail.gmail.com> (raw)
In-Reply-To: <20221107154838.29b2803b@xps-13>
Hi Miquel
>
> Hi JaimeLiao,
>
> jaimeliao.tw@gmail.com wrote on Thu, 11 Aug 2022 17:45:35 +0800:
>
> > The continuous read operation includes three phases:
> > Firstly, starting with the page read command and the 1st
> > page data will be read into the cache after the read latency tRD.
> > Secondly, Issuing the Read From Cache commands
> > (03h/0Bh/3Bh/6Bh/BBh/EBh) to read out the data from cache continuously.
> > After all the data is read out, the host should pull CS# high
> > to terminate this continuous read operation and wait tRST for the NAND
> > device resets read operation.
> >
> > Continuous reads have a positive impact when reading more than
> > one page and the column address is don't care in this operation,
> > a full page data will be read out for each page.
> >
> > The performance of continuous read mode is as follows. Set the
> > flash to QUAD mode and run 25MZ direct mapping mode on the SPI
>
> MHz
Thanks, this will be correct next patch.
>
> > bus and use the MTD test module to show the performance of
> > continuous reads.
>
> Please show before and after changes and just give us the lines
> regarding the read speeds rather than all the output of the tool.
Ok, I will attach test report for two cases.
>
> >
> > =================================================
> > mtd_speedtest: MTD device: 0 count: 100
> > mtd_speedtest: MTD device size 268435456, eraseblock size 131072,
> > page size 2048, count of eraseblocks 2048, pages per
> > eraseblock 64, OOB size 64
> > mtd_test: scanning for bad eraseblocks
> > mtd_test: scanned 100 eraseblocks, 0 are bad
> > mtd_speedtest: testing eraseblock write speed
> > mtd_speedtest: eraseblock write speed is 1298 KiB/s
> > mtd_speedtest: testing eraseblock read speed
> > mtd_speedtest: eraseblock read speed is 11053 KiB/s
> > mtd_speedtest: testing page write speed
> > mtd_speedtest: page write speed is 1291 KiB/s
> > mtd_speedtest: testing page read speed
> > mtd_speedtest: page read speed is 3240 KiB/s
> > mtd_speedtest: testing 2 page write speed
> > mtd_speedtest: 2 page write speed is 1289 KiB/s
> > mtd_speedtest: testing 2 page read speed
> > mtd_speedtest: 2 page read speed is 2909 KiB/s
> > mtd_speedtest: Testing erase speed
> > mtd_speedtest: erase speed is 45229 KiB/s
> > mtd_speedtest: Testing 2x multi-block erase speed
> > mtd_speedtest: 2x multi-block erase speed is 62135 KiB/s
> > mtd_speedtest: Testing 4x multi-block erase speed
> > mtd_speedtest: 4x multi-block erase speed is 60093 KiB/s
> > mtd_speedtest: Testing 8x multi-block erase speed
> > mtd_speedtest: 8x multi-block erase speed is 61244 KiB/s
> > mtd_speedtest: Testing 16x multi-block erase speed
> > mtd_speedtest: 16x multi-block erase speed is 61538 KiB/s
> > mtd_speedtest: Testing 32x multi-block erase speed
> > mtd_speedtest: 32x multi-block erase speed is 61835 KiB/s
> > mtd_speedtest: Testing 64x multi-block erase speed
> > mtd_speedtest: 64x multi-block erase speed is 60663 KiB/s
> > mtd_speedtest: finished
> > =================================================
> >
> > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> > ---
> > drivers/mtd/nand/spi/core.c | 99 +++++++++++++++++++++++++++++++++++++
> > include/linux/mtd/spinand.h | 1 +
> > 2 files changed, 100 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 380411feab6c..ef09bedda8d4 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -386,6 +386,10 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
> > if (req->datalen) {
> > buf = spinand->databuf;
> > nbytes = nanddev_page_size(nand);
> > + if (spinand->use_continuous_read) {
> > + buf = req->databuf.in;
> > + nbytes = req->datalen;
> > + }
>
> Does not look right. A cache read is still one page, and I believe the
> caller should have taken care of that?
For this part, buf is req->databuf.in.
Multi page could be receive at this operation.
>
> > column = 0;
> > }
> >
> > @@ -412,6 +416,9 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
> > buf += ret;
> > }
> >
> > + if (spinand->use_continuous_read)
> > + return 0;
>
> For me this is not the right place.
Would it be better If using "goto"?
>
> > +
> > if (req->datalen)
> > memcpy(req->databuf.in, spinand->databuf + req->dataoffs,
> > req->datalen);
> > @@ -640,6 +647,80 @@ static int spinand_write_page(struct spinand_device *spinand,
> > return nand_ecc_finish_io_req(nand, (struct nand_page_io_req *)req);
> > }
> >
> > +static int spinand_mtd_continuous_read(struct mtd_info *mtd, loff_t from,
> > + struct mtd_oob_ops *ops,
> > + struct nand_io_iter *iter)
> > +{
> > + struct spinand_device *spinand = mtd_to_spinand(mtd);
> > + struct nand_device *nand = mtd_to_nanddev(mtd);
> > + int ret = 0;
> > +
> > + /*
> > + * Continuous read mode could reduce some operation in On-die ECC free
> > + * flash when read page sequentially.
> > + */
> > + iter->req.type = NAND_PAGE_READ;
> > + iter->req.mode = MTD_OPS_RAW;
>
> I don't get what you do here.
>
> Continuous read shall either work with ECC engines or just not be
> implemented at all. You cannot blindly disable ECC support.
For Macronix continuous read feature is for "On-die ECC" flash only.
So that I think ecc have been handled in flash.
>
> > + iter->req.dataoffs = nanddev_offs_to_pos(nand, from, &iter->req.pos);
> > + iter->req.databuf.in = ops->datbuf;
> > + iter->req.datalen = ops->len;
> > +
> > + if (from & (nanddev_page_size(nand) - 1)) {
> > + pr_debug("%s: unaligned address\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + ret = spinand_continuous_read_enable(spinand);
> > + if (ret)
> > + return ret;
> > +
> > + spinand->use_continuous_read = true;
> > +
> > + ret = spinand_select_target(spinand, iter->req.pos.target);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * The continuous read operation including: firstly, starting with the
> > + * page read command and the 1 st page data will be read into the cache
> > + * after the read latency tRD. Secondly, Issuing the Read From Cache
> > + * commands (03h/0Bh/3Bh/6Bh/BBh/EBh) to read out the data from cache
> > + * continuously.
> > + *
> > + * The cache is divided into two halves, while one half of the cache is
> > + * outputting the data, the other half will be loaded for the new data;
> > + * therefore, the host can read out the data continuously from page to
> > + * page. Multiple of Read From Cache commands can be issued in one
> > + * continuous read operation, each Read From Cache command is required
> > + * to read multiple 4-byte data exactly; otherwise, the data output will
> > + * be out of sequence from one Read From Cache command to another Read
> > + * From Cache command.
> > + *
> > + * After all the data is read out, the host should pull CS# high to
> > + * terminate this continuous read operation and wait a 6us of tRST for
> > + * the NAND device resets read operation. The data output for each page
> > + * will always start from byte 0 and a full page data should be read out
> > + * for each page.
> > + */
> > + ret = spinand_read_page(spinand, &iter->req);
> > + if (ret)
> > + return ret;
> > +
> > + ret = spinand_reset_op(spinand);
> > + if (ret)
> > + return ret;
> > +
> > + ret = spinand_continuous_read_disable(spinand);
> > + if (ret)
> > + return ret;
> > +
> > + spinand->use_continuous_read = false;
>
> Any error in this function should lead to resetting use_continuous_read
> to false and disabling continuous read on the flash side.
OK, this will be correct next version.
>
> > +
> > + ops->retlen = iter->req.datalen;
> > +
> > + return ret;
> > +}
> > +
> > static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> > struct mtd_oob_ops *ops)
> > {
> > @@ -656,6 +737,24 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from,
> >
> > mutex_lock(&spinand->lock);
> >
> > + /*
> > + * If the device support continuous read mode and read length larger
> > + * than one page size will enter the continuous read mode. This mode
> > + * helps avoid issuing a page read command and read from cache command
> > + * again, and improves read performance for continuous addresses.
> > + */
> > + if ((spinand->flags & SPINAND_HAS_CONT_READ_BIT) &&
> > + (ops->len > nanddev_page_size(nand))) {
> > + ret = spinand_mtd_continuous_read(mtd, from, ops, &iter);
> > +
> > + mutex_unlock(&spinand->lock);
> > +
> > + if (ecc_failed && !ret)
> > + ret = -EBADMSG;
> > +
> > + return ret ? ret : max_bitflips;
> > + }
> > +
> > nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) {
> > if (disable_ecc)
> > iter.req.mode = MTD_OPS_RAW;
> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index d70a6f49cc6f..c296d4d0dda5 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -308,6 +308,7 @@ struct spinand_ecc_info {
> >
> > #define SPINAND_HAS_QE_BIT BIT(0)
> > #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> > +#define SPINAND_HAS_CONT_READ_BIT BIT(2)
>
> Is this bit standard?
It is using to add into flash id table and judge whether continuous
read feature is support or not.
>
> >
> > /**
> > * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
>
>
> Thanks,
> Miquèl
Thanks
Jaime
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2023-02-08 11:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-11 9:45 [PATCH v1 0/4] mtd: spinand: Add continuous read mode support JaimeLiao
2022-08-11 9:45 ` [PATCH v1 1/4] mtd: spinand: Add support continuous read mode JaimeLiao
2022-08-11 9:45 ` [PATCH v1 2/4] mtd: spinand: Add continuous read state JaimeLiao
2022-08-11 9:45 ` [PATCH v1 3/4] mtd: spinand: Add support continuous read operation JaimeLiao
2022-11-07 14:48 ` Miquel Raynal
2023-02-08 11:14 ` liao jaime [this message]
2023-02-15 8:23 ` Miquel Raynal
2022-08-11 9:45 ` [PATCH v1 4/4] mtd: spinand: macronix: Add continuous read support for MX35LF2GE4AD JaimeLiao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAQoYR=4SeCLVzAa14t8UOJXK0ntymRdsvdb0DO6LcJWtiz5tg@mail.gmail.com' \
--to=jaimeliao.tw@gmail.com \
--cc=jaimeliao@mxic.com.tw \
--cc=linux-mtd@lists.infradead.org \
--cc=miquel.raynal@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).