[PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Sean Nyekjaer]

Linux kernel defaults to use legacy bch setting, this was creating a
mismatch between U-boot and Linux default settings.

Fixes: 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch geometry")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/mtd/nand/raw/mxs_nand.c    | 2 +-
 drivers/mtd/nand/raw/mxs_nand_dt.c | 2 +-
 include/mxs_nand.h                 | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
index e6bbfac4d6..49795e20d2 100644
--- a/drivers/mtd/nand/raw/mxs_nand.c
+++ b/drivers/mtd/nand/raw/mxs_nand.c
@@ -1119,7 +1119,7 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
 	}
 
 	if ((!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) &&
-	     mtd->oobsize < 1024) || nand_info->legacy_bch_geometry) {
+	     mtd->oobsize < 1024) || !nand_info->modern_bch_geometry) {
 		dev_warn(mtd->dev, "use legacy bch geometry\n");
 		return mxs_nand_legacy_calc_ecc_layout(geo, mtd);
 	}
diff --git a/drivers/mtd/nand/raw/mxs_nand_dt.c b/drivers/mtd/nand/raw/mxs_nand_dt.c
index 878796d555..bbb7396324 100644
--- a/drivers/mtd/nand/raw/mxs_nand_dt.c
+++ b/drivers/mtd/nand/raw/mxs_nand_dt.c
@@ -92,7 +92,7 @@ static int mxs_nand_dt_probe(struct udevice *dev)
 
 	info->use_minimum_ecc = dev_read_bool(dev, "fsl,use-minimum-ecc");
 
-	info->legacy_bch_geometry = dev_read_bool(dev, "fsl,legacy-bch-geometry");
+	info->modern_bch_geometry = dev_read_bool(dev, "fsl,modern-bch-geometry");
 
 	if (IS_ENABLED(CONFIG_CLK) && IS_ENABLED(CONFIG_IMX8)) {
 		/* Assigned clock already set clock */
diff --git a/include/mxs_nand.h b/include/mxs_nand.h
index c0cefaca90..7e254d6b31 100644
--- a/include/mxs_nand.h
+++ b/include/mxs_nand.h
@@ -43,8 +43,8 @@ struct mxs_nand_info {
 	struct udevice *dev;
 	unsigned int	max_ecc_strength_supported;
 	bool		use_minimum_ecc;
-	/* legacy bch geometry flag */
-	bool		legacy_bch_geometry;
+	/* modern bch geometry flag */
+	bool		modern_bch_geometry;
 	int		cur_chip;
 
 	uint32_t	cmd_queue_len;
-- 
2.31.0

[EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[han.xu]

On 21/05/10 12:00PM, Sean Nyekjaer wrote:
> Caution: EXT Email
> 
> Linux kernel defaults to use legacy bch setting, this was creating a
> mismatch between U-boot and Linux default settings.

Kernel uses the NAND chip specified minimum ecc strength and steps by default
not the legacy bch setting, unless users enable it in DT file.

> 
> Fixes: 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch geometry")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/mtd/nand/raw/mxs_nand.c    | 2 +-
>  drivers/mtd/nand/raw/mxs_nand_dt.c | 2 +-
>  include/mxs_nand.h                 | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/mxs_nand.c b/drivers/mtd/nand/raw/mxs_nand.c
> index e6bbfac4d6..49795e20d2 100644
> --- a/drivers/mtd/nand/raw/mxs_nand.c
> +++ b/drivers/mtd/nand/raw/mxs_nand.c
> @@ -1119,7 +1119,7 @@ static int mxs_nand_set_geometry(struct mtd_info *mtd, struct bch_geometry *geo)
>         }
> 
>         if ((!(chip->ecc_strength_ds > 0 && chip->ecc_step_ds > 0) &&
> -            mtd->oobsize < 1024) || nand_info->legacy_bch_geometry) {
> +            mtd->oobsize < 1024) || !nand_info->modern_bch_geometry) {
>                 dev_warn(mtd->dev, "use legacy bch geometry\n");
>                 return mxs_nand_legacy_calc_ecc_layout(geo, mtd);
>         }
> diff --git a/drivers/mtd/nand/raw/mxs_nand_dt.c b/drivers/mtd/nand/raw/mxs_nand_dt.c
> index 878796d555..bbb7396324 100644
> --- a/drivers/mtd/nand/raw/mxs_nand_dt.c
> +++ b/drivers/mtd/nand/raw/mxs_nand_dt.c
> @@ -92,7 +92,7 @@ static int mxs_nand_dt_probe(struct udevice *dev)
> 
>         info->use_minimum_ecc = dev_read_bool(dev, "fsl,use-minimum-ecc");
> 
> -       info->legacy_bch_geometry = dev_read_bool(dev, "fsl,legacy-bch-geometry");
> +       info->modern_bch_geometry = dev_read_bool(dev, "fsl,modern-bch-geometry");
> 
>         if (IS_ENABLED(CONFIG_CLK) && IS_ENABLED(CONFIG_IMX8)) {
>                 /* Assigned clock already set clock */
> diff --git a/include/mxs_nand.h b/include/mxs_nand.h
> index c0cefaca90..7e254d6b31 100644
> --- a/include/mxs_nand.h
> +++ b/include/mxs_nand.h
> @@ -43,8 +43,8 @@ struct mxs_nand_info {
>         struct udevice *dev;
>         unsigned int    max_ecc_strength_supported;
>         bool            use_minimum_ecc;
> -       /* legacy bch geometry flag */
> -       bool            legacy_bch_geometry;
> +       /* modern bch geometry flag */
> +       bool            modern_bch_geometry;
>         int             cur_chip;
> 
>         uint32_t        cmd_queue_len;
> --
> 2.31.0
> 

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Sean Nyekjaer]

On 11/05/2021 04.49, han.xu wrote:
> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>> Caution: EXT Email
>>
>> Linux kernel defaults to use legacy bch setting, this was creating a
>> mismatch between U-boot and Linux default settings.
> Kernel uses the NAND chip specified minimum ecc strength and steps by default
> not the legacy bch setting, unless users enable it in DT file.
> 

Hi,

Adding, mtd-list and Miquel

With u-boot dtb:
&gpmi {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_gpmi_nand1>;
	compatible = "fsl,imx7d-gpmi-nand";
	nand-on-flash-bbt;
	status = "okay";
};

With linux dtb (mainline 5.10):
&gpmi {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_gpmi_nand1>;
	nand-on-flash-bbt;
	status = "okay";
};

U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
With legacy option it selects 18.
Linux is selecting 18 bits ;) So now we have a mismatch.

I have been searching for the legacy option in the mainline kernel can't find it ;)
Please show me where it is (is it in the NXP fork?)

/Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Sean Nyekjaer]

On 11/05/2021 04.49, han.xu wrote:
> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>> Caution: EXT Email
>>
>> Linux kernel defaults to use legacy bch setting, this was creating a
>> mismatch between U-boot and Linux default settings.
> Kernel uses the NAND chip specified minimum ecc strength and steps by default
> not the legacy bch setting, unless users enable it in DT file.
> 

Hi,

Adding, mtd-list and Miquel

With u-boot dtb:
&gpmi {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_gpmi_nand1>;
	compatible = "fsl,imx7d-gpmi-nand";
	nand-on-flash-bbt;
	status = "okay";
};

With linux dtb (mainline 5.10):
&gpmi {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_gpmi_nand1>;
	nand-on-flash-bbt;
	status = "okay";
};

U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
With legacy option it selects 18.
Linux is selecting 18 bits ;) So now we have a mismatch.

I have been searching for the legacy option in the mainline kernel can't find it ;)
Please show me where it is (is it in the NXP fork?)

/Sean

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[han.xu]

On 21/05/11 07:08AM, Sean Nyekjaer wrote:
> Caution: EXT Email
> 
> On 11/05/2021 04.49, han.xu wrote:
> > On 21/05/10 12:00PM, Sean Nyekjaer wrote:
> >> Caution: EXT Email
> >>
> >> Linux kernel defaults to use legacy bch setting, this was creating a
> >> mismatch between U-boot and Linux default settings.
> > Kernel uses the NAND chip specified minimum ecc strength and steps by default
> > not the legacy bch setting, unless users enable it in DT file.
> >
> 
> Hi,
> 
> Adding, mtd-list and Miquel
> 
> With u-boot dtb:
> &gpmi {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>         compatible = "fsl,imx7d-gpmi-nand";
>         nand-on-flash-bbt;
>         status = "okay";
> };
> 
> With linux dtb (mainline 5.10):
> &gpmi {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>         nand-on-flash-bbt;
>         status = "okay";
> };
> 
> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
> With legacy option it selects 18.
> Linux is selecting 18 bits ;) So now we have a mismatch.
> 
> I have been searching for the legacy option in the mainline kernel can't find it ;)
> Please show me where it is (is it in the NXP fork?)

