* [PATCH 0/4] mtd: spinand: Add continuous read mode support
@ 2021-10-08 6:57 Zhengxun
2021-10-08 6:57 ` [PATCH 1/4] mtd: spinand: Add support continuous read mode Zhengxun
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Zhengxun @ 2021-10-08 6:57 UTC (permalink / raw)
To: linux-mtd, miquel.raynal; +Cc: zhengxunli, Zhengxun
This series add support continuous read mode for SPI NAND.
The Macronix SPI NAND supports conventional read mode and continuous
read modes. If the configuration register bit “CONT” = 0, the device
is in conventional read mode. The page read operation and page read
cache Random/ sequential operation is supported in the conventional
read mode. If the configuration register bit “CONT” = 1, the device
is in continuous read mode and only continuous read operation is
supported. During the continuous read mode, the page read cache
related commands are not supported. (Page Cache Sequential (31h),
Page Read Cache Random (30h) and Page Read Cache End (3Fh)).
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_CONT1. 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 issue Exit Continuous Read Command (63h)
to terminate this continuous read operation.
The data output for each page will always start from byte 0 and a
full page data (2KB) should be read out for each page. If the host
just wants to read out partial page data, Exit Continuous Read
Command (63h) should be issued to terminate the current continuous
read operation.
Zhengxun (4):
mtd: spinand: Add support continuous read mode
mtd: spinand: Add continuous read exit command
mtd: spinand: Add support continuous read operation
mtd: spinand: macronix: Add support for Macronix MX31LF2GE4BC SPI NAND
flash
drivers/mtd/nand/spi/core.c | 65 +++++++++++++++++++++++++++++++++++------
drivers/mtd/nand/spi/macronix.c | 11 +++++++
include/linux/mtd/spinand.h | 10 +++++++
3 files changed, 77 insertions(+), 9 deletions(-)
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] mtd: spinand: Add support continuous read mode
2021-10-08 6:57 [PATCH 0/4] mtd: spinand: Add continuous read mode support Zhengxun
@ 2021-10-08 6:57 ` Zhengxun
2021-10-08 6:57 ` [PATCH 2/4] mtd: spinand: Add continuous read exit command Zhengxun
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Zhengxun @ 2021-10-08 6:57 UTC (permalink / raw)
To: linux-mtd, miquel.raynal; +Cc: zhengxunli, Zhengxun
The patch supports setting the "CONT" bit of the configuration
register and adding a continuous read mode flag for identification.
Signed-off-by: Zhengxun <zhengxunli.mxic@gmail.com>
---
drivers/mtd/nand/spi/core.c | 17 +++++++++++++++++
include/linux/mtd/spinand.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 2c8685f..bcdd438 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -193,6 +193,23 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
enable ? CFG_QUAD_ENABLE : 0);
}
+static int spinand_continuous_read_enable(struct spinand_device *spinand)
+{
+ if (!(spinand->flags & SPINAND_HAS_CONT_READ_BIT))
+ return 0;
+
+ return spinand_upd_cfg(spinand, CFG_CONT_READ_ENABLE,
+ CFG_CONT_READ_ENABLE);
+}
+
+static int spinand_continuous_read_disable(struct spinand_device *spinand)
+{
+ if (!(spinand->flags & SPINAND_HAS_CONT_READ_BIT))
+ return 0;
+
+ return spinand_upd_cfg(spinand, CFG_CONT_READ_ENABLE, 0);
+}
+
static int spinand_ecc_enable(struct spinand_device *spinand,
bool enable)
{
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6988956..04be10e 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -154,6 +154,7 @@
#define REG_CFG 0xb0
#define CFG_OTP_ENABLE BIT(6)
#define CFG_ECC_ENABLE BIT(4)
+#define CFG_CONT_READ_ENABLE BIT(2)
#define CFG_QUAD_ENABLE BIT(0)
/* status register */
@@ -307,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)
/**
* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] mtd: spinand: Add continuous read exit command
2021-10-08 6:57 [PATCH 0/4] mtd: spinand: Add continuous read mode support Zhengxun
2021-10-08 6:57 ` [PATCH 1/4] mtd: spinand: Add support continuous read mode Zhengxun
@ 2021-10-08 6:57 ` Zhengxun
2021-10-08 6:57 ` [PATCH 3/4] mtd: spinand: Add support continuous read operation Zhengxun
2021-10-08 6:57 ` [PATCH 4/4] mtd: spinand: macronix: Add support for Macronix MX31LF2GE4BC SPI NAND flash Zhengxun
3 siblings, 0 replies; 10+ messages in thread
From: Zhengxun @ 2021-10-08 6:57 UTC (permalink / raw)
To: linux-mtd, miquel.raynal; +Cc: zhengxunli, Zhengxun
Add a continuous read exit command to terminate continuous read
operation.
Signed-off-by: Zhengxun <zhengxunli.mxic@gmail.com>
---
drivers/mtd/nand/spi/core.c | 10 ++++++++++
include/linux/mtd/spinand.h | 6 ++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index bcdd438..0d9632f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -431,6 +431,16 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
return 0;
}
+static int spinand_continuous_read_exit(struct spinand_device *spinand)
+{
+ struct spi_mem_op op = SPINAND_EXIT_CONT_READ;
+
+ if (!(spinand->flags & SPINAND_HAS_CONT_READ_BIT))
+ return 0;
+
+ return spi_mem_exec_op(spinand->spimem, &op);
+}
+
static int spinand_write_to_cache_op(struct spinand_device *spinand,
const struct nand_page_io_req *req)
{
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 04be10e..e044aba 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -122,6 +122,12 @@
SPI_MEM_OP_DUMMY(ndummy, 4), \
SPI_MEM_OP_DATA_IN(len, buf, 4))
+#define SPINAND_EXIT_CONT_READ \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(0x63, 1), \
+ SPI_MEM_OP_NO_ADDR, \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_NO_DATA)
+
#define SPINAND_PROG_EXEC_OP(addr) \
SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1), \
SPI_MEM_OP_ADDR(3, addr, 1), \
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] mtd: spinand: Add support continuous read operation
2021-10-08 6:57 [PATCH 0/4] mtd: spinand: Add continuous read mode support Zhengxun
2021-10-08 6:57 ` [PATCH 1/4] mtd: spinand: Add support continuous read mode Zhengxun
2021-10-08 6:57 ` [PATCH 2/4] mtd: spinand: Add continuous read exit command Zhengxun
@ 2021-10-08 6:57 ` Zhengxun
2021-11-09 11:10 ` Miquel Raynal
2021-10-08 6:57 ` [PATCH 4/4] mtd: spinand: macronix: Add support for Macronix MX31LF2GE4BC SPI NAND flash Zhengxun
3 siblings, 1 reply; 10+ messages in thread
From: Zhengxun @ 2021-10-08 6:57 UTC (permalink / raw)
To: linux-mtd, miquel.raynal; +Cc: zhengxunli, Zhengxun
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) and cannot read the oob data, 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;
+
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) {
- 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;
+
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;
};
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] mtd: spinand: macronix: Add support for Macronix MX31LF2GE4BC SPI NAND flash
2021-10-08 6:57 [PATCH 0/4] mtd: spinand: Add continuous read mode support Zhengxun
` (2 preceding siblings ...)
2021-10-08 6:57 ` [PATCH 3/4] mtd: spinand: Add support continuous read operation Zhengxun
@ 2021-10-08 6:57 ` Zhengxun
3 siblings, 0 replies; 10+ messages in thread
From: Zhengxun @ 2021-10-08 6:57 UTC (permalink / raw)
To: linux-mtd, miquel.raynal; +Cc: zhengxunli, Zhengxun
The Macronix MX31LF2GE4BC are 3V, 2Gbit serial SLC NAND flash device.
MX"31", means SPI NAND
MX31"LF", LF means 3V
MX31LF"2G", 2G means 2Gbits
MX31LF2G"E4", E4 means internal ECC and Quad I/O(x4)
Validated via normal(default) and QUAD mode by read, erase, read back,
on Xilinx Zynq PicoZed FPGA board which included Macronix
SPI Host(drivers/spi/spi-mxic.c).
Signed-off-by: Zhengxun <zhengxunli.mxic@gmail.com>
---
drivers/mtd/nand/spi/macronix.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
index 3f31f13..43cfd85 100644
--- a/drivers/mtd/nand/spi/macronix.c
+++ b/drivers/mtd/nand/spi/macronix.c
@@ -176,6 +176,17 @@ static const struct spinand_info macronix_spinand_table[] = {
SPINAND_HAS_QE_BIT,
SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
mx35lf1ge4ab_ecc_get_status)),
+ SPINAND_INFO("MX31LF2GE4BC",
+ SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x2e),
+ NAND_MEMORG(1, 2048, 64, 64, 2048, 40, 1, 1, 1),
+ NAND_ECCREQ(8, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+ &write_cache_variants,
+ &update_cache_variants),
+ SPINAND_HAS_QE_BIT | SPINAND_HAS_CONT_READ_BIT,
+ SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
+ mx35lf1ge4ab_ecc_get_status)),
+
SPINAND_INFO("MX31UF1GE4BC",
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x9e),
NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
--
2.7.4
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation
2021-10-08 6:57 ` [PATCH 3/4] mtd: spinand: Add support continuous read operation Zhengxun
@ 2021-11-09 11:10 ` Miquel Raynal
2021-11-17 9:30 ` Zhengxun Li
0 siblings, 1 reply; 10+ messages in thread
From: Miquel Raynal @ 2021-11-09 11:10 UTC (permalink / raw)
To: Zhengxun; +Cc: linux-mtd, zhengxunli
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?
> and cannot read the oob data,
This is something that you seem to skip to check in your series.
> 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 ?
> +
> 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.
>
> - 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?
Do you have any indicators that this change improves the performances?
It would be good to share them in the commit log.
> +
> 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation
2021-11-09 11:10 ` Miquel Raynal
@ 2021-11-17 9:30 ` Zhengxun Li
2021-11-22 9:21 ` Miquel Raynal
0 siblings, 1 reply; 10+ messages in thread
From: Zhengxun Li @ 2021-11-17 9:30 UTC (permalink / raw)
To: Miquel Raynal; +Cc: linux-mtd, zhengxunli
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.
> >
> > - 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation
2021-11-17 9:30 ` Zhengxun Li
@ 2021-11-22 9:21 ` Miquel Raynal
2022-01-12 9:49 ` Zhengxun Li
0 siblings, 1 reply; 10+ messages in thread
From: Miquel Raynal @ 2021-11-22 9:21 UTC (permalink / raw)
To: Zhengxun Li; +Cc: linux-mtd, zhengxunli
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation
2021-11-22 9:21 ` Miquel Raynal
@ 2022-01-12 9:49 ` Zhengxun Li
2022-01-26 10:40 ` Miquel Raynal
0 siblings, 1 reply; 10+ messages in thread
From: Zhengxun Li @ 2022-01-12 9:49 UTC (permalink / raw)
To: Miquel Raynal; +Cc: linux-mtd, zhengxunli
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() ?
> _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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] mtd: spinand: Add support continuous read operation
2022-01-12 9:49 ` Zhengxun Li
@ 2022-01-26 10:40 ` Miquel Raynal
0 siblings, 0 replies; 10+ messages in thread
From: Miquel Raynal @ 2022-01-26 10:40 UTC (permalink / raw)
To: Zhengxun Li; +Cc: linux-mtd, zhengxunli
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-26 10:41 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-08 6:57 [PATCH 0/4] mtd: spinand: Add continuous read mode support Zhengxun
2021-10-08 6:57 ` [PATCH 1/4] mtd: spinand: Add support continuous read mode Zhengxun
2021-10-08 6:57 ` [PATCH 2/4] mtd: spinand: Add continuous read exit command Zhengxun
2021-10-08 6:57 ` [PATCH 3/4] mtd: spinand: Add support continuous read operation Zhengxun
2021-11-09 11:10 ` Miquel Raynal
2021-11-17 9:30 ` Zhengxun Li
2021-11-22 9:21 ` Miquel Raynal
2022-01-12 9:49 ` Zhengxun Li
2022-01-26 10:40 ` Miquel Raynal
2021-10-08 6:57 ` [PATCH 4/4] mtd: spinand: macronix: Add support for Macronix MX31LF2GE4BC SPI NAND flash Zhengxun
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).