[PATCH RFC 0/2] arm64: dts: qcom: move board clocks to DTS files

[PATCH RFC 0/2] arm64: dts: qcom: move board clocks to DTS files

[Dmitry Baryshkov]

Multiple Qualcomm platforms play strange tricks with board-level clocks
(XO, sleep) definitions. On some (older) platforms such clocks are
completely defined within SoC.dtsi file (although these clocks are not a
part of the SoC). On other platforms definitions of such clocks  are
split between the SoC dtsi file and the board file.
Such definitions don't fully follow the DT guidelines. Move these two
clocks to the board files completely.

Being an RFC this series converts two platforms as a way to raise the
question. However note, this is still not fully correct.

The XO clock is (usually) an external crystal used by the external PMIC,
which then provides RF CLK and LN BB CLK to the main SoC. However for
technical reasons this part of the PMIC is modelled as a part of the SoC
as RPM or RPMh clock controllers. It makes it impractical to describe XO
clock as being used or being connected to the PMIC.

Sleep clock is a 32 KHz or 32.768 kHz RC oscillator provided by one of
PMICs. However pushing it into the PMIC might interact badly with fw
devlink, causing unnecessary probe delays and/or devlink loops. One of
the possible solutions might be to move it to the corresponding
PMIC.dtsi, but model the clock outside of the PMIC node, providing
/clocks/sleep-clk node from that file.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (2):
      arm64: dts: qcom: sm8450: move board clocks to DTS files
      arm64: dts: qcom: sm8650: move board clocks to DTS files

 arch/arm64/boot/dts/qcom/sm8450-hdk.dts            | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/sm8450-qrd.dts            | 14 ++++++++++++++
 .../boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi   | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/sm8450.dtsi               | 14 --------------
 arch/arm64/boot/dts/qcom/sm8650-hdk.dts            | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts            | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts            | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650.dtsi               | 10 ----------
 8 files changed, 84 insertions(+), 48 deletions(-)
---
base-commit: 6d59cab07b8d74d0f0422b750038123334f6ecc2
change-id: 20241115-fix-board-clocks-e3afe520627c

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH RFC 1/2] arm64: dts: qcom: sm8450: move board clocks to DTS files

[Dmitry Baryshkov]

