Messages in this thread | | | From | Eric Anholt <> | Subject | Re: [PATCH v2 2/3] clk: bcm2835: Support for clock parent selection | Date | Sat, 05 Dec 2015 16:19:14 -0800 |
| |
Remi Pommarel <repk@triplefau.lt> writes:
> On Thu, Dec 03, 2015 at 04:37:07PM -0800, Eric Anholt wrote: >> Remi Pommarel <repk@triplefau.lt> writes: >> >> > On Wed, Nov 18, 2015 at 10:30:17AM -0800, Eric Anholt wrote: >> > >> > [...] >> > >> >> > +static int bcm2835_clock_determine_rate(struct clk_hw *hw, >> >> > + struct clk_rate_request *req) >> >> > +{ >> >> > + struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw); >> >> > + struct clk_hw *parent, *best_parent = NULL; >> >> > + struct clk_rate_request parent_req; >> >> > + unsigned long rate, best_rate = 0; >> >> > + unsigned long prate, best_prate = 0; >> >> > + size_t i; >> >> > + u32 div; >> >> > + >> >> > + /* >> >> > + * Select parent clock that results in the closest but lower rate >> >> > + */ >> >> > + for (i = 0; i < clk_hw_get_num_parents(hw); ++i) { >> >> > + parent = clk_hw_get_parent_by_index(hw, i); >> >> > + if (!parent) >> >> > + continue; >> >> > + parent_req = *req; >> >> >> >> parent_req appears dead, so it should be removed. >> > >> > Yes, will do thanks. >> > >> >> > + prate = clk_hw_get_rate(parent); >> >> > + div = bcm2835_clock_choose_div(hw, req->rate, prate); >> >> > + rate = bcm2835_clock_rate_from_divisor(clock, prate, div); >> >> > + if (rate > best_rate && rate <= req->rate) { >> >> > + best_parent = parent; >> >> > + best_prate = prate; >> >> > + best_rate = rate; >> >> > + } >> >> > + } >> >> > + >> >> > + if (!best_parent) >> >> > + return -EINVAL; >> >> > + >> >> > + req->best_parent_hw = best_parent; >> >> > + req->best_parent_rate = best_prate; >> >> >> >> I think you're supposed to req->rate = best_rate, here, too. With these >> >> two fixes, >> > >> > I did not set req->rate to best_rate in order to avoid rounding down >> > twice the actual clock rate. >> > >> > Indeed with patch 1 from this patchset bcm2835_clock_choose_div() >> > chooses a divisor that produces a rate lower or equal to the requested >> > one. As we call bcm2835_clock_choose_div() twice when using >> > clk_set_rate() (once with ->determine_rate() and once with ->set_rate()), >> > if I set req->rate in bcm2835_clock_determine_rate to the rounded down >> > one, the final rate will likely be again rounded down in >> > bcm2835_clock_set_rate(). >> >> If we pass bcm2835_clock_rate_from_divisor(bcm2835_clock_choose_div()), >> to bcm2835_clock_choose_div(), will it actually give a different divisor >> than the first call? (That seems like an unfortunate problem in our >> implementation, if so). > > Unfortunately yes. Because we want the divided rate to be lower or equal > to the expected one, I round up the div each time the div_64() produces a > reminder. Thus calling bcm2835_clock_choose_div() with > bcm2835_clock_rate_from_divisor(bcm2835_clock_choose_div()) will still > likely see a reminder from div_64(). > >> >> I'd be willing to go along with this, but if so I'd like a comment >> explaining why we aren't setting the field that we should pretty >> obviously be setting. > > I can either put a comment here explaining why we do not update > req->rate or do as the patch attached at the end. > > This patch adds an argument to bcm2835_clock_choose_div() to switch on or > off the div round up. Then bcm2835_clock_determine_rate() could choose > the appropriate divisor that produces the highest lower rate while > bcm2835_clock_set_rate() can actually set the divisor which will remain > the same. > > On second though I prefer the second solution. What do you think ?
Make "round_up" be bool and use true/false as its values, and it looks good to me! [unhandled content-type:application/pgp-signature] | |