mtd: rawnand: Sequential page reads

Instead of checking if a pattern is supported each time we need it, let's create a bitfield that only the core would be allowed to fill at startup time. The core and the individual drivers may then use it in order to check what operation they should use. This bitfield is supposed to grow over time.

Signed-off-by Miquel Raynal <[email protected]>

Add an empty envelope just to show how to add additional checks for new operations. This is going to be used for sequential cached reads, which require the page size to be known (and the discovery to be over), hence the "late" designation.

Signed-off-by Miquel Raynal <[email protected]>

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
        00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
        00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)

drivers/mtd/nand/raw/nand_base.c +115 -4

index 34395d5d3a47..0b1fd6bbb36b 100644
@@ -1208,6 +1208,73 @@ static int nand_lp_exec_read_page_op(struct nand_chip *chip, unsigned int page,
        return nand_exec_op(chip, &op);
 }
 
+static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page,
+                                         unsigned int offset_in_page, void *buf,
+                                         unsigned int len, bool check_only)
+{
+       const struct nand_interface_config *conf =
+               nand_get_interface_config(chip);
+       u8 addrs[5];
+       struct nand_op_instr start_instrs[] = {
+               NAND_OP_CMD(NAND_CMD_READ0, 0),
+               NAND_OP_ADDR(4, addrs, 0),
+               NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), 0),
+               NAND_OP_CMD(NAND_CMD_READCACHESEQ, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
+                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_op_instr cont_instrs[] = {
+               NAND_OP_CMD(page == chip->cont_read.last_page ?
+                           NAND_CMD_READCACHEEND : NAND_CMD_READCACHESEQ,
+                           NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
+                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_operation start_op = NAND_OPERATION(chip->cur_cs, start_instrs);
+       struct nand_operation cont_op = NAND_OPERATION(chip->cur_cs, cont_instrs);
+       int ret;
+
+       if (!len) {
+               start_op.ninstrs--;
+               cont_op.ninstrs--;
+       }
+
+       ret = nand_fill_column_cycles(chip, addrs, offset_in_page);
+       if (ret < 0)
+               return ret;
+
+       addrs[2] = page;
+       addrs[3] = page >> 8;
+
+       if (chip->options & NAND_ROW_ADDR_3) {
+               addrs[4] = page >> 16;
+               start_instrs[1].ctx.addr.naddrs++;
+       }
+
+       /* Check if cache reads are supported */
+       if (check_only) {
+               if (nand_check_op(chip, &start_op) || nand_check_op(chip, &cont_op))
+                       return -EOPNOTSUPP;
+
+               return 0;
+       }
+
+       if (page == chip->cont_read.first_page)
+               return nand_exec_op(chip, &start_op);
+       else
+               return nand_exec_op(chip, &cont_op);
+}
+
+static bool rawnand_cont_read_ongoing(struct nand_chip *chip, unsigned int page)
+{
+       return chip->cont_read.ongoing &&
+               page >= chip->cont_read.first_page &&
+               page <= chip->cont_read.last_page;
+}
+
 /**
  * nand_read_page_op - Do a READ PAGE operation
  * @chip: The NAND chip

@@ -1233,10 +1300,16 @@ int nand_read_page_op(struct nand_chip *chip, unsigned int page,
                return -EINVAL;
 
        if (nand_has_exec_op(chip)) {
-               if (mtd->writesize > 512)
-                       return nand_lp_exec_read_page_op(chip, page,
-                                                        offset_in_page, buf,
-                                                        len);
+               if (mtd->writesize > 512) {
+                       if (rawnand_cont_read_ongoing(chip, page))
+                               return nand_lp_exec_cont_read_page_op(chip, page,
+                                                                     offset_in_page,
+                                                                     buf, len, false);
+                       else
+                               return nand_lp_exec_read_page_op(chip, page,
+                                                                offset_in_page, buf,
+                                                                len);
+               }
 
                return nand_sp_exec_read_page_op(chip, page, offset_in_page,
                                                 buf, len);

@@ -3353,6 +3426,27 @@ static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob,
        return NULL;
 }
 
+static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
+                                     u32 readlen, int col)
+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (!chip->controller->supported_op.cont_read)
+               return;
+
+       if ((col && col + readlen < (3 * mtd->writesize)) ||
+           (!col && readlen < (2 * mtd->writesize))) {
+               chip->cont_read.ongoing = false;
+               return;
+       }
+
+       chip->cont_read.ongoing = true;
+       chip->cont_read.first_page = page;
+       if (col)
+               chip->cont_read.first_page++;
+       chip->cont_read.last_page = page + ((readlen >> chip->page_shift) & chip->pagemask);
+}
+
 /**
  * nand_setup_read_retry - [INTERN] Set the READ RETRY mode
  * @chip: NAND chip object

@@ -3426,6 +3520,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
        oob = ops->oobbuf;
        oob_required = oob ? 1 : 0;
 
+       rawnand_enable_cont_reads(chip, page, readlen, col);
+
        while (1) {
                struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
 

@@ -5009,12 +5105,27 @@ static void rawnand_early_check_supported_ops(struct nand_chip *chip)
        rawnand_check_data_only_read_support(chip);
 }
 
+static void rawnand_check_cont_read_support(struct nand_chip *chip)
+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (chip->read_retries)
+               return;
+
+       if (!nand_lp_exec_cont_read_page_op(chip, 0, 0, NULL,
+                                           mtd->writesize, true))
+               chip->controller->supported_op.cont_read = 1;
+}
+
 static void rawnand_late_check_supported_ops(struct nand_chip *chip)
 {
        /* The supported_op fields should not be set by individual drivers */
+       WARN_ON_ONCE(chip->controller->supported_op.cont_read);
 
        if (!nand_has_exec_op(chip))
                return;
+
+       rawnand_check_cont_read_support(chip);
 }
 
 /*

include/linux/mtd/rawnand.h +9 -1

index 28c5dce782dd..1b0936fe3c6e 100644
@@ -67,6 +67,8 @@ struct gpio_desc;
 
 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART     0x30
+#define NAND_CMD_READCACHESEQ  0x31
+#define NAND_CMD_READCACHEEND  0x3f
 #define NAND_CMD_RNDOUTSTART   0xE0
 #define NAND_CMD_CACHEDPROG    0x15
 

@@ -1099,12 +1101,14 @@ struct nand_controller_ops {
  * @supported_op.data_only_read: The controller supports reading more data from
  *                     the bus without restarting an entire read operation nor
  *                     changing the column.
+ * @supported_op.cont_read: The controller supports sequential cache reads.
  */
 struct nand_controller {
        struct mutex lock;
        const struct nand_controller_ops *ops;
        struct {
                unsigned int data_only_read: 1;
+               unsigned int cont_read: 1;
        } supported_op;
 };
 

@@ -1308,6 +1312,11 @@ struct nand_chip {
        int read_retries;
        struct nand_secure_region *secure_regions;
        u8 nr_secure_regions;
+       struct {
+               bool ongoing;
+               unsigned int first_page;
+               unsigned int last_page;
+       } cont_read;
 
        /* Externals */
        struct nand_controller *controller;
--

2.34.1

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

Tested-by Liao Jaime <[email protected]>

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 7 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)  

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Is there anything I can test to get more information?

Hi Måns,

[email protected] wrote on Thu, 22 Jun 2023 15:59:25 +0100:

Quoted 77 lines

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 7 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)  

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Quoted 2 lines

Is there anything I can test to get more information?

May I know what NAND chip you are using?

Quoted 84 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Thu, 22 Jun 2023 15:59:25 +0100:

Quoted 79 lines

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 7 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)    

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Is there anything I can test to get more information?

Quoted 1 line

May I know what NAND chip you are using?

