From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752885AbbCJBxY (ORCPT ); Mon, 9 Mar 2015 21:53:24 -0400 Received: from mail-lb0-f172.google.com ([209.85.217.172]:39507 "EHLO mail-lb0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751515AbbCJBxV (ORCPT ); Mon, 9 Mar 2015 21:53:21 -0400 MIME-Version: 1.0 In-Reply-To: <20150305074207.GC11010@pengutronix.de> References: <1425466152-7867-1-git-send-email-pi-cheng.chen@linaro.org> <20150304112109.GB11010@pengutronix.de> <20150305074207.GC11010@pengutronix.de> Date: Tue, 10 Mar 2015 09:53:19 +0800 Message-ID: Subject: Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control From: Pi-Cheng Chen To: Sascha Hauer Cc: Mike Turquette , Stephen Boyd , Matthias Brugger , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Henry Chen , James Liao , Chen Fan , Eddie Huang , "Joe.C" , Linux Kernel Mailing List , "linux-arm-kernel@lists.infradead.org" , "devicetree@vger.kernel.org" , Linaro Kernel Mailman List , linux-mediatek@lists.infradead.org, Viresh Kumar Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 5 March 2015 at 15:42, Sascha Hauer 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 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 >> >> --- >> >> +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 |