Re: [PATCH v4 15/24] clk: mediatek: Add MT8189 apmixedsys clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reusing `mtk_pll_unprepare_common()` with a CLR register breaks PLL disable logic, either accidentally clearing other active PLLs or failing to disable the target PLL.
--

--- Patch [15]: [PATCH v4 15/24] clk: mediatek: Add MT8189 apmixedsys clock support ---
commit 5a8d4164915ab1ef9117354d41ec7d48062de00e
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 apmixedsys clock support
    
This commit adds support for the MT8189 apmixedsys clock controller,
which provides PLLs generated from the SoC 26m.

> diff --git a/drivers/clk/mediatek/clk-mt8189-apmixedsys.c b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> new file mode 100644
> index 0000000000000..a50a8979b8495
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
> @@ -0,0 +1,203 @@
[ ... ]
> +#define MT8189_INTEGER_BITS	8
> +
> +#define PLL_SETCLR(_id, _name, _reg, _pwr_reg, _en_setclr_bit,		\
> +			_rstb_setclr_bit, _flags, _pd_reg,	\
> +			_pd_shift, _tuner_reg, _tuner_en_reg,	\
> +			_tuner_en_bit, _pcw_reg, _pcw_shift,	\
> +			_pcwbits) {				\
> +		.id = _id,					\
> +		.name = _name,					\
[ ... ]
> +		.pcw_shift = _pcw_shift,			\
> +		.pcwbits = _pcwbits,				\
> +		.pcwibits = MT8189_INTEGER_BITS,		\
> +		.ops = &mtk_pll_setclr_ops,			\

[Severity: High]
Will using mtk_pll_setclr_ops here cause system instability when disabling
PLLs?

Looking at mtk_pll_unprepare_setclr() (which is called by these ops), it
delegates to mtk_pll_unprepare_common(pll, pll->en_clr_addr). That common
function performs a read-modify-write:

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

If reading the CLR register returns the current PLL status, 'r' will contain
1s for all other active PLLs and 0 for the target PLL. Writing this back to
the CLR register would clear all other active PLLs and leave the target PLL
enabled.

If the CLR register returns 0 (write-only), 'r' becomes 0, and writing 0
does nothing, meaning the target PLL is never disabled.

Should this use a direct write instead of a read-modify-write on a CLR
register, similar to how the reset bar CLR register is handled?

> +	}
> +
> +static const struct mtk_pll_data apmixed_plls[] = {
> +	PLL_SETCLR(CLK_APMIXED_ARMPLL_LL, "armpll-ll", 0x204, 0x210, 18,
[ ... ]

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