Re: [PATCH 5/5] arm64: dts: qcom: ipq5210: Add NSS clock controller node

[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 405d44391cff203718eda5567f73dbca56529ae8
Author: Luo Jie <jie.luo@oss.qualcomm.com>

arm64: dts: qcom: ipq5210: Add NSS clock controller node

This commit adds the NSS clock controller (nsscc) device tree node to
ipq5210.dtsi to provide clocks and resets for the networking hardware blocks
on IPQ5210.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 0c1ede0f73b21..11915f8eb1567 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -331,6 +331,46 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		nsscc: clock-controller@39b00000 {
> +			compatible = "qcom,ipq5210-nsscc";
> +			reg = <0x0 0x39b00000 0x0 0x100000>;
> +			/*
> +			 * Note: CMN PLL output clock names below are reversed relative
> +			 * to the hardware block they feed - IPQ5210_PPE_CLK is wired to
> +			 * the NSS hardware block ("nss") and IPQ5210_NSS_CLK is wired
> +			 * to the PPE hardware block ("ppe").
> +			 */
> +			clocks = <&xo_board>,

[Severity: Medium]
Here nsscc consumes xo_board as its 24 MHz "xo" reference clock (expected by
ftbl_nss_cc_ppe_clk_src in drivers/clk/qcom/nsscc-ipq5210.c).

In arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts, the comments explain that the
board XO crystal (xo_clk) can be 48 MHz, 96 MHz, or 192 MHz, which is divided
down to a fixed 48 MHz WiFi output clock (ref_48mhz_clk), and xo_board is
routed from that 48 MHz WiFi output clock divided by 2:

/*
 * 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>;
};
...
/*
 * 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-div = <2>;
	clock-mult = <1>;
};

However, in arch/arm64/boot/dts/qcom/ipq5210.dtsi (updated in commit
6f35f463dd9a ("arm64: dts: qcom: ipq5210: Convert xo_board to fixed factor
clock")), xo_board is parented to <&xo_clk> instead of <&ref_48mhz_clk>:

		xo_board: xo-board-clk {
			compatible = "fixed-factor-clock";
			clocks = <&xo_clk>;
			#clock-cells = <0>;
		};

Unlike arch/arm64/boot/dts/qcom/ipq5424.dtsi (as well as ipq5332.dtsi and
ipq9574.dtsi), where xo_board uses clocks = <&ref_48mhz_clk>, parenting
xo_board directly to <&xo_clk> bypasses ref_48mhz_clk.

On IPQ5210 boards with a 96 MHz or 192 MHz xo_clk where xo_board uses
clock-div = <2>, wouldn't xo_board resolve to 48 MHz or 96 MHz instead of the
expected 24 MHz for nsscc, gcc, and sdhc? Should xo_board in ipq5210.dtsi have
clocks = <&ref_48mhz_clk> instead of <&xo_clk>?

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