On 5 March 2015 at 15:42, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote: > > +Cc Viresh Kumar > > Viresh, this is the patch for the underlying clocks for the Mediatek > cpufreq driver. > > On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote: >> Hi Sascha, >> >> On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote: >> > On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote: >> >> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver >> >> for intermediate clock source switching. This patch is based on Mediatek >> >> clock driver patches[1]. >> >> >> >> [1] http://thread.gmane.org/gmane.linux.kernel/1892436 >> >> >> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@xxxxxxxxxx> >> >> --- >> >> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate, >> >> + unsigned long min_rate, >> >> + unsigned long max_rate, >> >> + unsigned long *best_parent_rate, >> >> + struct clk_hw **best_parent_p) >> >> +{ >> >> + struct clk *clk = hw->clk, *parent; >> >> + unsigned long parent_rate; >> >> + int i; >> >> + >> >> + for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) { >> >> + parent = clk_get_parent_by_index(clk, i); >> >> + if (!parent) >> >> + return 0; >> >> + >> >> + if (i == MAINPLL_INDEX) { >> >> + parent_rate = __clk_get_rate(parent); >> >> + if (parent_rate == rate) >> >> + break; >> >> + } >> >> + >> >> + parent_rate = __clk_round_rate(parent, rate); >> >> + } >> >> + >> >> + *best_parent_rate = parent_rate; >> >> + *best_parent_p = __clk_get_hw(parent); >> >> + return parent_rate; >> >> +} >> > >> > Why this determine_rate hook? If you want to switch the clock to some >> > intermediate parent I would assume you do this explicitly by setting the >> > parent and not implicitly by setting a rate. >> > >> >> I use determine_rate hook here because I am using generic cpufreq-dt >> driver and I want to make clock switching transparent to cpufreq-dt. >> I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I >> call clk_set_rate() with the rate of MAINPLL, and determine_rate will >> select MAINPLL as the new parent. > > We have clk_set_parent for changing the parent and clk_set_rate to > change the rate. Use the former for changing the parent and the latter > for changing the rate. What you are interested in is changing the > parent, so use clk_set_parent for this and not abuse a side effect > of clk_set_rate. > > My suggestion is to take another approach. Implement clk_set_rate for > these muxes and in the set_rate hook: > > - switch mux to intermediate PLL parent > - call clk_set_rate() for the real parent PLL > - switch mux back to real parent PLL Hi Sascha, Thanks for your suggestion. I've tried to take this approach, but there's some issues here. Calling clk_set_rate() inside the set_rate callback of cpumux will cause an infinite recursive calling in the clock framework: mux.set_rate() -> pll.set_rate() -> mux.set_rate -> ... I've also tries to update pll register settings in the set_rate() callback of cpumux, but the PLL clock information will not be correctly updated in this case. How do you think to create a new "CPU PLL" type of clock and do underlying mux switching in the set_rate callback of "CPU PLL"? Best Regards, Pi-Cheng > > This way the things happening behind the scenes are completely transparent > to the cpufreq driver and you can use cpufreq-dt as is without changes. > > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
