Re: [PATCH] mtd: spinand: Add support for HeYangTek HYF1GQ4UDACAE

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

Hi Maxim,

maxim.anisimov.ua@gmail.com wrote on Mon, 22 Jul 2024 09:54:55 +0300:

> Hi Miquel,
> 
> Sorry for late answer. I was on vocation. To be honest, I don't understand very well how this works. One of the OpenWrt developers asked me to send this patch to Linux upstream. Related OpenWrt pull request is here https://github.com/openwrt/openwrt/pull/15551 . Originally this patch code was taken from the device manufacturer's repository https://github.com/keenetic/kernel-49/commit/bacade569fb12bc0ad31ba09bca9b890118fbca7 . I pointed to this in the source code. From my side I only adapted the code for successful compilation on the Linux upstream. It's difficult for me to answer your questions because I don't understand how the spi nand framework works. I can make corrections in the patch but with outside help. Thanks!

I'm sorry I cannot write the changes for you, but I can explain my
point below.

> >> +static int hyfxgq4uda_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +				   struct mtd_oob_region *region)
> >> +{
> >> +	if (section > 3)
> >> +		return -ERANGE;
> >> +
> >> +	region->offset = section * 16 + 8;
> >> +	region->length = 8;  
> > This is: 8-15, 24-31, 40-47, 56-62
> >  
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int hyfxgq4uda_ooblayout_free(struct mtd_info *mtd, int section,
> >> +				   struct mtd_oob_region *region)
> >> +{
> >> +	if (section > 3)
> >> +		return -ERANGE;
> >> +
> >> +	/* ECC-protected user meta-data */
> >> +	region->offset = section * 16 + 4;
> >> +	region->length = 4;  
> > This is: 4-7, 20-23, 32-35, 48-51
> >
> > So what about 2-4, 16-19, 36-39, 52-55, 63-64 ?

The OOB area is a specific area where we store:
- the bad block marker: it is at offset 0 and 1 and must be excluded
  from both functions.
- ECC bytes, so data for the correction engine: the amount depend on
  the strength of your correction and the position depends on the
  hardware. If this is not told in the datasheet explicitly you can
  play with flash_erase/nandwrite/nanddump: you can write a known
  pattern to an OOB area with the ECC engine enabled and you'll see
  what bytes have been smashed by the ECC engine.
- the rest is user data (typically jffs2 metadata).

In the current implementation, some bytes are just undefined, which is
not expected.

> >  
> >> +
> >> +	return 0;
> >> +}

...

> >> +static int hyfxgq4uda_ecc_get_status(struct spinand_device *spinand,
> >> +				     u8 status)
> >> +{
> >> +	struct nand_device *nand = spinand_to_nand(spinand);
> >> +
> >> +	switch (status & STATUS_ECC_MASK) {
> >> +	case STATUS_ECC_NO_BITFLIPS:
> >> +		return 0;
> >> +
> >> +	case STATUS_ECC_UNCOR_ERROR:
> >> +		return -EBADMSG;
> >> +
> >> +	case STATUS_ECC_HAS_BITFLIPS:
> >> +		return nanddev_get_ecc_conf(nand)->strength >> 1;  
> > Maybe an explanation of this line is needed. Is this just guessing or
> > is this defined in the datasheet?

The datasheet should tell you what this bit means. If it means some
bits have been corrected up to half of the strength capability, then
this is fine. But please make sure this is right by using nandbiterrs
-i from the mtd-utils test suite.

> > Also please do not use shifts when you want to divide. Just use / 2
> > which is easier to understand. Compilers know how to optimize that.

This is self explanatory.

Thanks,
Miquèl