| Message ID | 17737873.r3cte6TEQ5@phil (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Heiko, On 23/05/2014 21:33, Heiko Stübner wrote: > From: Boris BREZILLON <b.brezillon@overkiz.com> > > In case the rate_hw does not implement determine_rate, but only round_rate > we fallback to best_parent selection if mux_hw is present and support > reparenting. > > Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> > > This also fixes a rate calculation problem when using the standard div and > mux ops, as in this case currently only the mux->determine_rate is used > in the composite rate calculation. > [fixed the output to actually return a rate instead of the diff] Sorry for the delay and thanks for fixing this. Anyway, when I first proposed this patch, Emilio (added in Cc) told me this should not be automatically done by the composite clk driver, and the driver should instead provide the determine_rate function. Mike, any opinion on this patch ? Best Regards, Boris > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/clk/clk-composite.c | 51 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 50 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c > index 57a078e..0a4cd21 100644 > --- a/drivers/clk/clk-composite.c > +++ b/drivers/clk/clk-composite.c > @@ -64,11 +64,59 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, > const struct clk_ops *mux_ops = composite->mux_ops; > struct clk_hw *rate_hw = composite->rate_hw; > struct clk_hw *mux_hw = composite->mux_hw; > + struct clk *parent; > + unsigned long parent_rate; > + long tmp_rate, best_rate; > + unsigned long rate_diff; > + unsigned long best_rate_diff = ULONG_MAX; > + int i; > > if (rate_hw && rate_ops && rate_ops->determine_rate) { > rate_hw->clk = hw->clk; > return rate_ops->determine_rate(rate_hw, rate, best_parent_rate, > best_parent_p); > + } else if (rate_hw && rate_ops && rate_ops->round_rate && > + mux_hw && mux_ops && mux_ops->set_parent) { > + *best_parent_p = NULL; > + > + if (__clk_get_flags(hw->clk) & CLK_SET_RATE_NO_REPARENT) { > + *best_parent_p = clk_get_parent(mux_hw->clk); > + *best_parent_rate = __clk_get_rate(*best_parent_p); > + > + return rate_ops->round_rate(rate_hw, rate, > + best_parent_rate); > + } > + > + for (i = 0; i < __clk_get_num_parents(mux_hw->clk); i++) { > + parent = clk_get_parent_by_index(mux_hw->clk, i); > + if (!parent) > + continue; > + > + parent_rate = __clk_get_rate(parent); > + > + tmp_rate = rate_ops->round_rate(rate_hw, rate, > + &parent_rate); > + if (tmp_rate < 0) > + continue; > + > + if (tmp_rate < rate) > + rate_diff = rate - tmp_rate; > + else > + rate_diff = tmp_rate - rate; > + > + if (!rate_diff || !*best_parent_p > + || best_rate_diff > rate_diff) { > + *best_parent_p = parent; > + *best_parent_rate = parent_rate; > + best_rate_diff = rate_diff; > + best_rate = tmp_rate; > + } > + > + if (!rate_diff) > + return rate; > + } > + > + return best_rate; > } else if (mux_hw && mux_ops && mux_ops->determine_rate) { > mux_hw->clk = hw->clk; > return mux_ops->determine_rate(mux_hw, rate, best_parent_rate, > @@ -196,7 +244,8 @@ struct clk *clk_register_composite(struct device *dev, const char *name, > composite->rate_hw = rate_hw; > composite->rate_ops = rate_ops; > clk_composite_ops->recalc_rate = clk_composite_recalc_rate; > - if (rate_ops->determine_rate) > + if (rate_ops->determine_rate || > + (rate_ops->round_rate && clk_composite_ops->set_parent)) > clk_composite_ops->determine_rate = clk_composite_determine_rate; > } >
Am Montag, 26. Mai 2014, 11:24:44 schrieb Boris BREZILLON: > Hello Heiko, > > On 23/05/2014 21:33, Heiko Stübner wrote: > > From: Boris BREZILLON <b.brezillon@overkiz.com> > > > > In case the rate_hw does not implement determine_rate, but only round_rate > > we fallback to best_parent selection if mux_hw is present and support > > reparenting. > > > > Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> > > > > This also fixes a rate calculation problem when using the standard div and > > mux ops, as in this case currently only the mux->determine_rate is used > > in the composite rate calculation. > > [fixed the output to actually return a rate instead of the diff] > > Sorry for the delay and thanks for fixing this. > > Anyway, when I first proposed this patch, Emilio (added in Cc) told me > this should not be automatically done by the composite clk driver, and > the driver should instead provide the determine_rate function. Just to point out, as I'm not sure if it comes across correctly from my description above, it looks like the composite behaviour is currently broken when using the standard mux and div ops for it. As only mux_ops provides a determine rate callback, it will always only run mux_ops->determine_rate thus returning the parent rate as the target rate. So when the parents are 600 and 891 MHz and you want 75MHz [which the divider can provide], the clock would always be set to 600MHz, which may be way out of spec - as seen on my rk3188 clock tree. When looking at the other composite users, only st/clkgen-mux.c uses the same pattern with both the standard mux and div operations. I've added Gabriel, maybe he can check what happens on his arch, when an affected clock is changed. > Mike, any opinion on this patch ? > > Best Regards, > > Boris > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > > > drivers/clk/clk-composite.c | 51 > > ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 > > insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c > > index 57a078e..0a4cd21 100644 > > --- a/drivers/clk/clk-composite.c > > +++ b/drivers/clk/clk-composite.c > > @@ -64,11 +64,59 @@ static long clk_composite_determine_rate(struct clk_hw > > *hw, unsigned long rate,> > > const struct clk_ops *mux_ops = composite->mux_ops; > > struct clk_hw *rate_hw = composite->rate_hw; > > struct clk_hw *mux_hw = composite->mux_hw; > > > > + struct clk *parent; > > + unsigned long parent_rate; > > + long tmp_rate, best_rate; > > + unsigned long rate_diff; > > + unsigned long best_rate_diff = ULONG_MAX; > > + int i; > > > > if (rate_hw && rate_ops && rate_ops->determine_rate) { > > > > rate_hw->clk = hw->clk; > > return rate_ops->determine_rate(rate_hw, rate, best_parent_rate, > > > > best_parent_p); > > > > + } else if (rate_hw && rate_ops && rate_ops->round_rate && > > + mux_hw && mux_ops && mux_ops->set_parent) { > > + *best_parent_p = NULL; > > + > > + if (__clk_get_flags(hw->clk) & CLK_SET_RATE_NO_REPARENT) { > > + *best_parent_p = clk_get_parent(mux_hw->clk); > > + *best_parent_rate = __clk_get_rate(*best_parent_p); > > + > > + return rate_ops->round_rate(rate_hw, rate, > > + best_parent_rate); > > + } > > + > > + for (i = 0; i < __clk_get_num_parents(mux_hw->clk); i++) { > > + parent = clk_get_parent_by_index(mux_hw->clk, i); > > + if (!parent) > > + continue; > > + > > + parent_rate = __clk_get_rate(parent); > > + > > + tmp_rate = rate_ops->round_rate(rate_hw, rate, > > + &parent_rate); > > + if (tmp_rate < 0) > > + continue; > > + > > + if (tmp_rate < rate) > > + rate_diff = rate - tmp_rate; > > + else > > + rate_diff = tmp_rate - rate; > > + > > + if (!rate_diff || !*best_parent_p > > + || best_rate_diff > rate_diff) { > > + *best_parent_p = parent; > > + *best_parent_rate = parent_rate; > > + best_rate_diff = rate_diff; > > + best_rate = tmp_rate; > > + } > > + > > + if (!rate_diff) > > + return rate; > > + } > > + > > + return best_rate; > > > > } else if (mux_hw && mux_ops && mux_ops->determine_rate) { > > > > mux_hw->clk = hw->clk; > > return mux_ops->determine_rate(mux_hw, rate, best_parent_rate, > > > > @@ -196,7 +244,8 @@ struct clk *clk_register_composite(struct device *dev, > > const char *name,> > > composite->rate_hw = rate_hw; > > composite->rate_ops = rate_ops; > > clk_composite_ops->recalc_rate = clk_composite_recalc_rate; > > > > - if (rate_ops->determine_rate) > > + if (rate_ops->determine_rate || > > + (rate_ops->round_rate && clk_composite_ops->set_parent)) > > > > clk_composite_ops->determine_rate = clk_composite_determine_rate; > > > > }
Hi Heiko, On 05/26/2014 12:27 PM, Heiko Stübner wrote: > Am Montag, 26. Mai 2014, 11:24:44 schrieb Boris BREZILLON: >> Hello Heiko, >> >> On 23/05/2014 21:33, Heiko Stübner wrote: >>> From: Boris BREZILLON <b.brezillon@overkiz.com> >>> >>> In case the rate_hw does not implement determine_rate, but only round_rate >>> we fallback to best_parent selection if mux_hw is present and support >>> reparenting. >>> >>> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> >>> >>> This also fixes a rate calculation problem when using the standard div and >>> mux ops, as in this case currently only the mux->determine_rate is used >>> in the composite rate calculation. >>> [fixed the output to actually return a rate instead of the diff] >> Sorry for the delay and thanks for fixing this. >> >> Anyway, when I first proposed this patch, Emilio (added in Cc) told me >> this should not be automatically done by the composite clk driver, and >> the driver should instead provide the determine_rate function. > Just to point out, as I'm not sure if it comes across correctly from my > description above, it looks like the composite behaviour is currently broken > when using the standard mux and div ops for it. > > As only mux_ops provides a determine rate callback, it will always only run > mux_ops->determine_rate > thus returning the parent rate as the target rate. YesI saw this regression for me later, i had planned to propose a correction this patch is therefore highly welcome. Thanks ! > So when the parents are 600 and 891 MHz and you want 75MHz [which the divider > can provide], the clock would always be set to 600MHz, which may be way out of > spec - as seen on my rk3188 clock tree. > > > When looking at the other composite users, only st/clkgen-mux.c uses the same > pattern with both the standard mux and div operations. I've added Gabriel, > maybe he can check what happens on his arch, when an affected clock is changed. I tested this use case with the patch, the good parent is selected (600 Mhz) and make the division to have 75 Mhz. But if CLK_SET_RATE_PARENT is set, it will take the 981 Mhz clock and recalculate the parent rate (because the diff rate is equal or a little better than the 600 Mhz clock) Which is a shame, all child clocks are affected. It was the best choice to choose other parent and do just a division (not recalculate the parent rate) Best Regards Gabriel. > >> Mike, any opinion on this patch ? >> >> Best Regards, >> >> Boris >>
diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c index 57a078e..0a4cd21 100644 --- a/drivers/clk/clk-composite.c +++ b/drivers/clk/clk-composite.c @@ -64,11 +64,59 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate, const struct clk_ops *mux_ops = composite->mux_ops; struct clk_hw *rate_hw = composite->rate_hw; struct clk_hw *mux_hw = composite->mux_hw; + struct clk *parent; + unsigned long parent_rate; + long tmp_rate, best_rate; + unsigned long rate_diff; + unsigned long best_rate_diff = ULONG_MAX; + int i; if (rate_hw && rate_ops && rate_ops->determine_rate) { rate_hw->clk = hw->clk; return rate_ops->determine_rate(rate_hw, rate, best_parent_rate, best_parent_p); + } else if (rate_hw && rate_ops && rate_ops->round_rate && + mux_hw && mux_ops && mux_ops->set_parent) { + *best_parent_p = NULL; + + if (__clk_get_flags(hw->clk) & CLK_SET_RATE_NO_REPARENT) { + *best_parent_p = clk_get_parent(mux_hw->clk); + *best_parent_rate = __clk_get_rate(*best_parent_p); + + return rate_ops->round_rate(rate_hw, rate, + best_parent_rate); + } + + for (i = 0; i < __clk_get_num_parents(mux_hw->clk); i++) { + parent = clk_get_parent_by_index(mux_hw->clk, i); + if (!parent) + continue; + + parent_rate = __clk_get_rate(parent); + + tmp_rate = rate_ops->round_rate(rate_hw, rate, + &parent_rate); + if (tmp_rate < 0) + continue; + + if (tmp_rate < rate) + rate_diff = rate - tmp_rate; + else + rate_diff = tmp_rate - rate; + + if (!rate_diff || !*best_parent_p + || best_rate_diff > rate_diff) { + *best_parent_p = parent; + *best_parent_rate = parent_rate; + best_rate_diff = rate_diff; + best_rate = tmp_rate; + } + + if (!rate_diff) + return rate; + } + + return best_rate; } else if (mux_hw && mux_ops && mux_ops->determine_rate) { mux_hw->clk = hw->clk; return mux_ops->determine_rate(mux_hw, rate, best_parent_rate, @@ -196,7 +244,8 @@ struct clk *clk_register_composite(struct device *dev, const char *name, composite->rate_hw = rate_hw; composite->rate_ops = rate_ops; clk_composite_ops->recalc_rate = clk_composite_recalc_rate; - if (rate_ops->determine_rate) + if (rate_ops->determine_rate || + (rate_ops->round_rate && clk_composite_ops->set_parent)) clk_composite_ops->determine_rate = clk_composite_determine_rate; }