Re: [PATCH v2 2/3] clk: bcm2835: Support for clock parent selection

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]