Re: [PATCH RESEND v11 5/5] arm64: dts: qcom: Add SM7250 devicetree file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: qcom: sm7250: invalid cpu-map core numbering in cluster1/cluster2
- [Low] arm64: dts: qcom: sm7250: invalid empty ranges in timer@17c20000
- [Low] arm64: dts: qcom: sm7250: undocumented properties in gcc node
--

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

arm64: dts: qcom: Add SM7250 devicetree file

This commit adds the base devicetree file for the Qualcomm SM7250 SoC,
including CPU, PSCI, reserved memory, clock controller, interrupt controller,
RPMh, cpufreq, and timer nodes.

> diff --git a/arch/arm64/boot/dts/qcom/sm7250.dtsi b/arch/arm64/boot/dts/qcom/sm7250.dtsi
> new file mode 100644
> index 0000000000000..48480e6855f0c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm7250.dtsi
> @@ -0,0 +1,771 @@

[ ... ]

> +		cpu-map {
> +			cluster0 {
> +				core0 {
> +					cpu = <&cpu0>;
> +				};
> +
> +				core1 {
> +					cpu = <&cpu1>;
> +				};
> +
> +				core2 {
> +					cpu = <&cpu2>;
> +				};
> +
> +				core3 {
> +					cpu = <&cpu3>;
> +				};
> +
> +				core4 {
> +					cpu = <&cpu4>;
> +				};
> +
> +				core5 {
> +					cpu = <&cpu5>;
> +				};
> +			};
> +
> +			cluster1 {
> +				core6 {
> +					cpu = <&cpu6>;
> +				};
> +			};
> +
> +			cluster2 {
> +				core7 {
> +					cpu = <&cpu7>;
> +				};
> +			};
> +		};

[Severity: High]
Will parse_cluster() in drivers/base/arch_topology.c fail to find cpu6 and
cpu7 with this cpu-map node layout in arch/arm64/boot/dts/qcom/sm7250.dtsi?

During boot, init_cpu_topology() calls parse_dt_topology() -> parse_cluster(),
which looks up core subnodes inside each cluster sequentially starting from
core0:

drivers/base/arch_topology.c:parse_cluster() {
	/* Now check for cores */
	i = 0;
	do {
		snprintf(name, sizeof(name), "core%d", i);
		struct device_node *c __free(device_node) =
			of_get_child_by_name(cluster, name);

		if (!c)
			break;
}

Because cluster1 and cluster2 only contain core6 and core7 (with no core0),
of_get_child_by_name(cluster, "core0") returns NULL on the first iteration,
logging empty cluster warnings and never calling parse_core() ->
get_cpu_for_node() -> topology_parse_cpu_capacity() for cpu6 or cpu7.

Since parse_core() succeeds for cpu0..cpu5, raw_capacity is allocated with
entries for cpu0..cpu5 set to 1024 while raw_capacity[6] and raw_capacity[7]
remain 0. When parse_dt_topology() subsequently runs:

drivers/base/arch_topology.c:parse_dt_topology() {
	topology_normalize_cpu_scale();

	/*
	 * Check that all cores are in the topology; the SMP code will
	 * only mark cores described in the DT as possible.
	 */
	for_each_possible_cpu(cpu)
		if (cpu_topology[cpu].package_id < 0) {
			return -EINVAL;
		}
}

topology_normalize_cpu_scale() sets the scheduler capacity scale (cpu_scale)
for cpu6 and cpu7 to 0, and parse_dt_topology() returns -EINVAL because cpu6
and cpu7 still have package_id < 0, causing init_cpu_topology() to call
reset_cpu_topology() and discard the DT CPU topology.

Should core6 and core7 be placed under cluster0 (since all 8 cores share l3_0
and cluster_pd) or renamed to core0 within their respective clusters?

[ ... ]

> +	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,sm7250-gcc";
> +			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: Low]
Should clock-names and power-domains be dropped from the gcc
(clock-controller@100000) node in arch/arm64/boot/dts/qcom/sm7250.dtsi, or
documented in the binding schema?

In Documentation/devicetree/bindings/clock/qcom,sm6375-gcc.yaml (which covers
qcom,sm7250-gcc and references qcom,gcc.yaml), unevaluatedProperties is set to
false and neither clock-names nor power-domains is defined, causing dtbs_check
validation to fail on clock-controller@100000. In addition,
drivers/clk/qcom/gcc-sm7250.c resolves its parent clocks by DT index rather
than by name.

[ ... ]

> +		timer@17c20000 {
> +			compatible = "arm,armv7-timer-mem";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			reg = <0x0 0x17c20000 0x0 0x1000>;

[Severity: Low]
Does the empty ranges property in timer@17c20000 in
arch/arm64/boot/dts/qcom/sm7250.dtsi trigger dtc ranges_format warnings when
building a board dtb?

The parent node soc@0 uses #address-cells = <2> and #size-cells = <2>, while
timer@17c20000 uses #address-cells = <1> and #size-cells = <1> with an empty
ranges property. In scripts/dtc/checks.c, check_ranges_format() checks for
this mismatch:

scripts/dtc/checks.c:check_ranges_format() {
	if (prop->val.len == 0) {
		if (p_addr_cells != c_addr_cells)
			FAIL_PROP(c, dti, node, prop, "empty \"%s\" property but its "
				  "#address-cells (%d) differs from %s (%d)",
				  ranges, c_addr_cells, node->parent->fullpath,
				  p_addr_cells);
		if (p_size_cells != c_size_cells)
			FAIL_PROP(c, dti, node, prop, "empty \"%s\" property but its "
				  "#size-cells (%d) differs from %s (%d)",
				  ranges, c_size_cells, node->parent->fullpath,
				  p_size_cells);
	}
}

Should this use an explicit mapping such as ranges = <0 0 0 0x20000000>; as on
other Qualcomm SoCs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-master-v11-0-2c29e382162c@gmail.com?part=5