Message-ID: <aDYNdvQTBSDdyE0H@x1>
Date: Tue, 27 May 2025 15:07:34 -0400
From: Brian Masney <bmasney@...hat.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: sboyd@...nel.org, mturquette@...libre.com, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] clk: preserve original rate when a sibling clk
 changes it's rate

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

Powered by blists - more mailing lists