Re: [PATCH v2 1/2] arm64: dts: qcom: ipq5210: Add nodes for multiple peripherals

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing `phys` and `phy-names` properties in the PCIe root controller nodes.
- [High] The `gcc` node is missing the `#interconnect-cells = <1>;` property.
- [High] The `gcc` clock-controller node is missing the reference to the `ssphy_0` USB3 pipe clock provider.
- [Medium] Missing `cooling-maps` in `cpu1-thermal` zone.
- [Medium] Undocumented compatible strings and missing schema updates.
- [Low] Missing interconnect dt-bindings header file causes build failure.
--

--- Patch [1]: [PATCH v2 1/2] arm64: dts: qcom: ipq5210: Add nodes for multiple peripherals ---
commit 86962855e3cf6145af313fddfdeed7cd398a2a13
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

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

Describe common IPQ5210 SoC peripherals in the base DTSI so board DTS files
can enable the hardware they use.

Add nodes for USB, PCIe, QPIC NAND/BAM, QFPROM, TSENS and thermal zones.
Also describe the CPU OPP table and CPU cooling cells needed for CPU
frequency scaling and thermal throttling.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 2cdc10529c489..8fc8673ef6763 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -4,7 +4,9 @@
>   */
>  
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/qcom,apss-ipq.h>
>  #include <dt-bindings/clock/qcom,ipq5210-gcc.h>
> +#include <dt-bindings/interconnect/qcom,ipq5210.h>

[Severity: Low]
Will this cause a build failure? This patch includes
dt-bindings/interconnect/qcom,ipq5210.h, but it doesn't appear this header
exists in the tree yet.

>  #include <dt-bindings/reset/qcom,ipq5210-gcc.h>
>  
>  / {

[ ... ]

> @@ -130,6 +178,170 @@ soc@0 {
>  		dma-ranges = <0 0 0 0 0x10 0>;
>  		ranges = <0 0 0 0 0x10 0>;
>  

[ ... ]

> +		tsens: thermal-sensor@4a9000 {
> +			compatible = "qcom,ipq5210-tsens", "qcom,ipq5332-tsens";

[Severity: Medium]
Does this compatible string setup violate the qcom-tsens.yaml schema? The
schema restricts "qcom,ipq5332-tsens" within an enum block that prohibits
fallbacks, which causes dtbs_check to fail unconditionally here.
Additionally, qcom,ipq5210-tsens appears to be undocumented.

> +			reg = <0x0 0x004a9000 0x0 0x1000>,
> +			      <0x0 0x004a8000 0x0 0x1000>;

[ ... ]

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

[Severity: High]
Should the 5th item in this clocks array be updated to <&ssphy_0>? The
gcc-ipq5210 driver defines the USB3 PHY pipe clock source at parent index 4
(DT_USB3_PHY0_CC_PIPE_CLK). Leaving this as <0> will leave the pipe clock
without a parent and could break SuperSpeed USB operation.

>  			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;

[Severity: High]
Is #interconnect-cells = <1>; missing here? The pcie0 and pcie1 nodes declare
gcc as an interconnect provider. The generic OF interconnect parsing will fail
during the PCIe controller probe without this property.

>  		};
>  

[ ... ]

> @@ -243,6 +538,24 @@ watchdog@b017000 {
>  			clocks = <&sleep_clk>;
>  		};
>  
> +		apcs_glb: mailbox@b111000 {
> +			compatible = "qcom,ipq5210-apcs-apps-global",
> +				     "qcom,ipq6018-apcs-apps-global";
> +			reg = <0x0 0x0b111000 0x0 0x1000>;
> +			#clock-cells = <1>;
> +			clocks = <&a53pll>, <&xo_board>, <&gcc GPLL0>;
> +			clock-names = "pll", "xo", "gpll0";
> +			#mbox-cells = <1>;
> +		};
> +
> +		a53pll: clock@b116000 {
> +			compatible = "qcom,ipq5210-a53pll";

[Severity: Medium]
Is this compatible string documented in the YAML bindings? It appears to be
missing, which will cause dtbs_check to fail.

> +			reg = <0x0 0x0b116000 0x0 0x1000>;
> +			#clock-cells = <0>;
> +			clocks = <&xo_board>;
> +			clock-names = "xo";
> +		};
> +

[ ... ]

> +		pcie1: pcie@50000000 {
> +			compatible = "qcom,pcie-ipq5210", "qcom,pcie-ipq9574";

[ ... ]

> +			pcie1_port0: pcie@0 {
> +				device_type = "pci";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +				phys = <&pcie1_phy>;

[Severity: High]
Will the QCOM PCIe host controller driver find the PHY here? The driver uses
devm_phy_optional_get(dev, "pciephy") on the root PCIe device node. With the
phys property nested inside the child pcie@0 port node and lacking the
phy-names = "pciephy" property, the PHY initialization will silently fail.

Should phys and phy-names be moved up to the parent pcie1 node?

> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};
> +
> +		pcie0: pcie@70000000 {
> +			compatible = "qcom,pcie-ipq5210", "qcom,pcie-ipq9574";

[ ... ]

> +			pcie0_port0: pcie@0 {
> +				device_type = "pci";
> +				reg = <0x0 0x0 0x0 0x0 0x0>;
> +				bus-range = <0x01 0xff>;
> +				phys = <&pcie0_phy>;

[Severity: High]
Similar to pcie1, should this phys property be moved to the parent pcie0
node and accompanied by phy-names = "pciephy"? The PCIe interface won't
initialize its physical layer correctly otherwise.

> +
> +				#address-cells = <3>;
> +				#size-cells = <2>;
> +				ranges;
> +			};
> +		};
> +	};
> +
> +	thermal_zones: thermal-zones {

[ ... ]

> +		cpu1-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 15>;
> +
> +			trips {
> +				cpu-critical {
> +					temperature = <120000>;
> +					hysteresis = <9000>;
> +					type = "critical";
> +				};
> +
> +				cpu-passive {
> +					temperature = <110000>;
> +					hysteresis = <9000>;
> +					type = "passive";
> +				};

[Severity: Medium]
Is there a cooling-maps block missing for this passive trip point? If heat
is localized to CPU1, the thermal framework will reach this 110C trip point
but take no action to throttle the CPU because there are no cooling devices
mapped to it.

> +			};
> +		};
>  	};

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