Re: [PATCH v6 04/10] clk: realtek: Add support for phase locked loops (PLLs)

Brian Masney
2026-04-03T14:44:53Z

Hi Cheng-Yu and Yu-Chun,

On Thu, Apr 02, 2026 at 03:39:51PM +0800, Yu-Chun Lin wrote:
> From: Cheng-Yu Lee <cylee12@realtek.com>
>
> Provide a full set of PLL operations for programmable PLLs and a read-only
> variant for fixed or hardware-managed PLLs.
>
> Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> ---
> +static int clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> + const struct freq_table *fv;
> + int ret;
> +
> + fv = ftbl_find_by_rate(clkp->freq_tbl, rate);
> + if (!fv || fv->rate != rate)
> + return -EINVAL;
> +
> + if (clkp->seq_pre_set_freq) {
> + ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_pre_set_freq,
> + clkp->num_seq_pre_set_freq);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_update_bits(clkp->clkr.regmap, clkp->freq_reg,
> + clkp->freq_mask, fv->val);
> + if (ret)
> + return ret;
> +
> + if (clkp->seq_post_set_freq) {
> + ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_post_set_freq,
> + clkp->num_seq_post_set_freq);
> + if (ret)
> + return ret;
> + }
> +
> + if (is_power_on(clkp)) {
> + ret = wait_freq_ready(clkp);

I should have checked Sashiko before I hit send on my last review.
https://sashiko.dev/#/patchset/20260402073957.2742459-1-eleanor.lin%40realtek.com

It suggested the following:

In the Common Clock Framework, .set_rate executes under the prepare_lock
mutex, while .enable and .disable execute under the enable_lock spinlock.

Could an interleaved clk_pll_enable() corrupt the hardware state by running
its seq_power_on sequence concurrently with these multi-step register
updates?

There also appears to be a potential race condition later in this function:

if (is_power_on(clkp)) {
ret = wait_freq_ready(clkp);
...
}

If .disable() powers off the PLL right before wait_freq_ready() is called,
will wait_freq_ready() poll a disabled PLL and erroneously return
-ETIMEDOUT? Is a private spinlock needed to serialize these operations?

Brian