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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Clock Tree Misconfiguration / Unintended Clock Bypass in CMU_PERI
- [High] Incorrect Mux Parent Clock Order in CMU_FSYS User MUXes
- [Medium] Missing Initialization and State Preservation for EXYNOS5515_MEMCLK_OFFSET
--

--- Patch [3]: [PATCH 3/3] clk: samsung: Introduce Exynos5515 clock driver ---
commit 30844f6e4b1e1ae616f216fdbd770df29393e674
Author: Aiden Isik <[email protected]>

clk: samsung: Introduce Exynos5515 clock driver

CMU_TOP is the top level clock management unit which contains PLLs,
muxes, dividers and gates that feed the other clock management units.
CMU_PERI provides clocks for USI blocks and their children, PWM, MCT,
and watchdog timers. CMU_FSYS provides clocks for MMC, USB, GPIO, etc.

> diff --git a/drivers/clk/samsung/clk-exynos5515.c b/drivers/clk/samsung/clk-exynos5515.c
> new file mode 100644
> index 0000000000000..f989a13f1a574
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos5515.c
> @@ -0,0 +1,1485 @@
> +// SPDX-License-Identifier: GPL-2.0-only

[ ... ]

> +static const unsigned long drcg_sysreg[] __initconst = {
> +	EXYNOS5515_DRCG_EN_OFFSET,
> +};

[Severity: Medium]
Is it intended that EXYNOS5515_MEMCLK_OFFSET is missing from the drcg_sysreg
array? Without this offset, the memory clock gating state won't be saved or
restored across power management suspend/resume cycles.

> +
> +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,
> +};

[Severity: Medium]
Will this break the dynamic root clock gating initialization? The
.memclk_offset = EXYNOS5515_MEMCLK_OFFSET assignment seems to be missing
here, which might prevent samsung_en_dyn_root_clk_gating() from enabling the
memory clock gate correctly.

[ ... ]

> +/* 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" };

[Severity: High]
Does this correctly route the clocks from CMU_TOP? Referencing the
intermediate mout_cmu_peri_* clocks here instead of the final
dout_cmu_peri_* dividers appears to bypass the hardware gates and dividers
defined in CMU_TOP. Could this lead to incorrect clock rate calculations and
resource leaks if the CMU_TOP blocks fail to gate when PERI clocks are
disabled?

[ ... ]

> +/* Parent clock list for CMU_FSYS muxes */
> +PNAME(mout_pll_fsys_bus_user_p)		= { "dout_cmu_fsys_bus",
> +					    "oscclk" };
> +
> +/* Parent clock list for CMU_FSYS muxes: for MMC */
> +PNAME(mout_pll_fsys_mmc_card_user_p)	= { "dout_cmu_fsys_mmc_card",
> +					    "oscclk" };
> +
> +PNAME(mout_pll_fsys_mmc_embd_user_p)	= { "dout_cmu_fsys_mmc_embd",
> +					    "oscclk" };
> +
> +/* Parent clock list for CMU_FSYS muxes: for USB */
> +PNAME(mout_pll_fsys_usb20drd_user_p)	= { "dout_cmu_fsys_usb20drd",
> +					    "oscclk" };

[Severity: High]
Is the parent clock order inverted in these MUXes? User MUX arrays like those
in CMU_PERI map oscclk to hardware value 0 and the bus clock to 1. Since the
clock framework uses the array index for the register value, placing oscclk
at index 1 here will write 0 when requesting the high-speed bus clock,
inadvertently selecting the 26MHz oscclk instead. Could this cause
performance degradation and hardware instability?

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