You are right, it's only fixed in NXP fork, with kernel driver modification. We
prefer the current u-boot bch geometry, so I will send out a kernel patch to
make them align.

> 
> /Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[han.xu]

On 21/05/11 07:08AM, Sean Nyekjaer wrote:
> Caution: EXT Email
> 
> On 11/05/2021 04.49, han.xu wrote:
> > On 21/05/10 12:00PM, Sean Nyekjaer wrote:
> >> Caution: EXT Email
> >>
> >> Linux kernel defaults to use legacy bch setting, this was creating a
> >> mismatch between U-boot and Linux default settings.
> > Kernel uses the NAND chip specified minimum ecc strength and steps by default
> > not the legacy bch setting, unless users enable it in DT file.
> >
> 
> Hi,
> 
> Adding, mtd-list and Miquel
> 
> With u-boot dtb:
> &gpmi {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>         compatible = "fsl,imx7d-gpmi-nand";
>         nand-on-flash-bbt;
>         status = "okay";
> };
> 
> With linux dtb (mainline 5.10):
> &gpmi {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>         nand-on-flash-bbt;
>         status = "okay";
> };
> 
> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
> With legacy option it selects 18.
> Linux is selecting 18 bits ;) So now we have a mismatch.
> 
> I have been searching for the legacy option in the mainline kernel can't find it ;)
> Please show me where it is (is it in the NXP fork?)

You are right, it's only fixed in NXP fork, with kernel driver modification. We
prefer the current u-boot bch geometry, so I will send out a kernel patch to
make them align.

> 
> /Sean

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Sean Nyekjaer]

On 13/05/2021 22.02, han.xu wrote:
> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>> Caution: EXT Email
>>
>> On 11/05/2021 04.49, han.xu wrote:
>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>> Caution: EXT Email
>>>>
>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>> mismatch between U-boot and Linux default settings.
>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>> not the legacy bch setting, unless users enable it in DT file.
>>>
>>
>> Hi,
>>
>> Adding, mtd-list and Miquel
>>
>> With u-boot dtb:
>> &gpmi {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>         compatible = "fsl,imx7d-gpmi-nand";
>>         nand-on-flash-bbt;
>>         status = "okay";
>> };
>>
>> With linux dtb (mainline 5.10):
>> &gpmi {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>         nand-on-flash-bbt;
>>         status = "okay";
>> };
>>
>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>> With legacy option it selects 18.
>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>
>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>> Please show me where it is (is it in the NXP fork?)
> 
> You are right, it's only fixed in NXP fork, with kernel driver modification. We
> prefer the current u-boot bch geometry, so I will send out a kernel patch to
> make them align.
> 
Any progress on this?

I see your patch in:
https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/mtd/nand/raw/gpmi-nand?h=imx_5.4.70_2.3.0&id=ae980dccc6189956fab047958ad0a70ec4951439

But I can't find the option for selecting legacy mode in the devicetree...

/Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Sean Nyekjaer]

On 13/05/2021 22.02, han.xu wrote:
> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>> Caution: EXT Email
>>
>> On 11/05/2021 04.49, han.xu wrote:
>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>> Caution: EXT Email
>>>>
>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>> mismatch between U-boot and Linux default settings.
>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>> not the legacy bch setting, unless users enable it in DT file.
>>>
>>
>> Hi,
>>
>> Adding, mtd-list and Miquel
>>
>> With u-boot dtb:
>> &gpmi {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>         compatible = "fsl,imx7d-gpmi-nand";
>>         nand-on-flash-bbt;
>>         status = "okay";
>> };
>>
>> With linux dtb (mainline 5.10):
>> &gpmi {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>         nand-on-flash-bbt;
>>         status = "okay";
>> };
>>
>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>> With legacy option it selects 18.
>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>
>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>> Please show me where it is (is it in the NXP fork?)
> 
> You are right, it's only fixed in NXP fork, with kernel driver modification. We
> prefer the current u-boot bch geometry, so I will send out a kernel patch to
> make them align.
> 
Any progress on this?

I see your patch in:
https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/mtd/nand/raw/gpmi-nand?h=imx_5.4.70_2.3.0&id=ae980dccc6189956fab047958ad0a70ec4951439

But I can't find the option for selecting legacy mode in the devicetree...

/Sean

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Frieder Schrempf]

Hi Stefano,

this old patch was delegated to you in patchwork. If you're not the
correct maintainer to address, please let me know. As the NAND layer
seems to be unmaintained at the moment, I'm not sure whom to ask.

This patch fixes a regression that was introduced by 616f03dabacb ("
mtd: gpmi: change the BCH layout setting for large oob NAND") which
alters the BCH layout in a way that doesn't match with the
implementation in the Linux kernel.

This causes failures when loading an UBI image in U-Boot that was
flashed by Linux or vice versa (see [1]).

There has been an approach to fix this through an optional devicetree
property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
geometry"), but this is not acceptable. The "legacy" BCH layout
compatible with Linux should be used by default.

The approach to upstream the "new" layout to the kernel [2] seems to be
stalled and even if it would succeed, it would break systems that use an
old U-Boot and a new kernel, which is again not really acceptable in my
opinion.

For the reasons above I would like to ask U-Boot maintainers to pick up
this patch.

Thanks
Frieder

[1] https://lists.denx.de/pipermail/u-boot/2022-March/477828.html
[2]
https://patchwork.ozlabs.org/project/linux-mtd/patch/20210522205136.19465-2-han.xu@nxp.com/

Am 20.05.21 um 11:09 schrieb Sean Nyekjaer:
> On 13/05/2021 22.02, han.xu wrote:
>> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>>> Caution: EXT Email
>>>
>>> On 11/05/2021 04.49, han.xu wrote:
>>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>>> Caution: EXT Email
>>>>>
>>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>>> mismatch between U-boot and Linux default settings.
>>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>>> not the legacy bch setting, unless users enable it in DT file.
>>>>
>>>
>>> Hi,
>>>
>>> Adding, mtd-list and Miquel
>>>
>>> With u-boot dtb:
>>> &gpmi {
>>>         pinctrl-names = "default";
>>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>         compatible = "fsl,imx7d-gpmi-nand";
>>>         nand-on-flash-bbt;
>>>         status = "okay";
>>> };
>>>
>>> With linux dtb (mainline 5.10):
>>> &gpmi {
>>>         pinctrl-names = "default";
>>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>         nand-on-flash-bbt;
>>>         status = "okay";
>>> };
>>>
>>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>>> With legacy option it selects 18.
>>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>>
>>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>>> Please show me where it is (is it in the NXP fork?)
>>
>> You are right, it's only fixed in NXP fork, with kernel driver modification. We
>> prefer the current u-boot bch geometry, so I will send out a kernel patch to
>> make them align.
>>
> Any progress on this?
> 
> I see your patch in:
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fmtd%2Fnand%2Fraw%2Fgpmi-nand%3Fh%3Dimx_5.4.70_2.3.0%26id%3Dae980dccc6189956fab047958ad0a70ec4951439&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C275b7f11361e4fd3eb7e08d91b6f0e45%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637570986066857865%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=L4xAN9HDl68dmA%2FX6nnP%2Buqh6CUKTQZJeFWja5EvmPI%3D&amp;reserved=0
> 
> But I can't find the option for selecting legacy mode in the devicetree...
> 
> /Sean

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Frieder Schrempf]

Hi Stefano,

this old patch was delegated to you in patchwork. If you're not the
correct maintainer to address, please let me know. As the NAND layer
seems to be unmaintained at the moment, I'm not sure whom to ask.

