[PATCH V2 0/2] cpufreq: suspend governors during s2r/hibernation

[PATCH V2 0/2] cpufreq: suspend governors during s2r/hibernation

[Viresh Kumar]

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(+)

-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Viresh Kumar]

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,
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Viresh Kumar]

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;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Rafael J. Wysocki]

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!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Rafael J. Wysocki]

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!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Viresh Kumar]

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..

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Viresh Kumar]

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.

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Viresh Kumar]

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..

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Rafael J. Wysocki]

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!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Stephen Warren]

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.

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Rafael J. Wysocki]

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!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Viresh Kumar]

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

Re: [PATCH V2 2/2] cpufreq: Change freq before suspending governors

[Viresh Kumar]

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..

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Nishanth Menon]

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.

-- 
Regards,
Nishanth Menon

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Viresh Kumar]

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

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Rafael J. Wysocki]

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

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Viresh Kumar]

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?

Re: [PATCH V2 1/2] cpufreq: suspend governors on system suspend/hibernate

[Rafael J. Wysocki]

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!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.