Re: [PATCH 4/5] arm64: dts: qcom: Update IPQ5210 xo_board to use fixed factor clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] arm64: dts: qcom: ipq5210: xo_board uses xo_clk instead of
  ref_48mhz_clk
--

commit 6f35f463dd9af9e90cdf14e9597dae99779f246f
Author: Luo Jie <jie.luo@oss.qualcomm.com>

arm64: dts: qcom: Update IPQ5210 xo_board to use fixed factor clock

This commit converts the IPQ5210 xo_board clock from a fixed-clock to a
fixed-factor-clock dividing its parent clock by 2 to produce 24 MHz.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> index 61a53361138b1..519307c6b61e5 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> @@ -85,8 +85,13 @@ &uart1 {
>  	status = "okay";
>  };
>  
> +/*
> + * The frequency of xo_board is fixed to 24 MHz, which is routed
> + * from WiFi output clock 48 MHz divided by 2.
> + */
>  &xo_board {
> -	clock-frequency = <24000000>;
> +	clock-div = <2>;
> +	clock-mult = <1>;
>  };
>  
>  &xo_clk {
> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 52d851a0dc2b7..0c1ede0f73b21 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -26,7 +26,8 @@ sleep_clk: sleep-clk {
>  		};
>  
>  		xo_board: xo-board-clk {
> -			compatible = "fixed-clock";
> +			compatible = "fixed-factor-clock";
> +			clocks = <&xo_clk>;

[Severity: Medium]
Should xo_board use <&ref_48mhz_clk> as its parent clock instead of
<&xo_clk>?

Both the commit message and the comment in ipq5210-rdp504.dts state that
xo_board is fixed to 24 MHz and routed from the 48 MHz WiFi output clock
(which is also the reference clock of the CMN PLL) divided by 2.

In ipq5210.dtsi and ipq5210-rdp504.dts, xo_clk represents the external board
crystal (which can be 48 MHz, 96 MHz, or 192 MHz depending on board
bootstrap pins), whereas ref_48mhz_clk represents the 48 MHz WiFi output
clock routed to cmn_pll:

arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts:
/*
 * The bootstrap pins for the board select the XO clock frequency that
 * supports 48 MHz, 96 MHz or 192 MHz. This setting automatically
 * enables the right dividers, to ensure the reference clock output
 * from WiFi to the CMN PLL is 48 MHz.
 */
&ref_48mhz_clk {
	clock-div = <1>;
	clock-mult = <1>;
};

arch/arm64/boot/dts/qcom/ipq5210.dtsi:
		cmn_pll: clock-controller@9b000 {
			compatible = "qcom,ipq5210-cmn-pll";
			reg = <0x0 0x0009b000 0x0 0x800>;
			clocks = <&ref_48mhz_clk>,

With clocks = <&xo_clk> and a fixed factor of 1/2, wouldn't xo_board and its
downstream consumers (gcc, sdhc, and nsscc) evaluate to 48 MHz or 96 MHz
instead of 24 MHz on IPQ5210 boards strapped for a 96 MHz or 192 MHz xo_clk?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-qcom_ipq5210_nsscc-v1-0-d4fcb5c32b51@oss.qualcomm.com?part=4