common clock framework

[Lei Wen]

Hi Mike,

On Wed, May 23, 2012 at 3:11 AM, Mike Turquette <mturquette at ti.com> wrote:
> On 20120518-01:41, Chao Xie wrote:
>> Hi
>> Since the issues are not described very clearly, so I want to clarify it.
>
> Is is possible for you to wrap your lines to a reasonable length?  I've
> done it for you in the email below.
>
>> Issue 1. clock with mux, divisor and trigger
>> The clock A will have a single register that some bits control the
>> mux, some bits control the divisor, and a bit is a trigger. It means
>> that setting mux and divisor bits will only take effect when trigger
>> bit is set.  There are 2 models that we can abstract this kind of
>> clock.
>> a. make three clocks.
>> clock A <= clock div <= clock mux
>> clock mux control the mux bits while clock div control the divisor
>> bits, while clock a control trigger bit.  For example mux has two
>> inputs 312MHZ and 1066MHZ, while divisor can be 1 or 2. At beginning
>> clock mux is from 312M and clock div is 1. Clock A can only have clock
>> as 312MHZ and 533MHZ.  so when invoke clk_set_rate(A, 512MHZ), it will
>> try to upstream and find the "top" clock that can satisfy the 512MHZ.
>> the "top" clock is clock mux. After find the "top" clock, the current
>> code will have the following flows.
>>
>> clk_notify(PRE_RATE_CHANGE)
>>     ==> clk_change_rate(mux)
>>         ==> mux->setrate: it will set mux bits
>>         ==> polling all children, and for each one, invoke clk_change_rate
>>            ==> clk_change_rate(div)
>>               ==> div->setrate: it will set div bits
>>               ==> polling all children, and for each one, invoke clk_change_rate
>>                   ==>A->setrate: it will set trigger bit
>> From above that the function looks well, but there will be some
>> problem.  Clock mux change the mux bits will not finally switch the
>> input from 312MHZ to 533MHZ. The switch is controlled by "trigger"
>> bit. So clock mux should have .setrate implementation as you suggest
>> as
>>       1. enable new parent
>>       2. set mux bits
>>       3. __clk_reparent
>>       4. disable old parent
>> The clock mux implementation looks like omap dpll implementation.
>> But there are two bugs.
>>    a) "Disable old parent" is only SW setting, we depends on trigger
>>    to do it. So if for clock mux and clock div we all have trigger set
>>    when set mux bits and div bits. It will import a temporary clock
>>    1066MHZ(input = 1066, div = 1), it will harm the device. In fact,
>>    there is why there is a trigger bit here. It will forbid there is
>>    no temporary clock.
>
> Let me make sure I understand you here.  You are concerned that the
> .set_rate implementations for each clock (div and mux) will set the
> trigger bit, causing intermediate clock rates.  Do I have that correct?
>

Yes, that is correct, we don't want the trigger bit to set multiple times.


> If that is the case then one solution is to not have your div and mux
> .set_rate implementations set the trigger bit.  Instead always propagate
> up to "clk A", the trigger clock so that the whole operation is atomic.
>
> The only problem with this scenario is if you ever change only the div
> (and not the mux) and there is no parent propagation up to "clk A".  Is
> this a possibility on your platform?

I think it may has some problem here. We need to adjust "clk A"'s parent's
speed when we need change "clk A" speed.

However, we could handle it by another way by caching the register
modification and no really touch the hw register in the div/mux clk node.
So we also could propagation to A's parent, while have no trouble like
set trigger bit multi-times.


>
>>    b) if clk_change_rate(A) will invoke cpu_notify(POST_RATE_CHANGE),
>>    while __clk_reparent will invoke __clk_recalc_rates, and in
>>    __clk_recalc_rates, it will polling every children, and for each
>>    one will invoke __clk_recalc_rates too. For __clk_recalc_rates, it
>>    will invoke cpu_notify(POST_RATE_CHANGE), and it means that the
>>    notification of POST_RATE_CHANGE will be invoke twice. I think the
>>    omap dpll implementation will have this bug too.
>>
>
> This has come up on the list recently.  I think one solution is to
> remove the call to __clk_recalc_rates from __clk_reparent.  To be honest
> __clk_reparent has grown beyond it's original intent.

Could we have something like flag judgment here?
If some flag is set, then do __clk_recalc_rates inside __clk_reparent.
By this way, we could have more flexible to handle complex scenario.

Thanks,
Lei