Re: [PATCH 1/2] clk: renesas: rzg2l: Don't assume all CPG_MOD clocks support PM

[Geert Uytterhoeven]

Hi Prabhakar,

On Thu, Sep 29, 2022 at 8:51 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> There are cases where not all CPG_MOD clocks should be assumed to support
> PM. For example on the CRU block there is a particular sequence that needs
> to be followed to initialize the CSI-2 D-PHY in which individual clocks
> need to be turned ON/OFF, due to which Runtime PM support wasn't used by
> the CRU CSI-2 driver.
>
> This patch adds support to allow indicating if PM is supported by the
> CPG_MOD clocks. A new macro is DEF_NO_PM() is added which sets the no_pm
> flag in struct rzg2l_mod_clk and when the driver uses Runtime PM support
> no_pm flag is checked to see if the clk needs to included as part of
> Runtime PM.
>
> CPG_MOD clocks with no_pm flag set need to be individually turned ON/OFF
> depending on the requirement of the driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> RFC->v1
> * Added no_pm_mod_clks and num_no_pm_mod_clks members as part of
>   struct rzg2l_cpg_priv

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -108,12 +108,16 @@ struct rzg2l_cpg_priv {
>         unsigned int num_mod_clks;
>         unsigned int num_resets;
>         unsigned int last_dt_core_clk;
> +       int *no_pm_mod_clks;
> +       unsigned int num_no_pm_mod_clks;
>
>         const struct rzg2l_cpg_info *info;
>
>         struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
>  };
>
> +static struct rzg2l_cpg_priv *rzg2l_cpg_priv;

I think you can do without this, by moving the currently separately
allocated struct generic_pm_domain into struct rzg2l_cpg_priv,
and using container_of() on the currently unused pointer passed to
rzg2l_cpg_attach_dev().

Note to self: get rid of the cpg_mssr_clk_domain static pointer in
the CPG/MSSR driver.

> +
>  static void rzg2l_cpg_del_clk_provider(void *data)
>  {
>         of_clk_del_provider(data);
> @@ -1225,16 +1229,24 @@ static int rzg2l_cpg_reset_controller_register(struct rzg2l_cpg_priv *priv)
>
>  static bool rzg2l_cpg_is_pm_clk(const struct of_phandle_args *clkspec)
>  {
> +       struct rzg2l_cpg_priv *priv = rzg2l_cpg_priv;
> +       const struct rzg2l_cpg_info *info = priv->info;

info is used only once, expand user below?

> +       unsigned int id;
> +       unsigned int i;
> +
>         if (clkspec->args_count != 2)
>                 return false;
>
> -       switch (clkspec->args[0]) {
> -       case CPG_MOD:
> -               return true;
> -
> -       default:
> +       if (clkspec->args[0] != CPG_MOD)
>                 return false;
> +
> +       id = clkspec->args[1] + info->num_total_core_clks;
> +       for (i = 0; i < priv->num_no_pm_mod_clks; i++) {
> +               if (priv->no_pm_mod_clks[i] == id)
> +                       return false;
>         }
> +
> +       return true;
>  }
>
>  static int rzg2l_cpg_attach_dev(struct generic_pm_domain *unused, struct device *dev)

> @@ -1366,8 +1379,26 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
>         for (i = 0; i < info->num_core_clks; i++)
>                 rzg2l_cpg_register_core_clk(&info->core_clks[i], info, priv);
>
> -       for (i = 0; i < info->num_mod_clks; i++)
> +       priv->num_no_pm_mod_clks = 0;
> +       for (i = 0; i < info->num_mod_clks; i++) {
> +               if (info->mod_clks[i].no_pm)
> +                       priv->num_no_pm_mod_clks++;
> +       }
> +
> +       if (priv->num_no_pm_mod_clks && info->num_mod_clks) {
> +               priv->no_pm_mod_clks =
> +                               devm_kmalloc_array(dev, priv->num_no_pm_mod_clks,
> +                                                  sizeof(info->mod_clks[0].id),
> +                                                  GFP_KERNEL);
> +               if (!priv->no_pm_mod_clks)
> +                       return -ENOMEM;
> +       }
> +
> +       for (i = 0, j = 0; i < info->num_mod_clks; i++) {
> +               if (info->mod_clks[i].no_pm)
> +                       priv->no_pm_mod_clks[j++] = info->mod_clks[i].id;
>                 rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
> +       }

Alternatively, you could have a separate rzg2l_cpg_info.no_pm_clks[]
array, like .crit_mod_clks[], and do the counting etc. at compile-time.

>
>         error = of_clk_add_provider(np, rzg2l_cpg_clk_src_twocell_get, priv);
>         if (error)

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