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

[Yu-Chun Lin <eleanor.lin@realtek.com>]

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