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 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.

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