Re: [PATCH 1/5] arm64: dts: qcom: Add SM7250 base dts file

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The `timer@17c20000` node incorrectly configures `#address-cells` and `#size-cells`, leading to broken memory mappings.
- [High] The `spmi@c440000` bus node incorrectly sets `#address-cells` and `#size-cells`, breaking SPMI device enumeration.
- [Medium] Missing mandatory properties `cache-level` and `cache-unified` in L2 and L3 cache nodes.
- [Medium] The CPU topology `cpu-map` is entirely missing, breaking cluster-awareness and Energy Aware Scheduling.
- [Low] The SPMI node name `qcom,spmi@c440000` violates the standard generic node naming requirements.
- [Low] The memory-mapped `tcsr_mutex` is placed at the root of the device tree rather than within the `soc` simple-bus node.
--

--- Patch [1]: [PATCH 1/5] arm64: dts: qcom: Add SM7250 base dts file ---
commit 0271d54882ba31c5bab214b6f9e5a7997f5a44fd
Author: Sreeshankar K <[email protected]>

arm64: dts: qcom: Add SM7250 base dts file

This patch adds the base devicetree file for the Qualcomm SM7250 SoC and the
corresponding SM7250 MTP platform. It configures basic nodes like CPU, PSCI,
and serial console required for booting.

> diff --git a/arch/arm64/boot/dts/qcom/sm7250.dtsi b/arch/arm64/boot/dts/qcom/sm7250.dtsi
> new file mode 100644
> index 0000000000000..66b3705461f1e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm7250.dtsi
[ ... ]
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {

[Severity: Medium]
Is a cpu-map node missing here?

Without a cpu-map to define the physical and logical clusters, the kernel
will treat all cores as a single flat cluster. This degrades scheduling
decisions and prevents Energy Aware Scheduling from functioning on this
big.LITTLE SoC.

> +			device_type = "cpu";
> +			compatible = "qcom,kryo475";
> +			reg = <0x0 0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +			L2_0: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +				L3_0: l3-cache {
> +					compatible = "cache";
> +				};
> +			};

[Severity: Medium]
Should these cache nodes include the cache-level and cache-unified properties?

Without these mandatory properties, the kernel cache topology parser cannot
accurately identify or expose the cache layout to userspace via sysfs,
which prevents userspace applications from optimizing workloads based on
cache topology.

[ ... ]
> +	firmware {
> +		scm: scm {
> +			compatible = "qcom,scm-sm7250", "qcom,scm";
> +			#reset-cells = <1>;
> +		};
> +	};
> +
> +	tcsr_mutex: hwlock@1f40000 {
> +		compatible = "qcom,tcsr-mutex";
> +		reg = <0x0 0x01f40000 0x0 0x40000>;
> +		#hwlock-cells = <1>;
> +	};

[Severity: Low]
Should this memory-mapped peripheral be instantiated as a child of the soc
simple-bus node instead of the root node?

Placing it at the root bypasses the address translation layers (ranges,
dma-ranges) established by the soc node and misrepresents the hardware
bus hierarchy.

[ ... ]
> +		pdc: interrupt-controller@b220000 {
[ ... ]
> +		};
> +
> +		spmi: qcom,spmi@c440000 {

[Severity: Low]
Does this node name violate the standard generic node naming conventions?

The node name should ideally be generic, such as spmi@c440000. Including
the vendor prefix (qcom,) in the node name typically results in dtbs_check
validation errors against the spmi-pmic-arb schema.

> +			compatible = "qcom,spmi-pmic-arb";
> +			reg = <0x0 0x0c440000 0x0 0x0001100>,
> +			      <0x0 0x0c600000 0x0 0x2000000>,
> +			      <0x0 0x0e600000 0x0 0x0100000>,
> +			      <0x0 0x0e700000 0x0 0x00a0000>,
> +			      <0x0 0x0c40a000 0x0 0x0026000>;
> +			reg-names = "core", "chnls", "obsrvr", "intr", "cnfg";
> +			interrupt-names = "periph_irq";
> +			interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,ee = <0>;
> +			qcom,channel = <0>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;

[Severity: High]
Are these address and size cells correct for an SPMI bus?

The SPMI bus bindings require #address-cells = <2> (for the 8-bit slave ID
and 16-bit register offset) and #size-cells = <0>. Using <1> and <1> will
cause parsing of any child PMIC nodes to fail, breaking PMIC enumeration
and disabling dependent power domains.

[ ... ]
> +		timer@17c20000 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			compatible = "arm,armv7-timer-mem";
> +			reg = <0x0 0x17c20000 0x0 0x1000>;
> +			clock-frequency = <19200000>;
> +
> +			frame@17c21000 {
> +				frame-number = <0>;
> +				interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +					     <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +				reg = <0x0 0x17c21000 0x0 0x1000>,
> +				      <0x0 0x17c22000 0x0 0x1000>;

[Severity: High]
Will this configuration correctly parse the 64-bit addresses in the child
nodes?

The child frame nodes use 4 cells for their reg properties (e.g.,
<0x0 0x17c21000 0x0 0x1000>). Because this parent node sets #address-cells
and #size-cells to 1, the dt-parser will incorrectly split this single
64-bit region into distinct, erroneous memory regions. Attempting to map a
region starting at physical address 0x0 with a size of 0x17c21000 can lead
to a kernel panic during boot.

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