[PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[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>
---
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/
---
Changes in v3:
- remove the fix for clk_test.c
- limit the fix to CLK_MUX_ROUND_CLOSEST
- Link to v2: https://lore.kernel.org/r/20240306-mux-v2-0-92a5fa461fd2@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
---
 drivers/clk/clk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2253c154a824..74dd61f7269f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -647,6 +647,12 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 	if (core->flags & CLK_SET_RATE_NO_REPARENT)
 		return clk_core_determine_rate_no_reparent(hw, req);
 
+	/* if MUX_ROUND_CLOSEST is set, set the initial best parent to current parent */
+	if (flags & CLK_MUX_ROUND_CLOSEST) {
+		best_parent = core->parent;
+		best = clk_core_get_rate_nolock(core);
+	}
+
 	/* find the parent that can provide the fastest rate <= rate */
 	num_parents = core->num_parents;
 	for (i = 0; i < num_parents; i++) {

---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240215-mux-6db8b3714590

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

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Maxime Ripard]

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

Hi,

On Thu, Mar 07, 2024 at 10:03:50AM +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>

I don't think it would be the way to go. The biggest issue to me is that
it's inconsistent, and only changing the behaviour for a given flag
doesn't solve that.

And again, either way, we should document it. And run it through kernelci.

Maxime

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

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Yang Xiwen]

On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> Hi,
>
> On Thu, Mar 07, 2024 at 10:03:50AM +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>
> I don't think it would be the way to go. The biggest issue to me is that
> it's inconsistent, and only changing the behaviour for a given flag
> doesn't solve that.


I think the current behavior is odd but conforms to the document if 
CLK_MUX_ROUND_CLOSEST is not specified. If i understand correctly, the 
default behavior of mux clocks is to select the closest rate lower than 
requested rate, and CLK_MUX_ROUND_CLOSEST removes the "lower than" 
limitation, which is what this version tries to accomplish.


>
> And again, either way, we should document it. And run it through kernelci.
>
> Maxime


-- 
Regards,
Yang Xiwen

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Maxime Ripard]

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

On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
> On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, Mar 07, 2024 at 10:03:50AM +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>
> > I don't think it would be the way to go. The biggest issue to me is that
> > it's inconsistent, and only changing the behaviour for a given flag
> > doesn't solve that.
> 
> 
> I think the current behavior is odd but conforms to the document if
> CLK_MUX_ROUND_CLOSEST is not specified.

clk_mux_determine_rate_flags isn't documented, and the determine_rate
clk_ops documentation doesn't mention it can return an error.

> If i understand correctly, the default behavior of mux clocks is to
> select the closest rate lower than requested rate, and
> CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
> what this version tries to accomplish.

The situation is not as clear-cut as you make it to be, unfortunately.
The determine_rate clk_ops implementation states:

  Given a target rate as input, returns the closest rate actually
  supported by the clock, and optionally the parent clock that should be
  used to provide the clock rate.

So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
determine_rate expects so it should always be there.

Now, the "actually supported by the clock" can be interpreted in
multiple ways, and most importantly, doesn't state what the behaviour is
if we can't find a rate actually supported by the clock.

But now, this situation has been ambiguous for a while and thus drivers
kind of relied on that ambiguity.

So the way to fix it up is:

  - Assess what drivers are relying on
  - Document the current behaviour in clk_ops determine_rate
  - Make clk_mux_determine_rate_flags consistent with that
  - Run that through kernelci to make sure we don't have any regression

Maxime

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

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Yang Xiwen]

On 3/11/2024 5:34 PM, Maxime Ripard wrote:
> On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
>> On 3/7/2024 4:48 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Thu, Mar 07, 2024 at 10:03:50AM +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>
>>> I don't think it would be the way to go. The biggest issue to me is that
>>> it's inconsistent, and only changing the behaviour for a given flag
>>> doesn't solve that.
>>
>> I think the current behavior is odd but conforms to the document if
>> CLK_MUX_ROUND_CLOSEST is not specified.
> clk_mux_determine_rate_flags isn't documented, and the determine_rate
> clk_ops documentation doesn't mention it can return an error.
>
>> If i understand correctly, the default behavior of mux clocks is to
>> select the closest rate lower than requested rate, and
>> CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
>> what this version tries to accomplish.
> The situation is not as clear-cut as you make it to be, unfortunately.
> The determine_rate clk_ops implementation states:
>
>    Given a target rate as input, returns the closest rate actually
>    supported by the clock, and optionally the parent clock that should be
>    used to provide the clock rate.
>
> So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
> determine_rate expects so it should always be there.
>
> Now, the "actually supported by the clock" can be interpreted in
> multiple ways, and most importantly, doesn't state what the behaviour is
> if we can't find a rate actually supported by the clock.
>
> But now, this situation has been ambiguous for a while and thus drivers
> kind of relied on that ambiguity.
>
> So the way to fix it up is:
>
>    - Assess what drivers are relying on
>    - Document the current behaviour in clk_ops determine_rate


 From my investigation, it's totally a mess, especially for platform clk 
