Re: [PATCH 06/10] clk: amlogic: PLL reset signal supports active-low configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `meson_clk_pll_disable` function was not updated to handle the new `CLK_MESON_PLL_RST_ACTIVE_LOW` flag, causing it to incorrectly de-assert the reset signal instead of asserting it.
- [Medium] The flag name documented in the commit message does not match the actual code implementation.
--

commit c5af3b10e7a77c9b06f4a72f2dacfc47fec10acf
Author: Jian Hu <jian.hu@amlogic.com>

clk: amlogic: PLL reset signal supports active-low configuration

In the A9 design, the PLL reset signal is configured as active-low.

Add the flag 'CLK_MESON_PLL_RST_N' to indicate that the PLL reset signal
is active-low.

> Add the flag 'CLK_MESON_PLL_RST_N' to indicate that the PLL reset signal
> is active-low.

This isn't a bug, but the commit message says 'CLK_MESON_PLL_RST_N' while
the code defines CLK_MESON_PLL_RST_ACTIVE_LOW. Could the commit message
be updated to match the code?

> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 5a0bd75f85a92..8568ad6ba7b6e 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c

[ ... ]

> @@ -363,15 +375,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  		return 0;
>  
>  	/* Make sure the pll is in reset */
> -	if (MESON_PARM_APPLICABLE(&pll->rst))
> -		meson_parm_write(clk->map, &pll->rst, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->rst)) {
> +		if (pll->flags & CLK_MESON_PLL_RST_ACTIVE_LOW)
> +			meson_parm_write(clk->map, &pll->rst, 0);
> +		else
> +			meson_parm_write(clk->map, &pll->rst, 1);
> +	}

Since active-low PLLs use 0 to assert the reset here, does
meson_clk_pll_disable() also need to be updated to respect the
CLK_MESON_PLL_RST_ACTIVE_LOW flag?

Currently it unconditionally writes 1 to assert reset:

drivers/clk/meson/clk-pll.c:meson_clk_pll_disable() {
	...
	/* Put the pll is in reset */
	if (MESON_PARM_APPLICABLE(&pll->rst))
		meson_parm_write(clk->map, &pll->rst, 1);
	...
}

If this is an active-low PLL, wouldn't writing 1 de-assert the reset when
the clock is disabled, potentially causing hardware state inconsistencies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-b4-a9_clk-v1-0-41cb4071b7c9@amlogic.com?part=6