[v1] mtd: spinand: micron: correct parameters

This patch includes following fixes:
  1. Correct bitmask for ecc status. Valid bitmask is 0x70 in the
      status register.
  2. Fix oob layout:
        - The first 4 bytes are reserved for bad block data.
        - Use only non-protected ECC bytes for free data:
          OOB ECC protected Area is not used due to partial
          programming from some filesystems (like JFFS2 with
          cleanmarkers).

Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
---
 drivers/mtd/nand/spi/micron.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Hi Martin,

On 15.08.23 18:10, Martin Kurbanov wrote:
> This patch includes following fixes:
>   1. Correct bitmask for ecc status. Valid bitmask is 0x70 in the
>       status register.
>   2. Fix oob layout:
>         - The first 4 bytes are reserved for bad block data.
>         - Use only non-protected ECC bytes for free data:
>           OOB ECC protected Area is not used due to partial
>           programming from some filesystems (like JFFS2 with
>           cleanmarkers).

Can you please move 1. and 2. into separate patches? Maybe even
separating the OOB layout change into "fixing the offset" and "reducing
the free area size".

I'm okay with 1. and with adjusting region->offset to 4. But I don't
really get why we want to restrict the free oob data to the
non-ECC-protected area only. Is this specific to Micron? Other SPI NAND
drivers also spread the free area over both, the ECC-protected and the
non-protected bytes. Why do it differently here?

Thanks
Frieder

> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru>
> ---
>  drivers/mtd/nand/spi/micron.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 50b7295bc922..897e70913ed0 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -12,7 +12,7 @@
>  
>  #define SPINAND_MFR_MICRON		0x2c
>  
> -#define MICRON_STATUS_ECC_MASK		GENMASK(7, 4)
> +#define MICRON_STATUS_ECC_MASK		GENMASK(6, 4)
>  #define MICRON_STATUS_ECC_NO_BITFLIPS	(0 << 4)
>  #define MICRON_STATUS_ECC_1TO3_BITFLIPS	(1 << 4)
>  #define MICRON_STATUS_ECC_4TO6_BITFLIPS	(3 << 4)
> @@ -57,6 +57,20 @@ static SPINAND_OP_VARIANTS(x1_write_cache_variants,
>  static SPINAND_OP_VARIANTS(x1_update_cache_variants,
>  			   SPINAND_PROG_LOAD(false, 0, NULL, 0));
>  
> +/*
> + * OOB spare area map (128 and 256 bytes)
> + *
> + *           +-----+-----------------+-------------------+---------------------+
> + *           | BBM |     Non ECC     |   ECC protected   |      ECC Area       |
> + *           |     | protected Area  |       Area        |                     |
> + * ----------+-----+-----------------+-------------------+---------------------+
> + *  oobsize  | 0:3 | 4:31 (28 bytes) | 32:63 (32 bytes)  | 64:127 (64 bytes)   |
> + * 128 bytes |     |                 |                   |                     |
> + * ----------+-----+-----------------+-------------------+---------------------+
> + *  oobsize  | 0:3 | 4:63 (60 bytes) | 64:127 (64 bytes) | 127:255 (128 bytes) |
> + * 256 bytes |     |                 |                   |                     |
> + * ----------+-----+-----------------+-------------------+---------------------+
> + */
>  static int micron_8_ooblayout_ecc(struct mtd_info *mtd, int section,
>  				  struct mtd_oob_region *region)
>  {
> @@ -75,9 +89,15 @@ static int micron_8_ooblayout_free(struct mtd_info *mtd, int section,
>  	if (section)
>  		return -ERANGE;
>  
> -	/* Reserve 2 bytes for the BBM. */
> -	region->offset = 2;
> -	region->length = (mtd->oobsize / 2) - 2;
> +	/* Reserve 4 bytes for the BBM. */
> +	region->offset = 4;
> +
> +	/* The OOB Free (User) area is divided into two equal parts:
> +	 *   the first part is not protected by ECC;
> +	 *   the second part is protected by ECC.
> +	 * Use only non-protected ECC bytes.
> +	 */
> +	region->length = (mtd->oobsize / 2) / 2 - 4;
>  
>  	return 0;
>  }

Hi Frieder.

On 16.08.2023 10:21, Frieder Schrempf wrote:
> I'm okay with 1. and with adjusting region->offset to 4. But I don't
> really get why we want to restrict the free oob data to the
> non-ECC-protected area only. Is this specific to Micron? Other SPI NAND
> drivers also spread the free area over both, the ECC-protected and the
> non-protected bytes. Why do it differently here?

We encountered a problem with the JFFS2 file system: JFFS2 marks erased
blocks with a marker to avoid re-erasing them. To do this, it writes
a special marker (cleanmarker) in the free OOB area. And if this OOB
area is protected by ECC, the ECC will be written. However, during
the next write to the main area of the same block, the ECC will be
incorrect because it's necessary to program both the main area and
the OOB area at one programming time, so that the ECC parity code can
be calculated properly. Other SPI NAND flash also susceptible to
this problem.

On 16.08.23 17:28, Martin Kurbanov wrote:
> Hi Frieder.
> 
> On 16.08.2023 10:21, Frieder Schrempf wrote:
>> I'm okay with 1. and with adjusting region->offset to 4. But I don't
>> really get why we want to restrict the free oob data to the
>> non-ECC-protected area only. Is this specific to Micron? Other SPI NAND
>> drivers also spread the free area over both, the ECC-protected and the
>> non-protected bytes. Why do it differently here?
> 
> We encountered a problem with the JFFS2 file system: JFFS2 marks erased
> blocks with a marker to avoid re-erasing them. To do this, it writes
> a special marker (cleanmarker) in the free OOB area. And if this OOB
> area is protected by ECC, the ECC will be written. However, during
> the next write to the main area of the same block, the ECC will be
> incorrect because it's necessary to program both the main area and
> the OOB area at one programming time, so that the ECC parity code can
> be calculated properly. Other SPI NAND flash also susceptible to
> this problem.

Thanks for the explanation. As this is a generic issue, we need to fix
it in the core and not in the Micron driver.

Also I wonder if JFFS2 should instead write the cleanmarker with ECC
being turned of explicitly.

I don't know enough about NAND and JFFS2 to point out a correct fix, but
I hope Miquel and Richard have some ideas.

Hi Frieder,

frieder.schrempf@kontron.de wrote on Wed, 16 Aug 2023 17:35:56 +0200:

> On 16.08.23 17:28, Martin Kurbanov wrote:
> > Hi Frieder.
> > 
> > On 16.08.2023 10:21, Frieder Schrempf wrote:  
> >> I'm okay with 1. and with adjusting region->offset to 4. But I don't
> >> really get why we want to restrict the free oob data to the
> >> non-ECC-protected area only. Is this specific to Micron? Other SPI NAND
> >> drivers also spread the free area over both, the ECC-protected and the
> >> non-protected bytes. Why do it differently here?  
> > 
> > We encountered a problem with the JFFS2 file system: JFFS2 marks erased
> > blocks with a marker to avoid re-erasing them. To do this, it writes
> > a special marker (cleanmarker) in the free OOB area. And if this OOB
> > area is protected by ECC, the ECC will be written. However, during
> > the next write to the main area of the same block, the ECC will be
> > incorrect because it's necessary to program both the main area and
> > the OOB area at one programming time, so that the ECC parity code can
> > be calculated properly. Other SPI NAND flash also susceptible to
> > this problem.  
> 
> Thanks for the explanation. As this is a generic issue, we need to fix
> it in the core and not in the Micron driver.

It's not the first time we face this issue and the first approach we
used was to "fix" the OOB layout to include all free bytes (not only
protected bytes), which had the nice side-effect of allowing to write
the cleanmarker in an ECC-free area and allow that chip to be used with
JFFS2. This is indeed not a proper solution and I agree we should have
a system-wide solution.

> Also I wonder if JFFS2 should instead write the cleanmarker with ECC
> being turned of explicitly.

The real question is, why would you still want to use JFFS2 on
SPI-NAND? UBI is meant for that. JFFS2 was designed with NORs in mind,
it can be used on small NAND chips because UBI is a bit glutton wrt,
but I doubt we still have "small" SPI-NANDs on the market which require
JFFS2 anymore. Do we?

Anyhow, if people want JFFS2 on NANDs, I agree we should maybe change
how JFFS2 works and force raw accesses when it comes to writing the
cleanmarker, because there is no knowledge of what is ECC protected or
not in the current OOB layouts. I however have no idea of the possible
side-effects, I've never looked into JFFS2 so deeply.

> I don't know enough about NAND and JFFS2 to point out a correct fix, but
> I hope Miquel and Richard have some ideas.

Thanks,
Miquèl

Hi Miquel,

On 17.08.2023 10:53, Miquel Raynal wrote:
> It's not the first time we face this issue and the first approach we
> used was to "fix" the OOB layout to include all free bytes (not only
> protected bytes), which had the nice side-effect of allowing to write
> the cleanmarker in an ECC-free area and allow that chip to be used with
> JFFS2. This is indeed not a proper solution and I agree we should have
> a system-wide solution.
> 
>> Also I wonder if JFFS2 should instead write the cleanmarker with ECC
>> being turned of explicitly.
> The real question is, why would you still want to use JFFS2 on
> SPI-NAND? UBI is meant for that. JFFS2 was designed with NORs in mind,
> it can be used on small NAND chips because UBI is a bit glutton wrt,
> but I doubt we still have "small" SPI-NANDs on the market which require
> JFFS2 anymore. Do we?

Unfortunately, we cannot use UBI because we have a small flash SPI-NAND
and we need a small partition.

> Anyhow, if people want JFFS2 on NANDs, I agree we should maybe change
> how JFFS2 works and force raw accesses when it comes to writing the
> cleanmarker, because there is no knowledge of what is ECC protected or
> not in the current OOB layouts. I however have no idea of the possible
> side-effects, I've never looked into JFFS2 so deeply.

Then I can prepare a patchset that will disable cleanamarkers on the
compile time.
And today I will send the second version of this patch without fixes
to the OOB area.