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
