There are times when the requested rate on a clk cannot be fulfilled
due to the current rate of the parent clk. If CLK_SET_RATE_PARENT is
set, then the parent rate will be adjusted so that the child can
obtain the requested rate.

When the parent rate is adjusted, there's currently an issue where
the rates of the other children are unnecessarily changed. Let's take
the following simplified clk tree as an example:

                     parent
                     24 MHz
                    /      \
              child1        child2
              24 MHz        24 MHz

The child1 and child2 clks are simple divider clocks in this example,
and it doesn't really matter what kind of clk parent is; for simplicity
we can say that the parent can obtain any rate necessary. Now, let's
update the value of child2 with the existing clk code:

- child2 requests 48 MHz. This is incompatible the current parent rate
  of 24 MHz.
- parent is updated to 48 MHz so that child2 can obtain the requested
  rate of 48 MHz by using a divider of 0.
- child1 should stay at 24 MHz, and the divider should be automatically
  changed from 0 to 1.

The current bug in the code sets the rate of child1 to 48 MHz by calling
the clk_op's recalc_rate() with only the new parent rate, which keeps
the clk's divider at 0. Specifically this example occurs in this part of
the call tree:

clk_core_set_rate_nolock(child2, 48_MHZ)
-> clk_calc_new_rates(child2, 48_MHZ)
  # clk has CLK_SET_RATE_PARENT set, so clk_calc_new_rates() is invoked
  # via the following block:
  # if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
  #     best_parent_rate != parent->rate)
  #      top = clk_calc_new_rates(parent, best_parent_rate);
  -> clk_calc_new_rates(parent, 48_MHZ)
    -> clk_calc_subtree(parent, 48_MHZ, ...)
      -> clk_recalc(child1, 48_MHZ)
         # BOOM! This is where the bug occurs. This invokes the
         # clk_op's recalc_rate() with the new parent rate of 48 MHz,
         # and the original divider of 0 is kept intact, so child1's
         # rate is changed from 24 MHz to 48 MHz by the clk core.

When the clk core requests rate changes, the struct clk_core contains
a new_rate field that contains the rate that the clk is changed to by
clk_change_rate().

When a parent changes it's rate, only ensure that the section of the
clk tree where the rate change request propagated up is changed. All
other sibling nodes should try to keep the same rate (or close to it)
that was previously set. We avoid this by not initially calling the
clk_op's recalc_rate() for parts of the subtree that haven't been
modified.

Once the new_rate fields are populated with the correct values,
eventually clk_change_rate() is called on the parent, and the parent
will invoke clk_change_rate() for all of the children with the expected
rates stored in the new_rate fields. This will invoke the clk_op's
set_rate() on each of the children.

This doesn't fix all of the issues where a clk can unknowingly change
the rate of it's siblings, or put them in an invalid state, however
this is a relatively small change that can fix some issues. A correct
change that includes coordination with the other children in the
subtree, and works across the various types of clks will involve a
much more elaborate patch set.

This change was tested with kunit tests, and also boot tested on a
Lenovo Thinkpad x13s laptop.

Fixes: b2476490ef11 ("clk: introduce the common clock framework")
Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a130eac9072dc7e71f840a0edf51c368650f8386..65408899a4ae8674e78494d77ff07fa658f7d3b0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -78,6 +78,10 @@ struct clk_parent_map {
  * @rate:              Current clock rate (Hz).
  * @req_rate:          Requested clock rate (Hz).
  * @new_rate:          New rate to be set during a rate change operation.
+ * @rate_directly_changed: Clocks have the ability to change the rate of it's
+ *                     parent in order to satisfy a rate change request. This
+ *                     flag indicates that the rate change request initiated
+ *                     with this particular node.
  * @new_parent:        Pointer to new parent during parent change.
  * @new_child:         Pointer to new child during reparenting.
  * @flags:             Clock property and capability flags.
@@ -114,6 +118,7 @@ struct clk_core {
 	unsigned long		rate;
 	unsigned long		req_rate;
 	unsigned long		new_rate;
+	bool			rate_directly_changed;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
 	unsigned long		flags;
@@ -2306,7 +2311,11 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 		new_parent->new_child = core;
 
 	hlist_for_each_entry(child, &core->children, child_node) {
-		child->new_rate = clk_recalc(child, new_rate);
+		if (child->rate_directly_changed)
+			child->new_rate = clk_recalc(child, new_rate);
+		else
+			child->new_rate = child->rate;
+
 		clk_calc_subtree(child, child->new_rate, NULL, 0);
 	}
 }
@@ -2579,6 +2588,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;
 
+	core->rate_directly_changed = true;
+
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
@@ -2601,6 +2612,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	/* change the rates */
 	clk_change_rate(top);
 
+	core->rate_directly_changed = false;
+
 	core->req_rate = req_rate;
 err:
 	clk_pm_runtime_put(core);

-- 
2.49.0

On Wed, May 28, 2025 at 07:16:48PM -0400, Brian Masney wrote:
> There are times when the requested rate on a clk cannot be fulfilled
> due to the current rate of the parent clk. If CLK_SET_RATE_PARENT is
> set, then the parent rate will be adjusted so that the child can
> obtain the requested rate.
> 
> When the parent rate is adjusted, there's currently an issue where
> the rates of the other children are unnecessarily changed. Let's take
> the following simplified clk tree as an example:
> 
>                      parent
>                      24 MHz
>                     /      \
>               child1        child2
>               24 MHz        24 MHz
> 
> The child1 and child2 clks are simple divider clocks in this example,
> and it doesn't really matter what kind of clk parent is; for simplicity
> we can say that the parent can obtain any rate necessary. Now, let's
> update the value of child2 with the existing clk code:
> 
> - child2 requests 48 MHz. This is incompatible the current parent rate
>   of 24 MHz.
> - parent is updated to 48 MHz so that child2 can obtain the requested
>   rate of 48 MHz by using a divider of 0.

If we're talking about theory, a divider of 0 doesn't sound great :)

