[06/13] clk: keep track if a clock is explicitly configured

Commit Message

Benjamin Bara Sept. 17, 2023, 10:40 p.m. UTC

From: Benjamin Bara <[e-mail]>

When we keep track if a clock has a given rate explicitly set by a
consumer, we can identify unintentional clock rate changes in an easy
way. This also helps during debugging, as one can see if a rate is set
by accident or due to a consumer-related change.

Signed-off-by: Benjamin Bara <[e-mail]>
---
 drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
 include/linux/clk-provider.h |  1 +
 2 files changed, 26 insertions(+)

Comments

Maxime Ripard Sept. 19, 2023, 7:07 a.m. UTC | #1

Hi,

On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <[e-mail]>
> 
> When we keep track if a clock has a given rate explicitly set by a
> consumer, we can identify unintentional clock rate changes in an easy
> way. This also helps during debugging, as one can see if a rate is set
> by accident or due to a consumer-related change.
> 
> Signed-off-by: Benjamin Bara <[e-mail]>
> ---
>  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8f4f92547768..82c65ed432c5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -70,6 +70,7 @@ struct clk_core {
>  	unsigned long		rate;
>  	unsigned long		req_rate;
>  	unsigned long		new_rate;
> +	unsigned long		set_rate;

This is pretty much what req_rate is supposed to be about. Why didn't it
work in your case?

Maxime

Benjamin Bara Sept. 20, 2023, 7:22 a.m. UTC | #2

Hi Maxime!

thanks for taking the time to look through :)

On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <[e-mail]> wrote:
> On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <[e-mail]>
> >
> > When we keep track if a clock has a given rate explicitly set by a
> > consumer, we can identify unintentional clock rate changes in an easy
> > way. This also helps during debugging, as one can see if a rate is set
> > by accident or due to a consumer-related change.
> >
> > Signed-off-by: Benjamin Bara <[e-mail]>
> > ---
> >  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
> >  include/linux/clk-provider.h |  1 +
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8f4f92547768..82c65ed432c5 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -70,6 +70,7 @@ struct clk_core {
> >       unsigned long           rate;
> >       unsigned long           req_rate;
> >       unsigned long           new_rate;
> > +     unsigned long           set_rate;
>
> This is pretty much what req_rate is supposed to be about. Why didn't it
> work in your case?

I picked this one to respond first because I think some of the
implemented stuff just workarounds the current req_rate behaviour.

Currently, I have two "problems" with it:
1. It's set during initialization[1]. In this phase, the *required* rate
   isn't known yet, so it should be 0 imo.
2. It's set during re-parenting[2,3]. Also here, just because we
   re-parent, the active consumer (which set the req_rate to a valid
   value) still requires the clock to have the same rate.

That is basically the reason why we have no info if the req_rate is
really "required" by a consumer or if it is just set because the parent
had it at some time. It's only usage is here[4], which IMO doesn't
really depends on the wrong behaviour I described above.

The respective sub-tree we talk about on the imx8mp looks like this (one
example for the the LVDS-only case):
video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt)
  video_pll1_bypass (mux; 7x crtc rate)
    video_pll1_out (gate; 7x crtc rate)
      media_ldb (divider; 7x crtc rate)
        media_ldb_root_clk (gate; 7x crtc rate)
      media_disp2_pix (divider; 1x crtc rate)
        media_disp2_pix_root_clk (gate; 1x crtc rate)
      media_disp1_pix (divider; unused for now)
        media_disp1_pix_root_clk (gate; unused for now)

The problem is that the panel driver sets media_disp1_pix_root_clk,
ldb-bridge driver sets media_ldb_root_clk. All the others have a
req_rate of the rate video_pll1 had when they got initialized or
re-parented.

