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

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Oct 25, 2022 at 9:56 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> 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().
>
Agreed.

> 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?
>
With the below change suggested info will be used more than once
(3-times), so I'll keep it.

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

Cheers,
Prabhakar