* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-19 15:54 ` [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver Shawn Guo
@ 2012-07-20 6:52 ` Shilimkar, Santosh
2012-07-20 12:33 ` Shawn Guo
2012-07-20 12:51 ` Richard Zhao
` (2 subsequent siblings)
3 siblings, 1 reply; 34+ messages in thread
From: Shilimkar, Santosh @ 2012-07-20 6:52 UTC (permalink / raw)
To: linux-arm-kernel
Shawn,
On Thu, Jul 19, 2012 at 9:24 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support. It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
[...]
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>
> If in doubt, say N.
>
> +config GENERIC_CPUFREQ_CPU0
> + bool "Generic cpufreq driver for CPU0"
> + depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> + select CPU_FREQ_TABLE
> + help
> + This adds a generic cpufreq driver for CPU0 frequency management.
> + It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> + systems which share clock and voltage across all CPUs.
> +
> + If in doubt, say N.
> +
This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
used on SMP systems where CPUs shares clock/voltage rails. May be you can
get rid of that unless there is a strong need.
[...]
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> + return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> + return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> + unsigned int target_freq, unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + struct opp *opp;
> + unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> + unsigned int index, cpu;
> + int ret = 0;
> +
> + ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> + relation, &index);
> + if (ret) {
> + pr_err("failed to match target freqency %d: %d\n",
> + target_freq, ret);
> + return ret;
> + }
> +
> + freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> + if (freq_Hz < 0)
> + freq_Hz = freq_table[index].frequency * 1000;
> + freqs.new = freq_Hz / 1000;
> + freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> + if (freqs.old == freqs.new)
> + return 0;
> +
> + for_each_cpu(cpu, policy->cpus) {
You need for_each_online_cpu() here o.w you will end up calling the
notifier on CPU which is are hot-plugged out or not online yet.
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> + }
> +
> + if (cpu_reg) {
> + opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> + if (IS_ERR(opp)) {
> + pr_err("failed to find OPP for %ld\n", freq_Hz);
> + return PTR_ERR(opp);
> + }
> + volt = opp_get_voltage(opp);
> + tol = volt * voltage_tolerance / 100;
> + volt_old = regulator_get_voltage(cpu_reg);
> + }
> +
> + pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> + freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> + freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> + /* scaling up? scale voltage before frequency */
> + if (cpu_reg && freqs.new > freqs.old) {
> + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> + pr_warn("Unable to scale voltage up\n");
> + freqs.new = freqs.old;
> + goto done;
Do you need a POST notifier in case of the failure ?
> + }
> + }
> +
> + ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> +
> + /* scaling down? scale voltage after frequency */
> + if (cpu_reg && (freqs.new < freqs.old)) {
> + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> + pr_warn("Unable to scale voltage down\n");
> + ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> + freqs.new = freqs.old;
> + goto done;
Here too.
> + }
> + }
> +
> +done:
> + for_each_cpu(cpu, policy->cpus) {
Same as first comment.
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + }
> +
> + return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> + struct device_node *np;
> + int ret;
> +
> + /* Initialize resources via CPU0 */
> + if (policy->cpu != 0)
> + return -EINVAL;
> +
> + np = of_find_node_by_path("/cpus/cpu at 0");
> + if (!np) {
> + pr_err("failed to find cpu0 node\n");
> + return -ENOENT;
> + }
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev) {
> + pr_err("failed to get cpu0 device\n");
> + ret = -ENODEV;
> + goto out_put_node;
> + }
> +
> + cpu_dev->of_node = np;
> + ret = of_init_opp_table(cpu_dev);
> + if (ret) {
> + pr_err("failed to init OPP table\n");
> + goto out_put_node;
> + }
> +
> + ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> + if (ret) {
> + pr_err("failed to init cpufreq table\n");
> + goto out_put_node;
> + }
> +
> + ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> + if (ret) {
> + pr_err("invalid frequency table for cpu0\n");
> + goto out_free_table;
> + }
> +
> + if (of_property_read_u32(np, "transition-latency",
> + &policy->cpuinfo.transition_latency))
> + policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> + of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> + cpu_clk = clk_get(cpu_dev, NULL);
> + if (IS_ERR(cpu_clk)) {
> + pr_err("failed to get cpu0 clock\n");
> + ret = PTR_ERR(cpu_clk);
> + goto out_free_table;
> + }
> + policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> + cpu_reg = regulator_get(cpu_dev, "cpu0");
> + if (IS_ERR(cpu_reg)) {
> + pr_warn("failed to get cpu0 regulator\n");
> + cpu_reg = NULL;
> + }
> +
> + /*
> + * The driver only supports the SMP configuartion where all processors
> + * share the clock and voltage and clock. Use cpufreq affected_cpus
> + * interface to have all CPUs scaled together.
> + */
> + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> + cpumask_setall(policy->cpus);
> +
> + cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> + of_node_put(np);
> + return 0;
> +
> +out_free_table:
> + opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> + of_node_put(np);
> + return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> + cpufreq_frequency_table_put_attr(policy->cpu);
> + regulator_put(cpu_reg);
> + clk_put(cpu_clk);
> + opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> + .flags = CPUFREQ_STICKY,
> + .verify = cpu0_verify_speed,
> + .target = cpu0_set_target,
> + .get = cpu0_get_speed,
> + .init = cpu0_cpufreq_init,
> + .exit = cpu0_cpufreq_exit,
> + .name = "generic_cpu0",
> + .attr = cpu0_cpufreq_attr,
> +};
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> + return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);
Can you also add support to build this as loadable module ?
Regards
santosh
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-20 6:52 ` Shilimkar, Santosh
@ 2012-07-20 12:33 ` Shawn Guo
2012-07-20 15:50 ` Turquette, Mike
0 siblings, 1 reply; 34+ messages in thread
From: Shawn Guo @ 2012-07-20 12:33 UTC (permalink / raw)
To: linux-arm-kernel
Thanks for the reviewing, Santosh.
On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
> > +config GENERIC_CPUFREQ_CPU0
> > + bool "Generic cpufreq driver for CPU0"
> > + depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> > + select CPU_FREQ_TABLE
> > + help
> > + This adds a generic cpufreq driver for CPU0 frequency management.
> > + It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> > + systems which share clock and voltage across all CPUs.
> > +
> > + If in doubt, say N.
> > +
> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
> used on SMP systems where CPUs shares clock/voltage rails. May be you can
> get rid of that unless there is a strong need.
>
All the resource handlers, clk, regulator, opp, DT node, are retrieved
from CPU0 device even for SMP. I hope the naming of the driver could
tell:
- The driver supports UP
- The driver supports SMP where CPUs shares clock/voltage rails by
managing CPU0 (resource)
- The driver does not support SMP where individual CPU can scale
clock/voltage independently.
I also thought about naming the driver cpufreq-coupled, but the name
misses the implication of UP support somehow, so I chose cpufreq-cpu0.
> > +static int cpu0_set_target(struct cpufreq_policy *policy,
> > + unsigned int target_freq, unsigned int relation)
> > +{
> > + struct cpufreq_freqs freqs;
> > + struct opp *opp;
> > + unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> > + unsigned int index, cpu;
> > + int ret = 0;
> > +
> > + ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> > + relation, &index);
> > + if (ret) {
> > + pr_err("failed to match target freqency %d: %d\n",
> > + target_freq, ret);
> > + return ret;
> > + }
> > +
> > + freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> > + if (freq_Hz < 0)
> > + freq_Hz = freq_table[index].frequency * 1000;
> > + freqs.new = freq_Hz / 1000;
> > + freqs.old = clk_get_rate(cpu_clk) / 1000;
> > +
> > + if (freqs.old == freqs.new)
> > + return 0;
> > +
> > + for_each_cpu(cpu, policy->cpus) {
> You need for_each_online_cpu() here o.w you will end up calling the
> notifier on CPU which is are hot-plugged out or not online yet.
>
Yes, that's sensible. Since we have all the CPUs set in policy->cpus
in this driver, we do not have to enumerate policy->cpus, and checking
online CPUs should be better for the cases you mentioned.
> > + freqs.cpu = cpu;
> > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > + }
> > +
> > + if (cpu_reg) {
> > + opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > + if (IS_ERR(opp)) {
> > + pr_err("failed to find OPP for %ld\n", freq_Hz);
> > + return PTR_ERR(opp);
> > + }
> > + volt = opp_get_voltage(opp);
> > + tol = volt * voltage_tolerance / 100;
> > + volt_old = regulator_get_voltage(cpu_reg);
> > + }
> > +
> > + pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> > + freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> > + freqs.new / 1000, volt ? volt / 1000 : -1);
> > +
> > + /* scaling up? scale voltage before frequency */
> > + if (cpu_reg && freqs.new > freqs.old) {
> > + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> > + pr_warn("Unable to scale voltage up\n");
> > + freqs.new = freqs.old;
> > + goto done;
> Do you need a POST notifier in case of the failure ?
>
Honestly, I'm not quite sure about that. This is a piece of code
reused from omap-cpufreq driver. Looking at cpufreq_notify_transition,
it seems to me that the POST notifier is not really necessary for
failure case.
> > +static struct cpufreq_driver cpu0_cpufreq_driver = {
> > + .flags = CPUFREQ_STICKY,
> > + .verify = cpu0_verify_speed,
> > + .target = cpu0_set_target,
> > + .get = cpu0_get_speed,
> > + .init = cpu0_cpufreq_init,
> > + .exit = cpu0_cpufreq_exit,
> > + .name = "generic_cpu0",
> > + .attr = cpu0_cpufreq_attr,
> > +};
> > +
> > +static int __devinit cpu0_cpufreq_driver_init(void)
> > +{
> > + return cpufreq_register_driver(&cpu0_cpufreq_driver);
> > +}
> > +late_initcall(cpu0_cpufreq_driver_init);
>
> Can you also add support to build this as loadable module ?
>
I'm just following the common pattern of ARM cpufreq drivers to have
CPUFREQ_STICKY set in cpufreq_driver.flag and use "bool" for the driver
Kconfig. I'm not sure about the necessity of building this as a module.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-20 12:33 ` Shawn Guo
@ 2012-07-20 15:50 ` Turquette, Mike
2012-07-21 5:04 ` Shilimkar, Santosh
0 siblings, 1 reply; 34+ messages in thread
From: Turquette, Mike @ 2012-07-20 15:50 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>> get rid of that unless there is a strong need.
>>
> All the resource handlers, clk, regulator, opp, DT node, are retrieved
> from CPU0 device even for SMP. I hope the naming of the driver could
> tell:
>
> - The driver supports UP
> - The driver supports SMP where CPUs shares clock/voltage rails by
> managing CPU0 (resource)
> - The driver does not support SMP where individual CPU can scale
> clock/voltage independently.
>
How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
That makes sense for both UP and SMP.
Regards,
Mike
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-20 15:50 ` Turquette, Mike
@ 2012-07-21 5:04 ` Shilimkar, Santosh
2012-07-21 6:38 ` Shawn Guo
0 siblings, 1 reply; 34+ messages in thread
From: Shilimkar, Santosh @ 2012-07-21 5:04 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> On Fri, Jul 20, 2012 at 5:33 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> On Fri, Jul 20, 2012 at 12:22:14PM +0530, Shilimkar, Santosh wrote:
>>> This *_CPU0 doesn't seems to be appropriate since this infrastructure will be
>>> used on SMP systems where CPUs shares clock/voltage rails. May be you can
>>> get rid of that unless there is a strong need.
>>>
>> All the resource handlers, clk, regulator, opp, DT node, are retrieved
>> from CPU0 device even for SMP. I hope the naming of the driver could
>> tell:
>>
>> - The driver supports UP
>> - The driver supports SMP where CPUs shares clock/voltage rails by
>> managing CPU0 (resource)
>> - The driver does not support SMP where individual CPU can scale
>> clock/voltage independently.
>>
>
> How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> That makes sense for both UP and SMP.
>
Indeed. This sounds more appropriate and also reflects what actually happens
with a UP or shared clock SMP case.
Regards
Santosh
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-21 5:04 ` Shilimkar, Santosh
@ 2012-07-21 6:38 ` Shawn Guo
2012-07-27 2:04 ` Richard Zhao
0 siblings, 1 reply; 34+ messages in thread
From: Shawn Guo @ 2012-07-21 6:38 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > That makes sense for both UP and SMP.
> >
> Indeed. This sounds more appropriate and also reflects what actually happens
> with a UP or shared clock SMP case.
>
While I agree with this observation, the suggested naming does not
reflect the rationale of CPU0 though, which is really important for
driver to work, and is exactly the thing I like about *_CPU0 naming.
The driver needs the following stuff around CPU0 to be functional.
- Device tree node /cpus/cpu at 0
- clk lookup with dev_id being "cpu0"
- regulator with id being "cpu0"
I think the *_CPU0 naming does a better job on emphasising those in the
first place.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-21 6:38 ` Shawn Guo
@ 2012-07-27 2:04 ` Richard Zhao
2012-07-30 4:57 ` Shawn Guo
0 siblings, 1 reply; 34+ messages in thread
From: Richard Zhao @ 2012-07-27 2:04 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Jul 21, 2012 at 02:38:13PM +0800, Shawn Guo wrote:
> On Sat, Jul 21, 2012 at 10:34:29AM +0530, Shilimkar, Santosh wrote:
> > On Fri, Jul 20, 2012 at 9:20 PM, Turquette, Mike <mturquette@ti.com> wrote:
> > > How about cpufreq-single-thread.c and CONFIG_CPUFREQ_SINGLE_THREAD?
> > > That makes sense for both UP and SMP.
> > >
> > Indeed. This sounds more appropriate and also reflects what actually happens
> > with a UP or shared clock SMP case.
> >
> While I agree with this observation, the suggested naming does not
> reflect the rationale of CPU0 though, which is really important for
> driver to work, and is exactly the thing I like about *_CPU0 naming.
>
> The driver needs the following stuff around CPU0 to be functional.
>
> - Device tree node /cpus/cpu at 0
> - clk lookup with dev_id being "cpu0"
> - regulator with id being "cpu0"
>
> I think the *_CPU0 naming does a better job on emphasising those in the
> first place.
Are we going to support the case in the future that cores has
independent freq? If yes, maybe we should have a more common name.
Thanks
Richard
>
> --
> Regards,
> Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-19 15:54 ` [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver Shawn Guo
2012-07-20 6:52 ` Shilimkar, Santosh
@ 2012-07-20 12:51 ` Richard Zhao
2012-07-20 13:15 ` Shawn Guo
2012-07-26 13:11 ` Mark Brown
2012-07-27 6:33 ` Richard Zhao
3 siblings, 1 reply; 34+ messages in thread
From: Richard Zhao @ 2012-07-20 12:51 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Shawn,
I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
Is there any reason besides adding opp support?
Thanks
Richard
--
?????????????????????K-9 Mail?Android????????
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-20 12:51 ` Richard Zhao
@ 2012-07-20 13:15 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2012-07-20 13:15 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 20, 2012 at 08:51:17PM +0800, Richard Zhao wrote:
> Hi, Shawn,
>
> I find you create a new driver rather than work on my generic cpufreq driver based on clk and regulator.
> Is there any reason besides adding opp support?
>
Adopting OPP support forces me to look at omap-cpufreq driver, hence
makes me create a new driver by referencing to omap-cpufreq.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-19 15:54 ` [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver Shawn Guo
2012-07-20 6:52 ` Shilimkar, Santosh
2012-07-20 12:51 ` Richard Zhao
@ 2012-07-26 13:11 ` Mark Brown
2012-07-27 2:13 ` Richard Zhao
2012-07-30 6:52 ` Shawn Guo
2012-07-27 6:33 ` Richard Zhao
3 siblings, 2 replies; 34+ messages in thread
From: Mark Brown @ 2012-07-26 13:11 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> + in unit of nanoseconds.
This should make it clear that the transition latency being documented
here is just that for the core clock change itself, there may be other
sources of latency like the regulator ramp time or reprogramming PLLs.
> + if (cpu_reg) {
> + opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> + if (IS_ERR(opp)) {
> + pr_err("failed to find OPP for %ld\n", freq_Hz);
> + return PTR_ERR(opp);
> + }
> + volt = opp_get_voltage(opp);
> + tol = volt * voltage_tolerance / 100;
> + volt_old = regulator_get_voltage(cpu_reg);
regulator_get_voltage() might be a bit expensive, many regulators will
query the hardware every time. Since it's just for diagnostic purposes
it'd be better to not bother and let people read the historical
information from the logs.
> + /* scaling up? scale voltage before frequency */
> + if (cpu_reg && freqs.new > freqs.old) {
> + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
This is a totally sane and sensible use case for specifying a voltage
range, we should move it into the regulator core for other users so you
can just specify the voltage and the tolerance directly.
It is a little sad here as it means that we loose the ability to do
frequency only scaling if there's no regulator which is nice.
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-26 13:11 ` Mark Brown
@ 2012-07-27 2:13 ` Richard Zhao
2012-07-27 10:08 ` Mark Brown
2012-07-30 6:52 ` Shawn Guo
1 sibling, 1 reply; 34+ messages in thread
From: Richard Zhao @ 2012-07-27 2:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
>
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > + in unit of nanoseconds.
>
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
I think it's the total time and board dts can over-write it if it
needs. Different transitions between different operating points may
differ, and regulator may be able to indicate the transition time but
clk don't have such api, and probably not worth to have.
Thanks
Richard
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-27 2:13 ` Richard Zhao
@ 2012-07-27 10:08 ` Mark Brown
0 siblings, 0 replies; 34+ messages in thread
From: Mark Brown @ 2012-07-27 10:08 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 27, 2012 at 10:13:04AM +0800, Richard Zhao wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> > > +Optional properties:
> > > +- transition-latency: Specify the possible maximum transition latency,
> > > + in unit of nanoseconds.
> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.
> I think it's the total time and board dts can over-write it if it
> needs. Different transitions between different operating points may
> differ, and regulator may be able to indicate the transition time but
> clk don't have such api, and probably not worth to have.
That's going to be awfully manual if every board has to tweak values
(though obviously the main effect is just going to be bad decisions
rather than breakage). I've seen several systems where the clock could
provide useful timing input here - the usual pattern is that you've got
a PLL which you can use as well as some dividers. Transitions that only
need a divider update are extremely quick but transitions that change
the PLL setup can take much longer. It seems better to just allow the
board maintainer to plug everything together rather than having to work
out their specific latencies to squeeze the performance out of the
system.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120727/673349c5/attachment.sig>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-26 13:11 ` Mark Brown
2012-07-27 2:13 ` Richard Zhao
@ 2012-07-30 6:52 ` Shawn Guo
2012-07-30 8:20 ` Shawn Guo
2012-07-30 18:53 ` Mark Brown
1 sibling, 2 replies; 34+ messages in thread
From: Shawn Guo @ 2012-07-30 6:52 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
>
> > +Optional properties:
> > +- transition-latency: Specify the possible maximum transition latency,
> > + in unit of nanoseconds.
>
> This should make it clear that the transition latency being documented
> here is just that for the core clock change itself, there may be other
> sources of latency like the regulator ramp time or reprogramming PLLs.
>
I intended to make it be the total latency from whatever sources that
should be counted on particular system. Rather than adding complexity
for the driver to figure out these latencies from every single source,
I would expect that people know the possible maximum latency in total
for their systems and specify it in device tree.
> > + if (cpu_reg) {
> > + opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> > + if (IS_ERR(opp)) {
> > + pr_err("failed to find OPP for %ld\n", freq_Hz);
> > + return PTR_ERR(opp);
> > + }
> > + volt = opp_get_voltage(opp);
> > + tol = volt * voltage_tolerance / 100;
> > + volt_old = regulator_get_voltage(cpu_reg);
>
> regulator_get_voltage() might be a bit expensive, many regulators will
> query the hardware every time. Since it's just for diagnostic purposes
> it'd be better to not bother and let people read the historical
> information from the logs.
>
Ok, makes sense to me.
> > + /* scaling up? scale voltage before frequency */
> > + if (cpu_reg && freqs.new > freqs.old) {
> > + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
>
> This is a totally sane and sensible use case for specifying a voltage
> range, we should move it into the regulator core for other users so you
> can just specify the voltage and the tolerance directly.
>
Are you asking for a change on regulator_set_voltage API? While I agree
with your comment, it's not a thing we need to necessarily do in this
series. The change on the API requires a touch on all the existing
users. I do not think it's nice to carry such a patch in this series.
> It is a little sad here as it means that we loose the ability to do
> frequency only scaling if there's no regulator which is nice.
I do not understand it. The regulator is optional for the driver, and
we can still scale frequency even if there is no regulator.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 6:52 ` Shawn Guo
@ 2012-07-30 8:20 ` Shawn Guo
2012-07-30 18:55 ` Mark Brown
2012-07-30 18:53 ` Mark Brown
1 sibling, 1 reply; 34+ messages in thread
From: Shawn Guo @ 2012-07-30 8:20 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > regulator_get_voltage() might be a bit expensive, many regulators will
> > query the hardware every time. Since it's just for diagnostic purposes
> > it'd be better to not bother and let people read the historical
> > information from the logs.
> >
> Ok, makes sense to me.
>
In the reply to one of Richard's comment, the volt_old will be used
not only for diagnostic purpose but also for reverting voltage back
when clk_set_rate fails.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 8:20 ` Shawn Guo
@ 2012-07-30 18:55 ` Mark Brown
0 siblings, 0 replies; 34+ messages in thread
From: Mark Brown @ 2012-07-30 18:55 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 04:20:54PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> > Ok, makes sense to me.
> In the reply to one of Richard's comment, the volt_old will be used
> not only for diagnostic purpose but also for reverting voltage back
> when clk_set_rate fails.
We should be able to figure out what operating point we set without
having to read it back from the hardware like this.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 6:52 ` Shawn Guo
2012-07-30 8:20 ` Shawn Guo
@ 2012-07-30 18:53 ` Mark Brown
2012-07-31 4:20 ` Shawn Guo
1 sibling, 1 reply; 34+ messages in thread
From: Mark Brown @ 2012-07-30 18:53 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 02:52:20PM +0800, Shawn Guo wrote:
> On Thu, Jul 26, 2012 at 02:11:21PM +0100, Mark Brown wrote:
> > This should make it clear that the transition latency being documented
> > here is just that for the core clock change itself, there may be other
> > sources of latency like the regulator ramp time or reprogramming PLLs.
> I intended to make it be the total latency from whatever sources that
> should be counted on particular system. Rather than adding complexity
> for the driver to figure out these latencies from every single source,
> I would expect that people know the possible maximum latency in total
> for their systems and specify it in device tree.
Quite honestly this seems totally unrealistic for the majority of users,
especially given the very poor documentation for this stuff which SoC
vendors typically provide. It's a reasonable amount of work to go back
and figure this stuff out (especially given that it should be varying
depending on the transition in question), and it's going to give us a
bunch of magic numbers in people's bindings.
> > > + if (cpu_reg && freqs.new > freqs.old) {
> > > + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> > This is a totally sane and sensible use case for specifying a voltage
> > range, we should move it into the regulator core for other users so you
> > can just specify the voltage and the tolerance directly.
> Are you asking for a change on regulator_set_voltage API? While I agree
> with your comment, it's not a thing we need to necessarily do in this
> series. The change on the API requires a touch on all the existing
> users. I do not think it's nice to carry such a patch in this series.
No, add a new API.
> > It is a little sad here as it means that we loose the ability to do
> > frequency only scaling if there's no regulator which is nice.
> I do not understand it. The regulator is optional for the driver, and
> we can still scale frequency even if there is no regulator.
As soon as a regulator is available your code will insist that we're
able to set the voltage specified to set the frequency, and since it's
using tolerances not ranges to whatever the chip maximum is that'll mean
it'll stop doing frequency scaling at all.
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 18:53 ` Mark Brown
@ 2012-07-31 4:20 ` Shawn Guo
2012-07-31 13:40 ` Mark Brown
0 siblings, 1 reply; 34+ messages in thread
From: Shawn Guo @ 2012-07-31 4:20 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:
> Quite honestly this seems totally unrealistic for the majority of users,
I doubt that. Searching drivers/cpufreq folder, I can see there are
several cpufreq drivers scaling voltage with regulator API, but none
of them is calling regulator_set_voltage_time to find voltage latency.
That said, all these users are specify transition latency on their own.
> especially given the very poor documentation for this stuff which SoC
> vendors typically provide. It's a reasonable amount of work to go back
> and figure this stuff out (especially given that it should be varying
> depending on the transition in question), and it's going to give us a
> bunch of magic numbers in people's bindings.
>
There will be only one magic number, and it can easily become "magic"
with some comments put there.
> No, add a new API.
>
Is the following patch what you are ordering here?
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index da339fd..bfd3cfb 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -352,4 +352,11 @@ static inline void regulator_set_drvdata(struct regulator *regulator,
#endif
+static inline int regulator_set_voltage_tolerance(struct regulator *regulator,
+ int new_uV, int tol_uV)
+{
+ return regulator_set_voltage(regulator,
+ new_uV - tol_uV, new_uV + tol_uV);
+}
+
#endif
--
Regards,
Shawn
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-31 4:20 ` Shawn Guo
@ 2012-07-31 13:40 ` Mark Brown
0 siblings, 0 replies; 34+ messages in thread
From: Mark Brown @ 2012-07-31 13:40 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 31, 2012 at 12:20:52PM +0800, Shawn Guo wrote:
> On Mon, Jul 30, 2012 at 07:53:45PM +0100, Mark Brown wrote:
> > Quite honestly this seems totally unrealistic for the majority of users,
> I doubt that. Searching drivers/cpufreq folder, I can see there are
> several cpufreq drivers scaling voltage with regulator API, but none
> of them is calling regulator_set_voltage_time to find voltage latency.
> That said, all these users are specify transition latency on their own.
This is more a sign of poor quality of implementation than anything
else, and obviously the fact that DT is supposed to be an ABI makes it
much more important to get things right.
There's also a bunch of problems in ondemand, it really doesn't cope
terribly well if you actually tell it what the latencies are for most
systems as it becomes absurdly conservative about not ramping.
> > especially given the very poor documentation for this stuff which SoC
> > vendors typically provide. It's a reasonable amount of work to go back
> > and figure this stuff out (especially given that it should be varying
> > depending on the transition in question), and it's going to give us a
> > bunch of magic numbers in people's bindings.
> There will be only one magic number, and it can easily become "magic"
> with some comments put there.
This doesn't sound like a recipie for success..
> > No, add a new API.
> Is the following patch what you are ordering here?
Yes. I'd go with _tol rather than _tolerance probably but something
like that, yes.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-19 15:54 ` [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver Shawn Guo
` (2 preceding siblings ...)
2012-07-26 13:11 ` Mark Brown
@ 2012-07-27 6:33 ` Richard Zhao
2012-07-30 8:17 ` Shawn Guo
3 siblings, 1 reply; 34+ messages in thread
From: Richard Zhao @ 2012-07-27 6:33 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 19, 2012 at 11:54:41PM +0800, Shawn Guo wrote:
> It adds a generic cpufreq driver for CPU0 frequency management based on
> clk, regulator, OPP and device tree support. It can support both
> uniprocessor (UP) and those symmetric multiprocessor (SMP) systems which
> share clock and voltage across all CPUs.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 58 +++++
> drivers/cpufreq/Kconfig | 11 +
> drivers/cpufreq/Makefile | 2 +
> drivers/cpufreq/cpufreq-cpu0.c | 235 ++++++++++++++++++++
> 4 files changed, 306 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> create mode 100644 drivers/cpufreq/cpufreq-cpu0.c
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> new file mode 100644
> index 0000000..94799bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> @@ -0,0 +1,58 @@
> +Generic cpufreq driver for CPU0
It's going to have generic name if it will become more generic.
> +
> +It is a generic cpufreq driver for CPU0 frequency management. It
> +supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> +systems which share clock and voltage across all CPUs.
> +
> +Both required and optional properties listed below must be defined
> +under node /cpus/cpu at 0.
> +
> +Required properties:
> +- operating-points: Refer to Documentation/devicetree/bindings/power/opp.txt
> + for details
> +
> +Optional properties:
> +- transition-latency: Specify the possible maximum transition latency,
> + in unit of nanoseconds.
> +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
Why do we have the same tolerance for all points? I think you can
either remove tolerance or add it to opps.
> +
> +Examples:
> +
> +cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu at 0 {
> + compatible = "arm,cortex-a9";
> + reg = <0>;
> + next-level-cache = <&L2>;
> + operating-points = <
> + /* kHz uV en */
> + 1200000 1275000 0
> + 996000 1225000 1
> + 792000 1100000 1
> + 672000 1100000 0
> + 396000 950000 1
> + 198000 850000 1
> + >;
> + transition-latency = <61036>; /* two CLK32 periods */
> + };
> +
> + cpu at 1 {
> + compatible = "arm,cortex-a9";
> + reg = <1>;
> + next-level-cache = <&L2>;
> + };
> +
> + cpu at 2 {
> + compatible = "arm,cortex-a9";
> + reg = <2>;
> + next-level-cache = <&L2>;
> + };
> +
> + cpu at 3 {
> + compatible = "arm,cortex-a9";
> + reg = <3>;
> + next-level-cache = <&L2>;
> + };
> +};
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..e77a1e0 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,17 @@ config CPU_FREQ_GOV_CONSERVATIVE
>
> If in doubt, say N.
>
> +config GENERIC_CPUFREQ_CPU0
> + bool "Generic cpufreq driver for CPU0"
> + depends on HAVE_CLK && REGULATOR && PM_OPP && OF
> + select CPU_FREQ_TABLE
> + help
> + This adds a generic cpufreq driver for CPU0 frequency management.
> + It supports both uniprocessor (UP) and symmetric multiprocessor (SMP)
> + systems which share clock and voltage across all CPUs.
> +
> + If in doubt, say N.
> +
> menu "x86 CPU frequency scaling drivers"
> depends on X86
> source "drivers/cpufreq/Kconfig.x86"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 9531fc2..a378ed2 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) += cpufreq_conservative.o
> # CPUfreq cross-arch helpers
> obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
>
> +obj-$(CONFIG_GENERIC_CPUFREQ_CPU0) += cpufreq-cpu0.o
> +
> ##################################################################################
> # x86 drivers.
> # Link order matters. K8 is preferred to ACPI because of firmware bugs in early
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
> new file mode 100644
> index 0000000..f1bb7f37
> --- /dev/null
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Reuse some codes from drivers/cpufreq/omap-cpufreq.c
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +/* voltage tolerance in percentage */
> +static unsigned int voltage_tolerance;
> +
> +static struct device *cpu_dev;
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int cpu0_verify_speed(struct cpufreq_policy *policy)
> +{
> + return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int cpu0_get_speed(unsigned int cpu)
> +{
> + return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int cpu0_set_target(struct cpufreq_policy *policy,
> + unsigned int target_freq, unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + struct opp *opp;
> + unsigned long freq_Hz, volt = 0, volt_old = 0, tol = 0;
> + unsigned int index, cpu;
> + int ret = 0;
> +
> + ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
> + relation, &index);
> + if (ret) {
> + pr_err("failed to match target freqency %d: %d\n",
> + target_freq, ret);
> + return ret;
> + }
> +
> + freq_Hz = clk_round_rate(cpu_clk, freq_table[index].frequency * 1000);
> + if (freq_Hz < 0)
> + freq_Hz = freq_table[index].frequency * 1000;
> + freqs.new = freq_Hz / 1000;
> + freqs.old = clk_get_rate(cpu_clk) / 1000;
> +
> + if (freqs.old == freqs.new)
> + return 0;
> +
> + for_each_cpu(cpu, policy->cpus) {
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> + }
> +
> + if (cpu_reg) {
> + opp = opp_find_freq_ceil(cpu_dev, &freq_Hz);
> + if (IS_ERR(opp)) {
> + pr_err("failed to find OPP for %ld\n", freq_Hz);
> + return PTR_ERR(opp);
> + }
> + volt = opp_get_voltage(opp);
> + tol = volt * voltage_tolerance / 100;
> + volt_old = regulator_get_voltage(cpu_reg);
> + }
> +
> + pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> + freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
> + freqs.new / 1000, volt ? volt / 1000 : -1);
> +
> + /* scaling up? scale voltage before frequency */
> + if (cpu_reg && freqs.new > freqs.old) {
> + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> + pr_warn("Unable to scale voltage up\n");
> + freqs.new = freqs.old;
> + goto done;
> + }
> + }
> +
> + ret = clk_set_rate(cpu_clk, freqs.new * 1000);
Check return value and fall back to previous point if it needs?
> +
> + /* scaling down? scale voltage after frequency */
> + if (cpu_reg && (freqs.new < freqs.old)) {
> + if (regulator_set_voltage(cpu_reg, volt - tol, volt + tol)) {
> + pr_warn("Unable to scale voltage down\n");
> + ret = clk_set_rate(cpu_clk, freqs.old * 1000);
> + freqs.new = freqs.old;
> + goto done;
> + }
> + }
> +
> +done:
> + for_each_cpu(cpu, policy->cpus) {
> + freqs.cpu = cpu;
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + }
> +
> + return ret;
> +}
> +
> +static int cpu0_cpufreq_init(struct cpufreq_policy *policy)
> +{
> + struct device_node *np;
> + int ret;
> +
> + /* Initialize resources via CPU0 */
> + if (policy->cpu != 0)
> + return -EINVAL;
> +
> + np = of_find_node_by_path("/cpus/cpu at 0");
> + if (!np) {
> + pr_err("failed to find cpu0 node\n");
> + return -ENOENT;
> + }
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev) {
> + pr_err("failed to get cpu0 device\n");
> + ret = -ENODEV;
> + goto out_put_node;
> + }
> +
> + cpu_dev->of_node = np;
hmm.. sys dev can not set of_node when populate it?
And why not do it in module init?
> + ret = of_init_opp_table(cpu_dev);
> + if (ret) {
> + pr_err("failed to init OPP table\n");
> + goto out_put_node;
> + }
> +
> + ret = opp_init_cpufreq_table(cpu_dev, &freq_table);
> + if (ret) {
> + pr_err("failed to init cpufreq table\n");
> + goto out_put_node;
> + }
> +
> + ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> + if (ret) {
> + pr_err("invalid frequency table for cpu0\n");
> + goto out_free_table;
> + }
> +
> + if (of_property_read_u32(np, "transition-latency",
> + &policy->cpuinfo.transition_latency))
> + policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> +
> + of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
> +
> + cpu_clk = clk_get(cpu_dev, NULL);
> + if (IS_ERR(cpu_clk)) {
> + pr_err("failed to get cpu0 clock\n");
> + ret = PTR_ERR(cpu_clk);
> + goto out_free_table;
> + }
> + policy->cur = clk_get_rate(cpu_clk) / 1000;
> +
> + cpu_reg = regulator_get(cpu_dev, "cpu0");
> + if (IS_ERR(cpu_reg)) {
> + pr_warn("failed to get cpu0 regulator\n");
> + cpu_reg = NULL;
> + }
> +
> + /*
> + * The driver only supports the SMP configuartion where all processors
> + * share the clock and voltage and clock. Use cpufreq affected_cpus
> + * interface to have all CPUs scaled together.
> + */
> + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> + cpumask_setall(policy->cpus);
> +
> + cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> +
> + of_node_put(np);
> + return 0;
> +
> +out_free_table:
> + opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_node:
> + of_node_put(np);
> + return ret;
> +}
> +
> +static int cpu0_cpufreq_exit(struct cpufreq_policy *policy)
> +{
> + cpufreq_frequency_table_put_attr(policy->cpu);
> + regulator_put(cpu_reg);
> + clk_put(cpu_clk);
> + opp_free_cpufreq_table(cpu_dev, &freq_table);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *cpu0_cpufreq_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> +};
> +
> +static struct cpufreq_driver cpu0_cpufreq_driver = {
> + .flags = CPUFREQ_STICKY,
> + .verify = cpu0_verify_speed,
> + .target = cpu0_set_target,
> + .get = cpu0_get_speed,
> + .init = cpu0_cpufreq_init,
> + .exit = cpu0_cpufreq_exit,
> + .name = "generic_cpu0",
> + .attr = cpu0_cpufreq_attr,
> +};
static u32 max_freq = UINT_MAX / 1000; /* kHz */
module_param(max_freq, uint, 0);
MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
It's for debug. Make sense?
Thanks
Richard
> +
> +static int __devinit cpu0_cpufreq_driver_init(void)
> +{
> + return cpufreq_register_driver(&cpu0_cpufreq_driver);
> +}
> +late_initcall(cpu0_cpufreq_driver_init);
> +
> +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> +MODULE_DESCRIPTION("Generic cpufreq driver for CPU0");
> +MODULE_LICENSE("GPL");
> --
> 1.7.5.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-27 6:33 ` Richard Zhao
@ 2012-07-30 8:17 ` Shawn Guo
2012-07-30 8:50 ` Richard Zhao
0 siblings, 1 reply; 34+ messages in thread
From: Shawn Guo @ 2012-07-30 8:17 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > +Generic cpufreq driver for CPU0
> It's going to have generic name if it will become more generic.
>
I'm not in the position to say that it will become even more generic.
> > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> Why do we have the same tolerance for all points?
Because I haven't seen any case that needs different tolerance for
different operating points.
> I think you can
> either remove tolerance or add it to opps.
I'm not going to remove it, because I have seen potential cpufreq-cpu0
candidates, e.g. omap-cpufreq, need it. It's also improper to encode
it in operating-points, since OPP library does not have it.
> > + ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> Check return value and fall back to previous point if it needs?
Right, the voltage should be reverted back if clk_set_rate fails.
> > + cpu_dev->of_node = np;
> hmm.. sys dev can not set of_node when populate it?
Since the sys dev is not populated from device tree, the of_node is
not set, and we have to do it here on our own.
> And why not do it in module init?
What's the advantage of doing it in module init over here?
> static u32 max_freq = UINT_MAX / 1000; /* kHz */
> module_param(max_freq, uint, 0);
> MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
>
> It's for debug. Make sense?
>
It does not look so useful to me, as it never came to me when I was
debugging the driver.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 8:17 ` Shawn Guo
@ 2012-07-30 8:50 ` Richard Zhao
2012-07-30 9:24 ` Shawn Guo
0 siblings, 1 reply; 34+ messages in thread
From: Richard Zhao @ 2012-07-30 8:50 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 04:17:53PM +0800, Shawn Guo wrote:
> On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:
> > > +Generic cpufreq driver for CPU0
> > It's going to have generic name if it will become more generic.
> >
> I'm not in the position to say that it will become even more generic.
>
> > > +- voltage-tolerance: Specify the CPU voltage tolerance in percentage.
> > Why do we have the same tolerance for all points?
>
> Because I haven't seen any case that needs different tolerance for
> different operating points.
>
> > I think you can
> > either remove tolerance or add it to opps.
>
> I'm not going to remove it, because I have seen potential cpufreq-cpu0
> candidates, e.g. omap-cpufreq, need it. It's also improper to encode
> it in operating-points, since OPP library does not have it.
I think the real problem here is OPP only provide exact voltage but
regulator api needs a range.
>
> > > + ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> > Check return value and fall back to previous point if it needs?
>
> Right, the voltage should be reverted back if clk_set_rate fails.
>
> > > + cpu_dev->of_node = np;
> > hmm.. sys dev can not set of_node when populate it?
>
> Since the sys dev is not populated from device tree, the of_node is
> not set, and we have to do it here on our own.
It sounds strange. But I think it's ok.
>
> > And why not do it in module init?
>
> What's the advantage of doing it in module init over here?
IIRC, cpufreq driver init is called every time a new cpu get online.
So things that only needs to be done once can be put in module init.
>
> > static u32 max_freq = UINT_MAX / 1000; /* kHz */
> > module_param(max_freq, uint, 0);
> > MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");
> >
> > It's for debug. Make sense?
> >
> It does not look so useful to me, as it never came to me when I was
> debugging the driver.
Alright.
Thanks
Richard
>
> --
> Regards,
> Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] cpufreq: Add a generic cpufreq-cpu0 driver
2012-07-30 8:50 ` Richard Zhao
@ 2012-07-30 9:24 ` Shawn Guo
0 siblings, 0 replies; 34+ messages in thread
From: Shawn Guo @ 2012-07-30 9:24 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 30, 2012 at 04:50:41PM +0800, Richard Zhao wrote:
> I think the real problem here is OPP only provide exact voltage but
> regulator api needs a range.
>
Maybe OPP will get improved on that when the real use cases occur.
> IIRC, cpufreq driver init is called every time a new cpu get online.
> So things that only needs to be done once can be put in module init.
>
Ah, right. Something should be moved to module init then.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 34+ messages in thread