Re: [PATCH v12 6/6] arm64: dts: qcom: Add SM7250 devicetree file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] arm64: dts: qcom: sm7250: lmh_cluster1 cpu6 rejected by lmh_probe()
- [Medium] arm64: dts: qcom: sm7250: invalid PSCI parameters in domain-idle-states
- [Medium] clk: qcom: gcc-sm7250: out-of-bounds -EINVAL lookup for GCC_VIDEO_XO_CLK
- [Medium] arm64: dts: qcom: sm7250: non-page-aligned ipa_gsi_mem and gpu_mem regions
- [Low] arm64: dts: qcom: sm7250: child nodes under soc@0 not sorted by address
- [Low] arm64: dts: qcom: sm7250: unused dt-bindings/power/qcom-rpmpd.h include
--

commit 133903425754af1429d60a918bf4c66666da655c
Author: Sreeshankar K <sreeshankar0910@gmail.com>

arm64: dts: qcom: Add SM7250 devicetree file

This commit adds the base SoC devicetree file for Qualcomm SM7250, defining
CPU topology, PSCI idle states, reserved memory regions, clocks, interrupt
controllers, RPMh, cpufreq, LMh, and timers needed to boot to serial console.

> diff --git a/arch/arm64/boot/dts/qcom/sm7250.dtsi b/arch/arm64/boot/dts/qcom/sm7250.dtsi
> new file mode 100644
> index 0000000000000..02901f0b08a3f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm7250.dtsi
> @@ -0,0 +1,795 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2026, Sreeshankar K <sreeshankar0910@gmail.com>
> + */
> +
> +#include <dt-bindings/clock/qcom,rpmh.h>
> +#include <dt-bindings/clock/qcom,gcc-sm7250.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/power/qcom,rpmhpd.h>
> +#include <dt-bindings/power/qcom-rpmpd.h>

[Severity: Low]
As reported by Sashiko on v11 [1], is the dt-bindings/power/qcom-rpmpd.h
include needed here? SM7250 uses the RPMh power domain definitions from
dt-bindings/power/qcom,rpmhpd.h, and none of the legacy RPM definitions from
qcom-rpmpd.h are referenced in this file.

[ ... ]

> +		domain-idle-states {
> +			cluster_sleep_pc: cluster-sleep-0 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x40000004>;
> +				entry-latency-us = <2752>;
> +				exit-latency-us = <3048>;
> +				min-residency-us = <6118>;
> +			};
> +
> +			cluster_sleep_cx: cluster-sleep-1 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000224>;
> +				entry-latency-us = <3638>;
> +				exit-latency-us = <4562>;
> +				min-residency-us = <8467>;
> +			};
> +
> +			cluster_sleep_aoss: cluster-sleep-2 {
> +				compatible = "domain-idle-state";
> +				arm,psci-suspend-param = <0x41000c24>;
> +				entry-latency-us = <3263>;
> +				exit-latency-us = <6562>;
> +				min-residency-us = <9826>;
> +			};
> +		};

[Severity: Medium]
As reported by Filippo Falezza on v11 [2], are the arm,psci-suspend-param
values in cluster_sleep_pc, cluster_sleep_cx, and cluster_sleep_aoss encoded
as expected by psci_pd_power_off() and __psci_enter_domain_idle_state()?

When cluster_pd powers off in PSCI OSI mode, __psci_enter_domain_idle_state()
replaces the per-CPU state parameter with ds->state:

drivers/cpuidle/cpuidle-psci.c:__psci_enter_domain_idle_state() {
	...
	if (ds->state)
		state = ds->state;

	ret = psci_cpu_suspend_enter(state);
	...
}

Because ds->state replaces the per-CPU parameter rather than being ORed with
it, Qualcomm cluster domain-idle-state parameters encode the per-CPU rail
power collapse state (0x4) in bits [3:0], the cluster mode shifted left by
4 bits in bits [15:4] (0x004 << 4 = 0x0040, 0x224 << 4 = 0x2240, and
0xc24 << 4 = 0xc240), and affinity level 1 in bit 24 (0x01000000).

Here, cluster_sleep_pc uses 0x40000004 (which is the affinity-level-0 per-CPU
rail power collapse parameter used by little_cpu_sleep_1 and big_cpu_sleep_1),
and cluster_sleep_cx and cluster_sleep_aoss use unshifted 0x41000224 and
0x41000c24. Should these three parameters be 0x41000044, 0x41002244, and
0x4100c244 instead?

[ ... ]

