[PATCH 0/2] clk: preserve original rate when a sibling clk changes it's rate

[PATCH 0/2] clk: preserve original rate when a sibling clk changes it's rate

[Brian Masney]

Here's a patch that helps to preserve the original clk rate on sibling
clks when the parent has it's rate changed. More details are on the
patch.

This series needs to be applied on top of my clk kunit tests that
document some issues that need to be fixed in the clk core:
https://lore.kernel.org/lkml/20250407131258.70638-1-bmasney@redhat.com/
This series fixes an issue in the clk core so that two of my kunit
tests can be enabled.

I will be at Embedded Linux Conference / OSS Summit in Denver, CO from
June 23-25th 2025 in case anyone would like to discuss some of these
issues and changes to the clk core in person.

Brian Masney (2):
  clk: preserve original rate when a sibling clk changes it's rate
  clk: test: remove kunit_skip() for divider tests that have been fixed

 drivers/clk/clk.c      | 28 ++++++++++++++++++++++++++--
 drivers/clk/clk_test.c |  4 ----
 2 files changed, 26 insertions(+), 6 deletions(-)

-- 
2.49.0

[PATCH 1/2] clk: preserve original rate when a sibling clk changes it's rate

[Brian Masney]

When a clk requests a new rate, there are times when the requested rate
cannot be fulfilled due to the current rate of the parent clk. If
CLK_SET_RATE_PARENT is set, then the parent rate can also be changed.
However, the clk core currently doesn't negotiate with any of the other
children to see if the new parent rate is acceptable, and will currently
just change the rates of the sibling clks.

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 a rate close to where they
were originally at. The rate will go through a recalc_rate() with the
new parent rate, so the rate may possibly change.

This doesn't fix all of the issues where a clk can unknowingly change
the rate of it's siblings, however this is a relatively small change
that can fix some issues. A correct change that includes voting across
the various nodes 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.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0565c87656cf..713d4d8a9b1e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -72,6 +72,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;
@@ -2254,6 +2255,7 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
 			     struct clk_core *new_parent, u8 p_index)
 {
 	struct clk_core *child;
+	unsigned long tmp_rate;
 
 	core->new_rate = new_rate;
 	core->new_parent = new_parent;
@@ -2264,7 +2266,14 @@ 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);
+		/*
+		 * 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 a rate
+		 * close to where they were originally at.
+		 */
+		tmp_rate = child->rate_directly_changed ? new_rate : child->rate;
+		child->new_rate = clk_recalc(child, tmp_rate);
 		clk_calc_subtree(child, child->new_rate, NULL, 0);
 	}
 }
@@ -2346,8 +2355,10 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	}
 
 	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
-	    best_parent_rate != parent->rate)
+	    best_parent_rate != parent->rate) {
+		parent->rate_directly_changed = true;
 		top = clk_calc_new_rates(parent, best_parent_rate);
+	}
 
 out:
 	clk_calc_subtree(core, new_rate, parent, p_index);
@@ -2487,6 +2498,15 @@ static void clk_change_rate(struct clk_core *core)
 	clk_pm_runtime_put(core);
 }
 
+static void clk_clear_rate_flags(struct clk_core *top)
+{
+	struct clk_core *child;
+
+	top->rate_directly_changed = false;
+	hlist_for_each_entry(child, &top->children, child_node) {
+		clk_clear_rate_flags(child);
+	}
+}
 static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
 						     unsigned long req_rate)
 {
@@ -2537,6 +2557,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)
@@ -2559,6 +2581,8 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	/* change the rates */
 	clk_change_rate(top);
 
+	clk_clear_rate_flags(top);
+
 	core->req_rate = req_rate;
 err:
 	clk_pm_runtime_put(core);
-- 
2.49.0

Re: [PATCH 1/2] clk: preserve original rate when a sibling clk changes it's rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5446 bytes --]

Hi Brian,

Thanks for your patch

