Re: [PATCH] clk: sunxi-ng: h616: Gate the PLLs with their output enable bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] clk: sunxi-ng: h616: sun50i_h616_ccu_probe() leaves BIT(27)
  forced on in loop
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] clk: sunxi-ng: h616: wrong GPU notifier registration order in
  sun50i_h616_ccu_probe() [1]
--

commit 733f137b19620733cea59e88a8f5b18ba7301d4a
Author: Arun Iyengar <aiyengar1990@gmail.com>

clk: sunxi-ng: h616: Gate the PLLs with their output enable bit

This commit changes the H616 PLL clocks to gate using their output enable
bit (BIT(27)) instead of the PLL enable bit (BIT(31)), and powers up the
PLLs in sun50i_h616_ccu_probe().

> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> index 955c614830fae..da4156b39c2cc 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c

[ ... ]

> @@ -1214,6 +1214,13 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>  	val |= BIT(0);
>  	writel(val, reg + SUN50I_H616_PLL_AUDIO_REG);
>  
> +	/* Keep the PLLs powered and gate them with bit 27 (manual 3.3.3.6). */
> +	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
> +		val = readl(reg + pll_regs[i]);
> +		val |= BIT(31);
> +		writel(val, reg + pll_regs[i]);
> +	}
> +
>  	/*
>  	 * Set the input-divider for the gpu1 clock to 3, to reach a safe 400 MHz.
>  	 */

[Severity: Medium]
Should the existing pll_regs loop earlier in sun50i_h616_ccu_probe() be
updated to set BIT(31) | BIT(29) instead of adding this second loop?

drivers/clk/sunxi-ng/ccu-sun50i-h616.c:sun50i_h616_ccu_probe() {
    ...
	/* Enable the lock bits and the output enable bits on all PLLs */
	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
		val = readl(reg + pll_regs[i]);
		val |= BIT(29) | BIT(27);
		writel(val, reg + pll_regs[i]);
	}
    ...
}

Leaving that first loop setting BIT(27) unconditionally ungates the output
of all 12 PLLs before the video and audio PLL dividers are configured and
before disabled PLLs are powered up via BIT(31), and causes the clock
framework to see every PLL as already enabled at registration time.

[Severity: High]
This problem wasn't introduced by this patch, but later in
sun50i_h616_ccu_probe() the GPU clock mux notifier is registered before
the GPU PLL notifier [1]:

drivers/clk/sunxi-ng/ccu-sun50i-h616.c:sun50i_h616_ccu_probe() {
    ...
	/* Reparent GPU during GPU PLL rate changes */
	ccu_mux_notifier_register(pll_gpu_clk.common.hw.clk,
				  &sun50i_h616_gpu_nb);

	/* Re-lock the GPU PLL after any rate changes */
	ccu_pll_notifier_register(&sun50i_h616_pll_gpu_nb);
    ...
}

Because both notifiers use priority 0, they run in registration order.
On POST_RATE_CHANGE during GPU frequency scaling, ccu_mux_notifier_cb()
reparents gpu0_clk back to pll_gpu_clk before ccu_pll_notifier_cb()
re-enables and waits for the PLL to lock, driving the GPU from an
unlocked PLL.

[1] https://sashiko.dev/#/bug/linux-b48c9987-4bbf-4231-b1fc-24d8ccdf62d9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-h616-pll-output-gate-v1-1-4cb3fc4d6c48@gmail.com?part=1