Re: [PATCH v9 1/6] arm64: dts: qcom: Add device tree for Nord SoC series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The CPU idle state `core_off_c4` is incorrectly assigned as a `domain-idle-states` in the CPU power domains instead of `cpu-idle-states` in the CPU nodes.
- [High] All thermal zones lack a 'critical' trip point, disabling the OS's emergency thermal shutdown mechanism.
- [Low] The GENI SPI controller nodes are missing `#address-cells` and `#size-cells` properties.
- [Low] Missing `spi9` and `spi10` nodes in the `qupv3_1` GENI wrapper.
- [Low] Use of an invalid property name `#qcom,sensors` in the TSENS nodes.
--

--- Patch [1]: [PATCH v9 1/6] arm64: dts: qcom: Add device tree for Nord SoC series ---
commit 0e9fe50e315b6e84c6b74e06788fe6d49d6b2710
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

arm64: dts: qcom: Add device tree for Nord SoC series

Adds the base device tree include (nord.dtsi) for the Nord SoC series,
describing the core hardware components including Oryon cores, interrupt
controllers, pinctrl, thermal sensors, SMMUs, and QUP blocks.

> diff --git a/arch/arm64/boot/dts/qcom/nord.dtsi b/arch/arm64/boot/dts/qcom/nord.dtsi
> new file mode 100644
> index 0000000000000..6373d2e65f24d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/nord.dtsi

[ ... ]

> +		idle-states {
> +			entry-method = "psci";
> +
> +			core_off_c4: cluster-c4 {
> +				compatible = "arm,idle-state";
> +				idle-state-name = "retention";
> +				entry-latency-us = <93>;
> +				exit-latency-us = <129>;
> +				min-residency-us = <560>;
> +				arm,psci-suspend-param = <0x00000003>;
> +			};
> +		};

[ ... ]

> +	psci {
> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +
> +		cpu0_pd: power-domain-cpu0 {
> +			#power-domain-cells = <0>;
> +			power-domains = <&cluster0_pd>;
> +			domain-idle-states = <&core_off_c4>;

[Severity: High]
Will the generic power domain (genpd) framework fail to parse these CPU power
domains? Since core_off_c4 is defined as compatible = "arm,idle-state" rather
than "domain-idle-state", it looks like this miswires the CPU idle state as a
domain idle state. Should this be in cpu-idle-states under the CPU node
instead?

[ ... ]

> +		tsens0: thermal-sensor@c22c000 {
> +			compatible = "qcom,nord-tsens",
> +				     "qcom,tsens-v2";
> +			reg = <0x0 0x0c22c000 0x0 0x1000>,
> +			      <0x0 0x0c222000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 771 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 484 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "uplow",
> +					  "critical";
> +			#qcom,sensors = <16>;

[Severity: Low]
Does this property name violate device tree naming conventions? Since the hash
character is typically reserved for cell count specifiers like #address-cells,
should this be named qcom,sensors without the prefix so that tools and drivers
can parse it correctly?

> +			#thermal-sensor-cells = <1>;
> +		};

[ ... ]

> +		qupv3_2: geniqup@8c0000 {
> +			compatible = "qcom,geni-se-qup";
> +			reg = <0x0 0x008c0000 0x0 0x2000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			iommus = <&apps_smmu_0 0x15a3 0x0>;
> +			ranges;
> +
> +			i2c14: i2c@880000 {
> +				compatible = "qcom,geni-i2c";
> +				reg = <0x0 0x00880000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				status = "disabled";
> +			};
> +
> +			spi14: spi@880000 {
> +				compatible = "qcom,geni-spi";
> +				reg = <0x0 0x00880000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};

[Severity: Low]
Are these SPI controller nodes missing #address-cells and #size-cells
properties? Without them, won't board files encounter compilation errors when
they attempt to add child devices to the SPI bus?

[ ... ]

> +		qupv3_1: geniqup@ac0000 {
> +			compatible = "qcom,geni-se-qup";

[ ... ]

> +			i2c9: i2c@a88000 {
> +				compatible = "qcom,geni-i2c";
> +				reg = <0x0 0x00a88000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				status = "disabled";
> +			};
> +
> +			uart9: serial@a88000 {
> +				compatible = "qcom,geni-uart";
> +				reg = <0x0 0x00a88000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};

[Severity: Low]
Are the spi9 and spi10 nodes intentionally omitted from the qupv3_1 wrapper?
Board files won't be able to use the QUPv3 Serial Engine instances 9 and 10 in
SPI mode because their base definitions appear to be missing here.

[ ... ]

> +	thermal_zones: thermal-zones {
> +		ddr-0-0-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens0 0>;
> +
> +			trips {
> +				trip-point0 {
> +					temperature = <105000>;
> +					hysteresis = <10000>;
> +					type = "passive";
> +				};
> +
> +				trip-point1 {
> +					temperature = <115000>;
> +					hysteresis = <10000>;
> +					type = "passive";

[Severity: High]
Do these thermal zones need a trip point with type = "critical" rather than
just "passive"? Without a critical trip point, it looks like the Linux thermal
core will not invoke the emergency orderly_poweroff() when the hardware
severely overheats, and will rely instead entirely on abrupt hardware cutoffs.

> +				};
> +			};
> +		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806081554.448531-1-shengchao.guo@oss.qualcomm.com?part=1