This patch fixes a regression that was introduced by 616f03dabacb ("
mtd: gpmi: change the BCH layout setting for large oob NAND") which
alters the BCH layout in a way that doesn't match with the
implementation in the Linux kernel.

This causes failures when loading an UBI image in U-Boot that was
flashed by Linux or vice versa (see [1]).

There has been an approach to fix this through an optional devicetree
property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
geometry"), but this is not acceptable. The "legacy" BCH layout
compatible with Linux should be used by default.

The approach to upstream the "new" layout to the kernel [2] seems to be
stalled and even if it would succeed, it would break systems that use an
old U-Boot and a new kernel, which is again not really acceptable in my
opinion.

For the reasons above I would like to ask U-Boot maintainers to pick up
this patch.

Thanks
Frieder

[1] https://lists.denx.de/pipermail/u-boot/2022-March/477828.html
[2]
https://patchwork.ozlabs.org/project/linux-mtd/patch/20210522205136.19465-2-han.xu@nxp.com/

Am 20.05.21 um 11:09 schrieb Sean Nyekjaer:
> On 13/05/2021 22.02, han.xu wrote:
>> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>>> Caution: EXT Email
>>>
>>> On 11/05/2021 04.49, han.xu wrote:
>>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>>> Caution: EXT Email
>>>>>
>>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>>> mismatch between U-boot and Linux default settings.
>>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>>> not the legacy bch setting, unless users enable it in DT file.
>>>>
>>>
>>> Hi,
>>>
>>> Adding, mtd-list and Miquel
>>>
>>> With u-boot dtb:
>>> &gpmi {
>>>         pinctrl-names = "default";
>>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>         compatible = "fsl,imx7d-gpmi-nand";
>>>         nand-on-flash-bbt;
>>>         status = "okay";
>>> };
>>>
>>> With linux dtb (mainline 5.10):
>>> &gpmi {
>>>         pinctrl-names = "default";
>>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>         nand-on-flash-bbt;
>>>         status = "okay";
>>> };
>>>
>>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>>> With legacy option it selects 18.
>>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>>
>>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>>> Please show me where it is (is it in the NXP fork?)
>>
>> You are right, it's only fixed in NXP fork, with kernel driver modification. We
>> prefer the current u-boot bch geometry, so I will send out a kernel patch to
>> make them align.
>>
> Any progress on this?
> 
> I see your patch in:
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fmtd%2Fnand%2Fraw%2Fgpmi-nand%3Fh%3Dimx_5.4.70_2.3.0%26id%3Dae980dccc6189956fab047958ad0a70ec4951439&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C275b7f11361e4fd3eb7e08d91b6f0e45%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637570986066857865%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=L4xAN9HDl68dmA%2FX6nnP%2Buqh6CUKTQZJeFWja5EvmPI%3D&amp;reserved=0
> 
> But I can't find the option for selecting legacy mode in the devicetree...
> 
> /Sean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Han Xu]

On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf <
frieder.schrempf@kontron.de> wrote:

> Hi Stefano,
>
> this old patch was delegated to you in patchwork. If you're not the
> correct maintainer to address, please let me know. As the NAND layer
> seems to be unmaintained at the moment, I'm not sure whom to ask.
>
> This patch fixes a regression that was introduced by 616f03dabacb ("
> mtd: gpmi: change the BCH layout setting for large oob NAND") which
> alters the BCH layout in a way that doesn't match with the
> implementation in the Linux kernel.
>
> This causes failures when loading an UBI image in U-Boot that was
> flashed by Linux or vice versa (see [1]).
>
> There has been an approach to fix this through an optional devicetree
> property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> geometry"), but this is not acceptable. The "legacy" BCH layout
> compatible with Linux should be used by default.
>
> The approach to upstream the "new" layout to the kernel [2] seems to be
> stalled and even if it would succeed, it would break systems that use an
> old U-Boot and a new kernel, which is again not really acceptable in my
> opinion.


Hi Frieder,

I am not in office this week. I will send another patch set to change in
both kernel and u-boot to fix the compatible issue.


>
> For the reasons above I would like to ask U-Boot maintainers to pick up
> this patch.
>
> Thanks
> Frieder
>
> [1] https://lists.denx.de/pipermail/u-boot/2022-March/477828.html
> [2]
>
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20210522205136.19465-2-han.xu@nxp.com/
>
> Am 20.05.21 um 11:09 schrieb Sean Nyekjaer:
> > On 13/05/2021 22.02, han.xu wrote:
> >> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
> >>> Caution: EXT Email
> >>>
> >>> On 11/05/2021 04.49, han.xu wrote:
> >>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
> >>>>> Caution: EXT Email
> >>>>>
> >>>>> Linux kernel defaults to use legacy bch setting, this was creating a
> >>>>> mismatch between U-boot and Linux default settings.
> >>>> Kernel uses the NAND chip specified minimum ecc strength and steps by
> default
> >>>> not the legacy bch setting, unless users enable it in DT file.
> >>>>
> >>>
> >>> Hi,
> >>>
> >>> Adding, mtd-list and Miquel
> >>>
> >>> With u-boot dtb:
> >>> &gpmi {
> >>>         pinctrl-names = "default";
> >>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
> >>>         compatible = "fsl,imx7d-gpmi-nand";
> >>>         nand-on-flash-bbt;
> >>>         status = "okay";
> >>> };
> >>>
> >>> With linux dtb (mainline 5.10):
> >>> &gpmi {
> >>>         pinctrl-names = "default";
> >>>         pinctrl-0 = <&pinctrl_gpmi_nand1>;
> >>>         nand-on-flash-bbt;
> >>>         status = "okay";
> >>> };
> >>>
> >>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that
> commit it selects 8 bits.
> >>> With legacy option it selects 18.
> >>> Linux is selecting 18 bits ;) So now we have a mismatch.
> >>>
> >>> I have been searching for the legacy option in the mainline kernel
> can't find it ;)
> >>> Please show me where it is (is it in the NXP fork?)
> >>
> >> You are right, it's only fixed in NXP fork, with kernel driver
> modification. We
> >> prefer the current u-boot bch geometry, so I will send out a kernel
> patch to
> >> make them align.
> >>
> > Any progress on this?
> >
> > I see your patch in:
> >
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fmtd%2Fnand%2Fraw%2Fgpmi-nand%3Fh%3Dimx_5.4.70_2.3.0%26id%3Dae980dccc6189956fab047958ad0a70ec4951439&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C275b7f11361e4fd3eb7e08d91b6f0e45%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637570986066857865%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=L4xAN9HDl68dmA%2FX6nnP%2Buqh6CUKTQZJeFWja5EvmPI%3D&amp;reserved=0
> >
> > But I can't find the option for selecting legacy mode in the
> devicetree...
> >
> > /Sean
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
-- 
Sincerely,

Han XU

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Frieder Schrempf]

Hi Han,

Am 17.03.22 um 14:33 schrieb Han Xu:
> 
> 
> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> 
>     Hi Stefano,
> 
>     this old patch was delegated to you in patchwork. If you're not the
>     correct maintainer to address, please let me know. As the NAND layer
>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> 
>     This patch fixes a regression that was introduced by 616f03dabacb ("
>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
>     alters the BCH layout in a way that doesn't match with the
>     implementation in the Linux kernel.
> 
>     This causes failures when loading an UBI image in U-Boot that was
>     flashed by Linux or vice versa (see [1]).
> 
>     There has been an approach to fix this through an optional devicetree
>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
>     geometry"), but this is not acceptable. The "legacy" BCH layout
>     compatible with Linux should be used by default.
> 
>     The approach to upstream the "new" layout to the kernel [2] seems to be
>     stalled and even if it would succeed, it would break systems that use an
>     old U-Boot and a new kernel, which is again not really acceptable in my
>     opinion.
> 
> 
> Hi Frieder,
> 
> I am not in office this week. I will send another patch set to change in
> both kernel and u-boot to fix the compatible issue.

You already claimed that months ago, but nothing happened:

> I will send patches for both kernel and u-boot to use legacy bch
> scheme by default, and add some code to treat few MLC nand chips as
> corner cases.

Sean's U-Boot patch is effectively reverting the default behavior to use
the "legacy" BCH scheme. So that's in line with what you want to do and
you can base your work on top of this fix. But we should get the basic
fix in regardless.

Even more so because switching the layout in U-Boot by using
fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
which causes the bootloader size to increase by around 250 KiB in my
case which might not be an option for some boards.

Thanks
Frieder

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Frieder Schrempf]

Hi Han,

Am 17.03.22 um 14:33 schrieb Han Xu:
> 
> 
> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> 
>     Hi Stefano,
> 
>     this old patch was delegated to you in patchwork. If you're not the
>     correct maintainer to address, please let me know. As the NAND layer
>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> 
>     This patch fixes a regression that was introduced by 616f03dabacb ("
>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
>     alters the BCH layout in a way that doesn't match with the
>     implementation in the Linux kernel.
> 
>     This causes failures when loading an UBI image in U-Boot that was
>     flashed by Linux or vice versa (see [1]).
> 
>     There has been an approach to fix this through an optional devicetree
>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
>     geometry"), but this is not acceptable. The "legacy" BCH layout
>     compatible with Linux should be used by default.
> 
>     The approach to upstream the "new" layout to the kernel [2] seems to be
>     stalled and even if it would succeed, it would break systems that use an
>     old U-Boot and a new kernel, which is again not really acceptable in my
>     opinion.
> 
> 
> Hi Frieder,
> 
> I am not in office this week. I will send another patch set to change in
> both kernel and u-boot to fix the compatible issue.

You already claimed that months ago, but nothing happened:

> I will send patches for both kernel and u-boot to use legacy bch
> scheme by default, and add some code to treat few MLC nand chips as
> corner cases.

Sean's U-Boot patch is effectively reverting the default behavior to use
the "legacy" BCH scheme. So that's in line with what you want to do and
you can base your work on top of this fix. But we should get the basic
fix in regardless.

Even more so because switching the layout in U-Boot by using
fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
which causes the bootloader size to increase by around 250 KiB in my
case which might not be an option for some boards.

Thanks
Frieder

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Frieder Schrempf]