It's a Micron MT29F4G16ABBDAH4-IT:D. From the kernel logs:

nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xbc
nand: Micron MT29F4G16ABBDAH4
nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64

ID="0xec,0xa1,0x00,0x15" # 128M 128KB 2KB

-----Original Message-----
From: Miquel Raynal <[email protected]>
Sent: Friday, June 23, 2023 4:08 PM
To: Måns Rullgård <[email protected]>
Cc: Richard Weinberger <[email protected]>; Vignesh Raghavendra <[email protected]>; Tudor Ambarus <[email protected]>; Pratyush Yadav <[email protected]>; Michael Walle <[email protected]>; linux- [email protected]; Julien Su <[email protected]>; Jaime Liao <[email protected]>; Alvin Zhou <[email protected]>; Thomas Petazzoni <[email protected]>; JaimeLiao <[email protected]>; Bean Huo <[email protected]>
Subject: [EXT] Re: [PATCH v2 3/3] mtd: rawnand: Support for sequential cache reads

CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless you recognize the sender and were expecting this message.

Hi Bean,

[email protected] wrote on Fri, 23 Jun 2023 12:27:54 +0100:

Quoted 68 lines

Quoted 66 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Thu, 22 Jun 2023 15:59:25 +0100:

Quoted 62 lines

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN 00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN 00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

 00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119

+++++++++++++++++++++++++++++--

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Is there anything I can test to get more information?

Quoted 7 lines

Quoted 1 line

May I know what NAND chip you are using?

It's a Micron MT29F4G16ABBDAH4-IT:D. From the kernel logs:

nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xbc
nand: Micron MT29F4G16ABBDAH4
nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64

There is definitely something wrong with Micron's handling of the sequential reads. Can you make a setup on your side with one of these chips and try to reproduce?

I will have to discard Micron's chips if we don't find a solution very soon, too bad, it is a nice performance improvement -when it works-.

Thanks,
Miquèl

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
	00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
	00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

	00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
		       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
		       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 7 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)  

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Is there anything I can test to get more information?

Hello all,

So here is a summary of the situation:
- we have two bug reports regarding the use of sequential page reads
- both are on TI OMAP platforms: AM33XX and AM3517. I believe both are using the same omap2.c driver
- they use a Micron and a Samsung NAND chip

All these information gives me the hint that it is related to the controller driver which does something silly during the exec_op phase.

Alexander and Måns, can you please tell me:
- Are you using a gpio for the waitrdy thing or do you leverage nand_soft_waitrdy()? If you are using the gpio, can you both try with the soft implementation and see if it changes something?

- Are you using any POLL or DMA prefetch mode? Can you please force the default in and out helpers by using only omap_nand_data_in() and omap_nand_data_out() to see if it changes something?

I believe there is something wrong in the timings, while properly implemented in theory there might be some cases where we miss a barrier or something like that. I would like to try the following two hacks, and see if we can find what is the timing that is not observed, despite the lack of probing. The first one is a real hack, the second one might actually look like a real fix. Please let me know, both of you, if you see different behaviors.

*** HACK #1 ***

drivers/mtd/nand/raw/omap2.c +3 -0

@@ -2113,6 +2113,9 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
        case NAND_OP_CMD_INSTR:
                iowrite8(instr->ctx.cmd.opcode,
                         info->reg.gpmc_nand_command);
+               if (instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ ||
+                   instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND)
+                       udelay(50);
                break;
 
        case NAND_OP_ADDR_INSTR:

*** HACK #2 ***

drivers/mtd/nand/raw/omap2.c +0 -0

@@ -2143,8 +2146,10 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
                break;

Quoted 8 lines · Miquel Raynal

Hello all,

So here is a summary of the situation:
- we have two bug reports regarding the use of sequential page reads
- both are on TI OMAP platforms: AM33XX and AM3517. I believe both are using the same omap2.c driver
- they use a Micron and a Samsung NAND chip

Quoted 7 lines

All these information gives me the hint that it is related to the controller driver which does something silly during the exec_op phase.

Alexander and Måns, can you please tell me:
- Are you using a gpio for the waitrdy thing or do you leverage nand_soft_waitrdy()? If you are using the gpio, can you both try with the soft implementation and see if it changes something?

There's no gpio specified in the devicetree, so I guess it must be using nand_soft_waitrdy().

Quoted 3 lines

- Are you using any POLL or DMA prefetch mode? Can you please force the default in and out helpers by using only omap_nand_data_in() and omap_nand_data_out() to see if it changes something?

It was using the default PREFETCH_POLLED mode. Switching it to POLLED (and thus omap_nand_data_in/out()) doesn't change anything.

Quoted 37 lines

I believe there is something wrong in the timings, while properly implemented in theory there might be some cases where we miss a barrier or something like that. I would like to try the following two hacks, and see if we can find what is the timing that is not observed, despite the lack of probing. The first one is a real hack, the second one might actually look like a real fix. Please let me know, both of you, if you see different behaviors.

*** HACK #1 ***

drivers/mtd/nand/raw/omap2.c +3 -0

@@ -2113,6 +2113,9 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
        case NAND_OP_CMD_INSTR:
                iowrite8(instr->ctx.cmd.opcode,
                         info->reg.gpmc_nand_command);
+               if (instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ ||
+                   instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND)
+                       udelay(50);
                break;
 
        case NAND_OP_ADDR_INSTR:

*** HACK #2 ***

drivers/mtd/nand/raw/omap2.c +3 -1

@@ -2143,8 +2146,10 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
                break;
        }
 
-       if (instr->delay_ns)
+       if (instr->delay_ns) {
+               mb();
                ndelay(instr->delay_ns);
+       }
 

drivers/mtd/nand/raw/omap2.c static int omap_nand_exec_instr(struct nand_chip *chip, · line 2155

Quoted 2 lines

drivers/mtd/nand/raw/omap2.c +0 -0

        return 0;
 }  

Neither of these help.

We are using GPIO for waitrdy. However, even when switching to soft implementation,
the system still behaves incorrectly.
We are using prefetch-dma mode for xfer. Changing to the default implementation does not result in any improvements.

Both patches don't help :(

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

In a second test, you could simply add a udelay(50); at the end of omap_nand_exec_instr().

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Quoted 2 lines

In a second test, you could simply add a udelay(50); at the end of omap_nand_exec_instr().

That didn't help.

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 21 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 16 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Quoted 3 lines

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

Hi Måns,

[email protected] wrote on Tue, 18 Jul 2023 15:03:14 +0100:

Quoted 27 lines

Quoted 24 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 3 lines

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

I would like to know, how much time we spend waiting in both cases.

Is there something wrong with the "wait ready"? As we cannot observe the timings with a scope, because we are using a "soft" controller implementation somehow, we can easily measure how much time we spend in each operation by measuring the time before and after.

These information are only useful when we are doing operations related to sequential reads.

Quoted 31 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Tue, 18 Jul 2023 15:03:14 +0100:

Quoted 27 lines

Quoted 24 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 5 lines

Quoted 3 lines

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

I would like to know, how much time we spend waiting in both cases.

Which "both" cases?

Quoted 7 lines

Is there something wrong with the "wait ready"? As we cannot observe the timings with a scope, because we are using a "soft" controller implementation somehow, we can easily measure how much time we spend in each operation by measuring the time before and after.

These information are only useful when we are doing operations related to sequential reads.

I have hooked up some spare GPIOs to a scope, which should be more accurate (nanosecond) than software timestamps. All I need to know is what to measure and what to look for in those measurements.

Hi Måns,

[email protected] wrote on Wed, 19 Jul 2023 10:26:09 +0100:

Quoted 33 lines

Quoted 31 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Tue, 18 Jul 2023 15:03:14 +0100:

