* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-25 10:56 MyungJoo Ham 2011-01-25 12:40 ` Mark Brown 0 siblings, 1 reply; 23+ messages in thread From: MyungJoo Ham @ 2011-01-25 10:56 UTC (permalink / raw) To: linux-arm-kernel 2011. 1. 25. ?? 1:27? "Kyungmin Park" <kmpark@infradead.org>?? ??: > > On Tue, Jan 25, 2011 at 4:35 AM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: > > On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: > >> On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > > > >> > Hrm, what's the situation where that happens and why does it cause > >> > problems? The regulator API doesn't care if suspend is going on, and > >> > nor do any of the current drivers for regulators. There is an issue > >> > with keeping things like I2C alive until the bitter end of suspend so > >> > you've got a control bus to the regulators but that's a generic issue > >> > which crops up with other subsystems too so a regulator-specific > >> > workaround seems dodgy. > > > >> The end result is that the regulator driver calls the i2c driver after > >> the i2c driver has been suspended. On Tegra, this happens because of > >> voltage scaling on the CPU regulator, which can be on the i2c bus. A > >> sleep in a suspend handler after the i2c bus has been suspended can > >> cause the cpufreq governor to lower the frequency, and try to lower > >> the voltage as well. > > > > OK, that's a general problem - you need to ensure that the I2C and SPI > > controllers are suspended really late. The OMAP guys also have this > > problem for some RTCs, though in their case things are slightly > > different due to their use of runtime PM. Looking at your I2C driver > > (BTW, I'd suggest reminding Ben on a more regular basis about that) I > > suspect that moving your suspend and resume callbacks to the _noirq > > varaints will cover a lot of this. > > Right, it's same as Samsung SoCs. we also disable to access the > cpufreq when suspending. > the PMIC is connected with I2C and I2C is suspended before PMIC. > > Thank you, > Kyungmin Park > Moreover, the suspend-related cpufreq issue gets worse with hibernation. Generally PMICs preserve their register values during suspend-to-mem; however, they lose the values with power-off (and hibernation). In samsung SoCs, we have mitigated the issue by stopping cpufreq at pm_begin (could have a bit different name) and resume with end. When stopping cpufreq, we have made it to fix at the boot freq so that we dont face into the inconsistency with hibernation. Sometimes, when we set voltages for the core, bus, and others, simply saving the whole things and restoring them does not work because the order how the voltages change may matter (and it does with some of Samsung SoCs). Thus, we made them to stop changing and fix at the boot-time default during suspend and hibernation. Cheers! MyungJoo > > > >> The problem isn't limited to i2c busses, there are some regulators on > >> spi busses. You would need to ensure that any driver that has a > > > > For all practical purposes in this sort of discussion you can typically > > do a s/I2C/I2C and SPI/ - in terms of system integration issues they're > > very similar. > > > >> regulator consumer suspends before the regulator driver it > >> communicates with, and that still wouldn't fix cpufreq, which has a > >> sysdev suspend handler. > > > > So you actively need to push the processor into low power mode during > > suspend? I'm still not 100% clear what's triggering the issues you're > > seeing and why this seems to be Tegra-only. If there were a general > > cpufreq/regulator interaction here I'd expect to see all ARM cpufreq > > drivers needing to do the same thing (and probably some handling of this > > in cpufreq core as a result). If it's not such an issue I'd expect > > there's also entertaining suspend ordering issues elsewhere. > > > > Typically this stuff isn't a problem for regulators themselves since if > > they're active by the time I2C is off they normally get suspended by > > hardware handshakes from the CPU as that enters low power mode - right > > now we have no regulators at all in mainline that do anything in > > software on suspend. We can get a long way by just ignoring what > > happens to regulators over suspend (there is some work to do here but > > it's orthogonal to this sort of issue). > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110125/5ffddccf/attachment-0001.html> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-25 10:56 [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend MyungJoo Ham @ 2011-01-25 12:40 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-25 12:40 UTC (permalink / raw) To: MyungJoo Ham Cc: Kyungmin Park, linux-tegra, konkers, linux-arm-kernel, Russell King, linux-kernel, olof, Colin Cross On Tue, Jan 25, 2011 at 07:56:19PM +0900, MyungJoo Ham wrote: > Moreover, the suspend-related cpufreq issue gets worse with hibernation. > Generally PMICs preserve their register values during suspend-to-mem; > however, they lose the values with power-off (and hibernation). That's interesting - hibernation has been a bit easier when I've looked at this stuff precisely because it's equivalent to a cold boot, if the PMIC weren't able to boot the CPU then us software engineers wouldn't have to worry about any of this stuff :) > In samsung SoCs, we have mitigated the issue by stopping cpufreq at pm_begin > (could have a bit different name) and resume with end. When stopping > cpufreq, we have made it to fix at the boot freq so that we dont face into > the inconsistency with hibernation. > Sometimes, when we set voltages for the core, bus, and others, simply saving > the whole things and restoring them does not work because the order how the > voltages change may matter (and it does with some of Samsung SoCs). Thus, we > made them to stop changing and fix at the boot-time default during suspend > and hibernation. The ordering matters greatly, this is the big part of what the PMIC power sequencing is doing. There's also often some handshaking going on so the PMIC has to wait for the CPU at various points (with delays or with explicit signals) in the process. Just thinking about how we could address this in cpufreq is the major issue that the CPU comes back from resume in a state other than that which cpufreq remembered it being in or is the issue that the system tries to restore old voltages as part of the resume process? I guess either way a feature in cpufreq which allows the driver to override the governor-chosen configuration for suspend would provide a mechanism to deal with the issue. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-25 12:40 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-25 12:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 25, 2011 at 07:56:19PM +0900, MyungJoo Ham wrote: > Moreover, the suspend-related cpufreq issue gets worse with hibernation. > Generally PMICs preserve their register values during suspend-to-mem; > however, they lose the values with power-off (and hibernation). That's interesting - hibernation has been a bit easier when I've looked at this stuff precisely because it's equivalent to a cold boot, if the PMIC weren't able to boot the CPU then us software engineers wouldn't have to worry about any of this stuff :) > In samsung SoCs, we have mitigated the issue by stopping cpufreq at pm_begin > (could have a bit different name) and resume with end. When stopping > cpufreq, we have made it to fix at the boot freq so that we dont face into > the inconsistency with hibernation. > Sometimes, when we set voltages for the core, bus, and others, simply saving > the whole things and restoring them does not work because the order how the > voltages change may matter (and it does with some of Samsung SoCs). Thus, we > made them to stop changing and fix at the boot-time default during suspend > and hibernation. The ordering matters greatly, this is the big part of what the PMIC power sequencing is doing. There's also often some handshaking going on so the PMIC has to wait for the CPU at various points (with delays or with explicit signals) in the process. Just thinking about how we could address this in cpufreq is the major issue that the CPU comes back from resume in a state other than that which cpufreq remembered it being in or is the issue that the system tries to restore old voltages as part of the resume process? I guess either way a feature in cpufreq which allows the driver to override the governor-chosen configuration for suspend would provide a mechanism to deal with the issue. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 00/28] Updates for Tegra support in 2.6.39 @ 2011-01-24 2:01 Colin Cross 2011-01-24 2:01 ` Colin Cross 0 siblings, 1 reply; 23+ messages in thread From: Colin Cross @ 2011-01-24 2:01 UTC (permalink / raw) To: linux-arm-kernel This patch set adds support for suspend, idle, and cpufreq for the Tegra subsystem. Patch 2 adds functions to save and restore the GIC state during suspend and resume, and patch 3 exports the GIC irq chip functions to support custom IRQ chips that need to update the GIC as well as another block during IRQ operations. Patches 2 and 3 depend on patch 1, which fixes a naming conflict in mach-tegra, so the whole stack will need to go in through one tree. arch/arm/common/gic.c | 138 ++++- arch/arm/configs/tegra_defconfig | 123 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 3 +- arch/arm/include/asm/hardware/gic.h | 13 + arch/arm/mach-tegra/Makefile | 12 +- arch/arm/mach-tegra/board.h | 6 + arch/arm/mach-tegra/common.c | 49 ++- arch/arm/mach-tegra/cortex-a9.S | 504 +++++++++++++++ arch/arm/mach-tegra/cpu-tegra.c | 75 ++- arch/arm/mach-tegra/cpuidle.c | 714 ++++++++++++++++++++++ arch/arm/mach-tegra/dma.c | 198 ++++--- arch/arm/mach-tegra/gpio.c | 15 +- arch/arm/mach-tegra/headsmp-t2.S | 216 +++++++ arch/arm/mach-tegra/headsmp.S | 61 -- arch/arm/mach-tegra/hotplug.c | 126 ---- arch/arm/mach-tegra/include/mach/clk.h | 2 + arch/arm/mach-tegra/include/mach/clkdev.h | 2 + arch/arm/mach-tegra/include/mach/debug-macro.S | 25 +- arch/arm/mach-tegra/include/mach/iomap.h | 69 ++- arch/arm/mach-tegra/include/mach/irqs.h | 14 +- arch/arm/mach-tegra/include/mach/legacy_irq.h | 7 +- arch/arm/mach-tegra/include/mach/pinmux-t2.h | 10 + arch/arm/mach-tegra/include/mach/powergate.h | 40 ++ arch/arm/mach-tegra/include/mach/suspend.h | 80 +++ arch/arm/mach-tegra/include/mach/system.h | 10 +- arch/arm/mach-tegra/include/mach/uncompress.h | 18 +- arch/arm/mach-tegra/irq.c | 334 ++++++++--- arch/arm/mach-tegra/legacy_irq.c | 109 ++++- arch/arm/mach-tegra/pinmux-t2-tables.c | 26 +- arch/arm/mach-tegra/platsmp.c | 89 +++- arch/arm/mach-tegra/power-macros.S | 57 ++ arch/arm/mach-tegra/power.h | 64 ++ arch/arm/mach-tegra/powergate.c | 212 +++++++ arch/arm/mach-tegra/suspend-t2.c | 414 +++++++++++++ arch/arm/mach-tegra/suspend.c | 776 ++++++++++++++++++++++++ arch/arm/mach-tegra/tegra2_clocks.c | 264 ++++++++- arch/arm/mach-tegra/tegra2_save.S | 413 +++++++++++++ arch/arm/mach-tegra/timer.c | 96 +++- arch/arm/mm/cache-l2x0.c | 14 +- 39 files changed, 4898 insertions(+), 500 deletions(-) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 2:01 [PATCH v2 00/28] Updates for Tegra support in 2.6.39 Colin Cross @ 2011-01-24 2:01 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 2:01 UTC (permalink / raw) To: linux-tegra Cc: linux-arm-kernel, olof, konkers, Colin Cross, Russell King, linux-kernel Adds a SUSPEND_PREPARE notification hook to drop the frequency to the lowest possible during suspend. This prevents the cpufreq driver from attempting regulator calls after suspend has started - the regulator api can call into drivers that have already been suspended. Also adds 216MHz (off of PLLP) as the lowest CPU frequency, which allows PLLX to be turned off. Signed-off-by: Colin Cross <ccross@android.com> --- arch/arm/mach-tegra/cpu-tegra.c | 75 +++++++++++++++++++++++++++++++-------- 1 files changed, 60 insertions(+), 15 deletions(-) diff --git a/arch/arm/mach-tegra/cpu-tegra.c b/arch/arm/mach-tegra/cpu-tegra.c index fea5719..ad26a9f 100644 --- a/arch/arm/mach-tegra/cpu-tegra.c +++ b/arch/arm/mach-tegra/cpu-tegra.c @@ -28,6 +28,7 @@ #include <linux/err.h> #include <linux/clk.h> #include <linux/io.h> +#include <linux/suspend.h> #include <asm/system.h> @@ -36,14 +37,15 @@ /* Frequency table index must be sequential starting at 0 */ static struct cpufreq_frequency_table freq_table[] = { - { 0, 312000 }, - { 1, 456000 }, - { 2, 608000 }, - { 3, 760000 }, - { 4, 816000 }, - { 5, 912000 }, - { 6, 1000000 }, - { 7, CPUFREQ_TABLE_END }, + { 0, 216000 }, + { 1, 312000 }, + { 2, 456000 }, + { 3, 608000 }, + { 4, 760000 }, + { 5, 816000 }, + { 6, 912000 }, + { 7, 1000000 }, + { 8, CPUFREQ_TABLE_END }, }; #define NUM_CPUS 2 @@ -51,6 +53,8 @@ static struct cpufreq_frequency_table freq_table[] = { static struct clk *cpu_clk; static unsigned long target_cpu_speed[NUM_CPUS]; +static DEFINE_MUTEX(tegra_cpu_lock); +static bool is_suspended; int tegra_verify_speed(struct cpufreq_policy *policy) { @@ -68,16 +72,11 @@ unsigned int tegra_getspeed(unsigned int cpu) return rate; } -static int tegra_update_cpu_speed(void) +static int tegra_update_cpu_speed(unsigned long rate) { - int i; - unsigned long rate = 0; int ret = 0; struct cpufreq_freqs freqs; - for_each_online_cpu(i) - rate = max(rate, target_cpu_speed[i]); - freqs.old = tegra_getspeed(0); freqs.new = rate; @@ -105,12 +104,30 @@ static int tegra_update_cpu_speed(void) return 0; } +static unsigned long tegra_cpu_highest_speed(void) +{ + unsigned long rate = 0; + int i; + + for_each_online_cpu(i) + rate = max(rate, target_cpu_speed[i]); + return rate; +} + static int tegra_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) { int idx; unsigned int freq; + int ret = 0; + + mutex_lock(&tegra_cpu_lock); + + if (is_suspended) { + ret = -EBUSY; + goto out; + } cpufreq_frequency_table_target(policy, freq_table, target_freq, relation, &idx); @@ -119,9 +136,34 @@ static int tegra_target(struct cpufreq_policy *policy, target_cpu_speed[policy->cpu] = freq; - return tegra_update_cpu_speed(); + ret = tegra_update_cpu_speed(tegra_cpu_highest_speed()); + +out: + mutex_unlock(&tegra_cpu_lock); + return ret; } +static int tegra_pm_notify(struct notifier_block *nb, unsigned long event, + void *dummy) +{ + mutex_lock(&tegra_cpu_lock); + if (event == PM_SUSPEND_PREPARE) { + is_suspended = true; + pr_info("Tegra cpufreq suspend: setting frequency to %d kHz\n", + freq_table[0].frequency); + tegra_update_cpu_speed(freq_table[0].frequency); + } 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) { if (policy->cpu >= NUM_CPUS) @@ -142,6 +184,9 @@ static int tegra_cpu_init(struct cpufreq_policy *policy) policy->shared_type = CPUFREQ_SHARED_TYPE_ALL; cpumask_copy(policy->related_cpus, cpu_possible_mask); + if (policy->cpu == 0) + register_pm_notifier(&tegra_cpu_pm_notifier); + return 0; } -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 2:01 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 2:01 UTC (permalink / raw) To: linux-arm-kernel Adds a SUSPEND_PREPARE notification hook to drop the frequency to the lowest possible during suspend. This prevents the cpufreq driver from attempting regulator calls after suspend has started - the regulator api can call into drivers that have already been suspended. Also adds 216MHz (off of PLLP) as the lowest CPU frequency, which allows PLLX to be turned off. Signed-off-by: Colin Cross <ccross@android.com> --- arch/arm/mach-tegra/cpu-tegra.c | 75 +++++++++++++++++++++++++++++++-------- 1 files changed, 60 insertions(+), 15 deletions(-) diff --git a/arch/arm/mach-tegra/cpu-tegra.c b/arch/arm/mach-tegra/cpu-tegra.c index fea5719..ad26a9f 100644 --- a/arch/arm/mach-tegra/cpu-tegra.c +++ b/arch/arm/mach-tegra/cpu-tegra.c @@ -28,6 +28,7 @@ #include <linux/err.h> #include <linux/clk.h> #include <linux/io.h> +#include <linux/suspend.h> #include <asm/system.h> @@ -36,14 +37,15 @@ /* Frequency table index must be sequential starting at 0 */ static struct cpufreq_frequency_table freq_table[] = { - { 0, 312000 }, - { 1, 456000 }, - { 2, 608000 }, - { 3, 760000 }, - { 4, 816000 }, - { 5, 912000 }, - { 6, 1000000 }, - { 7, CPUFREQ_TABLE_END }, + { 0, 216000 }, + { 1, 312000 }, + { 2, 456000 }, + { 3, 608000 }, + { 4, 760000 }, + { 5, 816000 }, + { 6, 912000 }, + { 7, 1000000 }, + { 8, CPUFREQ_TABLE_END }, }; #define NUM_CPUS 2 @@ -51,6 +53,8 @@ static struct cpufreq_frequency_table freq_table[] = { static struct clk *cpu_clk; static unsigned long target_cpu_speed[NUM_CPUS]; +static DEFINE_MUTEX(tegra_cpu_lock); +static bool is_suspended; int tegra_verify_speed(struct cpufreq_policy *policy) { @@ -68,16 +72,11 @@ unsigned int tegra_getspeed(unsigned int cpu) return rate; } -static int tegra_update_cpu_speed(void) +static int tegra_update_cpu_speed(unsigned long rate) { - int i; - unsigned long rate = 0; int ret = 0; struct cpufreq_freqs freqs; - for_each_online_cpu(i) - rate = max(rate, target_cpu_speed[i]); - freqs.old = tegra_getspeed(0); freqs.new = rate; @@ -105,12 +104,30 @@ static int tegra_update_cpu_speed(void) return 0; } +static unsigned long tegra_cpu_highest_speed(void) +{ + unsigned long rate = 0; + int i; + + for_each_online_cpu(i) + rate = max(rate, target_cpu_speed[i]); + return rate; +} + static int tegra_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) { int idx; unsigned int freq; + int ret = 0; + + mutex_lock(&tegra_cpu_lock); + + if (is_suspended) { + ret = -EBUSY; + goto out; + } cpufreq_frequency_table_target(policy, freq_table, target_freq, relation, &idx); @@ -119,9 +136,34 @@ static int tegra_target(struct cpufreq_policy *policy, target_cpu_speed[policy->cpu] = freq; - return tegra_update_cpu_speed(); + ret = tegra_update_cpu_speed(tegra_cpu_highest_speed()); + +out: + mutex_unlock(&tegra_cpu_lock); + return ret; } +static int tegra_pm_notify(struct notifier_block *nb, unsigned long event, + void *dummy) +{ + mutex_lock(&tegra_cpu_lock); + if (event == PM_SUSPEND_PREPARE) { + is_suspended = true; + pr_info("Tegra cpufreq suspend: setting frequency to %d kHz\n", + freq_table[0].frequency); + tegra_update_cpu_speed(freq_table[0].frequency); + } 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) { if (policy->cpu >= NUM_CPUS) @@ -142,6 +184,9 @@ static int tegra_cpu_init(struct cpufreq_policy *policy) policy->shared_type = CPUFREQ_SHARED_TYPE_ALL; cpumask_copy(policy->related_cpus, cpu_possible_mask); + if (policy->cpu == 0) + register_pm_notifier(&tegra_cpu_pm_notifier); + return 0; } -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 2:01 ` Colin Cross @ 2011-01-24 14:41 ` Mark Brown -1 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 14:41 UTC (permalink / raw) To: Colin Cross Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Sun, Jan 23, 2011 at 06:01:25PM -0800, Colin Cross wrote: > Adds a SUSPEND_PREPARE notification hook to drop the frequency to > the lowest possible during suspend. This prevents the cpufreq driver > from attempting regulator calls after suspend has started - the > regulator api can call into drivers that have already been suspended. Hrm, what's the situation where that happens and why does it cause problems? The regulator API doesn't care if suspend is going on, and nor do any of the current drivers for regulators. There is an issue with keeping things like I2C alive until the bitter end of suspend so you've got a control bus to the regulators but that's a generic issue which crops up with other subsystems too so a regulator-specific workaround seems dodgy. The patch itself seems fine, it's just that it feels like there's something else going on. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 14:41 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 14:41 UTC (permalink / raw) To: linux-arm-kernel On Sun, Jan 23, 2011 at 06:01:25PM -0800, Colin Cross wrote: > Adds a SUSPEND_PREPARE notification hook to drop the frequency to > the lowest possible during suspend. This prevents the cpufreq driver > from attempting regulator calls after suspend has started - the > regulator api can call into drivers that have already been suspended. Hrm, what's the situation where that happens and why does it cause problems? The regulator API doesn't care if suspend is going on, and nor do any of the current drivers for regulators. There is an issue with keeping things like I2C alive until the bitter end of suspend so you've got a control bus to the regulators but that's a generic issue which crops up with other subsystems too so a regulator-specific workaround seems dodgy. The patch itself seems fine, it's just that it feels like there's something else going on. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 14:41 ` Mark Brown @ 2011-01-24 18:50 ` Colin Cross -1 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 18:50 UTC (permalink / raw) To: Mark Brown Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Sun, Jan 23, 2011 at 06:01:25PM -0800, Colin Cross wrote: >> Adds a SUSPEND_PREPARE notification hook to drop the frequency to >> the lowest possible during suspend. This prevents the cpufreq driver >> from attempting regulator calls after suspend has started - the >> regulator api can call into drivers that have already been suspended. > > Hrm, what's the situation where that happens and why does it cause > problems? The regulator API doesn't care if suspend is going on, and > nor do any of the current drivers for regulators. There is an issue > with keeping things like I2C alive until the bitter end of suspend so > you've got a control bus to the regulators but that's a generic issue > which crops up with other subsystems too so a regulator-specific > workaround seems dodgy. The end result is that the regulator driver calls the i2c driver after the i2c driver has been suspended. On Tegra, this happens because of voltage scaling on the CPU regulator, which can be on the i2c bus. A sleep in a suspend handler after the i2c bus has been suspended can cause the cpufreq governor to lower the frequency, and try to lower the voltage as well. The problem isn't limited to i2c busses, there are some regulators on spi busses. You would need to ensure that any driver that has a regulator consumer suspends before the regulator driver it communicates with, and that still wouldn't fix cpufreq, which has a sysdev suspend handler. > The patch itself seems fine, it's just that it feels like there's > something else going on. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 18:50 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 18:50 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Sun, Jan 23, 2011 at 06:01:25PM -0800, Colin Cross wrote: >> Adds a SUSPEND_PREPARE notification hook to drop the frequency to >> the lowest possible during suspend. ?This prevents the cpufreq driver >> from attempting regulator calls after suspend has started - the >> regulator api can call into drivers that have already been suspended. > > Hrm, what's the situation where that happens and why does it cause > problems? ?The regulator API doesn't care if suspend is going on, and > nor do any of the current drivers for regulators. ?There is an issue > with keeping things like I2C alive until the bitter end of suspend so > you've got a control bus to the regulators but that's a generic issue > which crops up with other subsystems too so a regulator-specific > workaround seems dodgy. The end result is that the regulator driver calls the i2c driver after the i2c driver has been suspended. On Tegra, this happens because of voltage scaling on the CPU regulator, which can be on the i2c bus. A sleep in a suspend handler after the i2c bus has been suspended can cause the cpufreq governor to lower the frequency, and try to lower the voltage as well. The problem isn't limited to i2c busses, there are some regulators on spi busses. You would need to ensure that any driver that has a regulator consumer suspends before the regulator driver it communicates with, and that still wouldn't fix cpufreq, which has a sysdev suspend handler. > The patch itself seems fine, it's just that it feels like there's > something else going on. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 18:50 ` Colin Cross @ 2011-01-24 19:35 ` Mark Brown -1 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 19:35 UTC (permalink / raw) To: Colin Cross Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: > On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > > Hrm, what's the situation where that happens and why does it cause > > problems? The regulator API doesn't care if suspend is going on, and > > nor do any of the current drivers for regulators. There is an issue > > with keeping things like I2C alive until the bitter end of suspend so > > you've got a control bus to the regulators but that's a generic issue > > which crops up with other subsystems too so a regulator-specific > > workaround seems dodgy. > The end result is that the regulator driver calls the i2c driver after > the i2c driver has been suspended. On Tegra, this happens because of > voltage scaling on the CPU regulator, which can be on the i2c bus. A > sleep in a suspend handler after the i2c bus has been suspended can > cause the cpufreq governor to lower the frequency, and try to lower > the voltage as well. OK, that's a general problem - you need to ensure that the I2C and SPI controllers are suspended really late. The OMAP guys also have this problem for some RTCs, though in their case things are slightly different due to their use of runtime PM. Looking at your I2C driver (BTW, I'd suggest reminding Ben on a more regular basis about that) I suspect that moving your suspend and resume callbacks to the _noirq varaints will cover a lot of this. > The problem isn't limited to i2c busses, there are some regulators on > spi busses. You would need to ensure that any driver that has a For all practical purposes in this sort of discussion you can typically do a s/I2C/I2C and SPI/ - in terms of system integration issues they're very similar. > regulator consumer suspends before the regulator driver it > communicates with, and that still wouldn't fix cpufreq, which has a > sysdev suspend handler. So you actively need to push the processor into low power mode during suspend? I'm still not 100% clear what's triggering the issues you're seeing and why this seems to be Tegra-only. If there were a general cpufreq/regulator interaction here I'd expect to see all ARM cpufreq drivers needing to do the same thing (and probably some handling of this in cpufreq core as a result). If it's not such an issue I'd expect there's also entertaining suspend ordering issues elsewhere. Typically this stuff isn't a problem for regulators themselves since if they're active by the time I2C is off they normally get suspended by hardware handshakes from the CPU as that enters low power mode - right now we have no regulators at all in mainline that do anything in software on suspend. We can get a long way by just ignoring what happens to regulators over suspend (there is some work to do here but it's orthogonal to this sort of issue). ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 19:35 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 19:35 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: > On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > > Hrm, what's the situation where that happens and why does it cause > > problems? ?The regulator API doesn't care if suspend is going on, and > > nor do any of the current drivers for regulators. ?There is an issue > > with keeping things like I2C alive until the bitter end of suspend so > > you've got a control bus to the regulators but that's a generic issue > > which crops up with other subsystems too so a regulator-specific > > workaround seems dodgy. > The end result is that the regulator driver calls the i2c driver after > the i2c driver has been suspended. On Tegra, this happens because of > voltage scaling on the CPU regulator, which can be on the i2c bus. A > sleep in a suspend handler after the i2c bus has been suspended can > cause the cpufreq governor to lower the frequency, and try to lower > the voltage as well. OK, that's a general problem - you need to ensure that the I2C and SPI controllers are suspended really late. The OMAP guys also have this problem for some RTCs, though in their case things are slightly different due to their use of runtime PM. Looking at your I2C driver (BTW, I'd suggest reminding Ben on a more regular basis about that) I suspect that moving your suspend and resume callbacks to the _noirq varaints will cover a lot of this. > The problem isn't limited to i2c busses, there are some regulators on > spi busses. You would need to ensure that any driver that has a For all practical purposes in this sort of discussion you can typically do a s/I2C/I2C and SPI/ - in terms of system integration issues they're very similar. > regulator consumer suspends before the regulator driver it > communicates with, and that still wouldn't fix cpufreq, which has a > sysdev suspend handler. So you actively need to push the processor into low power mode during suspend? I'm still not 100% clear what's triggering the issues you're seeing and why this seems to be Tegra-only. If there were a general cpufreq/regulator interaction here I'd expect to see all ARM cpufreq drivers needing to do the same thing (and probably some handling of this in cpufreq core as a result). If it's not such an issue I'd expect there's also entertaining suspend ordering issues elsewhere. Typically this stuff isn't a problem for regulators themselves since if they're active by the time I2C is off they normally get suspended by hardware handshakes from the CPU as that enters low power mode - right now we have no regulators at all in mainline that do anything in software on suspend. We can get a long way by just ignoring what happens to regulators over suspend (there is some work to do here but it's orthogonal to this sort of issue). ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 19:35 ` Mark Brown @ 2011-01-24 19:52 ` Colin Cross -1 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 19:52 UTC (permalink / raw) To: Mark Brown Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > >> > Hrm, what's the situation where that happens and why does it cause >> > problems? The regulator API doesn't care if suspend is going on, and >> > nor do any of the current drivers for regulators. There is an issue >> > with keeping things like I2C alive until the bitter end of suspend so >> > you've got a control bus to the regulators but that's a generic issue >> > which crops up with other subsystems too so a regulator-specific >> > workaround seems dodgy. > >> The end result is that the regulator driver calls the i2c driver after >> the i2c driver has been suspended. On Tegra, this happens because of >> voltage scaling on the CPU regulator, which can be on the i2c bus. A >> sleep in a suspend handler after the i2c bus has been suspended can >> cause the cpufreq governor to lower the frequency, and try to lower >> the voltage as well. > > OK, that's a general problem - you need to ensure that the I2C and SPI > controllers are suspended really late. The OMAP guys also have this > problem for some RTCs, though in their case things are slightly > different due to their use of runtime PM. Looking at your I2C driver > (BTW, I'd suggest reminding Ben on a more regular basis about that) I > suspect that moving your suspend and resume callbacks to the _noirq > varaints will cover a lot of this. Even _noirq isn't late enough, if cpufreq keeps trying to change the frequency (and thus voltage) until sysdev suspend. >> The problem isn't limited to i2c busses, there are some regulators on >> spi busses. You would need to ensure that any driver that has a > > For all practical purposes in this sort of discussion you can typically > do a s/I2C/I2C and SPI/ - in terms of system integration issues they're > very similar. True >> regulator consumer suspends before the regulator driver it >> communicates with, and that still wouldn't fix cpufreq, which has a >> sysdev suspend handler. > > So you actively need to push the processor into low power mode during > suspend? I'm still not 100% clear what's triggering the issues you're > seeing and why this seems to be Tegra-only. If there were a general > cpufreq/regulator interaction here I'd expect to see all ARM cpufreq > drivers needing to do the same thing (and probably some handling of this > in cpufreq core as a result). If it's not such an issue I'd expect > there's also entertaining suspend ordering issues elsewhere. It's more of a voltage scaling issue than a cpufreq issue directly. Tegra requires the voltage be set to nominal during resume, and the only time it can be set for resume is before I2C suspends. I handle the problem with a suspend notifier in the latest version of the clock voltage scaling patches, but I kept this patch to avoid cpufreq trying to go to frequencies that are not supported by the suspend voltage. > Typically this stuff isn't a problem for regulators themselves since if > they're active by the time I2C is off they normally get suspended by > hardware handshakes from the CPU as that enters low power mode - right > now we have no regulators at all in mainline that do anything in > software on suspend. We can get a long way by just ignoring what > happens to regulators over suspend (there is some work to do here but > it's orthogonal to this sort of issue). The regulator driver itself has nothing to do, and when the CPU enters its lowest power mode it signals the regulator to turn off, but something needs to tell the regulator to go to the correct voltage. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 19:52 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 19:52 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > >> > Hrm, what's the situation where that happens and why does it cause >> > problems? ?The regulator API doesn't care if suspend is going on, and >> > nor do any of the current drivers for regulators. ?There is an issue >> > with keeping things like I2C alive until the bitter end of suspend so >> > you've got a control bus to the regulators but that's a generic issue >> > which crops up with other subsystems too so a regulator-specific >> > workaround seems dodgy. > >> The end result is that the regulator driver calls the i2c driver after >> the i2c driver has been suspended. ?On Tegra, this happens because of >> voltage scaling on the CPU regulator, which can be on the i2c bus. ?A >> sleep in a suspend handler after the i2c bus has been suspended can >> cause the cpufreq governor to lower the frequency, and try to lower >> the voltage as well. > > OK, that's a general problem - you need to ensure that the I2C and SPI > controllers are suspended really late. ?The OMAP guys also have this > problem for some RTCs, though in their case things are slightly > different due to their use of runtime PM. ?Looking at your I2C driver > (BTW, I'd suggest reminding Ben on a more regular basis about that) I > suspect that moving your suspend and resume callbacks to the _noirq > varaints will cover a lot of this. Even _noirq isn't late enough, if cpufreq keeps trying to change the frequency (and thus voltage) until sysdev suspend. >> The problem isn't limited to i2c busses, there are some regulators on >> spi busses. ?You would need to ensure that any driver that has a > > For all practical purposes in this sort of discussion you can typically > do a s/I2C/I2C and SPI/ - in terms of system integration issues they're > very similar. True >> regulator consumer suspends before the regulator driver it >> communicates with, and that still wouldn't fix cpufreq, which has a >> sysdev suspend handler. > > So you actively need to push the processor into low power mode during > suspend? ?I'm still not 100% clear what's triggering the issues you're > seeing and why this seems to be Tegra-only. ?If there were a general > cpufreq/regulator interaction here I'd expect to see all ARM cpufreq > drivers needing to do the same thing (and probably some handling of this > in cpufreq core as a result). ?If it's not such an issue I'd expect > there's also entertaining suspend ordering issues elsewhere. It's more of a voltage scaling issue than a cpufreq issue directly. Tegra requires the voltage be set to nominal during resume, and the only time it can be set for resume is before I2C suspends. I handle the problem with a suspend notifier in the latest version of the clock voltage scaling patches, but I kept this patch to avoid cpufreq trying to go to frequencies that are not supported by the suspend voltage. > Typically this stuff isn't a problem for regulators themselves since if > they're active by the time I2C is off they normally get suspended by > hardware handshakes from the CPU as that enters low power mode - right > now we have no regulators at all in mainline that do anything in > software on suspend. ?We can get a long way by just ignoring what > happens to regulators over suspend (there is some work to do here but > it's orthogonal to this sort of issue). The regulator driver itself has nothing to do, and when the CPU enters its lowest power mode it signals the regulator to turn off, but something needs to tell the regulator to go to the correct voltage. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 19:52 ` Colin Cross @ 2011-01-24 20:26 ` Mark Brown -1 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 20:26 UTC (permalink / raw) To: Colin Cross Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 11:52:12AM -0800, Colin Cross wrote: > Even _noirq isn't late enough, if cpufreq keeps trying to change the > frequency (and thus voltage) until sysdev suspend. If it were just cpufreq in isolation I'm not sure it'd be a big deal - nothing will go hideously wrong if it's ticking away by itself providing it's just governors working away providing the error handling is OK. However... > On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown > > So you actively need to push the processor into low power mode during > > suspend? I'm still not 100% clear what's triggering the issues you're > It's more of a voltage scaling issue than a cpufreq issue directly. > Tegra requires the voltage be set to nominal during resume, and the > only time it can be set for resume is before I2C suspends. I handle > the problem with a suspend notifier in the latest version of the clock > voltage scaling patches, but I kept this patch to avoid cpufreq trying > to go to frequencies that are not supported by the suspend voltage. ...it sounds like you do actually have something of the above issue - but I think it's a generic-ish problem. See below. > > Typically this stuff isn't a problem for regulators themselves since if > > they're active by the time I2C is off they normally get suspended by > > hardware handshakes from the CPU as that enters low power mode - right > > now we have no regulators at all in mainline that do anything in > > software on suspend. We can get a long way by just ignoring what > > happens to regulators over suspend (there is some work to do here but > > it's orthogonal to this sort of issue). > The regulator driver itself has nothing to do, and when the CPU enters > its lowest power mode it signals the regulator to turn off, but > something needs to tell the regulator to go to the correct voltage. Normally that's handled by the same transition logic - the core PMIC will usually sequence a bunch of voltage and power status changes when entering its own low power mode and have a similar transition on resume which will ensure that the regulators power back up with a sane setup regardless of the state they were in beforehand. It sounds like for your systems what's happening is that the resume is restoring the previously configured voltages for the core rails rather than going to a known good state. Is my understanding correct here? While your fix is good and I don't see any reason not to do it this does sound like something we should have a solution for in common code as I'd not be surprised if there were other hardware out there which did a similar thing. Assuming I'm not completely off base with the above it'd be good if you could clarify this in the commit message - there's enough dragons with this stuff and it's common to refer to other implementations so it'd be nice if we had a clear record of the issue in the log. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 20:26 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 20:26 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 11:52:12AM -0800, Colin Cross wrote: > Even _noirq isn't late enough, if cpufreq keeps trying to change the > frequency (and thus voltage) until sysdev suspend. If it were just cpufreq in isolation I'm not sure it'd be a big deal - nothing will go hideously wrong if it's ticking away by itself providing it's just governors working away providing the error handling is OK. However... > On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown > > So you actively need to push the processor into low power mode during > > suspend? ?I'm still not 100% clear what's triggering the issues you're > It's more of a voltage scaling issue than a cpufreq issue directly. > Tegra requires the voltage be set to nominal during resume, and the > only time it can be set for resume is before I2C suspends. I handle > the problem with a suspend notifier in the latest version of the clock > voltage scaling patches, but I kept this patch to avoid cpufreq trying > to go to frequencies that are not supported by the suspend voltage. ...it sounds like you do actually have something of the above issue - but I think it's a generic-ish problem. See below. > > Typically this stuff isn't a problem for regulators themselves since if > > they're active by the time I2C is off they normally get suspended by > > hardware handshakes from the CPU as that enters low power mode - right > > now we have no regulators at all in mainline that do anything in > > software on suspend. ?We can get a long way by just ignoring what > > happens to regulators over suspend (there is some work to do here but > > it's orthogonal to this sort of issue). > The regulator driver itself has nothing to do, and when the CPU enters > its lowest power mode it signals the regulator to turn off, but > something needs to tell the regulator to go to the correct voltage. Normally that's handled by the same transition logic - the core PMIC will usually sequence a bunch of voltage and power status changes when entering its own low power mode and have a similar transition on resume which will ensure that the regulators power back up with a sane setup regardless of the state they were in beforehand. It sounds like for your systems what's happening is that the resume is restoring the previously configured voltages for the core rails rather than going to a known good state. Is my understanding correct here? While your fix is good and I don't see any reason not to do it this does sound like something we should have a solution for in common code as I'd not be surprised if there were other hardware out there which did a similar thing. Assuming I'm not completely off base with the above it'd be good if you could clarify this in the commit message - there's enough dragons with this stuff and it's common to refer to other implementations so it'd be nice if we had a clear record of the issue in the log. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 20:26 ` Mark Brown @ 2011-01-24 20:52 ` Colin Cross -1 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 20:52 UTC (permalink / raw) To: Mark Brown Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 11:52:12AM -0800, Colin Cross wrote: > >> Even _noirq isn't late enough, if cpufreq keeps trying to change the >> frequency (and thus voltage) until sysdev suspend. > > If it were just cpufreq in isolation I'm not sure it'd be a big deal - > nothing will go hideously wrong if it's ticking away by itself providing > it's just governors working away providing the error handling is OK. > However... > >> On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown > >> > So you actively need to push the processor into low power mode during >> > suspend? I'm still not 100% clear what's triggering the issues you're > >> It's more of a voltage scaling issue than a cpufreq issue directly. >> Tegra requires the voltage be set to nominal during resume, and the >> only time it can be set for resume is before I2C suspends. I handle >> the problem with a suspend notifier in the latest version of the clock >> voltage scaling patches, but I kept this patch to avoid cpufreq trying >> to go to frequencies that are not supported by the suspend voltage. > > ...it sounds like you do actually have something of the above issue - > but I think it's a generic-ish problem. See below. > >> > Typically this stuff isn't a problem for regulators themselves since if >> > they're active by the time I2C is off they normally get suspended by >> > hardware handshakes from the CPU as that enters low power mode - right >> > now we have no regulators at all in mainline that do anything in >> > software on suspend. We can get a long way by just ignoring what >> > happens to regulators over suspend (there is some work to do here but >> > it's orthogonal to this sort of issue). > >> The regulator driver itself has nothing to do, and when the CPU enters >> its lowest power mode it signals the regulator to turn off, but >> something needs to tell the regulator to go to the correct voltage. > > Normally that's handled by the same transition logic - the core PMIC > will usually sequence a bunch of voltage and power status changes when > entering its own low power mode and have a similar transition on resume > which will ensure that the regulators power back up with a sane setup > regardless of the state they were in beforehand. I believe Tegra supports this mode, where the Tegra PMC can master the I2C bus to control an external PMIC during resume, but I've never seen it used. > It sounds like for your systems what's happening is that the resume is > restoring the previously configured voltages for the core rails rather > than going to a known good state. Is my understanding correct here? > While your fix is good and I don't see any reason not to do it this does > sound like something we should have a solution for in common code as I'd > not be surprised if there were other hardware out there which did a > similar thing. Yes - the core can be connected to a dumb I2C regulator, which the core can turn on and off with a request line, but can't control the voltage without a regulator driver controlling the I2C bus. > Assuming I'm not completely off base with the above it'd be good if you > could clarify this in the commit message - there's enough dragons with > this stuff and it's common to refer to other implementations so it'd be > nice if we had a clear record of the issue in the log. Will do ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 20:52 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 20:52 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 11:52:12AM -0800, Colin Cross wrote: > >> Even _noirq isn't late enough, if cpufreq keeps trying to change the >> frequency (and thus voltage) until sysdev suspend. > > If it were just cpufreq in isolation I'm not sure it'd be a big deal - > nothing will go hideously wrong if it's ticking away by itself providing > it's just governors working away providing the error handling is OK. > However... > >> On Mon, Jan 24, 2011 at 11:35 AM, Mark Brown > >> > So you actively need to push the processor into low power mode during >> > suspend? ?I'm still not 100% clear what's triggering the issues you're > >> It's more of a voltage scaling issue than a cpufreq issue directly. >> Tegra requires the voltage be set to nominal during resume, and the >> only time it can be set for resume is before I2C suspends. ?I handle >> the problem with a suspend notifier in the latest version of the clock >> voltage scaling patches, but I kept this patch to avoid cpufreq trying >> to go to frequencies that are not supported by the suspend voltage. > > ...it sounds like you do actually have something of the above issue - > but I think it's a generic-ish problem. ?See below. > >> > Typically this stuff isn't a problem for regulators themselves since if >> > they're active by the time I2C is off they normally get suspended by >> > hardware handshakes from the CPU as that enters low power mode - right >> > now we have no regulators at all in mainline that do anything in >> > software on suspend. ?We can get a long way by just ignoring what >> > happens to regulators over suspend (there is some work to do here but >> > it's orthogonal to this sort of issue). > >> The regulator driver itself has nothing to do, and when the CPU enters >> its lowest power mode it signals the regulator to turn off, but >> something needs to tell the regulator to go to the correct voltage. > > Normally that's handled by the same transition logic - the core PMIC > will usually sequence a bunch of voltage and power status changes when > entering its own low power mode and have a similar transition on resume > which will ensure that the regulators power back up with a sane setup > regardless of the state they were in beforehand. I believe Tegra supports this mode, where the Tegra PMC can master the I2C bus to control an external PMIC during resume, but I've never seen it used. > It sounds like for your systems what's happening is that the resume is > restoring the previously configured voltages for the core rails rather > than going to a known good state. ?Is my understanding correct here? > While your fix is good and I don't see any reason not to do it this does > sound like something we should have a solution for in common code as I'd > not be surprised if there were other hardware out there which did a > similar thing. Yes - the core can be connected to a dumb I2C regulator, which the core can turn on and off with a request line, but can't control the voltage without a regulator driver controlling the I2C bus. > Assuming I'm not completely off base with the above it'd be good if you > could clarify this in the commit message - there's enough dragons with > this stuff and it's common to refer to other implementations so it'd be > nice if we had a clear record of the issue in the log. Will do ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 20:52 ` Colin Cross @ 2011-01-24 21:08 ` Mark Brown -1 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 21:08 UTC (permalink / raw) To: Colin Cross Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 12:52:51PM -0800, Colin Cross wrote: > On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown > > Normally that's handled by the same transition logic - the core PMIC > > will usually sequence a bunch of voltage and power status changes when > > entering its own low power mode and have a similar transition on resume > > which will ensure that the regulators power back up with a sane setup > > regardless of the state they were in beforehand. > I believe Tegra supports this mode, where the Tegra PMC can master the > I2C bus to control an external PMIC during resume, but I've never seen > it used. In almost all cases it's implemented in the PMIC rather than the CPU - it's essentially the same logic as implements the initial power on of the system and the PMIC needs to be capable of starting the system boot without external assistance anyway, including sequencing the power up of the CPU. With most PMICs the settings will be hard coded into the silicon so you really need to get a PMIC specifically designed for whatever CPU you're using (this is one of the reasons you always see the same CPU/PMIC combinations, people are terrified of changing a known good pairing in case they build something that won't power on). Some more modern PMICs make this configurable which gives system designers a lot more flexibility. The PMC stuff is newer and in the designs I've seen is more focused on runtime management than this stuff. > > It sounds like for your systems what's happening is that the resume is > > restoring the previously configured voltages for the core rails rather > > than going to a known good state. Is my understanding correct here? > > While your fix is good and I don't see any reason not to do it this does > > sound like something we should have a solution for in common code as I'd > > not be surprised if there were other hardware out there which did a > > similar thing. > Yes - the core can be connected to a dumb I2C regulator, which the > core can turn on and off with a request line, but can't control the > voltage without a regulator driver controlling the I2C bus. AIUI generally they end up working because whatever puts them into suspend actually resets them so when you release reset during resume they just recover whatever the default voltage they have is and normally the boot and resume conditions for the CPU are identical. I could be off base there, most of the regulator API stuff has been done with integrated PMICs. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 21:08 ` Mark Brown 0 siblings, 0 replies; 23+ messages in thread From: Mark Brown @ 2011-01-24 21:08 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 12:52:51PM -0800, Colin Cross wrote: > On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown > > Normally that's handled by the same transition logic - the core PMIC > > will usually sequence a bunch of voltage and power status changes when > > entering its own low power mode and have a similar transition on resume > > which will ensure that the regulators power back up with a sane setup > > regardless of the state they were in beforehand. > I believe Tegra supports this mode, where the Tegra PMC can master the > I2C bus to control an external PMIC during resume, but I've never seen > it used. In almost all cases it's implemented in the PMIC rather than the CPU - it's essentially the same logic as implements the initial power on of the system and the PMIC needs to be capable of starting the system boot without external assistance anyway, including sequencing the power up of the CPU. With most PMICs the settings will be hard coded into the silicon so you really need to get a PMIC specifically designed for whatever CPU you're using (this is one of the reasons you always see the same CPU/PMIC combinations, people are terrified of changing a known good pairing in case they build something that won't power on). Some more modern PMICs make this configurable which gives system designers a lot more flexibility. The PMC stuff is newer and in the designs I've seen is more focused on runtime management than this stuff. > > It sounds like for your systems what's happening is that the resume is > > restoring the previously configured voltages for the core rails rather > > than going to a known good state. ?Is my understanding correct here? > > While your fix is good and I don't see any reason not to do it this does > > sound like something we should have a solution for in common code as I'd > > not be surprised if there were other hardware out there which did a > > similar thing. > Yes - the core can be connected to a dumb I2C regulator, which the > core can turn on and off with a request line, but can't control the > voltage without a regulator driver controlling the I2C bus. AIUI generally they end up working because whatever puts them into suspend actually resets them so when you release reset during resume they just recover whatever the default voltage they have is and normally the boot and resume conditions for the CPU are identical. I could be off base there, most of the regulator API stuff has been done with integrated PMICs. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 21:08 ` Mark Brown @ 2011-01-24 21:24 ` Colin Cross -1 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 21:24 UTC (permalink / raw) To: Mark Brown Cc: linux-tegra, Russell King, konkers, linux-kernel, olof, linux-arm-kernel On Mon, Jan 24, 2011 at 1:08 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 12:52:51PM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown > >> > Normally that's handled by the same transition logic - the core PMIC >> > will usually sequence a bunch of voltage and power status changes when >> > entering its own low power mode and have a similar transition on resume >> > which will ensure that the regulators power back up with a sane setup >> > regardless of the state they were in beforehand. > >> I believe Tegra supports this mode, where the Tegra PMC can master the >> I2C bus to control an external PMIC during resume, but I've never seen >> it used. > > In almost all cases it's implemented in the PMIC rather than the CPU - > it's essentially the same logic as implements the initial power on of > the system and the PMIC needs to be capable of starting the system boot > without external assistance anyway, including sequencing the power up of > the CPU. > > With most PMICs the settings will be hard coded into the silicon so > you really need to get a PMIC specifically designed for whatever CPU > you're using (this is one of the reasons you always see the same > CPU/PMIC combinations, people are terrified of changing a known good > pairing in case they build something that won't power on). Some more > modern PMICs make this configurable which gives system designers a lot > more flexibility. I'm seeing the opposite problem - changing CPUs, and keeping the PMIC. > The PMC stuff is newer and in the designs I've seen is more focused on > runtime management than this stuff. > >> > It sounds like for your systems what's happening is that the resume is >> > restoring the previously configured voltages for the core rails rather >> > than going to a known good state. Is my understanding correct here? >> > While your fix is good and I don't see any reason not to do it this does >> > sound like something we should have a solution for in common code as I'd >> > not be surprised if there were other hardware out there which did a >> > similar thing. > >> Yes - the core can be connected to a dumb I2C regulator, which the >> core can turn on and off with a request line, but can't control the >> voltage without a regulator driver controlling the I2C bus. > > AIUI generally they end up working because whatever puts them into > suspend actually resets them so when you release reset during resume > they just recover whatever the default voltage they have is and normally > the boot and resume conditions for the CPU are identical. I could be > off base there, most of the regulator API stuff has been done with > integrated PMICs. Resetting during suspend would be the sane thing to do, but that's not what the hardware does. The regulator also gets turned off during idle, so it can't just be reset every time it turns off. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-24 21:24 ` Colin Cross 0 siblings, 0 replies; 23+ messages in thread From: Colin Cross @ 2011-01-24 21:24 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 24, 2011 at 1:08 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 12:52:51PM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 12:26 PM, Mark Brown > >> > Normally that's handled by the same transition logic - the core PMIC >> > will usually sequence a bunch of voltage and power status changes when >> > entering its own low power mode and have a similar transition on resume >> > which will ensure that the regulators power back up with a sane setup >> > regardless of the state they were in beforehand. > >> I believe Tegra supports this mode, where the Tegra PMC can master the >> I2C bus to control an external PMIC during resume, but I've never seen >> it used. > > In almost all cases it's implemented in the PMIC rather than the CPU - > it's essentially the same logic as implements the initial power on of > the system and the PMIC needs to be capable of starting the system boot > without external assistance anyway, including sequencing the power up of > the CPU. > > With most PMICs the settings will be hard coded into the silicon so > you really need to get a PMIC specifically designed for whatever CPU > you're using (this is one of the reasons you always see the same > CPU/PMIC combinations, people are terrified of changing a known good > pairing in case they build something that won't power on). ?Some more > modern PMICs make this configurable which gives system designers a lot > more flexibility. I'm seeing the opposite problem - changing CPUs, and keeping the PMIC. > The PMC stuff is newer and in the designs I've seen is more focused on > runtime management than this stuff. > >> > It sounds like for your systems what's happening is that the resume is >> > restoring the previously configured voltages for the core rails rather >> > than going to a known good state. ?Is my understanding correct here? >> > While your fix is good and I don't see any reason not to do it this does >> > sound like something we should have a solution for in common code as I'd >> > not be surprised if there were other hardware out there which did a >> > similar thing. > >> Yes - the core can be connected to a dumb I2C regulator, which the >> core can turn on and off with a request line, but can't control the >> voltage without a regulator driver controlling the I2C bus. > > AIUI generally they end up working because whatever puts them into > suspend actually resets them so when you release reset during resume > they just recover whatever the default voltage they have is and normally > the boot and resume conditions for the CPU are identical. ?I could be > off base there, most of the regulator API stuff has been done with > integrated PMICs. Resetting during suspend would be the sane thing to do, but that's not what the hardware does. The regulator also gets turned off during idle, so it can't just be reset every time it turns off. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend 2011-01-24 19:35 ` Mark Brown @ 2011-01-25 4:26 ` Kyungmin Park -1 siblings, 0 replies; 23+ messages in thread From: Kyungmin Park @ 2011-01-25 4:26 UTC (permalink / raw) To: Mark Brown Cc: Colin Cross, Russell King, konkers, linux-kernel, olof, linux-tegra, linux-arm-kernel, 함명주 On Tue, Jan 25, 2011 at 4:35 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > >> > Hrm, what's the situation where that happens and why does it cause >> > problems? The regulator API doesn't care if suspend is going on, and >> > nor do any of the current drivers for regulators. There is an issue >> > with keeping things like I2C alive until the bitter end of suspend so >> > you've got a control bus to the regulators but that's a generic issue >> > which crops up with other subsystems too so a regulator-specific >> > workaround seems dodgy. > >> The end result is that the regulator driver calls the i2c driver after >> the i2c driver has been suspended. On Tegra, this happens because of >> voltage scaling on the CPU regulator, which can be on the i2c bus. A >> sleep in a suspend handler after the i2c bus has been suspended can >> cause the cpufreq governor to lower the frequency, and try to lower >> the voltage as well. > > OK, that's a general problem - you need to ensure that the I2C and SPI > controllers are suspended really late. The OMAP guys also have this > problem for some RTCs, though in their case things are slightly > different due to their use of runtime PM. Looking at your I2C driver > (BTW, I'd suggest reminding Ben on a more regular basis about that) I > suspect that moving your suspend and resume callbacks to the _noirq > varaints will cover a lot of this. Right, it's same as Samsung SoCs. we also disable to access the cpufreq when suspending. the PMIC is connected with I2C and I2C is suspended before PMIC. Thank you, Kyungmin Park > >> The problem isn't limited to i2c busses, there are some regulators on >> spi busses. You would need to ensure that any driver that has a > > For all practical purposes in this sort of discussion you can typically > do a s/I2C/I2C and SPI/ - in terms of system integration issues they're > very similar. > >> regulator consumer suspends before the regulator driver it >> communicates with, and that still wouldn't fix cpufreq, which has a >> sysdev suspend handler. > > So you actively need to push the processor into low power mode during > suspend? I'm still not 100% clear what's triggering the issues you're > seeing and why this seems to be Tegra-only. If there were a general > cpufreq/regulator interaction here I'd expect to see all ARM cpufreq > drivers needing to do the same thing (and probably some handling of this > in cpufreq core as a result). If it's not such an issue I'd expect > there's also entertaining suspend ordering issues elsewhere. > > Typically this stuff isn't a problem for regulators themselves since if > they're active by the time I2C is off they normally get suspended by > hardware handshakes from the CPU as that enters low power mode - right > now we have no regulators at all in mainline that do anything in > software on suspend. We can get a long way by just ignoring what > happens to regulators over suspend (there is some work to do here but > it's orthogonal to this sort of issue). > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend @ 2011-01-25 4:26 ` Kyungmin Park 0 siblings, 0 replies; 23+ messages in thread From: Kyungmin Park @ 2011-01-25 4:26 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 25, 2011 at 4:35 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, Jan 24, 2011 at 10:50:53AM -0800, Colin Cross wrote: >> On Mon, Jan 24, 2011 at 6:41 AM, Mark Brown > >> > Hrm, what's the situation where that happens and why does it cause >> > problems? ?The regulator API doesn't care if suspend is going on, and >> > nor do any of the current drivers for regulators. ?There is an issue >> > with keeping things like I2C alive until the bitter end of suspend so >> > you've got a control bus to the regulators but that's a generic issue >> > which crops up with other subsystems too so a regulator-specific >> > workaround seems dodgy. > >> The end result is that the regulator driver calls the i2c driver after >> the i2c driver has been suspended. ?On Tegra, this happens because of >> voltage scaling on the CPU regulator, which can be on the i2c bus. ?A >> sleep in a suspend handler after the i2c bus has been suspended can >> cause the cpufreq governor to lower the frequency, and try to lower >> the voltage as well. > > OK, that's a general problem - you need to ensure that the I2C and SPI > controllers are suspended really late. ?The OMAP guys also have this > problem for some RTCs, though in their case things are slightly > different due to their use of runtime PM. ?Looking at your I2C driver > (BTW, I'd suggest reminding Ben on a more regular basis about that) I > suspect that moving your suspend and resume callbacks to the _noirq > varaints will cover a lot of this. Right, it's same as Samsung SoCs. we also disable to access the cpufreq when suspending. the PMIC is connected with I2C and I2C is suspended before PMIC. Thank you, Kyungmin Park > >> The problem isn't limited to i2c busses, there are some regulators on >> spi busses. ?You would need to ensure that any driver that has a > > For all practical purposes in this sort of discussion you can typically > do a s/I2C/I2C and SPI/ - in terms of system integration issues they're > very similar. > >> regulator consumer suspends before the regulator driver it >> communicates with, and that still wouldn't fix cpufreq, which has a >> sysdev suspend handler. > > So you actively need to push the processor into low power mode during > suspend? ?I'm still not 100% clear what's triggering the issues you're > seeing and why this seems to be Tegra-only. ?If there were a general > cpufreq/regulator interaction here I'd expect to see all ARM cpufreq > drivers needing to do the same thing (and probably some handling of this > in cpufreq core as a result). ?If it's not such an issue I'd expect > there's also entertaining suspend ordering issues elsewhere. > > Typically this stuff isn't a problem for regulators themselves since if > they're active by the time I2C is off they normally get suspended by > hardware handshakes from the CPU as that enters low power mode - right > now we have no regulators at all in mainline that do anything in > software on suspend. ?We can get a long way by just ignoring what > happens to regulators over suspend (there is some work to do here but > it's orthogonal to this sort of issue). > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2011-01-25 12:40 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-25 10:56 [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend MyungJoo Ham 2011-01-25 12:40 ` Mark Brown 2011-01-25 12:40 ` Mark Brown -- strict thread matches above, loose matches on Subject: below -- 2011-01-24 2:01 [PATCH v2 00/28] Updates for Tegra support in 2.6.39 Colin Cross 2011-01-24 2:01 ` [PATCH v2 20/28] ARM: tegra: cpufreq: Disable cpufreq during suspend Colin Cross 2011-01-24 2:01 ` Colin Cross 2011-01-24 14:41 ` Mark Brown 2011-01-24 14:41 ` Mark Brown 2011-01-24 18:50 ` Colin Cross 2011-01-24 18:50 ` Colin Cross 2011-01-24 19:35 ` Mark Brown 2011-01-24 19:35 ` Mark Brown 2011-01-24 19:52 ` Colin Cross 2011-01-24 19:52 ` Colin Cross 2011-01-24 20:26 ` Mark Brown 2011-01-24 20:26 ` Mark Brown 2011-01-24 20:52 ` Colin Cross 2011-01-24 20:52 ` Colin Cross 2011-01-24 21:08 ` Mark Brown 2011-01-24 21:08 ` Mark Brown 2011-01-24 21:24 ` Colin Cross 2011-01-24 21:24 ` Colin Cross 2011-01-25 4:26 ` Kyungmin Park 2011-01-25 4:26 ` Kyungmin Park
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.