diff mbox series

[1/2] clk: renesas: rzg2l: Don't assume all CPG_MOD clocks support PM

Message ID 20220929185119.335273-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: renesas: RZ/G2L: Add support for no PM clocks | expand

Commit Message

Lad, Prabhakar Sept. 29, 2022, 6:51 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

There are cases where not all CPG_MOD clocks should be assumed to support
PM. For example on the CRU block there is a particular sequence that needs
to be followed to initialize the CSI-2 D-PHY in which individual clocks
need to be turned ON/OFF, due to which Runtime PM support wasn't used by
the CRU CSI-2 driver.

This patch adds support to allow indicating if PM is supported by the
CPG_MOD clocks. A new macro is DEF_NO_PM() is added which sets the no_pm
flag in struct rzg2l_mod_clk and when the driver uses Runtime PM support
no_pm flag is checked to see if the clk needs to included as part of
Runtime PM.

CPG_MOD clocks with no_pm flag set need to be individually turned ON/OFF
depending on the requirement of the driver.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC->v1
* Added no_pm_mod_clks and num_no_pm_mod_clks members as part of
  struct rzg2l_cpg_priv
---
 drivers/clk/renesas/rzg2l-cpg.c | 45 ++++++++++++++++++++++++++++-----
 drivers/clk/renesas/rzg2l-cpg.h | 12 ++++++---
 2 files changed, 47 insertions(+), 10 deletions(-)

Comments

Geert Uytterhoeven Oct. 25, 2022, 8:55 a.m. UTC | #1 
Hi Prabhakar,

On Thu, Sep 29, 2022 at 8:51 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> There are cases where not all CPG_MOD clocks should be assumed to support
> PM. For example on the CRU block there is a particular sequence that needs
> to be followed to initialize the CSI-2 D-PHY in which individual clocks
> need to be turned ON/OFF, due to which Runtime PM support wasn't used by
> the CRU CSI-2 driver.
>
> This patch adds support to allow indicating if PM is supported by the
> CPG_MOD clocks. A new macro is DEF_NO_PM() is added which sets the no_pm
> flag in struct rzg2l_mod_clk and when the driver uses Runtime PM support
> no_pm flag is checked to see if the clk needs to included as part of
> Runtime PM.
>
> CPG_MOD clocks with no_pm flag set need to be individually turned ON/OFF
> depending on the requirement of the driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v1
> * Added no_pm_mod_clks and num_no_pm_mod_clks members as part of
>   struct rzg2l_cpg_priv

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -108,12 +108,16 @@ struct rzg2l_cpg_priv {
>         unsigned int num_mod_clks;
>         unsigned int num_resets;
>         unsigned int last_dt_core_clk;
> +       int *no_pm_mod_clks;
> +       unsigned int num_no_pm_mod_clks;
>
>         const struct rzg2l_cpg_info *info;
>
>         struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
>  };
>
> +static struct rzg2l_cpg_priv *rzg2l_cpg_priv;

I think you can do without this, by moving the currently separately
allocated struct generic_pm_domain into struct rzg2l_cpg_priv,
and using container_of() on the currently unused pointer passed to
rzg2l_cpg_attach_dev().

Note to self: get rid of the cpg_mssr_clk_domain static pointer in
the CPG/MSSR driver.