Quoted 27 lines

Quoted 24 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

I would like to know, how much time we spend waiting in both cases.

Which "both" cases?

ndelay and more importantly, waitrdy:

Quoted 8 lines

Quoted 7 lines

Is there something wrong with the "wait ready"? As we cannot observe the timings with a scope, because we are using a "soft" controller implementation somehow, we can easily measure how much time we spend in each operation by measuring the time before and after.

These information are only useful when we are doing operations related to sequential reads.

Quoted 3 lines

I have hooked up some spare GPIOs to a scope, which should be more accurate (nanosecond) than software timestamps. All I need to know is what to measure and what to look for in those measurements.

Great. The only issue with the scope is the fact that we might actually look at something that is not a faulty sequential read op.

Unless you hack into the core to perform these in a loop (with a brutal "while (1)"). But I don't think we require big precision here, at least as a first step, looking at software timestamps like hinted above is enough so we can easily identify the different delays and compare them with nand_timings.c.

Please use whatever method is easier for you.

Quoted 46 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Wed, 19 Jul 2023 10:26:09 +0100:

Quoted 33 lines

Quoted 31 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Tue, 18 Jul 2023 15:03:14 +0100:

Quoted 27 lines

Quoted 24 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

I would like to know, how much time we spend waiting in both cases.

Which "both" cases?

ndelay and more importantly, waitrdy:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Is there something wrong with the "wait ready"? As we cannot observe the timings with a scope, because we are using a "soft" controller implementation somehow, we can easily measure how much time we spend in each operation by measuring the time before and after.

These information are only useful when we are doing operations related to sequential reads.

Quoted 6 lines

Quoted 3 lines

I have hooked up some spare GPIOs to a scope, which should be more accurate (nanosecond) than software timestamps. All I need to know is what to measure and what to look for in those measurements.

Great. The only issue with the scope is the fact that we might actually look at something that is not a faulty sequential read op.

How exactly do I know which ones are faulty?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 7 lines

Unless you hack into the core to perform these in a loop (with a brutal "while (1)"). But I don't think we require big precision here, at least as a first step, looking at software timestamps like hinted above is enough so we can easily identify the different delays and compare them with nand_timings.c.

Please use whatever method is easier for you.

Which values should be compared?

Sorry if I'm being obtuse, I don't normally have to deal with these things.

Hi Måns,

[email protected] wrote on Wed, 19 Jul 2023 14:15:48 +0100:

Quoted 59 lines

Quoted 46 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Wed, 19 Jul 2023 10:26:09 +0100:

Quoted 33 lines

Quoted 31 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Tue, 18 Jul 2023 15:03:14 +0100:

Quoted 27 lines

Quoted 24 lines · Miquel Raynal

Hi Måns,

[email protected] wrote on Mon, 17 Jul 2023 14:11:31 +0100:

Quoted 13 lines

Quoted 10 lines · Miquel Raynal

So, I should have done that earlier but, could you please slow the whole operation down, just to see if there is something wrong with the timings or if we should look in another direction.

Maybe you could add a boolean to flag if the last CMD was a READCACHESEQ, READCACHESTART or READCACHEEND, and if the flag is true, please get the jiffies before and after each waitrdy and delay_ns. Finally, please print the expected delay and the actual one and compare to see if something was too fast compared to what we expected.

Quoted 3 lines

Between which points exactly should the delay be measured? Also, there is no command called READCACHESTART. Did you mean READSTART or something else?

Yeah, whatever command is specific to sequential cache reads:
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I'm still not sure what exactly you want to me measure. The waitrdy and ndelay combined, each separately, or something else?

I would like to know, how much time we spend waiting in both cases.

Which "both" cases?

ndelay and more importantly, waitrdy:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Is there something wrong with the "wait ready"? As we cannot observe the timings with a scope, because we are using a "soft" controller implementation somehow, we can easily measure how much time we spend in each operation by measuring the time before and after.

These information are only useful when we are doing operations related to sequential reads.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

I have hooked up some spare GPIOs to a scope, which should be more accurate (nanosecond) than software timestamps. All I need to know is what to measure and what to look for in those measurements.

Great. The only issue with the scope is the fact that we might actually look at something that is not a faulty sequential read op.

How exactly do I know which ones are faulty?

Right now I expect all sequential ops to be faulty. As mentioned above, I don't think we are interested in all the commands that are sent through the NAND bus, but just the READSTART/READCACHESEQ/READCACHEEND sequences, see these two ops there, that's what we want to capture:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L1228

That's why a regular scope is not as easy as it sounds to use to capture these timings.

Quoted 9 lines

Quoted 7 lines

Unless you hack into the core to perform these in a loop (with a brutal "while (1)"). But I don't think we require big precision here, at least as a first step, looking at software timestamps like hinted above is enough so we can easily identify the different delays and compare them with nand_timings.c.

Please use whatever method is easier for you.

Which values should be compared?

The specification declares minimum and maximum times (see nand_timings.c). I want to see if these timings, which are requested by the core (links above) are correctly observed or not. The ones that are particularly critical because they are different than what the other ops use, are the ones around READSTART/READCACHESEQ/READCACHEEND. Anything else, you already use them, so it's quite likely that they are not problematic. These are new.

Hello all,

So here is a summary of the situation:
- we have two bug reports regarding the use of sequential page reads
- both are on TI OMAP platforms: AM33XX and AM3517. I believe both are using the same omap2.c driver
- they use a Micron and a Samsung NAND chip

All these information gives me the hint that it is related to the controller driver which does something silly during the exec_op phase.

Alexander and Måns, can you please tell me:
- Are you using a gpio for the waitrdy thing or do you leverage nand_soft_waitrdy()? If you are using the gpio, can you both try with the soft implementation and see if it changes something?
- Are you using any POLL or DMA prefetch mode? Can you please force the default in and out helpers by using only omap_nand_data_in() and omap_nand_data_out() to see if it changes something?

I believe there is something wrong in the timings, while properly implemented in theory there might be some cases where we miss a barrier or something like that. I would like to try the following two hacks, and see if we can find what is the timing that is not observed, despite the lack of probing. The first one is a real hack, the second one might actually look like a real fix. Please let me know, both of you, if you see different behaviors.

*** HACK #1 ***

drivers/mtd/nand/raw/omap2.c +3 -0

@@ -2113,6 +2113,9 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
        case NAND_OP_CMD_INSTR:
                iowrite8(instr->ctx.cmd.opcode,
                         info->reg.gpmc_nand_command);
+               if (instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ ||
+                   instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND)
+                       udelay(50);
                break;
 
        case NAND_OP_ADDR_INSTR:

*** HACK #2 ***

drivers/mtd/nand/raw/omap2.c +3 -1

@@ -2143,8 +2146,10 @@ static int omap_nand_exec_instr(struct nand_chip *chip,
                break;
        }
 
-       if (instr->delay_ns)
+       if (instr->delay_ns) {
+               mb();
                ndelay(instr->delay_ns);
+       }
 
        return 0;
 }

Thanks a lot!
Miquèl

[email protected] wrote on Thu, 22 Jun 2023 15:59:25+0100:

Quoted 79 lines

Quoted 60 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
Requires the NAND chip to load the next page into cache while keeping the current cache available for host reads.
* 0x3F - READ CACHE END:
Tells the NAND chip this is the end of the sequential cache read, the current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

    00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN
    00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN
    00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

    00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                   31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                   31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                   3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Quoted 7 lines

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119 +++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++
 2 files changed, 124 insertions(+), 4 deletions(-)

This change broke something on a TI AM3517 based system. What I'm noticing is that the u-boot fw_setenv tool is failing due to the MEMGETBADBLOCK ioctl reporting some blocks as bad when they are not. Everything else is, somehow, working fine. Reverting this commit fixes it, though I don't know why. I'm seeing the same behaviour on multiple devices, so I doubt there is a problem with the flash memory.

