| Message ID | 20240224-mux-v1-1-608cc704ef43@outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RESEND] clk: set initial best mux parent to current parent when determining rate | expand |
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? > > 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()? ----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); return now <= rate && now > best; }
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; > }
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@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. 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'?
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@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. > > 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.
Quoting Yang Xiwen (2024-02-28 18:33:11) > On 2/29/2024 10:25 AM, Stephen Boyd wrote: > >>> > >>> 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. I will have to fix this as well in a different patch. > > > >> > >>> > >>> ----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. Yes the 'struct clk_rate_request' is pre-filled with many details, including the rate of the clk and the current parent rate and parent hw pointer. I'm pretty sure you're trying to fix this fixme from clk_test.c 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. */
On 3/1/2024 9:42 AM, Stephen Boyd wrote: > Quoting Yang Xiwen (2024-02-28 18:33:11) >> On 2/29/2024 10:25 AM, Stephen Boyd wrote: >>>>> >>>>> 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. > > I will have to fix this as well in a different patch. > >>> >>>> >>>>> >>>>> ----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. > > Yes the 'struct clk_rate_request' is pre-filled with many details, > including the rate of the clk and the current parent rate and parent hw > pointer. I'm pretty sure you're trying to fix this fixme from clk_test.c > > 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. > */ If CLK_MUX_ROUND_CLOSEST is not specified, I think both setting lowest possible rate and returning -EINVAL are okay, just as documented(It will ONLY return a rate lower or equal to the rate requested). But if CLK_MUX_ROUND_CLOSEST is specified, the behavior would be wrong in no doubt. I don't know which behavior consumers would expect. Maybe some consumer code has already been relying on this (undocumented) behavior. This patch indeed also has an influence on clocks without CLK_MUX_ROUND_CLOSEST. So you are right, I'll have to fix doc for clk_mux_determine_rate too.
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); + } for (i = 0; i < num_parents; i++) { unsigned long parent_rate;