> +
>  static void rzg2l_cpg_del_clk_provider(void *data)
>  {
>         of_clk_del_provider(data);
> @@ -1225,16 +1229,24 @@ static int rzg2l_cpg_reset_controller_register(struct rzg2l_cpg_priv *priv)
>
>  static bool rzg2l_cpg_is_pm_clk(const struct of_phandle_args *clkspec)
>  {
> +       struct rzg2l_cpg_priv *priv = rzg2l_cpg_priv;
> +       const struct rzg2l_cpg_info *info = priv->info;

info is used only once, expand user below?

> +       unsigned int id;
> +       unsigned int i;
> +
>         if (clkspec->args_count != 2)
>                 return false;
>
> -       switch (clkspec->args[0]) {
> -       case CPG_MOD:
> -               return true;
> -
> -       default:
> +       if (clkspec->args[0] != CPG_MOD)
>                 return false;
> +
> +       id = clkspec->args[1] + info->num_total_core_clks;
> +       for (i = 0; i < priv->num_no_pm_mod_clks; i++) {
> +               if (priv->no_pm_mod_clks[i] == id)
> +                       return false;
>         }
> +
> +       return true;
>  }
>
>  static int rzg2l_cpg_attach_dev(struct generic_pm_domain *unused, struct device *dev)

> @@ -1366,8 +1379,26 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
>         for (i = 0; i < info->num_core_clks; i++)
>                 rzg2l_cpg_register_core_clk(&info->core_clks[i], info, priv);
>
> -       for (i = 0; i < info->num_mod_clks; i++)
> +       priv->num_no_pm_mod_clks = 0;
> +       for (i = 0; i < info->num_mod_clks; i++) {
> +               if (info->mod_clks[i].no_pm)
> +                       priv->num_no_pm_mod_clks++;
> +       }
> +
> +       if (priv->num_no_pm_mod_clks && info->num_mod_clks) {
> +               priv->no_pm_mod_clks =
> +                               devm_kmalloc_array(dev, priv->num_no_pm_mod_clks,
> +                                                  sizeof(info->mod_clks[0].id),
> +                                                  GFP_KERNEL);
> +               if (!priv->no_pm_mod_clks)
> +                       return -ENOMEM;
> +       }
> +
> +       for (i = 0, j = 0; i < info->num_mod_clks; i++) {
> +               if (info->mod_clks[i].no_pm)
> +                       priv->no_pm_mod_clks[j++] = info->mod_clks[i].id;
>                 rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
> +       }

Alternatively, you could have a separate rzg2l_cpg_info.no_pm_clks[]
array, like .crit_mod_clks[], and do the counting etc. at compile-time.

>
>         error = of_clk_add_provider(np, rzg2l_cpg_clk_src_twocell_get, priv);
>         if (error)

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
Lad, Prabhakar Oct. 26, 2022, 12:33 a.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Tue, Oct 25, 2022 at 9:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Thu, Sep 29, 2022 at 8:51 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > There are cases where not all CPG_MOD clocks should be assumed to support
> > PM. For example on the CRU block there is a particular sequence that needs
> > to be followed to initialize the CSI-2 D-PHY in which individual clocks
> > need to be turned ON/OFF, due to which Runtime PM support wasn't used by
> > the CRU CSI-2 driver.
> >
> > This patch adds support to allow indicating if PM is supported by the
> > CPG_MOD clocks. A new macro is DEF_NO_PM() is added which sets the no_pm
> > flag in struct rzg2l_mod_clk and when the driver uses Runtime PM support
> > no_pm flag is checked to see if the clk needs to included as part of
> > Runtime PM.
> >
> > CPG_MOD clocks with no_pm flag set need to be individually turned ON/OFF
> > depending on the requirement of the driver.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v1
> > * Added no_pm_mod_clks and num_no_pm_mod_clks members as part of
> >   struct rzg2l_cpg_priv
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -108,12 +108,16 @@ struct rzg2l_cpg_priv {
> >         unsigned int num_mod_clks;
> >         unsigned int num_resets;
> >         unsigned int last_dt_core_clk;
> > +       int *no_pm_mod_clks;
> > +       unsigned int num_no_pm_mod_clks;
> >
> >         const struct rzg2l_cpg_info *info;
> >
> >         struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
> >  };
> >
> > +static struct rzg2l_cpg_priv *rzg2l_cpg_priv;
>
> I think you can do without this, by moving the currently separately
> allocated struct generic_pm_domain into struct rzg2l_cpg_priv,
> and using container_of() on the currently unused pointer passed to
> rzg2l_cpg_attach_dev().
>
Agreed.

> Note to self: get rid of the cpg_mssr_clk_domain static pointer in
> the CPG/MSSR driver.
>
> > +
> >  static void rzg2l_cpg_del_clk_provider(void *data)
> >  {
> >         of_clk_del_provider(data);
> > @@ -1225,16 +1229,24 @@ static int rzg2l_cpg_reset_controller_register(struct rzg2l_cpg_priv *priv)
> >
> >  static bool rzg2l_cpg_is_pm_clk(const struct of_phandle_args *clkspec)
> >  {
> > +       struct rzg2l_cpg_priv *priv = rzg2l_cpg_priv;
> > +       const struct rzg2l_cpg_info *info = priv->info;
>
> info is used only once, expand user below?
>
With the below change suggested info will be used more than once
(3-times), so I'll keep it.

> > +       unsigned int id;
> > +       unsigned int i;
> > +
> >         if (clkspec->args_count != 2)
> >                 return false;
> >
> > -       switch (clkspec->args[0]) {
> > -       case CPG_MOD:
> > -               return true;
> > -
> > -       default:
> > +       if (clkspec->args[0] != CPG_MOD)
> >                 return false;
> > +
> > +       id = clkspec->args[1] + info->num_total_core_clks;
> > +       for (i = 0; i < priv->num_no_pm_mod_clks; i++) {
> > +               if (priv->no_pm_mod_clks[i] == id)
> > +                       return false;
> >         }
> > +
> > +       return true;
> >  }
> >
> >  static int rzg2l_cpg_attach_dev(struct generic_pm_domain *unused, struct device *dev)
>
> > @@ -1366,8 +1379,26 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
> >         for (i = 0; i < info->num_core_clks; i++)
> >                 rzg2l_cpg_register_core_clk(&info->core_clks[i], info, priv);
> >
> > -       for (i = 0; i < info->num_mod_clks; i++)
> > +       priv->num_no_pm_mod_clks = 0;
> > +       for (i = 0; i < info->num_mod_clks; i++) {
> > +               if (info->mod_clks[i].no_pm)
> > +                       priv->num_no_pm_mod_clks++;
> > +       }
> > +
> > +       if (priv->num_no_pm_mod_clks && info->num_mod_clks) {
> > +               priv->no_pm_mod_clks =
> > +                               devm_kmalloc_array(dev, priv->num_no_pm_mod_clks,
> > +                                                  sizeof(info->mod_clks[0].id),
> > +                                                  GFP_KERNEL);
> > +               if (!priv->no_pm_mod_clks)
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       for (i = 0, j = 0; i < info->num_mod_clks; i++) {
> > +               if (info->mod_clks[i].no_pm)
> > +                       priv->no_pm_mod_clks[j++] = info->mod_clks[i].id;
> >                 rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
> > +       }
>
> Alternatively, you could have a separate rzg2l_cpg_info.no_pm_clks[]
> array, like .crit_mod_clks[], and do the counting etc. at compile-time.
>
Agreed.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 3ff6ecd61756..431697a37692 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -108,12 +108,16 @@  struct rzg2l_cpg_priv {
 	unsigned int num_mod_clks;
 	unsigned int num_resets;
 	unsigned int last_dt_core_clk;
+	int *no_pm_mod_clks;
+	unsigned int num_no_pm_mod_clks;
 
 	const struct rzg2l_cpg_info *info;
 
 	struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
 };
 
+static struct rzg2l_cpg_priv *rzg2l_cpg_priv;
+
 static void rzg2l_cpg_del_clk_provider(void *data)
 {
 	of_clk_del_provider(data);
@@ -1225,16 +1229,24 @@  static int rzg2l_cpg_reset_controller_register(struct rzg2l_cpg_priv *priv)
 
 static bool rzg2l_cpg_is_pm_clk(const struct of_phandle_args *clkspec)
 {
+	struct rzg2l_cpg_priv *priv = rzg2l_cpg_priv;
+	const struct rzg2l_cpg_info *info = priv->info;
+	unsigned int id;
+	unsigned int i;
+
 	if (clkspec->args_count != 2)
 		return false;
 
-	switch (clkspec->args[0]) {
-	case CPG_MOD:
-		return true;
-
-	default:
+	if (clkspec->args[0] != CPG_MOD)
 		return false;
+
+	id = clkspec->args[1] + info->num_total_core_clks;
+	for (i = 0; i < priv->num_no_pm_mod_clks; i++) {
+		if (priv->no_pm_mod_clks[i] == id)
+			return false;
 	}
+
+	return true;
 }
 
 static int rzg2l_cpg_attach_dev(struct generic_pm_domain *unused, struct device *dev)
@@ -1330,7 +1342,7 @@  static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	const struct rzg2l_cpg_info *info;
 	struct rzg2l_cpg_priv *priv;
-	unsigned int nclks, i;
+	unsigned int nclks, i, j;
 	struct clk **clks;
 	int error;
 
@@ -1348,6 +1360,7 @@  static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
+	rzg2l_cpg_priv = priv;
 	nclks = info->num_total_core_clks + info->num_hw_mod_clks;
 	clks = devm_kmalloc_array(dev, nclks, sizeof(*clks), GFP_KERNEL);
 	if (!clks)
@@ -1366,8 +1379,26 @@  static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	for (i = 0; i < info->num_core_clks; i++)
 		rzg2l_cpg_register_core_clk(&info->core_clks[i], info, priv);
 
-	for (i = 0; i < info->num_mod_clks; i++)
+	priv->num_no_pm_mod_clks = 0;
+	for (i = 0; i < info->num_mod_clks; i++) {
+		if (info->mod_clks[i].no_pm)
+			priv->num_no_pm_mod_clks++;
+	}
+
+	if (priv->num_no_pm_mod_clks && info->num_mod_clks) {
+		priv->no_pm_mod_clks =
+				devm_kmalloc_array(dev, priv->num_no_pm_mod_clks,
+						   sizeof(info->mod_clks[0].id),
+						   GFP_KERNEL);
+		if (!priv->no_pm_mod_clks)
+			return -ENOMEM;
+	}
+
+	for (i = 0, j = 0; i < info->num_mod_clks; i++) {
+		if (info->mod_clks[i].no_pm)
+			priv->no_pm_mod_clks[j++] = info->mod_clks[i].id;
 		rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
+	}
 
 	error = of_clk_add_provider(np, rzg2l_cpg_clk_src_twocell_get, priv);
 	if (error)
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index cecbdf5e4f93..1d68d3838392 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -176,6 +176,7 @@  enum clk_types {
  * @off: register offset
  * @bit: ON/MON bit
  * @is_coupled: flag to indicate coupled clock
+ * @no_pm: flag to indicate if clock doesn't support PM
  */
 struct rzg2l_mod_clk {
 	const char *name;
@@ -184,9 +185,10 @@  struct rzg2l_mod_clk {
 	u16 off;
 	u8 bit;
 	bool is_coupled;
+	bool no_pm;
 };
 
-#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)	\
+#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled, _no_pm)	\
 	{ \
 		.name = _name, \
 		.id = MOD_CLK_BASE + (_id), \
@@ -194,13 +196,17 @@  struct rzg2l_mod_clk {
 		.off = (_off), \
 		.bit = (_bit), \
 		.is_coupled = (_is_coupled), \
+		.no_pm = (_no_pm), \
 	}
 
 #define DEF_MOD(_name, _id, _parent, _off, _bit)	\
-	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false, false)
 
 #define DEF_COUPLED(_name, _id, _parent, _off, _bit)	\
-	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true, false)
+
+#define DEF_NO_PM(_name, _id, _parent, _off, _bit)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false, true)
 
 /**
  * struct rzg2l_reset - Reset definitions