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

Yu-Chun Lin
2026-04-10T07:56:03Z

Hi Brian,

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

Agreed, I will add a private spinlock here.

Best Regards,
Yu-Chun