[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+V-a8tXp=z757===FgBrBCc0Hn1w8yzG-TJzwRkLuz3ttsSUA@mail.gmail.com>
Date: Mon, 4 Nov 2024 12:23:13 +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 1/2] clk: renesas: rzv2h-cpg: Add support for clocks
without PM
Hi Geert,
Thank you for the review.
On Wed, Oct 30, 2024 at 4:56 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > Allow certain CPG_MOD clocks to be excluded from Runtime PM support. Some
> > clocks, like those in the CRU block, require specific sequences for
> > enabling/disabling, making them incompatible with standard Runtime PM
> > handling. For instance, initializing the CSI-2 D-PHY involves toggling
> > individual clocks, which prevents the use of Runtime PM.
> >
> > Introduce a `no_pm` flag in the `mod_clock` and `rzv2h_mod_clk` structures
> > to indicate clocks that do not support PM. Add a helper function
> > `rzv2h_cpg_is_pm_clk()` to determine if a clock should be managed by
> > Runtime PM based on this flag. Define new macros like `DEF_MOD_NO_PM` for
> > easier specification of such clocks.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -658,6 +661,32 @@ static int rzv2h_cpg_reset_controller_register(struct rzv2h_cpg_priv *priv)
> > return devm_reset_controller_register(priv->dev, &priv->rcdev);
> > }
> >
> > +static bool rzv2h_cpg_is_pm_clk(struct rzv2h_cpg_priv *priv,
> > + const struct of_phandle_args *clkspec)
> > +{
> > + struct mod_clock *clock;
> > + struct clk_hw *hw;
> > + unsigned int id;
> > +
>
> Forgot to check clkspec->np?
> Ah, rzg2l_cpg_is_pm_clk() also lacks this.
>
I will add this check for g2l cpg too now.
> > + if (clkspec->args_count != 2)
> > + return true;
> > +
> > + id = clkspec->args[1];
> > + if (clkspec->args[0] != CPG_MOD ||
> > + id >= priv->num_core_clks + priv->num_mod_clks)
>
> Adding "priv->num_core_clks" looks wrong to me.
>
Agreed, I will drop it.
> > + return true;
> > +
> > + if (priv->clks[priv->num_core_clks + id] == ERR_PTR(-ENOENT))
> > + return true;
> > +
> > + hw = __clk_get_hw(priv->clks[priv->num_core_clks + id]);
> > + clock = to_mod_clock(hw);
> > + if (clock->no_pm)
> > + return false;
> > +
> > + return true;
>
> return !clock->no_pm;
>
Agreed.
> > +}
> > +
> > /**
> > * struct rzv2h_cpg_pd - RZ/V2H power domain data structure
> > * @priv: pointer to CPG private data structure
> > @@ -670,6 +699,8 @@ struct rzv2h_cpg_pd {
> >
> > 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 rzv2h_cpg_priv *priv = pd->priv;
> > struct device_node *np = dev->of_node;
> > struct of_phandle_args clkspec;
> > bool once = true;
> > @@ -679,6 +710,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)) {
>
> So this should have checked that clkspec.np actually points to our
> clock provider. Else all clocks (e.g. the external CAN clock, or
> external audio clocks) are controlled through Runtime PM.
>
Agreed, I had missed this case, thanks.
Cheers,
Prabhakar
Powered by blists - more mailing lists