Re: [PATCH v9 02/11] arm64: dts: qcom: ipq5210: Introduce RDP504 common DT file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] arm64: dts: qcom: ipq5210: missing ssphy_0 pipe clock phandle
  in gcc
- [Low] arm64: dts: qcom: duplicated sdhc properties in
  ipq5210-rdp504.dts
- [Low] arm64: dts: qcom: commit message mismatch on QPIC BAM and file
  rename
--

commit feb4d109494ac58f48292db37ef205aa3dd5e3c3
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

arm64: dts: qcom: ipq5210: Introduce RDP504 common DT file

This commit factors common board nodes out of ipq5210-rdp504.dts into
ipq5210-rdp504-common.dtsi and adds USB, PCIe, and QPIC SPI NAND nodes
for the RDP504 board.

> RDP504 has USB, PCIe and SPI NAND devices populated. EMMC and NAND
> share the GPIOs. The board has to be reworked to change from EMMC
> to NAND variant. Rename ipq5210-rdp504.dts as ipq5210-rdp504-common.dtsi
> and include it in variant specific DTS file.
>
> Describe the fixed USB regulators, PCIe reset GPIOs, QPIC SPI NAND flash
> and required pinctrl states. Enable QPIC BAM, USB and PCIe for the board.

[Severity: Low]
Is the commit message accurate about enabling QPIC BAM and renaming
ipq5210-rdp504.dts?

Neither ipq5210-rdp504-common.dtsi nor ipq5210-rdp504.dts references or
enables qpic_bam, which was already set to status = "okay" unconditionally
in arch/arm64/boot/dts/qcom/ipq5210.dtsi.  Also, ipq5210-rdp504.dts is not
renamed; it is kept as the eMMC variant DTS while shared nodes are moved to
ipq5210-rdp504-common.dtsi.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210-rdp504-common.dtsi b/arch/arm64/boot/dts/qcom/ipq5210-rdp504-common.dtsi
> new file mode 100644
> index 0000000000000..6fb8666ffbfda
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/ipq5210-rdp504-common.dtsi
[ ... ]
> +&sdhc {
> +	max-frequency = <192000000>;
> +	bus-width = <4>;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	pinctrl-0 = <&sdhc_default_state>;
> +	pinctrl-names = "default";
> +};
> +
> +&sleep_clk {
> +	clock-frequency = <32000>;
> +};
> +
> +&ssphy_0 {
> +	vdda-pll-supply = <&vreg_misc_1p8>;
> +	vdda-phy-supply = <&vreg_misc_0p925>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Does enabling ssphy_0 leave gcc_usb0_pipe_clk_src and gcc_usb0_pipe_clk
orphaned because gcc has <0> instead of <&ssphy_0> at clock index 4 in
ipq5210.dtsi?

When ssphy_0 probes, qmp_usb_probe() registers the 125 MHz usb0_pipe_clk
provider and requests <&gcc GCC_USB0_PIPE_CLK>.  In
drivers/clk/qcom/gcc-ipq5210.c, gcc_usb0_pipe_clk_src resolves its parent
solely through .parent_data = { .index = DT_USB3_PHY0_CC_PIPE_CLK } (index
4), but commit "arm64: dts: qcom: ipq5210: Add nodes for multiple
peripherals" left index 4 of gcc clocks as <0>:

arch/arm64/boot/dts/qcom/ipq5210.dtsi:
		gcc: clock-controller@1800000 {
			...
			clocks = <&xo_board>,
				 <&sleep_clk>,
				 <&pcie0_phy>,
				 <&pcie1_phy>,
				 <0>,
				 <0>;

Because index 4 is <0>, of_parse_clkspec() returns -ENOENT when resolving
gcc_usb0_pipe_clk_src's parent, leaving gcc_usb0_pipe_clk_src and
gcc_usb0_pipe_clk permanently orphaned with a reported rate of 0 Hz and
never preparing or enabling usb0_pipe_clk as their parent.

[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> index 519307c6b61e5..43cdb981a4baa 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq5210-rdp504.dts
> @@ -5,30 +5,11 @@
>  
>  /dts-v1/;
>  
> -#include "ipq5210.dtsi"
> +#include "ipq5210-rdp504-common.dtsi"
>  
>  / {
> -	model = "Qualcomm Technologies, Inc. IPQ5210 RDP504";
> +	model = "Qualcomm Technologies, Inc. IPQ5210 RDP504 (eMMC)";
>  	compatible = "qcom,ipq5210-rdp504", "qcom,ipq5210";
> -
> -	aliases {
> -		serial0 = &uart1;
> -	};
> -
> -	chosen {
> -		stdout-path = "serial0";
> -	};
> -};
> -
> -/*
> - * 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>;
>  };
>  
>  &sdhc {
> @@ -41,59 +22,3 @@ &sdhc {
>  
>  	status = "okay";
>  };

[Severity: Low]
This isn't a bug, but &sdhc in ipq5210-rdp504.dts repeats max-frequency,
bus-width, mmc-ddr-1_8v, mmc-hs200-1_8v, pinctrl-0, and pinctrl-names,
which are already defined in &sdhc in ipq5210-rdp504-common.dtsi.  Can
ipq5210-rdp504.dts just set status = "okay" on &sdhc?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-08-dts-v9-0-c34de4f78fbb@oss.qualcomm.com?part=2