Hi Ye,

Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> Hi Han,
> 
> Am 17.03.22 um 14:33 schrieb Han Xu:
>>
>>
>> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
>> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
>>
>>     Hi Stefano,
>>
>>     this old patch was delegated to you in patchwork. If you're not the
>>     correct maintainer to address, please let me know. As the NAND layer
>>     seems to be unmaintained at the moment, I'm not sure whom to ask.
>>
>>     This patch fixes a regression that was introduced by 616f03dabacb ("
>>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
>>     alters the BCH layout in a way that doesn't match with the
>>     implementation in the Linux kernel.
>>
>>     This causes failures when loading an UBI image in U-Boot that was
>>     flashed by Linux or vice versa (see [1]).
>>
>>     There has been an approach to fix this through an optional devicetree
>>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
>>     geometry"), but this is not acceptable. The "legacy" BCH layout
>>     compatible with Linux should be used by default.
>>
>>     The approach to upstream the "new" layout to the kernel [2] seems to be
>>     stalled and even if it would succeed, it would break systems that use an
>>     old U-Boot and a new kernel, which is again not really acceptable in my
>>     opinion.
>>
>>
>> Hi Frieder,
>>
>> I am not in office this week. I will send another patch set to change in
>> both kernel and u-boot to fix the compatible issue.
> 
> You already claimed that months ago, but nothing happened:
> 
>> I will send patches for both kernel and u-boot to use legacy bch
>> scheme by default, and add some code to treat few MLC nand chips as
>> corner cases.
> 
> Sean's U-Boot patch is effectively reverting the default behavior to use
> the "legacy" BCH scheme. So that's in line with what you want to do and
> you can base your work on top of this fix. But we should get the basic
> fix in regardless.
> 
> Even more so because switching the layout in U-Boot by using
> fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> which causes the bootloader size to increase by around 250 KiB in my
> case which might not be an option for some boards.

Ye's reply copied over to not break the thread:

> The dt nand driver will check "fsl,legacy-bch-geometry" property to
> use legacy bch. If this can't work for you in case you don't use DM
> driver, I prefer adding a config to select the legacy bch not
> reverting the patch.

I think you miss my point. I don't really care about your preferences.
But I do care about not introducing breaking changes. IMHO the "correct"
way to introduce the new BCH layout would have been:

1. Introduce the new layout in Linux and U-Boot behind a feature-flag
(e.g. DT property)
2. Make all upstream board configurations and DTs in Linux and U-Boot
use the feature-flag.
3. Drop the flag and make the new BCH layout the default.

That way the change would be much more controlled and raise the
awareness of board maintainers that are affected.

The current situation (layout changes applied in U-Boot without any
questioning, layout changes in Linux rejected for good reasons) leaves
everyone who uses the GPMI NAND on i.MX in a situation where they
probably will see failures and need to spend some hours of debugging
until they find out the reason is that someone carelessly introduced
breaking changes.

And even with the procedure described above we break compatibility
between old and new versions of U-Boot and Linux and create a dependency
between the two which is what we should try to avoid wherever possible.

Frieder

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Ye,
>
> Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > Hi Han,
> >
> > Am 17.03.22 um 14:33 schrieb Han Xu:
> >>
> >>
> >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> >>
> >>     Hi Stefano,
> >>
> >>     this old patch was delegated to you in patchwork. If you're not the
> >>     correct maintainer to address, please let me know. As the NAND layer
> >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> >>
> >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> >>     alters the BCH layout in a way that doesn't match with the
> >>     implementation in the Linux kernel.
> >>
> >>     This causes failures when loading an UBI image in U-Boot that was
> >>     flashed by Linux or vice versa (see [1]).
> >>
> >>     There has been an approach to fix this through an optional devicetree
> >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> >>     compatible with Linux should be used by default.
> >>
> >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> >>     stalled and even if it would succeed, it would break systems that use an
> >>     old U-Boot and a new kernel, which is again not really acceptable in my
> >>     opinion.
> >>
> >>
> >> Hi Frieder,
> >>
> >> I am not in office this week. I will send another patch set to change in
> >> both kernel and u-boot to fix the compatible issue.
> >
> > You already claimed that months ago, but nothing happened:
> >
> >> I will send patches for both kernel and u-boot to use legacy bch
> >> scheme by default, and add some code to treat few MLC nand chips as
> >> corner cases.
> >
> > Sean's U-Boot patch is effectively reverting the default behavior to use
> > the "legacy" BCH scheme. So that's in line with what you want to do and
> > you can base your work on top of this fix. But we should get the basic
> > fix in regardless.
> >
> > Even more so because switching the layout in U-Boot by using
> > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > which causes the bootloader size to increase by around 250 KiB in my
> > case which might not be an option for some boards.
>
> Ye's reply copied over to not break the thread:
>
> > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > use legacy bch. If this can't work for you in case you don't use DM
> > driver, I prefer adding a config to select the legacy bch not
> > reverting the patch.
>
> I think you miss my point. I don't really care about your preferences.
> But I do care about not introducing breaking changes. IMHO the "correct"
> way to introduce the new BCH layout would have been:
>
> 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> (e.g. DT property)
> 2. Make all upstream board configurations and DTs in Linux and U-Boot
> use the feature-flag.
> 3. Drop the flag and make the new BCH layout the default.
>
> That way the change would be much more controlled and raise the
> awareness of board maintainers that are affected.
>
> The current situation (layout changes applied in U-Boot without any
> questioning, layout changes in Linux rejected for good reasons) leaves
> everyone who uses the GPMI NAND on i.MX in a situation where they
> probably will see failures and need to spend some hours of debugging
> until they find out the reason is that someone carelessly introduced
> breaking changes.
>
> And even with the procedure described above we break compatibility
> between old and new versions of U-Boot and Linux and create a dependency
> between the two which is what we should try to avoid wherever possible.
>
> Frieder

Frieder,

I agree that we need to get this fixed ASAP. Because of this IMX GPMI
NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
be really nice to get this fixed in v2022.01 which releases in a
couple of weeks. I'm surprised neither of us noticed this problem but
likely we've both been so busy trying to keep up with forced DM
migrations we haven't fully tested the 'little things' like booting to
an actual OS. I know the boards I support using IMX GPMI NAND UBI are
still using a 2017 U-Boot where quite honestly everything worked fine
so there hasn't been a reason to push people to something new.

That said, applying this patch does 'not' fix things for my boards.
For my boards mxs_nand_set_geometry is called with:
oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0

and I still get ecc errors when mounting a UBI that worked fine with a
v2020.04 U-Boot:
Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
ubi0: attaching mtd3
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 0:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 1:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 2:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
bytes from PEB 2:4096, read 4096 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 3:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
bytes from PEB 3:4096, read 4096 bytes

The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
layout setting for large oob NAND") no longer reverts and I haven't
dug in to find out what I can do about that but i'm a little concerned
I get different results than you do regarding the patch from this
thread.

Thank you for pursuing this issue!

Best Regards,

Tim



Best Regards,

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Ye,
>
> Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > Hi Han,
> >
> > Am 17.03.22 um 14:33 schrieb Han Xu:
> >>
> >>
> >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> >>
> >>     Hi Stefano,
> >>
> >>     this old patch was delegated to you in patchwork. If you're not the
> >>     correct maintainer to address, please let me know. As the NAND layer
> >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> >>
> >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> >>     alters the BCH layout in a way that doesn't match with the
> >>     implementation in the Linux kernel.
> >>
> >>     This causes failures when loading an UBI image in U-Boot that was
> >>     flashed by Linux or vice versa (see [1]).
> >>
> >>     There has been an approach to fix this through an optional devicetree
> >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> >>     compatible with Linux should be used by default.
> >>
> >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> >>     stalled and even if it would succeed, it would break systems that use an
> >>     old U-Boot and a new kernel, which is again not really acceptable in my
> >>     opinion.
> >>
> >>
> >> Hi Frieder,
> >>
> >> I am not in office this week. I will send another patch set to change in
> >> both kernel and u-boot to fix the compatible issue.
> >
> > You already claimed that months ago, but nothing happened:
> >
> >> I will send patches for both kernel and u-boot to use legacy bch
> >> scheme by default, and add some code to treat few MLC nand chips as
> >> corner cases.
> >
> > Sean's U-Boot patch is effectively reverting the default behavior to use
> > the "legacy" BCH scheme. So that's in line with what you want to do and
> > you can base your work on top of this fix. But we should get the basic
> > fix in regardless.
> >
> > Even more so because switching the layout in U-Boot by using
> > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > which causes the bootloader size to increase by around 250 KiB in my
> > case which might not be an option for some boards.
>
> Ye's reply copied over to not break the thread:
>
> > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > use legacy bch. If this can't work for you in case you don't use DM
> > driver, I prefer adding a config to select the legacy bch not
> > reverting the patch.
>
> I think you miss my point. I don't really care about your preferences.
> But I do care about not introducing breaking changes. IMHO the "correct"
> way to introduce the new BCH layout would have been:
>
> 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> (e.g. DT property)
> 2. Make all upstream board configurations and DTs in Linux and U-Boot
> use the feature-flag.
> 3. Drop the flag and make the new BCH layout the default.
>
> That way the change would be much more controlled and raise the
> awareness of board maintainers that are affected.
>
> The current situation (layout changes applied in U-Boot without any
> questioning, layout changes in Linux rejected for good reasons) leaves
> everyone who uses the GPMI NAND on i.MX in a situation where they
> probably will see failures and need to spend some hours of debugging
> until they find out the reason is that someone carelessly introduced
> breaking changes.
>
> And even with the procedure described above we break compatibility
> between old and new versions of U-Boot and Linux and create a dependency
> between the two which is what we should try to avoid wherever possible.
>
> Frieder

Frieder,

I agree that we need to get this fixed ASAP. Because of this IMX GPMI
NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
be really nice to get this fixed in v2022.01 which releases in a
couple of weeks. I'm surprised neither of us noticed this problem but
likely we've both been so busy trying to keep up with forced DM
migrations we haven't fully tested the 'little things' like booting to
an actual OS. I know the boards I support using IMX GPMI NAND UBI are
still using a 2017 U-Boot where quite honestly everything worked fine
so there hasn't been a reason to push people to something new.

That said, applying this patch does 'not' fix things for my boards.
For my boards mxs_nand_set_geometry is called with:
oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0

and I still get ecc errors when mounting a UBI that worked fine with a
v2020.04 U-Boot:
Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
ubi0: attaching mtd3
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 0:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 1:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 2:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
bytes from PEB 2:4096, read 4096 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
from PEB 3:0, read 64 bytes
ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
bytes from PEB 3:4096, read 4096 bytes

The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
layout setting for large oob NAND") no longer reverts and I haven't
dug in to find out what I can do about that but i'm a little concerned
I get different results than you do regarding the patch from this
thread.

Thank you for pursuing this issue!

Best Regards,

Tim



Best Regards,

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Stefano Babic]

