Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo <t-kristo@ti.com>]

On 09/30/2014 10:07 AM, Mike Turquette wrote:
> Quoting Tero Kristo (2014-09-29 01:09:24)
>> On 09/27/2014 02:24 AM, Mike Turquette wrote:
>>> Quoting Tero Kristo (2014-09-26 00:18:55)
>>>> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
>>>>> On 09/23/14 06:38, Tero Kristo wrote:
>>>>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
>>>>>>> On 08/21, Tero Kristo wrote:
>>>>>>>>              /* Skip children who will be reparented to another clock */
>>>>>>>>              if (child->new_parent && child->new_parent != clk)
>>>>>>>>                  continue;
>>>>>>>
>>>>>>> Are we not hitting the new_parent check here? I don't understand
>>>>>>> how we can be changing parents here unless the check is being
>>>>>>> avoided, in which case I wonder why determine_rate isn't being
>>>>>>> used.
>>>>>>>
>>>>>>
>>>>>> It depends how the clock underneath handles the situation. The error I
>>>>>> am seeing actually happens with a SoC specific compound clock (DPLL)
>>>>>> which integrates set_rate + mux functionality into a single clock
>>>>>> node. A call to the clk_set_rate changes the parent of this clock
>>>>>> (from bypass clock to reference clock), in addition to changing the
>>>>>> rate (tune the mul+div.) I looked at using the determine rate call
>>>>>> with this type but it breaks everything up... the parent gets changed
>>>>>> but not the clock rate, in addition to some other issues.
>>>>>
>>>>> Ok. Is this omap3_noncore_dpll_set_rate()?
>>>>
>>>> Yes.
>>>>
>>>>    > Can we use determine_rate +
>>>>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
>>>>> allow us to do the mult+div and the parent in the same op call, although
>>>>> I don't understand why setting the parent and then setting the rate is
>>>>> not going to work.
>>>>
>>>> Well, setting parent first, then rate later causes problems with the
>>>> DPLL ending up running with illegal (non-specified) rate, the M+N values
>>>> are most likely wrong if you just switch from bypass clock to reference
>>>> clock first without programming the M+N first.
>>>
>>> I took a quick look and it still seems to me that the OMAP DPLLs are
>>> still not modeled properly as mux clocks. Is this correct?
>>
>> Yeah, they are not mux clocks, but rather a compound of mux + DPLL
>> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL
>> div/mult clock will still have overlapping usage of the DPLL_EN field,
>
> I'm not talking about splitting up the clock into two separate clocks.
> If memory serves the DPLL clock implementation "cheats" and hides the
> bypass_clk info from the clock framework. To be explicit, from the
> perspective of Linux clock framework DPLL clocks only have one parent.
>
> In reality a typical DPLL should have at least 2 parents (and in some
> cases starting with OMAP4, some of the DPLL output clocks should have a
> second HSD parent). But the implementation does not reflect this.

No, this is not the DPLLs are modelled. Each DPLL has currently two 
parents, ref-clk and bypass-clk, which are both modelled as separate 
clock nodes, and the DPLL switches parents based on bypass/lock mode. 
The bypass clock is also usually a mux clock, which further selects 
separate bypass parent, resulting in 3 or more parents for a certain DPLL.

>
>> as the DPLL must be in bypass mode during M+N change. Or, should the
>> DPLL rate change only be allowed if the mux is in bypass setting?
>> Several drivers still depend on direct dpll clk_set_rate working
>> 'properly' (there are some other issues currently present also which
>> have nothing to do with the mux behavior.)
>>
>>> This issue has been lingering for a long time and we can't use
>>> determine_rate unless that clock has multiple parents. Simply hacking
>>> knowledge of the parent bypass clock into the .set_rate callback is not
>>> enough.
>>
>> If you believe this _must_ be changed, I can take a look at this for
>> next merge window, but this will cause a DT data compatibility break if
>> nothing else (personally I don't care about this as I always rebuild DT
>> blob with kernel, but lots of other people seem to do.)
>
> Well I guess the question is how long will we put up with the many small
> headaches caused by incorrectly modeling the clock?

Well, its not kind of incorrectly modelled, it is just modelled in such 
way that clk_set_rate doesn't cope too well with it.

> determine_rate and clk_set_parent_and_rate should be sufficient for the
> OMAP DPLLs but only if they are correctly modeled in the framework.

Do we have implementation for clk_set_parent_and_rate someplace? I 
looked at rc7 and didn't find this. I think this would fix the issues I 
am seeing combined with determine_rate, if clk_set_rate would internally 
handle changing both rate + parent.

-Tero

>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>>
>>> Regards,
>>> Mike
>>>
>>>>
>>>>     I'm interested in the other issues that you mentioned
>>>>> too.
>>>>
>>>> Mostly these were side-effects from the illegal DPLL setup I guess, like
>>>> boot hang, failed drivers etc. I didn't really investigate this that
>>>> much as it is much more simpler just to use safe list iteration here.
>>>>
>>>> -Tero
>>