On 3 July 2014 06:54, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: > I gave it a spin. It works so you can have my > > Tested-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx> 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.. @Mike: Is this less ugly ? :) -- 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