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

[Geert Uytterhoeven]

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.
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.

> +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.

> +       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