Messages in this thread | | | Date | Thu, 29 Feb 2024 10:13:04 +0800 | Subject | Re: [PATCH RESEND] clk: set initial best mux parent to current parent when determining rate | From | Yang Xiwen <> |
| |
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@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_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@outlook.com> >> --- >> 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@outlook.com/ > > 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.
> > ----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.
> > return now <= rate && now > best; > }
-- Best regards, Yang Xiwen
| |