[2nd RESEND PATCH v2 0/6] 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
Resent after modifying commit message in #5 (mention Linux commit first) and collecting A-b tags.
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...
Changes in v2: - Fix an issue in setting macronix_octal_fixups - Rework fixup hooks
Takahiro Kuwano (6): 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: Check nor->info before setting macronix_octal_fixups mtd: spi-nor: Replace default_init() hook with late_init() mtd: spi-nor: Call spi_nor_post_sfdp_fixups() only after spi_nor_parse_sfdp() mtd: spi-nor: Set ECC unit size to MTD writesize in Infineon SEMPER flashes
drivers/mtd/spi/spi-nor-core.c | 88 +++++++++++++++++++++------------- drivers/mtd/ubi/build.c | 4 +- drivers/mtd/ubi/io.c | 9 +++- include/linux/mtd/spi-nor.h | 1 + 4 files changed, 65 insertions(+), 37 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.
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 Acked-by: Pratyush Yadav pratyush@kernel.org 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 cf0d702421f..104537784ab 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 14be95b74bc..45699b4a477 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;
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.
This patch replicates the following upstream linux commit: afd473e85827 ("mtd: spi-nor: core: Allow flashes to specify MTD writesize")
Acked-by: Tudor Ambarus tudor.ambarus@linaro.org 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 e126fa37c16..4dfc110b02a 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -2839,6 +2839,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;
@@ -4142,7 +4143,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 d1dbf3eadbf..0d37a806c49 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;
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
The macronix_octal_fixups should be set only when mfr and flags match.
Fixes: df3d5f9e41 ("mtd: spi-nor: add support for Macronix Octal flash") Acked-by: Tudor Ambarus tudor.ambarus@linaro.org Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com Cc: JaimeLiao jaimeliao.tw@gmail.com --- drivers/mtd/spi/spi-nor-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 4dfc110b02a..6eaed3da392 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -4067,7 +4067,9 @@ void spi_nor_set_fixups(struct spi_nor *nor) #endif
#if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX) - nor->fixups = ¯onix_octal_fixups; + if (JEDEC_MFR(nor->info) == SNOR_MFR_MACRONIX && + nor->info->flags & SPI_NOR_OCTAL_DTR_READ) + nor->fixups = ¯onix_octal_fixups; #endif /* SPI_FLASH_MACRONIX */ }
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
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.
There are six default_init() hooks implemented just for initializing octal_dtr_enable() and/or setup() hooks that called later on. Just moving those to late_init() does not change functionality.
Suggested-by: Tudor Ambarus tudor.ambarus@linaro.org Acked-by: Tudor Ambarus tudor.ambarus@linaro.org Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 48 +++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index 6eaed3da392..e69a25195c9 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -188,27 +188,27 @@ struct sfdp_bfpt {
/** * struct spi_nor_fixups - SPI NOR fixup hooks - * @default_init: called after default flash parameters init. Used to tweak - * flash parameters when information provided by the flash_info - * table is incomplete or wrong. * @post_bfpt: called after the BFPT table has been parsed * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs * that do not support RDSFDP). Typically used to tweak various * parameters that could not be extracted by other means (i.e. * when information provided by the SFDP/flash_info tables are * incomplete or wrong). + * @late_init: used to initialize flash parameters that are not declared in the + * JESD216 SFDP standard, or where SFDP tables not defined at all. * * Those hooks can be used to tweak the SPI NOR configuration when the SFDP * table is broken or not available. */ struct spi_nor_fixups { - void (*default_init)(struct spi_nor *nor); int (*post_bfpt)(struct spi_nor *nor, const struct sfdp_parameter_header *bfpt_header, const struct sfdp_bfpt *bfpt, struct spi_nor_flash_parameter *params); void (*post_sfdp)(struct spi_nor *nor, struct spi_nor_flash_parameter *params); + void (*late_init)(struct spi_nor *nor, + struct spi_nor_flash_parameter *params); };
#define SPI_NOR_SRST_SLEEP_LEN 200 @@ -2825,10 +2825,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_late_init_fixups(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { - if (nor->fixups && nor->fixups->default_init) - nor->fixups->default_init(nor); + if (nor->fixups && nor->fixups->late_init) + nor->fixups->late_init(nor, params); }
static int spi_nor_init_params(struct spi_nor *nor, @@ -2935,8 +2936,6 @@ static int spi_nor_init_params(struct spi_nor *nor, } }
- spi_nor_default_init_fixups(nor); - /* Override the parameters with data read from SFDP tables. */ nor->addr_width = 0; nor->mtd.erasesize = 0; @@ -2955,6 +2954,7 @@ static int spi_nor_init_params(struct spi_nor *nor, }
spi_nor_post_sfdp_fixups(nor, params); + spi_nor_late_init_fixups(nor, params);
return 0; } @@ -3380,7 +3380,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_late_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25fs_s_setup; } @@ -3430,9 +3431,9 @@ static void s25fs_s_post_sfdp_fixup(struct spi_nor *nor, }
static struct spi_nor_fixups s25fs_s_fixups = { - .default_init = s25fs_s_default_init, .post_bfpt = s25fs_s_post_bfpt_fixup, .post_sfdp = s25fs_s_post_sfdp_fixup, + .late_init = s25fs_s_late_init, };
static int s25_s28_mdp_ready(struct spi_nor *nor) @@ -3516,7 +3517,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_late_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25_s28_setup; } @@ -3597,9 +3599,9 @@ static void s25_post_sfdp_fixup(struct spi_nor *nor, }
static struct spi_nor_fixups s25_fixups = { - .default_init = s25_default_init, .post_bfpt = s25_s28_post_bfpt_fixup, .post_sfdp = s25_post_sfdp_fixup, + .late_init = s25_late_init, };
static int s25fl256l_setup(struct spi_nor *nor, const struct flash_info *info, @@ -3608,13 +3610,14 @@ 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_late_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->setup = s25fl256l_setup; }
static struct spi_nor_fixups s25fl256l_fixups = { - .default_init = s25fl256l_default_init, + .late_init = s25fl256l_late_init, }; #endif
@@ -3677,7 +3680,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_late_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; @@ -3715,9 +3719,9 @@ static void s28hx_t_post_sfdp_fixup(struct spi_nor *nor, }
static struct spi_nor_fixups s28hx_t_fixups = { - .default_init = s28hx_t_default_init, .post_sfdp = s28hx_t_post_sfdp_fixup, .post_bfpt = s25_s28_post_bfpt_fixup, + .late_init = s28hx_t_late_init, }; #endif /* CONFIG_SPI_FLASH_S28HX_T */
@@ -3769,7 +3773,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_late_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->octal_dtr_enable = spi_nor_micron_octal_dtr_enable; } @@ -3798,8 +3803,8 @@ static void mt35xu512aba_post_sfdp_fixup(struct spi_nor *nor, }
static struct spi_nor_fixups mt35xu512aba_fixups = { - .default_init = mt35xu512aba_default_init, .post_sfdp = mt35xu512aba_post_sfdp_fixup, + .late_init = mt35xu512aba_late_init, }; #endif /* CONFIG_SPI_FLASH_MT35XU */
@@ -3859,7 +3864,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_late_init(struct spi_nor *nor, + struct spi_nor_flash_parameter *params) { nor->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable; } @@ -3876,8 +3882,8 @@ static void macronix_octal_post_sfdp_fixup(struct spi_nor *nor, }
static struct spi_nor_fixups macronix_octal_fixups = { - .default_init = macronix_octal_default_init, .post_sfdp = macronix_octal_post_sfdp_fixup, + .late_init = macronix_octal_late_init, }; #endif /* CONFIG_SPI_FLASH_MACRONIX */
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
This patch follows the upstream linux commit: 5273cc6df984("mtd: spi-nor: core: Call spi_nor_post_sfdp_fixups() only when SFDP is defined")
spi_nor_post_sfdp_fixups() was called regardless of if spi_nor_parse_sfdp() had been called or not. late_init() should be instead used to initialize the parameters that are not defined in SFDP.
Ideally spi_nor_post_sfdp_fixups() is called only after successful parse of SFDP. However, in case SFDP support is disabled by .config, that can break current functionality. Therefore, we would call it after spi_nor_parse_sfdp() regardless of its return value.
Acked-by: Tudor Ambarus tudor.ambarus@linaro.org Signed-off-by: Takahiro Kuwano Takahiro.Kuwano@infineon.com --- drivers/mtd/spi/spi-nor-core.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index e69a25195c9..a8f3edc9f69 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -189,11 +189,10 @@ struct sfdp_bfpt { /** * struct spi_nor_fixups - SPI NOR fixup hooks * @post_bfpt: called after the BFPT table has been parsed - * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs - * that do not support RDSFDP). Typically used to tweak various - * parameters that could not be extracted by other means (i.e. - * when information provided by the SFDP/flash_info tables are - * incomplete or wrong). + * @post_sfdp: called after SFDP has been parsed. Typically used to tweak + * various parameters that could not be extracted by other means + * (i.e. when information provided by the SFDP tables are incomplete + * or wrong). * @late_init: used to initialize flash parameters that are not declared in the * JESD216 SFDP standard, or where SFDP tables not defined at all. * @@ -2810,13 +2809,12 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
/** * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings - * after SFDP has been parsed (is also called for SPI NORs that do not - * support RDSFDP). + * after SFDP has been parsed. * @nor: pointer to a 'struct spi_nor' * * Typically used to tweak various parameters that could not be extracted by - * other means (i.e. when information provided by the SFDP/flash_info tables - * are incomplete or wrong). + * other means (i.e. when information provided by the SFDP tables are incomplete + * or wrong). */ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor, struct spi_nor_flash_parameter *params) @@ -2951,9 +2949,10 @@ static int spi_nor_init_params(struct spi_nor *nor, } else { memcpy(params, &sfdp_params, sizeof(*params)); } + + spi_nor_post_sfdp_fixups(nor, params); }
- spi_nor_post_sfdp_fixups(nor, params); spi_nor_late_init_fixups(nor, params);
return 0;
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.
Acked-by: Tudor Ambarus tudor.ambarus@linaro.org 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 a8f3edc9f69..23e71243927 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -3520,6 +3520,13 @@ static void s25_late_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, @@ -3684,6 +3691,13 @@ static void s28hx_t_late_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 Fri, Sep 27, 2024 at 10:18:05AM +0900, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Resent after modifying commit message in #5 (mention Linux commit first) and collecting A-b tags.
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...
Changes in v2:
- Fix an issue in setting macronix_octal_fixups
- Rework fixup hooks
Sorry, can you please rebase this on top of current master? With some other related backlogged patches now applied this doesn't apply cleanly. Thanks.
Hello Tom,
On 10/11/2024 2:32 AM, Tom Rini wrote:
On Fri, Sep 27, 2024 at 10:18:05AM +0900, tkuw584924@gmail.com wrote:
From: Takahiro Kuwano Takahiro.Kuwano@infineon.com
Resent after modifying commit message in #5 (mention Linux commit first) and collecting A-b tags.
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...
Changes in v2:
- Fix an issue in setting macronix_octal_fixups
- Rework fixup hooks
Sorry, can you please rebase this on top of current master? With some other related backlogged patches now applied this doesn't apply cleanly. Thanks.
I have sent v3 based on current master.
Thanks! Takahiro
participants (3)
-
Takahiro Kuwano -
tkuw584924＠gmail.com
-
Tom Rini