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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt: qcom: ipq5210: missing USB3 PHY pipe clock link in gcc node
- [Medium] dt: qcom: ipq5210: missing cooling-maps for cpu1-thermal passive trip
- [Low] dt: qcom: ipq5210: comment and code mismatch regarding CPU thermal throttling limits
--

commit e1cb08a769aa7e73265d212c8410f86f91d91e54
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
arm64: dts: qcom: ipq5210: Add nodes for multiple peripherals

This commit adds common IPQ5210 SoC peripherals to the base DTSI, including
USB, PCIe, QPIC, TSENS, and thermal zones. It also describes the CPU OPP
table and CPU cooling cells.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 20e8b240642c6..0c03295f61b07 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
[ ... ]
> @@ -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>;

[Severity: Medium]
Should the fifth entry in this clocks array be linked to the USB3 PHY pipe 
clock (e.g., <&ssphy_0>)? 

The qcom,ipq5210-gcc binding defines the fifth clock entry as the USB3 PHY 
pipe clock source. Leaving it as <0> instead of linking to the newly added 
ssphy_0 node might result in a broken clock topology, potentially preventing 
the USB PHY and controller from initializing successfully.

[ ... ]
> +		cpu0-thermal {
> +			thermal-sensors = <&tsens 14>;
[ ... ]
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu0_thermal_alert>;
> +					/* Set maximum frequency as 816 MHz */
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,

[Severity: Low]
Is the comment accurate regarding the maximum frequency limit? 

The cooling-device property uses THERMAL_NO_LIMIT for both limits, which 
allows the full range of available cooling states instead of enforcing a 
strict 816 MHz hard-cap. If a strict cap was intended, should these limits 
be updated to reflect the specific restricted cooling state?

> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +
> +		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]
Does cpu1-thermal need a cooling-maps array for the passive trip point?

The cpu1-thermal zone defines a cpu-passive trip at 100000 mC but omits the 
cooling-maps block needed to bind cooling devices to it. In the Linux 
thermal framework, a passive trip point without associated cooling-maps 
takes no active mitigation (such as CPU frequency throttling). As a result, 
the system may take no action to prevent continued heating until it hits 
the critical shutdown threshold.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-08-dts-v7-0-77bd3d68d850@oss.qualcomm.com?part=1