| Message ID | 20230113-syscon-child-mfd-v1-2-0dd31b7de373@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Support Child MFD Cells for the i.MX6QDL Reset Controller | expand |
On 16/01/2023 09:20, Krzysztof Kozlowski wrote: > On 13/01/2023 18:32, Bastian Krause wrote: >> The actual syscon-reboot-mode child node can be added by a board >> device-tree or fixed up by the bootloader. For the child node to be >> probed, the compatible needs to include simple-mfd. The binding now >> specifies this, so have the SoC dtsi adhere to it. >> >> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> Signed-off-by: Bastian Krause <bst@pengutronix.de> >> --- >> arch/arm/boot/dts/imx6qdl.dtsi | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi >> index ff1e0173b39be..b16be39458aa6 100644 >> --- a/arch/arm/boot/dts/imx6qdl.dtsi >> +++ b/arch/arm/boot/dts/imx6qdl.dtsi >> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */ >> }; >> >> src: reset-controller@20d8000 { >> - compatible = "fsl,imx6q-src", "fsl,imx51-src"; >> + compatible = "fsl,imx6q-src", "fsl,imx51-src", >> + "syscon", "simple-mfd"; > > You need children here. Otherwise simple-mfd does not make sense. If you > expect something else to add children (I don't understand why, usually > reboot capability is fixed per SoC and only sometimes extended with some > other means), then this "else" will also change compatible. Hm, although dtbs_check would complain... so let's clarify - why you are not adding syscon-reboot now? It should be always here - either SoC supports it or it does not. Board support is other thing, but this is DTSI for SoC. Best regards, Krzysztof
Hello Krzysztof, On 16.01.23 09:20, Krzysztof Kozlowski wrote: > On 13/01/2023 18:32, Bastian Krause wrote: >> The actual syscon-reboot-mode child node can be added by a board >> device-tree or fixed up by the bootloader. For the child node to be >> probed, the compatible needs to include simple-mfd. The binding now >> specifies this, so have the SoC dtsi adhere to it. >> >> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> Signed-off-by: Bastian Krause <bst@pengutronix.de> >> --- >> arch/arm/boot/dts/imx6qdl.dtsi | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi >> index ff1e0173b39be..b16be39458aa6 100644 >> --- a/arch/arm/boot/dts/imx6qdl.dtsi >> +++ b/arch/arm/boot/dts/imx6qdl.dtsi >> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */ >> }; >> >> src: reset-controller@20d8000 { >> - compatible = "fsl,imx6q-src", "fsl,imx51-src"; >> + compatible = "fsl,imx6q-src", "fsl,imx51-src", >> + "syscon", "simple-mfd"; > > You need children here. Otherwise simple-mfd does not make sense. If you > expect something else to add children (I don't understand why, usually > reboot capability is fixed per SoC and only sometimes extended with some > other means), then this "else" will also change compatible. It's about syscon-reboot-mode, not syscon-reboot. Such modes are board- not soc-specific. Yes, a board DTS can override a compatible, but this is error prone as you can't just add a compatible, you need to hardcode the other compatibles in the SoC dtsi, which may change for good reason in future. barebox supports fixing up syscon-reboot-mode nodes defined in its own device tree to the Linux DT. While in theory, it could check if the parent node contains syscon/simple-mfd and fix it up if necessary, it sounds to me like this should be rather part of the upstream DT. Cheers, Ahmad > > Best regards, > Krzysztof > >
On 16/01/2023 09:27, Ahmad Fatoum wrote: > Hello Krzysztof, > > On 16.01.23 09:20, Krzysztof Kozlowski wrote: >> On 13/01/2023 18:32, Bastian Krause wrote: >>> The actual syscon-reboot-mode child node can be added by a board >>> device-tree or fixed up by the bootloader. For the child node to be >>> probed, the compatible needs to include simple-mfd. The binding now >>> specifies this, so have the SoC dtsi adhere to it. >>> >>> Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >>> Signed-off-by: Bastian Krause <bst@pengutronix.de> >>> --- >>> arch/arm/boot/dts/imx6qdl.dtsi | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi >>> index ff1e0173b39be..b16be39458aa6 100644 >>> --- a/arch/arm/boot/dts/imx6qdl.dtsi >>> +++ b/arch/arm/boot/dts/imx6qdl.dtsi >>> @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */ >>> }; >>> >>> src: reset-controller@20d8000 { >>> - compatible = "fsl,imx6q-src", "fsl,imx51-src"; >>> + compatible = "fsl,imx6q-src", "fsl,imx51-src", >>> + "syscon", "simple-mfd"; >> >> You need children here. Otherwise simple-mfd does not make sense. If you >> expect something else to add children (I don't understand why, usually >> reboot capability is fixed per SoC and only sometimes extended with some >> other means), then this "else" will also change compatible. > > It's about syscon-reboot-mode, not syscon-reboot. Such modes are board- > not soc-specific. syscon-reboot-mode is also mostly SoC specific. What exactly would differ on different boards? Register offsets of SoC component? Register values used by SoC power management unit? Anyway, the binding is then not correct - it does not allow syscon-reboot-mode child. I'll comment there. Best regards, Krzysztof
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi index ff1e0173b39be..b16be39458aa6 100644 --- a/arch/arm/boot/dts/imx6qdl.dtsi +++ b/arch/arm/boot/dts/imx6qdl.dtsi @@ -865,7 +865,8 @@ epit2: epit@20d4000 { /* EPIT2 */ }; src: reset-controller@20d8000 { - compatible = "fsl,imx6q-src", "fsl,imx51-src"; + compatible = "fsl,imx6q-src", "fsl,imx51-src", + "syscon", "simple-mfd"; reg = <0x020d8000 0x4000>; interrupts = <0 91 IRQ_TYPE_LEVEL_HIGH>, <0 96 IRQ_TYPE_LEVEL_HIGH>;
The actual syscon-reboot-mode child node can be added by a board device-tree or fixed up by the bootloader. For the child node to be probed, the compatible needs to include simple-mfd. The binding now specifies this, so have the SoC dtsi adhere to it. Suggested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> Signed-off-by: Bastian Krause <bst@pengutronix.de> --- arch/arm/boot/dts/imx6qdl.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)