Re: [PATCH v2 2/8] dts: imx: Use minimum NAND ECC strength on BSH boards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ARM: dts: imx: unjustified incompatible NAND ECC layout change on BSH boards
--

commit 18a50759ffc5f62c90affe97ba8849157120079d
Author: Michael Trimarchi <michael@amarulasolutions.com>

dts: imx: Use minimum NAND ECC strength on BSH boards

This commit enables the fsl,use-minimum-ecc property on the GPMI NAND
controller nodes for the BSH SMM-M2 and SMM-S2 boards.

>     dts: imx: Use minimum NAND ECC strength on BSH boards
>     
>     Configure the GPMI NAND controllers on the BSH SMM-M2 and SMM-S2
>     to use the minimum ECC strength required by the NAND flash.
>     
>     Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
>     Signed-off-by: Haoning Cheng <Haoning.CHENG@cn.bosch.com>
> 
> diff --git a/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2.dts b/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2.dts
> index 2d9f495660c9a..aac211a9f9ebd 100644
> --- a/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2.dts
> +++ b/arch/arm/boot/dts/nxp/imx/imx6ulz-bsh-smm-m2.dts
> @@ -26,6 +26,7 @@ &gpmi {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_gpmi_nand>;
>  	nand-on-flash-bbt;
> +	fsl,use-minimum-ecc;
>  	status = "okay";
>  };
>  
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2.dts b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2.dts
> index 11a1ba5bfdb7e..df11f81ac50c1 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-bsh-smm-s2.dts
> @@ -21,6 +21,7 @@ memory@40000000 {
>  &gpmi {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_gpmi_nand>;
> +	fsl,use-minimum-ecc;

[Severity: Medium]
As reported by Sashiko on v1 [1], both imx6ulz-bsh-smm-m2.dts and
imx8mn-bsh-smm-s2.dts have been in-tree since 2021 without
fsl,use-minimum-ecc.

In drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c, gpmi_nand_probe() calls
common_nfc_set_geometry() to configure the BCH hardware:

	use_minimun_ecc = of_property_read_bool(this->dev->of_node,
						"fsl,use-minimum-ecc");

	/* use legacy bch geometry settings by default*/
	if ((!use_minimun_ecc && mtd->oobsize < 1024) ||
	    !(requirements->strength > 0 && requirements->step_size > 0)) {
		dev_dbg(this->dev, "use legacy bch geometry\n");
		err = legacy_set_geometry(this);
		if (!err)
			return 0;
	}

Without fsl,use-minimum-ecc (when mtd->oobsize < 1024),
common_nfc_set_geometry() calls legacy_set_geometry(), which maximizes
geo->ecc_strength across the available OOB area via get_ecc_strength().
Adding fsl,use-minimum-ecc bypasses legacy_set_geometry() and instead calls
set_geometry_by_ecc_info() to use the NAND chip's minimum required ECC
strength and step size.

Because the GPMI BCH controller interleaves metadata, data chunks, and ECC
parity bits across the physical page and computes geo->page_size,
geo->block_mark_byte_offset, and geo->block_mark_bit_offset from
geo->ecc_strength and geo->gf_len, switching from legacy_set_geometry() to
set_geometry_by_ecc_info() alters the physical on-flash page layout.

Won't this change cause a regression on existing boards where NAND
partitions, UBI volumes, or on-flash bad block tables formatted under the
previous device tree become unreadable with -EBADMSG ECC errors? Could the
commit message also explain why this incompatible layout change is needed
(for example, to match the ECC geometry used by U-Boot or the BootROM) and
how existing flashed devices are affected?

[1] https://lore.kernel.org/r/20261104-add-bsh-smm-m2-v1-2-369f0a77e021@cn.bosch.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-add-bsh-smm-m2-v2-0-fb5ffc7da486@cn.bosch.com?part=2