Re: [PATCH v2 3/3] mtd: rawnand: Support for sequential cache reads

Martin Hundebøll
2023-09-15T11:20:38Z

Hi Miquel,

On Tue, 2023-09-12 at 17:59 +0200, Miquel Raynal wrote:
> Hi Martin,
>
> martin@geanix.com wrote on Fri, 08 Sep 2023 14:25:59 +0200:
>
> > Hi Miquel et al.
> >
> > On Thu, 2023-01-12 at 10:36 +0100, Miquel Raynal wrote:
> > > From: JaimeLiao <jaimeliao.tw@gmail.com>
> > >
> > > 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 <miquel.raynal@bootlin.com>
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Signed-off-by: JaimeLiao <jaimeliao.tw@gmail.com>
> >
> > 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.
>
> 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.
> >

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.

// Martin

> > // Martin
> >
> > > ---
> > >  drivers/mtd/nand/raw/nand_base.c | 119
> > > +++++++++++++++++++++++++++++--
> > >  include/linux/mtd/rawnand.h      |   9 +++
> > >  2 files changed, 124 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > > b/drivers/mtd/nand/raw/nand_base.c
> > > index 34395d5d3a47..0b1fd6bbb36b 100644
> > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > @@ -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);
> > >  }
> > >
> > >  /*
> > > diff --git a/include/linux/mtd/rawnand.h
> > > b/include/linux/mtd/rawnand.h
> > > index 28c5dce782dd..1b0936fe3c6e 100644
> > > --- a/include/linux/mtd/rawnand.h
> > > +++ b/include/linux/mtd/rawnand.h
> > > @@ -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;
> >