Re: [PATCH v2] clk: sunxi-ng: h616: Reparent GPU clock during frequency changes

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 12 Feb 2025 18:36:39 +0100
Philippe Simons <simons.philippe@gmail.com> wrote:

Hi,

> The H616 manual does not state that the GPU PLL supports
> dynamic frequency configuration, so we must take extra care when changing
> the frequency. Currently any attempt to do device DVFS on the GPU lead
> to panfrost various ooops, and GPU hangs.
> 
> The manual describes the algorithm for changing the PLL
> frequency, which the CPU PLL notifier code already support, so we reuse
> that to reparent the GPU clock to GPU1 clock during frequency
> changes.
> 
> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 38 ++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> index 190816c35..884f9a6b5 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> @@ -328,10 +328,16 @@ static SUNXI_CCU_M_WITH_MUX_GATE(gpu0_clk, "gpu0", gpu0_parents, 0x670,
>  				       24, 1,	/* mux */
>  				       BIT(31),	/* gate */
>  				       CLK_SET_RATE_PARENT);
> -static SUNXI_CCU_M_WITH_GATE(gpu1_clk, "gpu1", "pll-periph0-2x", 0x674,
> +
> +/*
> + * This clk is needed as a temporary fall back during GPU PLL freq changes.
> + * Set CLK_IS_CRITICAL flag to prevent from being disabled.
> + */
> + #define SUN50I_H616_GPU_CLK1_REG        0x674
> + static SUNXI_CCU_M_WITH_GATE(gpu1_clk, "gpu1", "pll-periph0-2x", 0x674,

There seems to be a stray space there at the beginning of those two
lines.

>  					0, 2,	/* M */
>  					BIT(31),/* gate */
> -					0);
> +					CLK_IS_CRITICAL);
>  
>  static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "psi-ahb1-ahb2",
>  		      0x67c, BIT(0), 0);
> @@ -1120,6 +1126,19 @@ static struct ccu_pll_nb sun50i_h616_pll_cpu_nb = {
>  	.lock		= BIT(28),
>  };
>  
> +static struct ccu_mux_nb sun50i_h616_gpu_nb = {
> +	.common			= &gpu0_clk.common,
> +	.cm				= &gpu0_clk.mux,
> +	.delay_us		= 1, /* manual doesn't really say */

Indentation is off here. Please align to the last line here below.

> +	.bypass_index	= 1, /* GPU_CLK1@400MHz */
> +};
> +
> +static struct ccu_pll_nb sun50i_h616_pll_gpu_nb = {
> +	.common		= &pll_gpu_clk.common,
> +	.enable		= BIT(29),	/* LOCK_ENABLE */
> +	.lock		= BIT(28),
> +};
> +
>  static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>  {
>  	void __iomem *reg;
> @@ -1170,6 +1189,14 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>  	val |= BIT(0);
>  	writel(val, reg + SUN50I_H616_PLL_AUDIO_REG);
>  
> +	/*
> +	 * Set the input-divider for the gpu1 clock to 3.

Can you extend the comment, like:
 ... clock to 3, to reach a safe 400 MHz.

> +	 */
> +	val = readl(reg + SUN50I_H616_GPU_CLK1_REG);
> +	val &= ~GENMASK(1, 0);
> +	val |= BIT(1);

Can you replace the BIT(1) with a "2" here? At the moment it looks like
it's a single flag to be set, whereas we really just want to write the
value 2 (3 - 1) into that field.

The rest looks alright, so with those smaller things fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> +	writel(val, reg + SUN50I_H616_GPU_CLK1_REG);
> +
>  	/*
>  	 * First clock parent (osc32K) is unusable for CEC. But since there
>  	 * is no good way to force parent switch (both run with same frequency),
> @@ -1190,6 +1217,13 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev)
>  	/* Re-lock the CPU PLL after any rate changes */
>  	ccu_pll_notifier_register(&sun50i_h616_pll_cpu_nb);
>  
> +	/* 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);
> +
>  	return 0;
>  }
>