Re: [PATCH 4/5] clk: spacemit: reject rate changes to running firmware PLLs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling clk_set_rate() immediately after clk_disable() may intermittently fail with -EBUSY because the hardware PLL lock bit is checked without polling.
--

commit a90cd779d21914719856a7e102c5e63e9892f73b
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>

clk: spacemit: reject rate changes to running firmware PLLs

CLK_SET_RATE_GATE only protects clocks prepared through CCF. A PLL left
running by firmware can have a zero prepare count, so this flag alone
cannot prevent set_rate() from reprogramming a live PLL.

> diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> index c9852fed31017..2573b8396cefa 100644
> --- a/drivers/clk/spacemit/ccu_pll.c
> +++ b/drivers/clk/spacemit/ccu_pll.c

[ ... ]

> @@ -113,6 +122,17 @@ static void ccu_pll_disable(struct clk_hw *hw)
>  	ccu_update(common, swcr3, PLL_SWCR3_EN, 0);
>  }
>  
> +static int ccu_pll_check_stopped(struct ccu_pll *pll)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(pll->common.lock_regmap, pll->config.reg_lock, &val);
> +	if (ret)
> +		return ret;
> +	return val & pll->config.mask_lock ? -EBUSY : 0;
> +}

[Severity: High]
Can this cause clk_set_rate() to spuriously fail with -EBUSY?

If a driver calls clk_disable_unprepare() followed immediately by
clk_set_rate(), the .disable callback will clear the software enable bit
and return instantly.

When .set_rate calls ccu_pll_check_stopped(), it performs a single
non-blocking read of the hardware PLL lock bit here. Since the analog
PLL requires time to lose lock, wouldn't the lock bit often still be
set, causing the valid rate change to be rejected?

A follow-up patch "clk: spacemit: safely synchronize PLL parameters during
init" seems to acknowledge this behavior by adding a ccu_pll_stop() function
with a polling timeout. Should a similar polling mechanism be used here
instead of a single read?

> +
>  /*
>   * PLLs must be gated before changing rate, which is ensured by
>   * flag CLK_SET_RATE_GATE.
> @@ -122,11 +142,20 @@ static int ccu_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  {
>  	struct ccu_pll *pll = hw_to_ccu_pll(hw);
>  	const struct ccu_pll_rate_tbl *entry;
> +	int ret;
>  
> -	entry = ccu_pll_lookup_best_rate(pll, rate);
> -	ccu_pll_update_param(pll, entry);
> +	/* CLK_SET_RATE_GATE does not account for firmware-only users. */
> +	ret = ccu_pll_is_enabled(hw);
> +	if (ret)
> +		return ret < 0 ? ret : -EBUSY;
> +	ret = ccu_pll_check_stopped(pll);
> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-spacemit-pll-init-v1-0-b3065ad5a4ac@linux.spacemit.com?part=4