From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
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.
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@bp.renesas.com>
---
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)
+#define PLL_MON_RESETB BIT(0)
+#define PLL_MON_LOCK BIT(4)
+
#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))
+#define PLL_MON_OFFSET(n) (PLL_STBY_OFFSET(n) + (0x10))
/**
* 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);
+
+ /* 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)
+ 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);
+
+ /* 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);
+ if (ret)
+ dev_err(priv->dev, "Failed to enable PLL 0x%x/%pC\n",
+ stby_offset, hw->clk);
+
+ return ret;
+}
+
static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
{
@@ -165,6 +220,8 @@ static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
}
static const struct clk_ops rzv2h_cpg_pll_ops = {
+ .is_enabled = rzv2h_cpg_pll_clk_is_enabled,
+ .enable = rzv2h_cpg_pll_clk_enable,
.recalc_rate = rzv2h_cpg_pll_clk_recalc_rate,
};
--
2.43.0Hi Prabhakar,
Thanks for your patch!
On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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?
> 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@bp.renesas.com>
> ---
> 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()).
> +#define PLL_MON_RESETB BIT(0)
> +#define PLL_MON_LOCK BIT(4)
Please move these just below the definition of PLL_MON_OFFSET().
> +
> #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:
#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.
> +
> + /* 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.
> + 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.
> +
> + /* 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()?
> + if (ret)
> + dev_err(priv->dev, "Failed to enable PLL 0x%x/%pC\n",
> + stby_offset, hw->clk);
> +
> + return ret;
> +}
> +
> static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
> unsigned long parent_rate)
> {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
Hi Geert,
Thank you for the review.
On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > 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@bp.renesas.com>
> > ---
> > 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
Hi Prabhakar,
On Thu, 6 Mar 2025 at 14:04, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > 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@bp.renesas.com>
> > > ---
> > > 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@linux-m68k.org
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
Hi Geert
On Thu, Mar 6, 2025 at 1:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, 6 Mar 2025 at 14:04, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > 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@bp.renesas.com>
> > > > ---
> > > > 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
© 2016 - 2025 Red Hat, Inc.