[PATCH v2 0/2] clk: fix mux determine rate logic

[PATCH v2 0/2] clk: fix mux determine rate logic

[Yang Xiwen via B4 Relay]

This is actually a v2 of [1]. It's tested in a mmc host driver.

[1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@outlook.com/

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v2:
- cover letter: remove statements about unittest
- add fix for clk_test.c
- s/privide/provide
- Cc Maxime
All suggested by Stephan Boyd
- Link to v1: https://lore.kernel.org/r/20240215-mux-v1-1-ebb2fba31d49@outlook.com

---
Yang Xiwen (2):
      clk: set initial best mux parent to current parent when determining rate
      clk: test: use __clk_mux_determine_rate() and remove FIXME

 drivers/clk/clk.c      |  4 ++--
 drivers/clk/clk_test.c | 18 +-----------------
 2 files changed, 3 insertions(+), 19 deletions(-)
---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240215-mux-6db8b3714590

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

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

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

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 <forbidden405@outlook.com>
---
 drivers/clk/clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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)

-- 
2.43.0

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

[Maxime Ripard]

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

Hi,

On Wed, Mar 06, 2024 at 12:22:23AM +0800, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> 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 <forbidden405@outlook.com>

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

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

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

[Yang Xiwen]

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 <forbidden405@outlook.com>
>>
>> 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 <forbidden405@outlook.com>
> 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


-- 
Regards,
Yang Xiwen

[PATCH v2 2/2] clk: test: use __clk_mux_determine_rate() and remove FIXME

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Recent clk core fix resolved the bug in the FIXME comment. Now replace
the .determine_rate field back to __clk_mux_determine_rate() and remove
the FIXME comment.

Fixes: aebddfe2dfaf ("clk: test: Add a determine_rate hook")
Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/clk/clk_test.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 39e2b5ff4f51..0c91dc14d942 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -106,23 +106,7 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = {
 };
 
 static const struct clk_ops clk_dummy_single_parent_ops = {
-	/*
-	 * FIXME: Even though we should probably be able to use
-	 * __clk_mux_determine_rate() here, if we use it and call
-	 * clk_round_rate() or clk_set_rate() with a rate lower than
-	 * what all the parents can provide, it will return -EINVAL.
-	 *
-	 * This is due to the fact that it has the undocumented
-	 * behaviour to always pick up the closest rate higher than the
-	 * requested rate. If we get something lower, it thus considers
-	 * that it's not acceptable and will return an error.
-	 *
-	 * It's somewhat inconsistent and creates a weird threshold
-	 * between rates above the parent rate which would be rounded to
-	 * what the parent can provide, but rates below will simply
-	 * return an error.
-	 */
-	.determine_rate = __clk_mux_determine_rate_closest,
+	.determine_rate = __clk_mux_determine_rate,
 	.set_parent = clk_dummy_single_set_parent,
 	.get_parent = clk_dummy_single_get_parent,
 };

-- 
2.43.0