Hi Frieder,

sorry for late answer.
On 17.03.22 14:24, Frieder Schrempf wrote:
> Hi Stefano,
> 
> this old patch was delegated to you in patchwork. If you're not the
> correct maintainer to address, please let me know. As the NAND layer
> seems to be unmaintained at the moment, I'm not sure whom to ask.
> 

I am not the maintainer, but I picked up also in the past patches for 
NAND else they could not be merged. I will do this time, too.

Han has sent a patch yesterday to align u-boot and kernel, and it is 
okay if it is assigned to me, I will then merge it. I see from Sean he 
can test on thursday.

Regards,
Stefano

> This patch fixes a regression that was introduced by 616f03dabacb ("
> mtd: gpmi: change the BCH layout setting for large oob NAND") which
> alters the BCH layout in a way that doesn't match with the
> implementation in the Linux kernel.
> 
> This causes failures when loading an UBI image in U-Boot that was
> flashed by Linux or vice versa (see [1]).
> 
> There has been an approach to fix this through an optional devicetree
> property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> geometry"), but this is not acceptable. The "legacy" BCH layout
> compatible with Linux should be used by default.
> 
> The approach to upstream the "new" layout to the kernel [2] seems to be
> stalled and even if it would succeed, it would break systems that use an
> old U-Boot and a new kernel, which is again not really acceptable in my
> opinion.
> 
> For the reasons above I would like to ask U-Boot maintainers to pick up
> this patch.
> 
> Thanks
> Frieder
> 
> [1] https://lists.denx.de/pipermail/u-boot/2022-March/477828.html
> [2]
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20210522205136.19465-2-han.xu@nxp.com/
> 
> Am 20.05.21 um 11:09 schrieb Sean Nyekjaer:
>> On 13/05/2021 22.02, han.xu wrote:
>>> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>>>> Caution: EXT Email
>>>>
>>>> On 11/05/2021 04.49, han.xu wrote:
>>>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>>>> Caution: EXT Email
>>>>>>
>>>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>>>> mismatch between U-boot and Linux default settings.
>>>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>>>> not the legacy bch setting, unless users enable it in DT file.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Adding, mtd-list and Miquel
>>>>
>>>> With u-boot dtb:
>>>> &gpmi {
>>>>          pinctrl-names = "default";
>>>>          pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>>          compatible = "fsl,imx7d-gpmi-nand";
>>>>          nand-on-flash-bbt;
>>>>          status = "okay";
>>>> };
>>>>
>>>> With linux dtb (mainline 5.10):
>>>> &gpmi {
>>>>          pinctrl-names = "default";
>>>>          pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>>          nand-on-flash-bbt;
>>>>          status = "okay";
>>>> };
>>>>
>>>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>>>> With legacy option it selects 18.
>>>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>>>
>>>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>>>> Please show me where it is (is it in the NXP fork?)
>>>
>>> You are right, it's only fixed in NXP fork, with kernel driver modification. We
>>> prefer the current u-boot bch geometry, so I will send out a kernel patch to
>>> make them align.
>>>
>> Any progress on this?
>>
>> I see your patch in:
>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fmtd%2Fnand%2Fraw%2Fgpmi-nand%3Fh%3Dimx_5.4.70_2.3.0%26id%3Dae980dccc6189956fab047958ad0a70ec4951439&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C275b7f11361e4fd3eb7e08d91b6f0e45%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637570986066857865%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=L4xAN9HDl68dmA%2FX6nnP%2Buqh6CUKTQZJeFWja5EvmPI%3D&amp;reserved=0
>>
>> But I can't find the option for selecting legacy mode in the devicetree...
>>
>> /Sean


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Stefano Babic]

Hi Frieder,

sorry for late answer.
On 17.03.22 14:24, Frieder Schrempf wrote:
> Hi Stefano,
> 
> this old patch was delegated to you in patchwork. If you're not the
> correct maintainer to address, please let me know. As the NAND layer
> seems to be unmaintained at the moment, I'm not sure whom to ask.
> 

I am not the maintainer, but I picked up also in the past patches for 
NAND else they could not be merged. I will do this time, too.

Han has sent a patch yesterday to align u-boot and kernel, and it is 
okay if it is assigned to me, I will then merge it. I see from Sean he 
can test on thursday.

Regards,
Stefano

