Re: [PATCH v2 3/5] clk: samsung: clk-pll: Add support for pll_1061x

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the fast-path check `samsung_pll46xx_mpk_change()` hardcodes a 9-bit mask for `mdiv`, which incorrectly truncates the 10-bit `mdiv` used by the newly added `pll_1061x` and the existing `pll_1460x`. This can bypass the main multiplier update and leave the PLL running at the wrong frequency.
--

--- Patch [3]: [PATCH v2 3/5] clk: samsung: clk-pll: Add support for pll_1061x ---
commit 22c944c044739f58fab83d8908a877841003ecb0
Author: Alexandru Chimac <alex@chimac.ro>

clk: samsung: clk-pll: Add support for pll_1061x

This patch adds support for the pll_1061x found in the Exynos9610 and
Exynos9810 SoCs. It reuses the logic from pll_1460x, adapting it to use
CON3 instead of CON1.

> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index fdb84bcec9127..5ec9423009c7b 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c

[ ... ]

> @@ -827,7 +832,10 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,
>  	}
>  
>  	con0 = readl_relaxed(pll->con_reg);
> -	con1 = readl_relaxed(pll->con_reg + 0x4);
> +	if (pll->type == pll_1061x)
> +		con1 = readl_relaxed(pll->con_reg + 0xc);
> +	else
> +		con1 = readl_relaxed(pll->con_reg + 0x4);
>  
>  	if (!(samsung_pll46xx_mpk_change(con0, con1, rate))) {
>  		/* If only s change, change just s value only*/
> @@ -845,7 +853,7 @@ static int samsung_pll46xx_set_rate(struct clk_hw *hw, unsigned long drate,

[Severity: High]
This isn't a bug introduced by this patch, but does the fast-path check
samsung_pll46xx_mpk_change() incorrectly truncate the 10-bit mdiv for
the newly added pll_1061x and the existing pll_1460x?

Looking at samsung_pll46xx_mpk_change(), it hardcodes a 9-bit mask for
the comparison:

    old_mdiv = (pll_con0 >> PLL46XX_MDIV_SHIFT) & PLL46XX_MDIV_MASK;

If the hardware boots with a 10-bit MDIV and the driver requests a new rate
where the lower 9 bits match the current hardware state, old_mdiv == rate->mdiv
will incorrectly evaluate to true.

Will this cause the driver to mistakenly take this SDIV-only fast-path,
skipping the required main multiplier (MDIV) update and leaving the PLL
running at the wrong frequency?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-exynos9610-clocks-v2-0-c13ce8ef1861@chimac.ro?part=3