Message ID | 20230815161024.810729-1-mmkurbanov@sberdevices.ru |
---|---|
State | New |
Headers | show |
Series | [v1] mtd: spinand: micron: correct parameters | expand |
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.
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; }
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(-)