| Message ID | 20230918-imx8mp-dtsi-v1-6-1d008b3237c0@skidata.com |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: first clock propagation attempt (for LVDS) | expand |
Hi, On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote: > From: Benjamin Bara <benjamin.bara@skidata.com> > > 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 <benjamin.bara@skidata.com> > --- > 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
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 <mripard@kernel.org> wrote: > > On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote: > > > From: Benjamin Bara <benjamin.bara@skidata.com> > > > > > > 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 <benjamin.bara@skidata.com> > > > --- > > > 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
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);