> This patch fixes a regression that was introduced by 616f03dabacb ("
> mtd: gpmi: change the BCH layout setting for large oob NAND") which
> alters the BCH layout in a way that doesn't match with the
> implementation in the Linux kernel.
> 
> This causes failures when loading an UBI image in U-Boot that was
> flashed by Linux or vice versa (see [1]).
> 
> There has been an approach to fix this through an optional devicetree
> property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> geometry"), but this is not acceptable. The "legacy" BCH layout
> compatible with Linux should be used by default.
> 
> The approach to upstream the "new" layout to the kernel [2] seems to be
> stalled and even if it would succeed, it would break systems that use an
> old U-Boot and a new kernel, which is again not really acceptable in my
> opinion.
> 
> For the reasons above I would like to ask U-Boot maintainers to pick up
> this patch.
> 
> Thanks
> Frieder
> 
> [1] https://lists.denx.de/pipermail/u-boot/2022-March/477828.html
> [2]
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20210522205136.19465-2-han.xu@nxp.com/
> 
> Am 20.05.21 um 11:09 schrieb Sean Nyekjaer:
>> On 13/05/2021 22.02, han.xu wrote:
>>> On 21/05/11 07:08AM, Sean Nyekjaer wrote:
>>>> Caution: EXT Email
>>>>
>>>> On 11/05/2021 04.49, han.xu wrote:
>>>>> On 21/05/10 12:00PM, Sean Nyekjaer wrote:
>>>>>> Caution: EXT Email
>>>>>>
>>>>>> Linux kernel defaults to use legacy bch setting, this was creating a
>>>>>> mismatch between U-boot and Linux default settings.
>>>>> Kernel uses the NAND chip specified minimum ecc strength and steps by default
>>>>> not the legacy bch setting, unless users enable it in DT file.
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Adding, mtd-list and Miquel
>>>>
>>>> With u-boot dtb:
>>>> &gpmi {
>>>>          pinctrl-names = "default";
>>>>          pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>>          compatible = "fsl,imx7d-gpmi-nand";
>>>>          nand-on-flash-bbt;
>>>>          status = "okay";
>>>> };
>>>>
>>>> With linux dtb (mainline 5.10):
>>>> &gpmi {
>>>>          pinctrl-names = "default";
>>>>          pinctrl-0 = <&pinctrl_gpmi_nand1>;
>>>>          nand-on-flash-bbt;
>>>>          status = "okay";
>>>> };
>>>>
>>>> U-boot prior to commit 51cdf83eea selected 18 bit ECC, after that commit it selects 8 bits.
>>>> With legacy option it selects 18.
>>>> Linux is selecting 18 bits ;) So now we have a mismatch.
>>>>
>>>> I have been searching for the legacy option in the mainline kernel can't find it ;)
>>>> Please show me where it is (is it in the NXP fork?)
>>>
>>> You are right, it's only fixed in NXP fork, with kernel driver modification. We
>>> prefer the current u-boot bch geometry, so I will send out a kernel patch to
>>> make them align.
>>>
>> Any progress on this?
>>
>> I see your patch in:
>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fmtd%2Fnand%2Fraw%2Fgpmi-nand%3Fh%3Dimx_5.4.70_2.3.0%26id%3Dae980dccc6189956fab047958ad0a70ec4951439&amp;data=04%7C01%7Cfrieder.schrempf%40kontron.de%7C275b7f11361e4fd3eb7e08d91b6f0e45%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637570986066857865%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=L4xAN9HDl68dmA%2FX6nnP%2Buqh6CUKTQZJeFWja5EvmPI%3D&amp;reserved=0
>>
>> But I can't find the option for selecting legacy mode in the devicetree...
>>
>> /Sean


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Mon, Mar 21, 2022 at 12:42 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
> >
> > Hi Ye,
> >
> > Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > > Hi Han,
> > >
> > > Am 17.03.22 um 14:33 schrieb Han Xu:
> > >>
> > >>
> > >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> > >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> > >>
> > >>     Hi Stefano,
> > >>
> > >>     this old patch was delegated to you in patchwork. If you're not the
> > >>     correct maintainer to address, please let me know. As the NAND layer
> > >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> > >>
> > >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> > >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> > >>     alters the BCH layout in a way that doesn't match with the
> > >>     implementation in the Linux kernel.
> > >>
> > >>     This causes failures when loading an UBI image in U-Boot that was
> > >>     flashed by Linux or vice versa (see [1]).
> > >>
> > >>     There has been an approach to fix this through an optional devicetree
> > >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> > >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> > >>     compatible with Linux should be used by default.
> > >>
> > >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> > >>     stalled and even if it would succeed, it would break systems that use an
> > >>     old U-Boot and a new kernel, which is again not really acceptable in my
> > >>     opinion.
> > >>
> > >>
> > >> Hi Frieder,
> > >>
> > >> I am not in office this week. I will send another patch set to change in
> > >> both kernel and u-boot to fix the compatible issue.
> > >
> > > You already claimed that months ago, but nothing happened:
> > >
> > >> I will send patches for both kernel and u-boot to use legacy bch
> > >> scheme by default, and add some code to treat few MLC nand chips as
> > >> corner cases.
> > >
> > > Sean's U-Boot patch is effectively reverting the default behavior to use
> > > the "legacy" BCH scheme. So that's in line with what you want to do and
> > > you can base your work on top of this fix. But we should get the basic
> > > fix in regardless.
> > >
> > > Even more so because switching the layout in U-Boot by using
> > > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > > which causes the bootloader size to increase by around 250 KiB in my
> > > case which might not be an option for some boards.
> >
> > Ye's reply copied over to not break the thread:
> >
> > > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > > use legacy bch. If this can't work for you in case you don't use DM
> > > driver, I prefer adding a config to select the legacy bch not
> > > reverting the patch.
> >
> > I think you miss my point. I don't really care about your preferences.
> > But I do care about not introducing breaking changes. IMHO the "correct"
> > way to introduce the new BCH layout would have been:
> >
> > 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> > (e.g. DT property)
> > 2. Make all upstream board configurations and DTs in Linux and U-Boot
> > use the feature-flag.
> > 3. Drop the flag and make the new BCH layout the default.
> >
> > That way the change would be much more controlled and raise the
> > awareness of board maintainers that are affected.
> >
> > The current situation (layout changes applied in U-Boot without any
> > questioning, layout changes in Linux rejected for good reasons) leaves
> > everyone who uses the GPMI NAND on i.MX in a situation where they
> > probably will see failures and need to spend some hours of debugging
> > until they find out the reason is that someone carelessly introduced
> > breaking changes.
> >
> > And even with the procedure described above we break compatibility
> > between old and new versions of U-Boot and Linux and create a dependency
> > between the two which is what we should try to avoid wherever possible.
> >
> > Frieder
>
> Frieder,
>
> I agree that we need to get this fixed ASAP. Because of this IMX GPMI
> NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
> be really nice to get this fixed in v2022.01 which releases in a
> couple of weeks. I'm surprised neither of us noticed this problem but
> likely we've both been so busy trying to keep up with forced DM
> migrations we haven't fully tested the 'little things' like booting to
> an actual OS. I know the boards I support using IMX GPMI NAND UBI are
> still using a 2017 U-Boot where quite honestly everything worked fine
> so there hasn't been a reason to push people to something new.
>
> That said, applying this patch does 'not' fix things for my boards.
> For my boards mxs_nand_set_geometry is called with:
> oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0
>
> and I still get ecc errors when mounting a UBI that worked fine with a
> v2020.04 U-Boot:
> Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
> ubi0: attaching mtd3
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 0:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 1:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 2:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> bytes from PEB 2:4096, read 4096 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 3:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> bytes from PEB 3:4096, read 4096 bytes
>
> The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
> layout setting for large oob NAND") no longer reverts and I haven't
> dug in to find out what I can do about that but i'm a little concerned
> I get different results than you do regarding the patch from this
> thread.
>
> Thank you for pursuing this issue!
>

It looks like for my board this patch on top of commit 51cdf83eea
("mtd: gpmi: provide the option to use legacy bch geometry") does
resolve the issue however on top of current 2022.04-rc4 there seems to
be a new problem for me.

Adding some debugging I find that I used to see the following:
mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
max_ecc_strength_supported=40
mxs_nand_legacy_calc_ecc_layout oobsize=224 ecc_chunk_count=8
ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=16

But now with this patch on top of 2022.04-rc4 I get:
mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
max_ecc_strength_supported=40
mxs_nand_legacy_calc_ecc_layout oobsize=128 ecc_chunk_count=8
ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=8
^^^ oobsize is wrong causing the wrong ecc strength (8 instead of the
correct 16)

I'm not sure yet what happened between 51cdf83eea and now that causes
oobsize to be wrong for my device.

Frieder, are you really able to run ok on 2022.04-rc4 with legacy mode selected?

Best Regards,

