RE: [PATCH v5 4/9] clk: renesas: rzg2l-cpg: Re-enable critical module clocks during resume

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 19 March 2026 07:48
> Subject: Re: [PATCH v5 4/9] clk: renesas: rzg2l-cpg: Re-enable critical module clocks during resume
> 
> Hi Biju,
> 
> On Wed, 18 Mar 2026 at 18:06, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> On Wed, 18 Mar 2026
> > > at 15:54, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > On Wed, 18 Mar 2026 at 09:42, Biju <biju.das.au@xxxxxxxxx> wrote:
> > > > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > >
> > > > > After a suspend/resume cycle, critical module clocks may be left
> > > > > disabled as the hardware state is not automatically restored.
> > > > > Unlike regular clocks which are re-enabled by their respective
> > > > > drivers, critical clocks
> > > > > (CLK_IS_CRITICAL) have no owning driver to restore them, so the
> > > > > CPG driver must take responsibility for re-enabling them on resume.
> > > > >
> > > > > Introduce struct rzg2l_crit_clk_hw to track critical module
> > > > > clock hardware entries in a singly-linked list anchored at
> > > > > crit_clk_hw_head in rzg2l_cpg_priv. Populate the list during
> > > > > module clock registration by checking for the CLK_IS_CRITICAL flag after clk_hw_register()
> succeeds.
> > > > >
> > > > > On resume, walk the list and re-enable any critical module clock
> > > > > that is found to be disabled, before deasserting critical
> > > > > resets, ensuring the correct clock-before-reset restore ordering.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > > > @@ -130,6 +130,12 @@ struct div_hw_data {
> > > > >         u32 width;
> > > > >  };
> > > > >
> > > > > +/* Critical clk list  */
> > > > > +struct rzg2l_crit_clk_hw {
> > > > > +       struct clk_hw *hw;
> > > > > +       struct rzg2l_crit_clk_hw *next; };
> > > > > +
> > > > >  #define to_div_hw_data(_hw)    container_of(_hw, struct div_hw_data, hw_data)
> > > > >
> > > > >  struct rzg2l_pll5_param {
> > > > > @@ -168,6 +174,7 @@ struct rzg2l_pll5_mux_dsi_div_param {
> > > > >   * @info: Pointer to platform data
> > > > >   * @genpd: PM domain
> > > > >   * @mux_dsi_div_params: pll5 mux and dsi div parameters
> > > > > + * @crit_clk_hw_head: Head of the linked list critical clk
> > > > > + entries
> > > > >   */
> > > > >  struct rzg2l_cpg_priv {
> > > > >         struct reset_controller_dev rcdev; @@ -186,8 +193,26 @@
> > > > > struct rzg2l_cpg_priv {
> > > > >         struct generic_pm_domain genpd;
> > > > >
> > > > >         struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
> > > > > +
> > > > > +       struct rzg2l_crit_clk_hw *crit_clk_hw_head;
> > > > >  };
> > > > >
> > > > > +static int rzg2l_cpg_add_crit_clk_hw_entry(struct rzg2l_cpg_priv *priv,
> > > > > +                                          struct clk_hw *hw) {
> > > > > +       struct rzg2l_crit_clk_hw *node;
> > > > > +
> > > > > +       node = devm_kzalloc(priv->dev, sizeof(*node),
> > > > > + GFP_KERNEL);
> > > >
> > > > This ends up allocating quite some memory to store just a single
> > > > clk_hw pointer.   Alternatively, you could use an array and size,
> > > > and grow that using devm_krealloc().
> > >
> > > Upon second thought, you already know how many there are upfront,
> > > thanks to rzg2l_cpg_info.num_crit_mod_clks? You even already have an array (but it's __initconst).
> > >
> > > > Another alternative would be saving and restoring all clocks
> > > > during suspend/resume, like renesas-cpg-mssr.c does.
> > >
> > > Another alternative: rzg2l_mod_clock_init_mstop() already iterates
> > > over all module clocks during resume, so it could be modified to also force-enable all critical
> module clocks.
> >
> > Looks this is simple. I will send next version based on this
> >
> > diff --git a/drivers/clk/renesas/rzg2l-cpg.c
> > b/drivers/clk/renesas/rzg2l-cpg.c index f9e4af7f49d0..eeafbfe3c725
> > 100644
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -1601,8 +1601,11 @@ static void rzg2l_mod_clock_init_mstop(struct rzg2l_cpg_priv *priv)
> >                  * module is in invalid HW state.
> >                  */
> >                 scoped_guard(spinlock_irqsave, &priv->rmw_lock) {
> > -                       if (!rzg2l_mod_clock_is_enabled(&clk->hw))
> > +                       if (!rzg2l_mod_clock_is_enabled(&clk->hw)) {
> > +                               if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
> > +
> > + rzg2l_mod_clock_endisable(&clk->hw, true);
> >                                 rzg2l_mod_clock_module_set_state(clk,
> > true);
> > +                       }
> >                 }
> >         }
> >  }
> 
> Looks good in principle, but there are a few gotyas:
>   1. rzg2l_mod_clock_is_enabled() does not return the hardware state
>      if there is a sibling (but that may not be an issue, I didn't check
>      the actual critical clocks),

So far there is no critical clks that has sibling. Will revisit if there is a
Critical clock that has sibling.


>   2. rzg2l_mod_clock_endisable() takes &priv->rmw_lock, so you have
>      to move it out of the scoped_guard() to avoid deadlock,

Good catch. Unfortunately, during testing there is no deadlock happened

V2L: s2idle
G3L: s2ram and s2idle


>   3. You need to move this to the top of the loop anyway, else it
>      doesn't work for critical clocks without mstop,

OK.

>   4. Probably you want to rename the function.

OK I will refactor the code as it is called from probe and resume()
Resume() needed enabling critical clocks.

I will send next version fixing all those points above.

Cheers,
Biju