Is there anything I can test to get more information?

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic
core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
  Requires the NAND chip to load the next page into cache while keeping
  the current cache available for host reads.
* 0x3F - READ CACHE END:
  Tells the NAND chip this is the end of the sequential cache read, the
  current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN 00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN 00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with
a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

---
 drivers/mtd/nand/raw/nand_base.c | 119
+++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++

 2 files changed, 124 insertions(+), 4 deletions(-)

drivers/mtd/nand/raw/nand_base.c +0 -0

index 34395d5d3a47..0b1fd6bbb36b 100644
@@ -1208,6 +1208,73 @@ static int nand_lp_exec_read_page_op(struct
 nand_chip *chip, unsigned int page,
         return nand_exec_op(chip, &op);
  }

+static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page,
+                                         unsigned int offset_in_page, void *buf,
+                                         unsigned int len, bool check_only)
+{
+       const struct nand_interface_config *conf =
+               nand_get_interface_config(chip);
+       u8 addrs[5];
+       struct nand_op_instr start_instrs[] = {
+               NAND_OP_CMD(NAND_CMD_READ0, 0),
+               NAND_OP_ADDR(4, addrs, 0),
+               NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), 0),
+               NAND_OP_CMD(NAND_CMD_READCACHESEQ, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), +                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_op_instr cont_instrs[] = {
+               NAND_OP_CMD(page == chip->cont_read.last_page ? +                           NAND_CMD_READCACHEEND :
NAND_CMD_READCACHESEQ,
+                           NAND_COMMON_TIMING_NS(conf, tWB_max)), +               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), +                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_operation start_op = NAND_OPERATION(chip->cur_cs, start_instrs);
+       struct nand_operation cont_op = NAND_OPERATION(chip->cur_cs, cont_instrs);
+       int ret;

drivers/mtd/nand/raw/nand_base.c +21 -0

+
+       if (!len) {
+               start_op.ninstrs--;
+               cont_op.ninstrs--;
+       }
+
+       ret = nand_fill_column_cycles(chip, addrs, offset_in_page);
+       if (ret < 0)
+               return ret;
+
+       addrs[2] = page;
+       addrs[3] = page >> 8;
+
+       if (chip->options & NAND_ROW_ADDR_3) {
+               addrs[4] = page >> 16;
+               start_instrs[1].ctx.addr.naddrs++;
+       }
+
+       /* Check if cache reads are supported */
+       if (check_only) {
+               if (nand_check_op(chip, &start_op) ||

nand_check_op(chip, &cont_op))
+                       return -EOPNOTSUPP;

drivers/mtd/nand/raw/nand_base.c +11 -0

+
+               return 0;
+       }
+
+       if (page == chip->cont_read.first_page)
+               return nand_exec_op(chip, &start_op);
+       else
+               return nand_exec_op(chip, &cont_op);
+}
+
+static bool rawnand_cont_read_ongoing(struct nand_chip *chip,

unsigned int page)

drivers/mtd/nand/raw/nand_base.c +6 -0

+{
+       return chip->cont_read.ongoing &&
+               page >= chip->cont_read.first_page &&
+               page <= chip->cont_read.last_page;
+}
+

 /**
  * nand_read_page_op - Do a READ PAGE operation
  * @chip: The NAND chip

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -1233,10 +1300,16 @@ int nand_read_page_op(struct nand_chip *chip,
 unsigned int page,
                 return -EINVAL;

        if (nand_has_exec_op(chip)) {
-               if (mtd->writesize > 512)
-                       return nand_lp_exec_read_page_op(chip, page, -
offset_in_page, buf,
-                                                        len); +               if (mtd->writesize > 512) {
+                       if (rawnand_cont_read_ongoing(chip, page)) +                               return nand_lp_exec_cont_read_page_op(chip, page,
+
offset_in_page,
+
buf, len, false);
+                       else
+                               return nand_lp_exec_read_page_op(chip, page,
+
offset_in_page, buf,
+
len);
+               }

                return nand_sp_exec_read_page_op(chip, page, offset_in_page,
                                                 buf, len);

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3353,6 +3426,27 @@ static uint8_t *nand_transfer_oob(struct
 nand_chip *chip, uint8_t *oob,
         return NULL;
  }

+static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
+                                     u32 readlen, int col)

drivers/mtd/nand/raw/nand_base.c +17 -0

+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (!chip->controller->supported_op.cont_read)
+               return;
+
+       if ((col && col + readlen < (3 * mtd->writesize)) ||
+           (!col && readlen < (2 * mtd->writesize))) {
+               chip->cont_read.ongoing = false;
+               return;
+       }
+
+       chip->cont_read.ongoing = true;
+       chip->cont_read.first_page = page;
+       if (col)
+               chip->cont_read.first_page++;
+       chip->cont_read.last_page = page + ((readlen >> chip-

Quoted 1 line

page_shift) & chip->pagemask);

drivers/mtd/nand/raw/nand_base.c +2 -0

+}
+

 /**
  * nand_setup_read_retry - [INTERN] Set the READ RETRY mode
  * @chip: NAND chip object

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3426,6 +3520,8 @@ static int nand_do_read_ops(struct nand_chip
 *chip, loff_t from,
         oob = ops->oobbuf;
         oob_required = oob ? 1 : 0;

+       rawnand_enable_cont_reads(chip, page, readlen, col); +
        while (1) {
                struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -5009,12 +5105,27 @@ static void
 rawnand_early_check_supported_ops(struct nand_chip *chip)
         rawnand_check_data_only_read_support(chip);
  }

drivers/mtd/nand/raw/nand_base.c +12 -0

+static void rawnand_check_cont_read_support(struct nand_chip *chip)
+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (chip->read_retries)
+               return;
+
+       if (!nand_lp_exec_cont_read_page_op(chip, 0, 0, NULL,
+                                           mtd->writesize, true))
+               chip->controller->supported_op.cont_read = 1;
+}
+

 static void rawnand_late_check_supported_ops(struct nand_chip *chip) {
        /* The supported_op fields should not be set by individual drivers */
+       WARN_ON_ONCE(chip->controller->supported_op.cont_read);
        if (!nand_has_exec_op(chip))
                return;
+
+       rawnand_check_cont_read_support(chip);
 }

 /*

include/linux/mtd/rawnand.h +0 -0

index 28c5dce782dd..1b0936fe3c6e 100644
@@ -67,6 +67,8 @@ struct gpio_desc;

 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART     0x30

include/linux/mtd/rawnand.h +2 -0

+#define NAND_CMD_READCACHESEQ  0x31
+#define NAND_CMD_READCACHEEND  0x3f

 #define NAND_CMD_RNDOUTSTART   0xE0
 #define NAND_CMD_CACHEDPROG    0x15

include/linux/mtd/rawnand.h +0 -0

@@ -1099,12 +1101,14 @@ struct nand_controller_ops {
   * @supported_op.data_only_read: The controller supports reading

more data from
  *                     the bus without restarting an entire read operation nor
  *                     changing the column.
+ * @supported_op.cont_read: The controller supports sequential cache reads.
  */
 struct nand_controller {
        struct mutex lock;
        const struct nand_controller_ops *ops;
        struct {
                unsigned int data_only_read: 1;
+               unsigned int cont_read: 1;
        } supported_op;
 };

include/linux/mtd/rawnand.h +5 -0

