lkml.org 
[lkml]   [2014]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] clk: prevent erronous parsing of children during rate change
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
>>



\
 
 \ /
  Last update: 2014-09-30 11:01    [W:2.167 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site