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); 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 > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html