[PATCH 1/4] AM3517 : support for suspend/resume
1. Patch to disable dynamic sleep (as it is not supported
on AM35xx).
2. Imported the unique suspend/resume sequence for AM3517,
contained in the new file arch/arm/mach-omap2/sleep3517.S.
3. Added omap3517_ to symbol-names in sleep3517.S which are common
with sleep34xx.S, and added appropriate checks.
There are still 3 caveats:
1. If "no_console_suspend" is enabled (via boot-args), the device
doesnot resume but simply hangs.
2. Every second and subsequent attempt to suspend/resume prints this slow-path
WARNING (for both uart1 and uart2), while resuming :
[ 70.943939] omap_hwmod: uart1: idle state can only be entered from
enabled state
3. Wakeup using the TSC2004 touch-screen controller is not supported.
Signed-off-by: Ranjith Lohithakshan
Reviewed-by: Vaibhav Hiremath
Signed-off-by: Abhilash K V
---
arch/arm/mach-omap2/Makefile|2 +-
arch/arm/mach-omap2/control.c |7 ++-
arch/arm/mach-omap2/control.h |1 +
arch/arm/mach-omap2/pm.h|4 +
arch/arm/mach-omap2/pm34xx.c| 18 -
arch/arm/mach-omap2/sleep3517.S | 144 +++
6 files changed, 170 insertions(+), 6 deletions(-)
create mode 100644 arch/arm/mach-omap2/sleep3517.S
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 46f5fbc..3fdf086 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -61,7 +61,7 @@ endif
ifeq ($(CONFIG_PM),y)
obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o
obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o
-obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o \
+obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o sleep3517.o \
cpuidle34xx.o
obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o
obj-$(CONFIG_PM_DEBUG) += pm-debug.o
diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index da53ba3..7d2d8a8 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -284,10 +284,13 @@ void omap3_save_scratchpad_contents(void)
* The restore pointer is stored into the scratchpad.
*/
scratchpad_contents.boot_config_ptr = 0x0;
- if (cpu_is_omap3630())
+ if (cpu_is_omap3505() || cpu_is_omap3517()) {
+ scratchpad_contents.public_restore_ptr =
+ virt_to_phys(omap3517_get_restore_pointer());
+ } else if (cpu_is_omap3630()) {
scratchpad_contents.public_restore_ptr =
virt_to_phys(get_omap3630_restore_pointer());
- else if (omap_rev() != OMAP3430_REV_ES3_0 &&
+ } else if (omap_rev() != OMAP3430_REV_ES3_0 &&
omap_rev() != OMAP3430_REV_ES3_1)
scratchpad_contents.public_restore_ptr =
virt_to_phys(get_restore_pointer());
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index ad024df..3003940 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -389,6 +389,7 @@ extern void omap4_ctrl_pad_writel(u32 val, u16 offset);
extern void omap3_save_scratchpad_contents(void);
extern void omap3_clear_scratchpad_contents(void);
extern u32 *get_restore_pointer(void);
+extern u32 *omap3517_get_restore_pointer(void);
extern u32 *get_es3_restore_pointer(void);
extern u32 *get_omap3630_restore_pointer(void);
extern u32 omap3_arm_context[128];
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index 5c2bd2f..d773e07 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -77,13 +77,17 @@ extern void omap24xx_idle_loop_suspend(void);
extern void omap24xx_cpu_suspend(u32 dll_ctrl, void __iomem *sdrc_dlla_ctrl,
void __iomem *sdrc_power);
extern void omap34xx_cpu_suspend(u32 *addr, int save_state);
+extern void omap3517_cpu_suspend(u32 *addr, int save_state);
extern int save_secure_ram_context(u32 *addr);
+extern void omap3517_save_secure_ram_context(u32 *addr);
extern void omap3_save_scratchpad_contents(void);
extern unsigned int omap24xx_idle_loop_suspend_sz;
extern unsigned int save_secure_ram_context_sz;
+extern unsigned int omap3517_save_secure_ram_context_sz;
extern unsigned int omap24xx_cpu_suspend_sz;
extern unsigned int omap34xx_cpu_suspend_sz;
+extern unsigned int omap3517_cpu_suspend_sz;
#define PM_RTA_ERRATUM_i608(1 << 0)
#define PM_SDRC_WAKEUP_ERRATUM_i583(1 << 1)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 96a7624..12af5b9 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -497,6 +497,8 @@ console_still_active:
int omap3_can_sleep(void)
{
+ if (cpu_is_omap3505() || cpu_is_omap3517())
+ return 0;
if (!omap_uart_can_sleep())
return 0;
return 1;
@@ -848,11 +850,21 @@ static int __init clkdms_setu
Re: [PATCH 1/4] AM3517 : support for suspend/resume
On Fri, Aug 19, 2011 at 05:25:24PM +0530, Abhilash K V wrote: > 1. Patch to disable dynamic sleep (as it is not supported >on AM35xx). > 2. Imported the unique suspend/resume sequence for AM3517, >contained in the new file arch/arm/mach-omap2/sleep3517.S. > 3. Added omap3517_ to symbol-names in sleep3517.S which are common >with sleep34xx.S, and added appropriate checks. You don't say what kernel these are against, it looks like it is not the latest mainline kernel, as I cleaned up OMAP3 suspend/resume significantly recently. I think you need to update these patches against the latest kernel and repost them. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] AM3517 : support for suspend/resume
Abhilash K V writes:
> 1. Patch to disable dynamic sleep (as it is not supported
>on AM35xx).
> 2. Imported the unique suspend/resume sequence for AM3517,
>contained in the new file arch/arm/mach-omap2/sleep3517.S.
> 3. Added omap3517_ to symbol-names in sleep3517.S which are common
>with sleep34xx.S, and added appropriate checks.
>
> There are still 3 caveats:
>
> 1. If "no_console_suspend" is enabled (via boot-args), the device
>doesnot resume but simply hangs.
> 2. Every second and subsequent attempt to suspend/resume prints this slow-path
>WARNING (for both uart1 and uart2), while resuming :
>[ 70.943939] omap_hwmod: uart1: idle state can only be entered from
>enabled state
> 3. Wakeup using the TSC2004 touch-screen controller is not supported.
>
> Signed-off-by: Ranjith Lohithakshan
> Reviewed-by: Vaibhav Hiremath
> Signed-off-by: Abhilash K V
In addition to Russell's comments about using the latest code from
mainline, I have some comments below.
> ---
> arch/arm/mach-omap2/Makefile|2 +-
> arch/arm/mach-omap2/control.c |7 ++-
> arch/arm/mach-omap2/control.h |1 +
> arch/arm/mach-omap2/pm.h|4 +
> arch/arm/mach-omap2/pm34xx.c| 18 -
> arch/arm/mach-omap2/sleep3517.S | 144
> +++
> 6 files changed, 170 insertions(+), 6 deletions(-)
> create mode 100644 arch/arm/mach-omap2/sleep3517.S
>
> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
> index 46f5fbc..3fdf086 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -61,7 +61,7 @@ endif
> ifeq ($(CONFIG_PM),y)
> obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o
> obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o
> -obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o \
> +obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o sleep3517.o \
> cpuidle34xx.o
> obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o
> obj-$(CONFIG_PM_DEBUG) += pm-debug.o
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index da53ba3..7d2d8a8 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -284,10 +284,13 @@ void omap3_save_scratchpad_contents(void)
>* The restore pointer is stored into the scratchpad.
>*/
> scratchpad_contents.boot_config_ptr = 0x0;
> - if (cpu_is_omap3630())
> + if (cpu_is_omap3505() || cpu_is_omap3517()) {
> + scratchpad_contents.public_restore_ptr =
> + virt_to_phys(omap3517_get_restore_pointer());
Based on the contents of the get_restore_pointer() added below, this
value is obviously not being used. Either off-mode was not tested (or not
supported.) Either way, unused code like this should not be
added if it is not tested or supported.
> + } else if (cpu_is_omap3630()) {
> scratchpad_contents.public_restore_ptr =
> virt_to_phys(get_omap3630_restore_pointer());
> - else if (omap_rev() != OMAP3430_REV_ES3_0 &&
> + } else if (omap_rev() != OMAP3430_REV_ES3_0 &&
> omap_rev() != OMAP3430_REV_ES3_1)
> scratchpad_contents.public_restore_ptr =
> virt_to_phys(get_restore_pointer());
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index ad024df..3003940 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -389,6 +389,7 @@ extern void omap4_ctrl_pad_writel(u32 val, u16 offset);
> extern void omap3_save_scratchpad_contents(void);
> extern void omap3_clear_scratchpad_contents(void);
> extern u32 *get_restore_pointer(void);
> +extern u32 *omap3517_get_restore_pointer(void);
> extern u32 *get_es3_restore_pointer(void);
> extern u32 *get_omap3630_restore_pointer(void);
> extern u32 omap3_arm_context[128];
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 5c2bd2f..d773e07 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -77,13 +77,17 @@ extern void omap24xx_idle_loop_suspend(void);
> extern void omap24xx_cpu_suspend(u32 dll_ctrl, void __iomem *sdrc_dlla_ctrl,
> void __iomem *sdrc_power);
> extern void omap34xx_cpu_suspend(u32 *addr, int save_state);
> +extern void omap3517_cpu_suspend(u32 *addr, int save_state);
> extern int save_secure_ram_context(u32 *addr);
> +extern void omap3517_save_secure_ram_context(u32 *addr);
> extern void omap3_save_scratchpad_contents(void);
>
> extern unsigned int omap24xx_idle_loop_suspend_sz;
> extern unsigned int save_secure_ram_context_sz;
> +extern unsigned int omap3517_save_secure_ram_context_sz;
> extern unsigned int omap24xx_cpu_suspend_sz;
> extern unsigned int omap34xx_cpu_suspend_sz;
> +extern unsigned int omap3517_cpu_suspend_sz;
>
> #define PM_RTA_ERRATU
RE: [PATCH 1/4] AM3517 : support for suspend/resume
Hi
On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>
> Abhilash K V writes:
>
>> 1. Patch to disable dynamic sleep (as it is not supported
>>on AM35xx).
>> 2. Imported the unique suspend/resume sequence for AM3517,
>>contained in the new file arch/arm/mach-omap2/sleep3517.S.
>> 3. Added omap3517_ to symbol-names in sleep3517.S which are common
>>with sleep34xx.S, and added appropriate checks.
>>
>> There are still 3 caveats:
>>
>> 1. If "no_console_suspend" is enabled (via boot-args), the device
>>doesnot resume but simply hangs.
>> 2. Every second and subsequent attempt to suspend/resume prints this
>> slow-path
>>WARNING (for both uart1 and uart2), while resuming :
>>[ 70.943939] omap_hwmod: uart1: idle state can only be entered from
>>enabled state
>> 3. Wakeup using the TSC2004 touch-screen controller is not supported.
>>
>> Signed-off-by: Ranjith Lohithakshan
>> Reviewed-by: Vaibhav Hiremath
>> Signed-off-by: Abhilash K V
>
> In addition to Russell's comments about using the latest code from
> mainline, I have some comments below.
[Abhilash K V] I have reworked the patch against the tip (as suggested by
Russell).
And I've incorporated all of Kevin's comments too.
There is one "known" issue left which needs to be closed before I can submit v2
of this patch.
With no_console_suspend, suspend to RAM hangs right now on AM3517, after
the message:
Disabling non-boot CPUs ...
There is no error message or dump.
I found that this crash is happening in a call to pr_warning(), from
_omap_device_deactivate().
The same code does not produce this issue on omap34xx due to this snippet from
omap_sram_idle() :
/* PER */
if (per_next_state < PWRDM_POWER_ON) {
per_going_off = (per_next_state == PWRDM_POWER_OFF) ? 1 : 0;
omap_uart_prepare_idle(2);
omap_uart_prepare_idle(3);
omap2_gpio_prepare_for_idle(per_going_off);
if (per_next_state == PWRDM_POWER_OFF)
omap3_per_save_context();
}
/* CORE */
if (core_next_state < PWRDM_POWER_ON) {
omap_uart_prepare_idle(0);
omap_uart_prepare_idle(1);
if (core_next_state == PWRDM_POWER_OFF) {
omap3_core_save_context();
omap3_cm_save_context();
}
}
This happens in preparation to the suspend operation (I,e. the WFI).
As seen here, on 34xx the sequence in which the uarts are disabled is 2, 3, 0
and 1.The console-uart, which is uart-1 here (starting from uart-0) is disabled
last.
For AM3517 EVM, the console-uart is uart-2 and this ought to be disabled at the
last to prevent this crash from occurring.
So my suggestion is to use a console_uart flag to store the appropriate uart
no. for that platform rather than hard-code it and to ensure that this uart is
disabled at the last.
Another point that complicates matters is that uart-1 and uart-2 are in
different power domains (CORE and PER respectively) - so that would amount to
using the console-uart
no. to decide whether CORE or PER power-domains are disabled first.
Would this have any side-effects?
Is there a better way to go?
-Abhilash
>
>
>> ---
>> arch/arm/mach-omap2/Makefile|2 +-
>> arch/arm/mach-omap2/control.c |7 ++-
>> arch/arm/mach-omap2/control.h |1 +
>> arch/arm/mach-omap2/pm.h|4 +
>> arch/arm/mach-omap2/pm34xx.c| 18 -
>> arch/arm/mach-omap2/sleep3517.S | 144
>> +++
>> 6 files changed, 170 insertions(+), 6 deletions(-)
>> create mode 100644 arch/arm/mach-omap2/sleep3517.S
>>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 46f5fbc..3fdf086 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>> @@ -61,7 +61,7 @@ endif
>> ifeq ($(CONFIG_PM),y)
>> obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o
>> obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o
>> -obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o \
>> +obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o sleep3517.o \
>> cpuidle34xx.o
>> obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o
>> obj-$(CONFIG_PM_DEBUG) += pm-debug.o
>> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
>> index da53ba3..7d2d8a8 100644
>> --- a/arch/arm/mach-omap2/control.c
>> +++ b/arch/arm/mach-omap2/control.c
>> @@ -284,10 +284,13 @@ void omap3_save_scratchpad_contents(void)
>>* The restore pointer is stored into the scratchpad.
>>*/
>> scratchpad_contents.boot_config_ptr = 0x0;
>> - if (cpu_is_omap3630())
>> + if (cpu_is_omap3505() || cpu_is_omap3517()) {
>> + scratchpad_contents.public_restore_ptr =
>> + virt_to_phys(omap3517_get_restore_pointer());
>
Re: [PATCH 1/4] AM3517 : support for suspend/resume
Hi Abhilash,
"Koyamangalath, Abhilash" writes:
> Hi
>
> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>
>> Abhilash K V writes:
>>
>>> 1. Patch to disable dynamic sleep (as it is not supported
>>>on AM35xx).
>>> 2. Imported the unique suspend/resume sequence for AM3517,
>>>contained in the new file arch/arm/mach-omap2/sleep3517.S.
>>> 3. Added omap3517_ to symbol-names in sleep3517.S which are common
>>>with sleep34xx.S, and added appropriate checks.
>>>
>>> There are still 3 caveats:
>>>
>>> 1. If "no_console_suspend" is enabled (via boot-args), the device
>>>doesnot resume but simply hangs.
>>> 2. Every second and subsequent attempt to suspend/resume prints this
>>> slow-path
>>>WARNING (for both uart1 and uart2), while resuming :
>>>[ 70.943939] omap_hwmod: uart1: idle state can only be entered from
>>>enabled state
>>> 3. Wakeup using the TSC2004 touch-screen controller is not supported.
>>>
>>> Signed-off-by: Ranjith Lohithakshan
>>> Reviewed-by: Vaibhav Hiremath
>>> Signed-off-by: Abhilash K V
>>
>> In addition to Russell's comments about using the latest code from
>> mainline, I have some comments below.
> [Abhilash K V] I have reworked the patch against the tip (as suggested by
> Russell).
> And I've incorporated all of Kevin's comments too.
Great, thanks!
> There is one "known" issue left which needs to be closed before I can submit
> v2 of this patch.
> With no_console_suspend, suspend to RAM hangs right now on AM3517, after
> the message:
> Disabling non-boot CPUs ...
> There is no error message or dump.
> I found that this crash is happening in a call to pr_warning(), from
> _omap_device_deactivate().
> The same code does not produce this issue on omap34xx due to this snippet
> from omap_sram_idle() :
> /* PER */
> if (per_next_state < PWRDM_POWER_ON) {
> per_going_off = (per_next_state == PWRDM_POWER_OFF) ? 1 : 0;
> omap_uart_prepare_idle(2);
> omap_uart_prepare_idle(3);
> omap2_gpio_prepare_for_idle(per_going_off);
> if (per_next_state == PWRDM_POWER_OFF)
> omap3_per_save_context();
> }
> /* CORE */
> if (core_next_state < PWRDM_POWER_ON) {
> omap_uart_prepare_idle(0);
> omap_uart_prepare_idle(1);
> if (core_next_state == PWRDM_POWER_OFF) {
> omap3_core_save_context();
> omap3_cm_save_context();
> }
> }
> This happens in preparation to the suspend operation (I,e. the WFI).
> As seen here, on 34xx the sequence in which the uarts are disabled is 2, 3, 0
> and 1.The console-uart, which is uart-1 here (starting from uart-0) is
> disabled last.
> For AM3517 EVM, the console-uart is uart-2 and this ought to be
> disabled at the last to prevent this crash from occurring.
There are several other OMAP3 platforms (n900, Beagle, etc.) where the
UART console is also UART2, so console ordering is not the problem.
The fact that that pr_warning is making it to the console suggests that
the console is not locked. In the idle path, we take the console lock
(using console_trylock(), just above the code you showed above.)
But during suspend, there was an assumption (by me[2]) that the console
would always be locked in the suspend path. During no_console_suspend,
it appears that is not the case.
Can you try the patch below[1] to see if that fixes your problem? I
think it should.
Kevin
[1]
>From 5b5a73101fcfa042d53828c017ee3149eae44b50 Mon Sep 17 00:00:00 2001
From: Kevin Hilman
Date: Tue, 13 Sep 2011 11:18:44 -0700
Subject: [PATCH] OMAP3: PM: fix UART handling when using no_console_suspend
During the idle/suspend path, we expect the console lock to be held so
that no console output is done during/after the UARTs are idled.
However, when using the no_console_suspend argument on the
command-line, the console driver does not take the console lock. This
allows the possibility of console activity after UARTs have been
disabled.
To fix, update the current is_suspending() to also check the
console_suspend_enabled flag.
NOTE: this is short-term workaround until the OMAP serial driver
is fully converted to use runtime PM.
Signed-off-by: Kevin Hilman
---
arch/arm/mach-omap2/pm34xx.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 7255d9b..c8cbd00 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -55,7 +55,7 @@
static suspend_state_t suspend_state = PM_SUSPEND_ON;
static inline bool is_suspending(void)
{
- return (suspend_state != PM_SUSPEND_ON);
+ return (suspend_state != PM_SUSPEND_ON) && console_suspend_enabled;
}
#else
static inline bool is_suspending(void)
--
1.7.6
[2]
commit e83df17f178360a8e7874441bca04a710c869e42
Autho
RE: [PATCH 1/4] AM3517 : support for suspend/resume
hi Kevin
On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
> Hi Abhilash,
>
> "Koyamangalath, Abhilash" writes:
>
>> Hi
>>
>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>
>>> Abhilash K V writes:
>>>
>>>
>>> In addition to Russell's comments about using the latest code from
>>> mainline, I have some comments below.
>> [Abhilash K V] I have reworked the patch against the tip (as suggested by
>> Russell).
>> And I've incorporated all of Kevin's comments too.
>
> Great, thanks!
>
>> There is one "known" issue left which needs to be closed before I can submit
>> v2 of this patch.
>> With no_console_suspend, suspend to RAM hangs right now on AM3517, after
>> the message:
>> Disabling non-boot CPUs ...
>> There is no error message or dump.
>> I found that this crash is happening in a call to pr_warning(), from
>> _omap_device_deactivate().
>> The same code does not produce this issue on omap34xx due to this snippet
>> from omap_sram_idle() :
>> /* PER */
>> if (per_next_state < PWRDM_POWER_ON) {
>> per_going_off = (per_next_state == PWRDM_POWER_OFF) ? 1 : 0;
>> omap_uart_prepare_idle(2);
>> omap_uart_prepare_idle(3);
>> omap2_gpio_prepare_for_idle(per_going_off);
>> if (per_next_state == PWRDM_POWER_OFF)
>> omap3_per_save_context();
>> }
>> /* CORE */
>> if (core_next_state < PWRDM_POWER_ON) {
>> omap_uart_prepare_idle(0);
>> omap_uart_prepare_idle(1);
>> if (core_next_state == PWRDM_POWER_OFF) {
>> omap3_core_save_context();
>> omap3_cm_save_context();
>> }
>> }
>> This happens in preparation to the suspend operation (I,e. the WFI).
>> As seen here, on 34xx the sequence in which the uarts are disabled is 2, 3,
>> 0 and 1.The console-uart, which is uart-1 here (starting from uart-0) is
>> disabled last.
>
>> For AM3517 EVM, the console-uart is uart-2 and this ought to be
>> disabled at the last to prevent this crash from occurring.
>
> There are several other OMAP3 platforms (n900, Beagle, etc.) where the
> UART console is also UART2, so console ordering is not the problem.
[Abhilash K V] OK. Yet changing the order so that uart-2 is disabled at the
last
seems to rid me of the crash on AM35x.
I understand that holding the console_sem before suspend (your fix below) is the
way to go, but I'm just curious over what's happening here.
Simply put what is the rationale behind choosing this order of UART-disables ?
>
> The fact that that pr_warning is making it to the console suggests that
> the console is not locked. In the idle path, we take the console lock
> (using console_trylock(), just above the code you showed above.)
>
> But during suspend, there was an assumption (by me[2]) that the console
> would always be locked in the suspend path. During no_console_suspend,
> it appears that is not the case.
>
> Can you try the patch below[1] to see if that fixes your problem? I
> think it should.
[Abhilash K V] Yes it does, thanks.
>
> Kevin
>
>
> [1]
> From 5b5a73101fcfa042d53828c017ee3149eae44b50 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman
> Date: Tue, 13 Sep 2011 11:18:44 -0700
> Subject: [PATCH] OMAP3: PM: fix UART handling when using no_console_suspend
>
> During the idle/suspend path, we expect the console lock to be held so
> that no console output is done during/after the UARTs are idled.
>
> However, when using the no_console_suspend argument on the
> command-line, the console driver does not take the console lock. This
> allows the possibility of console activity after UARTs have been
> disabled.
>
> To fix, update the current is_suspending() to also check the
> console_suspend_enabled flag.
>
> NOTE: this is short-term workaround until the OMAP serial driver
> is fully converted to use runtime PM.
>
> Signed-off-by: Kevin Hilman
> ---
> arch/arm/mach-omap2/pm34xx.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 7255d9b..c8cbd00 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -55,7 +55,7 @@
> static suspend_state_t suspend_state = PM_SUSPEND_ON;
> static inline bool is_suspending(void)
> {
> - return (suspend_state != PM_SUSPEND_ON);
> + return (suspend_state != PM_SUSPEND_ON) && console_suspend_enabled;
> }
> #else
> static inline bool is_suspending(void)
> --
> 1.7.6
>
>
>
> [2]
> commit e83df17f178360a8e7874441bca04a710c869e42
> Author: Kevin Hilman
> Date: Wed Dec 8 22:40:40 2010 +
>
>OMAP2+: PM/serial: fix console semaphore acquire during suspend
>
>commit 0d8e2d0dad98a693bad88aea6876ac8b94ad95c6 (OMAP2+: PM/serial:
>hold console semaphore while OMAP UARTs are disabled) added use of the
>console semaphore to
Re: [PATCH 1/4] AM3517 : support for suspend/resume
"Koyamangalath, Abhilash" writes:
> hi Kevin
>
> On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
>> Hi Abhilash,
>>
>> "Koyamangalath, Abhilash" writes:
>>
>>> Hi
>>>
>>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
Abhilash K V writes:
>
In addition to Russell's comments about using the latest code from
mainline, I have some comments below.
>>> [Abhilash K V] I have reworked the patch against the tip (as suggested by
>>> Russell).
>>> And I've incorporated all of Kevin's comments too.
>>
>> Great, thanks!
>>
>>> There is one "known" issue left which needs to be closed before I can
>>> submit v2 of this patch.
>>> With no_console_suspend, suspend to RAM hangs right now on AM3517, after
>>> the message:
>>> Disabling non-boot CPUs ...
>>> There is no error message or dump.
>>> I found that this crash is happening in a call to pr_warning(), from
>>> _omap_device_deactivate().
>>> The same code does not produce this issue on omap34xx due to this snippet
>>> from omap_sram_idle() :
>>> /* PER */
>>> if (per_next_state < PWRDM_POWER_ON) {
>>> per_going_off = (per_next_state == PWRDM_POWER_OFF) ? 1 : 0;
>>> omap_uart_prepare_idle(2);
>>> omap_uart_prepare_idle(3);
>>> omap2_gpio_prepare_for_idle(per_going_off);
>>> if (per_next_state == PWRDM_POWER_OFF)
>>> omap3_per_save_context();
>>> }
>>> /* CORE */
>>> if (core_next_state < PWRDM_POWER_ON) {
>>> omap_uart_prepare_idle(0);
>>> omap_uart_prepare_idle(1);
>>> if (core_next_state == PWRDM_POWER_OFF) {
>>> omap3_core_save_context();
>>> omap3_cm_save_context();
>>> }
>>> }
>>> This happens in preparation to the suspend operation (I,e. the WFI).
>>> As seen here, on 34xx the sequence in which the uarts are disabled is 2, 3,
>>> 0 and 1.The console-uart, which is uart-1 here (starting from uart-0) is
>>> disabled last.
>>
>>> For AM3517 EVM, the console-uart is uart-2 and this ought to be
>>> disabled at the last to prevent this crash from occurring.
>>
>> There are several other OMAP3 platforms (n900, Beagle, etc.) where the
>> UART console is also UART2, so console ordering is not the problem.
>
> [Abhilash K V] OK. Yet changing the order so that uart-2 is disabled
> at the last seems to rid me of the crash on AM35x. I understand that
> holding the console_sem before suspend (your fix below) is the way to
> go, but I'm just curious over what's happening here.
You're unlucky. :)
Each time an omap_device is disabled, we print a message if it's a new
worst case value.
What's happening to you is that one of the UARTs disabled after the
console is getting a new worst case value. Because of the missing
console locking, it's trying to write to the console, and boom.
> Simply put what is the rationale behind choosing this order of
> UART-disables ?
The order is not important at all.
Changing the order might fix this problem for your current situation,
but the same problem could pop up elsewhere.
In fact, even if you disable the console UART last, we can still try to
print the "new worst case" messages after the console is disabled.
That's why the console locking is important.
>>
>> The fact that that pr_warning is making it to the console suggests that
>> the console is not locked. In the idle path, we take the console lock
>> (using console_trylock(), just above the code you showed above.)
>>
>> But during suspend, there was an assumption (by me[2]) that the console
>> would always be locked in the suspend path. During no_console_suspend,
>> it appears that is not the case.
>>
>> Can you try the patch below[1] to see if that fixes your problem? I
>> think it should.
> [Abhilash K V] Yes it does, thanks.
Great, thanks for testing. I'll add a Tested-by for you, and post it
to the list.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
