Hi!
This is a spin-off of my initial series[1] with the core-related parts
picked out. Without the core part, the rest of the series only partly
makes sense, therefore I wanted to clarify the state of this first.
That's also the reason why it is marked as RFC for now.
Background:
The CCF is currently very rigid in terms of dealing with multiple rate
changes in a single clk_set_rate() call. However, with the
CLK_SET_RATE_PARENT property, it is very likely that a shared clock has
a couple of children which are changed "by accident" when the common
parent is changed. These children might be clock inputs of hardware
modules, which might have set a required rate previously. These required
rates are most likely still expected after the parent has been changed
by another clock (e.g. a sibling). Currently, it is not very trivial to
get these required rates inside of a clock driver's
{determine,round}_rate() op. Therefore, I think the core should also
participate in the process of ensuring that consumer-set requirements
are still fulfilled after a rate has changed.
Idea:
The idea is to have three rates set per clock, which need to be
considered during the whole process:
1. req_rate: This is the rate required by a consumer. It is set during a
clk_set_rate() call.
2. new_rate: This is the "new planned rate" for the clock, which it will
set, once the "finding new rates" process is finished.
3. req_rate_tmp: This rate is set if the clock is "required to change"
during the process. It basically means that the clock is an ancestor
of a rate-change trigger.
With these available, the core is able to validate the changed subtree
in a more simple way. It also allows us to re-enter calc_new_rates(),
which is one of the key components of clk_set_rate().
Thanks & regards
Benjamin
[1] https://lore.kernel.org/lkml/[email protected]/
---
Benjamin Bara (4):
clk: only set req_rate if it is set by consumer
clk: reset new_rates for next run
clk: introduce req_rate_tmp
clk: consider rates when calculating subtree
drivers/clk/clk.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 107 insertions(+), 18 deletions(-)
---
base-commit: df964ce9ef9fea10cf131bf6bad8658fde7956f6
change-id: 20230927-ccf-set-multiple-3c291416fc98
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
Currently, the req_rate is set during initialization and during
re-parenting. Therefore, it is not clear whether the req_rate is really
required by a consumer or just set by accident. Fix this by only setting
the req_rate when it is really required (clk_set_rate() is called by
consumer).
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/clk/clk.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..82f954121e4d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -24,6 +24,8 @@
#include "clk.h"
+#define CLK_RATE_UNSET -1UL
+
static DEFINE_SPINLOCK(enable_lock);
static DEFINE_MUTEX(prepare_lock);
@@ -1832,7 +1834,6 @@ static unsigned long clk_recalc(struct clk_core *core,
/**
* __clk_recalc_rates
* @core: first clk in the subtree
- * @update_req: Whether req_rate should be updated with the new rate
* @msg: notification type (see include/linux/clk.h)
*
* Walks the subtree of clks starting with clk and recalculates rates as it
@@ -1842,8 +1843,7 @@ static unsigned long clk_recalc(struct clk_core *core,
* clk_recalc_rates also propagates the POST_RATE_CHANGE notification,
* if necessary.
*/
-static void __clk_recalc_rates(struct clk_core *core, bool update_req,
- unsigned long msg)
+static void __clk_recalc_rates(struct clk_core *core, unsigned long msg)
{
unsigned long old_rate;
unsigned long parent_rate = 0;
@@ -1857,8 +1857,6 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
parent_rate = core->parent->rate;
core->rate = clk_recalc(core, parent_rate);
- if (update_req)
- core->req_rate = core->rate;
/*
* ignore NOTIFY_STOP and NOTIFY_BAD return values for POST_RATE_CHANGE
@@ -1868,13 +1866,13 @@ static void __clk_recalc_rates(struct clk_core *core, bool update_req,
__clk_notify(core, msg, old_rate, core->rate);
hlist_for_each_entry(child, &core->children, child_node)
- __clk_recalc_rates(child, update_req, msg);
+ __clk_recalc_rates(child, msg);
}
static unsigned long clk_core_get_rate_recalc(struct clk_core *core)
{
if (core && (core->flags & CLK_GET_RATE_NOCACHE))
- __clk_recalc_rates(core, false, 0);
+ __clk_recalc_rates(core, 0);
return clk_core_get_rate_nolock(core);
}
@@ -2455,7 +2453,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
/* change the rates */
clk_change_rate(top);
- core->req_rate = req_rate;
err:
clk_pm_runtime_put(core);
@@ -2485,6 +2482,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
*/
int clk_set_rate(struct clk *clk, unsigned long rate)
{
+ unsigned long old_req_rate;
int ret;
if (!clk)
@@ -2493,6 +2491,9 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ old_req_rate = clk->core->req_rate;
+ clk->core->req_rate = rate;
+
if (clk->exclusive_count)
clk_core_rate_unprotect(clk->core);
@@ -2501,6 +2502,9 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
if (clk->exclusive_count)
clk_core_rate_protect(clk->core);
+ if (ret)
+ clk->core->req_rate = old_req_rate;
+
clk_prepare_unlock();
return ret;
@@ -2528,6 +2532,7 @@ EXPORT_SYMBOL_GPL(clk_set_rate);
*/
int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
{
+ unsigned long old_req_rate;
int ret;
if (!clk)
@@ -2536,6 +2541,9 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ old_req_rate = clk->core->req_rate;
+ clk->core->req_rate = rate;
+
/*
* The temporary protection removal is not here, on purpose
* This function is meant to be used instead of clk_rate_protect,
@@ -2546,6 +2554,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
if (!ret) {
clk_core_rate_protect(clk->core);
clk->exclusive_count++;
+ } else {
+ clk->core->req_rate = old_req_rate;
}
clk_prepare_unlock();
@@ -2723,7 +2733,7 @@ static void clk_core_reparent(struct clk_core *core,
{
clk_reparent(core, new_parent);
__clk_recalc_accuracies(core);
- __clk_recalc_rates(core, true, POST_RATE_CHANGE);
+ __clk_recalc_rates(core, POST_RATE_CHANGE);
}
void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent)
@@ -2807,9 +2817,9 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
/* propagate rate an accuracy recalculation accordingly */
if (ret) {
- __clk_recalc_rates(core, true, ABORT_RATE_CHANGE);
+ __clk_recalc_rates(core, ABORT_RATE_CHANGE);
} else {
- __clk_recalc_rates(core, true, POST_RATE_CHANGE);
+ __clk_recalc_rates(core, POST_RATE_CHANGE);
__clk_recalc_accuracies(core);
}
@@ -3706,7 +3716,7 @@ static void clk_core_reparent_orphans_nolock(void)
__clk_set_parent_before(orphan, parent);
__clk_set_parent_after(orphan, parent, NULL);
__clk_recalc_accuracies(orphan);
- __clk_recalc_rates(orphan, true, 0);
+ __clk_recalc_rates(orphan, 0);
/*
* __clk_init_parent() will set the initial req_rate to
@@ -3888,7 +3898,8 @@ static int __clk_core_init(struct clk_core *core)
rate = parent->rate;
else
rate = 0;
- core->rate = core->req_rate = rate;
+ core->rate = rate;
+ core->req_rate = CLK_RATE_UNSET;
/*
* Enable CLK_IS_CRITICAL clocks so newly added critical clocks
--
2.34.1
Hi,
On Mon, Oct 02, 2023 at 11:23:31AM +0200, Benjamin Bara wrote:
> This is a spin-off of my initial series[1] with the core-related parts
> picked out. Without the core part, the rest of the series only partly
> makes sense, therefore I wanted to clarify the state of this first.
> That's also the reason why it is marked as RFC for now.
>
> Background:
> The CCF is currently very rigid in terms of dealing with multiple rate
> changes in a single clk_set_rate() call. However, with the
> CLK_SET_RATE_PARENT property, it is very likely that a shared clock has
> a couple of children which are changed "by accident" when the common
> parent is changed. These children might be clock inputs of hardware
> modules, which might have set a required rate previously. These required
> rates are most likely still expected after the parent has been changed
> by another clock (e.g. a sibling). Currently, it is not very trivial to
> get these required rates inside of a clock driver's
> {determine,round}_rate() op. Therefore, I think the core should also
> participate in the process of ensuring that consumer-set requirements
> are still fulfilled after a rate has changed.
>
> Idea:
> The idea is to have three rates set per clock, which need to be
> considered during the whole process:
>
> 1. req_rate: This is the rate required by a consumer. It is set during a
> clk_set_rate() call.
> 2. new_rate: This is the "new planned rate" for the clock, which it will
> set, once the "finding new rates" process is finished.
> 3. req_rate_tmp: This rate is set if the clock is "required to change"
> during the process. It basically means that the clock is an ancestor
> of a rate-change trigger.
>
> With these available, the core is able to validate the changed subtree
> in a more simple way. It also allows us to re-enter calc_new_rates(),
> which is one of the key components of clk_set_rate().
There's a couple of things you didn't reply on the first version and you
didn't really expand it here:
- Most clocks don't care, and only the clocks that have used
clk_set_rate_exclusive actually care. clk_set_rate never provided
that guarantee, you're effectively merging clk_set_rate and
clk_set_rate_exclusive. This might or might not be a good idea (it's
probably not unless you want to track down regressions forever), but
we should really tie this to clk_set_rate_exclusive or merge both.
- Why do we need a new req_rate, and why req_rate can't be changed to
accomodate your changes.
- Why do you even need the core to be involved in the first place? You say you
think it does, but you haven't answered any of my mails to provide more
details and figure out if we can do it without it.
Maxime
Hi Maxime,
thank you for the feedback!
On Mon, 2 Oct 2023 at 14:27, Maxime Ripard <[email protected]> wrote:
> There's a couple of things you didn't reply on the first version and
> you didn't really expand it here:
Sorry for that, wanted to get the reduced series out first to have a
better discussion base. Planned to reply to them and link to the
spin-off later, probably should have mentioned that :/ Thanks for
summarizing!
> Most clocks don't care, and only the clocks that have used
> clk_set_rate_exclusive actually care.
I think that is one of the main points I don't understand yet... Why? I
mean, the point of calling clk_set_rate() is to get a certain rate for a
clock, right? Why should the clock not care if it is changed to
something completely different? Maybe I am a bit biased here because I
use the imx8mp as a reference. On this platform, most hardware blocks
have an own divider and therefore the clocks which are connected to the
blocks are mostly "exclusive". E.g. the tree for a panel looks like
this:
-osc_24m (oscillator)
-- video_pll1_ref_sel (mux)
--- video_pll1 (configurable; shared)
---- video_pll1_bypass (mux; shared)
----- video_pll1_out (gate; shared)
------ media_disp2_pix (divider; "panel exclusive")
------- media_disp2_pix_root_clk (gate; "panel exclusive")
-------- <PANEL>
> clk_set_rate never provided that guarantee, you're effectively merging
> clk_set_rate and clk_set_rate_exclusive.
Ah, I guess I see what you mean... Since we would error out now on a
"conflict", this becomes very close to the "exclusiveness concept".
However, what I actually try to achieve is to leave the rest of the
subtree unaffected by a change (if required and possible).
> This might or might not be a good idea (it's probably not unless you
> want to track down regressions forever), but we should really tie this
> to clk_set_rate_exclusive or merge both.
I see that the current "conflict handling" might fit very well for
clk_set_rate_exclusive(). However, I think it's pretty hard to use
clk_set_rate_exclusive() in a multi-platform driver, as the other
competing consumers are not known. But maybe it makes sense to have the
same path and decide on a conflict whether we are allowed to do the
change or not (exclusive/protected).
> Why do we need a new req_rate, and why req_rate can't be changed to
> accomodate your changes.
For me, the existing req_rate is a "persistent" rate. It is the rate a
consumer requires the clock to have. It's something typically for leaves
of the clock-tree, which are directly connected to (probably
multi-platform) clock-consuming blocks, e.g. the dividers mentioned.
The new req_rate is "temporary". It is rather important for the !leaves
and indicate that a clock is required to change during this
clk_set_rate() call, in order to fulfill the requested rate.
Short example, let's say we have something like this:
- Video PLL
-- LVDS divider
--- LVDS bridge (HW block)
-- CRTC divider
--- Panel (HW block)
From a hardware-description point of view, the CRTC divider is exclusive
to the panel and the LVDS divider exclusive to the LVDS bridge. However,
the Video PLL is not the only possible parent of both and it should also
not be set exclusively by one of them.
When a CRTC rate of 35M is required by the panel, it would be set to the
following:
- Video PLL: req_tmp=35M, req=-1, new=35M
-- LVDS divider: req_tmp=-1, req=-1, new=35M (div=1)
-- CRTC divider: req_tmp=35M, req=35M, new=35M (div=1)
Next, the LVDS bridge requires 245M, which would be a multiple of
35M. The Video PLL is configured again, this time "by" the LVDS divider:
- Video PLL: req_tmp=245M, req=-1, new=245M
-- LVDS divider: req_tmp=245M, req=245M, new=245M (div=1)
-- CRTC divider: req_tmp=-1, req=35M, new=245M (div=1)
So without additional interaction (current behaviour), we would set the
CRTC divider to 245M, which contradicts with the unchanged previous
requirement stored in req. As req_tmp == -1, we know that the new rate
of the CRTC divider is not crucial for the actual requested change (LVDS
=> 245M). Therefore, what I would like to achieve is to have some
component/process that tells the CRTC divider to set its div to 7, as
this won't affect the ongoing requested change and would restore a
required rate of a different component, which was changed "unintended".
> Why do you even need the core to be involved in the first place? You
> say you think it does, but you haven't answered any of my mails to
> provide more details and figure out if we can do it without it.
We already have this functionality (calc required new rates) inside the
core and the core currently is the only one knowing all the context
about the tree-structure and the required and new rates. So I think in
the example above, calling calc_new_rates() again, this time with the
CRTC divider and req, might be the simplest solution to the problem.
I think as the Video PLL isn't directly consumed, we don't really have a
different possibility to achieve the same outcome, except of starting
Video PLL already with 245M (e.g. via device-tree).
Just for the sake of completeness:
A "conflict" occurs if this call would try to re-configure Video PLL
again (if req_tmp is already set; by not involving req here, we
basically avoid the "exclusiveness"). IMO, there are different ways to
proceed on a conflict: A possible clk_set_rate() option would be to
ignore a potential re-change of Video PLL by the second calc_new_rates()
and just set a somewhat close to the req rate for CRTC divider. A
possible clk_set_rate_exclusive() option is the one implemented here:
error out if we cannot guarantee the existing required rates for the
rest of the subtree.
Thanks & regards
Benjamin
On Tue, Oct 03, 2023 at 09:44:07AM +0200, Benjamin Bara wrote:
> Hi Maxime,
>
> thank you for the feedback!
>
> On Mon, 2 Oct 2023 at 14:27, Maxime Ripard <[email protected]> wrote:
> > There's a couple of things you didn't reply on the first version and
> > you didn't really expand it here:
>
> Sorry for that, wanted to get the reduced series out first to have a
> better discussion base. Planned to reply to them and link to the
> spin-off later, probably should have mentioned that :/ Thanks for
> summarizing!
That's fine, don't worry :)
> > Most clocks don't care, and only the clocks that have used
> > clk_set_rate_exclusive actually care.
>
> I think that is one of the main points I don't understand yet... Why? I
> mean, the point of calling clk_set_rate() is to get a certain rate for a
> clock, right? Why should the clock not care if it is changed to
> something completely different?
Because it doesn't matter for most clocks, and doesn't really affect the
system in any way. Or there's never any conflicting rate change that
would impact them. I guess it's on a per-clock basis so it's hard to
generalize, but the fact alone that the CCF is 10+ years old and we only
have to discuss it now is kind of a proof that things mostly work out
for most cases :)
Also, some systems have likely side-stepped the entire problem by
switching different outputs to different PLLs for example if the SoCs
allow to.
And while the vast majority of those drivers don't really care about the
rate being strictly enforced, I would expect a reasonable amount of them
to check the return code of clk_set_rate(). So if you start strictly
enforcing the rate, you'll also have to fail more often because there's
configuration that you just can't handle.
And that would lead to more drivers failing for something they don't
really care about :)
> Maybe I am a bit biased here because I use the imx8mp as a reference.
> On this platform, most hardware blocks have an own divider and
> therefore the clocks which are connected to the blocks are mostly
> "exclusive". E.g. the tree for a panel looks like
> this:
> -osc_24m (oscillator)
> -- video_pll1_ref_sel (mux)
> --- video_pll1 (configurable; shared)
> ---- video_pll1_bypass (mux; shared)
> ----- video_pll1_out (gate; shared)
> ------ media_disp2_pix (divider; "panel exclusive")
> ------- media_disp2_pix_root_clk (gate; "panel exclusive")
> -------- <PANEL>
I guess that's mostly a typical clock tree for a panel. However, notice
that you only took a handful of clocks as an example, and not the 50-100
others in your system. While the panel clock rate is critical, the
register clock speed of the timers probably won't be as much.
And that's what I was mentioning really: out of the 50-100 clocks in
your system right now, you only really care about the rate of ~5, and
most of them are going to be in different subtrees.
So, while your problem is indeed a concern, it's also a corner case.
> > clk_set_rate never provided that guarantee, you're effectively merging
> > clk_set_rate and clk_set_rate_exclusive.
>
> Ah, I guess I see what you mean... Since we would error out now on a
> "conflict", this becomes very close to the "exclusiveness concept".
Yeah, exactly. clk_set_rate() gains the "I want this rate to be enforced
forever" property that clk_set_rate_exclusive() provides already.
> However, what I actually try to achieve is to leave the rest of the
> subtree unaffected by a change (if required and possible).
>
> > This might or might not be a good idea (it's probably not unless you
> > want to track down regressions forever), but we should really tie this
> > to clk_set_rate_exclusive or merge both.
>
> I see that the current "conflict handling" might fit very well for
> clk_set_rate_exclusive(). However, I think it's pretty hard to use
> clk_set_rate_exclusive() in a multi-platform driver, as the other
> competing consumers are not known.
You don't know that with clk_set_rate either though?
> But maybe it makes sense to have the same path and decide on a
> conflict whether we are allowed to do the change or not
> (exclusive/protected).
>
> > Why do we need a new req_rate, and why req_rate can't be changed to
> > accomodate your changes.
>
> For me, the existing req_rate is a "persistent" rate. It is the rate a
> consumer requires the clock to have. It's something typically for leaves
> of the clock-tree, which are directly connected to (probably
> multi-platform) clock-consuming blocks, e.g. the dividers mentioned.
> The new req_rate is "temporary". It is rather important for the !leaves
> and indicate that a clock is required to change during this
> clk_set_rate() call, in order to fulfill the requested rate.
>
> Short example, let's say we have something like this:
> - Video PLL
> -- LVDS divider
> --- LVDS bridge (HW block)
> -- CRTC divider
> --- Panel (HW block)
>
> From a hardware-description point of view, the CRTC divider is exclusive
> to the panel and the LVDS divider exclusive to the LVDS bridge. However,
> the Video PLL is not the only possible parent of both and it should also
> not be set exclusively by one of them.
>
> When a CRTC rate of 35M is required by the panel, it would be set to the
> following:
> - Video PLL: req_tmp=35M, req=-1, new=35M
> -- LVDS divider: req_tmp=-1, req=-1, new=35M (div=1)
> -- CRTC divider: req_tmp=35M, req=35M, new=35M (div=1)
>
> Next, the LVDS bridge requires 245M, which would be a multiple of
> 35M. The Video PLL is configured again, this time "by" the LVDS divider:
> - Video PLL: req_tmp=245M, req=-1, new=245M
> -- LVDS divider: req_tmp=245M, req=245M, new=245M (div=1)
> -- CRTC divider: req_tmp=-1, req=35M, new=245M (div=1)
>
> So without additional interaction (current behaviour), we would set the
> CRTC divider to 245M, which contradicts with the unchanged previous
> requirement stored in req. As req_tmp == -1, we know that the new rate
> of the CRTC divider is not crucial for the actual requested change (LVDS
> => 245M). Therefore, what I would like to achieve is to have some
> component/process that tells the CRTC divider to set its div to 7, as
> this won't affect the ongoing requested change and would restore a
> required rate of a different component, which was changed "unintended".
My point earlier was that if we configure the Video PLL from the start
to be 245MHz, then we don't need to worry about it.
For the temporary requested rate, it's not clear to me why we should
store it into the struct clk_core itself. It looks to be transient by
nature, so it would be better in the clk_rate_request structure.
clk_rate_request are now instantiated by functions, maybe we could add a
pointer to the clk_rate_request that triggered it (some kind of
"parent", but most likely to go from the child clock to the parent)?
> > Why do you even need the core to be involved in the first place? You
> > say you think it does, but you haven't answered any of my mails to
> > provide more details and figure out if we can do it without it.
>
> We already have this functionality (calc required new rates) inside the
> core and the core currently is the only one knowing all the context
> about the tree-structure and the required and new rates.
I mean, that whole discussion kind of proves that we don't have that
functionality in the core.
> So I think in the example above, calling calc_new_rates() again, this
> time with the CRTC divider and req, might be the simplest solution to
> the problem.
>
> I think as the Video PLL isn't directly consumed, we don't really have a
> different possibility to achieve the same outcome, except of starting
> Video PLL already with 245M (e.g. via device-tree).
It doesn't need to happen in the device tree, but that sounds completely
reasonable to me.
> Just for the sake of completeness:
> A "conflict" occurs if this call would try to re-configure Video PLL
> again (if req_tmp is already set; by not involving req here, we
> basically avoid the "exclusiveness"). IMO, there are different ways to
> proceed on a conflict: A possible clk_set_rate() option would be to
> ignore a potential re-change of Video PLL by the second calc_new_rates()
> and just set a somewhat close to the req rate for CRTC divider. A
> possible clk_set_rate_exclusive() option is the one implemented here:
> error out if we cannot guarantee the existing required rates for the
> rest of the subtree.
The problem here is that you effectively want to coordinate clocks to
change their rate. Most of the logic of whether or not they care about
it, and how they care about it cannot be embedded in the clock
framework.
If we want to address this properly, I think we would need to switch the
entire clock framework to some kind of state like KMS does.
Something like:
- All the affected clocks by a configuration (rate, parent, phase,
accuracy, etc.) change are supposed to be within the state.
- Whenever someone does a clk_set_rate call, we build up a state with
the clock it was called on and all its child clocks.
- We introduce a new hook in clk_ops that will take that state and the
clock tells whether it's ok, or whether it needs to modify some
parameters. We call every clock, starting from the top most clock,
asking whether it's ok or if they need to change anything. If they
need to change anything, we start again with the new set of
parameters.
- Clocks are free to pull into the state more clocks (ie, their
parent), which in turn will pull their child.
- Once every driver agrees, we "commit" that state.
That way, we can keep the driver requirements in the driver, and every
clock affected by a rate change can adapt.
That's a major undertaking, and we would need a bunch of helpers to
maintain compatibility with the current API we have. Plus tons of kunit
tests.
Again, if we have the option to just run the PLL at some multiple of
both, I really think for everyone's sanity that we should do that.
Maxime