Message-Id: <20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com>
Date: Wed, 28 May 2025 19:16:48 -0400
From: Brian Masney <bmasney@...hat.com>
To: Stephen Boyd <sboyd@...nel.org>, Maxime Ripard <mripard@...nel.org>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
 Arnd Bergmann <arnd@...db.de>, Thomas Gleixner <tglx@...utronix.de>, 
 Michael Turquette <mturquette@...libre.com>, 
 Alberto Ruiz <aruiz@...hat.com>, Brian Masney <bmasney@...hat.com>
Subject: [PATCH v2 02/10] clk: preserve original rate when a sibling clk
 changes it's rate

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@...hat.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

Powered by blists - more mailing lists