On Tue, May 20, 2025 at 03:28:45PM -0400, Brian Masney wrote:
> When a clk requests a new rate, there are times when the requested rate
> cannot be fulfilled due to the current rate of the parent clk. If
> CLK_SET_RATE_PARENT is set, then the parent rate can also be changed.
> However, the clk core currently doesn't negotiate with any of the other
> children to see if the new parent rate is acceptable, and will currently
> just change the rates of the sibling clks.
> 
> 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 a rate close to where they
> were originally at. The rate will go through a recalc_rate() with the
> new parent rate, so the rate may possibly change.
> 
> This doesn't fix all of the issues where a clk can unknowingly change
> the rate of it's siblings, however this is a relatively small change
> that can fix some issues. A correct change that includes voting across
> the various nodes 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.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  drivers/clk/clk.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0565c87656cf..713d4d8a9b1e 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -72,6 +72,7 @@ struct clk_core {
>  	unsigned long		rate;
>  	unsigned long		req_rate;
>  	unsigned long		new_rate;
> +	bool			rate_directly_changed;

I think it would be worth documenting (some parts of) clk_core. Starting
with that new field looks like a good idea.

>  	struct clk_core		*new_parent;
>  	struct clk_core		*new_child;
>  	unsigned long		flags;
> @@ -2254,6 +2255,7 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate,
>  			     struct clk_core *new_parent, u8 p_index)
>  {
>  	struct clk_core *child;
> +	unsigned long tmp_rate;
>  
>  	core->new_rate = new_rate;
>  	core->new_parent = new_parent;
> @@ -2264,7 +2266,14 @@ 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);
> +		/*
> +		 * 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 a rate
> +		 * close to where they were originally at.
> +		 */
> +		tmp_rate = child->rate_directly_changed ? new_rate : child->rate;

There's something I don't quite understand here, sorry.

new_rate here is the parent rate, but child->rate is the current (as in,
currently programmed in hardware) rate.

> +		child->new_rate = clk_recalc(child, tmp_rate);

And child->new_rate is the future rate we will try to enforce when we'll
actually perform the rate change.

In the case where rate_directly_changed is set to false, you thus asks
for the future rate a clock will have, using its former rate as input?

I think that breaks the rate propagation for all the
rate_directly_changed=false clocks.

It probably works in div_div_2 test case because you're using an
(implicit) divider of 1 for both child1 and child2.

So, child1 == child2 == parent == 24MHz.

Thus, child2->rate will be 24MHz, new_rate would be 48MHz, and thus
using child2->rate instead of new_rate would work because we indeed have
the old parent rate and the new child2 rate lined up.

But from a logical point-of-view, it doesn't really work, and I'm sure
it would break some platforms too.

My understanding of the clk_change_rate logic is that you would get in
that test something like:

parent->rate = 24MHz
child1->rate = 24MHz? (it's implicit, we should probably improve that by setting it and using an assertion)
child2->rate = 24MHz? (Ditto)

then with the call to clk_set_rate,

parent->new_rate = 48MHz
child1->new_rate = 48MHz
child2->new_rate = 48MHz? (probably, since we keep the same divider)

So we want child2->new_rate to be what child2->rate is (or better,
req_rate if it was set, but we don't really have a way to tell afaik).

Unfortunately, I don't see an easy way to do that. My first thought
would be to use clk_core_determine_round_nolock() with a clk_request
with new_rate as the parent rate, and child->req_rate as the target
rate.

However, calling round_rate or determine_rate might affect the parent
rate, which is not something we want here.

Maybe we could clean up a bit req_rate to turn it into an optional value
(either 0 or a rate) that would allow us to tell if something has set
it, and if not we know we can fall back to clk->rate.

Then, we have a useful (for this anyway) req_rate, and we could add a
flag to round_rate and determine_rate to let drivers know that they
can't update their parent or parent rate, just report what they can do
with that set of parameters.

I guess it can even be that we convert everyone to determine_rate, and
repurpose round_rate for this?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/2] clk: preserve original rate when a sibling clk changes it's rate

[Brian Masney]

Hi Maxime,

Thanks for the review!

On Tue, May 27, 2025 at 02:36:49PM +0200, Maxime Ripard wrote:
> On Tue, May 20, 2025 at 03:28:45PM -0400, Brian Masney wrote:
> > @@ -2264,7 +2266,14 @@ 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);
> > +		/*
> > +		 * 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 a rate
> > +		 * close to where they were originally at.
> > +		 */
> > +		tmp_rate = child->rate_directly_changed ? new_rate : child->rate;
> 
> There's something I don't quite understand here, sorry.
> 
> new_rate here is the parent rate, but child->rate is the current (as in,
> currently programmed in hardware) rate.

