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

[Geert Uytterhoeven]

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),
  2. rzg2l_mod_clock_endisable() takes &priv->rmw_lock, so you have
     to move it out of the scoped_guard() to avoid deadlock,
  3. You need to move this to the top of the loop anyway, else it
     doesn't work for critical clocks without mstop,
  4. Probably you want to rename the function.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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