[v2,1/2] clk: set initial best mux parent to current parent when determining rate

Commit Message

Yang Xiwen via B4 Relay March 5, 2024, 4:22 p.m. UTC

From: Yang Xiwen <[e-mail]>

Originally, the initial clock rate is hardcoded to 0, this can lead to
some problem when setting a very small rate with CLK_MUX_ROUND_CLOSEST.

For example, if the lowest possible rate provided by the mux is 1000Hz,
setting a rate below 500Hz will fail, because no clock can provide a
better rate than the non-existant 0Hz. But it should succeed with 1000Hz
being set.

Setting the initial best parent to current parent could solve this bug.

Signed-off-by: Yang Xiwen <[e-mail]>
---
 drivers/clk/clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Maxime Ripard March 6, 2024, 2:24 p.m. UTC | #1

Hi,

On Wed, Mar 06, 2024 at 12:22:23AM +0800, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <[e-mail]>
> 
> Originally, the initial clock rate is hardcoded to 0, this can lead to
> some problem when setting a very small rate with CLK_MUX_ROUND_CLOSEST.
> 
> For example, if the lowest possible rate provided by the mux is 1000Hz,
> setting a rate below 500Hz will fail, because no clock can provide a
> better rate than the non-existant 0Hz. But it should succeed with 1000Hz
> being set.
> 
> Setting the initial best parent to current parent could solve this bug.
> 
> Signed-off-by: Yang Xiwen <[e-mail]>

That patch makes sense to me, but this changes the behaviour of the function.

Before, if we couldn't find a good configuration for the rate, we were
error'ing out. Now, we keep the current configuration. We should
document the new behaviour in the function documentation, and we should
probably run that through kernelci to make sure we aren't breaking any
platform (and from experience, we probably are).

Maxime

Yang Xiwen March 6, 2024, 2:30 p.m. UTC | #2

On 3/6/2024 10:24 PM, Maxime Ripard wrote:
> Hi,
>
> On Wed, Mar 06, 2024 at 12:22:23AM +0800, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <[e-mail]>
>>
>> Originally, the initial clock rate is hardcoded to 0, this can lead to
>> some problem when setting a very small rate with CLK_MUX_ROUND_CLOSEST.
>>
>> For example, if the lowest possible rate provided by the mux is 1000Hz,
>> setting a rate below 500Hz will fail, because no clock can provide a
>> better rate than the non-existant 0Hz. But it should succeed with 1000Hz
>> being set.
>>
>> Setting the initial best parent to current parent could solve this bug.
>>
>> Signed-off-by: Yang Xiwen <[e-mail]>
> That patch makes sense to me, but this changes the behaviour of the function.
>
> Before, if we couldn't find a good configuration for the rate, we were
> error'ing out. Now, we keep the current configuration. We should
> document the new behaviour in the function documentation, and we should
> probably run that through kernelci to make sure we aren't breaking any
> platform (and from experience, we probably are).


We can limit the new behavior to CLK_MUX_ROUND_CLOSEST as well. The 
current behavior is okay for common muxes i think. Though probably wrong 
for CLK_MUX_ROUND_CLOSEST.


>
> Maxime

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2253c154a824..5fa92227b355 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -639,9 +639,9 @@  int clk_mux_determine_rate_flags(struct clk_hw *hw,
 				 struct clk_rate_request *req,
 				 unsigned long flags)
 {
-	struct clk_core *core = hw->core, *parent, *best_parent = NULL;
+	struct clk_core *core = hw->core, *parent, *best_parent = core->parent;
 	int i, num_parents, ret;
-	unsigned long best = 0;
+	unsigned long best = clk_core_get_rate_nolock(core);
 
 	/* if NO_REPARENT flag set, pass through to current parent */
 	if (core->flags & CLK_SET_RATE_NO_REPARENT)