> +		ipa_gsi_mem: ipa-gsi@8bc10000 {
> +			reg = <0x0 0x8bc10000 0x0 0x5400>;
> +			no-map;
> +		};
> +
> +		gpu_mem: gpu@8bc15400 {
> +			reg = <0x0 0x8bc15400 0x0 0x2000>;
> +			no-map;
> +		};

[Severity: Medium]
Can the non-page-aligned boundary between ipa_gsi_mem and gpu_mem cause page
mapping and PAS setup issues when loading the GPU zap shader?

Here ipa_gsi_mem has a size of 0x5400 (ending at 0x8bc153ff) and gpu_mem
starts at 0x8bc15400 (ending at 0x8bc173ff), so both no-map regions share the
4KB physical page at 0x8bc15000, while map_mem() and __free_memory_core()
round the start of the following System RAM region up via PAGE_ALIGN() /
PFN_UP() to 0x8bc18000, leaving the remaining 3KB at 0x8bc17400..0x8bc17fff
unmapped.

When adreno_zap_shader_load() -> zap_shader_load_mdt() in
drivers/gpu/drm/msm/adreno/adreno_gpu.c loads the GPU zap shader, it remaps
and initializes the region via:

	mem_region = memremap(mem_phys, mem_size, MEMREMAP_WC);
	...
	ret = qcom_mdt_load(dev, fw, fwname, pas_id,
			    mem_region, mem_phys, mem_size, NULL);

