RE: [PATCH v5 2/4] clk: renesas: rzg2l: Add support for enabling PLLs

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 23 April 2026 10:37
> Subject: Re: [PATCH v5 2/4] clk: renesas: rzg2l: Add support for enabling PLLs
> 
> Hi Biju,
> 
> On Thu, 26 Mar 2026 at 12:06, Biju <biju.das.au@xxxxxxxxx> wrote:
> > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Add support for enabling PLL clocks in the RZ/G3L CPG driver to turn
> > off some PLLs, if they are not in use(eg: PLL6, PLL7)
> >
> > Introduce `is_enabled` and `enable` callbacks to handle PLL state
> > transitions. With the `enable` callback, PLL will be turned ON only
> > when the PLL consumer device is enabled; otherwise, it will remain
> > off. Define new macros for PLL standby and monitor registers to
> > facilitate this process.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -58,6 +58,13 @@
> >  #define RZG3S_DIV_NF           GENMASK(12, 1)
> >  #define RZG3S_SEL_PLL          BIT(0)
> >
> > +#define RZG3L_PLL_STBY_OFFSET(x)       (GET_REG_SAMPLL_CLK1(x) - 0x4)
> > +#define RZG3L_PLL_STBY_RESETB          BIT(0)
> > +#define RZG3L_PLL_STBY_RESETB_WEN      BIT(16)
> > +#define RZG3L_PLL_MON_OFFSET(x)                (GET_REG_SAMPLL_CLK1(x) + 0x8)
> 
> This - 0x4 / + 0x8 is a bit hard to follow.  I don't want to block this series, so for now it's OK.

Ok.

> I think it would be good to refactor the whole PLL sub register offset
> handling: currently the config value contains the offsets of both the
> CLK1 and CLK2 registers (which differ by a fixed value of 4) in the config value, and the other
> register offsets are derived using the macros above.  Instead, it could store the lowest PLL registers
> offset (STBY), and derive all others from that by additions only.

I will send follow up patch to correct this logic later.


> 
> > +static int rzg3l_cpg_pll_clk_endisable(struct clk_hw *hw, bool
> > +enable) {
> > +       struct pll_clk *pll_clk = to_pll(hw);
> > +       struct rzg2l_cpg_priv *priv = pll_clk->priv;
> > +       u32 stby_offset, mon_offset;
> > +       u32 val, mon_val;
> > +       int ret;
> > +
> > +       stby_offset = RZG3L_PLL_STBY_OFFSET(pll_clk->conf);
> > +       mon_offset = RZG3L_PLL_MON_OFFSET(pll_clk->conf);
> > +
> > +       if (enable) {
> > +               val = RZG3L_PLL_STBY_RESETB_WEN | RZG3L_PLL_STBY_RESETB;
> > +               mon_val = RZG3L_PLL_MON_RESETB | RZG3L_PLL_MON_LOCK;
> > +       } else {
> > +               val = RZG3L_PLL_STBY_RESETB_WEN;
> > +               mon_val = 0;
> > +       }
> > +
> > +       writel(val, priv->base + stby_offset);
> > +
> > +       /* ensure PLL is in normal/stanby mode */
> 
> standby.

Oops, missed this.

Cheers,
Biju

> 
> > +       ret = readl_poll_timeout_atomic(priv->base + mon_offset, val, mon_val ==
> > +                                       (val & (RZG3L_PLL_MON_RESETB | RZG3L_PLL_MON_LOCK)),
> > +                                       10, 100);
> > +       if (ret)
> > +               dev_err(priv->dev, "Failed to %s PLL 0x%x/%pC\n", enable ?
> > +                       "enable" : "disable", stby_offset, hw->clk);
> > +
> > +       return ret;
> > +}
> 
> With the typo fixed:
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But when I'm talking to
> journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds