Re: [PATCH v1] Revert "ARM: dts: imx7: Fix NAND controller size-cells"

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marek Vasut <marex@denx.de>
Cc: Francesco Dolcini <francesco@dolcini.it>,
	Shawn Guo <shawnguo@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Francesco Dolcini <francesco.dolcini@toradex.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	devicetree@vger.kernel.org, linux-mtd@lists.infradead.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v1] Revert "ARM: dts: imx7: Fix NAND controller size-cells"
Date: Mon, 5 Dec 2022 19:18:28 +0100	[thread overview]
Message-ID: <20221205191828.3072d872@xps-13> (raw)
In-Reply-To: <f69746b0-51c0-041c-4035-679c27fcba64@denx.de>

Hi Marek,

marex@denx.de wrote on Mon, 5 Dec 2022 19:07:14 +0100:

> On 12/5/22 18:58, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi,
> 
> > marex@denx.de wrote on Mon, 5 Dec 2022 17:26:53 +0100:
> >   
> >> On 12/5/22 16:23, Francesco Dolcini wrote:  
> >>> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> >>>
> >>> This reverts commit 753395ea1e45c724150070b5785900b6a44bd5fb.
> >>>
> >>> It introduced a boot regression on colibri-imx7, and potentially any
> >>> other i.MX7 boards with MTD partition list generated into the fdt by
> >>> U-Boot.
> >>>
> >>> While the commit we are reverting here is not obviously wrong, it fixes
> >>> only a dt binding checker warning that is non-functional, while it
> >>> introduces a boot regression and there is no obvious fix ready.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 753395ea1e45 ("ARM: dts: imx7: Fix NAND controller size-cells")
> >>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> >>> Link: https://lore.kernel.org/all/20221205144917.6514168a@xps-13/
> >>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> >>> ---
> >>>    arch/arm/boot/dts/imx7s.dtsi | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> >>> index 03d2e8544a4e..0fc9e6b8b05d 100644
> >>> --- a/arch/arm/boot/dts/imx7s.dtsi
> >>> +++ b/arch/arm/boot/dts/imx7s.dtsi
> >>> @@ -1270,10 +1270,10 @@ dma_apbh: dma-apbh@33000000 {
> >>>    			clocks = <&clks IMX7D_NAND_USDHC_BUS_RAWNAND_CLK>;
> >>>    		};  
> >>>    > -		gpmi: nand-controller@33002000 {  
> >>> +		gpmi: nand-controller@33002000{
> >>>    			compatible = "fsl,imx7d-gpmi-nand";
> >>>    			#address-cells = <1>;
> >>> -			#size-cells = <0>;
> >>> +			#size-cells = <1>;
> >>>    			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
> >>>    			reg-names = "gpmi-nand", "bch";
> >>>    			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;  
> >>
> >> I suspect this fix should eventually be reverted again, once a proper fix is agreed upon in the MTD OF parser, right ?  
> > 
> > I guess it's time to migrate to a more modern definition  
> 
> Is that the nand-chip@N { status="disabled"; } part ?

I would prefer the controller to be disabled if useless, but otherwise
yes.

> 
> >, it's not
> > complex to do, there are plenty of examples. This would be IMHO a
> > better step ahead rather than just a cell change. Anyway, I don't mind
> > reverting this once we've sorted this mess out and fixed U-Boot.  
> 
> Won't we still have issues with older bootloader versions which paste partitions directly into this &gpmi {} node, and which needs to be fixed up in the parser in the end ?

I believe fdt_fixup_mtdparts() should be killed, so we should no longer
have this problem.

Thanks,
Miquèl

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