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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/