Message-ID: <20250527-incredible-shaggy-stoat-8a5ba8@houat>
Date: Tue, 27 May 2025 14:36:49 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Brian Masney <bmasney@...hat.com>
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 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@...hat.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

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists