This patchset adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch: - Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here. - Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
This was earlier sent here: https://lkml.org/lkml/2013/11/15/107
V1->V2: - Used direct callbacks from dpm_{suspend|resume}_noirq() for suspending/resuming govenors instead of doing that with help of PM notifiers. - Patch 2/2: Switching to the desirable frequency before suspending the governors.
Viresh Kumar (2): cpufreq: suspend governors on system suspend/hibernate cpufreq: Change freq before suspending governors
drivers/base/power/main.c | 3 ++ drivers/cpufreq/cpufreq.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpufreq.h | 5 +++ 3 files changed, 87 insertions(+)
This patch adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch: - Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here. - Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
Reported-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Nishanth Menon nm@ti.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/base/power/main.c | 3 +++ drivers/cpufreq/cpufreq.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpufreq.h | 3 +++ 3 files changed, 68 insertions(+)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index c12e9b9..0fbe792 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -29,6 +29,7 @@ #include <linux/async.h> #include <linux/suspend.h> #include <trace/events/power.h> +#include <linux/cpufreq.h> #include <linux/cpuidle.h> #include <linux/timer.h>
@@ -540,6 +541,7 @@ static void dpm_resume_noirq(pm_message_t state) dpm_show_time(starttime, state, "noirq"); resume_device_irqs(); cpuidle_resume(); + cpufreq_resume(); }
/** @@ -955,6 +957,7 @@ static int dpm_suspend_noirq(pm_message_t state) ktime_t starttime = ktime_get(); int error = 0;
+ cpufreq_suspend(); cpuidle_pause(); suspend_device_irqs(); mutex_lock(&dpm_list_mtx); diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..540bd87 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -26,6 +26,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/suspend.h> #include <linux/syscore_ops.h> #include <linux/tick.h> #include <trace/events/power.h> @@ -47,6 +48,9 @@ static LIST_HEAD(cpufreq_policy_list); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif
+/* Flag to suspend/resume CPUFreq governors */ +static bool cpufreq_suspended; + static inline bool has_target(void) { return cpufreq_driver->target_index || cpufreq_driver->target; @@ -1462,6 +1466,54 @@ static struct subsys_interface cpufreq_interface = { .remove_dev = cpufreq_remove_dev, };
+/* + * Callbacks for suspending/resuming governors as some platforms can't change + * frequency after this point in suspend cycle. Because some of the devices + * (like: i2c, regulators, etc) they use for changing frequency are suspended + * quickly after this point. + */ +void cpufreq_suspend(void) +{ + struct cpufreq_policy *policy; + unsigned long flags; + + if (!has_target()) + return; + + pr_debug("%s: Suspending Governors\n", __func__); + + list_for_each_entry(policy, &cpufreq_policy_list, policy_list) + if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP)) + pr_err("%s: Failed to stop governor for policy: %p\n", + __func__, policy); + + write_lock_irqsave(&cpufreq_driver_lock, flags); + cpufreq_suspended = true; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); +} + +void cpufreq_resume(void) +{ + struct cpufreq_policy *policy; + unsigned long flags; + + if (!has_target()) + return; + + pr_debug("%s: Resuming Governors\n", __func__); + + write_lock_irqsave(&cpufreq_driver_lock, flags); + cpufreq_suspended = false; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + + list_for_each_entry(policy, &cpufreq_policy_list, policy_list) + if (__cpufreq_governor(policy, CPUFREQ_GOV_START) || + __cpufreq_governor(policy, + CPUFREQ_GOV_LIMITS)) + pr_err("%s: Failed to start governor for policy: %p\n", + __func__, policy); +} + /** * cpufreq_bp_suspend - Prepare the boot CPU for system suspend. * @@ -1752,6 +1804,8 @@ EXPORT_SYMBOL_GPL(cpufreq_driver_target); static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event) { + unsigned long flags; + bool is_suspended; int ret;
/* Only must be defined when default governor is known to have latency @@ -1764,6 +1818,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, struct cpufreq_governor *gov = NULL; #endif
+ /* Don't start any governor operations if we are entering suspend */ + read_lock_irqsave(&cpufreq_driver_lock, flags); + is_suspended = cpufreq_suspended; + read_unlock_irqrestore(&cpufreq_driver_lock, flags); + + if (is_suspended) + return 0; + if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) { diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index dc196bb..6d93f91 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -255,6 +255,9 @@ struct cpufreq_driver { int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
+void cpufreq_suspend(void); +void cpufreq_resume(void); + const char *cpufreq_get_current_driver(void);
static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy,
On Friday, November 22, 2013 04:59:48 PM Viresh Kumar wrote:
This patch adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch:
- Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here.
- Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
Reported-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Nishanth Menon nm@ti.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/base/power/main.c | 3 +++ drivers/cpufreq/cpufreq.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpufreq.h | 3 +++ 3 files changed, 68 insertions(+)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index c12e9b9..0fbe792 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -29,6 +29,7 @@ #include <linux/async.h> #include <linux/suspend.h> #include <trace/events/power.h> +#include <linux/cpufreq.h> #include <linux/cpuidle.h> #include <linux/timer.h> @@ -540,6 +541,7 @@ static void dpm_resume_noirq(pm_message_t state) dpm_show_time(starttime, state, "noirq"); resume_device_irqs(); cpuidle_resume();
- cpufreq_resume();
} /** @@ -955,6 +957,7 @@ static int dpm_suspend_noirq(pm_message_t state) ktime_t starttime = ktime_get(); int error = 0;
- cpufreq_suspend(); cpuidle_pause(); suspend_device_irqs(); mutex_lock(&dpm_list_mtx);
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 02d534d..540bd87 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -26,6 +26,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/suspend.h> #include <linux/syscore_ops.h> #include <linux/tick.h> #include <trace/events/power.h> @@ -47,6 +48,9 @@ static LIST_HEAD(cpufreq_policy_list); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif +/* Flag to suspend/resume CPUFreq governors */ +static bool cpufreq_suspended;
static inline bool has_target(void) { return cpufreq_driver->target_index || cpufreq_driver->target; @@ -1462,6 +1466,54 @@ static struct subsys_interface cpufreq_interface = { .remove_dev = cpufreq_remove_dev, }; +/*
- Callbacks for suspending/resuming governors as some platforms can't change
- frequency after this point in suspend cycle. Because some of the devices
- (like: i2c, regulators, etc) they use for changing frequency are suspended
- quickly after this point.
- */
+void cpufreq_suspend(void) +{
- struct cpufreq_policy *policy;
- unsigned long flags;
- if (!has_target())
return;- pr_debug("%s: Suspending Governors\n", __func__);
- list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))pr_err("%s: Failed to stop governor for policy: %p\n",__func__, policy);- write_lock_irqsave(&cpufreq_driver_lock, flags);
- cpufreq_suspended = true;
- write_unlock_irqrestore(&cpufreq_driver_lock, flags);
The locking here is pointless. It doesn't prevent any race conditions from happening.
+}
+void cpufreq_resume(void) +{
- struct cpufreq_policy *policy;
- unsigned long flags;
- if (!has_target())
return;- pr_debug("%s: Resuming Governors\n", __func__);
- write_lock_irqsave(&cpufreq_driver_lock, flags);
- cpufreq_suspended = false;
- write_unlock_irqrestore(&cpufreq_driver_lock, flags);
Same here.
- list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
if (__cpufreq_governor(policy, CPUFREQ_GOV_START) ||__cpufreq_governor(policy,CPUFREQ_GOV_LIMITS))pr_err("%s: Failed to start governor for policy: %p\n",__func__, policy);+}
/**
- cpufreq_bp_suspend - Prepare the boot CPU for system suspend.
@@ -1752,6 +1804,8 @@ EXPORT_SYMBOL_GPL(cpufreq_driver_target); static int __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event) {
- unsigned long flags;
- bool is_suspended; int ret;
/* Only must be defined when default governor is known to have latency @@ -1764,6 +1818,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, struct cpufreq_governor *gov = NULL; #endif
- /* Don't start any governor operations if we are entering suspend */
- read_lock_irqsave(&cpufreq_driver_lock, flags);
- is_suspended = cpufreq_suspended;
- read_unlock_irqrestore(&cpufreq_driver_lock, flags);
And same here.
- if (is_suspended)
And you can check cpufreq_suspended directly here.
return 0;- if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index dc196bb..6d93f91 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -255,6 +255,9 @@ struct cpufreq_driver { int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); +void cpufreq_suspend(void); +void cpufreq_resume(void);
const char *cpufreq_get_current_driver(void); static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy,
Thanks!
On 22 November 2013 18:03, Rafael J. Wysocki rjw@rjwysocki.net wrote:
The locking here is pointless. It doesn't prevent any race conditions from happening.
All comments accepted..
On 11/22/2013 05:29 AM, Viresh Kumar wrote:
This patch adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch:
- Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here.
- Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
Reported-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Nishanth Menon nm@ti.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/base/power/main.c | 3 +++ drivers/cpufreq/cpufreq.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/cpufreq.h | 3 +++ 3 files changed, 68 insertions(+)
yes, this seems to work for me as well. http://pastebin.mozilla.org/3670909 - no cpufreq attempts to transition were triggered.
On 26 November 2013 03:00, Nishanth Menon nm@ti.com wrote:
yes, this seems to work for me as well. http://pastebin.mozilla.org/3670909 - no cpufreq attempts to transition were triggered.
Ahh.. Thanks for confirming.. But we still can't work with noirq handlers as there are platforms which want to change frequency before going into suspend and so we need to do this from dpm_suspend() :)
Thanks for trying.
-- viresh
Hi!
This patch adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch:
- Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here.
- Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
+/*
- Callbacks for suspending/resuming governors as some platforms can't change
- frequency after this point in suspend cycle. Because some of the devices
- (like: i2c, regulators, etc) they use for changing frequency are suspended
- quickly after this point.
- */
+void cpufreq_suspend(void) +{
- struct cpufreq_policy *policy;
- unsigned long flags;
- if (!has_target())
return;- pr_debug("%s: Suspending Governors\n", __func__);
- list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))pr_err("%s: Failed to stop governor for policy: %p\n",__func__, policy);
So... we freeze frequencies in whatever state they are, yes?
Should we go to some specific frequency?
For example, early athlon64 notebooks were unable to run on battery power on max frequency... these days there are various "Turbo" modes, but IIRC staying at high frequency there is only safe as long as CPU stays cool enough... Pavel
On Tuesday, November 26, 2013 08:39:02 PM Pavel Machek wrote:
Hi!
This patch adds cpufreq callbacks to dpm_{suspend|resume}_noirq() for handling suspend/resume of cpufreq governors. This is required for early suspend and late resume of governors.
There are multiple problems that are fixed by this patch:
- Nishanth Menon (TI) found an interesting problem on his platform, OMAP. His board wasn't working well with suspend/resume as calls for removing non-boot CPUs was turning out into a call to drivers ->target() which then tries to play with regulators. But regulators and their I2C bus were already suspended and this resulted in a failure. This is why we need a PM notifier here.
- Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found another issue where tunables configuration for clusters/sockets with non-boot CPUs was getting lost after suspend/resume, as we were notifying governors with CPUFREQ_GOV_POLICY_EXIT on removal of the last cpu for that policy and so deallocating memory for tunables.
+/*
- Callbacks for suspending/resuming governors as some platforms can't change
- frequency after this point in suspend cycle. Because some of the devices
- (like: i2c, regulators, etc) they use for changing frequency are suspended
- quickly after this point.
- */
+void cpufreq_suspend(void) +{
- struct cpufreq_policy *policy;
- unsigned long flags;
- if (!has_target())
return;- pr_debug("%s: Suspending Governors\n", __func__);
- list_for_each_entry(policy, &cpufreq_policy_list, policy_list)
if (__cpufreq_governor(policy, CPUFREQ_GOV_STOP))pr_err("%s: Failed to stop governor for policy: %p\n",__func__, policy);So... we freeze frequencies in whatever state they are, yes?
Yes. The idea was to do that after suspending devices in which case it wouldn't matter so much. But Viresh always has to complicate things.
Should we go to some specific frequency?
If that is done where it is done, yes, we should.
Thanks, Rafael
On 27 November 2013 01:48, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Tuesday, November 26, 2013 08:39:02 PM Pavel Machek wrote:
So... we freeze frequencies in whatever state they are, yes?
Better go through the V3 of this patchset:
https://lkml.org/lkml/2013/11/25/838
We are giving drivers and opportunity to set core to whatever frequency they want before suspending.
Yes. The idea was to do that after suspending devices in which case it wouldn't matter so much. But Viresh always has to complicate things.
:)
Its complicated by the kind of designs we have for our hardware. We tried the noirq callbacks and it worked atleast for Nishanth, who reported the problem initially. But the problem started when drivers wanted to change their frequencies before suspending and that can't happen in noirq place..
I had another idea but then left it thinking that it might be even more complicated :)
What about both dpm_suspend_noirq and dpm_suspend callbacks. Drivers will change freq in dpm_suspend_noirq and dpm_suspend will stop governors?
But the question is can governors try another frequency at that time? i.e. override whatever is configured by drivers?
Should we go to some specific frequency?
If that is done where it is done, yes, we should.
You meant dpm_suspend() here, right?
On Wednesday, November 27, 2013 08:26:00 AM Viresh Kumar wrote:
On 27 November 2013 01:48, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Tuesday, November 26, 2013 08:39:02 PM Pavel Machek wrote:
So... we freeze frequencies in whatever state they are, yes?
Better go through the V3 of this patchset:
https://lkml.org/lkml/2013/11/25/838
We are giving drivers and opportunity to set core to whatever frequency they want before suspending.
Yes. The idea was to do that after suspending devices in which case it wouldn't matter so much. But Viresh always has to complicate things.
:)
Its complicated by the kind of designs we have for our hardware. We tried the noirq callbacks and it worked atleast for Nishanth, who reported the problem initially. But the problem started when drivers wanted to change their frequencies before suspending and that can't happen in noirq place..
This way you end up with a fix that may introduce other issues. Which is kind of fine before a merge window, but not so much after one. So at this point it's better to fix things that may be fixed without introducing those other issues *first* and then go for a more intrusive change that will cover more cases.
I had another idea but then left it thinking that it might be even more complicated :)
What about both dpm_suspend_noirq and dpm_suspend callbacks. Drivers will change freq in dpm_suspend_noirq and dpm_suspend will stop governors?
But the question is can governors try another frequency at that time? i.e. override whatever is configured by drivers?
Should we go to some specific frequency?
If that is done where it is done, yes, we should.
You meant dpm_suspend() here, right?
Yes.
Thanks!
Some platforms might want to change frequency before suspending governors. Like: - Some platform which want to set freq to max to speed up suspend/hibernation process. - Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 17 +++++++++++++++++ include/linux/cpufreq.h | 2 ++ 2 files changed, 19 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 540bd87..e609102 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1476,6 +1476,7 @@ void cpufreq_suspend(void) { struct cpufreq_policy *policy; unsigned long flags; + int ret;
if (!has_target()) return; @@ -1487,6 +1488,22 @@ void cpufreq_suspend(void) pr_err("%s: Failed to stop governor for policy: %p\n", __func__, policy);
+ /* + * In case platform wants some specific frequency to be configured + * during suspend + */ + if (policy->suspend_freq) { + pr_debug("%s: Suspending Governors: Setting suspend-freq: %u\n", + __func__, policy->suspend_freq); + + ret = __cpufreq_driver_target(policy, policy->suspend_freq, + CPUFREQ_RELATION_H); + /* We can still suspend even if this failed */ + if (ret) + pr_err("%s: Unable to set suspend-freq: %u. Err: %d\n", + __func__, policy->suspend_freq, ret); + } + write_lock_irqsave(&cpufreq_driver_lock, flags); cpufreq_suspended = true; write_unlock_irqrestore(&cpufreq_driver_lock, flags); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 6d93f91..867fdd4 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -72,6 +72,8 @@ struct cpufreq_policy { unsigned int max; /* in kHz */ unsigned int cur; /* in kHz, only needed if cpufreq * governors are used */ + unsigned int suspend_freq; /* freq to set during suspend */ + unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data;
On Friday, November 22, 2013 04:59:49 PM Viresh Kumar wrote:
Some platforms might want to change frequency before suspending governors. Like:
- Some platform which want to set freq to max to speed up suspend/hibernation process.
- Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't think this is generally necessary, because the suspend/resume routines added by patch [1/2] will be executed very late during suspend or very early during resume and it shouldn't really matter what performance levels the CPUs are at then.
The only exception *may* be hibernation, because the amount of time needed to create the image will depend on the current performance level of the boot CPU, but that should be an explicitly special case in my opinion.
Thanks!
On 22 November 2013 18:07, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Friday, November 22, 2013 04:59:49 PM Viresh Kumar wrote:
Some platforms might want to change frequency before suspending governors. Like:
- Some platform which want to set freq to max to speed up suspend/hibernation process.
- Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't think this is generally necessary, because the suspend/resume routines added by patch [1/2] will be executed very late during suspend or very early during resume and it shouldn't really matter what performance levels the CPUs are at then.
There are few things here: - I feel that the current place from where we have suspended stuff is not gonna fly. We are doing that in noirq and probably devices which might be required during frequency transitions might already be down.. So we *may* need to move that in dpm_suspend().. - Secondly I want to understand why Tegra/Exynos has such code which I mentioned above..
@Stephen, Kukjin and other samsung folks: Please provide some input here, before your systems break in mainline :)
The only exception *may* be hibernation, because the amount of time needed to create the image will depend on the current performance level of the boot CPU, but that should be an explicitly special case in my opinion.
Hmm.. That looks sensible if there is nothing special required for Tegra/Exynos.
On Friday, November 22, 2013 06:22:52 PM Viresh Kumar wrote:
On 22 November 2013 18:07, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Friday, November 22, 2013 04:59:49 PM Viresh Kumar wrote:
Some platforms might want to change frequency before suspending governors. Like:
- Some platform which want to set freq to max to speed up suspend/hibernation process.
- Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't think this is generally necessary, because the suspend/resume routines added by patch [1/2] will be executed very late during suspend or very early during resume and it shouldn't really matter what performance levels the CPUs are at then.
There are few things here:
- I feel that the current place from where we have suspended stuff is not gonna
fly. We are doing that in noirq and probably devices which might be required during frequency transitions might already be down.. So we *may* need to move that in dpm_suspend()..
That would be a much more intrusive change. Definitely not 3.13 material at this point.
Thanks!
On 22 November 2013 18:55, Rafael J. Wysocki rjw@rjwysocki.net wrote:
That would be a much more intrusive change. Definitely not 3.13 material at this point.
Agreed..
On 11/22/2013 05:52 AM, Viresh Kumar wrote:
On 22 November 2013 18:07, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Friday, November 22, 2013 04:59:49 PM Viresh Kumar wrote:
Some platforms might want to change frequency before suspending governors. Like:
- Some platform which want to set freq to max to speed up suspend/hibernation process.
- Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't think this is generally necessary, because the suspend/resume routines added by patch [1/2] will be executed very late during suspend or very early during resume and it shouldn't really matter what performance levels the CPUs are at then.
There are few things here:
- I feel that the current place from where we have suspended stuff is not gonna
fly. We are doing that in noirq and probably devices which might be required during frequency transitions might already be down.. So we *may* need to move that in dpm_suspend()..
- Secondly I want to understand why Tegra/Exynos has such code which I
mentioned above..
@Stephen, Kukjin and other samsung folks: Please provide some input here, before your systems break in mainline :)
I believe we set the clock to a low value because fast clocks consume more power. Tegra architecturally supports a number of different suspend levels. Only some of those actually power off or gate the clock source itself.
On Friday, November 22, 2013 12:39:24 PM Stephen Warren wrote:
On 11/22/2013 05:52 AM, Viresh Kumar wrote:
On 22 November 2013 18:07, Rafael J. Wysocki rjw@rjwysocki.net wrote:
On Friday, November 22, 2013 04:59:49 PM Viresh Kumar wrote:
Some platforms might want to change frequency before suspending governors. Like:
- Some platform which want to set freq to max to speed up suspend/hibernation process.
- Some platform (like: Tegra or exynos), set this to min or bootloader's frequency.
This patch adds an option for those, so that they can specify this at call to ->init(), so that cpufreq core can take care of this before suspending system.
If this variable is not updated by ->init() then its value would be zero and so core wouldn't do anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't think this is generally necessary, because the suspend/resume routines added by patch [1/2] will be executed very late during suspend or very early during resume and it shouldn't really matter what performance levels the CPUs are at then.
There are few things here:
- I feel that the current place from where we have suspended stuff is not gonna
fly. We are doing that in noirq and probably devices which might be required during frequency transitions might already be down.. So we *may* need to move that in dpm_suspend()..
- Secondly I want to understand why Tegra/Exynos has such code which I
mentioned above..
@Stephen, Kukjin and other samsung folks: Please provide some input here, before your systems break in mainline :)
I believe we set the clock to a low value because fast clocks consume more power. Tegra architecturally supports a number of different suspend levels. Only some of those actually power off or gate the clock source itself.
Hmm.
Viresh, maybe make it possible for the cpufreq driver to provide suspend/resume callbacks to be executed by cpufreq_suspend() and cpufreq_resume() introduced by [1/2]? Then Tegra could set the frequencies to what it wants from there before the governors are stopped.
Thanks!
On 25 November 2013 03:02, Rafael J. Wysocki rjw@rjwysocki.net wrote:
Viresh, maybe make it possible for the cpufreq driver to provide suspend/resume callbacks to be executed by cpufreq_suspend() and cpufreq_resume() introduced by [1/2]? Then Tegra could set the frequencies to what it wants from there before the governors are stopped.
Giving cpufreq-drivers a chance to do whatever they want looks to be correct. So, maybe prepare() or suspend_prepare() for them can be implemented.
Though I would still go for a generic function in core, which can be just reused by samsung and tegra to set cores to specific frequencies.
-- viresh
On 25 November 2013 09:58, Viresh Kumar viresh.kumar@linaro.org wrote:
Giving cpufreq-drivers a chance to do whatever they want looks to be correct. So, maybe prepare() or suspend_prepare() for them can be implemented.
I have used the existing infrastructure here, i.e. suspend/resume callbacks for drivers.. As we don't need two suspend calls now..
linaro-kernel@lists.linaro.org
-
Nishanth Menon -
Pavel Machek -
Rafael J. Wysocki -
Stephen Warren -
Viresh Kumar