linux-kernel - Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 03 Jul 2014 15:16:09 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>,
	"Stephen Boyd" <sboyd@...eaurora.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Lists linaro-kernel" <linaro-kernel@...ts.linaro.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"Arvind Chauhan" <arvind.chauhan@....com>,
	linux-arm-msm@...r.kernel.org,
	"Sachin Kamat" <spk.linux@...il.com>,
	"Thomas P Abraham" <thomas.ab@...sung.com>,
	"Shawn Guo" <shawn.guo@...aro.org>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"Nishanth Menon" <nm@...com>, "Tomasz Figa" <t.figa@...sung.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"Kukjin Kim" <kgene.kim@...sung.com>,
	"Michal Simek" <michal.simek@...inx.com>,
	"Rob Herring" <rob.herring@...aro.org>,
	"Santosh Shilimkar" <santosh.shilimkar@...com>,
	"Simon Horman" <horms@...ge.net.au>
Subject: Re: [PATCH 00/14] cpufreq: cpu0: Extend support beyond CPU0, V2

Quoting Viresh Kumar (2014-07-02 19:44:04)
> On 3 July 2014 06:54, Stephen Boyd <sboyd@...eaurora.org> wrote:
> > I gave it a spin. It works so you can have my
> >
> > Tested-by: Stephen Boyd <sboyd@...eaurora.org>
> 
> 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 ? :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/