This patch adds cpufreq suspend/resume calls to dpm_{suspend|resume}() for handling suspend/resume of cpufreq governors.
Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found an 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 is fixed by this patch as we don't allow any operation on governors after device suspend and before device resume now.
We could have added these callbacks at dpm_{suspend|resume}_noirq() level but the problem here is that most of the devices (i.e. devices with ->suspend() callbacks) have already been suspended by now and so if drivers want to change frequency before suspending, then it might not be possible for many platforms (which depend on other peripherals like i2c, regulators, etc).
Reported-and-tested-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- V5->V6: 1-2-3/7 merged into 1/5
drivers/base/power/main.c | 5 +++ drivers/cpufreq/cpufreq.c | 111 +++++++++++++++++++++++----------------------- include/linux/cpufreq.h | 8 ++++ 3 files changed, 69 insertions(+), 55 deletions(-)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 42355e4..86d5e4f 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>
@@ -866,6 +867,8 @@ void dpm_resume(pm_message_t state) mutex_unlock(&dpm_list_mtx); async_synchronize_full(); dpm_show_time(starttime, state, NULL); + + cpufreq_resume(); }
/** @@ -1434,6 +1437,8 @@ int dpm_suspend(pm_message_t state)
might_sleep();
+ cpufreq_suspend(); + mutex_lock(&dpm_list_mtx); pm_transition = state; async_error = 0; diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 56b7b1b..2e43c08 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -26,7 +26,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> -#include <linux/syscore_ops.h> +#include <linux/suspend.h> #include <linux/tick.h> #include <trace/events/power.h>
@@ -47,6 +47,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; @@ -1576,82 +1579,77 @@ static struct subsys_interface cpufreq_interface = { };
/** - * cpufreq_bp_suspend - Prepare the boot CPU for system suspend. + * cpufreq_suspend() - Suspend CPUFreq governors * - * This function is only executed for the boot processor. The other CPUs - * have been put offline by means of CPU hotplug. + * Called during system wide Suspend/Hibernate cycles for suspending 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. */ -static int cpufreq_bp_suspend(void) +void cpufreq_suspend(void) { - int ret = 0; - - int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("suspending cpu %u\n", cpu); + if (!cpufreq_driver) + return;
- /* If there's no policy for the boot CPU, we have nothing to do. */ - policy = cpufreq_cpu_get(cpu); - if (!policy) - return 0; + if (!has_target()) + return;
- if (cpufreq_driver->suspend) { - ret = cpufreq_driver->suspend(policy); - if (ret) - printk(KERN_ERR "cpufreq: suspend failed in ->suspend " - "step on CPU %u\n", policy->cpu); + 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); + else if (cpufreq_driver->suspend + && cpufreq_driver->suspend(policy)) + pr_err("%s: Failed to suspend driver: %p\n", __func__, + policy); }
- cpufreq_cpu_put(policy); - return ret; + cpufreq_suspended = true; }
/** - * cpufreq_bp_resume - Restore proper frequency handling of the boot CPU. + * cpufreq_resume() - Resume CPUFreq governors * - * 1.) resume CPUfreq hardware support (cpufreq_driver->resume()) - * 2.) schedule call cpufreq_update_policy() ASAP as interrupts are - * restored. It will verify that the current freq is in sync with - * what we believe it to be. This is a bit later than when it - * should be, but nonethteless it's better than calling - * cpufreq_driver->get() here which might re-enable interrupts... - * - * This function is only executed for the boot CPU. The other CPUs have not - * been turned on yet. + * Called during system wide Suspend/Hibernate cycle for resuming governors that + * are suspended with cpufreq_suspend(). */ -static void cpufreq_bp_resume(void) +void cpufreq_resume(void) { - int ret = 0; - - int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("resuming cpu %u\n", cpu); + if (!cpufreq_driver) + return;
- /* If there's no policy for the boot CPU, we have nothing to do. */ - policy = cpufreq_cpu_get(cpu); - if (!policy) + if (!has_target()) return;
- if (cpufreq_driver->resume) { - ret = cpufreq_driver->resume(policy); - if (ret) { - printk(KERN_ERR "cpufreq: resume failed in ->resume " - "step on CPU %u\n", policy->cpu); - goto fail; - } - } + pr_debug("%s: Resuming Governors\n", __func__);
- schedule_work(&policy->update); + cpufreq_suspended = false;
-fail: - cpufreq_cpu_put(policy); -} + 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); + else if (cpufreq_driver->resume + && cpufreq_driver->resume(policy)) + pr_err("%s: Failed to resume driver: %p\n", __func__, + policy);
-static struct syscore_ops cpufreq_syscore_ops = { - .suspend = cpufreq_bp_suspend, - .resume = cpufreq_bp_resume, -}; + /* + * schedule call cpufreq_update_policy() for boot CPU, i.e. last + * policy in list. It will verify that the current freq is in + * sync with what we believe it to be. + */ + if (list_is_last(&policy->policy_list, &cpufreq_policy_list)) + schedule_work(&policy->update); + } +}
/** * cpufreq_get_current_driver - return current driver's name @@ -1868,6 +1866,10 @@ 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 */ + if (cpufreq_suspended) + return 0; + if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) { @@ -2407,7 +2409,6 @@ static int __init cpufreq_core_init(void)
cpufreq_global_kobject = kobject_create(); BUG_ON(!cpufreq_global_kobject); - register_syscore_ops(&cpufreq_syscore_ops);
return 0; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4d89e0e..94ed907 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -296,6 +296,14 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy) policy->cpuinfo.max_freq); }
+#ifdef CONFIG_CPU_FREQ +void cpufreq_suspend(void); +void cpufreq_resume(void); +#else +static inline void cpufreq_suspend(void) {} +static inline void cpufreq_resume(void) {} +#endif + /********************************************************************* * CPUFREQ NOTIFIER INTERFACE * *********************************************************************/
Multiple platforms need to set CPU to a particular frequency before suspending system. And so they need a common infrastructure which is provided by this patch. Those platforms just need to initialize their ->suspend() pointers with the generic routine.
Tested-by: Stephen Warren swarren@nvidia.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++++++++++++ include/linux/cpufreq.h | 3 +++ 2 files changed, 29 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2e43c08..b6f8545 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1578,6 +1578,32 @@ static struct subsys_interface cpufreq_interface = { .remove_dev = cpufreq_remove_dev, };
+/* + * In case platform wants some specific frequency to be configured + * during suspend.. + */ +int cpufreq_generic_suspend(struct cpufreq_policy *policy) +{ + int ret; + + if (!policy->suspend_freq) { + pr_err("%s: suspend_freq can't be zero\n", __func__); + return -EINVAL; + } + + pr_debug("%s: Setting suspend-freq: %u\n", __func__, + policy->suspend_freq); + + ret = __cpufreq_driver_target(policy, policy->suspend_freq, + CPUFREQ_RELATION_H); + if (ret) + pr_err("%s: unable to set suspend-freq: %u. err: %d\n", + __func__, policy->suspend_freq, ret); + + return ret; +} +EXPORT_SYMBOL(cpufreq_generic_suspend); + /** * cpufreq_suspend() - Suspend CPUFreq governors * diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 94ed907..325bab0 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -74,6 +74,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; @@ -299,6 +301,7 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy) #ifdef CONFIG_CPU_FREQ void cpufreq_suspend(void); void cpufreq_resume(void); +int cpufreq_generic_suspend(struct cpufreq_policy *policy); #else static inline void cpufreq_suspend(void) {} static inline void cpufreq_resume(void) {}
Currently we have implemented PM notifiers to disable/enable ->target() routines functionality during suspend/resume.
Now we have support present in cpufreq core, lets use it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/exynos-cpufreq.c | 96 +++------------------------------------- 1 file changed, 7 insertions(+), 89 deletions(-)
diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c index fcd2914..307f02e 100644 --- a/drivers/cpufreq/exynos-cpufreq.c +++ b/drivers/cpufreq/exynos-cpufreq.c @@ -16,7 +16,6 @@ #include <linux/slab.h> #include <linux/regulator/consumer.h> #include <linux/cpufreq.h> -#include <linux/suspend.h> #include <linux/platform_device.h>
#include <plat/cpu.h> @@ -24,12 +23,8 @@ #include "exynos-cpufreq.h"
static struct exynos_dvfs_info *exynos_info; - static struct regulator *arm_regulator; - static unsigned int locking_frequency; -static bool frequency_locked; -static DEFINE_MUTEX(cpufreq_lock);
static int exynos_cpufreq_get_index(unsigned int freq) { @@ -134,83 +129,13 @@ out:
static int exynos_target(struct cpufreq_policy *policy, unsigned int index) { - struct cpufreq_frequency_table *freq_table = exynos_info->freq_table; - int ret = 0; - - mutex_lock(&cpufreq_lock); - - if (frequency_locked) - goto out; - - ret = exynos_cpufreq_scale(freq_table[index].frequency); - -out: - mutex_unlock(&cpufreq_lock); - - return ret; -} - -#ifdef CONFIG_PM -static int exynos_cpufreq_suspend(struct cpufreq_policy *policy) -{ - return 0; -} - -static int exynos_cpufreq_resume(struct cpufreq_policy *policy) -{ - return 0; -} -#endif - -/** - * exynos_cpufreq_pm_notifier - block CPUFREQ's activities in suspend-resume - * context - * @notifier - * @pm_event - * @v - * - * While frequency_locked == true, target() ignores every frequency but - * locking_frequency. The locking_frequency value is the initial frequency, - * which is set by the bootloader. In order to eliminate possible - * inconsistency in clock values, we save and restore frequencies during - * suspend and resume and block CPUFREQ activities. Note that the standard - * suspend/resume cannot be used as they are too deep (syscore_ops) for - * regulator actions. - */ -static int exynos_cpufreq_pm_notifier(struct notifier_block *notifier, - unsigned long pm_event, void *v) -{ - int ret; - - switch (pm_event) { - case PM_SUSPEND_PREPARE: - mutex_lock(&cpufreq_lock); - frequency_locked = true; - mutex_unlock(&cpufreq_lock); - - ret = exynos_cpufreq_scale(locking_frequency); - if (ret < 0) - return NOTIFY_BAD; - - break; - - case PM_POST_SUSPEND: - mutex_lock(&cpufreq_lock); - frequency_locked = false; - mutex_unlock(&cpufreq_lock); - break; - } - - return NOTIFY_OK; + return exynos_cpufreq_scale(exynos_info->freq_table[index].frequency); }
-static struct notifier_block exynos_cpufreq_nb = { - .notifier_call = exynos_cpufreq_pm_notifier, -}; - static int exynos_cpufreq_cpu_init(struct cpufreq_policy *policy) { policy->clk = exynos_info->cpu_clk; + policy->suspend_freq = locking_frequency; return cpufreq_generic_init(policy, exynos_info->freq_table, 100000); }
@@ -227,8 +152,7 @@ static struct cpufreq_driver exynos_driver = { .boost_supported = true, #endif #ifdef CONFIG_PM - .suspend = exynos_cpufreq_suspend, - .resume = exynos_cpufreq_resume, + .suspend = cpufreq_generic_suspend, #endif };
@@ -263,19 +187,13 @@ static int exynos_cpufreq_probe(struct platform_device *pdev) goto err_vdd_arm; }
+ /* Done here as we want to capture boot frequency */ locking_frequency = clk_get_rate(exynos_info->cpu_clk) / 1000;
- register_pm_notifier(&exynos_cpufreq_nb); - - if (cpufreq_register_driver(&exynos_driver)) { - pr_err("%s: failed to register cpufreq driver\n", __func__); - goto err_cpufreq; - } - - return 0; -err_cpufreq: - unregister_pm_notifier(&exynos_cpufreq_nb); + if (!cpufreq_register_driver(&exynos_driver)) + return 0;
+ pr_err("%s: failed to register cpufreq driver\n", __func__); regulator_put(arm_regulator); err_vdd_arm: kfree(exynos_info);
Currently we have implemented PM notifiers to disable/enable ->target() routines functionality during suspend/resume.
Now we have support present in cpufreq core, lets use it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/s5pv210-cpufreq.c | 49 +++------------------------------------ 1 file changed, 3 insertions(+), 46 deletions(-)
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c index 55a8e9f..7242153 100644 --- a/drivers/cpufreq/s5pv210-cpufreq.c +++ b/drivers/cpufreq/s5pv210-cpufreq.c @@ -18,7 +18,6 @@ #include <linux/cpufreq.h> #include <linux/reboot.h> #include <linux/regulator/consumer.h> -#include <linux/suspend.h>
#include <mach/map.h> #include <mach/regs-clock.h> @@ -435,18 +434,6 @@ exit: return ret; }
-#ifdef CONFIG_PM -static int s5pv210_cpufreq_suspend(struct cpufreq_policy *policy) -{ - return 0; -} - -static int s5pv210_cpufreq_resume(struct cpufreq_policy *policy) -{ - return 0; -} -#endif - static int check_mem_type(void __iomem *dmc_reg) { unsigned long val; @@ -502,6 +489,7 @@ static int __init s5pv210_cpu_init(struct cpufreq_policy *policy) s5pv210_dram_conf[1].refresh = (__raw_readl(S5P_VA_DMC1 + 0x30) * 1000); s5pv210_dram_conf[1].freq = clk_get_rate(dmc1_clk);
+ policy->suspend_freq = SLEEP_FREQ; return cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
out_dmc1: @@ -511,32 +499,6 @@ out_dmc0: return ret; }
-static int s5pv210_cpufreq_notifier_event(struct notifier_block *this, - unsigned long event, void *ptr) -{ - int ret; - - switch (event) { - case PM_SUSPEND_PREPARE: - ret = cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0); - if (ret < 0) - return NOTIFY_BAD; - - /* Disable updation of cpu frequency */ - no_cpufreq_access = true; - return NOTIFY_OK; - case PM_POST_RESTORE: - case PM_POST_SUSPEND: - /* Enable updation of cpu frequency */ - no_cpufreq_access = false; - cpufreq_driver_target(cpufreq_cpu_get(0), SLEEP_FREQ, 0); - - return NOTIFY_OK; - } - - return NOTIFY_DONE; -} - static int s5pv210_cpufreq_reboot_notifier_event(struct notifier_block *this, unsigned long event, void *ptr) { @@ -558,15 +520,11 @@ static struct cpufreq_driver s5pv210_driver = { .init = s5pv210_cpu_init, .name = "s5pv210", #ifdef CONFIG_PM - .suspend = s5pv210_cpufreq_suspend, - .resume = s5pv210_cpufreq_resume, + .suspend = cpufreq_generic_suspend, + .resume = cpufreq_generic_suspend, /* We need to set SLEEP FREQ again */ #endif };
-static struct notifier_block s5pv210_cpufreq_notifier = { - .notifier_call = s5pv210_cpufreq_notifier_event, -}; - static struct notifier_block s5pv210_cpufreq_reboot_notifier = { .notifier_call = s5pv210_cpufreq_reboot_notifier_event, }; @@ -586,7 +544,6 @@ static int __init s5pv210_cpufreq_init(void) return PTR_ERR(int_regulator); }
- register_pm_notifier(&s5pv210_cpufreq_notifier); register_reboot_notifier(&s5pv210_cpufreq_reboot_notifier);
return cpufreq_register_driver(&s5pv210_driver);
Currently we have implemented PM notifiers to disable/enable ->target() routines functionality during suspend/resume.
Now we have support present in cpufreq core, lets use it.
Tested-by: Stephen Warren swarren@nvidia.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/tegra-cpufreq.c | 46 +++++------------------------------------ 1 file changed, 5 insertions(+), 41 deletions(-)
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index e652c1b..bcfed77 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -26,7 +26,6 @@ #include <linux/err.h> #include <linux/clk.h> #include <linux/io.h> -#include <linux/suspend.h>
static struct cpufreq_frequency_table freq_table[] = { { .frequency = 216000 }, @@ -47,9 +46,6 @@ static struct clk *pll_x_clk; static struct clk *pll_p_clk; static struct clk *emc_clk;
-static DEFINE_MUTEX(tegra_cpu_lock); -static bool is_suspended; - static int tegra_cpu_clk_set_rate(unsigned long rate) { int ret; @@ -112,42 +108,9 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy,
static int tegra_target(struct cpufreq_policy *policy, unsigned int index) { - int ret = -EBUSY; - - mutex_lock(&tegra_cpu_lock); - - if (!is_suspended) - ret = tegra_update_cpu_speed(policy, - freq_table[index].frequency); - - mutex_unlock(&tegra_cpu_lock); - return ret; + return tegra_update_cpu_speed(policy, freq_table[index].frequency); }
-static int tegra_pm_notify(struct notifier_block *nb, unsigned long event, - void *dummy) -{ - mutex_lock(&tegra_cpu_lock); - if (event == PM_SUSPEND_PREPARE) { - struct cpufreq_policy *policy = cpufreq_cpu_get(0); - is_suspended = true; - pr_info("Tegra cpufreq suspend: setting frequency to %d kHz\n", - freq_table[0].frequency); - if (clk_get_rate(cpu_clk) / 1000 != freq_table[0].frequency) - tegra_update_cpu_speed(policy, freq_table[0].frequency); - cpufreq_cpu_put(policy); - } else if (event == PM_POST_SUSPEND) { - is_suspended = false; - } - mutex_unlock(&tegra_cpu_lock); - - return NOTIFY_OK; -} - -static struct notifier_block tegra_cpu_pm_notifier = { - .notifier_call = tegra_pm_notify, -}; - static int tegra_cpu_init(struct cpufreq_policy *policy) { int ret; @@ -166,10 +129,8 @@ static int tegra_cpu_init(struct cpufreq_policy *policy) return ret; }
- if (policy->cpu == 0) - register_pm_notifier(&tegra_cpu_pm_notifier); - policy->clk = cpu_clk; + policy->suspend_freq = freq_table[0].frequency; return 0; }
@@ -190,6 +151,9 @@ static struct cpufreq_driver tegra_cpufreq_driver = { .exit = tegra_cpu_exit, .name = "tegra", .attr = cpufreq_generic_attr, +#ifdef CONFIG_PM + .suspend = cpufreq_generic_suspend, +#endif };
static int __init tegra_cpufreq_init(void)
On Tuesday, March 04, 2014 11:00:26 AM Viresh Kumar wrote:
This patch adds cpufreq suspend/resume calls to dpm_{suspend|resume}() for handling suspend/resume of cpufreq governors.
Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found an 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 is fixed by this patch as we don't allow any operation on governors after device suspend and before device resume now.
We could have added these callbacks at dpm_{suspend|resume}_noirq() level but the problem here is that most of the devices (i.e. devices with ->suspend() callbacks) have already been suspended by now and so if drivers want to change frequency before suspending, then it might not be possible for many platforms (which depend on other peripherals like i2c, regulators, etc).
Reported-and-tested-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
V5->V6: 1-2-3/7 merged into 1/5
drivers/base/power/main.c | 5 +++ drivers/cpufreq/cpufreq.c | 111 +++++++++++++++++++++++----------------------- include/linux/cpufreq.h | 8 ++++ 3 files changed, 69 insertions(+), 55 deletions(-)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 42355e4..86d5e4f 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> @@ -866,6 +867,8 @@ void dpm_resume(pm_message_t state) mutex_unlock(&dpm_list_mtx); async_synchronize_full(); dpm_show_time(starttime, state, NULL);
- cpufreq_resume();
} /** @@ -1434,6 +1437,8 @@ int dpm_suspend(pm_message_t state) might_sleep();
- cpufreq_suspend();
- mutex_lock(&dpm_list_mtx); pm_transition = state; async_error = 0;
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 56b7b1b..2e43c08 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -26,7 +26,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> -#include <linux/syscore_ops.h> +#include <linux/suspend.h> #include <linux/tick.h> #include <trace/events/power.h> @@ -47,6 +47,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; @@ -1576,82 +1579,77 @@ static struct subsys_interface cpufreq_interface = { }; /**
- cpufreq_bp_suspend - Prepare the boot CPU for system suspend.
- cpufreq_suspend() - Suspend CPUFreq governors
- This function is only executed for the boot processor. The other CPUs
- have been put offline by means of CPU hotplug.
- Called during system wide Suspend/Hibernate cycles for suspending 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.
-static int cpufreq_bp_suspend(void) +void cpufreq_suspend(void) {
- int ret = 0;
- int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("suspending cpu %u\n", cpu);
- if (!cpufreq_driver)
return;
- /* If there's no policy for the boot CPU, we have nothing to do. */
- policy = cpufreq_cpu_get(cpu);
- if (!policy)
return 0;
- if (!has_target())
return;
- if (cpufreq_driver->suspend) {
ret = cpufreq_driver->suspend(policy);
if (ret)
printk(KERN_ERR "cpufreq: suspend failed in ->suspend "
"step on CPU %u\n", policy->cpu);
- 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);
else if (cpufreq_driver->suspend
So, previously the driver's ->suspend callback was executed with interrupts off and it was only executed for the boot CPU.
Now, it is going to be executed for all CPUs and with interrupts on.
This sounds potentially dangerous, so did you check all drivers supplying ->suspend if they are fine with that?
And same for ->resume, of course.
&& cpufreq_driver->suspend(policy))
pr_err("%s: Failed to suspend driver: %p\n", __func__,
}policy);
- cpufreq_cpu_put(policy);
- return ret;
- cpufreq_suspended = true;
} /**
- cpufreq_bp_resume - Restore proper frequency handling of the boot CPU.
- cpufreq_resume() - Resume CPUFreq governors
- 1.) resume CPUfreq hardware support (cpufreq_driver->resume())
- 2.) schedule call cpufreq_update_policy() ASAP as interrupts are
restored. It will verify that the current freq is in sync with
what we believe it to be. This is a bit later than when it
should be, but nonethteless it's better than calling
cpufreq_driver->get() here which might re-enable interrupts...
- This function is only executed for the boot CPU. The other CPUs have not
- been turned on yet.
- Called during system wide Suspend/Hibernate cycle for resuming governors that
*/
- are suspended with cpufreq_suspend().
-static void cpufreq_bp_resume(void) +void cpufreq_resume(void) {
- int ret = 0;
- int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("resuming cpu %u\n", cpu);
- if (!cpufreq_driver)
return;
- /* If there's no policy for the boot CPU, we have nothing to do. */
- policy = cpufreq_cpu_get(cpu);
- if (!policy)
- if (!has_target()) return;
- if (cpufreq_driver->resume) {
ret = cpufreq_driver->resume(policy);
if (ret) {
printk(KERN_ERR "cpufreq: resume failed in ->resume "
"step on CPU %u\n", policy->cpu);
goto fail;
}
- }
- pr_debug("%s: Resuming Governors\n", __func__);
- schedule_work(&policy->update);
- cpufreq_suspended = false;
-fail:
- cpufreq_cpu_put(policy);
-}
- 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);
else if (cpufreq_driver->resume
&& cpufreq_driver->resume(policy))
pr_err("%s: Failed to resume driver: %p\n", __func__,
policy);
-static struct syscore_ops cpufreq_syscore_ops = {
- .suspend = cpufreq_bp_suspend,
- .resume = cpufreq_bp_resume,
-};
/*
* schedule call cpufreq_update_policy() for boot CPU, i.e. last
* policy in list. It will verify that the current freq is in
* sync with what we believe it to be.
*/
if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
schedule_work(&policy->update);
- }
+} /**
- cpufreq_get_current_driver - return current driver's name
@@ -1868,6 +1866,10 @@ 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 */
- if (cpufreq_suspended)
return 0;
- if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) {
@@ -2407,7 +2409,6 @@ static int __init cpufreq_core_init(void) cpufreq_global_kobject = kobject_create(); BUG_ON(!cpufreq_global_kobject);
- register_syscore_ops(&cpufreq_syscore_ops);
return 0; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4d89e0e..94ed907 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -296,6 +296,14 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy) policy->cpuinfo.max_freq); } +#ifdef CONFIG_CPU_FREQ +void cpufreq_suspend(void); +void cpufreq_resume(void); +#else +static inline void cpufreq_suspend(void) {} +static inline void cpufreq_resume(void) {} +#endif
/*********************************************************************
CPUFREQ NOTIFIER INTERFACE *
*********************************************************************/
On 5 March 2014 08:37, Rafael J. Wysocki rjw@rjwysocki.net wrote:
So, previously the driver's ->suspend callback was executed with interrupts off and it was only executed for the boot CPU.
Now, it is going to be executed for all CPUs and with interrupts on.
Its not called for all CPUs but once for each instance of 'policy'.
This sounds potentially dangerous, so did you check all drivers supplying ->suspend if they are fine with that?
And same for ->resume, of course.
Completely agree with you on that. I had similar doubts when I started earlier. And then grep'd for suspend/resume in drivers/cpufreq and this is what I found:
drivers/cpufreq/acpi-cpufreq.c: Only have resume implemented and is unaffected with new code.
drivers/cpufreq/pmac32-cpufreq.c: This is always UP and so no issues.
drivers/cpufreq/s3c24xx-cpufreq.c: There is only one policy and so would be called only once.
drivers/cpufreq/speedstep-smi.c: Only has resume implemented and it looks to be harmless again..
So, I think yes, we shouldn't end up breaking any stuff here. Fingers crossed :) Otherwise as well, I am not going on leaves or something else in coming weeks and would be there to fix any issues that might come in. But I am still hopeful of not breaking any existing stuff.
On Tuesday, March 04, 2014 11:00:26 AM Viresh Kumar wrote:
This patch adds cpufreq suspend/resume calls to dpm_{suspend|resume}() for handling suspend/resume of cpufreq governors.
Lan Tianyu (Intel) & Jinhyuk Choi (Broadcom) found an 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 is fixed by this patch as we don't allow any operation on governors after device suspend and before device resume now.
We could have added these callbacks at dpm_{suspend|resume}_noirq() level but the problem here is that most of the devices (i.e. devices with ->suspend() callbacks) have already been suspended by now and so if drivers want to change frequency before suspending, then it might not be possible for many platforms (which depend on other peripherals like i2c, regulators, etc).
Reported-and-tested-by: Lan Tianyu tianyu.lan@intel.com Reported-by: Jinhyuk Choi jinchoi@broadcom.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Queued up for 3.15, thanks!
V5->V6: 1-2-3/7 merged into 1/5
drivers/base/power/main.c | 5 +++ drivers/cpufreq/cpufreq.c | 111 +++++++++++++++++++++++----------------------- include/linux/cpufreq.h | 8 ++++ 3 files changed, 69 insertions(+), 55 deletions(-)
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 42355e4..86d5e4f 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> @@ -866,6 +867,8 @@ void dpm_resume(pm_message_t state) mutex_unlock(&dpm_list_mtx); async_synchronize_full(); dpm_show_time(starttime, state, NULL);
- cpufreq_resume();
} /** @@ -1434,6 +1437,8 @@ int dpm_suspend(pm_message_t state) might_sleep();
- cpufreq_suspend();
- mutex_lock(&dpm_list_mtx); pm_transition = state; async_error = 0;
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 56b7b1b..2e43c08 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -26,7 +26,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> -#include <linux/syscore_ops.h> +#include <linux/suspend.h> #include <linux/tick.h> #include <trace/events/power.h> @@ -47,6 +47,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; @@ -1576,82 +1579,77 @@ static struct subsys_interface cpufreq_interface = { }; /**
- cpufreq_bp_suspend - Prepare the boot CPU for system suspend.
- cpufreq_suspend() - Suspend CPUFreq governors
- This function is only executed for the boot processor. The other CPUs
- have been put offline by means of CPU hotplug.
- Called during system wide Suspend/Hibernate cycles for suspending 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.
-static int cpufreq_bp_suspend(void) +void cpufreq_suspend(void) {
- int ret = 0;
- int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("suspending cpu %u\n", cpu);
- if (!cpufreq_driver)
return;
- /* If there's no policy for the boot CPU, we have nothing to do. */
- policy = cpufreq_cpu_get(cpu);
- if (!policy)
return 0;
- if (!has_target())
return;
- if (cpufreq_driver->suspend) {
ret = cpufreq_driver->suspend(policy);
if (ret)
printk(KERN_ERR "cpufreq: suspend failed in ->suspend "
"step on CPU %u\n", policy->cpu);
- 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);
else if (cpufreq_driver->suspend
&& cpufreq_driver->suspend(policy))
pr_err("%s: Failed to suspend driver: %p\n", __func__,
}policy);
- cpufreq_cpu_put(policy);
- return ret;
- cpufreq_suspended = true;
} /**
- cpufreq_bp_resume - Restore proper frequency handling of the boot CPU.
- cpufreq_resume() - Resume CPUFreq governors
- 1.) resume CPUfreq hardware support (cpufreq_driver->resume())
- 2.) schedule call cpufreq_update_policy() ASAP as interrupts are
restored. It will verify that the current freq is in sync with
what we believe it to be. This is a bit later than when it
should be, but nonethteless it's better than calling
cpufreq_driver->get() here which might re-enable interrupts...
- This function is only executed for the boot CPU. The other CPUs have not
- been turned on yet.
- Called during system wide Suspend/Hibernate cycle for resuming governors that
*/
- are suspended with cpufreq_suspend().
-static void cpufreq_bp_resume(void) +void cpufreq_resume(void) {
- int ret = 0;
- int cpu = smp_processor_id(); struct cpufreq_policy *policy;
- pr_debug("resuming cpu %u\n", cpu);
- if (!cpufreq_driver)
return;
- /* If there's no policy for the boot CPU, we have nothing to do. */
- policy = cpufreq_cpu_get(cpu);
- if (!policy)
- if (!has_target()) return;
- if (cpufreq_driver->resume) {
ret = cpufreq_driver->resume(policy);
if (ret) {
printk(KERN_ERR "cpufreq: resume failed in ->resume "
"step on CPU %u\n", policy->cpu);
goto fail;
}
- }
- pr_debug("%s: Resuming Governors\n", __func__);
- schedule_work(&policy->update);
- cpufreq_suspended = false;
-fail:
- cpufreq_cpu_put(policy);
-}
- 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);
else if (cpufreq_driver->resume
&& cpufreq_driver->resume(policy))
pr_err("%s: Failed to resume driver: %p\n", __func__,
policy);
-static struct syscore_ops cpufreq_syscore_ops = {
- .suspend = cpufreq_bp_suspend,
- .resume = cpufreq_bp_resume,
-};
/*
* schedule call cpufreq_update_policy() for boot CPU, i.e. last
* policy in list. It will verify that the current freq is in
* sync with what we believe it to be.
*/
if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
schedule_work(&policy->update);
- }
+} /**
- cpufreq_get_current_driver - return current driver's name
@@ -1868,6 +1866,10 @@ 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 */
- if (cpufreq_suspended)
return 0;
- if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) {
@@ -2407,7 +2409,6 @@ static int __init cpufreq_core_init(void) cpufreq_global_kobject = kobject_create(); BUG_ON(!cpufreq_global_kobject);
- register_syscore_ops(&cpufreq_syscore_ops);
return 0; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 4d89e0e..94ed907 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -296,6 +296,14 @@ cpufreq_verify_within_cpu_limits(struct cpufreq_policy *policy) policy->cpuinfo.max_freq); } +#ifdef CONFIG_CPU_FREQ +void cpufreq_suspend(void); +void cpufreq_resume(void); +#else +static inline void cpufreq_suspend(void) {} +static inline void cpufreq_resume(void) {} +#endif
/*********************************************************************
CPUFREQ NOTIFIER INTERFACE *
*********************************************************************/
linaro-kernel@lists.linaro.org