Re: [PATCH v2 3/3] clk: samsung: Introduce Exynos5515 clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PERI CMU muxes incorrectly use TOP CMU mux outputs instead of divider outputs, bypassing clock gating and division.
- [Medium] EXYNOS5515_MEMCLK_OFFSET is uninitialized and excluded from the suspend/resume sysreg array.
--

--- Patch [3]: [PATCH v2 3/3] clk: samsung: Introduce Exynos5515 clock driver ---
commit ec18c4d69f3cc6d71671cf32be2a8263d5584524
Author: Aiden Isik <aidenisik@member.fsf.org>
Subject: clk: samsung: Introduce Exynos5515 clock driver

This commit adds Common Clock Framework support for the Exynos5515 SoC. It introduces the CMU_TOP, CMU_PERI, and CMU_FSYS clock management units.

> diff --git a/drivers/clk/samsung/clk-exynos5515.c b/drivers/clk/samsung/clk-exynos5515.c
> new file mode 100644
> index 0000000000000..aeaa51c1c7354
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos5515.c
> @@ -0,0 +1,1485 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +#define EXYNOS5515_GATE_DBG_OFFSET	0x4000
> +#define EXYNOS5515_DRCG_EN_OFFSET	0x0104
> +#define EXYNOS5515_MEMCLK_OFFSET	0x0108

[Severity: Medium]
EXYNOS5515_MEMCLK_OFFSET is defined here, but it appears to be missing from
the drcg_sysreg array below and is not assigned to the memclk_offset field
in any samsung_cmu_info structs (like top_cmu_info). Will this cause the MEMCLK
register state to be lost during system suspend and resume cycles, and skip
its initialization during boot?

[ ... ]
> +static const unsigned long drcg_sysreg[] __initconst = {
> +	EXYNOS5515_DRCG_EN_OFFSET,
> +};
> +
> +static const struct samsung_cmu_info top_cmu_info __initconst = {
[ ... ]
> +	.gate_dbg_offset	= EXYNOS5515_GATE_DBG_OFFSET,
> +	.option_offset		= CMU_CMU_TOP_CONTROLLER_OPTION,
> +	.drcg_offset		= EXYNOS5515_DRCG_EN_OFFSET,
> +};
[ ... ]
> +/* Parent clock list for CMU_PERI muxes */
> +PNAME(mout_pll_peri_bus_user_p)		= { "oscclk",
> +					    "mout_cmu_peri_bus" };
> +
> +PNAME(mout_pll_peri_ip_user_p)		= { "oscclk",
> +					    "mout_cmu_peri_ip" };
> +
> +/* Parent clock list for CMU_PERI muxes: for SPI */
> +PNAME(mout_peri_spi_p)			= { "oscclk",
> +					    "mout_pll_peri_ip_user" };
> +
> +/* Parent clock list for CMU_PERI muxes: for UART */
> +PNAME(mout_pll_peri_uart_user_p)	= { "oscclk",
> +					    "mout_cmu_peri_uart" };

[Severity: High]
Should these parent lists use the divided outputs from TOP CMU
(like dout_cmu_peri_bus, dout_cmu_peri_ip, and dout_cmu_peri_uart)
instead of the direct mux outputs (mout_cmu_peri_*)?

Using the direct mout_* clocks here bypasses the intended clock division
and gating, which might lead to incorrect peripheral clock frequencies and
break power management for these blocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-for-next-lucky7-clock-v2-0-cda0cc80f158@member.fsf.org?part=3