Hi Geert,
Thank you for the review.
On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> 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).
GPU node enabled in board DTS:
---------------------------------------------
root@rzv2h-evk-alpha:~# cat /sys/kernel/debug/clk/clk_summary | grep gpu
.pllgpu 1 1 0
1260000000 0 0 50000 Y deviceless
no_connection_id
.pllgpu_gear 1 1 0
630000000 0 0 50000 Y deviceless
no_connection_id
gpu_0_clk 1 2 0
630000000 0 0 50000 Y 14850000.gpu
no_connection_id
14850000.gpu
no_connection_id
gpu_0_ace_clk 0 1 0
400000000 0 0 50000 N deviceless
of_clk_get_from_provider
gpu_0_axi_clk 1 2 0
400000000 0 0 50000 Y 14850000.gpu
bus
root@rzv2h-evk-alpha:~#
GPU node disabled in board DTS:
---------------------------------------------
root@rzv2h-evk-alpha:~# cat /sys/kernel/debug/clk/clk_summary | grep gpu
.pllgpu 0 0 0
1260000000 0 0 50000 N deviceless
no_connection_id
.pllgpu_gear 0 0 0
630000000 0 0 50000 Y deviceless
no_connection_id
gpu_0_clk 0 0 0
630000000 0 0 50000 N deviceless
no_connection_id
gpu_0_ace_clk 0 0 0
400000000 0 0 50000 N deviceless
no_connection_id
gpu_0_axi_clk 0 0 0
400000000 0 0 50000 N deviceless
no_connection_id
root@rzv2h-evk-alpha:~#
> > 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
> > @@ -56,9 +56,16 @@
> >
> > #define CPG_CLKSTATUS0 (0x700)
> >
> > +#define PLL_STBY_RESETB BIT(0)
> > +#define PLL_STBY_RESETB_WEN BIT(16)
>
> Please move these just below the definition of PLL_STBY_OFFSET().
> (Hmm, [KMP]DIV() should be below PLL_CLK1_OFFSET(), and
> SDIV() below PLL_CLK2_OFFSET()).
>
OK.
> > +#define PLL_MON_RESETB BIT(0)
> > +#define PLL_MON_LOCK BIT(4)
>
> Please move these just below the definition of PLL_MON_OFFSET().
>
OK.
> > +
> > #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.
Or maybe instead of using a conf can I add the below?
+/**
+ * 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;
+};
+
+#define PLL_PACK(_offset, _clk, _sscen) \
+ ((struct pll){ \
+ .offset = _offset, \
+ .clk = _clk \
+ .sscen = _sscen \
+ })
+
+#define PLLCA55 PLL_PACK(0x64, 1, 0)
> #define CPG_PLL_STBY 0x000
> #define CPG_PLL_CLK1 0x004
> #define CPG_PLL_CLK2 0x008
> #define CPG_PLL_MON 0x010
>
> >
> > /**
> > * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> > @@ -144,6 +151,54 @@ struct ddiv_clk {
> >
> > #define to_ddiv_clock(_div) container_of(_div, struct ddiv_clk, div)
> >
> > +static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw)
> > +{
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + u32 mon_offset = PLL_MON_OFFSET(pll_clk->conf);
> > + u32 val;
> > +
> > + val = readl(priv->base + mon_offset);
>
> As mon_offset is used only once, you can combine the above 4 lines
> into a single line.
>
OK.
> > +
> > + /* Ensure both RESETB and LOCK bits are set */
> > + return (val & (PLL_MON_RESETB | PLL_MON_LOCK)) ==
> > + (PLL_MON_RESETB | PLL_MON_LOCK);
> > +}
> > +
> > +static int rzv2h_cpg_pll_clk_enable(struct clk_hw *hw)
> > +{
> > + bool enabled = rzv2h_cpg_pll_clk_is_enabled(hw);
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + u32 conf = pll_clk->conf;
> > + unsigned long flags = 0;
> > + u32 stby_offset;
> > + u32 mon_offset;
> > + u32 val;
> > + int ret;
> > +
> > + if (enabled)
>
> if (!rzv2h_cpg_pll_clk_is_enabled(hw))
>
> for brevity.
>
OK.
> > + return 0;
> > +
> > + stby_offset = PLL_STBY_OFFSET(conf);
> > + mon_offset = PLL_MON_OFFSET(conf);
> > +
> > + val = PLL_STBY_RESETB_WEN | PLL_STBY_RESETB;
> > + spin_lock_irqsave(&priv->rmw_lock, flags);
> > + writel(val, priv->base + stby_offset);
> > + spin_unlock_irqrestore(&priv->rmw_lock, flags);
>
> A single writel does not need protection by a spinlock.
>
OK, I will drop it.
> > +
> > + /* ensure PLL is in normal mode */
> > + ret = readl_poll_timeout(priv->base + mon_offset, val,
> > + (val & (PLL_MON_RESETB | PLL_MON_LOCK)) ==
> > + (PLL_MON_RESETB | PLL_MON_LOCK), 0, 250000);
>
> How long does this typically take?
> I.e. would it make sense to use a non-zero delay_us()?
>
I'll rework on timeout value.
Cheers,
Prabhakar
