[PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This series is equivalent to the one for Linux MTD submitted by Pratyush Yadav.
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=217759&state...
Takahiro Kuwano (4): mtd: ubi: Do not zero out EC and VID on ECC-ed NOR flashes mtd: spi-nor: Allow flashes to specify MTD writesize mtd: spi-nor-core: Rework default_init() to take flash_parameter mtd: spi-nor: Set ECC unit size to MTD writesize in Infineon SEMPER flashes
drivers/mtd/spi/spi-nor-core.c | 45 +++++++++++++++++++++++++--------- drivers/mtd/ubi/build.c | 4 +-- drivers/mtd/ubi/io.c | 9 ++++++- include/linux/mtd/spi-nor.h | 1 + 4 files changed, 44 insertions(+), 15 deletions(-)
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
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 Infineon Semper 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: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- 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 a1941b8eb8..81c1b7bdbc 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -679,10 +679,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 14be95b74b..45699b4a47 100644 --- a/drivers/mtd/ubi/io.c +++ b/drivers/mtd/ubi/io.c @@ -563,7 +563,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;
Hi, Takahiro,
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
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 Infineon Semper 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.
I'm not familiar with the u-boot requirements, but I think a good practice would be to specify if/when a commit follows the upstream linux implementation. It helps reviewers, gives a peace of mind to the maintainer(s), and gives credit to the author. If something breaks all parties can be involved.
This patch replicates the following upstream linux commit: f669e74be820 ("ubi: Do not zero out EC and VID on ECC-ed NOR flashes")
Acked-by: Tudor Ambarus tudor.ambarus@linaro.org
Cheers, ta
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some flashes like the Infineon SEMPER NOR flash family use ECC. Under this ECC scheme, multi-pass writes to an ECC block is not allowed. In other words, once data is programmed to an ECC block, it can't be programmed again without erasing it first.
Upper layers like file systems need to be given this information so they do not cause error conditions on the flash by attempting multi-pass programming. This can be done by setting 'writesize' in 'struct mtd_info'.
Set the default to 1 but allow flashes to modify it in fixup hooks. If more flashes show up with this constraint in the future it might be worth it to add it to 'struct flash_info', but for now increasing its size is not worth it.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 3 ++- include/linux/mtd/spi-nor.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index f86003ca8c..1bfef6797f 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2789,6 +2789,7 @@ static int spi_nor_init_params(struct spi_nor *nor, memset(params, 0, sizeof(*params));
/* Set SPI NOR sizes. */ + params->writesize = 1; params->size = info->sector_size * info->n_sectors; params->page_size = info->page_size;
@@ -4078,7 +4079,7 @@ int spi_nor_scan(struct spi_nor *nor) mtd->dev = nor->dev; mtd->priv = nor; mtd->type = MTD_NORFLASH; - mtd->writesize = 1; + mtd->writesize = params.writesize; mtd->flags = MTD_CAP_NORFLASH; mtd->size = params.size; mtd->_erase = spi_nor_erase; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index d1dbf3eadb..0d37a806c4 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -436,6 +436,7 @@ enum spi_nor_pp_command_index {
struct spi_nor_flash_parameter { u64 size; + u32 writesize; u32 page_size; u8 rdsr_dummy; u8 rdsr_addr_nbytes;
Hi, Takahiro!
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Some flashes like the Infineon SEMPER NOR flash family use ECC. Under this ECC scheme, multi-pass writes to an ECC block is not allowed. In other words, once data is programmed to an ECC block, it can't be programmed again without erasing it first.
Upper layers like file systems need to be given this information so they do not cause error conditions on the flash by attempting multi-pass programming. This can be done by setting 'writesize' in 'struct mtd_info'.
Set the default to 1 but allow flashes to modify it in fixup hooks. If more flashes show up with this constraint in the future it might be worth it to add it to 'struct flash_info', but for now increasing its size is not worth it.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Please specify when a patch follows linux upstream. This follows the following upstream linux commit:
afd473e85827 ("mtd: spi-nor: core: Allow flashes to specify MTD writesize")
Acked-by: Tudor Ambarus tudor.ambarus@linaro.org
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
default_init() fixup hook should be used to initialize flash parameters when its information is not provided in SFDP. To support that case, it needs to take flash_parameter structure like as other hooks.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 1bfef6797f..8f371a5213 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -203,7 +203,8 @@ struct sfdp_bfpt { * table is broken or not available. */ struct spi_nor_fixups { - void (*default_init)(struct spi_nor *nor); + void (*default_init)(struct spi_nor *nor, + struct spi_nor_flash_parameter *params); int (*post_bfpt)(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, const struct sfdp_bfpt *bfpt, @@ -2775,10 +2776,11 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor, nor->fixups->post_sfdp(nor, params); }
-static void spi_nor_default_init_fixups(struct spi_nor *nor) +static void spi_nor_default_init_fixups(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { if (nor->fixups && nor->fixups->default_init) - nor->fixups->default_init(nor); + nor->fixups->default_init(nor, params); }
static int spi_nor_init_params(struct spi_nor *nor, @@ -2885,7 +2887,7 @@ static int spi_nor_init_params(struct spi_nor *nor, } }
- spi_nor_default_init_fixups(nor); + spi_nor_default_init_fixups(nor, params);
/* Override the parameters with data read from SFDP tables. */ nor->addr_width = 0; @@ -3328,7 +3330,8 @@ static int s25fs_s_setup(struct spi_nor *nor, const struct flash_info *info, return spi_nor_default_setup(nor, info, params); }
-static void s25fs_s_default_init(struct spi_nor *nor) +static void s25fs_s_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25fs_s_setup; } @@ -3452,7 +3455,8 @@ static int s25_s28_setup(struct spi_nor *nor, const struct flash_info *info, return spi_nor_default_setup(nor, info, params); }
-static void s25_default_init(struct spi_nor *nor) +static void s25_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25_s28_setup; } @@ -3544,7 +3548,8 @@ static int s25fl256l_setup(struct spi_nor *nor, const struct flash_info *info, return -ENOTSUPP; /* Bank Address Register is not supported */ }
-static void s25fl256l_default_init(struct spi_nor *nor) +static void s25fl256l_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25fl256l_setup; } @@ -3613,7 +3618,8 @@ static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor) return 0; }
-static void s28hx_t_default_init(struct spi_nor *nor) +static void s28hx_t_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable; nor->setup = s25_s28_setup; @@ -3705,7 +3711,8 @@ static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor) return 0; }
-static void mt35xu512aba_default_init(struct spi_nor *nor) +static void mt35xu512aba_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->octal_dtr_enable = spi_nor_micron_octal_dtr_enable; } @@ -3795,7 +3802,8 @@ static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor) return 0; }
-static void macronix_octal_default_init(struct spi_nor *nor) +static void macronix_octal_default_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; }
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
default_init() fixup hook should be used to initialize flash parameters when its information is not provided in SFDP. To support that case, it needs to take flash_parameter structure like as other hooks.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
I'd like to get rid of the default_init hook, let's not extend it if possible. Can you use the late_init hook instead?
Cheers, ta
Hi Tudor,
On 4/15/2024 3:47 PM, Tudor Ambarus wrote:
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
default_init() fixup hook should be used to initialize flash parameters when its information is not provided in SFDP. To support that case, it needs to take flash_parameter structure like as other hooks.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
I'd like to get rid of the default_init hook, let's not extend it if possible. Can you use the late_init hook instead?
It looks easy to migrate from default_init to late_init so I will do it. Could you provide the links to related discussion in Linux MTD side so that I can summarize it in commit message?
Thanks, Takahiro
On 4/15/24 08:09, Takahiro Kuwano wrote:
Hi Tudor,
Hi!
On 4/15/2024 3:47 PM, Tudor Ambarus wrote:
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
default_init() fixup hook should be used to initialize flash parameters when its information is not provided in SFDP. To support that case, it needs to take flash_parameter structure like as other hooks.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
I'd like to get rid of the default_init hook, let's not extend it if possible. Can you use the late_init hook instead?
It looks easy to migrate from default_init to late_init so I will do it. Could you provide the links to related discussion in Linux MTD side so that I can summarize it in commit message?
I can't, I don't remember if I brought this up or when, but I can explain why.
default_init() is wrong, it contributes to the maze of initializing flash parameters. We'd like to get rid of it because the flash parameters that it initializes are not really used at SFDP parsing time, thus they can be initialized later.
Ideally we want SFDP to initialize all the flash parameters. If (when) SFDP tables are wrong, we fix them with the post_sfdp/bfpt hooks, to emphasize that SFDP is indeed wrong. When there are parameters that are not covered by SFDP, we initialize them in late_init() - these parameters have nothing to do with SFDP and they are not needed earlier. With this we'll have a clearer view of who initializes what.
Feel free to use this in the commit message if you think it helps. Cheers, ta
On 4/15/2024 4:27 PM, Tudor Ambarus wrote:
On 4/15/24 08:09, Takahiro Kuwano wrote:
Hi Tudor,
Hi!
On 4/15/2024 3:47 PM, Tudor Ambarus wrote:
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
default_init() fixup hook should be used to initialize flash parameters when its information is not provided in SFDP. To support that case, it needs to take flash_parameter structure like as other hooks.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com
I'd like to get rid of the default_init hook, let's not extend it if possible. Can you use the late_init hook instead?
It looks easy to migrate from default_init to late_init so I will do it. Could you provide the links to related discussion in Linux MTD side so that I can summarize it in commit message?
I can't, I don't remember if I brought this up or when, but I can explain why.
default_init() is wrong, it contributes to the maze of initializing flash parameters. We'd like to get rid of it because the flash parameters that it initializes are not really used at SFDP parsing time, thus they can be initialized later.
Ideally we want SFDP to initialize all the flash parameters. If (when) SFDP tables are wrong, we fix them with the post_sfdp/bfpt hooks, to emphasize that SFDP is indeed wrong. When there are parameters that are not covered by SFDP, we initialize them in late_init() - these parameters have nothing to do with SFDP and they are not needed earlier. With this we'll have a clearer view of who initializes what.
Feel free to use this in the commit message if you think it helps. Cheers, ta
Of course it helps!
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The Infineon SEMPER NOR flash family uses 2-bit ECC by default with each ECC block being 16 bytes. Under this scheme multi-pass programming to an ECC block is not allowed. Set the writesize to make sure multi-pass programming is not attempted on the flash.
Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 8f371a5213..773afd4040 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -3459,6 +3459,13 @@ static void s25_default_init(struct spi_nor *nor, struct spi_nor_flash_parameter *params) { nor->setup = s25_s28_setup; + + /* + * Programming is supported only in 16-byte ECC data unit granularity. + * Byte-programming, bit-walking, or multiple program operations to the + * same ECC data unit without an erase are not allowed. + */ + params->writesize = 16; }
static int s25_s28_post_bfpt_fixup(struct spi_nor *nor, @@ -3623,6 +3630,13 @@ static void s28hx_t_default_init(struct spi_nor *nor, { nor->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable; nor->setup = s25_s28_setup; + + /* + * Programming is supported only in 16-byte ECC data unit granularity. + * Byte-programming, bit-walking, or multiple program operations to the + * same ECC data unit without an erase are not allowed. + */ + params->writesize = 16; }
static void s28hx_t_post_sfdp_fixup(struct spi_nor *nor,
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 8f371a5213..773afd4040 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -3459,6 +3459,13 @@ static void s25_default_init(struct spi_nor *nor, struct spi_nor_flash_parameter *params) { nor->setup = s25_s28_setup;
- /*
* Programming is supported only in 16-byte ECC data unit granularity.* Byte-programming, bit-walking, or multiple program operations to the* same ECC data unit without an erase are not allowed.*/- params->writesize = 16;
}
Use late_init() please. Looks good.
ta
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This series is equivalent to the one for Linux MTD submitted by Pratyush Yadav.
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=217759&state...
Ah, I see you specified it here. I'd argue it's better to mention it in the commit message itself, it spares people searching on the ml archive.
Takahiro Kuwano (4): mtd: ubi: Do not zero out EC and VID on ECC-ed NOR flashes mtd: spi-nor: Allow flashes to specify MTD writesize mtd: spi-nor-core: Rework default_init() to take flash_parameter mtd: spi-nor: Set ECC unit size to MTD writesize in Infineon SEMPER flashes
drivers/mtd/spi/spi-nor-core.c | 45 +++++++++++++++++++++++++--------- drivers/mtd/ubi/build.c | 4 +-- drivers/mtd/ubi/io.c | 9 ++++++- include/linux/mtd/spi-nor.h | 1 + 4 files changed, 44 insertions(+), 15 deletions(-)
On 4/15/2024 3:56 PM, Tudor Ambarus wrote:
On 4/15/24 05:33, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This series is equivalent to the one for Linux MTD submitted by Pratyush Yadav.
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=217759&state...
Ah, I see you specified it here. I'd argue it's better to mention it in the commit message itself, it spares people searching on the ml archive.
Noted, thanks!
participants (3)
-
Takahiro Kuwano -
tkuw584924＠gmail.com
-
Tudor Ambarus