@@ -1308,6 +1312,11 @@ struct nand_chip {
         int read_retries;
         struct nand_secure_region *secure_regions;
         u8 nr_secure_regions;
+       struct {
+               bool ongoing;
+               unsigned int first_page;
+               unsigned int last_page;
+       } cont_read;

        /* Externals */
        struct nand_controller *controller;

Hi Miquel et al.

Quoted 61 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic
core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically continuous data by loading in cache the next page while the previous page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
  Requires the NAND chip to load the next page into cache while keeping
  the current cache available for host reads.
* 0x3F - READ CACHE END:
  Tells the NAND chip this is the end of the sequential cache read, the
  current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN 00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN 00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode with
a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

Quoted 7 lines

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the ~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

This patch broke our imx6ull system after doing a kernel upgrade:

[    2.921886] ubi0: default fastmap pool size: 100
[    2.926612] ubi0: default fastmap WL pool size: 50
[    2.931421] ubi0: attaching mtd1
[    3.515799] ubi0: scanning is finished
[    3.525237] ubi0 error: vtbl_check: bad CRC at record 0: 0x88cdfb6,
not 0xffffffff
[    3.532937] Volume table record 0 dump:
[    3.536783]  reserved_pebs   -1
[    3.539932]  alignment       -1
[    3.543101]  data_pad        -1
[    3.546251]  vol_type        255
[    3.549485]  upd_marker      255
[    3.552746]  name_len        65535
[    3.556155]  1st 5 characters of name: 
[    3.560429]  crc             0xffffffff
[    3.564294] ubi0 error: vtbl_check: bad CRC at record 0: 0x88cdfb6,
not 0xffffffff
[    3.571892] Volume table record 0 dump:
[    3.575754]  reserved_pebs   -1
[    3.578906]  alignment       -1
[    3.582049]  data_pad        -1
[    3.585216]  vol_type        255
[    3.588452]  upd_marker      255
[    3.591687]  name_len        65535
[    3.595108]  1st 5 characters of name: 
[    3.599384]  crc             0xffffffff
[    3.603285] ubi0 error: ubi_read_volume_table: both volume tables
are corrupted
[    3.611460] ubi0 error: ubi_attach_mtd_dev: failed to attach mtd1,
error -22
[    3.618760] UBI error: cannot attach mtd1
[    3.622831] UBI: block: can't open volume on ubi0_4, err=-19
[    3.628505] UBI: block: can't open volume on ubi0_6, err=-19
[    3.634196] UBI: block: can't open volume on ubi0_7, err=-19

It fails consistently at every attach operation. As mentioned above, this is on an i.MX6ULL and a Toshiba NAND chip:

[    0.530121] nand: device found, Manufacturer ID: 0x98, Chip ID: 0xdc
[    0.530173] nand: Toshiba NAND 512MiB 3,3V 8-bit
[    0.530194] nand: 512 MiB, SLC, erase size: 256 KiB, page size:

4096, OOB size: 128

I'm happy to perform experiments to fix this.

// Martin

Quoted 236 lines

---
 drivers/mtd/nand/raw/nand_base.c | 119
+++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++

 2 files changed, 124 insertions(+), 4 deletions(-)

drivers/mtd/nand/raw/nand_base.c +0 -0

index 34395d5d3a47..0b1fd6bbb36b 100644
@@ -1208,6 +1208,73 @@ static int nand_lp_exec_read_page_op(struct
 nand_chip *chip, unsigned int page,
         return nand_exec_op(chip, &op);
  }

+static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page,
+                                         unsigned int offset_in_page, void *buf,
+                                         unsigned int len, bool check_only)
+{
+       const struct nand_interface_config *conf =
+               nand_get_interface_config(chip);
+       u8 addrs[5];
+       struct nand_op_instr start_instrs[] = {
+               NAND_OP_CMD(NAND_CMD_READ0, 0),
+               NAND_OP_ADDR(4, addrs, 0),
+               NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), 0),
+               NAND_OP_CMD(NAND_CMD_READCACHESEQ, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), +                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_op_instr cont_instrs[] = {
+               NAND_OP_CMD(page == chip->cont_read.last_page ? +                           NAND_CMD_READCACHEEND :
NAND_CMD_READCACHESEQ,
+                           NAND_COMMON_TIMING_NS(conf, tWB_max)), +               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max), +                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_operation start_op = NAND_OPERATION(chip->cur_cs, start_instrs);
+       struct nand_operation cont_op = NAND_OPERATION(chip->cur_cs, cont_instrs);
+       int ret;

drivers/mtd/nand/raw/nand_base.c +21 -0

+
+       if (!len) {
+               start_op.ninstrs--;
+               cont_op.ninstrs--;
+       }
+
+       ret = nand_fill_column_cycles(chip, addrs, offset_in_page);
+       if (ret < 0)
+               return ret;
+
+       addrs[2] = page;
+       addrs[3] = page >> 8;
+
+       if (chip->options & NAND_ROW_ADDR_3) {
+               addrs[4] = page >> 16;
+               start_instrs[1].ctx.addr.naddrs++;
+       }
+
+       /* Check if cache reads are supported */
+       if (check_only) {
+               if (nand_check_op(chip, &start_op) ||

nand_check_op(chip, &cont_op))
+                       return -EOPNOTSUPP;

drivers/mtd/nand/raw/nand_base.c +11 -0

+
+               return 0;
+       }
+
+       if (page == chip->cont_read.first_page)
+               return nand_exec_op(chip, &start_op);
+       else
+               return nand_exec_op(chip, &cont_op);
+}
+
+static bool rawnand_cont_read_ongoing(struct nand_chip *chip,

unsigned int page)

drivers/mtd/nand/raw/nand_base.c +6 -0

+{
+       return chip->cont_read.ongoing &&
+               page >= chip->cont_read.first_page &&
+               page <= chip->cont_read.last_page;
+}
+

 /**
  * nand_read_page_op - Do a READ PAGE operation
  * @chip: The NAND chip

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -1233,10 +1300,16 @@ int nand_read_page_op(struct nand_chip *chip,
 unsigned int page,
                 return -EINVAL;

        if (nand_has_exec_op(chip)) {
-               if (mtd->writesize > 512)
-                       return nand_lp_exec_read_page_op(chip, page, -
offset_in_page, buf,
-                                                        len); +               if (mtd->writesize > 512) {
+                       if (rawnand_cont_read_ongoing(chip, page)) +                               return nand_lp_exec_cont_read_page_op(chip, page,
+
offset_in_page,
+
buf, len, false);
+                       else
+                               return nand_lp_exec_read_page_op(chip, page,
+
offset_in_page, buf,
+
len);
+               }

                return nand_sp_exec_read_page_op(chip, page, offset_in_page,
                                                 buf, len);

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3353,6 +3426,27 @@ static uint8_t *nand_transfer_oob(struct
 nand_chip *chip, uint8_t *oob,
         return NULL;
  }

+static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
+                                     u32 readlen, int col)

drivers/mtd/nand/raw/nand_base.c +17 -0

+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (!chip->controller->supported_op.cont_read)
+               return;
+
+       if ((col && col + readlen < (3 * mtd->writesize)) ||
+           (!col && readlen < (2 * mtd->writesize))) {
+               chip->cont_read.ongoing = false;
+               return;
+       }
+
+       chip->cont_read.ongoing = true;
+       chip->cont_read.first_page = page;
+       if (col)
+               chip->cont_read.first_page++;
+       chip->cont_read.last_page = page + ((readlen >> chip-  

Quoted 1 line

page_shift) & chip->pagemask);

drivers/mtd/nand/raw/nand_base.c +2 -0

+}
+

 /**
  * nand_setup_read_retry - [INTERN] Set the READ RETRY mode
  * @chip: NAND chip object

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3426,6 +3520,8 @@ static int nand_do_read_ops(struct nand_chip
 *chip, loff_t from,
         oob = ops->oobbuf;
         oob_required = oob ? 1 : 0;

+       rawnand_enable_cont_reads(chip, page, readlen, col); +
        while (1) {
                struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -5009,12 +5105,27 @@ static void
 rawnand_early_check_supported_ops(struct nand_chip *chip)
         rawnand_check_data_only_read_support(chip);
  }

drivers/mtd/nand/raw/nand_base.c +12 -0

+static void rawnand_check_cont_read_support(struct nand_chip *chip)
+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (chip->read_retries)
+               return;
+
+       if (!nand_lp_exec_cont_read_page_op(chip, 0, 0, NULL,
+                                           mtd->writesize, true))
+               chip->controller->supported_op.cont_read = 1;
+}
+

 static void rawnand_late_check_supported_ops(struct nand_chip *chip) {
        /* The supported_op fields should not be set by individual drivers */
