Re: [PATCH] mtd: rawnand: denali: add DT property to specify skipped bytes in OOB

[Boris Brezillon]

On Sat, 22 Sep 2018 09:41:11 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Masahiro,
> 
> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote on Sat, 8 Sep
> 2018 01:10:25 +0900:
> 
> > Hi Boris,
> > 
> > 2018-09-07 23:53 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxx>:  
> > > On Fri, 7 Sep 2018 23:42:53 +0900
> > > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> > >    
> > >> Hi Boris,
> > >>
> > >> 2018-09-07 23:08 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxx>:    
> > >> > Hi Masahiro,
> > >> >
> > >> > On Fri,  7 Sep 2018 19:56:23 +0900
> > >> > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> > >> >    
> > >> >> NAND devices need additional data area (OOB) for error correction,
> > >> >> but it is also used for Bad Block Marker (BBM).  In many cases, the
> > >> >> first byte in OOB is used for BBM, but the location actually depends
> > >> >> on chip vendors.  The NAND controller should preserve the precious
> > >> >> BBM to keep track of bad blocks.
> > >> >>
> > >> >> In Denali IP, the SPARE_AREA_SKIP_BYTES register is used to specify
> > >> >> the number of bytes to skip from the start of OOB.  The ECC engine
> > >> >> will automatically skip the specified number of bytes when it gets
> > >> >> access to OOB area.
> > >> >>
> > >> >> The same value for SPARE_AREA_SKIP_BYTES should be used between
> > >> >> firmware and the operating system if you intend to use the NAND
> > >> >> device across the control hand-off.
> > >> >>
> > >> >> In fact, the current denali.c code expects firmware to have already
> > >> >> set the SPARE_AREA_SKIP_BYTES register, then reads the value out.
> > >> >>
> > >> >> If no firmware (or bootloader) has initialized the controller, the
> > >> >> register value is zero, which is the default after power-on-reset.
> > >> >>
> > >> >> In other words, the Linux driver cannot initialize the controller
> > >> >> by itself.  You cannot support the reset control either because
> > >> >> resetting the controller will get register values lost.
> > >> >>
> > >> >> This commit adds a way to specify it via DT.  If the property
> > >> >> "denali,oob-skip-bytes" exists, the value will be set to the register.    
> > >> >
> > >> > Hm, do we really need to make this config customizable? I mean, either
> > >> > you have a large-page NAND (page > 512 bytes) and the 2 first bytes
> > >> > must be reserved for the BBM or you have a small-page NAND and the BBM
> > >> > is at position 4 and 5. Are you sure people configure that differently?
> > >> > Don't you always have SPARE_AREA_SKIP_BYTES set to 6 or 2?    
> > >>
> > >>
> > >> As I said in the patch description,
> > >> I need to use the same SPARE_AREA_SKIP_BYTES value
> > >> across firmware, boot-loader, Linux, and whatever.
> > >>
> > >> I want to set the value to 8 for my platform
> > >> because the on-chip boot ROM expects 8.
> > >> I cannot change it since the boot ROM is hard-wired.
> > >>
> > >>
> > >> The boot ROM skips 8 bytes in OOB
> > >> when it loads images from the on-board NAND device.
> > >>
> > >> So, when I update the image from U-Boot or Linux,
> > >> I need to make sure to set the register to 8.
> > >>
> > >> If I update the image with a different value,
> > >> the Boot ROM fails to boot.
> > >>
> > >>
> > >>
> > >> When the system has booted from NAND,
> > >> the register is already set to 8.  It works.
> > >>
> > >> However, when the system has booted from eMMC,
> > >> the register is not initialized by anyone.
> > >> I am searching for a way to set the register to 8
> > >> in this case.

Maybe there's a solution which does not involve attaching a per-compat
value or adding a DT prop. If the FW/bootloader has not initialized this
register the value is 0, right? Why not testing the value and
assigning it to the default (8) if it's not been initialized by the
bootloader. That shouldn't break existing platforms since I don't think
0 is a valid value anyway.

	denali->oob_skip_bytes = ioread32(denali->reg +
					  SPARE_AREA_SKIP_BYTES);
	if (!denali->oob_skip_bytes) {
		denali->oob_skip_bytes = DEFAULT_OOB_SKIP_BYTES;
		iowrite32(denali->oob_skip_bytes,
			  denali->reg + SPARE_AREA_SKIP_BYTES);
	}