| Message ID | 20220629164414.301813-11-viorel.suman@oss.nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dt-bindings: arm: freescale: Switch fsl,scu from txt to yaml | expand |
On 29/06/2022 18:44, Viorel Suman (OSS) wrote: > From: Viorel Suman <viorel.suman@nxp.com> > > "clocks" and "clock-names" are not used the driver, so > remove them in order to match the yaml definition. So this explains the unexpected change in the bindings... but actually it does not explain whether it is correct or not. Just because driver does not use it, is not a proof that clocks are not there. In different OS/implementation this DTS might break stuff, so basically it is ABI break. DTS should describe the hardware fully, so if the clocks are there, should be in DTS regardless of the driver. Best regards, Krzysztof
On 22-06-29 20:04:43, Krzysztof Kozlowski wrote: > On 29/06/2022 18:44, Viorel Suman (OSS) wrote: > > From: Viorel Suman <viorel.suman@nxp.com> > > > > "clocks" and "clock-names" are not used the driver, so > > remove them in order to match the yaml definition. > > So this explains the unexpected change in the bindings... but actually > it does not explain whether it is correct or not. Just because driver > does not use it, is not a proof that clocks are not there. In different > OS/implementation this DTS might break stuff, so basically it is ABI > break. DTS should describe the hardware fully, so if the clocks are > there, should be in DTS regardless of the driver. Hi Krzysztof, Both XTAL clocks - 24MHz and 32kHz - are still defined in DTSI files, see for instance in arch/arm64/boot/dts/freescale/imx8qxp.dtsi : --------------- xtal32k: clock-xtal32k { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; clock-output-names = "xtal_32KHz"; }; xtal24m: clock-xtal24m { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <24000000>; clock-output-names = "xtal_24MHz"; }; --------------- Both can be seen in /sys/kernel/debug/clk/clk_summary once boot is complete, both can be referenced in any DTS node, so there is no ABI break. "DTS should describe the hardware fully" - this is true in case the OS is supposed to controll the hardware fully. i.MX8 System Controller Unit concept implies resources being allocated and managed by SCU, there is no direct OS access to some hardware. SCU actually defines the hardware environment the OS is being able to see and run within. SCU is able to define several such isolated hardware environments, each having its own OS running. So, in this particular case - i.MX8 SCU concept - DTS should describe the hardware from the perspective of the hardware environment exposed by SCU to OS. Best regards, Viorel
On 30/06/2022 10:36, Viorel Suman wrote: > On 22-06-29 20:04:43, Krzysztof Kozlowski wrote: >> On 29/06/2022 18:44, Viorel Suman (OSS) wrote: >>> From: Viorel Suman <viorel.suman@nxp.com> >>> >>> "clocks" and "clock-names" are not used the driver, so >>> remove them in order to match the yaml definition. >> >> So this explains the unexpected change in the bindings... but actually >> it does not explain whether it is correct or not. Just because driver >> does not use it, is not a proof that clocks are not there. In different >> OS/implementation this DTS might break stuff, so basically it is ABI >> break. DTS should describe the hardware fully, so if the clocks are >> there, should be in DTS regardless of the driver. > > Hi Krzysztof, > > Both XTAL clocks - 24MHz and 32kHz - are still defined in DTSI files, see for instance in > arch/arm64/boot/dts/freescale/imx8qxp.dtsi : > --------------- > xtal32k: clock-xtal32k { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <32768>; > clock-output-names = "xtal_32KHz"; > }; > > xtal24m: clock-xtal24m { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <24000000>; > clock-output-names = "xtal_24MHz"; > }; > --------------- > Both can be seen in /sys/kernel/debug/clk/clk_summary once boot is complete, both can be referenced > in any DTS node, so there is no ABI break. ABI break is not relevant to the fixed clocks being or not being defined in the DTS. You have a device which was taking the clock inputs, so the clocks stayed enabled. Now, you don't take these inputs, so for example the clocks are getting disabled as not used. > > "DTS should describe the hardware fully" - this is true in case the OS is supposed to controll the > hardware fully. i.MX8 System Controller Unit concept implies resources being allocated and managed > by SCU, there is no direct OS access to some hardware. SCU actually defines the hardware environment > the OS is being able to see and run within. SCU is able to define several such isolated hardware > environments, each having its own OS running. So, in this particular case - i.MX8 SCU concept - > DTS should describe the hardware from the perspective of the hardware environment exposed by SCU to > OS. OK, that sounds good, but the question about these clocks remain - are they inputs to the SCU or not. Regardless whether they are actual input or not, you used not appropriate argument here - that Linux OS implementation does not use them. The proper argument is - whether the hardware environment has them connected or not. Best regards, Krzysztof
On 22-06-30 20:01:31, Krzysztof Kozlowski wrote: > On 30/06/2022 10:36, Viorel Suman wrote: > > On 22-06-29 20:04:43, Krzysztof Kozlowski wrote: > >> On 29/06/2022 18:44, Viorel Suman (OSS) wrote: > >>> From: Viorel Suman <viorel.suman@nxp.com> > >>> > >>> "clocks" and "clock-names" are not used the driver, so > >>> remove them in order to match the yaml definition. > >> > >> So this explains the unexpected change in the bindings... but actually > >> it does not explain whether it is correct or not. Just because driver > >> does not use it, is not a proof that clocks are not there. In different > >> OS/implementation this DTS might break stuff, so basically it is ABI > >> break. DTS should describe the hardware fully, so if the clocks are > >> there, should be in DTS regardless of the driver. > > > > Hi Krzysztof, > > > > Both XTAL clocks - 24MHz and 32kHz - are still defined in DTSI files, see for instance in > > arch/arm64/boot/dts/freescale/imx8qxp.dtsi : > > --------------- > > xtal32k: clock-xtal32k { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <32768>; > > clock-output-names = "xtal_32KHz"; > > }; > > > > xtal24m: clock-xtal24m { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <24000000>; > > clock-output-names = "xtal_24MHz"; > > }; > > --------------- > > Both can be seen in /sys/kernel/debug/clk/clk_summary once boot is complete, both can be referenced > > in any DTS node, so there is no ABI break. > > ABI break is not relevant to the fixed clocks being or not being defined > in the DTS. You have a device which was taking the clock inputs, so the > clocks stayed enabled. > > Now, you don't take these inputs, so for example the clocks are getting > disabled as not used. Ok, thanks for the explanation. > > > > "DTS should describe the hardware fully" - this is true in case the OS is supposed to controll the > > hardware fully. i.MX8 System Controller Unit concept implies resources being allocated and managed > > by SCU, there is no direct OS access to some hardware. SCU actually defines the hardware environment > > the OS is being able to see and run within. SCU is able to define several such isolated hardware > > environments, each having its own OS running. So, in this particular case - i.MX8 SCU concept - > > DTS should describe the hardware from the perspective of the hardware environment exposed by SCU to > > OS. > > OK, that sounds good, but the question about these clocks remain - are > they inputs to the SCU or not. The question context looks a bit shifted. The "clocks" and "clock-names" attributes are removed from a clock provider device. The OS clock provider in this case is a client which uses some protocol to communicate with SCU via a messaging unit. There is no access to xtal clocks via the existing OS<->SCU communication protocol. > > Regardless whether they are actual input or not, you used not > appropriate argument here - that Linux OS implementation does not use > them. The proper argument is - whether the hardware environment has them > connected or not. Right, agreed. Regards, Viorel
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi index 483996a1f2d5..878c2aa663f1 100644 --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi @@ -218,8 +218,6 @@ pd: imx8qx-pd { clk: clock-controller { compatible = "fsl,imx8qxp-clk"; #clock-cells = <2>; - clocks = <&xtal32k &xtal24m>; - clock-names = "xtal_32KHz", "xtal_24Mhz"; }; iomuxc: pinctrl {