Re: [PATCH v1] mtd: spinand: micron: correct parameters

[Frieder Schrempf]

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@xxxxxxxxxxxxxx>
> ---
>  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;
>  }