Re: [PATCH v9 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: swapped pcie-mem and cpu-pcie
  interconnects in pcie0/1
- [Medium] arm64: dts: qcom: ipq5210: missing ssphy_0 pipe clock phandle
  in gcc clocks
- [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 d1ab8c6e1b0ebf5f28504c611ba26174cc9742a0
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

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

This commit adds IPQ5210 device tree nodes for USB, PCIe, QPIC NAND/BAM,
QFPROM, TSENS, thermal zones, and the CPU OPP table and cooling cells.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index c4a760c1ab23e..504364ca031c4 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi

[ ... ]

> @@ -171,8 +384,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 DT clock index DT_USB3_PHY0_CC_PIPE_CLK (4).  Leaving that slot
as <0> keeps gcc_usb0_pipe_clk_src and its child GCC_USB0_PIPE_CLK
orphaned with a rate of 0 when USB3 is enabled.

> @@ -230,6 +443,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 so boards only enable it
alongside qpic_nand?  Keeping it enabled in the base DTSI causes
bam_dma_probe() to enable GCC_QPIC_AHB_CLK, reset the QPIC BAM block, and
claim GIC_SPI 56 even on eMMC boards that do not use QPIC NAND.

[ ... ]

> @@ -332,6 +645,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:
	{ 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 CNOC AXI slave clock with PCIe link speed while clamping the
SNOC AXI master DMA clock to 1 KBps, and disables the SNOC AXI master DMA
clock via icc_disable(pcie->icc_cpu) 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
instead of 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
do both cpu0-thermal and cpu1-thermal need polling-delay-passive?

Without cooling-maps in cpu1-thermal, crossing 100 C on TSENS sensor 15
binds no cooling devices and triggers no throttling.  In addition, because
neither zone sets polling-delay-passive, crossing the single 100 C passive
trip in cpu0-thermal steps the CPU cooling state down by only one OPP
(1416 MHz to 1200 MHz) while TSENS advances its upper interrupt threshold
to 115 C, with no passive polling scheduled to step down further through
1008 MHz and 816 MHz.

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

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