From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com>
---
drivers/clk/renesas/rzv2h-cpg.c | 37 +++++++++++++++++++++++++++++++++
drivers/clk/renesas/rzv2h-cpg.h | 12 ++++++++---
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index b524a9d33610..ed45dbc1cc40 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -98,6 +98,7 @@ struct pll_clk {
*
* @priv: CPG private data
* @hw: handle between common and hardware-specific interfaces
+ * @no_pm: flag to indicate PM is not supported
* @on_index: register offset
* @on_bit: ON/MON bit
* @mon_index: monitor register offset
@@ -106,6 +107,7 @@ struct pll_clk {
struct mod_clock {
struct rzv2h_cpg_priv *priv;
struct clk_hw hw;
+ bool no_pm;
u8 on_index;
u8 on_bit;
s8 mon_index;
@@ -541,6 +543,7 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
clock->on_bit = mod->on_bit;
clock->mon_index = mod->mon_index;
clock->mon_bit = mod->mon_bit;
+ clock->no_pm = mod->no_pm;
clock->priv = priv;
clock->hw.init = &init;
@@ -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;
+
+ 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)
+ 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;
+}
+
/**
* 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)) {
+ if (!rzv2h_cpg_is_pm_clk(priv, &clkspec)) {
+ of_node_put(clkspec.np);
+ i++;
+ continue;
+ }
+
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) \
{ \
.name = (_name), \
.parent = (_parent), \
.critical = (_critical), \
+ .no_pm = (_no_pm), \
.on_index = (_onindex), \
.on_bit = (_onbit), \
.mon_index = (_monindex), \
@@ -127,10 +130,13 @@ struct rzv2h_mod_clk {
}
#define DEF_MOD(_name, _parent, _onindex, _onbit, _monindex, _monbit) \
- DEF_MOD_BASE(_name, _parent, false, _onindex, _onbit, _monindex, _monbit)
+ DEF_MOD_BASE(_name, _parent, false, false, _onindex, _onbit, _monindex, _monbit)
#define DEF_MOD_CRITICAL(_name, _parent, _onindex, _onbit, _monindex, _monbit) \
- DEF_MOD_BASE(_name, _parent, true, _onindex, _onbit, _monindex, _monbit)
+ DEF_MOD_BASE(_name, _parent, true, false, _onindex, _onbit, _monindex, _monbit)
+
+#define DEF_MOD_NO_PM(_name, _parent, _onindex, _onbit, _monindex, _monbit) \
+ DEF_MOD_BASE(_name, _parent, false, true, _onindex, _onbit, _monindex, _monbit)
/**
* struct rzv2h_reset - Reset definitions
--
2.43.0Hi Prabhakar,
On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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.
> + 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.
> + 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;
> +}
> +
> /**
> * 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.
> + if (!rzv2h_cpg_is_pm_clk(priv, &clkspec)) {
> + of_node_put(clkspec.np);
> + i++;
> + continue;
> + }
> +
> if (once) {
> once = false;
> error = pm_clk_create(dev);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
Thank you for the review.
On Wed, Oct 30, 2024 at 4:56 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Oct 28, 2024 at 10:29 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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
© 2016 - 2024 Red Hat, Inc.