There is actually a bug in the section of code I posted.

Let me step back and describe the problem further in the clk core
since the bug is in this section of the code quoted above. Here's a
call tree and a description at each function call about what happens
today prior to my patches with my div_div_3 test, and how a clk can
unknowingly change the rate of it's sibling:

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
         # recalc_rate() op on the clk driver with the new parent rate,
         # and the original divider of 0 is kept intact. The old parent
         # rate is not passed in to the recalc_rate() op, and personally
         # I don't think it should pass in the old rate.

Here's another version of my patch that's a bit simpler and fixes the
issue:

        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);

So for the case of !child->rate_directly_changed, clk_calc_subtree() is
only called so that the grandchildren and further down towards the leaf
nodes will have the new_rate member populated.

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 set_rate() clk op on each of
the children, and this is where the divider on my test cases are updated.

So let's take your scenario:

> parent->rate = 24MHz
> child1->rate = 24MHz? (it's implicit, we should probably improve that by setting it and using an assertion)
> child2->rate = 24MHz? (Ditto)
> 
> then with the call to clk_set_rate,
> 
> parent->new_rate = 48MHz
> child1->new_rate = 48MHz
> child2->new_rate = 48MHz? (probably, since we keep the same divider)

Here's a new test case that shows that the rates and dividers are
updated as expected for that scenario:

static void clk_test_rate_change_sibling_div_div_4(struct kunit *test)
{
        struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
        int ret;

        ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_ASSERT_EQ(test, ret, 0);

        ret = clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_ASSERT_EQ(test, ret, 0);

        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_EXPECT_EQ(test, ctx->child1.div, 0);
        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_EXPECT_EQ(test, ctx->child2.div, 0);

        ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
        KUNIT_ASSERT_EQ(test, ret, 0);

        /*
         * Verify that child2 keeps it's rate at 24 MHz, however the divider is
         * automatically updated from 0 to 1. The parent rate was changed from 
         * 24 MHz to 48 MHz.
         */
        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_48_MHZ);
        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_48_MHZ);
        KUNIT_EXPECT_EQ(test, ctx->child1.div, 0);
        KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_24_MHZ);
        KUNIT_EXPECT_EQ(test, ctx->child2.div, 1);
}

Brian

[PATCH 2/2] clk: test: remove kunit_skip() for divider tests that have been fixed

[Brian Masney]

These issues have been fixed in the clk core, so let's remove the
kunit_skip().

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk_test.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index e6df1d2274b2..3f0bc44c06fd 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -788,8 +788,6 @@ static void clk_test_rate_change_sibling_div_div_2(struct kunit *test)
 	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
 	int ret;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_48_MHZ);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
@@ -808,8 +806,6 @@ static void clk_test_rate_change_sibling_div_div_3(struct kunit *test)
 	struct clk_rate_change_sibling_div_div_context *ctx = test->priv;
 	int ret;
 
-	kunit_skip(test, "This needs to be fixed in the core.");
-
 	ret = clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_16_MHZ);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
-- 
2.49.0

Re: [PATCH 0/2] clk: preserve original rate when a sibling clk changes it's rate

[Brian Masney]

On Tue, May 20, 2025 at 03:28:44PM -0400, Brian Masney wrote:
> Here's a patch that helps to preserve the original clk rate on sibling
> clks when the parent has it's rate changed. More details are on the
> patch.
> 
> This series needs to be applied on top of my clk kunit tests that
> document some issues that need to be fixed in the clk core:
> https://lore.kernel.org/lkml/20250407131258.70638-1-bmasney@redhat.com/
> This series fixes an issue in the clk core so that two of my kunit
> tests can be enabled.

I posted a v2 of this series with a new title for the cover letter. This
combines my v1 kunit series referenced above since I made a few changes
to the tests.

https://lore.kernel.org/lkml/20250528-clk-wip-v2-v2-0-0d2c2f220442@redhat.com/T/#t

Brian