From: Mike Turquette <mturquette@linaro.org> To: Viresh Kumar <viresh.kumar@linaro.org>, Rob Herring <rob.herring@linaro.org> Cc: Grant Likely <grant.likely@linaro.org>, Stephen Boyd <sboyd@codeaurora.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Shawn Guo <shawn.guo@linaro.org>, Lists linaro-kernel <linaro-kernel@lists.linaro.org>, "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Arvind Chauhan <arvind.chauhan@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, linux-arm-msm@vger.kernel.org, Sachin Kamat <spk.linux@gmail.com>, Thomas P Abraham <thomas.ab@samsung.com>, Nishanth Menon <nm@ti.com>, Tomasz Figa <t.figa@samsung.com>, Mark Brown <broonie@kernel.org>, Mark Rutland <Mark.Rutland@arm.com> Subject: Re: [PATCH 2/2] cpufreq: cpu0: Extend support beyond CPU0 Date: Tue, 01 Jul 2014 15:00:38 -0700 [thread overview] Message-ID: <20140701220038.32686.81633@quantum> (raw) In-Reply-To: <CAKohpokaVPn7yNPNAGrE6juGaOYuxZvVsbmkOZmyi9rDub6t7g@mail.gmail.com> Quoting Viresh Kumar (2014-07-01 04:14:04) > On 1 July 2014 00:03, Rob Herring <rob.herring@linaro.org> wrote: > >> What about comparing "clocks" property in cpu DT nodes? > > > > What if a different clock is selected for some reason. > > I don't know why that will happen for CPUs sharing clock line. > > > I think a clock api function would be better. > > @Mike: What do you think? I think we can get a clock API for > this. I can't help but think this is a pretty ugly solution. Why not specify the nature of the cpu clock(s) in DT directly? There was a thread already that discussed adding such a property to the CPU DT binding but it seems to have gone cold[1]. Furthermore my mailer sucks and I see now that my response to that thread never hit the list due to mangled headers. Here is a copy/paste of my response to the aforementioned thread: """ I'll join the bikeshedding. The hardware property that matters for cpufreq-cpu0 users is that a multi-core CPU uses a single clock input to scale frequency across all of the cores in that cluster. So an accurate description is: scaling-method = "clock-ganged"; //hardware-people-speak Or, scaling-method = "clock-shared"; //software-people-speak Versus independently scalable CPUs in an SMP cluster: scaling-method = "independent"; //x86, Krait, etc. Or perhaps instead of "independent" at the parent "cpus" node we would put the following in each cpu@N node: scaling-method = "clock"; Or "psci" or "acpi" or whatever. Thought exercise: for Hyperthreaded(tm) CPUs with 2 virtual cores for every hard CPU (and multiple CPUs in a cluster): scaling-method = "paired"; Or more simply, "hyperthreaded". """ Regards, Mike [1] http://www.spinics.net/lists/cpufreq/msg10034.html > > > That being said, I don't really have any issue with such a function. > > Some comments on the implementation. > > >> +static int of_property_match(const struct device_node *np1, > >> + const struct device_node *np2, > >> + const char *list_name) > >> +{ > >> + const __be32 *list1, *list2, *list1_end; > > > > s/list/prop/ > > > > Everywhere. > > Ok. > > >> + int size1, size2; > >> + phandle phandle1, phandle2; > >> + > >> + /* Retrieve the list property */ > >> + list1 = of_get_property(np1, list_name, &size1); > >> + if (!list1) > >> + return -ENOENT; > >> + > >> + list2 = of_get_property(np2, list_name, &size2); > >> + if (!list2) > >> + return -ENOENT; > >> + > >> + if (size1 != size2) > >> + return 0; > >> + > >> + list1_end = list1 + size1 / sizeof(*list1); > >> + > >> + /* Loop over the phandles */ > >> + while (list1 < list1_end) { > >> + phandle1 = be32_to_cpup(list1++); > >> + phandle2 = be32_to_cpup(list2++); > >> + > >> + if (phandle1 != phandle2) > >> + return 0; > >> + } > > > > You can just do a memcmp here. > > Yeah, that would be much better. > > > This is wrong anyway because you don't know #clock-cells size. > > I was actually comparing all the clock-cells, whatever there number > is to make sure "clocks" properties are exactly same. Anyway > memcmp will still guarantee that. > > Thanks for your review.
WARNING: multiple messages have this Message-ID (diff)
From: mturquette@linaro.org (Mike Turquette) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] cpufreq: cpu0: Extend support beyond CPU0 Date: Tue, 01 Jul 2014 15:00:38 -0700 [thread overview] Message-ID: <20140701220038.32686.81633@quantum> (raw) In-Reply-To: <CAKohpokaVPn7yNPNAGrE6juGaOYuxZvVsbmkOZmyi9rDub6t7g@mail.gmail.com> Quoting Viresh Kumar (2014-07-01 04:14:04) > On 1 July 2014 00:03, Rob Herring <rob.herring@linaro.org> wrote: > >> What about comparing "clocks" property in cpu DT nodes? > > > > What if a different clock is selected for some reason. > > I don't know why that will happen for CPUs sharing clock line. > > > I think a clock api function would be better. > > @Mike: What do you think? I think we can get a clock API for > this. I can't help but think this is a pretty ugly solution. Why not specify the nature of the cpu clock(s) in DT directly? There was a thread already that discussed adding such a property to the CPU DT binding but it seems to have gone cold[1]. Furthermore my mailer sucks and I see now that my response to that thread never hit the list due to mangled headers. Here is a copy/paste of my response to the aforementioned thread: """ I'll join the bikeshedding. The hardware property that matters for cpufreq-cpu0 users is that a multi-core CPU uses a single clock input to scale frequency across all of the cores in that cluster. So an accurate description is: scaling-method = "clock-ganged"; //hardware-people-speak Or, scaling-method = "clock-shared"; //software-people-speak Versus independently scalable CPUs in an SMP cluster: scaling-method = "independent"; //x86, Krait, etc. Or perhaps instead of "independent" at the parent "cpus" node we would put the following in each cpu at N node: scaling-method = "clock"; Or "psci" or "acpi" or whatever. Thought exercise: for Hyperthreaded(tm) CPUs with 2 virtual cores for every hard CPU (and multiple CPUs in a cluster): scaling-method = "paired"; Or more simply, "hyperthreaded". """ Regards, Mike [1] http://www.spinics.net/lists/cpufreq/msg10034.html > > > That being said, I don't really have any issue with such a function. > > Some comments on the implementation. > > >> +static int of_property_match(const struct device_node *np1, > >> + const struct device_node *np2, > >> + const char *list_name) > >> +{ > >> + const __be32 *list1, *list2, *list1_end; > > > > s/list/prop/ > > > > Everywhere. > > Ok. > > >> + int size1, size2; > >> + phandle phandle1, phandle2; > >> + > >> + /* Retrieve the list property */ > >> + list1 = of_get_property(np1, list_name, &size1); > >> + if (!list1) > >> + return -ENOENT; > >> + > >> + list2 = of_get_property(np2, list_name, &size2); > >> + if (!list2) > >> + return -ENOENT; > >> + > >> + if (size1 != size2) > >> + return 0; > >> + > >> + list1_end = list1 + size1 / sizeof(*list1); > >> + > >> + /* Loop over the phandles */ > >> + while (list1 < list1_end) { > >> + phandle1 = be32_to_cpup(list1++); > >> + phandle2 = be32_to_cpup(list2++); > >> + > >> + if (phandle1 != phandle2) > >> + return 0; > >> + } > > > > You can just do a memcmp here. > > Yeah, that would be much better. > > > This is wrong anyway because you don't know #clock-cells size. > > I was actually comparing all the clock-cells, whatever there number > is to make sure "clocks" properties are exactly same. Anyway > memcmp will still guarantee that. > > Thanks for your review.
next prev parent reply other threads:[~2014-07-01 22:00 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-06-25 8:42 [PATCH 0/2] cpufreq: cpu0: Extend support beyond CPU0 Viresh Kumar 2014-06-25 8:42 ` Viresh Kumar 2014-06-25 8:42 ` [PATCH 1/2] cpufreq: Add support for per-policy driver data Viresh Kumar 2014-06-25 8:42 ` Viresh Kumar 2014-06-25 8:42 ` [PATCH 2/2] cpufreq: cpu0: Extend support beyond CPU0 Viresh Kumar 2014-06-25 8:42 ` Viresh Kumar 2014-06-25 19:02 ` Stephen Boyd 2014-06-25 19:02 ` Stephen Boyd 2014-06-26 1:55 ` Viresh Kumar 2014-06-26 1:55 ` Viresh Kumar 2014-06-26 1:55 ` Viresh Kumar 2014-06-26 7:34 ` Viresh Kumar 2014-06-26 7:34 ` Viresh Kumar 2014-06-26 7:34 ` Viresh Kumar 2014-06-26 10:52 ` Viresh Kumar 2014-06-26 10:52 ` Viresh Kumar 2014-06-26 10:52 ` Viresh Kumar 2014-06-27 0:06 ` Stephen Boyd 2014-06-27 0:06 ` Stephen Boyd 2014-06-27 0:06 ` Stephen Boyd 2014-06-27 1:53 ` Mike Turquette 2014-06-27 1:53 ` Mike Turquette 2014-06-27 1:53 ` Mike Turquette 2014-06-27 2:15 ` Viresh Kumar 2014-06-27 2:15 ` Viresh Kumar 2014-06-27 2:15 ` Viresh Kumar 2014-06-30 7:57 ` Viresh Kumar 2014-06-30 7:57 ` Viresh Kumar 2014-06-30 7:57 ` Viresh Kumar 2014-06-30 18:33 ` Rob Herring 2014-06-30 18:33 ` Rob Herring 2014-06-30 18:33 ` Rob Herring 2014-07-01 11:14 ` Viresh Kumar 2014-07-01 11:14 ` Viresh Kumar 2014-07-01 11:14 ` Viresh Kumar 2014-07-01 22:00 ` Mike Turquette [this message] 2014-07-01 22:00 ` Mike Turquette 2014-07-01 22:00 ` Mike Turquette 2014-07-02 3:32 ` Viresh Kumar 2014-07-02 3:32 ` Viresh Kumar 2014-07-02 3:32 ` Viresh Kumar 2014-06-27 2:26 ` Viresh Kumar 2014-06-27 2:26 ` Viresh Kumar 2014-06-27 2:26 ` Viresh Kumar 2014-06-26 22:08 ` Mark Brown 2014-06-26 22:08 ` Mark Brown 2014-06-28 14:52 ` Shawn Guo 2014-06-28 14:52 ` Shawn Guo 2014-06-28 14:52 ` Shawn Guo 2014-06-30 4:50 ` Viresh Kumar 2014-06-30 4:50 ` Viresh Kumar 2014-06-30 4:50 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140701220038.32686.81633@quantum \ --to=mturquette@linaro.org \ --cc=Mark.Rutland@arm.com \ --cc=arvind.chauhan@arm.com \ --cc=broonie@kernel.org \ --cc=grant.likely@linaro.org \ --cc=linaro-kernel@lists.linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=nm@ti.com \ --cc=rjw@rjwysocki.net \ --cc=rob.herring@linaro.org \ --cc=sboyd@codeaurora.org \ --cc=shawn.guo@linaro.org \ --cc=spk.linux@gmail.com \ --cc=t.figa@samsung.com \ --cc=thomas.ab@samsung.com \ --cc=viresh.kumar@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.