Messages in this thread | | | Date | Tue, 30 Sep 2014 11:48:49 +0300 | From | Tero Kristo <> | Subject | Re: [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 >>
| |