Re: [PATCH RESEND] clk: set initial best mux parent to current parent when determining rate

[Yang Xiwen]

On 2/29/2024 10:25 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen (2024-02-28 18:13:04)
>> On 2/29/2024 9:58 AM, Stephen Boyd wrote:
>>> Quoting Yang Xiwen via B4 Relay (2024-02-23 09:18:52)
>>>> From: Yang Xiwen <forbidden405@xxxxxxxxxxx>
>>>>
>>>> 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_NEAREST.
>>>
>>> Did you mean CLK_MUX_ROUND_CLOSEST?
>>
>> You are right :).
>>
>>>
>>>>
>>>> For example, if the lowest possible rate privided by the mux is 1000Hz,
>>>
>>> s/privided/provided/
>>>
>>>> setting a rate below 500Hz will fail, because no clock can provide a
>>>> better rate than the non-existant 0. But it should succeed with 1000Hz
>>>> being set.
>>>>
>>>> Setting the initial best parent to current parent could solve this bug
>>>> very well.
>>>>
>>>> Signed-off-by: Yang Xiwen <forbidden405@xxxxxxxxxxx>
>>>> ---
>>>> This is actually a v2 of [1], but seems too simple to have a unittest.
>>>> It's tested in a mmc host driver.
>>>
>>> It's not too simple for a unittest.
>>>
>>>>
>>>> [1]: https://lore.kernel.org/linux-clk/20230421-clk-v3-1-9ff79e7e7fed@xxxxxxxxxxx/
>>>
>>> In that thread I asked you to please Cc Maxime. Please do that.
>>>
>>>> ---
>>>>  drivers/clk/clk.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>> index 2253c154a824..d98cebd7ff03 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -649,6 +649,10 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
>>>>  
>>>>         /* find the parent that can provide the fastest rate <= rate */
>>>>         num_parents = core->num_parents;
>>>> +       if (core->parent) {
>>>> +               best_parent = core->parent;
>>>> +               best = clk_core_get_rate_nolock(best_parent);
>>>> +       }
>>>
>>> Is the problem that we're not using abs_diff()?
>>
>>
>> No, i think. It has nothing to do with the code here. It's because of
>> the initial best_parent/best_parent_rate.
> 
> Alright.
> 
>>
>>>
>>> ----8<----
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index a3bc7fb90d0f..91023345595f 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -542,7 +542,7 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
>>>                          unsigned long best, unsigned long flags)
>>>  {
>>>       if (flags & CLK_MUX_ROUND_CLOSEST)
>>> -             return abs(now - rate) < abs(best - rate);
>>> +             return abs_diff(now, rate) < abs_diff(best, rate);
>>
>> Without this patch, the initial `best` rate would be always 0. This is
>> wrong for most cases, 0Hz might (usually) be unavailable. We should use
>> a valid rate(i.e. current rate) initially.
> 
> Ok. But you set best to the parent rate. So why not use 'core->rate'
> directly as 'best'?


I can't remember exactly. I just add this piece of code and found it's
working. Is this field already filled prior to setting rate? Anyway,
your suggestion is very reasonable. Maybe dear clk maintainers can fix
it as i'm not familiar with clk core code.

-- 
Best regards,
Yang Xiwen