Re: [PATCH 20/20] arm64: dts: synaptics: Add Synaptics SL261X SoC and RDK board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dt: cpu0-supply is deprecated and breaks scaling on secondary cores
- [Low] dt: missing root compatible string in sl2619.dtsi
- [Low] dt: schema violation by nesting l2-cache inside cpu node
- [Low] dt: missing whitespace before opening brace in cpu-wrap node
--

commit 58d4f39b5f4d2e3ef035e2e85d0944a8e6331b18
Author: Jisheng Zhang <jszhang@kernel.org>

arm64: dts: synaptics: Add Synaptics SL261X SoC and RDK board

Add dts(i) files for the Synaptics SL261X SoCs and corresponding
RDK boards.

> diff --git a/arch/arm64/boot/dts/synaptics/sl2611.dtsi b/arch/arm64/boot/dts/synaptics/sl2611.dtsi
> new file mode 100644
> index 0000000000000..1d640ffe4c4b2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/synaptics/sl2611.dtsi
[ ... ]
> @@ -0,0 +1,490 @@
[ ... ]
> +		cpu0: cpu@0 {
> +			compatible = "arm,cortex-a55";
> +			device_type = "cpu";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +			cpu-idle-states = <&CPU_SLEEP_0>;
> +			i-cache-size = <32768>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <128>;
> +			d-cache-size = <32768>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			next-level-cache = <&L2_0>;
> +
> +			L2_0: l2-cache {
> +				compatible = "cache";

[Severity: Low]
Could this violate the standard Device Tree schema for CPU topology? The
l2-cache nodes shouldn't be instantiated as children of the cpu nodes. Can
they be moved as siblings under the /cpus node to avoid dtbs_check validation
errors?

[ ... ]
> +		chip: chip-control@f7e10000 {
> +			compatible = "syna,sl261x-chip-control", "syscon", "simple-mfd";
> +			reg = <0xf7e10000 0x10000>;
> +
> +			clk: clock {
> +				compatible = "syna,sl261x-soc-clk";
> +				#clock-cells = <1>;
> +				clocks = <&osc>;
> +			};
> +
> +			pinctrl: pin-controller {
> +				compatible = "syna,sl261x-soc-pinctrl";
> +			};
> +
> +			rst: reset {
> +				compatible = "syna,sl261x-soc-reset";
> +				#reset-cells = <1>;
> +			};
> +		};
> +
> +		cpu_wrap: cpu-wrap@f7e30000{

[Severity: Low]
Should there be a space before the opening brace for cpu-wrap@f7e30000? While
the Device Tree Compiler parses this successfully, it goes against standard
formatting conventions.

> +			compatible = "syna,sl261x-cpu-wrp", "syscon", "simple-mfd";
> +			reg = <0xf7e30000 0x4000>;
[ ... ]
> diff --git a/arch/arm64/boot/dts/synaptics/sl2615.dtsi b/arch/arm64/boot/dts/synaptics/sl2615.dtsi
> new file mode 100644
> index 0000000000000..11b1fe316aa6d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/synaptics/sl2615.dtsi
[ ... ]
> @@ -0,0 +1,46 @@
[ ... ]
> +	cpus {
> +		cpu1: cpu@100 {
> +			compatible = "arm,cortex-a55";
> +			device_type = "cpu";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +			cpu-idle-states = <&CPU_SLEEP_0>;
> +			i-cache-size = <32768>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <128>;
> +			d-cache-size = <32768>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			next-level-cache = <&L2_1>;
> +
> +			L2_1: l2-cache {
> +				compatible = "cache";

[Severity: Low]
Similar to sl2611.dtsi, does this violate the standard Device Tree schema for
CPU topology? The l2-cache node is instantiated as a child of the cpu node
here, and it should likely be moved to be a sibling under the /cpus node.

[ ... ]
> diff --git a/arch/arm64/boot/dts/synaptics/sl2619.dtsi b/arch/arm64/boot/dts/synaptics/sl2619.dtsi
> new file mode 100644
> index 0000000000000..96bd58bbed3e1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/synaptics/sl2619.dtsi
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (C) 2025 Synaptics Incorporated
> + *
> + * Author: Jisheng Zhang <jszhang@kernel.org>
> + */
> +
> +#include "sl2613.dtsi"
> +
> +/ {

[Severity: Low]
Would it make sense to include the root compatible string here? sl2619.dtsi
includes sl2613.dtsi but doesn't override the root compatible string to
syna,sl2619 (unlike sl2615.dtsi and sl2617.dtsi). Even though sl2619-rdk.dts
overrides it, this omission seems logically inconsistent.

> +	cpus {
> +		cpu1: cpu@100 {
> +			compatible = "arm,cortex-a55";
[ ... ]
> diff --git a/arch/arm64/boot/dts/synaptics/sl261x-rdk-common.dtsi b/arch/arm64/boot/dts/synaptics/sl261x-rdk-common.dtsi
> new file mode 100644
> index 0000000000000..78a9b28fdf99e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/synaptics/sl261x-rdk-common.dtsi
[ ... ]
> @@ -0,0 +1,135 @@
[ ... ]
> +	cpus {
> +		cpu@0 {
> +			cpu0-supply = <&vcore>;

[Severity: Medium]
Will this break voltage scaling for secondary cores? cpu0-supply is deprecated
in the cpus.yaml devicetree bindings in favor of cpu-supply.

Also, since this is hardcoded strictly under cpu@0 in the common board dtsi,
any secondary cores introduced by multi-core SoCs (like cpu1 in SL2615) will
inherit no regulator supply. Could this cause the cpufreq driver to fail to
configure voltage scaling for secondary cores?

> +		};
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929061417.20733-1-jszhang@kernel.org?part=20

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c