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: 18 March 2026 15:07
> 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 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);
+			}
 		}
 	}
 }

Cheers,
Biju