Re: [PATCH v8 01/11] arm64: dts: qcom: ipq5210: Add nodes for multiple peripherals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] arm64: dts: qcom: ipq5210: missing ssphy_0 pipe clock phandle
  in gcc clocks
- [Medium] arm64: dts: qcom: ipq5210: swapped pcie-mem and cpu-pcie
  interconnects in pcie0/1
- [Medium] arm64: dts: qcom: ipq5210: incomplete passive cooling setup
  in CPU thermal zones
- [Low] arm64: dts: qcom: ipq5210: qpic_bam enabled unconditionally in
  base SoC DTSI
- [Low] arm64: dts: qcom: ipq5210: stale 816 MHz comment in cpu0-thermal
  cooling-maps
--

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

arm64: dts: qcom: ipq5210: Add nodes for multiple peripherals

This commit adds device tree nodes for USB, PCIe, QPIC NAND/BAM, QFPROM,
TSENS, thermal zones, and CPU OPPs/cooling in ipq5210.dtsi.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 20e8b240642c6..81af32af4047d 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi

[ ... ]

> @@ -130,6 +179,170 @@ soc@0 {
>  		dma-ranges = <0 0 0 0 0x10 0>;
>  		ranges = <0 0 0 0 0x10 0>;
>  
> +		qusb_phy_0: phy@7b000 {
> +			compatible = "qcom,ipq5210-qusb2-phy", "qcom,ipq6018-qusb2-phy";
> +			reg = <0x0 0x0007b000 0x0 0x180>;
> +			#phy-cells = <0>;
> +
> +			clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> +				 <&xo_board>;
> +			clock-names = "cfg_ahb",
> +				      "ref";
> +
> +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +			status = "disabled";
> +		};
> +
> +		ssphy_0: phy@7d000 {
> +			compatible = "qcom,ipq5210-qmp-usb3-phy", "qcom,ipq9574-qmp-usb3-phy";
> +			reg = <0x0 0x0007d000 0x0 0xa00>;
> +			#phy-cells = <0>;
> +
> +			clocks = <&gcc GCC_USB0_AUX_CLK>,
> +				 <&xo_board>,
> +				 <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> +				 <&gcc GCC_USB0_PIPE_CLK>;
> +			clock-names = "aux",
> +				      "ref",
> +				      "cfg_ahb",
> +				      "pipe";
> +
> +			resets = <&gcc GCC_USB0_PHY_BCR>,
> +				 <&gcc GCC_USB3PHY_0_PHY_BCR>;
> +			reset-names = "phy",
> +				      "phy_phy";
> +
> +			#clock-cells = <0>;
> +			clock-output-names = "usb0_pipe_clk";
> +
> +			status = "disabled";
> +		};

[ ... ]

> @@ -146,8 +359,8 @@ gcc: clock-controller@1800000 {
>  			reg = <0x0 0x01800000 0x0 0x40000>;
>  			clocks = <&xo_board>,
>  				 <&sleep_clk>,
> -				 <0>,
> -				 <0>,
> +				 <&pcie0_phy>,
> +				 <&pcie1_phy>,
>  				 <0>,
>  				 <0>;
>  			#clock-cells = <1>;

[Severity: Medium]
Should the fifth entry in gcc's clocks property be <&ssphy_0> instead of
<0>?

In drivers/clk/qcom/gcc-ipq5210.c, gcc_usb0_pipe_clk_src resolves its sole
parent via .index = DT_USB3_PHY0_CC_PIPE_CLK (4) with no fallback name.
Leaving that slot as <0> keeps gcc_usb0_pipe_clk_src and its child
GCC_USB0_PIPE_CLK (consumed by ssphy_0) orphaned with a rate of 0 when
USB3 is enabled.

[ ... ]

> @@ -205,6 +418,88 @@ sdhc: mmc@7804000 {
>  			status = "disabled";
>  		};
>  
> +		qpic_bam: dma-controller@7984000 {
> +			compatible = "qcom,bam-v1.7.0";
> +			reg = <0x0 0x07984000 0x0 0x1c000>;
> +			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_QPIC_AHB_CLK>;
> +			clock-names = "bam_clk";
> +			#dma-cells = <1>;
> +			qcom,ee = <0>;
> +
> +			status = "okay";
> +		};

[Severity: Low]
Should qpic_bam have status = "disabled" here like its consumer qpic_nand,
so boards that do not use QPIC NAND (such as eMMC boards) do not enable
GCC_QPIC_AHB_CLK and probe the QPIC BAM controller unconditionally?

[ ... ]

> @@ -307,6 +620,287 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		pcie1: pcie@50000000 {

[ ... ]

> +			interconnects = <&gcc MASTER_CNOC_PCIE1 &gcc SLAVE_CNOC_PCIE1>,
> +					<&gcc MASTER_SNOC_PCIE1 &gcc SLAVE_SNOC_PCIE1>;
> +			interconnect-names = "pcie-mem", "cpu-pcie";

[ ... ]

> +		pcie0: pcie@70000000 {

[ ... ]

> +			interconnects = <&gcc MASTER_CNOC_PCIE0 &gcc SLAVE_CNOC_PCIE0>,
> +					<&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> +			interconnect-names = "pcie-mem", "cpu-pcie";

[Severity: Medium]
Are the "pcie-mem" and "cpu-pcie" interconnect paths swapped in both pcie1
and pcie0?

drivers/clk/qcom/gcc-ipq5210.c maps the CNOC pairs to the AXI slave clocks
and the SNOC pairs to the AXI master clocks:

drivers/clk/qcom/gcc-ipq5210.c:icc_ipq5210_hws[] = {
	{ MASTER_CNOC_PCIE0, SLAVE_CNOC_PCIE0, GCC_CNOC_PCIE0_1LANE_S_CLK },
	{ MASTER_CNOC_PCIE1, SLAVE_CNOC_PCIE1, GCC_CNOC_PCIE1_2LANE_S_CLK },
	{ MASTER_SNOC_PCIE0, SLAVE_SNOC_PCIE0, GCC_SNOC_PCIE0_AXI_M_CLK },
	{ MASTER_SNOC_PCIE1, SLAVE_SNOC_PCIE1, GCC_SNOC_PCIE1_AXI_M_CLK },
    ...
};

With CNOC bound to "pcie-mem" and SNOC bound to "cpu-pcie", pcie-qcom scales
the AXI slave clock to the PCIe link bandwidth while clamping the SNOC AXI
master (PCIe DMA to memory) clock to 1 KBps, and disables the SNOC AXI
master clock during non-S2RAM suspend when the link stays active.

[ ... ]

> +		cpu0-thermal {
> +			thermal-sensors = <&tsens 14>;
> +
> +			trips {
> +				cpu-critical {
> +					temperature = <115000>;
> +					hysteresis = <9000>;
> +					type = "critical";
> +				};
> +
> +				cpu0_thermal_alert: cpu-passive {
> +					temperature = <100000>;
> +					hysteresis = <9000>;
> +					type = "passive";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu0_thermal_alert>;
> +					/* Set maximum frequency as 816 MHz */
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;

[Severity: Low]
As reported by Konrad Dybcio on v4 [1], the "Set maximum frequency as
816 MHz" comment is stale now that cooling-device uses THERMAL_NO_LIMIT
rather than pinning state 3.

> +				};
> +			};
> +		};
> +
> +		cpu1-thermal {
> +			thermal-sensors = <&tsens 15>;
> +
> +			trips {
> +				cpu-critical {
> +					temperature = <115000>;
> +					hysteresis = <9000>;
> +					type = "critical";
> +				};
> +
> +				cpu-passive {
> +					temperature = <100000>;
> +					hysteresis = <9000>;
> +					type = "passive";
> +				};
> +			};
> +		};

[Severity: Medium]
Is cpu1-thermal missing a cooling-maps node for its cpu-passive trip, and
should both cpu0-thermal and cpu1-thermal set polling-delay-passive?

Without cooling-maps in cpu1-thermal, thermal_of_should_bind() binds no
cooling devices when TSENS sensor 15 crosses 100 C.  In addition, because
neither zone sets polling-delay-passive, crossing the single 100 C passive
trip advances the TSENS upper interrupt threshold to 115 C while step_wise
steps CPU frequency down by only one OPP (1416 MHz to 1200 MHz) with no
passive polling scheduled to step through the remaining OPPs.

[1] https://lore.kernel.org/r/11f0df89-8bd5-41ed-85a3-b7164123f8f7@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-08-dts-v8-0-d91401955039@oss.qualcomm.com?part=1