drivers (PLL). Some drivers always round down, the others round to 
nearest, with or without a specific flag to switch between them, depend 
on the division functions they choose. Fixing all of them seems needs 
quite a lot of time and would probably introduce some regressions.

We'd probably only have to say both rounding to nearest and rounding 
down are acceptable, though either one is preferred.


>    - Make clk_mux_determine_rate_flags consistent with that


I think we must keep existing flags and document the current behavior 
correctly because of the massive existing users of clk_mux.


That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully 
it won't cause too many regressions.


>    - Run that through kernelci to make sure we don't have any regression


We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig 
drivers/clk/.kunitconfig' each time before i send patches.


Over all, it seems quite a lot of work here.


>
> Maxime


The situation here becomes even more complex when it comes to U-Boot clk 
framework. They chose slightly different prototypes and stated 
clk_set_rate() can fail with -ve. It's a great burden for clk driver 
authors and maintainers when they try to port their drivers to U-Boot. 
Let's Cc U-Boot clk maintainers as well, and see how we can resolve the 
mess here.

-- 
Regards,
Yang Xiwen

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Maxime Ripard]

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

On Tue, Mar 12, 2024 at 04:52:29PM +0800, Yang Xiwen wrote:
> On 3/11/2024 5:34 PM, Maxime Ripard wrote:
> > On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
> > > On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Mar 07, 2024 at 10:03:50AM +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>
> > > > I don't think it would be the way to go. The biggest issue to me is that
> > > > it's inconsistent, and only changing the behaviour for a given flag
> > > > doesn't solve that.
> > > 
> > > I think the current behavior is odd but conforms to the document if
> > > CLK_MUX_ROUND_CLOSEST is not specified.
> > clk_mux_determine_rate_flags isn't documented, and the determine_rate
> > clk_ops documentation doesn't mention it can return an error.
> > 
> > > If i understand correctly, the default behavior of mux clocks is to
> > > select the closest rate lower than requested rate, and
> > > CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
> > > what this version tries to accomplish.
> > The situation is not as clear-cut as you make it to be, unfortunately.
> > The determine_rate clk_ops implementation states:
> > 
> >    Given a target rate as input, returns the closest rate actually
> >    supported by the clock, and optionally the parent clock that should be
> >    used to provide the clock rate.
> > 
> > So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
> > determine_rate expects so it should always be there.
> > 
> > Now, the "actually supported by the clock" can be interpreted in
> > multiple ways, and most importantly, doesn't state what the behaviour is
> > if we can't find a rate actually supported by the clock.
> > 
> > But now, this situation has been ambiguous for a while and thus drivers
> > kind of relied on that ambiguity.
> > 
> > So the way to fix it up is:
> > 
> >    - Assess what drivers are relying on
> >    - Document the current behaviour in clk_ops determine_rate
> 
> 
> From my investigation, it's totally a mess, especially for platform clk
> drivers (PLL). Some drivers always round down, the others round to nearest,
> with or without a specific flag to switch between them, depend on the
> division functions they choose. Fixing all of them seems needs quite a lot
> of time and would probably introduce some regressions.

I agree it's a mess, but that's a mess you wanted to clean up in the
first place :)

> We'd probably only have to say both rounding to nearest and rounding down
> are acceptable, though either one is preferred.
> 
> 
> >    - Make clk_mux_determine_rate_flags consistent with that
> 
> 
> I think we must keep existing flags and document the current behavior
> correctly because of the massive existing users of clk_mux.
>
> That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully it
> won't cause too many regressions.

Which, without a documentation, makes it more of a mess.

