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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Asymmetry in power sequence execution for PLLs without a power status register will lead to permanently disabled clocks.
--

--- Patch [5]: [PATCH v13 05/11] clk: realtek: Add support for phase locked loops (PLLs) ---
commit e50b49e7f53401294ef59626cc76a96ba5a0781e
Author: Cheng-Yu Lee <[email protected]>

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

This commit provides 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
> index 0000000000000..31670b8316cd5
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c

[ ... ]

> +static bool is_power_on(struct rtk_clk_regmap_pll *clkp)
> +{
> +	u32 val;
> +
> +	/* reg == 0 means not configured (assume always on).
> +	 * Register offset 0 is never a valid address on Realtek SoCs.
> +	 */
> +	if (!clkp->power_reg)
> +		return true;

[Severity: High]
Could this result in a permanently disabled clock for write-only PLLs?

If a PLL is configured with power-on and power-off sequences but lacks a power
status register (power_reg == 0), is_power_on() will always return true.

[ ... ]

> +static void rtk_clk_regmap_pll_disable(struct clk_hw *hw)
> +{
> +	struct rtk_clk_regmap_pll *clkp = to_rtk_clk_regmap_pll(hw);
> +	unsigned long flags;
> +
> +	if (!clkp->seq_power_off)
> +		return;
> +
> +	spin_lock_irqsave(&clkp->lock, flags);
> +
> +	regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_off,
> +			       clkp->num_seq_power_off);

If such a clock is disabled (e.g., by clk_disable_unused during init), this
power-off sequence will execute successfully.

[ ... ]

> +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;
> +
> +	ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_on,
> +				     clkp->num_seq_power_on);

However, a subsequent call to rtk_clk_regmap_pll_enable() will hit the
is_power_on() check above, returning true and jumping to unlock before the
power-on sequence can be written.

Does this cause the clock to remain silently disabled in hardware, while the
clock framework considers it enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5