| Message ID | 20201118182459.18197-2-p.yadav@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it | expand |
On 11/18/20 8:24 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > For NOR flashes EC and VID are zeroed out before an erase is issued to > make sure UBI does not mistakenly treat the PEB as used and associate it > with an LEB. > > But on some flashes, like the Cypress Semper S28 SPI NOR flash family, > multi-pass page programming is not allowed on the default ECC scheme. > This means zeroing out these magic numbers will result in the flash > throwing a page programming error. > > Do not zero out EC and VID for such flashes. A writesize > 1 is an > indication of an ECC-ed flash. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > Notes: > Changes in v2: > > - Use mtd->writesize to check if multi-pass programming can be done > instead of using MTD_NO_MULTI_PASS_WRITE. > - Remove the assertion that a NOR flash most have writesize of 1. > > drivers/mtd/ubi/build.c | 4 +--- > drivers/mtd/ubi/io.c | 9 ++++++++- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index e85b04e9716b..25fd7816b1f4 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -628,10 +628,8 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024) > ubi->bad_peb_limit = get_bad_peb_limit(ubi, max_beb_per1024); > } > > - if (ubi->mtd->type == MTD_NORFLASH) { > - ubi_assert(ubi->mtd->writesize == 1); > + if (ubi->mtd->type == MTD_NORFLASH) > ubi->nor_flash = 1; > - } > > ubi->min_io_size = ubi->mtd->writesize; > ubi->hdrs_min_io_size = ubi->mtd->writesize >> ubi->mtd->subpage_sft; > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c > index 14d890b00d2c..2f3312c31e51 100644 > --- a/drivers/mtd/ubi/io.c > +++ b/drivers/mtd/ubi/io.c > @@ -535,7 +535,14 @@ int ubi_io_sync_erase(struct ubi_device *ubi, int pnum, int torture) > return -EROFS; > } > > - if (ubi->nor_flash) { > + /* > + * If the flash is ECC-ed then we have to erase the ECC block before we > + * can write to it. But the write is in preparation to an erase in the > + * first place. This means we cannot zero out EC and VID before the > + * erase and we just have to hope the flash starts erasing from the > + * start of the page. > + */ > + if (ubi->nor_flash && ubi->mtd->writesize == 1) { Are there any SPI NORs with ECC block size of 4 bytes? Should we call nor_erase_prepare() in this case? Anyway, there's none in SPI NOR as of now, so: Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > err = nor_erase_prepare(ubi, pnum); > if (err) > return err; > -- > 2.28.0 >
On 28/11/20 10:58AM, Tudor.Ambarus@microchip.com wrote: > On 11/18/20 8:24 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > For NOR flashes EC and VID are zeroed out before an erase is issued to > > make sure UBI does not mistakenly treat the PEB as used and associate it > > with an LEB. > > > > But on some flashes, like the Cypress Semper S28 SPI NOR flash family, > > multi-pass page programming is not allowed on the default ECC scheme. > > This means zeroing out these magic numbers will result in the flash > > throwing a page programming error. > > > > Do not zero out EC and VID for such flashes. A writesize > 1 is an > > indication of an ECC-ed flash. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > > > Notes: > > Changes in v2: > > > > - Use mtd->writesize to check if multi-pass programming can be done > > instead of using MTD_NO_MULTI_PASS_WRITE. > > - Remove the assertion that a NOR flash most have writesize of 1. > > > > drivers/mtd/ubi/build.c | 4 +--- > > drivers/mtd/ubi/io.c | 9 ++++++++- > > 2 files changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > > index e85b04e9716b..25fd7816b1f4 100644 > > --- a/drivers/mtd/ubi/build.c > > +++ b/drivers/mtd/ubi/build.c > > @@ -628,10 +628,8 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024) > > ubi->bad_peb_limit = get_bad_peb_limit(ubi, max_beb_per1024); > > } > > > > - if (ubi->mtd->type == MTD_NORFLASH) { > > - ubi_assert(ubi->mtd->writesize == 1); > > + if (ubi->mtd->type == MTD_NORFLASH) > > ubi->nor_flash = 1; > > - } > > > > ubi->min_io_size = ubi->mtd->writesize; > > ubi->hdrs_min_io_size = ubi->mtd->writesize >> ubi->mtd->subpage_sft; > > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c > > index 14d890b00d2c..2f3312c31e51 100644 > > --- a/drivers/mtd/ubi/io.c > > +++ b/drivers/mtd/ubi/io.c > > @@ -535,7 +535,14 @@ int ubi_io_sync_erase(struct ubi_device *ubi, int pnum, int torture) > > return -EROFS; > > } > > > > - if (ubi->nor_flash) { > > + /* > > + * If the flash is ECC-ed then we have to erase the ECC block before we > > + * can write to it. But the write is in preparation to an erase in the > > + * first place. This means we cannot zero out EC and VID before the > > + * erase and we just have to hope the flash starts erasing from the > > + * start of the page. > > + */ > > + if (ubi->nor_flash && ubi->mtd->writesize == 1) { > > Are there any SPI NORs with ECC block size of 4 bytes? Should we call > nor_erase_prepare() in this case? None that I know of. But even if there was such a flash this check would continue to be correct. Like Vignesh explained in the previous version, mtd->writesize > 1 means that multi-pass writes are not allowed in that ECC block. So even if a flash has ECC block size of 4, we still are not allowed to write 4 (or less) bytes to it before erasing first. > Anyway, there's none in SPI NOR as of now, so: > > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > > err = nor_erase_prepare(ubi, pnum); > > if (err) > > return err; > > -- > > 2.28.0 > > >
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index e85b04e9716b..25fd7816b1f4 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -628,10 +628,8 @@ static int io_init(struct ubi_device *ubi, int max_beb_per1024) ubi->bad_peb_limit = get_bad_peb_limit(ubi, max_beb_per1024); } - if (ubi->mtd->type == MTD_NORFLASH) { - ubi_assert(ubi->mtd->writesize == 1); + if (ubi->mtd->type == MTD_NORFLASH) ubi->nor_flash = 1; - } ubi->min_io_size = ubi->mtd->writesize; ubi->hdrs_min_io_size = ubi->mtd->writesize >> ubi->mtd->subpage_sft; diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index 14d890b00d2c..2f3312c31e51 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -535,7 +535,14 @@ int ubi_io_sync_erase(struct ubi_device *ubi, int pnum, int torture) return -EROFS; } - if (ubi->nor_flash) { + /* + * If the flash is ECC-ed then we have to erase the ECC block before we + * can write to it. But the write is in preparation to an erase in the + * first place. This means we cannot zero out EC and VID before the + * erase and we just have to hope the flash starts erasing from the + * start of the page. + */ + if (ubi->nor_flash && ubi->mtd->writesize == 1) { err = nor_erase_prepare(ubi, pnum); if (err) return err;
For NOR flashes EC and VID are zeroed out before an erase is issued to make sure UBI does not mistakenly treat the PEB as used and associate it with an LEB. But on some flashes, like the Cypress Semper S28 SPI NOR flash family, multi-pass page programming is not allowed on the default ECC scheme. This means zeroing out these magic numbers will result in the flash throwing a page programming error. Do not zero out EC and VID for such flashes. A writesize > 1 is an indication of an ECC-ed flash. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- Notes: Changes in v2: - Use mtd->writesize to check if multi-pass programming can be done instead of using MTD_NO_MULTI_PASS_WRITE. - Remove the assertion that a NOR flash most have writesize of 1. drivers/mtd/ubi/build.c | 4 +--- drivers/mtd/ubi/io.c | 9 ++++++++- 2 files changed, 9 insertions(+), 4 deletions(-)