My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate,
IMO all clocks along the line (especially media_disp1_pix, which is
"seen" as child of the PLL, and the actual divider for
media_disp2_pix_root_clk) need to set their new rate as "required",
because the subtree below them relies on it. This might be a wrong
approach. It might be sufficient to have a req_rate only on the nodes
that actually require it. However, IMHO we need to make sure that *all*
required rates (especially the ones of leaves!) are respected after a
change. Meaning if we e.g. request video_pll1 to change again (this time
by media_ldb_root_clk), we have to ensure that media_disp2_pix_root_clk
has still the rate which has been set as req_rate before.

Ultimately, my trigger patch is also just a really bad workaround for a
new_rate != req_rate check, so I want to re-build the idea behind it
based on a differently defined req_rate. Need to take a deeper look on
that.

Thanks & regards
Benjamin

[1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L3891
[2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2726
[3] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2812
[4] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2592

Maxime Ripard Sept. 25, 2023, 3:07 p.m. UTC | #3

Hi Benjamin,

On Wed, Sep 20, 2023 at 09:22:16AM +0200, Benjamin Bara wrote:
> On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <[e-mail]> wrote:
> > On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > > From: Benjamin Bara <[e-mail]>
> > >
> > > When we keep track if a clock has a given rate explicitly set by a
> > > consumer, we can identify unintentional clock rate changes in an easy
> > > way. This also helps during debugging, as one can see if a rate is set
> > > by accident or due to a consumer-related change.
> > >
> > > Signed-off-by: Benjamin Bara <[e-mail]>
> > > ---
> > >  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  1 +
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 8f4f92547768..82c65ed432c5 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -70,6 +70,7 @@ struct clk_core {
> > >       unsigned long           rate;
> > >       unsigned long           req_rate;
> > >       unsigned long           new_rate;
> > > +     unsigned long           set_rate;
> >
> > This is pretty much what req_rate is supposed to be about. Why didn't it
> > work in your case?
> 
> I picked this one to respond first because I think some of the
> implemented stuff just workarounds the current req_rate behaviour.
> 
> Currently, I have two "problems" with it:
> 1. It's set during initialization[1]. In this phase, the *required* rate
>    isn't known yet, so it should be 0 imo.

Agreed. Ideally, it should be another value (like -1) since 0 is also
used for rates in some drivers, but that's a separate story :)

> 2. It's set during re-parenting[2,3]. Also here, just because we
>    re-parent, the active consumer (which set the req_rate to a valid
>    value) still requires the clock to have the same rate.
>
> That is basically the reason why we have no info if the req_rate is
> really "required" by a consumer or if it is just set because the parent
> had it at some time. It's only usage is here[4], which IMO doesn't
> really depends on the wrong behaviour I described above.

Ah, right.

> The respective sub-tree we talk about on the imx8mp looks like this (one
> example for the the LVDS-only case):
> video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt)
>   video_pll1_bypass (mux; 7x crtc rate)
>     video_pll1_out (gate; 7x crtc rate)
>       media_ldb (divider; 7x crtc rate)
>         media_ldb_root_clk (gate; 7x crtc rate)
>       media_disp2_pix (divider; 1x crtc rate)
>         media_disp2_pix_root_clk (gate; 1x crtc rate)
>       media_disp1_pix (divider; unused for now)
>         media_disp1_pix_root_clk (gate; unused for now)
> 
> The problem is that the panel driver sets media_disp1_pix_root_clk,
> ldb-bridge driver sets media_ldb_root_clk. All the others have a
> req_rate of the rate video_pll1 had when they got initialized or
> re-parented.

So we have only dividers, but what is the range of those? ie, could we
get away with running the video-pll1 at 297/594MHz (or a multiple of it)
and cover most of the pixel rates for LVDS?

> My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate,
> IMO all clocks along the line (especially media_disp1_pix, which is
> "seen" as child of the PLL, and the actual divider for
> media_disp2_pix_root_clk) need to set their new rate as "required",
> because the subtree below them relies on it. This might be a wrong
> approach. It might be sufficient to have a req_rate only on the nodes
> that actually require it.

That makes total sense. However, the clock framework hasn't been
designed around modifying the rate of multiple clocks in one go, which
is pretty much what you want to achieve at the moment.

You're already reaching those limits in your patches since, for example,
you kind of hardcode the tolerance the clocks consider to be ok within
the framework, which something that really belongs to each clock driver.

This is why I'm insisting in figuring out whether we can run the main
PLL at a frequency that is good enough for each use-case. That way it
doesn't have to change, you don't have to propagate anything, the
problem becomes much simpler :)

> However, IMHO we need to make sure that *all* required rates
> (especially the ones of leaves!) are respected after a change.

Part of the issue I was telling you about is that clk_set_rate never
really expressed any time duration, it's very much a fire and forget
call, so for all the CCF cares the rate could change on the very next
instruction and it would be ok.

Doing so would also introduce some subtle corner-cases, like what is
happening if cpufreq set your CPU frequency to (for example) 1GHz, but
the firmware lowered it to 600MHz for thermal throttling. What happens
then? Which rate do you consider the required rate?

This would effectively mean merging clk_set_rate with
clk_set_rate_exclusive, but the latter never really caught up because
most clocks don't care, and it's fairly inconvenient to use.

If there is an effort to be made (and I still don't believe we need to),
then I think we should put in into improving clk_set_rate_exclusive()
rather than changing the semantics of clk_set_rate().

Maxime

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8f4f92547768..82c65ed432c5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -70,6 +70,7 @@  struct clk_core {
 	unsigned long		rate;
 	unsigned long		req_rate;
 	unsigned long		new_rate;
+	unsigned long		set_rate;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
 	unsigned long		flags;
@@ -541,6 +542,12 @@  bool __clk_is_enabled(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(__clk_is_enabled);
 
+bool __clk_is_rate_set(struct clk *clk)
+{
+	return clk->core->set_rate > 0;
+}
+EXPORT_SYMBOL_GPL(__clk_is_rate_set);
+
 static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 			   unsigned long best, unsigned long flags)
 {
@@ -578,6 +585,19 @@  static bool clk_core_has_parent(struct clk_core *core, const struct clk_core *pa
 	return false;
 }
 
+static bool clk_core_is_ancestor(struct clk_core *core, const struct clk_core *ancestor)
+{
+	struct clk_core *tmp = core->parent;
+
+	while (tmp) {
+		if (tmp == ancestor)
+			return true;
+		tmp = tmp->parent;
+	}
+
+	return false;
+}
+
 static void
 clk_core_forward_rate_req(struct clk_core *core,
 			  const struct clk_rate_request *old_req,
@@ -2358,6 +2378,9 @@  static void clk_change_rate(struct clk_core *core)
 
 	trace_clk_set_rate_complete(core, core->new_rate);
 
+	if (rate_trigger_clk && clk_core_is_ancestor(rate_trigger_clk->core, core))
+		core->set_rate = core->new_rate;
+
 	core->rate = clk_recalc(core, best_parent_rate);
 
 	if (core->flags & CLK_SET_RATE_UNGATE) {
@@ -2528,6 +2551,7 @@  int clk_set_rate(struct clk *clk, unsigned long rate)
 		clk_core_rate_protect(clk->core);
 
 	rate_trigger_clk = NULL;
+	clk->core->set_rate = rate;
 
 	clk_prepare_unlock();
 
@@ -2579,6 +2603,7 @@  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	}
 
 	rate_trigger_clk = NULL;
+	clk->core->set_rate = rate;
 
 	clk_prepare_unlock();
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 3fb99ed5e8d9..e3732e0bbed9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1325,6 +1325,7 @@  bool clk_hw_is_prepared(const struct clk_hw *hw);
 bool clk_hw_rate_is_protected(const struct clk_hw *hw);
 bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
+bool __clk_is_rate_set(struct clk *hw);
 struct clk *__clk_lookup(const char *name);
 int __clk_mux_determine_rate(struct clk_hw *hw,
 			     struct clk_rate_request *req);