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

[Eric Anholt <eric@anholt.net>]

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

Remi Pommarel <repk@triplefau.lt> writes:

> Hi,
>
> On Wed, Nov 04, 2015 at 06:03:31PM -0800, Eric Anholt wrote:
>
> [...]
>
>> 
>> It looks like you've dropped the use of the divisor off of the PLL
>> channel when setting a rate.  That seems bad for all the other clocks in
>> the system, and a feature we couldn't lose.
>
> Sorry, but I'm not sure to understand your point here. Are you afraid
> that clocks such as PWM, H264, etc, have lost the ability to divide the
> rate from the PLL or oscillator clock they cosume as source ?
>
> If so, I think it's ok. If I'm not wrong here, clk_set_rate() first
> calls clk->determinate_rate() then calls clk->set_rate(). This patch
> makes bcm2835_clock_determine_source() to only select the parent to use
> and does not set the clock's rate itself. The clock's rate is set later
> on when bcm2835_clock_set_parent() is called.
>
> bcm2835_clock_set_parent() still divides the parent rate so we are not
> loosing this feature here.

I see.  You're leaving req->rate as-is, so that it gets passed back in
on the set_rate() call.  Since you've chosen only a parent with a
rate greater than ours, we know we'll be able to divide into it.

This has the downside that anything using the min/max rate clamping
doesn't get to know before setting that we might be out of bounds.

> It would probably be better to have bcm2835_clock_determine_source()
> selects the parent by choosing the one that provides the rate which,
> after being divided, generates the highest but lower rate out of the
> PWM clock itself.
>
> Moreover, if you agree with the above modification I see no reason to
> not call it "bcm2835_clock_determine_rate"

This sounds like what the function should be doing.

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