Re: [PATCH v2 4/5] arm64: dts: exynos: Add initial support for Exynos5515 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] arm64: dts: exynos5515: undocumented and unsupported watchdog compatible
- [Medium] arm64: dts: exynos5515: pmu node uses software-model compatible
- [Medium] arm64: dts: exynos5515: missing clocks on sysreg_peri and sysreg_fsys
--

commit 004e20a5dc89e977ddee65c29861e06d78443621
Author: Aiden Isik <aidenisik@member.fsf.org>

arm64: dts: exynos: Add initial support for Exynos5515 SoC

This commit adds the initial device tree source for the Samsung Exynos 5515
wearable SoC, including nodes for the Cortex-A55 CPUs, PMU, ChipID, generic
timer, watchdog timer, and CMU_TOP, CMU_PERI, and CMU_FSYS clock units.

> diff --git a/arch/arm64/boot/dts/exynos/exynos5515.dtsi b/arch/arm64/boot/dts/exynos/exynos5515.dtsi
> new file mode 100644
> index 0000000000000..ce6fcbfffdb2e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos5515.dtsi
> @@ -0,0 +1,189 @@
[ ... ]
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu1: cpu@100 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a55";
> +			reg = <0x100>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	oscclk: osc-clock {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-output-names = "oscclk";
> +	};
> +
> +	pmu {
> +		compatible = "arm,armv8-pmuv3";
> +		interrupt-affinity = <&cpu0>,
> +				     <&cpu1>;
> +		interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
> +	};

[Severity: Medium]
Should the pmu node use compatible = "arm,cortex-a55-pmu" instead of
"arm,armv8-pmuv3"?

In Documentation/devicetree/bindings/arm/pmu.yaml, "arm,armv8-pmuv3" is
marked as only for software models:

          - arm,armv8-pmuv3 # Only for s/w models
          ...
          - arm,cortex-a55-pmu

When drivers/perf/arm_pmuv3.c matches "arm,armv8-pmuv3", it registers the
PMU with the generic name "armv8_pmuv3" instead of "armv8_cortex_a55",
preventing userspace perf tooling from identifying the Cortex-A55
microarchitectural events.

[ ... ]
> +		sysreg_peri: syscon@10020000 {
> +			compatible = "samsung,exynos5515-peri-sysreg", "syscon";
> +			reg = <0x10020000 0x10000>;
> +		};

[Severity: Medium]
Do sysreg_peri and sysreg_fsys need their clocks properties wired up to
CLK_GOUT_PERI_SYSREG_PCLK and CLK_GOUT_FSYS_SYSREG_PCLK?

In drivers/clk/samsung/clk-exynos5515.c, peri_gate_clks and fsys_gate_clks
register dedicated gate clocks for these sysreg blocks:

	/* SYSREG */
	GATE(CLK_GOUT_PERI_SYSREG_PCLK, "gout_peri_sysreg_pclk",
	     "mout_pll_peri_bus_user",
	     CLK_CON_GAT_CLK_BLK_PERI_UID_SYSREG_PERI_IPCLKPORT_PCLK,
	     21, 0, 0);

When cmu_peri and cmu_fsys look up samsung,sysreg via
samsung_cmu_register_one() -> samsung_en_dyn_root_clk_gating() ->
syscon_regmap_lookup_by_phandle() -> of_syscon_register(), the absence of
a clocks property means regmap_mmio_attach_clk() is not called, leaving
the SYSREG PCLK unprepared and unenabled during sysreg MMIO accesses.

In addition, Documentation/devicetree/bindings/soc/samsung/samsung,exynos-sysreg.yaml
currently omits samsung,exynos5515-peri-sysreg and
samsung,exynos5515-fsys-sysreg from the allOf conditional block that
requires clocks, causing the else branch (clocks: false) to apply.

> +
> +		cmu_peri: clock-controller@10030000 {
> +			compatible = "samsung,exynos5515-cmu-peri";
> +			reg = <0x10030000 0x10000>;
> +			#clock-cells = <1>;
> +			clocks = <&oscclk>,
> +				 <&cmu_top CLK_DOUT_CMU_PERI_BUS>,
> +				 <&cmu_top CLK_DOUT_CMU_PERI_IP>,
> +				 <&cmu_top CLK_DOUT_CMU_PERI_UART>;
> +			clock-names = "oscclk",
> +				      "bus",
> +				      "ip",
> +				      "uart";
> +			samsung,sysreg = <&sysreg_peri>;
> +		};
> +
> +		watchdog: watchdog@10050000 {
> +			compatible = "samsung,exynos5515-wdt";
> +			reg = <0x10050000 0x100>;
> +			clocks = <&cmu_peri CLK_GOUT_PERI_WDT0_PCLK>,
> +				 <&oscclk>;
> +			clock-names = "watchdog",
> +				      "watchdog_src";
> +			interrupts = <GIC_SPI 185 IRQ_TYPE_LEVEL_HIGH>;
> +			samsung,syscon-phandle = <&pmu_system_controller>;
> +
> +			status = "disabled";
> +		};

[Severity: Medium]
Can this watchdog node pass dtbs_check or bind to a driver when enabled?

The compatible string "samsung,exynos5515-wdt" has no fallback compatible
and is not present in Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
or in s3c2410_wdt_match[] in drivers/watchdog/s3c2410_wdt.c:

static const struct of_device_id s3c2410_wdt_match[] = {
	{ .compatible = "google,gs101-wdt",
	  .data = &drv_data_gs101_cl0 },
	...
	{ .compatible = "samsung,exynos850-wdt",
	  .data = &drv_data_exynos850_cl0 },
	...
};

Also, for dual-clock Exynos watchdog variants with "watchdog" and
"watchdog_src" clocks, both samsung-wdt.yaml and s3c2410_get_wdt_drv_data()
in drivers/watchdog/s3c2410_wdt.c require the samsung,cluster-index property
and fail probe with -EINVAL when it is missing.

> +
> +		cmu_fsys: clock-controller@10400000 {
[ ... ]
> +			samsung,sysreg = <&sysreg_fsys>;
> +		};
> +
> +		sysreg_fsys: syscon@10420000 {
> +			compatible = "samsung,exynos5515-fsys-sysreg", "syscon";
> +			reg = <0x10420000 0x10000>;
> +		};
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-for-next-lucky7-v2-0-c4ecab4b1c4c@member.fsf.org?part=4