generic_ioremap_prot() aligns mem_phys (0x8bc15400) down to page boundary
0x8bc15000 (overlapping the tail of ipa_gsi_mem), and qcom_mdt_load() ->
__qcom_mdt_pas_init() -> qcom_pas_mem_setup() -> qcom_scm_pas_mem_setup()
passes the unaligned base address 0x8bc15400 to TrustZone PIL setup (similar
to the issue fixed on SM6350 in commit 44bcded2be4f ("arm64: dts: qcom:
sm6350: Fix ZAP region")). Should ipa_gsi_mem and gpu_mem be aligned to 4KB
page boundaries?

[ ... ]

> +	soc: soc@0 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges = <0 0 0 0 0x10 0>;
> +		dma-ranges = <0 0 0 0 0x10 0>;
> +
> +		gcc: clock-controller@100000 {
> +			compatible = "qcom,gcc-sm7250";
> +			reg = <0x0 0x00100000 0x0 0x1f0000>;
> +			clock-names = "bi_tcxo",
> +				      "bi_tcxo_ao",
> +				      "sleep_clk";
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&rpmhcc RPMH_CXO_CLK_A>,
> +				 <&sleep_clk>;
> +			power-domains = <&rpmhpd RPMHPD_CX>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};

[Severity: Medium]
Does instantiating qcom,gcc-sm7250 expose an out-of-bounds lookup for
GCC_VIDEO_XO_CLK in qcom_cc_clk_hw_get()?

In include/dt-bindings/clock/qcom,gcc-sm7250.h, GCC_VIDEO_XO_CLK is defined as
122, the highest index in the header:

	#define GCC_VIDEO_AHB_CLK					120
	#define GCC_VIDEO_AXI_CLK					121
	#define GCC_VIDEO_XO_CLK					122

In drivers/clk/qcom/gcc-sm7250.c, GCC_VIDEO_XO_CLK is enabled as a critical
clock in gcc_sm7250_critical_cbcrs[] and omitted from gcc_sm7250_clocks[],
where the highest designated index is [GCC_VIDEO_AXI_CLK] (121):

	[GCC_VIDEO_AXI_CLK] = &gcc_video_axi_clk.clkr,
	[GPLL0] = &gpll0.clkr,
	[GPLL0_OUT_EVEN] = &gpll0_out_even.clkr,
	[GPLL6] = &gpll6.clkr,
	[GPLL9] = &gpll9.clkr,
};

Because ARRAY_SIZE(gcc_sm7250_clocks) (stored in cc->num_rclks) is 122, any
consumer requesting <&gcc GCC_VIDEO_XO_CLK> hits idx >= cc->num_rclks in
drivers/clk/qcom/common.c:qcom_cc_clk_hw_get() and gets ERR_PTR(-EINVAL)
instead of NULL as returned for lower-indexed critical clocks such as
GCC_CAMERA_XO_CLK or GCC_DISP_XO_CLK.

[ ... ]

> +		cpufreq_hw: cpufreq@18323000 {
> +			compatible = "qcom,sm7250-cpufreq-hw", "qcom,cpufreq-hw";
> +			reg = <0 0x18323000 0 0x1000>,
> +			      <0 0x18325800 0 0x1000>,
> +			      <0 0x18327800 0 0x1000>;
> +			reg-names = "freq-domain0",
> +				    "freq-domain1",
> +				    "freq-domain2";
> +			interrupts-extended = <&lmh_cluster0 0>,
> +					      <&lmh_cluster1 0>;
> +			interrupt-names = "dcvsh-irq-0",
> +					  "dcvsh-irq-1";
> +			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
> +			clock-names = "xo", "alternate";
> +			#freq-domain-cells = <1>;
> +			#clock-cells = <1>;
> +		};
> +
> +		lmh_cluster0: lmh@18358800 {
> +			compatible = "qcom,sm7250-lmh", "qcom,sm8150-lmh";
> +			reg = <0x0 0x18358800 0x0 0x400>;
> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +			cpus = <&cpu0>;
> +			qcom,lmh-temp-arm-millicelsius = <60000>;
> +			qcom,lmh-temp-low-millicelsius = <84500>;
> +			qcom,lmh-temp-high-millicelsius = <85000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};
> +
> +		lmh_cluster1: lmh@18350800 {
> +			compatible = "qcom,sm7250-lmh", "qcom,sm8150-lmh";
> +			reg = <0x0 0x18350800 0x0 0x400>;
> +			interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +			cpus = <&cpu6>;
> +			qcom,lmh-temp-arm-millicelsius = <60000>;
> +			qcom,lmh-temp-low-millicelsius = <84500>;
> +			qcom,lmh-temp-high-millicelsius = <85000>;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +		};

[Severity: High]
Will lmh_cluster1 fail to probe and prevent cpufreq_hw from initializing when
cpus = <&cpu6> is passed to lmh_probe()?

In drivers/thermal/qcom/lmh.c, lmh_probe() resolves the CPU node via
of_cpu_node_to_id() and only accepts logical CPU IDs 0 and 4:

	if (cpu_id == 0) {
		node_id = LMH_CLUSTER0_NODE_ID;
	} else if (cpu_id == 4) {
		node_id = LMH_CLUSTER1_NODE_ID;
	} else {
		dev_err(dev, "Wrong CPU id associated with LMh node\n");
		return -EINVAL;
	}

Because cpu6 is logical CPU 6 on SM7250 (where cpu0..cpu5 are in frequency
domain 0 and cpu6 is the first big core in frequency domain 1), lmh_probe()
returns -EINVAL for lmh_cluster1 before calling irq_domain_create_linear().

Because cpufreq_hw references <&lmh_cluster1 0> in interrupts-extended and
lmh_cluster1 is enabled in DT without ever registering its IRQ domain,
fw_devlink blocks cpufreq_hw on the unmet interrupt supplier link; even if
cpufreq_hw probes, qcom_cpufreq_hw_cpu_init() -> qcom_cpufreq_hw_lmh_init() ->
platform_get_irq_optional() returns -EPROBE_DEFER for domain 1 (and also leaks
policy->freq_table and dynamic OPPs allocated earlier in
qcom_cpufreq_hw_read_lut()). In addition, because qcom_cpufreq_hw_driver is
registered at postcore_initcall while lmh_driver is registered at
device_initcall (and cpu0 has no interconnects property to defer
qcom_cpufreq_hw_driver_probe()), domain 0 also hits -EPROBE_DEFER during
cpufreq_register_driver() while domain 2 (cpu7, which has no interrupt entry
and gets -ENXIO) succeeds.

> +
> +		tcsr_mutex: hwlock@1f40000 {
> +			compatible = "qcom,tcsr-mutex";
> +			reg = <0x0 0x01f40000 0x0 0x40000>;
> +			#hwlock-cells = <1>;
> +		};
> +
> +		timer@17c20000 {

[Severity: Low]
This isn't a bug, but several child nodes under soc@0 are out of ascending
unit-address order: interrupt-controller@17a00000 is placed before
interrupt-controller@b220000 and spmi@c440000, lmh@18358800 is placed before
lmh@18350800, and hwlock@1f40000 and timer@17c20000 are placed at the end
after lmh@18350800. Could the child nodes under soc@0 be sorted by unit
address in ascending order to match the DTS coding style?

[1] https://lore.kernel.org/r/20261016042432.824-6-sreeshankar0910@gmail.com
[2] https://lore.kernel.org/r/20261030004903.1068342-1-effeffe99@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-master-v12-0-57fbe9eef978@gmail.com?part=6