| Message ID | 20221205152327.26881-1-francesco@dolcini.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] Revert "ARM: dts: imx7: Fix NAND controller size-cells" | expand |
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 ? With that: Acked-by: Marek Vasut <marex@denx.de>
Hi Francesco, francesco@dolcini.it wrote on Mon, 5 Dec 2022 16:23:27 +0100: > 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> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.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>; Thanks, Miquèl
Hi Marek, 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, 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. Cheers, Miquèl
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 ? >, 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 ?
On Mon, Dec 05, 2022 at 07:07:14PM +0100, Marek Vasut wrote: > On 12/5/22 18:58, Miquel Raynal wrote: > > marex@denx.de wrote on Mon, 5 Dec 2022 17:26:53 +0100: > > , 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 ? Yes, I think so. While I do agree on printk warning and deprecated functions and use more modern and less problematic stuff, this should not come at the cost of failing the boot on board using some old U-Boot version. Francesco
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
On 12/5/22 19:18, Miquel Raynal wrote: > 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. ACK, but see below. >>> , 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. The fdt_fixup_mtdparts is U-Boot code. If contemporary Linux kernel is booted with ancient U-Boot, then you would still get defective DT passed to Linux, and that should be fixed up by Linux. Removing fdt_fixup_mtdparts() from current mainline U-Boot won't solve this problem. I think this is also what Francesco is trying to convey (please correct me if I'm wrong).
On Mon, Dec 05, 2022 at 07:52:08PM +0100, Marek Vasut wrote: > On 12/5/22 19:18, Miquel Raynal wrote: > > marex@denx.de wrote on Mon, 5 Dec 2022 19:07:14 +0100: > > > On 12/5/22 18:58, Miquel Raynal wrote: > > > > , 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. > > The fdt_fixup_mtdparts is U-Boot code. If contemporary Linux kernel is > booted with ancient U-Boot, then you would still get defective DT passed to > Linux, and that should be fixed up by Linux. Removing fdt_fixup_mtdparts() > from current mainline U-Boot won't solve this problem. > > I think this is also what Francesco is trying to convey (please correct me > if I'm wrong). Yes, exactly, thanks! Francesco
Hi Francesco, francesco@dolcini.it wrote on Mon, 5 Dec 2022 20:07:18 +0100: > On Mon, Dec 05, 2022 at 07:52:08PM +0100, Marek Vasut wrote: > > On 12/5/22 19:18, Miquel Raynal wrote: > > > marex@denx.de wrote on Mon, 5 Dec 2022 19:07:14 +0100: > > > > On 12/5/22 18:58, Miquel Raynal wrote: > > > > > , 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. > > > > The fdt_fixup_mtdparts is U-Boot code. If contemporary Linux kernel is > > booted with ancient U-Boot, then you would still get defective DT passed to > > Linux, and that should be fixed up by Linux. Removing fdt_fixup_mtdparts() > > from current mainline U-Boot won't solve this problem. > > > > I think this is also what Francesco is trying to convey (please correct me > > if I'm wrong). If we can get rid of fdt_fixup_mtdparts(), it means someone has to create the partitions. I guess the easy way would be to just provide mtdparts to Linux like all the other boards and let Linux deal with it. Then we can just assume in Linux that perhaps if the partitions are invalid (#size-cell is wrong?) then we should just stop their creation and fallback to another mechanism instead of failing entirely. This way no need for hackish changes in the parsers and compatibility is still valid with old U-Boot (if mtdparts was provided on the cmdline, to be checked). Otherwise we'll have to deal with it in Linux, that's a pity. Thanks, Miquèl
On 12/6/22 11:16, Miquel Raynal wrote: > Hi Francesco, Hello everyone, > francesco@dolcini.it wrote on Mon, 5 Dec 2022 20:07:18 +0100: > >> On Mon, Dec 05, 2022 at 07:52:08PM +0100, Marek Vasut wrote: >>> On 12/5/22 19:18, Miquel Raynal wrote: >>>> marex@denx.de wrote on Mon, 5 Dec 2022 19:07:14 +0100: >>>>> On 12/5/22 18:58, Miquel Raynal wrote: >>>>>> , 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. >>> >>> The fdt_fixup_mtdparts is U-Boot code. If contemporary Linux kernel is >>> booted with ancient U-Boot, then you would still get defective DT passed to >>> Linux, and that should be fixed up by Linux. Removing fdt_fixup_mtdparts() >>> from current mainline U-Boot won't solve this problem. >>> >>> I think this is also what Francesco is trying to convey (please correct me >>> if I'm wrong). > > If we can get rid of fdt_fixup_mtdparts(), it means someone has to > create the partitions. I guess the easy way would be to just provide > mtdparts to Linux like all the other boards and let Linux deal with it. This is based on an assumption that the platform kernel command line can be updated to insert such a workaround. If Francesco cannot update the bootloader, the kernel command line may be immutable all the same. > Then we can just assume in Linux that perhaps if the partitions are > invalid (#size-cell is wrong?) then we should just stop their creation > and fallback to another mechanism instead of failing entirely. This way > no need for hackish changes in the parsers and compatibility is still > valid with old U-Boot (if mtdparts was provided on the cmdline, to be > checked). Otherwise we'll have to deal with it in Linux, that's a pity. I am very much banking toward -- fix it up in the parser, just like any other firmware issue. Esp. since the fix up is printing a warning, and it is like a 2-liner patch.
On Tue, Dec 06, 2022 at 08:02:45PM +0100, Marek Vasut wrote: > On 12/6/22 11:16, Miquel Raynal wrote: > > Hi Francesco, > > Hello everyone, > > > francesco@dolcini.it wrote on Mon, 5 Dec 2022 20:07:18 +0100: > > > > > On Mon, Dec 05, 2022 at 07:52:08PM +0100, Marek Vasut wrote: > > > > On 12/5/22 19:18, Miquel Raynal wrote: > > > > > marex@denx.de wrote on Mon, 5 Dec 2022 19:07:14 +0100: > > > > > > On 12/5/22 18:58, Miquel Raynal wrote: > > > > > > > , 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. > > > > > > > > The fdt_fixup_mtdparts is U-Boot code. If contemporary Linux kernel is > > > > booted with ancient U-Boot, then you would still get defective DT passed to > > > > Linux, and that should be fixed up by Linux. Removing fdt_fixup_mtdparts() > > > > from current mainline U-Boot won't solve this problem. > > > > > > > > I think this is also what Francesco is trying to convey (please correct me > > > > if I'm wrong). > > > > If we can get rid of fdt_fixup_mtdparts(), it means someone has to > > create the partitions. I guess the easy way would be to just provide > > mtdparts to Linux like all the other boards and let Linux deal with it. > > This is based on an assumption that the platform kernel command line can be > updated to insert such a workaround. If Francesco cannot update the > bootloader, the kernel command line may be immutable all the same. Exactly. What I can do is update the latest stuff and enable people to upgrade, eventually. But here we are talking about a board that is just generally available in high volume to a multitude of people since years ... in practice the vast majority of the users will not upgrade it. > > Then we can just assume in Linux that perhaps if the partitions are > > invalid (#size-cell is wrong?) then we should just stop their creation > > and fallback to another mechanism instead of failing entirely. This way > > no need for hackish changes in the parsers and compatibility is still > > valid with old U-Boot (if mtdparts was provided on the cmdline, to be > > checked). Otherwise we'll have to deal with it in Linux, that's a pity. > > I am very much banking toward -- fix it up in the parser, just like any > other firmware issue. I agree again. > Esp. since the fix up is printing a warning, and it is like a 2-liner > patch. Here we might assess if we need more to handle the other weird situation in which a `partitions{}` node is present, U-Boot ignores it and the kernel fails to detect the partitions. As of today colibri-imx7 is not affected by this. Francesco
Hi Shawn, + Thorsten 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> [...] > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> [...] > Acked-by: Marek Vasut <marex@denx.de> [...] As discussed in the above links, boot is broken on imx7 Colibri boards, this revert was the most quick and straightforward fix we agreed upon with the hope (~ duty?) it would make it in v6.1. Any chance you could pick this up rapidly and forward it to Linus? Or should we involve him directly (Thorsten?). Thanks, Miquèl
On 08.12.22 11:51, Miquel Raynal wrote: > Hi Shawn, > > + Thorsten > > 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> > [...] >> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > [...] >> Acked-by: Marek Vasut <marex@denx.de> > [...] > > As discussed in the above links, boot is broken on imx7 Colibri boards, > this revert was the most quick and straightforward fix we agreed upon > with the hope (~ duty?) it would make it in v6.1. Any chance you could > pick this up rapidly and forward it to Linus? Or should we involve > him directly (Thorsten?). Asking Linus directly often is fine, if it's something urgent and the maintainer that usually would handle a patch is MIA. But in this particular case it's likely not the best strategy, as it seems 753395ea1e45 was merged via the ARM soc tree. In that case I'd say the revert should ideally go through there as well, hence I'd suggest asking those maintainers (e.g. Arnd and Olof) is the right move at this point in time (would be something different if today was release day; but even then it would be wise to have them involved). Ciao, Thorsten
On 12/8/22 11:51, Miquel Raynal wrote: > Hi Shawn, Hi, > + Thorsten > > 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> > [...] >> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > [...] >> Acked-by: Marek Vasut <marex@denx.de> > [...] > > As discussed in the above links, boot is broken on imx7 Colibri boards, > this revert was the most quick and straightforward fix we agreed upon > with the hope (~ duty?) it would make it in v6.1. Any chance you could > pick this up rapidly and forward it to Linus? Or should we involve > him directly (Thorsten?). It seems neither Francesco nor me agree that this is the right approach and rather the fix should be the two-liner change to the OF partition parser, so maybe this should not be picked ?
Il 8 dicembre 2022 14:21:31 CET, Marek Vasut <marex@denx.de> ha scritto: >On 12/8/22 11:51, Miquel Raynal wrote: >> Hi Shawn, > >Hi, > >> + Thorsten >> >> 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> >> [...] >>> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> >> [...] >>> Acked-by: Marek Vasut <marex@denx.de> >> [...] >> >> As discussed in the above links, boot is broken on imx7 Colibri boards, >> this revert was the most quick and straightforward fix we agreed upon >> with the hope (~ duty?) it would make it in v6.1. Any chance you could >> pick this up rapidly and forward it to Linus? Or should we involve >> him directly (Thorsten?). > >It seems neither Francesco nor me agree that this is the right approach and rather the fix should be the two-liner change to the OF partition parser, so maybe this should not be picked ? I think that the 2 lines change might not be good enough to properly handle the U-Boot generated OF partitions in the general case, even if it fixes my specific issue. Given that I would do the revert as an immediate first step. Francesco
On 12/8/22 14:49, Francesco Dolcini wrote: > Il 8 dicembre 2022 14:21:31 CET, Marek Vasut <marex@denx.de> ha scritto: >> On 12/8/22 11:51, Miquel Raynal wrote: >>> Hi Shawn, >> >> Hi, >> >>> + Thorsten >>> >>> 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> >>> [...] >>>> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> >>> [...] >>>> Acked-by: Marek Vasut <marex@denx.de> >>> [...] >>> >>> As discussed in the above links, boot is broken on imx7 Colibri boards, >>> this revert was the most quick and straightforward fix we agreed upon >>> with the hope (~ duty?) it would make it in v6.1. Any chance you could >>> pick this up rapidly and forward it to Linus? Or should we involve >>> him directly (Thorsten?). >> >> It seems neither Francesco nor me agree that this is the right approach and rather the fix should be the two-liner change to the OF partition parser, so maybe this should not be picked ? > > I think that the 2 lines change might not be good enough to properly handle the U-Boot generated OF partitions in the general case, even if it fixes my specific issue. > > Given that I would do the revert as an immediate first step. Then that's fine, let's do it. Will you also follow up on the parser fix please ?
On Thu, Dec 08, 2022 at 02:54:43PM +0100, Marek Vasut wrote: > On 12/8/22 14:49, Francesco Dolcini wrote: > > Given that I would do the revert as an immediate first step. > > Then that's fine, let's do it. > > Will you also follow up on the parser fix please ? Yep, I'll try to squeeze some time to work on it in the next weeks. Francesco
+ Arnd On Thu, Dec 08, 2022 at 11:51:24AM +0100, Miquel Raynal wrote: > 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> > [...] > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > [...] > > Acked-by: Marek Vasut <marex@denx.de> > [...] > > As discussed in the above links, boot is broken on imx7 Colibri boards, > this revert was the most quick and straightforward fix we agreed upon > with the hope (~ duty?) it would make it in v6.1. Any chance you could > pick this up rapidly and forward it to Linus? Or should we involve > him directly (Thorsten?). Hello Arnd, FYI - see Miquel explanation above. Francesco
On 08.12.22 17:40, Francesco Dolcini wrote: > + Arnd Arnd, have you seen this? We haven't heard anything from Shawn afaics, who normally would take care of a patch like this. Hence could you consider picking up the patch at the start of this thread (e.g. https://lore.kernel.org/all/20221205152327.26881-1-francesco@dolcini.it/ ) and send it to Linus in the next 48 hours? It seems low-risk and fixes a regression introduced this cycle various people care about. That's why I'll likely ask Linus to consider picking this up directly before releasing 6.1, if I don't hear anything from you soon. But I'd prefer if the patch would go through at least somewhat the proper channels; alternatively a ACK from you to signal Linus "yeah, pick this up" would help as well. Ciao, Thorsten > On Thu, Dec 08, 2022 at 11:51:24AM +0100, Miquel Raynal wrote: >> 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> >> [...] >>> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> >> [...] >>> Acked-by: Marek Vasut <marex@denx.de> >> [...] >> >> As discussed in the above links, boot is broken on imx7 Colibri boards, >> this revert was the most quick and straightforward fix we agreed upon >> with the hope (~ duty?) it would make it in v6.1. Any chance you could >> pick this up rapidly and forward it to Linus? Or should we involve >> him directly (Thorsten?). > > Hello Arnd, > FYI - see Miquel explanation above. > > Francesco >
On Fri, Dec 9, 2022, at 14:30, Thorsten Leemhuis wrote: > On 08.12.22 17:40, Francesco Dolcini wrote: >> + Arnd > > Arnd, have you seen this? We haven't heard anything from Shawn afaics, > who normally would take care of a patch like this. Hence could you > consider picking up the patch at the start of this thread (e.g. > https://lore.kernel.org/all/20221205152327.26881-1-francesco@dolcini.it/ > ) and send it to Linus in the next 48 hours? It seems low-risk and fixes > a regression introduced this cycle various people care about. That's why > I'll likely ask Linus to consider picking this up directly before > releasing 6.1, if I don't hear anything from you soon. But I'd prefer if > the patch would go through at least somewhat the proper channels; > alternatively a ACK from you to signal Linus "yeah, pick this up" would > help as well. Done now. Arnd
Hi Arnd, arnd@arndb.de wrote on Fri, 09 Dec 2022 16:25:28 +0100: > On Fri, Dec 9, 2022, at 14:30, Thorsten Leemhuis wrote: > > On 08.12.22 17:40, Francesco Dolcini wrote: > >> + Arnd > > > > Arnd, have you seen this? We haven't heard anything from Shawn afaics, > > who normally would take care of a patch like this. Hence could you > > consider picking up the patch at the start of this thread (e.g. > > https://lore.kernel.org/all/20221205152327.26881-1-francesco@dolcini.it/ > > ) and send it to Linus in the next 48 hours? It seems low-risk and fixes > > a regression introduced this cycle various people care about. That's why > > I'll likely ask Linus to consider picking this up directly before > > releasing 6.1, if I don't hear anything from you soon. But I'd prefer if > > the patch would go through at least somewhat the proper channels; > > alternatively a ACK from you to signal Linus "yeah, pick this up" would > > help as well. > > Done now. > > Arnd Thanks a lot. Miquèl
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>;