Re: [PATCH v4 12/24] clk: mediatek: pll: Add ops for PLLs using set/clr regs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Reusing common prepare/unprepare functions for SET/CLR registers incorrectly performs read-modify-write (RMW) operations, failing to toggle the target PLL and potentially disabling unrelated active PLLs.
- [High] `mtk_pll_prepare_setclr` and `unprepare_setclr` improperly call common functions that execute `pwr_addr` register writes, contradicting the hardware design.
- [High] Missing hardware state rollback when `rst_bar` polling fails in `mtk_pll_prepare_setclr()`.
- [High] Early return on `rst_bar` timeout in `mtk_pll_unprepare_setclr()` aborts PLL teardown, leaving the hardware fully powered on.
--

--- Patch [12]: [PATCH v4 12/24] clk: mediatek: pll: Add ops for PLLs using set/clr regs ---
commit 85e380ac652f1fdab76c2fb7262d10466dfdb403
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: pll: Add ops for PLLs using set/clr regs

MT8189 SoC uses a new combination of status and set/clr registers to
control its PLL power state and perform BAR reset, that are different
than the current default clock prepare/unprepare operations are using.

Add new set of PLL clock operations to support this logic that relies
on the following registers for prepare/unprepare operations:
  - en/en_set/en_clr, rather than pwr register, for PLL power
    control
  - rst_bar/rst_bar_set/rst_bar_clr, rather than default rst_bar
    register for BAR reset control

Also, handle rst_bar register setting/clearing with a timeout, to
verify the operation was correctly performed.

> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -305,6 +307,48 @@ void mtk_pll_unprepare(struct clk_hw *hw)
>  }
>  EXPORT_SYMBOL_GPL(mtk_pll_unprepare);
>  
> +int mtk_pll_prepare_setclr(struct clk_hw *hw)
> +{
> +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +	u32 val = 0;
> +	int ret;
> +
> +	mtk_pll_prepare_common(pll, pll->en_set_addr);

[Severity: High]
The commit message mentions these new PLLs use en/en_set/en_clr rather than
the pwr register for power control. Since mtk_pll_prepare_common() executes
read-modify-write operations on pll->pwr_addr (such as setting CON0_PWR_ON),
does this corrupt other configuration bits at REG_CON0 if pwr_addr points to
the default base offset?

> +
> +	if (pll->data->flags & HAVE_RST_BAR) {
> +		writel(pll->data->rst_bar_mask, pll->rst_bar_set_addr);
> +
> +		ret = readl_poll_timeout(pll->rst_bar_addr, val,
> +					(val & pll->data->rst_bar_mask), 1,
> +					RST_BAR_TIMEOUT);
> +		if (ret)
> +			return ret;

[Severity: High]
If the rst_bar readiness polling times out, does this leave the hardware in
an inconsistent state?

Since mtk_pll_prepare_common() was already called to power on the PLL and
enable the tuner, returning the error directly here skips undoing those
steps. The clock core sees the prepare fail and will not invoke the unprepare
callback on error paths, permanently leaking power.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_pll_prepare_setclr);
> +
> +void mtk_pll_unprepare_setclr(struct clk_hw *hw)
> +{
> +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +	u32 val = 0;
> +	int ret;
> +
> +	if (pll->data->flags & HAVE_RST_BAR) {
> +		writel(pll->data->rst_bar_mask, pll->rst_bar_clr_addr);
> +
> +		ret = readl_poll_timeout(pll->rst_bar_addr, val,
> +					!(val & pll->data->rst_bar_mask), 1,
> +					RST_BAR_TIMEOUT);
> +		if (ret)
> +			return;

[Severity: High]
If this polling times out, does the early return permanently leak the PLL's
active state?

Skipping the final call to mtk_pll_unprepare_common() leaves the hardware
fully powered on, but the clock core considers the clock disabled since this
function returns void. Hardware teardown paths typically proceed on a best
effort basis rather than bailing out at the first error.

> +	}
> +
> +	mtk_pll_unprepare_common(pll, pll->en_clr_addr);

[Severity: Critical]
Does passing a clear (W1C) register to mtk_pll_unprepare_common() cause
unintended side effects?

Looking at mtk_pll_unprepare_common(), it performs a read-modify-write:

    r = readl(en_addr) & ~BIT(pll->data->pll_en_bit);
    writel(r, en_addr);

If en_clr_addr reads back the status of all PLLs, wouldn't masking out the
target bit to 0 and writing the result back write 1s for all other active
PLLs, inadvertently disabling them?

> +}
> +EXPORT_SYMBOL_GPL(mtk_pll_unprepare_setclr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-mt8189-clocks-system-base-v4-0-e356b813a64c@collabora.com?part=12