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

> 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 <[email protected]>
> 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?

None of the CMUs introduced by this patchset have this. The
CMUs which have this on Exynos5515 are CMU_DPU, CMU_G3D, CMU_MFC,
CMU_VTS, and CMU_CHUB.

> 
> [ ... ]
> > +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.

This is how they are handled by Samsung themselves in their own kernel
fork.
The Exynos990 clock driver upstream also does this, so it is most likely
correct.

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