[PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it

[PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it

[tkuw584924]

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(-)

-- 
2.34.1

[PATCH 1/4] mtd: ubi: Do not zero out EC and VID on ECC-ed NOR flashes

[tkuw584924]

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;
-- 
2.34.1

[PATCH 2/4] mtd: spi-nor: Allow flashes to specify MTD writesize

[tkuw584924]

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;
-- 
2.34.1

[PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter

[tkuw584924]

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;
 }
-- 
2.34.1

[PATCH 4/4] mtd: spi-nor: Set ECC unit size to MTD writesize in Infineon SEMPER flashes

[tkuw584924]

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,
-- 
2.34.1

Re: [PATCH 1/4] mtd: ubi: Do not zero out EC and VID on ECC-ed NOR flashes

[Tudor Ambarus]

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

Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter

[Tudor Ambarus]

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

Re: [PATCH 2/4] mtd: spi-nor: Allow flashes to specify MTD writesize

[Tudor Ambarus]

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>

Re: [PATCH 4/4] mtd: spi-nor: Set ECC unit size to MTD writesize in Infineon SEMPER flashes

[Tudor Ambarus]

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

Re: [PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it

[Tudor Ambarus]

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(-)
> 

Re: [PATCH 0/4] mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it

[Takahiro Kuwano]

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!

Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter

[Takahiro Kuwano]

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

Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter

[Tudor Ambarus]

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

Re: [PATCH 3/4] mtd: spi-nor-core: Rework default_init() to take flash_parameter

[Takahiro Kuwano]

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!