Message-ID: <CA+V-a8vMMwyZ0rsdzrMuhvWDXa5dMtgYXf9+6YEAm1dXYUU+DQ@mail.gmail.com>
Date: Wed, 27 Nov 2024 12:41:18 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 1/2] clk: renesas: rzv2h-cpg: Add selective Runtime PM
 support for clocks

Hi Geert,

Thank you for the review.

On Wed, Nov 27, 2024 at 9:54 AM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> s/rzv2h-cpg/rzv2h/
>
Ok, I'll update the commit header.

> On Tue, Nov 5, 2024 at 12:24 AM Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Update `rzv2h_cpg_attach_dev` to prevent external clocks not tied to the
> > power domain from being managed by Runtime PM. This ensures that only
> > clocks originating from the domain are controlled, thereby avoiding
> > unintended handling of external clocks.
> >
> > Additionally, introduce a `no_pm` flag in `mod_clock` and `rzv2h_mod_clk`
> > structures to exclude specific clocks from Runtime PM when needed. Some
> > clocks, such as those in the CRU block, require unique enable/disable
> > sequences that are incompatible with standard Runtime PM. For example,
> > the CSI-2 D-PHY clock initialization requires toggling individual clocks,
> > making Runtime PM unsuitable.
> >
> > The helper function `rzv2h_cpg_is_pm_clk()` checks whether a clock should
> > be managed by Runtime PM based on this `no_pm` flag. New macros, such as
> > `DEF_MOD_NO_PM`, allow straightforward declaration of clocks that bypass
> > PM.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > ---
> > v1->v2
> > - Updated code to skip external clocks to be controlled from runtime PM
> > - Updated id range check
> > - Updated commit message
>
> Thanks for the update!
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -668,8 +671,38 @@ struct rzv2h_cpg_pd {
> >         struct generic_pm_domain genpd;
> >  };
> >
> > +static bool rzv2h_cpg_is_pm_clk(struct rzv2h_cpg_pd *pd,
> > +                               const struct of_phandle_args *clkspec)
> > +{
> > +       if (clkspec->np != pd->genpd.dev.of_node || clkspec->args_count != 2)
> > +               return false;
> > +
> > +       switch (clkspec->args[0]) {
> > +       case CPG_MOD: {
> > +               struct rzv2h_cpg_priv *priv = pd->priv;
> > +               unsigned int id = clkspec->args[1];
> > +               struct mod_clock *clock;
> > +
> > +               if (id >= priv->num_mod_clks)
> > +                       return true;
> > +
> > +               if (priv->clks[priv->num_core_clks + id] == ERR_PTR(-ENOENT))
> > +                       return true;
>
> Shouldn't this return false for the two invalid cases above?
>
Oops, I agree.

> > +
> > +               clock = to_mod_clock(__clk_get_hw(priv->clks[priv->num_core_clks + id]));
> > +
> > +               return !clock->no_pm;
> > +       }
> > +
> > +       case CPG_CORE:
> > +       default:
> > +               return true;
>
> False? (I know the code treated it is a PM clock before)
>
OK.

> > +       }
> > +}
> > +
> >  static int rzv2h_cpg_attach_dev(struct generic_pm_domain *domain, struct device *dev)
> >  {
> > +       struct rzv2h_cpg_pd *pd = container_of(domain, struct rzv2h_cpg_pd, genpd);
> >         struct device_node *np = dev->of_node;
> >         struct of_phandle_args clkspec;
> >         bool once = true;
> > @@ -679,6 +712,12 @@ static int rzv2h_cpg_attach_dev(struct generic_pm_domain *domain, struct device
> >
> >         while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> >                                            &clkspec)) {
> > +               if (!rzv2h_cpg_is_pm_clk(pd, &clkspec)) {
> > +                       of_node_put(clkspec.np);
> > +                       i++;
> > +                       continue;
>
> This loop may start to loop nicer using
> "for (i = 0; !of_parse_phandle_with_args(...); i++)".
>
Ok, I'll switch to a for loop here.

> > +               }
> > +
> >                 if (once) {
> >                         once = false;
> >                         error = pm_clk_create(dev);
> > diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
> > index 819029c81904..0723df4c1134 100644
> > --- a/drivers/clk/renesas/rzv2h-cpg.h
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > @@ -100,6 +100,7 @@ enum clk_types {
> >   * @name: handle between common and hardware-specific interfaces
> >   * @parent: id of parent clock
> >   * @critical: flag to indicate the clock is critical
> > + * @no_pm: flag to indicate PM is not supported
> >   * @on_index: control register index
> >   * @on_bit: ON bit
> >   * @mon_index: monitor register index
> > @@ -109,17 +110,19 @@ struct rzv2h_mod_clk {
> >         const char *name;
> >         u16 parent;
> >         bool critical;
> > +       bool no_pm;
> >         u8 on_index;
> >         u8 on_bit;
> >         s8 mon_index;
> >         u8 mon_bit;
> >  };
> >
> > -#define DEF_MOD_BASE(_name, _parent, _critical, _onindex, _onbit, _monindex, _monbit) \
> > +#define DEF_MOD_BASE(_name, _parent, _critical, _no_pm, _onindex, _onbit, _monindex, _monbit) \
>
> Note that this series conflicts with "[PATCH 00/12] Add support for
> Renesas RZ/G3E SoC and SMARC-EVK platform", which you are probably
> already aware of.
>
> [1] https://lore.kernel.org/all/20241122124558.149827-1-biju.das.jz@bp.renesas.com/
>
Yep, I'll ask Biju kindly to rebase the changes on top of v3 while he
sends v2. Or do you want me to rebase on the above?

Cheers,
Prabhakar

Powered by blists - more mailing lists