Hi Geert
On Thu, Mar 6, 2025 at 1:16 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> 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,
>
I'll reword it to make it clear.
> > > > 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!
>
Thanks.
> > +/**
> > + * 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?
>
I was experimenting with this so just included this hunk but I wont
add it as part of the current submission.
> > +};
> > +
> > +#define PLL_PACK(_offset, _clk, _sscen) \
> > + ((struct pll){ \
> > + .offset = _offset, \
> > + .clk = _clk \
> > + .sscen = _sscen \
> > + })
> > +
> > +#define PLLCA55 PLL_PACK(0x64, 1, 0)
>
> 0x60
Ouch.
Cheers,
Prabhakar
