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

[Brian Masney]

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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> ---
> +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