Re: [PATCH 2/3] clk: renesas: rzv2h-cpg: Add support for enabling PLLs

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, 6 Mar 2025 at 14:04, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > >
> > > Some RZ/V2H(P) SoC variants do not have a GPU, resulting in PLLGPU being
> > > disabled by default in TF-A. Add support for enabling PLL clocks in the
> > > RZ/V2H(P) CPG driver to manage this.
> >
> > Does it make sense to enable the GPU PLL if no GPU is present?
> >
> No it doesn't,  PLLGPU is enabled on needs basis ie if GPU node is
> enabled the PLLGPU is enabled, if GPU is disabled the PLLGPU will be
> untouched and will remain OFF. Note I also have a patch which does
> disable the PLL's but I have not added as this isn't tested with the
> full system running and I'm not sure if there will be any radiation if
> we turn ON/OFF PLLs (Im discussing this internally once approved I
> will add support to disable PLLs too).

OK. It just sounded a bit strange in the patch description,

> > > Introduce `is_enabled` and `enable` callbacks to handle PLL state
> > > transitions. With the `enable` callback, PLLGPU will be turned ON only
> > > when the GPU node is enabled; otherwise, it will remain off. Define new
> > > macros for PLL standby and monitor registers to facilitate this process.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/clk/renesas/rzv2h-cpg.c | 57 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > >
> > > diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
> > > index 1ebaefb36133..d7230a7e285c 100644
> > > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > > +++ b/drivers/clk/renesas/rzv2h-cpg.c

> > >  #define PLL_CLK_ACCESS(n)      (!!((n) & BIT(31)))
> > >  #define PLL_CLK1_OFFSET(n)     FIELD_GET(GENMASK(15, 0), (n))
> > >  #define PLL_CLK2_OFFSET(n)     (PLL_CLK1_OFFSET(n) + (0x4))
> > > +#define PLL_STBY_OFFSET(n)     (PLL_CLK1_OFFSET(n) - (0x4))
> >
> > Let's subtract 4...
> >
> > > +#define PLL_MON_OFFSET(n)      (PLL_STBY_OFFSET(n) + (0x10))
> >
> > ... and add 0x10. Where are we now? ;-)
> >
> > I think it would be better to store the PLL base offset instead of the
> > PLL CLK1 offset in cpg_core_clk.cfg.conf, and define offsets
> > relative to that:
> >
> You mean PLL_STBY offset in cpg_core_clk.cfg.conf and have the below
> CPG_PLL_XX macros.

Exactly, the PLL_STBY offset is the "base offset" of the various
CPG_PLL_* registers.

> Or maybe instead of using a conf can I add the below?

Sure, sounds fine!

> +/**
> + * struct pll - Structure for PLL configuration
> + *
> + * @offset: STBY register offset
> + * @clk: Flag to indicate if CLK1/2 are accessible or not
> + * @sscen: Flag to indicate if SSCEN bit needs enabling/disabling
> + */
> +struct pll {
> +    unsigned int offset:8;
> +    unsigned int clk:1;
> +    unsigned int sscen:1;

This is a new flag?

> +};
> +
> +#define PLL_PACK(_offset, _clk, _sscen) \
> +    ((struct pll){ \
> +        .offset = _offset, \
> +        .clk = _clk \
> +        .sscen = _sscen \
> +    })
> +
> +#define PLLCA55        PLL_PACK(0x64, 1, 0)

0x60

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