> - child1 should stay at 24 MHz, and the divider should be automatically
>   changed from 0 to 1.
> 
> The current bug in the code sets the rate of child1 to 48 MHz by calling
> the clk_op's recalc_rate() with only the new parent rate, which keeps
> the clk's divider at 0. Specifically this example occurs in this part of
> the call tree:
> 
> clk_core_set_rate_nolock(child2, 48_MHZ)
> -> clk_calc_new_rates(child2, 48_MHZ)
>   # clk has CLK_SET_RATE_PARENT set, so clk_calc_new_rates() is invoked
>   # via the following block:
>   # if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
>   #     best_parent_rate != parent->rate)
>   #      top = clk_calc_new_rates(parent, best_parent_rate);
>   -> clk_calc_new_rates(parent, 48_MHZ)
>     -> clk_calc_subtree(parent, 48_MHZ, ...)
>       -> clk_recalc(child1, 48_MHZ)
>          # BOOM! This is where the bug occurs. This invokes the
>          # clk_op's recalc_rate() with the new parent rate of 48 MHz,
>          # and the original divider of 0 is kept intact, so child1's
>          # rate is changed from 24 MHz to 48 MHz by the clk core.
> 
> When the clk core requests rate changes, the struct clk_core contains
> a new_rate field that contains the rate that the clk is changed to by
> clk_change_rate().

I'm not sure we should frame the problem as "the core calls clk_recalc",
but rather that it doesn't call round_rate or determine_rate on the new
parent rate, so the clock doesn't have the chance to update its
dividers.

If we were doing the latter, calling clk_recalc after would be ok (even
though a bit redundant).

And calling either will cause other challenges, since they can also
reparent or change the parent rate.

I think I'd still mention that it's a temporary solution because we
can't use them yet.

> When a parent changes it's rate, only ensure that the section of the
> clk tree where the rate change request propagated up is changed. All
> other sibling nodes should try to keep the same rate (or close to it)
> that was previously set. We avoid this by not initially calling the
> clk_op's recalc_rate() for parts of the subtree that haven't been
> modified.
> 
> Once the new_rate fields are populated with the correct values,
> eventually clk_change_rate() is called on the parent, and the parent
> will invoke clk_change_rate() for all of the children with the expected
> rates stored in the new_rate fields. This will invoke the clk_op's
> set_rate() on each of the children.
> 
> This doesn't fix all of the issues where a clk can unknowingly change
> the rate of it's siblings, or put them in an invalid state, however
> this is a relatively small change that can fix some issues. A correct
> change that includes coordination with the other children in the
> subtree, and works across the various types of clks will involve a
> much more elaborate patch set.
> 
> This change was tested with kunit tests, and also boot tested on a
> Lenovo Thinkpad x13s laptop.
> 
> Fixes: b2476490ef11 ("clk: introduce the common clock framework")
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a130eac9072dc7e71f840a0edf51c368650f8386..65408899a4ae8674e78494d77ff07fa658f7d3b0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -78,6 +78,10 @@ struct clk_parent_map {
>   * @rate:              Current clock rate (Hz).
>   * @req_rate:          Requested clock rate (Hz).
>   * @new_rate:          New rate to be set during a rate change operation.
> + * @rate_directly_changed: Clocks have the ability to change the rate of it's
> + *                     parent in order to satisfy a rate change request. This
> + *                     flag indicates that the rate change request initiated
> + *                     with this particular node.
>   * @new_parent:        Pointer to new parent during parent change.
>   * @new_child:         Pointer to new child during reparenting.
>   * @flags:             Clock property and capability flags.
> @@ -114,6 +118,7 @@ struct clk_core {
>  	unsigned long		rate;
>  	unsigned long		req_rate;
>  	unsigned long		new_rate;
> +	bool			rate_directly_changed;

clk_core already owns a lot of transient data, and I'm not sure adding
more is a good idea.

What if we were to pass the initiating clock to all these functions, and
test if we are indeed this clock instead?

Maxime