| Message ID | 20230822122534.872646-3-mmkurbanov@sberdevices.ru |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: spinand: micron: correct parameters | expand |
On 22.08.23 14:25, Martin Kurbanov wrote: > The first 4 bytes are reserved for bad block data. > > Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru> This should probably also have a Fixes tag like: Fixes: a508e8875e13 ("mtd: spinand: Add initial support for Micron MT29F2G01ABAGD") Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/mtd/nand/spi/micron.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c > index 12601bc4227a..64b41c7c9cce 100644 > --- a/drivers/mtd/nand/spi/micron.c > +++ b/drivers/mtd/nand/spi/micron.c > @@ -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,9 @@ 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; > + region->length = (mtd->oobsize / 2) - 4; > > return 0; > }
Hi Martin, mmkurbanov@sberdevices.ru wrote on Tue, 22 Aug 2023 15:25:34 +0300: > The first 4 bytes are reserved for bad block data. Are you sure about that? I've never seen 4-bytes BBM. > Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru> > --- > drivers/mtd/nand/spi/micron.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c > index 12601bc4227a..64b41c7c9cce 100644 > --- a/drivers/mtd/nand/spi/micron.c > +++ b/drivers/mtd/nand/spi/micron.c > @@ -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,9 @@ 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; > + region->length = (mtd->oobsize / 2) - 4; > > return 0; > } Thanks, Miquèl
Hi Miquel, On 22.08.2023 16:35, Miquel Raynal wrote: > Hi Martin, > > mmkurbanov@sberdevices.ru wrote on Tue, 22 Aug 2023 15:25:34 +0300: > >> The first 4 bytes are reserved for bad block data. > > Are you sure about that? I've never seen 4-bytes BBM. Yes, I'm sure. I have checked in all the relevant datasheets: ---------------------------------------------------------------------------------- | Name | Max Byte | Min Byte | Area | Description | | | Address | Address | | | |--------------------+----------+----------+---------+---------------------------| | MT29F2G01ABAGD [1] | | | | | |--------------------| | | | | | MT29F2G01ABBGD [2] | | | | | |--------------------| 803h | 800h | | | | MT29F1G01ABAFD [3] | | | | | |--------------------| | | | | | MT29F4G01ADAGD [4] | | | | | |--------------------+----------+----------| Spare 0 | Reserved (bad block data) | | MT29F4G01ABAFD [5] | | | | | |--------------------| | | | | | MT29F4G01ABBFD [6] | | | | | |--------------------| 1003h | 1000h | | | | MT29F8G01ADAFD [7] | | | | | |--------------------| | | | | | MT29F8G01ADBFD [8] | | | | | ---------------------------------------------------------------------------------- Note: to view the datasheet on the Micron (https://www.micron.com/) website, you need to register. Links: [1] https://datasheet.lcsc.com/lcsc/1912111437_Micron-Tech-MT29F2G01ABAGDWB-IT-G_C410863.pdf - page 45 [2] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m79a_2gb_1_8v_nand_spi.pdf - page 46 [3] https://datasheet.lcsc.com/lcsc/2209201030_Micron-Tech-MT29F1G01ABAFDWB-IT-F_C2905686.pdf - page 46 [4] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m79a_ddp_4gb_3v_nand_spi.pdf - page 46 [5] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_3v_nand_spi.pdf - page 47 [6] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_1_8v_nand_spi.pdf - page 49 [7] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_3v_nand_spi.pdf - page 48 [8] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_1_8v_nand_spi.pdf - page 48
Hi Martin, mmkurbanov@sberdevices.ru wrote on Tue, 22 Aug 2023 19:57:11 +0300: > Hi Miquel, > > On 22.08.2023 16:35, Miquel Raynal wrote: > > Hi Martin, > > > > mmkurbanov@sberdevices.ru wrote on Tue, 22 Aug 2023 15:25:34 +0300: > > > >> The first 4 bytes are reserved for bad block data. > > > > Are you sure about that? I've never seen 4-bytes BBM. > > Yes, I'm sure. I have checked in all the relevant datasheets: > ---------------------------------------------------------------------------------- > | Name | Max Byte | Min Byte | Area | Description | > | | Address | Address | | | > |--------------------+----------+----------+---------+---------------------------| > | MT29F2G01ABAGD [1] | | | | | > |--------------------| | | | | > | MT29F2G01ABBGD [2] | | | | | > |--------------------| 803h | 800h | | | > | MT29F1G01ABAFD [3] | | | | | > |--------------------| | | | | > | MT29F4G01ADAGD [4] | | | | | > |--------------------+----------+----------| Spare 0 | Reserved (bad block data) | > | MT29F4G01ABAFD [5] | | | | | > |--------------------| | | | | > | MT29F4G01ABBFD [6] | | | | | > |--------------------| 1003h | 1000h | | | > | MT29F8G01ADAFD [7] | | | | | > |--------------------| | | | | > | MT29F8G01ADBFD [8] | | | | | > ---------------------------------------------------------------------------------- > > Note: to view the datasheet on the Micron (https://www.micron.com/) website, you need > to register. I don't think the four bytes have any "bad block specific" meaning. In practice, the datasheet states: Value programmed for bad block at the first byte of spare area: 00h So only the first byte is used to mark the block bad, the rest is probably marked "reserved" for simplicity. I believe we should keep the current layout because it would otherwise break users for no real reason. > Links: > [1] https://datasheet.lcsc.com/lcsc/1912111437_Micron-Tech-MT29F2G01ABAGDWB-IT-G_C410863.pdf - page 45 > [2] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m79a_2gb_1_8v_nand_spi.pdf - page 46 > [3] https://datasheet.lcsc.com/lcsc/2209201030_Micron-Tech-MT29F1G01ABAFDWB-IT-F_C2905686.pdf - page 46 > [4] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m79a_ddp_4gb_3v_nand_spi.pdf - page 46 > [5] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_3v_nand_spi.pdf - page 47 > [6] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_4gb_1_8v_nand_spi.pdf - page 49 > [7] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_3v_nand_spi.pdf - page 48 > [8] https://www.micron.com/-/media/client/global/documents/products/data-sheet/nand-flash/70-series/m70a_ddp_8gb_1_8v_nand_spi.pdf - page 48 > Thanks, Miquèl
Hi Miquel, On 23.08.2023 11:41, Miquel Raynal wrote: > Hi Martin, > > I don't think the four bytes have any "bad block specific" meaning. In > practice, the datasheet states: > > Value programmed for bad block at the first byte of spare > area: 00h > > So only the first byte is used to mark the block bad, the rest is > probably marked "reserved" for simplicity. I believe we should keep the > current layout because it would otherwise break users for no real > reason. I agree with you that this can break the work of users who use OOB. However, I believe it would be more appropriate to use an offset of 4, as the micron chip can use all 4 bytes for additional data about the bad block. So, there is a non-zero probability of losing OOB data in the reserved area (2 bytes) when the hardware chip attempts to mark the block as bad.
Hi Martin, mmkurbanov@sberdevices.ru wrote on Wed, 23 Aug 2023 14:33:57 +0300: > Hi Miquel, > > On 23.08.2023 11:41, Miquel Raynal wrote: > > Hi Martin, > > > > I don't think the four bytes have any "bad block specific" meaning. In > > practice, the datasheet states: > > > > Value programmed for bad block at the first byte of spare > > area: 00h > > > > So only the first byte is used to mark the block bad, the rest is > > probably marked "reserved" for simplicity. I believe we should keep the > > current layout because it would otherwise break users for no real > > reason. > > I agree with you that this can break the work of users who use OOB. > However, I believe it would be more appropriate to use an offset of 4, > as the micron chip can use all 4 bytes for additional data about the > bad block. So, there is a non-zero probability of losing OOB data in > the reserved area (2 bytes) when the hardware chip attempts to mark > the block as bad. Is this really a process the chip can do? Aren't bad blocks factory marked only? Then it's mtd's duty to manage them. Thanks, Miquèl
On 23.08.2023 14:39, Miquel Raynal wrote: > Hi Martin, > > mmkurbanov@sberdevices.ru wrote on Wed, 23 Aug 2023 14:33:57 +0300: > >> Hi Miquel, >> >> On 23.08.2023 11:41, Miquel Raynal wrote: >>> Hi Martin, >>> >>> I don't think the four bytes have any "bad block specific" meaning. In >>> practice, the datasheet states: >>> >>> Value programmed for bad block at the first byte of spare >>> area: 00h >>> >>> So only the first byte is used to mark the block bad, the rest is >>> probably marked "reserved" for simplicity. I believe we should keep the >>> current layout because it would otherwise break users for no real >>> reason. >> >> I agree with you that this can break the work of users who use OOB. >> However, I believe it would be more appropriate to use an offset of 4, >> as the micron chip can use all 4 bytes for additional data about the >> bad block. So, there is a non-zero probability of losing OOB data in >> the reserved area (2 bytes) when the hardware chip attempts to mark >> the block as bad. > > Is this really a process the chip can do? Aren't bad blocks factory > marked only? Actually, there is my understanding, I’m not sure exactly.
Hi Miquel, On 24.08.2023 12:35, Martin Kurbanov wrote: > > > On 23.08.2023 14:39, Miquel Raynal wrote: >> Hi Martin, >> >> mmkurbanov@sberdevices.ru wrote on Wed, 23 Aug 2023 14:33:57 +0300: >> >>> Hi Miquel, >>> >>> On 23.08.2023 11:41, Miquel Raynal wrote: >>>> Hi Martin, >>>> >>>> I don't think the four bytes have any "bad block specific" meaning. In >>>> practice, the datasheet states: >>>> >>>> Value programmed for bad block at the first byte of spare >>>> area: 00h >>>> >>>> So only the first byte is used to mark the block bad, the rest is >>>> probably marked "reserved" for simplicity. I believe we should keep the >>>> current layout because it would otherwise break users for no real >>>> reason. >>> >>> I agree with you that this can break the work of users who use OOB. >>> However, I believe it would be more appropriate to use an offset of 4, >>> as the micron chip can use all 4 bytes for additional data about the >>> bad block. So, there is a non-zero probability of losing OOB data in >>> the reserved area (2 bytes) when the hardware chip attempts to mark >>> the block as bad. >> >> Is this really a process the chip can do? Aren't bad blocks factory >> marked only? > > Actually, there is my understanding, I’m not sure exactly. I tested with an offset of 2, no read/write errors were detected (including read/write to OOB). But I don't have a flash chip with factory bad blocks yet, when I find such a flash, I will report the results. Do I need to send the v3 of the patch with only first commit ("correct bitmask for ecc status")?
Hi Martin, mmkurbanov@sberdevices.ru wrote on Mon, 4 Sep 2023 17:20:59 +0300: > Hi Miquel, > > On 24.08.2023 12:35, Martin Kurbanov wrote: > > > > > > On 23.08.2023 14:39, Miquel Raynal wrote: > >> Hi Martin, > >> > >> mmkurbanov@sberdevices.ru wrote on Wed, 23 Aug 2023 14:33:57 +0300: > >> > >>> Hi Miquel, > >>> > >>> On 23.08.2023 11:41, Miquel Raynal wrote: > >>>> Hi Martin, > >>>> > >>>> I don't think the four bytes have any "bad block specific" meaning. In > >>>> practice, the datasheet states: > >>>> > >>>> Value programmed for bad block at the first byte of spare > >>>> area: 00h > >>>> > >>>> So only the first byte is used to mark the block bad, the rest is > >>>> probably marked "reserved" for simplicity. I believe we should keep the > >>>> current layout because it would otherwise break users for no real > >>>> reason. > >>> > >>> I agree with you that this can break the work of users who use OOB. > >>> However, I believe it would be more appropriate to use an offset of 4, > >>> as the micron chip can use all 4 bytes for additional data about the > >>> bad block. So, there is a non-zero probability of losing OOB data in > >>> the reserved area (2 bytes) when the hardware chip attempts to mark > >>> the block as bad. > >> > >> Is this really a process the chip can do? Aren't bad blocks factory > >> marked only? > > > > Actually, there is my understanding, I’m not sure exactly. > > I tested with an offset of 2, no read/write errors were detected > (including read/write to OOB). But I don't have a flash chip with > factory bad blocks yet, when I find such a flash, I will report the > results. Ok. > Do I need to send the v3 of the patch with only first commit ("correct > bitmask for ecc status")? Yes please, with Frieder's comments fixed. Thanks, Miquèl
diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c index 12601bc4227a..64b41c7c9cce 100644 --- a/drivers/mtd/nand/spi/micron.c +++ b/drivers/mtd/nand/spi/micron.c @@ -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,9 @@ 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; + region->length = (mtd->oobsize / 2) - 4; return 0; }
The first 4 bytes are reserved for bad block data. Signed-off-by: Martin Kurbanov <mmkurbanov@sberdevices.ru> --- drivers/mtd/nand/spi/micron.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)