Re: [PATCH v11 05/11] clk: realtek: Add support for phase locked loops (PLLs)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `rtk_clk_regmap_pll_enable()` skips the hardware readiness check (`wait_freq_ready`) if the PLL is already powered on, breaking error recovery and trusting potentially unready hardware.
- [Medium] `spinlock_t lock` in `struct rtk_clk_regmap_pll` is defined but lacks an initialization API, leading to potential kernel panics.
--

--- Patch [5]: [PATCH v11 05/11] clk: realtek: Add support for phase locked loops (PLLs) ---
commit 6b0f9bf0f933b92885502feb8287d2125013ada0
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add support for phase locked loops (PLLs)

Provide a full set of PLL operations for programmable PLLs and a read-only
variant for fixed or hardware-managed PLLs.

> diff --git a/drivers/clk/realtek/clk-pll.c b/drivers/clk/realtek/clk-pll.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c
[ ... ]
> +static int rtk_clk_regmap_pll_enable(struct clk_hw *hw)
> +{
> +	struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (!clkp->seq_power_on)
> +		return ret;
> +
> +	spin_lock_irqsave(&clkp->lock, flags);
> +
> +	if (is_power_on(clkp))
> +		goto unlock;

[Severity: High]
If the PLL is already powered on (or if power_reg is 0 and is_power_on()
defaults to true), does this skip verifying if the clock is actually locked?

If a previous enable attempt timed out in wait_freq_ready(), a subsequent
retry would see it powered on and return success here without ensuring the
hardware is ready. Could this lead to consumer peripherals operating with
an unstable or missing clock?

> +
> +	ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_on,
> +				     clkp->num_seq_power_on);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = wait_freq_ready(clkp);
> +	if (ret)
> +		goto unlock;
> +
> +unlock:
> +	spin_unlock_irqrestore(&clkp->lock, flags);
> +
> +	return ret;
> +}
[ ... ]
> diff --git a/drivers/clk/realtek/clk-pll.h b/drivers/clk/realtek/clk-pll.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.h
[ ... ]
> +struct rtk_clk_regmap_pll {
> +	struct rtk_clk_regmap clkr;
> +	const struct reg_sequence *seq_power_on;
> +	u32 num_seq_power_on;
[ ... ]
> +	u32 power_reg;
> +	u32 power_mask;
> +	u32 power_val_on;
> +
> +	/* This lock prevents race conditions when multiple CPUs or contexts
> +	 * simultaneously access this PLL's registers during multi-step operations
> +	 */
> +	spinlock_t lock;

[Severity: Medium]
Is there a missing helper macro or function to initialize this lock?

Since consumer drivers instantiate struct rtk_clk_regmap_pll, if they do not
manually initialize this lock using spin_lock_init(), could calls to
spin_lock_irqsave() in functions like rtk_clk_regmap_pll_recalc_rate()
trigger a kernel panic when CONFIG_DEBUG_SPINLOCK is enabled?

> +};
> +
> +#define __rtk_clk_regmap_pll_hw(_ptr)  __rtk_clk_regmap_hw(&(_ptr)->clkr)
> +
> +extern const struct clk_ops rtk_clk_pll_ops;
> +extern const struct clk_ops rtk_clk_pll_ro_ops;
> +
> +#endif /* __CLK_REALTEK_CLK_PLL_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728142806.1954638-1-eleanor.lin@realtek.com?part=5