Messages in this thread | | | Date | Thu, 2 Oct 2014 16:31:53 +0300 | From | Tero Kristo <> | Subject | Re: [PATCH] clk: prevent erronous parsing of children during rate change |
| |
On 09/30/2014 10:03 PM, Mike Turquette wrote: > Quoting Tero Kristo (2014-09-30 01:48:49) >> 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. > > I stand corrected. I thought it was still done the old way where the > machine-specific clock struct was holding the pointer to the ref_clk and > bypass_clk. I'm glad that is not the case any more. > >> >>> >>>> 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. > > I made it hard for you to find it because I typo'd. It's not a clk api > but a clk_op: > > int (*set_rate_and_parent)(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate, u8 index);
Oh I see. I have been experimenting with this last couple of days and it looks like I got a working solution with determine_rate / set_rate_and_parent for the DPLLs now. Need to do some cleanup for the patches and will hopefully post those tomorrow, but I guess this will probably miss the next merge window seeing we are pretty late in -rc.
-Tero
> > Regards, > Mike > >> >> -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 >>>> >>
| |