* Build failures in -next due to move of disable @ 2012-07-02 12:37 Mark Brown 2012-07-02 19:54 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Mark Brown @ 2012-07-02 12:37 UTC (permalink / raw) To: ShuoX Liu, Andrew Morton, Rafael J. Wysocki; +Cc: linux-pm, linux-kernel Today's -next is failing to build for me because commit 9f419c (PM / Domains: Add preliminary support for cpuidle) and commit b32e30 (cpuidle: move field disable from per-driver to per-cpu) disagree about where the disable field should be: CC drivers/base/power/domain.o drivers/base/power/domain.c: In function '__pm_genpd_poweron': drivers/base/power/domain.c:194: error: 'struct cpuidle_state' has no member named 'disable' drivers/base/power/domain.c: In function 'pm_genpd_poweroff': drivers/base/power/domain.c:490: error: 'struct cpuidle_state' has no member named 'disable' drivers/base/power/domain.c: In function 'genpd_attach_cpuidle': drivers/base/power/domain.c:1671: error: 'struct cpuidle_state' has no member named 'disable' drivers/base/power/domain.c: In function 'genpd_detach_cpuidle': drivers/base/power/domain.c:1706: error: 'struct cpuidle_state' has no member named 'disable' ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Build failures in -next due to move of disable 2012-07-02 12:37 Build failures in -next due to move of disable Mark Brown @ 2012-07-02 19:54 ` Rafael J. Wysocki 2012-07-02 19:53 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2012-07-02 19:54 UTC (permalink / raw) To: Mark Brown; +Cc: ShuoX Liu, Andrew Morton, linux-pm, linux-kernel On Monday, July 02, 2012, Mark Brown wrote: > Today's -next is failing to build for me because commit 9f419c (PM / > Domains: Add preliminary support for cpuidle) and commit b32e30 > (cpuidle: move field disable from per-driver to per-cpu) disagree about > where the disable field should be: Hmm, do you know what tree commit b32e30 comes from? Rafael > > CC drivers/base/power/domain.o > drivers/base/power/domain.c: In function '__pm_genpd_poweron': > drivers/base/power/domain.c:194: error: 'struct cpuidle_state' has no member named 'disable' > drivers/base/power/domain.c: In function 'pm_genpd_poweroff': > drivers/base/power/domain.c:490: error: 'struct cpuidle_state' has no member named 'disable' > drivers/base/power/domain.c: In function 'genpd_attach_cpuidle': > drivers/base/power/domain.c:1671: error: 'struct cpuidle_state' has no member named 'disable' > drivers/base/power/domain.c: In function 'genpd_detach_cpuidle': > drivers/base/power/domain.c:1706: error: 'struct cpuidle_state' has no member named 'disable' > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Build failures in -next due to move of disable 2012-07-02 19:54 ` Rafael J. Wysocki @ 2012-07-02 19:53 ` Andrew Morton 2012-07-02 20:16 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2012-07-02 19:53 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Mark Brown, ShuoX Liu, linux-pm, linux-kernel On Mon, 2 Jul 2012 21:54:35 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > On Monday, July 02, 2012, Mark Brown wrote: > > Today's -next is failing to build for me because commit 9f419c (PM / > > Domains: Add preliminary support for cpuidle) and commit b32e30 > > (cpuidle: move field disable from per-driver to per-cpu) disagree about > > where the disable field should be: > > Hmm, do you know what tree commit b32e30 comes from? From: ShuoX Liu <shuox.liu@intel.com> Subject: cpuidle: move field disable from per-driver to per-cpu Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine. The patch changes the `disable' field to per-cpu, so we could set this separately for each cpu. Signed-off-by: ShuoX Liu <shuox.liu@intel.com> Reported-by: Andrew J.Schorr <aschorr@telemetry-investments.com> Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com> Cc: Len Brown <lenb@kernel.org> Cc: "Brown, Len" <len.brown@intel.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- drivers/cpuidle/cpuidle.c | 1 - drivers/cpuidle/governors/menu.c | 5 +++-- drivers/cpuidle/sysfs.c | 21 ++++++++++++--------- include/linux/cpuidle.h | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-) diff -puN drivers/cpuidle/cpuidle.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/cpuidle.c --- a/drivers/cpuidle/cpuidle.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu +++ a/drivers/cpuidle/cpuidle.c @@ -276,7 +276,6 @@ static void poll_idle_init(struct cpuidl state->power_usage = -1; state->flags = 0; state->enter = poll_idle; - state->disable = 0; } #else static void poll_idle_init(struct cpuidle_driver *drv) {} diff -puN drivers/cpuidle/governors/menu.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/governors/menu.c --- a/drivers/cpuidle/governors/menu.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu +++ a/drivers/cpuidle/governors/menu.c @@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_dr * unless the timer is happening really really soon. */ if (data->expected_us > 5 && - drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0) + dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; /* @@ -290,8 +290,9 @@ static int menu_select(struct cpuidle_dr */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; + struct cpuidle_state_usage *su = &dev->states_usage[i]; - if (s->disable) + if (su->disable) continue; if (s->target_residency > data->predicted_us) continue; diff -puN drivers/cpuidle/sysfs.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/sysfs.c --- a/drivers/cpuidle/sysfs.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu +++ a/drivers/cpuidle/sysfs.c @@ -217,7 +217,8 @@ struct cpuidle_state_attr { struct attribute attr; ssize_t (*show)(struct cpuidle_state *, \ struct cpuidle_state_usage *, char *); - ssize_t (*store)(struct cpuidle_state *, const char *, size_t); + ssize_t (*store)(struct cpuidle_state *, \ + struct cpuidle_state_usage *, const char *, size_t); }; #define define_one_state_ro(_name, show) \ @@ -233,21 +234,22 @@ static ssize_t show_state_##_name(struct return sprintf(buf, "%u\n", state->_name);\ } -#define define_store_state_function(_name) \ +#define define_store_state_ull_function(_name) \ static ssize_t store_state_##_name(struct cpuidle_state *state, \ + struct cpuidle_state_usage *state_usage, \ const char *buf, size_t size) \ { \ - long value; \ + unsigned long long value; \ int err; \ if (!capable(CAP_SYS_ADMIN)) \ return -EPERM; \ - err = kstrtol(buf, 0, &value); \ + err = kstrtoull(buf, 0, &value); \ if (err) \ return err; \ if (value) \ - state->disable = 1; \ + state_usage->_name = 1; \ else \ - state->disable = 0; \ + state_usage->_name = 0; \ return size; \ } @@ -273,8 +275,8 @@ define_show_state_ull_function(usage) define_show_state_ull_function(time) define_show_state_str_function(name) define_show_state_str_function(desc) -define_show_state_function(disable) -define_store_state_function(disable) +define_show_state_ull_function(disable) +define_store_state_ull_function(disable) define_one_state_ro(name, show_state_name); define_one_state_ro(desc, show_state_desc); @@ -318,10 +320,11 @@ static ssize_t cpuidle_state_store(struc { int ret = -EIO; struct cpuidle_state *state = kobj_to_state(kobj); + struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj); struct cpuidle_state_attr *cattr = attr_to_stateattr(attr); if (cattr->store) - ret = cattr->store(state, buf, size); + ret = cattr->store(state, state_usage, buf, size); return ret; } diff -puN include/linux/cpuidle.h~cpuidle-move-field-disable-from-per-driver-to-per-cpu include/linux/cpuidle.h --- a/include/linux/cpuidle.h~cpuidle-move-field-disable-from-per-driver-to-per-cpu +++ a/include/linux/cpuidle.h @@ -34,6 +34,7 @@ struct cpuidle_driver; struct cpuidle_state_usage { void *driver_data; + unsigned long long disable; unsigned long long usage; unsigned long long time; /* in US */ }; @@ -46,7 +47,6 @@ struct cpuidle_state { unsigned int exit_latency; /* in US */ int power_usage; /* in mW */ unsigned int target_residency; /* in US */ - unsigned int disable; int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv, _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Build failures in -next due to move of disable 2012-07-02 19:53 ` Andrew Morton @ 2012-07-02 20:16 ` Rafael J. Wysocki 2012-07-02 21:37 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2012-07-02 20:16 UTC (permalink / raw) To: Andrew Morton, Mark Brown, ShuoX Liu; +Cc: linux-pm, linux-kernel On Monday, July 02, 2012, Andrew Morton wrote: > On Mon, 2 Jul 2012 21:54:35 +0200 > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > On Monday, July 02, 2012, Mark Brown wrote: > > > Today's -next is failing to build for me because commit 9f419c (PM / > > > Domains: Add preliminary support for cpuidle) and commit b32e30 > > > (cpuidle: move field disable from per-driver to per-cpu) disagree about > > > where the disable field should be: > > > > Hmm, do you know what tree commit b32e30 comes from? > > > From: ShuoX Liu <shuox.liu@intel.com> > Subject: cpuidle: move field disable from per-driver to per-cpu > > Andrew J.Schorr raises a question. When he changes the disable setting on > a single CPU, it affects all the other CPUs. Basically, currently, the > disable field is per-driver instead of per-cpu. All the C states of the > same driver are shared by all CPU in the same machine. > > The patch changes the `disable' field to per-cpu, so we could set this > separately for each cpu. > > Signed-off-by: ShuoX Liu <shuox.liu@intel.com> > Reported-by: Andrew J.Schorr <aschorr@telemetry-investments.com> > Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com> > Cc: Len Brown <lenb@kernel.org> > Cc: "Brown, Len" <len.brown@intel.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> I see, thanks. I guess I'll take this patch to linux-pm, then, and rebase my patches on top of it. Does anyone have any objections? Rafael > --- > > drivers/cpuidle/cpuidle.c | 1 - > drivers/cpuidle/governors/menu.c | 5 +++-- > drivers/cpuidle/sysfs.c | 21 ++++++++++++--------- > include/linux/cpuidle.h | 2 +- > 4 files changed, 16 insertions(+), 13 deletions(-) > > diff -puN drivers/cpuidle/cpuidle.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/cpuidle.c > --- a/drivers/cpuidle/cpuidle.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu > +++ a/drivers/cpuidle/cpuidle.c > @@ -276,7 +276,6 @@ static void poll_idle_init(struct cpuidl > state->power_usage = -1; > state->flags = 0; > state->enter = poll_idle; > - state->disable = 0; > } > #else > static void poll_idle_init(struct cpuidle_driver *drv) {} > diff -puN drivers/cpuidle/governors/menu.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/governors/menu.c > --- a/drivers/cpuidle/governors/menu.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu > +++ a/drivers/cpuidle/governors/menu.c > @@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_dr > * unless the timer is happening really really soon. > */ > if (data->expected_us > 5 && > - drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0) > + dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > > /* > @@ -290,8 +290,9 @@ static int menu_select(struct cpuidle_dr > */ > for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { > struct cpuidle_state *s = &drv->states[i]; > + struct cpuidle_state_usage *su = &dev->states_usage[i]; > > - if (s->disable) > + if (su->disable) > continue; > if (s->target_residency > data->predicted_us) > continue; > diff -puN drivers/cpuidle/sysfs.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu drivers/cpuidle/sysfs.c > --- a/drivers/cpuidle/sysfs.c~cpuidle-move-field-disable-from-per-driver-to-per-cpu > +++ a/drivers/cpuidle/sysfs.c > @@ -217,7 +217,8 @@ struct cpuidle_state_attr { > struct attribute attr; > ssize_t (*show)(struct cpuidle_state *, \ > struct cpuidle_state_usage *, char *); > - ssize_t (*store)(struct cpuidle_state *, const char *, size_t); > + ssize_t (*store)(struct cpuidle_state *, \ > + struct cpuidle_state_usage *, const char *, size_t); > }; > > #define define_one_state_ro(_name, show) \ > @@ -233,21 +234,22 @@ static ssize_t show_state_##_name(struct > return sprintf(buf, "%u\n", state->_name);\ > } > > -#define define_store_state_function(_name) \ > +#define define_store_state_ull_function(_name) \ > static ssize_t store_state_##_name(struct cpuidle_state *state, \ > + struct cpuidle_state_usage *state_usage, \ > const char *buf, size_t size) \ > { \ > - long value; \ > + unsigned long long value; \ > int err; \ > if (!capable(CAP_SYS_ADMIN)) \ > return -EPERM; \ > - err = kstrtol(buf, 0, &value); \ > + err = kstrtoull(buf, 0, &value); \ > if (err) \ > return err; \ > if (value) \ > - state->disable = 1; \ > + state_usage->_name = 1; \ > else \ > - state->disable = 0; \ > + state_usage->_name = 0; \ > return size; \ > } > > @@ -273,8 +275,8 @@ define_show_state_ull_function(usage) > define_show_state_ull_function(time) > define_show_state_str_function(name) > define_show_state_str_function(desc) > -define_show_state_function(disable) > -define_store_state_function(disable) > +define_show_state_ull_function(disable) > +define_store_state_ull_function(disable) > > define_one_state_ro(name, show_state_name); > define_one_state_ro(desc, show_state_desc); > @@ -318,10 +320,11 @@ static ssize_t cpuidle_state_store(struc > { > int ret = -EIO; > struct cpuidle_state *state = kobj_to_state(kobj); > + struct cpuidle_state_usage *state_usage = kobj_to_state_usage(kobj); > struct cpuidle_state_attr *cattr = attr_to_stateattr(attr); > > if (cattr->store) > - ret = cattr->store(state, buf, size); > + ret = cattr->store(state, state_usage, buf, size); > > return ret; > } > diff -puN include/linux/cpuidle.h~cpuidle-move-field-disable-from-per-driver-to-per-cpu include/linux/cpuidle.h > --- a/include/linux/cpuidle.h~cpuidle-move-field-disable-from-per-driver-to-per-cpu > +++ a/include/linux/cpuidle.h > @@ -34,6 +34,7 @@ struct cpuidle_driver; > struct cpuidle_state_usage { > void *driver_data; > > + unsigned long long disable; > unsigned long long usage; > unsigned long long time; /* in US */ > }; > @@ -46,7 +47,6 @@ struct cpuidle_state { > unsigned int exit_latency; /* in US */ > int power_usage; /* in mW */ > unsigned int target_residency; /* in US */ > - unsigned int disable; > > int (*enter) (struct cpuidle_device *dev, > struct cpuidle_driver *drv, > _ > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Build failures in -next due to move of disable 2012-07-02 20:16 ` Rafael J. Wysocki @ 2012-07-02 21:37 ` Rafael J. Wysocki 2012-07-03 17:13 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2012-07-02 21:37 UTC (permalink / raw) To: ShuoX Liu, linux-pm Cc: Andrew Morton, Mark Brown, linux-kernel, Kevin Hilman, Colin Cross On Monday, July 02, 2012, Rafael J. Wysocki wrote: > On Monday, July 02, 2012, Andrew Morton wrote: > > On Mon, 2 Jul 2012 21:54:35 +0200 > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > On Monday, July 02, 2012, Mark Brown wrote: > > > > Today's -next is failing to build for me because commit 9f419c (PM / > > > > Domains: Add preliminary support for cpuidle) and commit b32e30 > > > > (cpuidle: move field disable from per-driver to per-cpu) disagree about > > > > where the disable field should be: > > > > > > Hmm, do you know what tree commit b32e30 comes from? > > > > > > From: ShuoX Liu <shuox.liu@intel.com> > > Subject: cpuidle: move field disable from per-driver to per-cpu > > > > Andrew J.Schorr raises a question. When he changes the disable setting on > > a single CPU, it affects all the other CPUs. Basically, currently, the > > disable field is per-driver instead of per-cpu. All the C states of the > > same driver are shared by all CPU in the same machine. > > > > The patch changes the `disable' field to per-cpu, so we could set this > > separately for each cpu. > > > > Signed-off-by: ShuoX Liu <shuox.liu@intel.com> > > Reported-by: Andrew J.Schorr <aschorr@telemetry-investments.com> > > Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com> > > Cc: Len Brown <lenb@kernel.org> > > Cc: "Brown, Len" <len.brown@intel.com> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > I see, thanks. I guess I'll take this patch to linux-pm, then, and > rebase my patches on top of it. > > Does anyone have any objections? Well, if I do that, I pretty much will have to add the disable field to struct cpuidle_state to make commit 9f419c work again. The new patch (commit 9f419c replacement) follows, but does anyone have a better idea? Rafael --- From: Rafael J. Wysocki <rjw@sisk.pl> Subject: PM / Domains: Add preliminary support for cpuidle, v2 On some systems there are CPU cores located in the same power domains as I/O devices. Then, power can only be removed from the domain if all I/O devices in it are not in use and the CPU core is idle. Add preliminary support for that to the generic PM domains framework. First, the platform is expected to provide a cpuidle driver with one extra state designated for use with the generic PM domains code. This state should be initially disabled and its exit_latency value should be set to whatever time is needed to bring up the CPU core itself after restoring power to it, not including the domain's power on latency. Its .enter() callback should point to a procedure that will remove power from the domain containing the CPU core at the end of the CPU power transition. The remaining characteristics of the extra cpuidle state, referred to as the "domain" cpuidle state below, (e.g. power usage, target residency) should be populated in accordance with the properties of the hardware. Next, the platform should execute genpd_attach_cpuidle() on the PM domain containing the CPU core. That will cause the generic PM domains framework to treat that domain in a special way such that: * When all devices in the domain have been suspended and it is about to be turned off, the states of the devices will be saved, but power will not be removed from the domain. Instead, the "domain" cpuidle state will be enabled so that power can be removed from the domain when the CPU core is idle and the state has been chosen as the target by the cpuidle governor. * When the first I/O device in the domain is resumed and __pm_genpd_poweron(() is called for the first time after power has been removed from the domain, the "domain" cpuidle state will be disabled to avoid subsequent surprise power removals via cpuidle. The effective exit_latency value of the "domain" cpuidle state depends on the time needed to bring up the CPU core itself after restoring power to it as well as on the power on latency of the domain containing the CPU core. Thus the "domain" cpuidle state's exit_latency has to be recomputed every time the domain's power on latency is updated, which may happen every time power is restored to the domain, if the measured power on latency is greater than the latency stored in the corresponding generic_pm_domain structure. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> Reviewed-by: Kevin Hilman <khilman@ti.com> --- drivers/base/power/domain.c | 117 +++++++++++++++++++++++++++++++++++++++ drivers/cpuidle/cpuidle.c | 1 drivers/cpuidle/governors/menu.c | 3 - include/linux/cpuidle.h | 1 include/linux/pm_domain.h | 17 +++++ 5 files changed, 138 insertions(+), 1 deletion(-) Index: linux/include/linux/pm_domain.h =================================================================== --- linux.orig/include/linux/pm_domain.h +++ linux/include/linux/pm_domain.h @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/of.h> #include <linux/notifier.h> +#include <linux/cpuidle.h> enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ @@ -45,6 +46,11 @@ struct gpd_dev_ops { bool (*active_wakeup)(struct device *dev); }; +struct gpd_cpu_data { + unsigned int saved_exit_latency; + struct cpuidle_state *idle_state; +}; + struct generic_pm_domain { struct dev_pm_domain domain; /* PM domain operations */ struct list_head gpd_list_node; /* Node in the global PM domains list */ @@ -75,6 +81,7 @@ struct generic_pm_domain { bool max_off_time_changed; bool cached_power_down_ok; struct device_node *of_node; /* Node in device tree */ + struct gpd_cpu_data *cpu_data; }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) @@ -155,6 +162,8 @@ extern int pm_genpd_add_callbacks(struct struct gpd_dev_ops *ops, struct gpd_timing_data *td); extern int __pm_genpd_remove_callbacks(struct device *dev, bool clear_td); +extern int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state); +extern int genpd_detach_cpuidle(struct generic_pm_domain *genpd); extern void pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off); @@ -211,6 +220,14 @@ static inline int __pm_genpd_remove_call { return -ENOSYS; } +static inline int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int st) +{ + return -ENOSYS; +} +static inline int genpd_detach_cpuidle(struct generic_pm_domain *genpd) +{ + return -ENOSYS; +} static inline void pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off) { Index: linux/drivers/base/power/domain.c =================================================================== --- linux.orig/drivers/base/power/domain.c +++ linux/drivers/base/power/domain.c @@ -139,6 +139,19 @@ static void genpd_set_active(struct gene genpd->status = GPD_STATE_ACTIVE; } +static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd) +{ + s64 usecs64; + + if (!genpd->cpu_data) + return; + + usecs64 = genpd->power_on_latency_ns; + do_div(usecs64, NSEC_PER_USEC); + usecs64 += genpd->cpu_data->saved_exit_latency; + genpd->cpu_data->idle_state->exit_latency = usecs64; +} + /** * __pm_genpd_poweron - Restore power to a given PM domain and its masters. * @genpd: PM domain to power up. @@ -176,6 +189,13 @@ int __pm_genpd_poweron(struct generic_pm return 0; } + if (genpd->cpu_data) { + cpuidle_pause_and_lock(); + genpd->cpu_data->idle_state->disabled = true; + cpuidle_resume_and_unlock(); + goto out; + } + /* * The list is guaranteed not to change while the loop below is being * executed, unless one of the masters' .power_on() callbacks fiddles @@ -215,6 +235,7 @@ int __pm_genpd_poweron(struct generic_pm if (elapsed_ns > genpd->power_on_latency_ns) { genpd->power_on_latency_ns = elapsed_ns; genpd->max_off_time_changed = true; + genpd_recalc_cpu_exit_latency(genpd); if (genpd->name) pr_warning("%s: Power-on latency exceeded, " "new value %lld ns\n", genpd->name, @@ -222,6 +243,7 @@ int __pm_genpd_poweron(struct generic_pm } } + out: genpd_set_active(genpd); return 0; @@ -458,6 +480,21 @@ static int pm_genpd_poweroff(struct gene } } + if (genpd->cpu_data) { + /* + * If cpu_data is set, cpuidle should turn the domain off when + * the CPU in it is idle. In that case we don't decrement the + * subdomain counts of the master domains, so that power is not + * removed from the current domain prematurely as a result of + * cutting off the masters' power. + */ + genpd->status = GPD_STATE_POWER_OFF; + cpuidle_pause_and_lock(); + genpd->cpu_data->idle_state->disabled = false; + cpuidle_resume_and_unlock(); + goto out; + } + if (genpd->power_off) { ktime_t time_start; s64 elapsed_ns; @@ -1606,6 +1643,86 @@ int __pm_genpd_remove_callbacks(struct d } EXPORT_SYMBOL_GPL(__pm_genpd_remove_callbacks); +int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state) +{ + struct cpuidle_driver *cpuidle_drv; + struct gpd_cpu_data *cpu_data; + struct cpuidle_state *idle_state; + int ret = 0; + + if (IS_ERR_OR_NULL(genpd) || state < 0) + return -EINVAL; + + genpd_acquire_lock(genpd); + + if (genpd->cpu_data) { + ret = -EEXIST; + goto out; + } + cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL); + if (!cpu_data) { + ret = -ENOMEM; + goto out; + } + cpuidle_drv = cpuidle_driver_ref(); + if (!cpuidle_drv) { + ret = -ENODEV; + goto out; + } + if (cpuidle_drv->state_count <= state) { + ret = -EINVAL; + goto err; + } + idle_state = &cpuidle_drv->states[state]; + if (!idle_state->disabled) { + ret = -EAGAIN; + goto err; + } + cpu_data->idle_state = idle_state; + cpu_data->saved_exit_latency = idle_state->exit_latency; + genpd->cpu_data = cpu_data; + genpd_recalc_cpu_exit_latency(genpd); + + out: + genpd_release_lock(genpd); + return ret; + + err: + cpuidle_driver_unref(); + goto out; +} + +int genpd_detach_cpuidle(struct generic_pm_domain *genpd) +{ + struct gpd_cpu_data *cpu_data; + struct cpuidle_state *idle_state; + int ret = 0; + + if (IS_ERR_OR_NULL(genpd)) + return -EINVAL; + + genpd_acquire_lock(genpd); + + cpu_data = genpd->cpu_data; + if (!cpu_data) { + ret = -ENODEV; + goto out; + } + idle_state = cpu_data->idle_state; + if (!idle_state->disabled) { + ret = -EAGAIN; + goto out; + } + idle_state->exit_latency = cpu_data->saved_exit_latency; + cpuidle_driver_unref(); + genpd->cpu_data = NULL; + kfree(cpu_data); + + out: + genpd_release_lock(genpd); + return ret; +} + /* Default device callbacks for generic PM domains. */ /** Index: linux/drivers/cpuidle/cpuidle.c =================================================================== --- linux.orig/drivers/cpuidle/cpuidle.c +++ linux/drivers/cpuidle/cpuidle.c @@ -265,6 +265,7 @@ static void poll_idle_init(struct cpuidl state->power_usage = -1; state->flags = 0; state->enter = poll_idle; + state->disabled = false; } #else static void poll_idle_init(struct cpuidle_driver *drv) {} Index: linux/include/linux/cpuidle.h =================================================================== --- linux.orig/include/linux/cpuidle.h +++ linux/include/linux/cpuidle.h @@ -47,6 +47,7 @@ struct cpuidle_state { unsigned int exit_latency; /* in US */ int power_usage; /* in mW */ unsigned int target_residency; /* in US */ + bool disabled; /* disabled on all CPUs */ int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv, Index: linux/drivers/cpuidle/governors/menu.c =================================================================== --- linux.orig/drivers/cpuidle/governors/menu.c +++ linux/drivers/cpuidle/governors/menu.c @@ -281,6 +281,7 @@ static int menu_select(struct cpuidle_dr * unless the timer is happening really really soon. */ if (data->expected_us > 5 && + !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; @@ -292,7 +293,7 @@ static int menu_select(struct cpuidle_dr struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i]; - if (su->disable) + if (s->disabled || su->disable) continue; if (s->target_residency > data->predicted_us) continue; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Build failures in -next due to move of disable 2012-07-02 21:37 ` Rafael J. Wysocki @ 2012-07-03 17:13 ` Rafael J. Wysocki 0 siblings, 0 replies; 6+ messages in thread From: Rafael J. Wysocki @ 2012-07-03 17:13 UTC (permalink / raw) To: linux-pm Cc: ShuoX Liu, Andrew Morton, Mark Brown, linux-kernel, Kevin Hilman, Colin Cross On Monday, July 02, 2012, Rafael J. Wysocki wrote: > On Monday, July 02, 2012, Rafael J. Wysocki wrote: > > On Monday, July 02, 2012, Andrew Morton wrote: > > > On Mon, 2 Jul 2012 21:54:35 +0200 > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote: > > > > > > > On Monday, July 02, 2012, Mark Brown wrote: > > > > > Today's -next is failing to build for me because commit 9f419c (PM / > > > > > Domains: Add preliminary support for cpuidle) and commit b32e30 > > > > > (cpuidle: move field disable from per-driver to per-cpu) disagree about > > > > > where the disable field should be: > > > > > > > > Hmm, do you know what tree commit b32e30 comes from? > > > > > > > > > From: ShuoX Liu <shuox.liu@intel.com> > > > Subject: cpuidle: move field disable from per-driver to per-cpu > > > > > > Andrew J.Schorr raises a question. When he changes the disable setting on > > > a single CPU, it affects all the other CPUs. Basically, currently, the > > > disable field is per-driver instead of per-cpu. All the C states of the > > > same driver are shared by all CPU in the same machine. > > > > > > The patch changes the `disable' field to per-cpu, so we could set this > > > separately for each cpu. > > > > > > Signed-off-by: ShuoX Liu <shuox.liu@intel.com> > > > Reported-by: Andrew J.Schorr <aschorr@telemetry-investments.com> > > > Reviewed-by: Yanmin Zhang <yanmin_zhang@intel.com> > > > Cc: Len Brown <lenb@kernel.org> > > > Cc: "Brown, Len" <len.brown@intel.com> > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > > > I see, thanks. I guess I'll take this patch to linux-pm, then, and > > rebase my patches on top of it. > > > > Does anyone have any objections? > > Well, if I do that, I pretty much will have to add the disable field to > struct cpuidle_state to make commit 9f419c work again. > > The new patch (commit 9f419c replacement) follows, but does anyone have a > better idea? >From the lack of response I'm inferring that the answer is "no", so I'm going to use the patch below. Thanks, Rafael > --- > From: Rafael J. Wysocki <rjw@sisk.pl> > Subject: PM / Domains: Add preliminary support for cpuidle, v2 > > On some systems there are CPU cores located in the same power > domains as I/O devices. Then, power can only be removed from the > domain if all I/O devices in it are not in use and the CPU core > is idle. Add preliminary support for that to the generic PM domains > framework. > > First, the platform is expected to provide a cpuidle driver with one > extra state designated for use with the generic PM domains code. > This state should be initially disabled and its exit_latency value > should be set to whatever time is needed to bring up the CPU core > itself after restoring power to it, not including the domain's > power on latency. Its .enter() callback should point to a procedure > that will remove power from the domain containing the CPU core at > the end of the CPU power transition. > > The remaining characteristics of the extra cpuidle state, referred to > as the "domain" cpuidle state below, (e.g. power usage, target > residency) should be populated in accordance with the properties of > the hardware. > > Next, the platform should execute genpd_attach_cpuidle() on the PM > domain containing the CPU core. That will cause the generic PM > domains framework to treat that domain in a special way such that: > > * When all devices in the domain have been suspended and it is about > to be turned off, the states of the devices will be saved, but > power will not be removed from the domain. Instead, the "domain" > cpuidle state will be enabled so that power can be removed from > the domain when the CPU core is idle and the state has been chosen > as the target by the cpuidle governor. > > * When the first I/O device in the domain is resumed and > __pm_genpd_poweron(() is called for the first time after > power has been removed from the domain, the "domain" cpuidle > state will be disabled to avoid subsequent surprise power removals > via cpuidle. > > The effective exit_latency value of the "domain" cpuidle state > depends on the time needed to bring up the CPU core itself after > restoring power to it as well as on the power on latency of the > domain containing the CPU core. Thus the "domain" cpuidle state's > exit_latency has to be recomputed every time the domain's power on > latency is updated, which may happen every time power is restored > to the domain, if the measured power on latency is greater than > the latency stored in the corresponding generic_pm_domain structure. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > Reviewed-by: Kevin Hilman <khilman@ti.com> > --- > drivers/base/power/domain.c | 117 +++++++++++++++++++++++++++++++++++++++ > drivers/cpuidle/cpuidle.c | 1 > drivers/cpuidle/governors/menu.c | 3 - > include/linux/cpuidle.h | 1 > include/linux/pm_domain.h | 17 +++++ > 5 files changed, 138 insertions(+), 1 deletion(-) > > Index: linux/include/linux/pm_domain.h > =================================================================== > --- linux.orig/include/linux/pm_domain.h > +++ linux/include/linux/pm_domain.h > @@ -15,6 +15,7 @@ > #include <linux/err.h> > #include <linux/of.h> > #include <linux/notifier.h> > +#include <linux/cpuidle.h> > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > @@ -45,6 +46,11 @@ struct gpd_dev_ops { > bool (*active_wakeup)(struct device *dev); > }; > > +struct gpd_cpu_data { > + unsigned int saved_exit_latency; > + struct cpuidle_state *idle_state; > +}; > + > struct generic_pm_domain { > struct dev_pm_domain domain; /* PM domain operations */ > struct list_head gpd_list_node; /* Node in the global PM domains list */ > @@ -75,6 +81,7 @@ struct generic_pm_domain { > bool max_off_time_changed; > bool cached_power_down_ok; > struct device_node *of_node; /* Node in device tree */ > + struct gpd_cpu_data *cpu_data; > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) > @@ -155,6 +162,8 @@ extern int pm_genpd_add_callbacks(struct > struct gpd_dev_ops *ops, > struct gpd_timing_data *td); > extern int __pm_genpd_remove_callbacks(struct device *dev, bool clear_td); > +extern int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state); > +extern int genpd_detach_cpuidle(struct generic_pm_domain *genpd); > extern void pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off); > > @@ -211,6 +220,14 @@ static inline int __pm_genpd_remove_call > { > return -ENOSYS; > } > +static inline int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int st) > +{ > + return -ENOSYS; > +} > +static inline int genpd_detach_cpuidle(struct generic_pm_domain *genpd) > +{ > + return -ENOSYS; > +} > static inline void pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off) > { > Index: linux/drivers/base/power/domain.c > =================================================================== > --- linux.orig/drivers/base/power/domain.c > +++ linux/drivers/base/power/domain.c > @@ -139,6 +139,19 @@ static void genpd_set_active(struct gene > genpd->status = GPD_STATE_ACTIVE; > } > > +static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd) > +{ > + s64 usecs64; > + > + if (!genpd->cpu_data) > + return; > + > + usecs64 = genpd->power_on_latency_ns; > + do_div(usecs64, NSEC_PER_USEC); > + usecs64 += genpd->cpu_data->saved_exit_latency; > + genpd->cpu_data->idle_state->exit_latency = usecs64; > +} > + > /** > * __pm_genpd_poweron - Restore power to a given PM domain and its masters. > * @genpd: PM domain to power up. > @@ -176,6 +189,13 @@ int __pm_genpd_poweron(struct generic_pm > return 0; > } > > + if (genpd->cpu_data) { > + cpuidle_pause_and_lock(); > + genpd->cpu_data->idle_state->disabled = true; > + cpuidle_resume_and_unlock(); > + goto out; > + } > + > /* > * The list is guaranteed not to change while the loop below is being > * executed, unless one of the masters' .power_on() callbacks fiddles > @@ -215,6 +235,7 @@ int __pm_genpd_poweron(struct generic_pm > if (elapsed_ns > genpd->power_on_latency_ns) { > genpd->power_on_latency_ns = elapsed_ns; > genpd->max_off_time_changed = true; > + genpd_recalc_cpu_exit_latency(genpd); > if (genpd->name) > pr_warning("%s: Power-on latency exceeded, " > "new value %lld ns\n", genpd->name, > @@ -222,6 +243,7 @@ int __pm_genpd_poweron(struct generic_pm > } > } > > + out: > genpd_set_active(genpd); > > return 0; > @@ -458,6 +480,21 @@ static int pm_genpd_poweroff(struct gene > } > } > > + if (genpd->cpu_data) { > + /* > + * If cpu_data is set, cpuidle should turn the domain off when > + * the CPU in it is idle. In that case we don't decrement the > + * subdomain counts of the master domains, so that power is not > + * removed from the current domain prematurely as a result of > + * cutting off the masters' power. > + */ > + genpd->status = GPD_STATE_POWER_OFF; > + cpuidle_pause_and_lock(); > + genpd->cpu_data->idle_state->disabled = false; > + cpuidle_resume_and_unlock(); > + goto out; > + } > + > if (genpd->power_off) { > ktime_t time_start; > s64 elapsed_ns; > @@ -1606,6 +1643,86 @@ int __pm_genpd_remove_callbacks(struct d > } > EXPORT_SYMBOL_GPL(__pm_genpd_remove_callbacks); > > +int genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state) > +{ > + struct cpuidle_driver *cpuidle_drv; > + struct gpd_cpu_data *cpu_data; > + struct cpuidle_state *idle_state; > + int ret = 0; > + > + if (IS_ERR_OR_NULL(genpd) || state < 0) > + return -EINVAL; > + > + genpd_acquire_lock(genpd); > + > + if (genpd->cpu_data) { > + ret = -EEXIST; > + goto out; > + } > + cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL); > + if (!cpu_data) { > + ret = -ENOMEM; > + goto out; > + } > + cpuidle_drv = cpuidle_driver_ref(); > + if (!cpuidle_drv) { > + ret = -ENODEV; > + goto out; > + } > + if (cpuidle_drv->state_count <= state) { > + ret = -EINVAL; > + goto err; > + } > + idle_state = &cpuidle_drv->states[state]; > + if (!idle_state->disabled) { > + ret = -EAGAIN; > + goto err; > + } > + cpu_data->idle_state = idle_state; > + cpu_data->saved_exit_latency = idle_state->exit_latency; > + genpd->cpu_data = cpu_data; > + genpd_recalc_cpu_exit_latency(genpd); > + > + out: > + genpd_release_lock(genpd); > + return ret; > + > + err: > + cpuidle_driver_unref(); > + goto out; > +} > + > +int genpd_detach_cpuidle(struct generic_pm_domain *genpd) > +{ > + struct gpd_cpu_data *cpu_data; > + struct cpuidle_state *idle_state; > + int ret = 0; > + > + if (IS_ERR_OR_NULL(genpd)) > + return -EINVAL; > + > + genpd_acquire_lock(genpd); > + > + cpu_data = genpd->cpu_data; > + if (!cpu_data) { > + ret = -ENODEV; > + goto out; > + } > + idle_state = cpu_data->idle_state; > + if (!idle_state->disabled) { > + ret = -EAGAIN; > + goto out; > + } > + idle_state->exit_latency = cpu_data->saved_exit_latency; > + cpuidle_driver_unref(); > + genpd->cpu_data = NULL; > + kfree(cpu_data); > + > + out: > + genpd_release_lock(genpd); > + return ret; > +} > + > /* Default device callbacks for generic PM domains. */ > > /** > Index: linux/drivers/cpuidle/cpuidle.c > =================================================================== > --- linux.orig/drivers/cpuidle/cpuidle.c > +++ linux/drivers/cpuidle/cpuidle.c > @@ -265,6 +265,7 @@ static void poll_idle_init(struct cpuidl > state->power_usage = -1; > state->flags = 0; > state->enter = poll_idle; > + state->disabled = false; > } > #else > static void poll_idle_init(struct cpuidle_driver *drv) {} > Index: linux/include/linux/cpuidle.h > =================================================================== > --- linux.orig/include/linux/cpuidle.h > +++ linux/include/linux/cpuidle.h > @@ -47,6 +47,7 @@ struct cpuidle_state { > unsigned int exit_latency; /* in US */ > int power_usage; /* in mW */ > unsigned int target_residency; /* in US */ > + bool disabled; /* disabled on all CPUs */ > > int (*enter) (struct cpuidle_device *dev, > struct cpuidle_driver *drv, > Index: linux/drivers/cpuidle/governors/menu.c > =================================================================== > --- linux.orig/drivers/cpuidle/governors/menu.c > +++ linux/drivers/cpuidle/governors/menu.c > @@ -281,6 +281,7 @@ static int menu_select(struct cpuidle_dr > * unless the timer is happening really really soon. > */ > if (data->expected_us > 5 && > + !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && > dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > > @@ -292,7 +293,7 @@ static int menu_select(struct cpuidle_dr > struct cpuidle_state *s = &drv->states[i]; > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > - if (su->disable) > + if (s->disabled || su->disable) > continue; > if (s->target_residency > data->predicted_us) > continue; > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-03 17:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-02 12:37 Build failures in -next due to move of disable Mark Brown 2012-07-02 19:54 ` Rafael J. Wysocki 2012-07-02 19:53 ` Andrew Morton 2012-07-02 20:16 ` Rafael J. Wysocki 2012-07-02 21:37 ` Rafael J. Wysocki 2012-07-03 17:13 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®