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

This is intentional, downstream only has this on a few blocks and
these are not amongst those blocks. Will see if a maintainer agrees.

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

See above.

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

Unsure, but this is how downstream does it.

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

Possibly, I thought the ordering was just cosmetic and for the
kernel's informational purposes. HWACG seems to mask the problem
if there is one but will still fix in v2.

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

Kind regards,
- Aiden Isik