[PATCH v3 1/1] clk: sunxi-ng: h616: Reparent GPU clock during frequency changes

[PATCH v3 1/1] clk: sunxi-ng: h616: Reparent GPU clock during frequency changes

[Philippe Simons]

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>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 36 +++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
index 190816c35..6050cbfa9 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);
+
+/*
+ * 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,
 					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 */
+	.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, to reach a safe 400 MHz.
+	 */
+	val = readl(reg + SUN50I_H616_GPU_CLK1_REG);
+	val &= ~GENMASK(1, 0);
+	val |= 2;
+	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;
 }
 
-- 
2.48.1

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

[Jernej Škrabec]

Dne četrtek, 20. februar 2025 ob 12:38:08 Srednjeevropski standardni čas je Philippe Simons napisal(a):
> 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>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 36 +++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)

Changelog is missing here. What's changed?

In any case, this patch isn't useful on its own. What about PPU and GPU DT node?

Best regards,
Jernej

> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> index 190816c35..6050cbfa9 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);
> +
> +/*
> + * 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,
>  					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 */
> +	.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, to reach a safe 400 MHz.
> +	 */
> +	val = readl(reg + SUN50I_H616_GPU_CLK1_REG);
> +	val &= ~GENMASK(1, 0);
> +	val |= 2;
> +	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;
>  }
>  
> 

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

[Jernej Škrabec]

Dne četrtek, 20. februar 2025 ob 16:41:54 Srednjeevropski standardni čas je Jernej Škrabec napisal(a):
> Dne četrtek, 20. februar 2025 ob 12:38:08 Srednjeevropski standardni čas je Philippe Simons napisal(a):
> > 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>
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 36 +++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> Changelog is missing here. What's changed?
> 
> In any case, this patch isn't useful on its own. What about PPU and GPU DT node?

In the light of separate PPU & GPU DT series, this patch is gtg.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej
 
> > 
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > index 190816c35..6050cbfa9 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);
> > +
> > +/*
> > + * 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,
> >  					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 */
> > +	.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, to reach a safe 400 MHz.
> > +	 */
> > +	val = readl(reg + SUN50I_H616_GPU_CLK1_REG);
> > +	val &= ~GENMASK(1, 0);
> > +	val |= 2;
> > +	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;
> >  }
> >  
> > 
> 
> 
> 
> 
> 

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

[Chen-Yu Tsai]

On Thu, 20 Feb 2025 12:38:08 +0100, Philippe Simons wrote:
> 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.
> 
> [...]

Applied to clk-for-6.15 in git@github.com:linux-sunxi/linux-sunxi.git, thanks!

[1/1] clk: sunxi-ng: h616: Reparent GPU clock during frequency changes
      commit: eb963d7948ce6571939c6875424b557b25f16610

Best regards,
-- 
Chen-Yu Tsai <wens@csie.org>