Re: [PATCH v2 5/8] arm64: dts: qcom: Add device tree for Nord SoC series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The `ufs_mem_hc` node is missing the required `reg` property.
- [High] The CPU idle state `core_off_c4` is improperly wired to power domains instead of CPU nodes.
- [High] The `arch_timer` node incorrectly uses the legacy GICv2 `GIC_CPU_MASK_SIMPLE(8)` macro for its interrupts in a GICv3 system.
- [Medium] The `pdp_scmi` node provides fewer `shmem` entries than `mboxes`, which breaks channel assignments.
- [Medium] The `i2c15` and `spi15` nodes are missing their `compatible` properties.
- [Low] The `pdp0_a2p` shared memory node has a mismatched unit address in its name.
- [Low] The `l2-cache` nodes are incorrectly placed as nested children inside the `cpu` nodes.
--

commit e4aaf34e88835bde7e82aca29bb473de34bfe007
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

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

This commit adds the base device tree include (nord.dtsi) for the Nord SoC series.
It describes the core hardware components including CPUs, GICv3 interrupt
controller, TLMM, SMMU instances, QUPv3 blocks, SCMI, and timers.

> diff --git a/arch/arm64/boot/dts/qcom/nord.dtsi b/arch/arm64/boot/dts/qcom/nord.dtsi
> new file mode 100644
> index 0000000000000..b425a91a7890a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/nord.dtsi
> @@ -0,0 +1,4511 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	interrupt-parent = <&intc>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		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>;
> +
> +			l2_0: l2-cache {
> +				compatible = "cache";
> +				cache-level = <2>;
> +				cache-unified;
> +			};

Are these l2-cache nodes correctly placed?

The l2_0 (and other L2 cache) nodes are declared directly inside their
respective CPU nodes. The cpus.yaml schema forbids cache nodes as children
of a CPU node; they must be declared as peer nodes within the /cpus
hierarchy and referenced via next-level-cache phandles.

> +		};

[ ... ]

> +	firmware: firmware {
> +		scm {
> +			compatible = "qcom,scm-nord",
> +				     "qcom,scm";
> +			qcom,dload-mode = <&tcsr 0x79000>;
> +		};
> +
> +		pdp_scmi: scmi {
> +			compatible = "arm,scmi";
> +			mboxes = <&pdp0_mbox 0>,
> +				 <&pdp0_mbox 11>,
> +				 <&pdp0_mbox 1>;
> +			mbox-names = "tx",
> +				     "tx_reply",
> +				     "rx";
> +			shmem = <&pdp0_a2p>,
> +				<&pdp0_p2a>;

Does this configuration provide enough shmem entries for the defined mailboxes?

The SCMI node defines 3 mailboxes (tx, tx_reply, rx) but only 2 shmem
phandles. The SCMI driver maps shared memory regions to mailbox channels
sequentially.

This mismatch causes the tx_reply channel to erroneously bind to the p2a
buffer, and leaves the rx channel without any shared memory, which will
break SCMI notifications and split completions.

> +			#address-cells = <1>;
> +			#size-cells = <0>;

[ ... ]

> +		tme_crash_dump_mem: tme-crash-dump-region@87000000 {
> +			reg = <0x0 0x87000000 0x0 0xa0000>;
> +			no-map;
> +		};
> +
> +		pdp0_a2p: scmi-shmem@87060180 {
> +			compatible = "arm,scmi-shmem";
> +			reg = <0x0 0x87608180 0x0 0x80>;

Is there a typo in the unit address for this node?

The node is named scmi-shmem@87060180, but its reg property maps address
0x87608180. According to Device Tree specifications, the unit address in
the node name must exactly match the base address in the reg property.

> +			no-map;
> +		};

[ ... ]

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

Is core_off_c4 properly wired to this power domain?

The core_off_c4 idle state has compatible = "arm,idle-state", making it
a CPU idle state. However, it is assigned via the domain-idle-states
property inside CPU power domain nodes instead of the cpu-idle-states
property inside the CPU nodes. 

The domain-idle-states property only accepts targets with
compatible = "domain-idle-state". Due to this mismatch, the cpuidle driver
will not discover the CPU idle states, preventing the cores from entering
intended retention states.

> +
> +		cpu1_pd: power-domain-cpu1 {
> +			#power-domain-cells = <0>;

[ ... ]

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

[ ... ]

> +			uart14: serial@880000 {
> +				compatible = "qcom,geni-uart";
> +				reg = <0x0 0x00880000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 373 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c15: i2c@884000 {
> +				reg = <0x0 0x00884000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 583 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};
> +
> +			spi15: spi@884000 {
> +				reg = <0x0 0x00884000 0x0 0x4000>;
> +				interrupts = <GIC_SPI 583 IRQ_TYPE_LEVEL_HIGH>;
> +
> +				status = "disabled";
> +			};

Are these i2c15 and spi15 nodes missing their compatible strings?

They lack a compatible property (e.g., "qcom,geni-i2c"). Although
currently marked as disabled, any board device tree that sets them to okay
will find them completely non-functional because the kernel driver model
requires a compatible string to bind the correct device driver.

> +
> +			uart15: serial@884000 {
> +				compatible = "qcom,geni-debug-uart";

[ ... ]

> +		ufs_mem_hc: ufshc@1d44000 {
> +			compatible = "qcom,nord-ufshc",
> +				     "qcom,ufshc",
> +				     "jedec,ufs-2.0";
> +			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>;
> +		};

Is this node missing its reg property?

The ufshc@1d44000 node defines a unit address but omits the reg property
entirely. This will prevent the UFS host controller driver from mapping its
memory-mapped I/O registers during probe, rendering this hardware block
non-functional.

> +
> +		tcsr_mutex: hwlock@1f40000 {

[ ... ]

> +	arch_timer: timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
> +	};

Can the GIC_CPU_MASK_SIMPLE(8) macro be used safely here for a GICv3 system?

For an arm,gic-v3 interrupt controller, bits 8-15 of the third interrupt
cell specify the PPI partition number, not a CPU mask. Using
GIC_CPU_MASK_SIMPLE(8) resolves to (0xff << 8), setting the GICv3 PPI
partition number to an invalid 255. This will cause the architectural timer
interrupts to be misrouted or fail mapping altogether.

> +
> +	thermal_zones: thermal-zones {

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