+       WARN_ON_ONCE(chip->controller->supported_op.cont_read);
        if (!nand_has_exec_op(chip))
                return;
+
+       rawnand_check_cont_read_support(chip);
 }

 /*

include/linux/mtd/rawnand.h +0 -0

index 28c5dce782dd..1b0936fe3c6e 100644
@@ -67,6 +67,8 @@ struct gpio_desc;

 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART     0x30

include/linux/mtd/rawnand.h +2 -0

+#define NAND_CMD_READCACHESEQ  0x31
+#define NAND_CMD_READCACHEEND  0x3f

 #define NAND_CMD_RNDOUTSTART   0xE0
 #define NAND_CMD_CACHEDPROG    0x15

include/linux/mtd/rawnand.h +0 -0

@@ -1099,12 +1101,14 @@ struct nand_controller_ops {
   * @supported_op.data_only_read: The controller supports reading

more data from
  *                     the bus without restarting an entire read operation nor
  *                     changing the column.
+ * @supported_op.cont_read: The controller supports sequential cache reads.
  */
 struct nand_controller {
        struct mutex lock;
        const struct nand_controller_ops *ops;
        struct {
                unsigned int data_only_read: 1;
+               unsigned int cont_read: 1;
        } supported_op;
 };

include/linux/mtd/rawnand.h +0 -0

@@ -1308,6 +1312,11 @@ struct nand_chip {
         int read_retries;
         struct nand_secure_region *secure_regions;
         u8 nr_secure_regions;

Quoted 8 lines

include/linux/mtd/rawnand.h +5 -0

+       struct {
+               bool ongoing;
+               unsigned int first_page;
+               unsigned int last_page;
+       } cont_read;

        /* Externals */
        struct nand_controller *controller;

Hi Martin,

[email protected] wrote on Fri, 08 Sep 2023 14:25:59 +0200:

Quoted 127 lines

Hi Miquel et al.

Quoted 66 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic
core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically
continuous data by loading in cache the next page while the previous
page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to
handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
  Requires the NAND chip to load the next page into cache while keeping
  the current cache available for host reads.
* 0x3F - READ CACHE END:
  Tells the NAND chip this is the end of the sequential cache read,
the
  current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN 00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN 00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode
with
a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

Quoted 8 lines

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the
~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

This patch broke our imx6ull system after doing a kernel upgrade:
[    2.921886] ubi0: default fastmap pool size: 100
[    2.926612] ubi0: default fastmap WL pool size: 50
[    2.931421] ubi0: attaching mtd1
[    3.515799] ubi0: scanning is finished
[    3.525237] ubi0 error: vtbl_check: bad CRC at record 0:
0x88cdfb6,
not 0xffffffff
[    3.532937] Volume table record 0 dump:
[    3.536783]  reserved_pebs   -1
[    3.539932]  alignment       -1
[    3.543101]  data_pad        -1
[    3.546251]  vol_type        255
[    3.549485]  upd_marker      255
[    3.552746]  name_len        65535
[    3.556155]  1st 5 characters of name:
[    3.560429]  crc             0xffffffff
[    3.564294] ubi0 error: vtbl_check: bad CRC at record 0:
0x88cdfb6,
not 0xffffffff
[    3.571892] Volume table record 0 dump:
[    3.575754]  reserved_pebs   -1
[    3.578906]  alignment       -1
[    3.582049]  data_pad        -1
[    3.585216]  vol_type        255
[    3.588452]  upd_marker      255
[    3.591687]  name_len        65535
[    3.595108]  1st 5 characters of name:
[    3.599384]  crc             0xffffffff
[    3.603285] ubi0 error: ubi_read_volume_table: both volume tables
are corrupted
[    3.611460] ubi0 error: ubi_attach_mtd_dev: failed to attach mtd1,
error -22
[    3.618760] UBI error: cannot attach mtd1
[    3.622831] UBI: block: can't open volume on ubi0_4, err=-19 [    3.628505] UBI: block: can't open volume on ubi0_6, err=-19 [    3.634196] UBI: block: can't open volume on ubi0_7, err=-19

It fails consistently at every attach operation. As mentioned above,
this is on an i.MX6ULL and a Toshiba NAND chip:
[    0.530121] nand: device found, Manufacturer ID: 0x98, Chip ID:
0xdc
[    0.530173] nand: Toshiba NAND 512MiB 3,3V 8-bit
[    0.530194] nand: 512 MiB, SLC, erase size: 256 KiB, page size:
4096, OOB size: 128

Quoted 1 line

I'm happy to perform experiments to fix this.

I received two other reports using another controller and two different
chips, we investigated the timings which could be the issue but found nothing relevant. I thought it was specific to the controller (or its driver) but if you reproduce on imx6 it must be something else. Especially since this series was also tested on imx6. So maybe some devices do not really support what they advertise? Or they expect another sequence? I need to investigate this further but I am a bit clueless.

Quoted 1 line

Quoted 259 lines

// Martin

Quoted 257 lines

---
 drivers/mtd/nand/raw/nand_base.c | 119
+++++++++++++++++++++++++++++--
 include/linux/mtd/rawnand.h      |   9 +++

 2 files changed, 124 insertions(+), 4 deletions(-)

drivers/mtd/nand/raw/nand_base.c +0 -0

index 34395d5d3a47..0b1fd6bbb36b 100644
@@ -1208,6 +1208,73 @@ static int

nand_lp_exec_read_page_op(struct
nand_chip *chip, unsigned int page,
        return nand_exec_op(chip, &op);
 }

+static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip,
unsigned int page,
+                                         unsigned int offset_in_page, void *buf,
+                                         unsigned int len, bool check_only)
+{
+       const struct nand_interface_config *conf =
+               nand_get_interface_config(chip);
+       u8 addrs[5];
+       struct nand_op_instr start_instrs[] = {
+               NAND_OP_CMD(NAND_CMD_READ0, 0),
+               NAND_OP_ADDR(4, addrs, 0),
+               NAND_OP_CMD(NAND_CMD_READSTART, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
0),
+               NAND_OP_CMD(NAND_CMD_READCACHESEQ, NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
+                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_op_instr cont_instrs[] = {
+               NAND_OP_CMD(page == chip->cont_read.last_page ? +                           NAND_CMD_READCACHEEND :
NAND_CMD_READCACHESEQ,
+                           NAND_COMMON_TIMING_NS(conf, tWB_max)),
+               NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
+                                NAND_COMMON_TIMING_NS(conf, tRR_min)),
+               NAND_OP_DATA_IN(len, buf, 0),
+       };
+       struct nand_operation start_op = NAND_OPERATION(chip-

Quoted 1 line

cur_cs,

start_instrs);
+       struct nand_operation cont_op = NAND_OPERATION(chip-

Quoted 1 line

cur_cs,

cont_instrs);
+       int ret;

drivers/mtd/nand/raw/nand_base.c +7 -0

+
+       if (!len) {
+               start_op.ninstrs--;
+               cont_op.ninstrs--;
+       }
+
+       ret = nand_fill_column_cycles(chip, addrs,

offset_in_page);
+       if (ret < 0)
+               return ret;

drivers/mtd/nand/raw/nand_base.c +12 -0

+
+       addrs[2] = page;
+       addrs[3] = page >> 8;
+
+       if (chip->options & NAND_ROW_ADDR_3) {
+               addrs[4] = page >> 16;
+               start_instrs[1].ctx.addr.naddrs++;
+       }
+
+       /* Check if cache reads are supported */
+       if (check_only) {
+               if (nand_check_op(chip, &start_op) ||

nand_check_op(chip, &cont_op))
+                       return -EOPNOTSUPP;

drivers/mtd/nand/raw/nand_base.c +11 -0

+
+               return 0;
+       }
+
+       if (page == chip->cont_read.first_page)
+               return nand_exec_op(chip, &start_op);
+       else
+               return nand_exec_op(chip, &cont_op);
+}
+
+static bool rawnand_cont_read_ongoing(struct nand_chip *chip,

unsigned int page)

drivers/mtd/nand/raw/nand_base.c +6 -0

+{
+       return chip->cont_read.ongoing &&
+               page >= chip->cont_read.first_page &&
+               page <= chip->cont_read.last_page;
+}
+

 /**
  * nand_read_page_op - Do a READ PAGE operation
  * @chip: The NAND chip

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -1233,10 +1300,16 @@ int nand_read_page_op(struct nand_chip
 *chip,
 unsigned int page,
                 return -EINVAL;

        if (nand_has_exec_op(chip)) {
-               if (mtd->writesize > 512)
-                       return nand_lp_exec_read_page_op(chip, page,
-
offset_in_page, buf,
-                                                        len); +               if (mtd->writesize > 512) {
+                       if (rawnand_cont_read_ongoing(chip, page))
+                               return nand_lp_exec_cont_read_page_op(chip, page,
+

offset_in_page,
+

buf, len, false);
+                       else
+                               return nand_lp_exec_read_page_op(chip, page,
+
offset_in_page, buf,
+
len);
+               }

                return nand_sp_exec_read_page_op(chip, page, offset_in_page,
                                                 buf, len);

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3353,6 +3426,27 @@ static uint8_t *nand_transfer_oob(struct
 nand_chip *chip, uint8_t *oob,
         return NULL;
  }

+static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
+                                     u32 readlen, int col)

drivers/mtd/nand/raw/nand_base.c +17 -0

+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (!chip->controller->supported_op.cont_read)
+               return;
+
+       if ((col && col + readlen < (3 * mtd->writesize)) ||
+           (!col && readlen < (2 * mtd->writesize))) {
+               chip->cont_read.ongoing = false;
+               return;
+       }
+
+       chip->cont_read.ongoing = true;
+       chip->cont_read.first_page = page;
+       if (col)
+               chip->cont_read.first_page++;
+       chip->cont_read.last_page = page + ((readlen >> chip-  

Quoted 1 line

page_shift) & chip->pagemask);

drivers/mtd/nand/raw/nand_base.c +2 -0

+}
+

 /**
  * nand_setup_read_retry - [INTERN] Set the READ RETRY mode
  * @chip: NAND chip object

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -3426,6 +3520,8 @@ static int nand_do_read_ops(struct

nand_chip
*chip, loff_t from,
        oob = ops->oobbuf;
        oob_required = oob ? 1 : 0;

+       rawnand_enable_cont_reads(chip, page, readlen, col); +
        while (1) {
                struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;

drivers/mtd/nand/raw/nand_base.c +0 -0

@@ -5009,12 +5105,27 @@ static void
 rawnand_early_check_supported_ops(struct nand_chip *chip)
         rawnand_check_data_only_read_support(chip);
  }

+static void rawnand_check_cont_read_support(struct nand_chip *chip)

drivers/mtd/nand/raw/nand_base.c +8 -0

+{
+       struct mtd_info *mtd = nand_to_mtd(chip);
+
+       if (chip->read_retries)
+               return;
+
+       if (!nand_lp_exec_cont_read_page_op(chip, 0, 0, NULL,
+                                           mtd->writesize,

true))
+               chip->controller->supported_op.cont_read = 1;

drivers/mtd/nand/raw/nand_base.c +2 -0

+}
+

 static void rawnand_late_check_supported_ops(struct nand_chip *chip)
 {
        /* The supported_op fields should not be set by individual
drivers */
+       WARN_ON_ONCE(chip->controller->supported_op.cont_read);
        if (!nand_has_exec_op(chip))
                return;
+
+       rawnand_check_cont_read_support(chip);
 }

 /*

include/linux/mtd/rawnand.h +0 -0

index 28c5dce782dd..1b0936fe3c6e 100644
@@ -67,6 +67,8 @@ struct gpio_desc;

 /* Extended commands for large page devices */
 #define NAND_CMD_READSTART     0x30