Tim

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Mon, Mar 21, 2022 at 12:42 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
> >
> > Hi Ye,
> >
> > Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > > Hi Han,
> > >
> > > Am 17.03.22 um 14:33 schrieb Han Xu:
> > >>
> > >>
> > >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> > >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> > >>
> > >>     Hi Stefano,
> > >>
> > >>     this old patch was delegated to you in patchwork. If you're not the
> > >>     correct maintainer to address, please let me know. As the NAND layer
> > >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> > >>
> > >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> > >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> > >>     alters the BCH layout in a way that doesn't match with the
> > >>     implementation in the Linux kernel.
> > >>
> > >>     This causes failures when loading an UBI image in U-Boot that was
> > >>     flashed by Linux or vice versa (see [1]).
> > >>
> > >>     There has been an approach to fix this through an optional devicetree
> > >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> > >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> > >>     compatible with Linux should be used by default.
> > >>
> > >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> > >>     stalled and even if it would succeed, it would break systems that use an
> > >>     old U-Boot and a new kernel, which is again not really acceptable in my
> > >>     opinion.
> > >>
> > >>
> > >> Hi Frieder,
> > >>
> > >> I am not in office this week. I will send another patch set to change in
> > >> both kernel and u-boot to fix the compatible issue.
> > >
> > > You already claimed that months ago, but nothing happened:
> > >
> > >> I will send patches for both kernel and u-boot to use legacy bch
> > >> scheme by default, and add some code to treat few MLC nand chips as
> > >> corner cases.
> > >
> > > Sean's U-Boot patch is effectively reverting the default behavior to use
> > > the "legacy" BCH scheme. So that's in line with what you want to do and
> > > you can base your work on top of this fix. But we should get the basic
> > > fix in regardless.
> > >
> > > Even more so because switching the layout in U-Boot by using
> > > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > > which causes the bootloader size to increase by around 250 KiB in my
> > > case which might not be an option for some boards.
> >
> > Ye's reply copied over to not break the thread:
> >
> > > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > > use legacy bch. If this can't work for you in case you don't use DM
> > > driver, I prefer adding a config to select the legacy bch not
> > > reverting the patch.
> >
> > I think you miss my point. I don't really care about your preferences.
> > But I do care about not introducing breaking changes. IMHO the "correct"
> > way to introduce the new BCH layout would have been:
> >
> > 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> > (e.g. DT property)
> > 2. Make all upstream board configurations and DTs in Linux and U-Boot
> > use the feature-flag.
> > 3. Drop the flag and make the new BCH layout the default.
> >
> > That way the change would be much more controlled and raise the
> > awareness of board maintainers that are affected.
> >
> > The current situation (layout changes applied in U-Boot without any
> > questioning, layout changes in Linux rejected for good reasons) leaves
> > everyone who uses the GPMI NAND on i.MX in a situation where they
> > probably will see failures and need to spend some hours of debugging
> > until they find out the reason is that someone carelessly introduced
> > breaking changes.
> >
> > And even with the procedure described above we break compatibility
> > between old and new versions of U-Boot and Linux and create a dependency
> > between the two which is what we should try to avoid wherever possible.
> >
> > Frieder
>
> Frieder,
>
> I agree that we need to get this fixed ASAP. Because of this IMX GPMI
> NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
> be really nice to get this fixed in v2022.01 which releases in a
> couple of weeks. I'm surprised neither of us noticed this problem but
> likely we've both been so busy trying to keep up with forced DM
> migrations we haven't fully tested the 'little things' like booting to
> an actual OS. I know the boards I support using IMX GPMI NAND UBI are
> still using a 2017 U-Boot where quite honestly everything worked fine
> so there hasn't been a reason to push people to something new.
>
> That said, applying this patch does 'not' fix things for my boards.
> For my boards mxs_nand_set_geometry is called with:
> oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0
>
> and I still get ecc errors when mounting a UBI that worked fine with a
> v2020.04 U-Boot:
> Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
> ubi0: attaching mtd3
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 0:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 1:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 2:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> bytes from PEB 2:4096, read 4096 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> from PEB 3:0, read 64 bytes
> ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> bytes from PEB 3:4096, read 4096 bytes
>
> The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
> layout setting for large oob NAND") no longer reverts and I haven't
> dug in to find out what I can do about that but i'm a little concerned
> I get different results than you do regarding the patch from this
> thread.
>
> Thank you for pursuing this issue!
>

It looks like for my board this patch on top of commit 51cdf83eea
("mtd: gpmi: provide the option to use legacy bch geometry") does
resolve the issue however on top of current 2022.04-rc4 there seems to
be a new problem for me.

Adding some debugging I find that I used to see the following:
mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
max_ecc_strength_supported=40
mxs_nand_legacy_calc_ecc_layout oobsize=224 ecc_chunk_count=8
ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=16

But now with this patch on top of 2022.04-rc4 I get:
mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
max_ecc_strength_supported=40
mxs_nand_legacy_calc_ecc_layout oobsize=128 ecc_chunk_count=8
ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=8
^^^ oobsize is wrong causing the wrong ecc strength (8 instead of the
correct 16)

I'm not sure yet what happened between 51cdf83eea and now that causes
oobsize to be wrong for my device.

Frieder, are you really able to run ok on 2022.04-rc4 with legacy mode selected?

Best Regards,

Tim

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Tue, Mar 22, 2022 at 10:13 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 21, 2022 at 12:42 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
> > <frieder.schrempf@kontron.de> wrote:
> > >
> > > Hi Ye,
> > >
> > > Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > > > Hi Han,
> > > >
> > > > Am 17.03.22 um 14:33 schrieb Han Xu:
> > > >>
> > > >>
> > > >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> > > >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> > > >>
> > > >>     Hi Stefano,
> > > >>
> > > >>     this old patch was delegated to you in patchwork. If you're not the
> > > >>     correct maintainer to address, please let me know. As the NAND layer
> > > >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> > > >>
> > > >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> > > >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> > > >>     alters the BCH layout in a way that doesn't match with the
> > > >>     implementation in the Linux kernel.
> > > >>
> > > >>     This causes failures when loading an UBI image in U-Boot that was
> > > >>     flashed by Linux or vice versa (see [1]).
> > > >>
> > > >>     There has been an approach to fix this through an optional devicetree
> > > >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> > > >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> > > >>     compatible with Linux should be used by default.
> > > >>
> > > >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> > > >>     stalled and even if it would succeed, it would break systems that use an
> > > >>     old U-Boot and a new kernel, which is again not really acceptable in my
> > > >>     opinion.
> > > >>
> > > >>
> > > >> Hi Frieder,
> > > >>
> > > >> I am not in office this week. I will send another patch set to change in
> > > >> both kernel and u-boot to fix the compatible issue.
> > > >
> > > > You already claimed that months ago, but nothing happened:
> > > >
> > > >> I will send patches for both kernel and u-boot to use legacy bch
> > > >> scheme by default, and add some code to treat few MLC nand chips as
> > > >> corner cases.
> > > >
> > > > Sean's U-Boot patch is effectively reverting the default behavior to use
> > > > the "legacy" BCH scheme. So that's in line with what you want to do and
> > > > you can base your work on top of this fix. But we should get the basic
> > > > fix in regardless.
> > > >
> > > > Even more so because switching the layout in U-Boot by using
> > > > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > > > which causes the bootloader size to increase by around 250 KiB in my
> > > > case which might not be an option for some boards.
> > >
> > > Ye's reply copied over to not break the thread:
> > >
> > > > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > > > use legacy bch. If this can't work for you in case you don't use DM
> > > > driver, I prefer adding a config to select the legacy bch not
> > > > reverting the patch.
> > >
> > > I think you miss my point. I don't really care about your preferences.
> > > But I do care about not introducing breaking changes. IMHO the "correct"
> > > way to introduce the new BCH layout would have been:
> > >
> > > 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> > > (e.g. DT property)
> > > 2. Make all upstream board configurations and DTs in Linux and U-Boot
> > > use the feature-flag.
> > > 3. Drop the flag and make the new BCH layout the default.
> > >
> > > That way the change would be much more controlled and raise the
> > > awareness of board maintainers that are affected.
> > >
> > > The current situation (layout changes applied in U-Boot without any
> > > questioning, layout changes in Linux rejected for good reasons) leaves
> > > everyone who uses the GPMI NAND on i.MX in a situation where they
> > > probably will see failures and need to spend some hours of debugging
> > > until they find out the reason is that someone carelessly introduced
> > > breaking changes.
> > >
> > > And even with the procedure described above we break compatibility
> > > between old and new versions of U-Boot and Linux and create a dependency
> > > between the two which is what we should try to avoid wherever possible.
> > >
> > > Frieder
> >
> > Frieder,
> >
> > I agree that we need to get this fixed ASAP. Because of this IMX GPMI
> > NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
> > be really nice to get this fixed in v2022.01 which releases in a
> > couple of weeks. I'm surprised neither of us noticed this problem but
> > likely we've both been so busy trying to keep up with forced DM
> > migrations we haven't fully tested the 'little things' like booting to
> > an actual OS. I know the boards I support using IMX GPMI NAND UBI are
> > still using a 2017 U-Boot where quite honestly everything worked fine
> > so there hasn't been a reason to push people to something new.
> >
> > That said, applying this patch does 'not' fix things for my boards.
> > For my boards mxs_nand_set_geometry is called with:
> > oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0
> >
> > and I still get ecc errors when mounting a UBI that worked fine with a
> > v2020.04 U-Boot:
> > Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
> > ubi0: attaching mtd3
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 0:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 1:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 2:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> > bytes from PEB 2:4096, read 4096 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 3:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> > bytes from PEB 3:4096, read 4096 bytes
> >
> > The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
> > layout setting for large oob NAND") no longer reverts and I haven't
> > dug in to find out what I can do about that but i'm a little concerned
> > I get different results than you do regarding the patch from this
> > thread.
> >
> > Thank you for pursuing this issue!
> >
>
> It looks like for my board this patch on top of commit 51cdf83eea
> ("mtd: gpmi: provide the option to use legacy bch geometry") does
> resolve the issue however on top of current 2022.04-rc4 there seems to
> be a new problem for me.
>
> Adding some debugging I find that I used to see the following:
> mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
> max_ecc_strength_supported=40
> mxs_nand_legacy_calc_ecc_layout oobsize=224 ecc_chunk_count=8
> ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=16
>
> But now with this patch on top of 2022.04-rc4 I get:
> mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
> max_ecc_strength_supported=40
> mxs_nand_legacy_calc_ecc_layout oobsize=128 ecc_chunk_count=8
> ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=8
> ^^^ oobsize is wrong causing the wrong ecc strength (8 instead of the
> correct 16)
>
> I'm not sure yet what happened between 51cdf83eea and now that causes
> oobsize to be wrong for my device.
>
> Frieder, are you really able to run ok on 2022.04-rc4 with legacy mode selected?
>

