From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com>
---
v1->v2
- Updated code to skip external clocks to be controlled from runtime PM
- Updated id range check
- Updated commit message
---
 drivers/clk/renesas/rzv2h-cpg.c | 39 +++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h | 12 +++++++---
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index b524a9d33610..9645eb84bf9d 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;
 
@@ -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;
+
+		clock = to_mod_clock(__clk_get_hw(priv->clks[priv->num_core_clks + id]));
+
+		return !clock->no_pm;
+	}
+
+	case CPG_CORE:
+	default:
+		return true;
+	}
+}
+
 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;
+		}
+
 		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.0

Hi Prabhakar,

Thanks for your patch!

s/rzv2h-cpg/rzv2h/

On Tue, Nov 5, 2024 at 12:24 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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?

> +
> +               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)

> +       }
> +}
> +
>  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++)".

> +               }
> +
>                 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/

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, Nov 27, 2024 at 9:54 AM Geert Uytterhoeven <geert@linux-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@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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

Hi Prabhakar,

On Wed, Nov 27, 2024 at 1:41 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Wed, Nov 27, 2024 at 9:54 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Nov 5, 2024 at 12:24 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com>
> > > ---
> > > v1->v2
> > > - Updated code to skip external clocks to be controlled from runtime PM
> > > - Updated id range check
> > > - Updated commit message

> > 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?

I guess your series is closer to the point of acceptance.

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