diff mbox series

[1/2] clk: renesas: rzv2h-cpg: Add support for clocks without PM

Message ID 20241028212914.1057715-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series clk: renesas: rzv2h-cpg: Add CRU and CSI clocks | expand

Commit Message

Lad, Prabhakar Oct. 28, 2024, 9:29 p.m. UTC 
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(-)

Comments

Geert Uytterhoeven Oct. 30, 2024, 4:56 p.m. UTC | #1 
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.

> +       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
Lad, Prabhakar Nov. 4, 2024, 12:23 p.m. UTC | #2 
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
diff mbox series

Patch

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