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

Another alternative would be saving and restoring all clocks during
suspend/resume, like renesas-cpg-mssr.c does.
Thoughts?

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