[RFC PATCH] clk: sunxi-ng: h616: Reparent CPU clock during frequency changes

[Andre Przywara]

On Fri, 25 Oct 2024 22:49:27 +0800
Chen-Yu Tsai <wens at csie.org> wrote:

Hi,

> On Fri, Oct 25, 2024 at 6:56 PM Andre Przywara <andre.przywara at arm.com> wrote:
> >
> > The H616 user manual recommends to re-parent the CPU clock during
> > frequency changes of the PLL, and recommends PLL_PERI0(1X), which runs
> > at 600 MHz. 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 CPU mux clock, using the existing
> > notifier callbacks, and tell them to use mux 4 (the PLL_PERI0(1X) source),
> > and bit 29 (the LOCK_ENABLE) bit. The existing code already follows the
> > correct algorithms.
> >
> > Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > ---
> > Hi,
> >
> > the manual states that those changes would be needed to safely change
> > the CPU_PLL frequency during DVFS operation. On my H618 boards it works
> > fine without them, but Philippe reported problems on his H700 board.
> > Posting this for reference at this point, to see if it helps people.
> > I am not sure we should change this without it fixing any real issues.  
> 
> IIRC we do this for all the other SoCs. But if you want to be cautious,
> we can wait for Philippe to give a Tested-by?

Yes, I copied this code from the A64 CCU, but IIRC this was desperately
needed there. But so far I didn't hear many complaints on the H616, and I
ran through like 100,000 transistions in a matter on minutes without any
issues yesterday.
And apparently this patch doesn't fix Philippe's immediate problem, so I
would like to hold it back for now, until we have either more testing,
with or without this patch.

Thanks,
Andre

> ChenYu
> 
> > The same algorithm would apply to the A100/A133 (and the upcoming A523)
> > as well.
> >
> > Cheers,
> > Andre
> >
> >  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 28 ++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > index 84e406ddf9d12..85eea196f25e3 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > @@ -1095,11 +1095,24 @@ static const u32 usb2_clk_regs[] = {
> >         SUN50I_H616_USB3_CLK_REG,
> >  };
> >
> > +static struct ccu_mux_nb sun50i_h616_cpu_nb = {
> > +       .common         = &cpux_clk.common,
> > +       .cm             = &cpux_clk.mux,
> > +       .delay_us       = 1, /* manual doesn't really say */
> > +       .bypass_index   = 4, /* PLL_PERI0 at 600MHz, as recommended by manual */
> > +};
> > +
> > +static struct ccu_pll_nb sun50i_h616_pll_cpu_nb = {
> > +       .common         = &pll_cpux_clk.common,
> > +       .enable         = BIT(29),      /* LOCK_ENABLE */
> > +       .lock           = BIT(28),
> > +};
> > +
> >  static int sun50i_h616_ccu_probe(struct platform_device *pdev)
> >  {
> >         void __iomem *reg;
> >         u32 val;
> > -       int i;
> > +       int ret, i;
> >
> >         reg = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(reg))
> > @@ -1152,7 +1165,18 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev)
> >         val |= BIT(24);
> >         writel(val, reg + SUN50I_H616_HDMI_CEC_CLK_REG);
> >
> > -       return devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc);
> > +       ret = devm_sunxi_ccu_probe(&pdev->dev, reg, &sun50i_h616_ccu_desc);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Reparent CPU during CPU PLL rate changes */
> > +       ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> > +                                 &sun50i_h616_cpu_nb);
> > +
> > +       /* Re-lock the CPU PLL after any rate changes */
> > +       ccu_pll_notifier_register(&sun50i_h616_pll_cpu_nb);
> > +
> > +       return 0;
> >  }
> >
> >  static const struct of_device_id sun50i_h616_ccu_ids[] = {
> > --
> > 2.25.1
> >