include/linux/mtd/rawnand.h +2 -0

+#define NAND_CMD_READCACHESEQ  0x31
+#define NAND_CMD_READCACHEEND  0x3f

 #define NAND_CMD_RNDOUTSTART   0xE0
 #define NAND_CMD_CACHEDPROG    0x15

include/linux/mtd/rawnand.h +0 -0

@@ -1099,12 +1101,14 @@ struct nand_controller_ops {
   * @supported_op.data_only_read: The controller supports reading

more data from
  *                     the bus without restarting an entire read operation nor
  *                     changing the column.
+ * @supported_op.cont_read: The controller supports sequential cache
reads.
  */
 struct nand_controller {
        struct mutex lock;
        const struct nand_controller_ops *ops;
        struct {
                unsigned int data_only_read: 1;
+               unsigned int cont_read: 1;
        } supported_op;
 };

include/linux/mtd/rawnand.h +1 -0

@@ -1308,6 +1312,11 @@ struct nand_chip {
         int read_retries;
         struct nand_secure_region *secure_regions;
         u8 nr_secure_regions;
+       struct {

Quoted 8 lines

Quoted 7 lines

include/linux/mtd/rawnand.h +4 -0

+               bool ongoing;
+               unsigned int first_page;
+               unsigned int last_page;
+       } cont_read;

        /* Externals */
        struct nand_controller *controller;

Hi Miquel,

Quoted 133 lines · Miquel Raynal

Hi Martin,

[email protected] wrote on Fri, 08 Sep 2023 14:25:59 +0200:

Quoted 127 lines

Hi Miquel et al.

Quoted 66 lines · Miquel Raynal

From: JaimeLiao <[email protected]>

Add support for sequential cache reads for controllers using the generic
core helpers for their fast read/write helpers.

Sequential reads may reduce the overhead when accessing physically
continuous data by loading in cache the next page while the previous
page gets sent out on the NAND bus.

The ONFI specification provides the following additional commands to
handle sequential cached reads:

* 0x31 - READ CACHE SEQUENTIAL:
  Requires the NAND chip to load the next page into cache while keeping
  the current cache available for host reads.
* 0x3F - READ CACHE END:
  Tells the NAND chip this is the end of the sequential cache read,
the
  current cache shall remain accessible for the host but no more internal cache loading operation is required.

On the bus, a multi page read operation is currently handled like this:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR+tRR) -- DATA1_IN 00 -- ADDR2 -- 30 -- WAIT_RDY (tR+tRR) -- DATA2_IN 00 -- ADDR3 -- 30 -- WAIT_RDY (tR+tRR) -- DATA3_IN

Sequential cached reads may instead be achieved with:

        00 -- ADDR1 -- 30 -- WAIT_RDY (tR) -- \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA1_IN \
                       31 -- WAIT_RDY (tRCBSY+tRR) -- DATA2_IN \
                       3F -- WAIT_RDY (tRCBSY+tRR) -- DATA3_IN

Below are the read speed test results with regular reads and sequential cached reads, on NXP i.MX6 VAR-SOM-SOLO in mapping mode
with
a NAND chip characterized with the following timings:
* tR: 20 µs
* tRCBSY: 5 µs
* tRR: 20 ns
and the following geometry:
* device size: 2 MiB
* eraseblock size: 128 kiB
* page size: 2 kiB

============= Normal read @ 33MHz =================
mtd_speedtest: eraseblock read speed is 15633 KiB/s
mtd_speedtest: page read speed is 15515 KiB/s
mtd_speedtest: 2 page read speed is 15398 KiB/s
===================================================

========= Sequential cache read @ 33MHz ===========
mtd_speedtest: eraseblock read speed is 18285 KiB/s
mtd_speedtest: page read speed is 15875 KiB/s
mtd_speedtest: 2 page read speed is 16253 KiB/s
===================================================

Quoted 8 lines

We observe an overall speed improvement of about 5% when reading 2 pages, up to 15% when reading an entire block. This is due to the
~14us gain on each additional page read (tR - (tRCBSY + tRR)).

Co-developed-by Miquel Raynal <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

Signed-off-by JaimeLiao <[email protected]>

This patch broke our imx6ull system after doing a kernel upgrade:
[    2.921886] ubi0: default fastmap pool size: 100
[    2.926612] ubi0: default fastmap WL pool size: 50
[    2.931421] ubi0: attaching mtd1
[    3.515799] ubi0: scanning is finished
[    3.525237] ubi0 error: vtbl_check: bad CRC at record 0:
0x88cdfb6,
not 0xffffffff
[    3.532937] Volume table record 0 dump:
[    3.536783]  reserved_pebs   -1
[    3.539932]  alignment       -1
[    3.543101]  data_pad        -1
[    3.546251]  vol_type        255
[    3.549485]  upd_marker      255
[    3.552746]  name_len        65535
[    3.556155]  1st 5 characters of name:
[    3.560429]  crc             0xffffffff
[    3.564294] ubi0 error: vtbl_check: bad CRC at record 0:
0x88cdfb6,
not 0xffffffff
[    3.571892] Volume table record 0 dump:
[    3.575754]  reserved_pebs   -1
[    3.578906]  alignment       -1
[    3.582049]  data_pad        -1
[    3.585216]  vol_type        255
[    3.588452]  upd_marker      255
[    3.591687]  name_len        65535
[    3.595108]  1st 5 characters of name:
[    3.599384]  crc             0xffffffff
[    3.603285] ubi0 error: ubi_read_volume_table: both volume tables
are corrupted
[    3.611460] ubi0 error: ubi_attach_mtd_dev: failed to attach mtd1,
error -22
[    3.618760] UBI error: cannot attach mtd1
[    3.622831] UBI: block: can't open volume on ubi0_4, err=-19 [    3.628505] UBI: block: can't open volume on ubi0_6, err=-19 [    3.634196] UBI: block: can't open volume on ubi0_7, err=-19

It fails consistently at every attach operation. As mentioned above,
this is on an i.MX6ULL and a Toshiba NAND chip:
[    0.530121] nand: device found, Manufacturer ID: 0x98, Chip ID:
0xdc
[    0.530173] nand: Toshiba NAND 512MiB 3,3V 8-bit
[    0.530194] nand: 512 MiB, SLC, erase size: 256 KiB, page size:
4096, OOB size: 128

Quoted 1 line

I'm happy to perform experiments to fix this.

Quoted 10 lines

I received two other reports using another controller and two different
chips, we investigated the timings which could be the issue but found nothing relevant. I thought it was specific to the controller (or its driver) but if you reproduce on imx6 it must be something else. Especially since this series was also tested on imx6. So maybe some devices do not really support what they advertise? Or they expect another sequence? I need to investigate this further but I am a bit clueless.

Quoted 1 line

For what it's worth: I've tested sequential read on an almost identical board that has a Micron flash instead of the Toshiba one:

[    1.370336] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xdc
[    1.376830] nand: Micron MT29F4G08ABAFAWP
[    1.380870] nand: 512 MiB, SLC, erase size: 256 KiB, page size:

4096, OOB size: 256

Fist, ubi seems to be happy:

[    2.702415] ubi0: default fastmap pool size: 100                   
[    2.707138] ubi0: default fastmap WL pool size: 50                 
[    2.711946] ubi0: attaching mtd1                                   
[    3.528830] ubi0: scanning is finished                             
[    3.540626] ubi0: attached mtd1 (name "ubi", size 504 MiB)         
[    3.546235] ubi0: PEB size: 262144 bytes (256 KiB), LEB size: 253952
bytes                                                                 
[    3.553169] ubi0: min./max. I/O unit sizes: 4096/4096, sub-page size
4096                                                                  
[    3.559972] ubi0: VID header offset: 4096 (aligned 4096), data
offset: 8192                                                          
[    3.566968] ubi0: good PEBs: 2012, bad PEBs: 4, corrupted PEBs: 0  
[    3.573096] ubi0: user volume: 9, internal volumes: 1, max. volumes
count: 128                                                            
[    3.580330] ubi0: max/mean erase counter: 5/2, WL threshold: 4096,
image sequence number: 1298270752                                     
[    3.589496] ubi0: available PEBs: 12, total reserved PEBs: 2000,
PEBs reserved for bad PEB handling: 36                                
[    3.598945] ubi0: background thread "ubi_bgt0d" started, PID 35    
[    3.607409] block ubiblock0_4: created from ubi0:4(rootfs.a)       
[    3.615190] block ubiblock0_6: created from ubi0:6(appfs.a)        
[    3.622888] block ubiblock0_7: created from ubi0:7(appfs.b)

But then squashfs fails (we're using ubiblock devices):

[    3.651818] VFS: Mounted root (squashfs filesystem) readonly on
device 254:0.                                                         
[    3.660646] devtmpfs: mounted                                      
[    3.665515] Freeing unused kernel image (initmem) memory: 1024K    
[    3.672816] Run /sbin/init as init process                         
[    3.678097] SQUASHFS error: Unable to read inode 0x8008115d        
[    3.683917] Starting init: /sbin/init exists but couldn't execute it
(error -22)                                                           
[    3.691340] Run /etc/init as init process                          
[    3.697553] Run /bin/init as init process                          
[    3.702982] Run /bin/sh as init process

[    2.702415] ubi0: default fastmap pool size: 100
[    2.707138] ubi0: default fastmap WL pool size: 50
[    2.711946] ubi0: attaching mtd1                                   
[    3.528830] ubi0: scanning is finished                             
[    3.540626] ubi0: attached mtd1 (name "ubi", size 504 MiB)
[    3.546235] ubi0: PEB size: 262144 bytes (256 KiB), LEB size: 253952
bytes
[    3.553169] ubi0: min./max. I/O unit sizes: 4096/4096, sub-page size
4096
[    3.559972] ubi0: VID header offset: 4096 (aligned 4096), data

offset: 8192

[    3.566968] ubi0: good PEBs: 2012, bad PEBs: 4, corrupted PEBs: 0
[    3.573096] ubi0: user volume: 9, internal volumes: 1, max. volumes
count: 128
[    3.580330] ubi0: max/mean erase counter: 5/2, WL threshold: 4096,
image sequence number: 1298270752
[    3.589496] ubi0: available PEBs: 12, total reserved PEBs: 2000,
PEBs reserved for bad PEB handling: 36
[    3.598945] ubi0: background thread "ubi_bgt0d" started, PID 35

I think the patch should be reverted, until we get some more insight.