Re: [PATCH v7 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] All thermal zones lack a `type = "critical"` trip point for emergency shutdown.
- [Medium] The `dump_mem` reserved-memory node statically reserves 121MB but lacks `no-map`, a `compatible` string, and is orphaned.
- [Medium] Duplicate 'iommus' Stream ID assignment on both the BAM DMA controller and the Crypto engine.
- [Medium] Enabled GENI QUP wrapper nodes are missing their mandatory 'clocks' properties.
- [Low] Missing `#address-cells` and `#size-cells` properties in all SPI controller nodes.
--

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

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

This commit introduces the base device tree describing the core hardware
components of the Nord SoC series. It includes definitions for the CPUs,
interrupt controller, memory regions, and various peripheral interfaces.

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

[ ... ]

> +		dump_mem: mem-dump-region {
> +			alloc-ranges = <0x0 0x00000000 0x0 0xffffffff>;
> +			size = <0x0 0x79b0000>;
> +		};
> +	};

[Severity: Medium]
Does this node need a compatible string and a no-map or reusable property?
Since it dynamically reserves ~121MB of system memory using alloc-ranges and
size, but lacks no-map, the kernel maps it into the direct linear map but
permanently removes it from the page allocator. As there is no device node
claiming it, this effectively creates a boot-time memory leak.

[ ... ]

> +		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;

[Severity: Medium]
Since this wrapper node is active by default (lacking status = "disabled"),
does it need its clocks defined here? The qcom,geni-se-qup driver requires the
m-ahb and s-ahb clocks to probe successfully. Without them, the driver fails
to probe, which blocks any child node (I2C/SPI/UART) from probing even if a
board file enables them. This also applies to qupv3_0 and qupv3_1.

> +
> +			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]
Should the SPI controller nodes explicitly define #address-cells = <1> and
#size-cells = <0>? These controllers act as parent buses for SPI client
devices addressed via chip select numbers. Omitting these properties will
cause DTC warnings when child devices are attached in board files. This
pattern appears across all spi nodes within the three GENI SE QUP wrappers.

[ ... ]

> +		cryptobam: dma-controller@1dc4000 {
> +			compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> +			reg = <0x0 0x01dc4000 0x0 0x28000>;
> +			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
> +			#dma-cells = <1>;
> +			iommus = <&apps_smmu_0 0x1689 0>;
> +			qcom,ee = <0>;
> +			qcom,num-ees = <4>;
> +			num-channels = <20>;
> +			qcom,controlled-remotely;
> +		};
> +
> +		crypto: crypto@1dfa000 {
> +			compatible = "qcom,nord-qce", "qcom,sm8150-qce", "qcom,qce";
> +			reg = <0x0 0x01dfa000 0x0 0x6000>;
> +			dmas = <&cryptobam 4>, <&cryptobam 5>;
> +			dma-names = "rx", "tx";
> +			iommus = <&apps_smmu_0 0x1689 0>;
> +
> +			/* It stops nord-gearvm variant from booting to console */
> +			status = "disabled";
> +		};

[Severity: Medium]
Are cryptobam and crypto intended to share the exact same iommus Stream ID
(0x1689)? The ARM SMMU driver will fail to map the identical Stream ID to two
independent master devices in the device tree, causing an IOMMU group conflict
and a probe failure. While the crypto node is disabled to work around this,
enabling it on any board will re-trigger the failure.

[ ... ]

> +		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]
Does this thermal zone, along with all the other ones defined in this file,
need a type = "critical" trip point? By configuring the highest trip point
(e.g., 115000 or 125000) as "passive", the kernel will only attempt to
throttle the hardware. If passive cooling is insufficient, the OS will not
trigger an emergency shutdown, which could result in uncontrolled thermal
resets.

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