| Message ID | df162ba4cbdf1379ccfa5dd4ca34153626ae9c71.1385118256.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 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 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 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..
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;
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(+)