| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8uvfb=a=K1YzGNeZdiAzeXWMpdbxj=6UuL_xQfxKmOBZA@mail.gmail.com>
Date: Thu, 6 Mar 2025 13:04:24 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 2/3] clk: renesas: rzv2h-cpg: Add support for enabling PLLs
Hi Geert,
Thank you for the review.
On Wed, Mar 5, 2025 at 4:42 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Tue, 18 Feb 2025 at 12:44, Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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@...2h-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@...2h-evk-alpha:~#
GPU node disabled in board DTS:
---------------------------------------------
root@...2h-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@...2h-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@...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
Powered by blists - more mailing lists