I discovered my issue regarding the wrong oobsize. When I converted to
DM_MTD I removed CONFIG_SYS_NAND_ONFI_DETECTION which was needed.

With that fixed, Sean's patch here resolves the issue and puts me in
sync with your findings.

I will take a look at Han's latest patch and respond to that thread.

Best Regards,

Tim

Re: [EXT] [PATCH] mtd: mxs_nand: default to legacy bch and rename to modern bch option

[Tim Harvey]

On Tue, Mar 22, 2022 at 10:13 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Mon, Mar 21, 2022 at 12:42 PM Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Mon, Mar 21, 2022 at 1:34 AM Frieder Schrempf
> > <frieder.schrempf@kontron.de> wrote:
> > >
> > > Hi Ye,
> > >
> > > Am 17.03.22 um 14:54 schrieb Frieder Schrempf:
> > > > Hi Han,
> > > >
> > > > Am 17.03.22 um 14:33 schrieb Han Xu:
> > > >>
> > > >>
> > > >> On Thu, Mar 17, 2022 at 8:27 AM Frieder Schrempf
> > > >> <frieder.schrempf@kontron.de <mailto:frieder.schrempf@kontron.de>> wrote:
> > > >>
> > > >>     Hi Stefano,
> > > >>
> > > >>     this old patch was delegated to you in patchwork. If you're not the
> > > >>     correct maintainer to address, please let me know. As the NAND layer
> > > >>     seems to be unmaintained at the moment, I'm not sure whom to ask.
> > > >>
> > > >>     This patch fixes a regression that was introduced by 616f03dabacb ("
> > > >>     mtd: gpmi: change the BCH layout setting for large oob NAND") which
> > > >>     alters the BCH layout in a way that doesn't match with the
> > > >>     implementation in the Linux kernel.
> > > >>
> > > >>     This causes failures when loading an UBI image in U-Boot that was
> > > >>     flashed by Linux or vice versa (see [1]).
> > > >>
> > > >>     There has been an approach to fix this through an optional devicetree
> > > >>     property in 51cdf83eea ("mtd: gpmi: provide the option to use legacy bch
> > > >>     geometry"), but this is not acceptable. The "legacy" BCH layout
> > > >>     compatible with Linux should be used by default.
> > > >>
> > > >>     The approach to upstream the "new" layout to the kernel [2] seems to be
> > > >>     stalled and even if it would succeed, it would break systems that use an
> > > >>     old U-Boot and a new kernel, which is again not really acceptable in my
> > > >>     opinion.
> > > >>
> > > >>
> > > >> Hi Frieder,
> > > >>
> > > >> I am not in office this week. I will send another patch set to change in
> > > >> both kernel and u-boot to fix the compatible issue.
> > > >
> > > > You already claimed that months ago, but nothing happened:
> > > >
> > > >> I will send patches for both kernel and u-boot to use legacy bch
> > > >> scheme by default, and add some code to treat few MLC nand chips as
> > > >> corner cases.
> > > >
> > > > Sean's U-Boot patch is effectively reverting the default behavior to use
> > > > the "legacy" BCH scheme. So that's in line with what you want to do and
> > > > you can base your work on top of this fix. But we should get the basic
> > > > fix in regardless.
> > > >
> > > > Even more so because switching the layout in U-Boot by using
> > > > fsl,legacy-bch-geometry in the devicetree requires CONFIG_DM_MTD=y,
> > > > which causes the bootloader size to increase by around 250 KiB in my
> > > > case which might not be an option for some boards.
> > >
> > > Ye's reply copied over to not break the thread:
> > >
> > > > The dt nand driver will check "fsl,legacy-bch-geometry" property to
> > > > use legacy bch. If this can't work for you in case you don't use DM
> > > > driver, I prefer adding a config to select the legacy bch not
> > > > reverting the patch.
> > >
> > > I think you miss my point. I don't really care about your preferences.
> > > But I do care about not introducing breaking changes. IMHO the "correct"
> > > way to introduce the new BCH layout would have been:
> > >
> > > 1. Introduce the new layout in Linux and U-Boot behind a feature-flag
> > > (e.g. DT property)
> > > 2. Make all upstream board configurations and DTs in Linux and U-Boot
> > > use the feature-flag.
> > > 3. Drop the flag and make the new BCH layout the default.
> > >
> > > That way the change would be much more controlled and raise the
> > > awareness of board maintainers that are affected.
> > >
> > > The current situation (layout changes applied in U-Boot without any
> > > questioning, layout changes in Linux rejected for good reasons) leaves
> > > everyone who uses the GPMI NAND on i.MX in a situation where they
> > > probably will see failures and need to spend some hours of debugging
> > > until they find out the reason is that someone carelessly introduced
> > > breaking changes.
> > >
> > > And even with the procedure described above we break compatibility
> > > between old and new versions of U-Boot and Linux and create a dependency
> > > between the two which is what we should try to avoid wherever possible.
> > >
> > > Frieder
> >
> > Frieder,
> >
> > I agree that we need to get this fixed ASAP. Because of this IMX GPMI
> > NAND UBI has been broken since v2020.07 (6 releases ago!) and it would
> > be really nice to get this fixed in v2022.01 which releases in a
> > couple of weeks. I'm surprised neither of us noticed this problem but
> > likely we've both been so busy trying to keep up with forced DM
> > migrations we haven't fully tested the 'little things' like booting to
> > an actual OS. I know the boards I support using IMX GPMI NAND UBI are
> > still using a 2017 U-Boot where quite honestly everything worked fine
> > so there hasn't been a reason to push people to something new.
> >
> > That said, applying this patch does 'not' fix things for my boards.
> > For my boards mxs_nand_set_geometry is called with:
> > oobsize=224 ecc_str_ds=0 ecc_step_ds=0 modern=0
> >
> > and I still get ecc errors when mounting a UBI that worked fine with a
> > v2020.04 U-Boot:
> > Ventana > ubi part rootfs && ubifsmount ubi0:boot && ubifsls && ubifsumount
> > ubi0: attaching mtd3
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 0:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 1:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 2:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> > bytes from PEB 2:4096, read 4096 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 64 bytes
> > from PEB 3:0, read 64 bytes
> > ubi0 error: ubi_io_read: error -74 (ECC error) while reading 4096
> > bytes from PEB 3:4096, read 4096 bytes
> >
> > The original offending commit 616f03dabacb ("mtd: gpmi: change the BCH
> > layout setting for large oob NAND") no longer reverts and I haven't
> > dug in to find out what I can do about that but i'm a little concerned
> > I get different results than you do regarding the patch from this
> > thread.
> >
> > Thank you for pursuing this issue!
> >
>
> It looks like for my board this patch on top of commit 51cdf83eea
> ("mtd: gpmi: provide the option to use legacy bch geometry") does
> resolve the issue however on top of current 2022.04-rc4 there seems to
> be a new problem for me.
>
> Adding some debugging I find that I used to see the following:
> mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
> max_ecc_strength_supported=40
> mxs_nand_legacy_calc_ecc_layout oobsize=224 ecc_chunk_count=8
> ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=16
>
> But now with this patch on top of 2022.04-rc4 I get:
> mxs_nand_set_geometry legacy_bch_geometry=1 ecc_strength_ds=0
> max_ecc_strength_supported=40
> mxs_nand_legacy_calc_ecc_layout oobsize=128 ecc_chunk_count=8
> ecc_chunk0_size=512 ecc_chunkn_size=512 ecc_strength=8
> ^^^ oobsize is wrong causing the wrong ecc strength (8 instead of the
> correct 16)
>
> I'm not sure yet what happened between 51cdf83eea and now that causes
> oobsize to be wrong for my device.
>
> Frieder, are you really able to run ok on 2022.04-rc4 with legacy mode selected?
>

I discovered my issue regarding the wrong oobsize. When I converted to
DM_MTD I removed CONFIG_SYS_NAND_ONFI_DETECTION which was needed.

With that fixed, Sean's patch here resolves the issue and puts me in
sync with your findings.

I will take a look at Han's latest patch and respond to that thread.

Best Regards,

Tim

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/