Message-ID: <CADomA4-BuTtGva+MhcfZzeidsHr9pe6sibFYz97b-5of+0ucRw@mail.gmail.com>
Date: Mon, 10 Feb 2025 21:21:43 +0100
From: Philippe Simons <simons.philippe@...il.com>
To: Andre Przywara <andre.przywara@....com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	Samuel Holland <samuel@...lland.org>, 
	"open list:COMMON CLK FRAMEWORK" <linux-clk@...r.kernel.org>, 
	"moderated list:ARM/Allwinner sunXi SoC support" <linux-arm-kernel@...ts.infradead.org>, 
	"open list:ARM/Allwinner sunXi SoC support" <linux-sunxi@...ts.linux.dev>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: sunxi-ng: h616: Reparent GPU clock during frequency changes

On Mon, Feb 10, 2025 at 3:05 PM Andre Przywara <andre.przywara@....com> wrote:
>
> On Sun,  9 Feb 2025 19:31:42 +0100
> Philippe Simons <simons.philippe@...il.com> wrote:
>
> Hi Philippe,
>
> thanks for bringing this patch together!
>
> You should mention here *why* you want this patch, the motivation seems to
> be missing, but would be crucial - and preventing a sure crash should make
> this a no-brainer.
>
> Maybe start with: 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 ...
>
> Then mention that the manual describes the algorithm for changing the PLL
> frequency, which the CPU PLL notifier code already support, so we reuse
> that.
>
> > Re-parent the GPU clock during frequency changes of the PLL.
> > Also it asks to disable and then re-enable the PLL lock bit,
> > after the factor changes have been applied.
> >
> > Add clock notifiers for the PLL and the GPU mux clock, using the existing
> > notifier callbacks, and tell them to use mux 1 (the GPU_CLK1 source),
> > and bit 29 (the LOCK_ENABLE) bit. The existing code already follows the
> > correct algorithms.
> >
> > Signed-off-by: Philippe Simons <simons.philippe@...il.com>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 33 +++++++++++++++++++++++++-
> >  1 file changed, 32 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..e88eefa24 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > @@ -328,10 +328,12 @@ static SUNXI_CCU_M_WITH_MUX_GATE(gpu0_clk, "gpu0", gpu0_parents, 0x670,
> >                                      24, 1,   /* mux */
> >                                      BIT(31), /* gate */
> >                                      CLK_SET_RATE_PARENT);
> > +
> > +#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);
>
> The addition of CLK_IS_CRITICAL deserves a comment, something about that
> this clock is needed as a temporary fallback clock for the PLL frequency
> changes or so.
>
> >  static SUNXI_CCU_GATE(bus_gpu_clk, "bus-gpu", "psi-ahb1-ahb2",
> >                     0x67c, BIT(0), 0);
> > @@ -1120,6 +1122,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 */
> > +};
> > +
> > +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 +1185,15 @@ 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.
> > +      * Also enable gpu1 clock.
> > +      */
> > +     val = readl(reg + SUN50I_H616_GPU_CLK1_REG);
> > +     val |= BIT(31);
>
> Do we need this if the clock is marked as critical now?
I'll test that.
>
> > +     val |= BIT(1);
>
> You probably want to clear the lowest 2 bits first, then set bit 1,
> otherwise you end up with either 2 or 3, depending on what bit 0 was
> before.
Good catch, I'll address that in V2.
>
> 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 +1214,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;
> >  }
> >
>
Thanks for the review,
Philippe

Powered by blists - more mailing lists