Re: [PATCH 02/10] 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 8322a44ab1436f5d874adef6cee6881bf71a02c7
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
controllers for the BSH SMM-M2 and SMM-S2 boards.

> 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]
Does adding fsl,use-minimum-ecc to these existing board device trees cause an
incompatible on-flash ECC layout regression for already-provisioned devices?

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

    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 OOB area via get_ecc_strength(). With
fsl,use-minimum-ecc enabled, it instead calls set_geometry_by_ecc_info()
using the NAND chip's minimum required ECC strength and step size, changing
geo->ecc_strength, geo->page_size, geo->block_mark_byte_offset, and
geo->block_mark_bit_offset.

As a result, any NAND partitions written using the previous device tree will
fail to read with -EBADMSG ECC errors. Could the commit message explain why
this change is needed (for example, to match the ECC layout expected by the
bootloader or ROM) and why breaking compatibility with existing on-flash
layouts on these boards is acceptable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-add-bsh-smm-m2-v1-0-369f0a77e021@cn.bosch.com?part=2