> 
> >    - Run that through kernelci to make sure we don't have any regression
> 
> 
> We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig
> drivers/clk/.kunitconfig' each time before i send patches.

That's kunit, not kernelci (https://linux.kernelci.org/job/)

> 
> Over all, it seems quite a lot of work here.
>
> The situation here becomes even more complex when it comes to U-Boot clk
> framework. They chose slightly different prototypes and stated
> clk_set_rate() can fail with -ve. It's a great burden for clk driver authors
> and maintainers when they try to port their drivers to U-Boot. Let's Cc
> U-Boot clk maintainers as well, and see how we can resolve the mess here.

I mean, eventually, that's on them. If U-Boot chose to have a
somewhat-similar-but-not-really clock framework, there's nothing Linux
can solve here, even though I definitely can see the frustration for the
developpers that have to work on both.

Maxime

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

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Sean Anderson]

On 3/12/24 04:52, Yang Xiwen wrote:
> On 3/11/2024 5:34 PM, Maxime Ripard wrote:
>> On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
>>> On 3/7/2024 4:48 PM, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Thu, Mar 07, 2024 at 10:03:50AM +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>
>>>> I don't think it would be the way to go. The biggest issue to me is that
>>>> it's inconsistent, and only changing the behaviour for a given flag
>>>> doesn't solve that.
>>>
>>> I think the current behavior is odd but conforms to the document if
>>> CLK_MUX_ROUND_CLOSEST is not specified.
>> clk_mux_determine_rate_flags isn't documented, and the determine_rate
>> clk_ops documentation doesn't mention it can return an error.
>>
>>> If i understand correctly, the default behavior of mux clocks is to
>>> select the closest rate lower than requested rate, and
>>> CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
>>> what this version tries to accomplish.
>> The situation is not as clear-cut as you make it to be, unfortunately.
>> The determine_rate clk_ops implementation states:
>>
>>    Given a target rate as input, returns the closest rate actually
>>    supported by the clock, and optionally the parent clock that should be
>>    used to provide the clock rate.
>>
>> So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
>> determine_rate expects so it should always be there.
>>
>> Now, the "actually supported by the clock" can be interpreted in
>> multiple ways, and most importantly, doesn't state what the behaviour is
>> if we can't find a rate actually supported by the clock.
>>
>> But now, this situation has been ambiguous for a while and thus drivers
>> kind of relied on that ambiguity.
>>
>> So the way to fix it up is:
>>
>>    - Assess what drivers are relying on
>>    - Document the current behaviour in clk_ops determine_rate
> 
> 
>  From my investigation, it's totally a mess, especially for platform clk drivers (PLL). Some drivers always round down, the others round to nearest, with or without a specific flag to switch between them, depend on the division functions they choose. Fixing all of them seems needs quite a lot of time and would probably introduce some regressions.
> 
> We'd probably only have to say both rounding to nearest and rounding down are acceptable, though either one is preferred.


>>    - Make clk_mux_determine_rate_flags consistent with that
> 
> 
> I think we must keep existing flags and document the current behavior correctly because of the massive existing users of clk_mux.
> 
> 
> That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully it won't cause too many regressions.
> 
> 
>>    - Run that through kernelci to make sure we don't have any regression
> 
> 
> We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig drivers/clk/.kunitconfig' each time before i send patches.
> 
> 
> Over all, it seems quite a lot of work here.
> 
> 
>>
>> Maxime
> 
> 
> The situation here becomes even more complex when it comes to U-Boot clk framework. They chose slightly different prototypes and stated clk_set_rate() can fail with -ve. 

Maybe you mean clk_get_rate? Setting a rate can always fail due to the
nature of clocks...

Personally, I am not terribly attached to the API (as not many callers
handle errors correctly), but I have not had the time recently to do any
cleanup.

It's a great burden for clk driver authors and maintainers when they try to port their drivers to U-Boot. Let's Cc U-Boot clk maintainers as well, and see how we can resolve the mess here.
> 

Regarding rounding mode, IMO it is better to let driver authors pick
whatever is convenient. Round closest is best, but there may be code size
savings for round lowest or some other scheme. [1] has the current
recommendation, which is to punt and let the caller use round_rate if
they actually care.

--Sean

[1] https://source.denx.de/u-boot/custodians/u-boot-clk/-/blob/a8dc4965f09d28a59c156437673ddb66860c847e/include/clk-uclass.h#L143