Hi Geert,
Thank you for the review.
On Wed, Nov 27, 2024 at 9:54 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx/
>
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
