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

Miquel Raynal
2023-09-22T09:20:48Z

Hi Martin,

martin@geanix.com wrote on Fri, 15 Sep 2023 13:20:10 +0200:

> 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

Can you show me the errors? Did you look at the flash raw content,
anything special that can be seen?

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

I cannot make any sense of all the different feedback I've got (good
and bad). I will revert the patch for now.

Thanks,
Miquèl