From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Turquette Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2 Date: Thu, 03 Jul 2014 15:16:09 -0700 Message-ID: <20140703221609.11380.6884@quantum> References: <53B4B0B3.9080000@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: Sender: linux-pm-owner@vger.kernel.org To: Viresh Kumar , Stephen Boyd Cc: "Rafael J. Wysocki" , Lists linaro-kernel , "linux-pm@vger.kernel.org" , Arvind Chauhan , linux-arm-msm@vger.kernel.org, Sachin Kamat , Thomas P Abraham , Shawn Guo , Linux Kernel Mailing List , Nishanth Menon , Tomasz Figa , "devicetree@vger.kernel.org" , Kukjin Kim , Michal Simek , Rob Herring , Santosh Shilimkar , Simon Horman List-Id: linux-arm-msm@vger.kernel.org Quoting Viresh Kumar (2014-07-02 19:44:04) > On 3 July 2014 06:54, Stephen Boyd wrote: > > I gave it a spin. It works so you can have my > > > > Tested-by: Stephen Boyd > > Thanks, all suggested improvements are made and pushed again with > your Tested-by.. > > > I'm still concerned about the patch where we figure out if the clocks > > are shared. I worry about a configuration where there are different > > clocks for on/off (i.e. gates) that are per-cpu but they all source from > > the same divider or something that is per-cluster. In DT this may be > > described as different clock provider outputs for the gates and in the > > cpu node we would have different clock specifiers but in reality all the > > CPUs in that cluster are affected by the same frequency scaling. > > Yeah, this is probably what matches with Rob's doubt. These can > actually be different. Good point. > > > In this case we'll need to get help from the clock framework to > > determine that those gates clocks don't have any .set_rate() callback so > > they can't actually change rate independently, and then walk up the tree > > to their parents to see if they have a common ancestor that does change > > rates. That's where it becomes useful to have a clock framework API for > > this, like clk_shares_rate(struct clk *clk, struct clk *peer) or > > something that can hide all this from cpufreq. Here's what I think it > > would look like (totally untested/uncompiled): > > > > static struct clk *find_rate_changer(struct clk *clk) > > { > > > > if (!clk) > > return NULL; > > > > do { > > /* Rate could change by changing parents */ > > if (clk->num_parents > 1) > > return clk; > > > > /* Rate could change by calling clk_set_rate() */ > > if (clk->ops->set_rate) > > return clk; > > > > /* > > * This is odd, clk_set_rate() doesn't propagate > > * and this clock can't change rate or parents > > * so we must be at the root and the clock we > > * started at can't change rates. Just return the > > * root so that we can see if the other clock shares > > * the same root although CPUfreq should never care. > > */ > > if (!(clk->flags & CLK_SET_RATE_PARENT)) > > return clk; > > } while ((clk = clk->parent)) > > > > return NULL; > > } > > > > bool clk_shares_rate(struct clk *clk, struct clk *peer) > > { > > struct clk *p1, *p2; > > > > p1 = find_rate_changer(clk); > > p2 = find_rate_changer(peer) > > > > return p1 == p2; > > } > > I find it much better then doing what I did initially, simply matching clk_get() > outputs. Lets see what Mike has to say.. Sorry for being dense, but I still do not get why trying to dynamically discover a shared rate-changeable clock is a better approach than simply describing the hardware in DT? Is adding a property to the CPU binding that describes how the CPUs in a cluster expect to use a clock somehow a non-starter? It is certainly a win for readability when staring at DT and trying to understand how DVFS on that CPU is meant to work (as opposed to hiding that knowledge behind a tree walk). Regards, Mike > > @Mike: Is this less ugly ? :)