Re: [PATCH v5 10/25] clk: mediatek: pll: Add BAR reset register offsets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly introduced `rst_bar_set_addr` and `rst_bar_clr_addr` are completely unused in the `mtk_pll_prepare` and `mtk_pll_unprepare` callbacks, causing read-modify-writes to be performed on the status register instead of writing to the dedicated set/clear registers.
--

--- Patch [10]: [PATCH v5 10/25] clk: mediatek: pll: Add BAR reset register offsets ---
commit 9a53464d4cd99a5e5248c794e08d787f148b4b00
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: pll: Add BAR reset register offsets

This commit introduces new fields to the MTK PLL data structure to support
separate reset BAR status, set, and clear registers on the MT8189 SoC. It
also updates the prepare and unprepare callbacks to use the newly computed
reset BAR address instead of hardcoding the CON0 register offset.

> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index ee478bc640851..4b0f7e2bfc5a5 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
[ ... ]
> @@ -249,9 +249,9 @@ int mtk_pll_prepare(struct clk_hw *hw)
>  	udelay(20);
>  
>  	if (pll->data->flags & HAVE_RST_BAR) {
> -		r = readl(pll->base_addr + REG_CON0);
> +		r = readl(pll->rst_bar_addr);
>  		r |= pll->data->rst_bar_mask;
> -		writel(r, pll->base_addr + REG_CON0);
> +		writel(r, pll->rst_bar_addr);
>  	}

[Severity: High]
Does this logic correctly handle the new set and clear registers for MT8189?

The commit message mentions that MT8189 has separate set and clr registers 
to handle the reset BAR. However, this code continues to perform a 
read-modify-write on the status register (rst_bar_addr) instead of writing
to the newly added rst_bar_set_addr. 

Will this fail to assert the reset BAR on MT8189 hardware that requires
the dedicated set register?

>  
>  	return 0;
> @@ -263,9 +263,9 @@ void mtk_pll_unprepare(struct clk_hw *hw)
>  	u32 r;
>  
>  	if (pll->data->flags & HAVE_RST_BAR) {
> -		r = readl(pll->base_addr + REG_CON0);
> +		r = readl(pll->rst_bar_addr);
>  		r &= ~pll->data->rst_bar_mask;
> -		writel(r, pll->base_addr + REG_CON0);
> +		writel(r, pll->rst_bar_addr);
>  	}

[Severity: High]
Similarly, shouldn't this unprepare path write to rst_bar_clr_addr if it is 
available, rather than doing a read-modify-write on the status register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801-mt8189-clocks-system-base-v5-0-ac4410697cc2@collabora.com?part=10