diff mbox series

clk: clk-conf: bypass setting rate/parent if already same

Message ID 20231026063941.1882023-1-peng.fan@oss.nxp.com (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: clk-conf: bypass setting rate/parent if already same | expand

Commit Message

Peng Fan (OSS) Oct. 26, 2023, 6:39 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

If the original rate and parent is already the same as what users
wanna to configure through assigned clock rate and parent, there
is no need to configure them again which may cause more cpu cycles
or more SCMI RPC calls.

So check the rate and parent first, and bypass when the original
rate and parent are same as requested by device tree node.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk-conf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Oct. 26, 2023, 6:53 a.m. UTC | #1 
Hi Peng,

On Thu, Oct 26, 2023 at 8:35 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> If the original rate and parent is already the same as what users
> wanna to configure through assigned clock rate and parent, there
> is no need to configure them again which may cause more cpu cycles
> or more SCMI RPC calls.
>
> So check the rate and parent first, and bypass when the original
> rate and parent are same as requested by device tree node.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Thanks for your patch!

> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -65,7 +65,11 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>                         goto err;
>                 }
>
> -               rc = clk_set_parent(clk, pclk);
> +               if (__clk_get_hw(pclk) != __clk_get_hw(clk_get_parent(clk)))
> +                       rc = clk_set_parent(clk, pclk);
> +               else
> +                       rc = 0;

The else branch is not needed, as rc already indicates a non-error
condition.

> +
>                 if (rc < 0)
>                         pr_err("clk: failed to reparent %s to %s: %d\n",
>                                __clk_get_name(clk), __clk_get_name(pclk), rc);
> @@ -112,7 +116,10 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>                                 return PTR_ERR(clk);
>                         }
>
> -                       rc = clk_set_rate(clk, rate);
> +                       if (clk_get_rate(clk) != rate)
> +                               rc = clk_set_rate(clk, rate);
> +                       else
> +                               rc = 0;

Likewise.

>                         if (rc < 0)
>                                 pr_err("clk: couldn't set %s clk rate to %u (%d), current rate: %lu\n",
>                                        __clk_get_name(clk), rate, rc,

However, one can wonder whether this check should be done in
clk_set_parent() resp. clk_set_rate() instead, so it would benefit
all callers?

Also:

    /**
     * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
     *                This is only valid once the clock source has been enabled.
     * @clk: clock source
     */

So at least the second part is not guaranteed to work?

Gr{oetje,eeting}s,

                        Geert
Peng Fan Oct. 26, 2023, 8:01 a.m. UTC | #2 
> Subject: Re: [PATCH] clk: clk-conf: bypass setting rate/parent if already same
> 
> Hi Peng,
> 
> On Thu, Oct 26, 2023 at 8:35 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > If the original rate and parent is already the same as what users
> > wanna to configure through assigned clock rate and parent, there is no
> > need to configure them again which may cause more cpu cycles or more
> > SCMI RPC calls.
> >
> > So check the rate and parent first, and bypass when the original rate
> > and parent are same as requested by device tree node.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/clk-conf.c
> > +++ b/drivers/clk/clk-conf.c
> > @@ -65,7 +65,11 @@ static int __set_clk_parents(struct device_node
> *node, bool clk_supplier)
> >                         goto err;
> >                 }
> >
> > -               rc = clk_set_parent(clk, pclk);
> > +               if (__clk_get_hw(pclk) != __clk_get_hw(clk_get_parent(clk)))
> > +                       rc = clk_set_parent(clk, pclk);
> > +               else
> > +                       rc = 0;
> 
> The else branch is not needed, as rc already indicates a non-error condition.
> 
> > +
> >                 if (rc < 0)
> >                         pr_err("clk: failed to reparent %s to %s: %d\n",
> >                                __clk_get_name(clk),
> > __clk_get_name(pclk), rc); @@ -112,7 +116,10 @@ static int
> __set_clk_rates(struct device_node *node, bool clk_supplier)
> >                                 return PTR_ERR(clk);
> >                         }
> >
> > -                       rc = clk_set_rate(clk, rate);
> > +                       if (clk_get_rate(clk) != rate)
> > +                               rc = clk_set_rate(clk, rate);
> > +                       else
> > +                               rc = 0;
> 
> Likewise.
> 
> >                         if (rc < 0)
> >                                 pr_err("clk: couldn't set %s clk rate to %u (%d), current
> rate: %lu\n",
> >                                        __clk_get_name(clk), rate, rc,
> 
> However, one can wonder whether this check should be done in
> clk_set_parent() resp. clk_set_rate() instead, so it would benefit all callers?

Yeah, I could do that, but first Let's see whether Stephen is ok with
your suggestion or not.

> 
> Also:
> 
>     /**
>      * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>      *                This is only valid once the clock source has been enabled.
>      * @clk: clock source
>      */
> 
> So at least the second part is not guaranteed to work?

I am not sure, but seems there is no clk enabled check in clk_get_rate
function.

Thanks,
Peng.

> 
> 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
Geert Uytterhoeven Oct. 26, 2023, 8:03 a.m. UTC | #3 
Hi Peng,

On Thu, Oct 26, 2023 at 10:01 AM Peng Fan <peng.fan@nxp.com> wrote:
> > Subject: Re: [PATCH] clk: clk-conf: bypass setting rate/parent if already same
> > On Thu, Oct 26, 2023 at 8:35 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > If the original rate and parent is already the same as what users
> > > wanna to configure through assigned clock rate and parent, there is no
> > > need to configure them again which may cause more cpu cycles or more
> > > SCMI RPC calls.
> > >
> > > So check the rate and parent first, and bypass when the original rate
> > > and parent are same as requested by device tree node.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>

> >     /**
> >      * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> >      *                This is only valid once the clock source has been enabled.
> >      * @clk: clock source
> >      */
> >
> > So at least the second part is not guaranteed to work?
>
> I am not sure, but seems there is no clk enabled check in clk_get_rate
> function.

There is indeed no such check.  On most hardware, clk_get_rate()
works fine when the clock is disabled, but that is not guaranteed to
work everywhere.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index 1a4e6340f95c..c9ff4fcc8875 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -65,7 +65,11 @@  static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			goto err;
 		}
 
-		rc = clk_set_parent(clk, pclk);
+		if (__clk_get_hw(pclk) != __clk_get_hw(clk_get_parent(clk)))
+			rc = clk_set_parent(clk, pclk);
+		else
+			rc = 0;
+
 		if (rc < 0)
 			pr_err("clk: failed to reparent %s to %s: %d\n",
 			       __clk_get_name(clk), __clk_get_name(pclk), rc);
@@ -112,7 +116,10 @@  static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 				return PTR_ERR(clk);
 			}
 
-			rc = clk_set_rate(clk, rate);
+			if (clk_get_rate(clk) != rate)
+				rc = clk_set_rate(clk, rate);
+			else
+				rc = 0;
 			if (rc < 0)
 				pr_err("clk: couldn't set %s clk rate to %u (%d), current rate: %lu\n",
 				       __clk_get_name(clk), rate, rc,