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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] All thermal zones define high-temperature trip points as `passive` instead of `critical` and lack `cooling-maps`, disabling thermal emergency shutdowns.
- [Medium] The CPU idle state is broken due to a mismatch between its compatible string and how it is referenced, and CPUs lack the `cpu-idle-states` property.
- [Low] All SPI controller nodes (e.g., `spi0`, `spi1`, `spi14`) are missing `#address-cells = <1>;` and `#size-cells = <0>;`.
- [Low] Required `clocks` and `clock-names` properties are missing from various hardware block nodes (GENI QUP SEs, UFS, Crypto).
--

--- Patch [1]: [PATCH v11 1/6] arm64: dts: qcom: Add device tree for Nord SoC series ---
commit 3b03ce598a0e38b99b378a0b40cf86676c569565
Author: Shawn Guo <[email protected]>
Subject: arm64: dts: qcom: Add device tree for Nord SoC series

This commit adds the base device tree include for the Nord SoC series.
It describes core hardware components like CPUs, PSCI power management,
interrupt controllers, TLMM, thermal sensors, and SMMUs.

diff --git a/arch/arm64/boot/dts/qcom/nord.dtsi b/arch/arm64/boot/dts/qcom/nord.dtsi
index 0000000000000..6373d2e65f24d 100644
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/nord.dtsi
[ ... ]
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "qcom,oryon-1-5";
> +			reg = <0x0 0x0>;
> +			enable-method = "psci";
> +			power-domains = <&cpu0_pd>;
> +			power-domain-names = "psci";
> +			next-level-cache = <&l2_0>;
> +			clocks = <&cpu_perf 0>;
> +			#cooling-cells = <2>;
[ ... ]
> +		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: Medium]
Since core_off_c4 uses the arm,idle-state compatible, shouldn't it be
referenced directly via the cpu-idle-states property in the cpu nodes rather
than domain-idle-states in the power domains? 

The genpd framework requires nodes in domain-idle-states to have the
domain-idle-state compatible, so it appears this idle state might not be
discovered by either the cpuidle or genpd frameworks.

[ ... ]
> +			spi14: spi@880000 {
> +				compatible = "qcom,geni-spi";
> +				reg = <0x0 0x00880000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};

[Severity: Low]
Do these SPI controller nodes need #address-cells = <1>; and
#size-cells = <0>; properties? 

Without them, the device tree compiler and kernel might fail to parse the
child addresses correctly if a board device tree adds child devices with reg
properties.

[ ... ]
> +		ufs_mem_hc: ufshc@1d44000 {
> +			compatible = "qcom,nord-ufshc",
> +				     "qcom,ufshc",
> +				     "jedec,ufs-2.0";
> +			reg = <0x0 0x01d44000 0x0 0x3000>,
> +			      <0x0 0x01d60000 0x0 0x15000>;
> +			reg-names = "std",
> +				    "mcq";
> +			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
> +			lanes-per-direction = <2>;
> +			iommus = <&apps_smmu_0 0x14c0 0x0>;
> +			dma-coherent;
> +			msi-parent = <&gic_its 0x14c0>;
> +
> +			status = "disabled";
> +		};

[Severity: Low]
Are the mandatory clocks and clock-names properties missing for this block?

Bindings for peripherals like qcom,ufshc, qcom,geni-uart, and qcom,qce
typically require these. If they are omitted here, drivers might fail to
probe when enabled by a board file.

[ ... ]
> +	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: Critical]
Could this highest temperature trip point lead to thermal runaway? 

The trip-point1 is configured as type = "passive" without any cooling-maps,
which means the OS will not throttle or perform an emergency shutdown at 115C.

Furthermore, since there is no critical trip point defined, it looks like the
qcom,tsens-v2 hardware driver won't program the hardware critical threshold
either. 

Should this trip point be changed to critical to enable graceful OS
shutdowns and hardware-level protections?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1