SM8450 is one of the platforms where board-level clocks (XO, sleep) are
fully defined in the SoC dtsi file. This is not correct and doesn't
fully follow the DT guidelines. Move these two clocks to the board files
completely.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts                 | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/sm8450-qrd.dts                 | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/sm8450.dtsi                    | 14 --------------
 4 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index 2ff40a120aadf9e7556bb568b5c99847584189fa..f50a6cbd4b6800a0236f1e5b4e0242c50472cbf3 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -59,6 +59,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	hdmi-out {
 		compatible = "hdmi-connector";
 		type = "a";
diff --git a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
index 8c39fbcaad802893870141582731ea8105994dd1..c330f019a4ed42ee8a0ead2cd39c6b1dc45cb1b7 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
@@ -28,6 +28,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	vph_pwr: vph-pwr-regulator {
 		compatible = "regulator-fixed";
 		regulator-name = "vph_pwr";
diff --git a/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi b/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
index cc1335a07a35c7a4239d684f978d958f7d47f533..135666c96493c65ed4d926df7396f170f9b36356 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
@@ -28,6 +28,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		label = "gpio-keys";
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 53147aa6f7e4acb102dd5dee51f0aec164b971c7..7dc183cc5c29374a19aabb36086e27edfffeaf37 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -33,20 +33,6 @@ / {
 
 	chosen { };
 
-	clocks {
-		xo_board: xo-board {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <76800000>;
-		};
-
-		sleep_clk: sleep-clk {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-			clock-frequency = <32000>;
-		};
-	};
-
 	cpus {
 		#address-cells = <2>;
 		#size-cells = <0>;

-- 
2.39.5

[PATCH RFC 2/2] arm64: dts: qcom: sm8650: move board clocks to DTS files

[Dmitry Baryshkov]

SM8650 is one of the platforms where board-level clocks (XO, sleep)
definitions are split between the SoC dtsi file and the board file.
This is not correct, as these two clocks are not a part of the SoC. Also
such definitions don't fully follow the DT guidelines. Move these two
clocks to the board files completely.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 22 ++++++++++++++--------
 arch/arm64/boot/dts/qcom/sm8650.dtsi    | 10 ----------
 4 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
index f00bdff4280af22f6f8b23e33238f53c602bc169..641bd817d75439bc6a050189565437e1c5ead3b5 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
@@ -30,6 +30,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	hdmi-out {
 		compatible = "hdmi-connector";
 		type = "a";
@@ -1112,10 +1126,6 @@ &sdhc_2 {
 	status = "okay";
 };
 
-&sleep_clk {
-	clock-frequency = <32000>;
-};
-
 &swr0 {
 	status = "okay";
 
@@ -1337,7 +1347,3 @@ &usb_dp_qmpphy {
 &usb_dp_qmpphy_out {
 	remote-endpoint = <&pmic_glink_ss_in>;
 };
-
-&xo_board {
-	clock-frequency = <76800000>;
-};
diff --git a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
index 0db2cb03f252d19a1f8b171a562a8e06290c8a21..26ab08715473683082023ba53bef7ff37e1fbb83 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
@@ -28,6 +28,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	pmic-glink {
 		compatible = "qcom,sm8650-pmic-glink",
 			     "qcom,sm8550-pmic-glink",
@@ -729,10 +743,6 @@ &sdhc_2 {
 	status = "okay";
 };
 
-&sleep_clk {
-	clock-frequency = <32000>;
-};
-
 &swr0 {
 	status = "okay";
 
@@ -884,7 +894,3 @@ &usb_dp_qmpphy {
 &usb_dp_qmpphy_out {
 	remote-endpoint = <&pmic_glink_ss_in>;
 };
-
-&xo_board {
-	clock-frequency = <76800000>;
-};
diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
index c5e8c3c2df91a49476f49832c0bd6418f4dfae3d..2a512d75d15d01b14a1eabda875f6d8fbe70a409 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -30,6 +30,20 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		xo_board: xo-board {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <76800000>;
+		};
+
+		sleep_clk: sleep-clk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32000>;
+		};
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 
@@ -1040,10 +1054,6 @@ &remoteproc_mpss {
 	status = "okay";
 };
 
-&sleep_clk {
-	clock-frequency = <32000>;
-};
-
 &spi4 {
 	status = "okay";
 
@@ -1320,7 +1330,3 @@ &usb_dp_qmpphy {
 &usb_dp_qmpphy_out {
 	remote-endpoint = <&redriver_ss_in>;
 };
-
-&xo_board {
-	clock-frequency = <76800000>;
-};
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 25e47505adcb790d09f1d2726386438487255824..b960d58210f8199e1f4e080ce50cd574314fa3e8 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -35,16 +35,6 @@ / {
 	chosen { };
 
 	clocks {
-		xo_board: xo-board {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-		};
-
-		sleep_clk: sleep-clk {
-			compatible = "fixed-clock";
-			#clock-cells = <0>;
-		};
-
 		bi_tcxo_div2: bi-tcxo-div2-clk {
 			compatible = "fixed-factor-clock";
 			#clock-cells = <0>;

-- 
2.39.5

Re: [PATCH RFC 1/2] arm64: dts: qcom: sm8450: move board clocks to DTS files

[Krzysztof Kozlowski]

On 15/11/2024 07:59, Dmitry Baryshkov wrote:
> SM8450 is one of the platforms where board-level clocks (XO, sleep) are
> fully defined in the SoC dtsi file. This is not correct and doesn't
> fully follow the DT guidelines. Move these two clocks to the board files
> completely.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8450-hdk.dts                 | 14 ++++++++++++++
>  arch/arm64/boot/dts/qcom/sm8450-qrd.dts                 | 14 ++++++++++++++
>  arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi | 14 ++++++++++++++
>  arch/arm64/boot/dts/qcom/sm8450.dtsi                    | 14 --------------
>  4 files changed, 42 insertions(+), 14 deletions(-)


Clocks could stay and only their frequency would be in DTS, but either
way is fine:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH RFC 2/2] arm64: dts: qcom: sm8650: move board clocks to DTS files

[Krzysztof Kozlowski]

On 15/11/2024 07:59, Dmitry Baryshkov wrote:
> SM8650 is one of the platforms where board-level clocks (XO, sleep)
> definitions are split between the SoC dtsi file and the board file.
> This is not correct, as these two clocks are not a part of the SoC. Also
> such definitions don't fully follow the DT guidelines. Move these two
> clocks to the board files completely.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 22 ++++++++++++++--------
>  arch/arm64/boot/dts/qcom/sm8650-mtp.dts | 22 ++++++++++++++--------
>  arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 22 ++++++++++++++--------
>  arch/arm64/boot/dts/qcom/sm8650.dtsi    | 10 ----------
>  4 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> index f00bdff4280af22f6f8b23e33238f53c602bc169..641bd817d75439bc6a050189565437e1c5ead3b5 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> @@ -30,6 +30,20 @@ chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
>  
> +	clocks {
> +		xo_board: xo-board {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <76800000>;
> +		};
> +
> +		sleep_clk: sleep-clk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <32000>;
> +		};
> +	};
> +
>  	hdmi-out {
>  		compatible = "hdmi-connector";
>  		type = "a";
> @@ -1112,10 +1126,6 @@ &sdhc_2 {
>  	status = "okay";
>  };
>  
> -&sleep_clk {
> -	clock-frequency = <32000>;
> -};

No, code has correct style. To avoid duplication, the SoC DTSI keeps the
skeleton of the clock, which also indicates that SoC expects it to be
provided by board. Then the board provides the clock by setting the
frequency.

Best regards,
Krzysztof

Re: [PATCH RFC 2/2] arm64: dts: qcom: sm8650: move board clocks to DTS files

[Dmitry Baryshkov]

On Sat, Nov 23, 2024 at 06:02:06PM +0100, Krzysztof Kozlowski wrote:
> On 15/11/2024 07:59, Dmitry Baryshkov wrote:
> > SM8650 is one of the platforms where board-level clocks (XO, sleep)
> > definitions are split between the SoC dtsi file and the board file.
> > This is not correct, as these two clocks are not a part of the SoC. Also
> > such definitions don't fully follow the DT guidelines. Move these two
> > clocks to the board files completely.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 22 ++++++++++++++--------
> >  arch/arm64/boot/dts/qcom/sm8650-mtp.dts | 22 ++++++++++++++--------
> >  arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 22 ++++++++++++++--------
> >  arch/arm64/boot/dts/qcom/sm8650.dtsi    | 10 ----------
> >  4 files changed, 42 insertions(+), 34 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> > index f00bdff4280af22f6f8b23e33238f53c602bc169..641bd817d75439bc6a050189565437e1c5ead3b5 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> > +++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
> > @@ -30,6 +30,20 @@ chosen {
> >  		stdout-path = "serial0:115200n8";
> >  	};
> >  
> > +	clocks {
> > +		xo_board: xo-board {
> > +			compatible = "fixed-clock";
> > +			#clock-cells = <0>;
> > +			clock-frequency = <76800000>;
> > +		};
> > +
> > +		sleep_clk: sleep-clk {
> > +			compatible = "fixed-clock";
> > +			#clock-cells = <0>;
> > +			clock-frequency = <32000>;
> > +		};
> > +	};
> > +
> >  	hdmi-out {
> >  		compatible = "hdmi-connector";
> >  		type = "a";
> > @@ -1112,10 +1126,6 @@ &sdhc_2 {
> >  	status = "okay";
> >  };
> >  
> > -&sleep_clk {
> > -	clock-frequency = <32000>;
> > -};
> 
> No, code has correct style. To avoid duplication, the SoC DTSI keeps the
> skeleton of the clock, which also indicates that SoC expects it to be
> provided by board. Then the board provides the clock by setting the
> frequency.

So, what is the decision maker: a deduplication or board vs soc
placement? From the duplication point of view we can keep the complete
clock in SoC.dtsi (like we did for older platforms or I did for
sar2130p). Most if not all boards use the same clock for the SoC family.
From the correctness point of view the clock should be completely in the
board DT, the clock is completely on the board and not on the SoC.
Having clock declaration in SoC.dtsi and patching the freq in board.dts
is a strange hack and mix of two solutions. For me as a reviewer and a
porter having such a split is a nightmare.

-- 
With best wishes
Dmitry

Re: [PATCH RFC 1/2] arm64: dts: qcom: sm8450: move board clocks to DTS files

[Tingwei Zhang]

On 11/15/2024 2:59 PM, Dmitry Baryshkov wrote:
> SM8450 is one of the platforms where board-level clocks (XO, sleep) are
> fully defined in the SoC dtsi file. This is not correct and doesn't
> fully follow the DT guidelines. Move these two clocks to the board files
> completely.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sm8450-hdk.dts                 | 14 ++++++++++++++
>   arch/arm64/boot/dts/qcom/sm8450-qrd.dts                 | 14 ++++++++++++++
>   arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi | 14 ++++++++++++++
>   arch/arm64/boot/dts/qcom/sm8450.dtsi                    | 14 --------------
>   4 files changed, 42 insertions(+), 14 deletions(-)
> 
<...>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 53147aa6f7e4acb102dd5dee51f0aec164b971c7..7dc183cc5c29374a19aabb36086e27edfffeaf37 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -33,20 +33,6 @@ / {
>   
>   	chosen { };
>   
> -	clocks {
> -		xo_board: xo-board {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <76800000>;
> -		};
> -
> -		sleep_clk: sleep-clk {
> -			compatible = "fixed-clock";
> -			#clock-cells = <0>;
> -			clock-frequency = <32000>;
> -		};
> -	};
> -
Sleep clock is moved to board device tree while reference of this clock 
is still in soc device tree like gcc node. Is this preferrable way to 
reference the node defined in board device in soc device tree?
>   	cpus {
>   		#address-cells = <2>;
>   		#size-cells = <0>;
> 


-- 
Thanks,
Tingwei