* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-arm-kernel
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 <ranjithl@ti.com>
Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
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_setup(struct clockdomain *clkdm, void *unused)
void omap_push_sram_idle(void)
{
- _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
+ if (cpu_is_omap3505() || cpu_is_omap3517())
+ _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
+ omap3517_cpu_suspend_sz);
+ else
+ _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
if (omap_type() != OMAP2_DEVICE_TYPE_GP)
- _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
- save_secure_ram_context_sz);
+ if (cpu_is_omap3505() || cpu_is_omap3517())
+ _omap_save_secure_sram = omap_sram_push(
+ omap3517_save_secure_ram_context,
+ omap3517_save_secure_ram_context_sz);
+ else
+ _omap_save_secure_sram = omap_sram_push(
+ save_secure_ram_context,
+ save_secure_ram_context_sz);
}
static void __init pm_errata_configure(void)
diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
new file mode 100644
index 0000000..3fceefc
--- /dev/null
+++ b/arch/arm/mach-omap2/sleep3517.S
@@ -0,0 +1,144 @@
+/*
+/* linux/arch/arm/mach-omap2/sleep3517.S
+ *
+ * AM3505/3517 Sleep Code.
+ * Ranjith Lohithakshan <ranjithl@ti.com>
+ *
+ * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+#include <plat/sram.h>
+#include <mach/io.h>
+
+#include "cm2xxx_3xxx.h"
+#include "prm2xxx_3xxx.h"
+#include "sdrc.h"
+#include "control.h"
+
+#define CM_IDLEST1_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
+#define CM_CLKST_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, OMAP3430_CM_CLKSTST)
+#define CM_ICLKEN1_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)
+
+#define EMIF_PM_CTR_V OMAP2_L3_IO_ADDRESS(0x6D000038)
+#define OMAP3517_CONF1_REG_V OMAP2_L4_IO_ADDRESS(0x48002584)
+
+/*
+ * Forces OMAP into idle state
+ *
+ * omap34xx_suspend() - This bit of code just executes the WFI
+ * for normal idles.
+ *
+ * Note: This code get's copied to internal SRAM at boot. When the OMAP
+ * wakes up it continues execution at the point it went to sleep.
+ */
+ENTRY(omap3517_cpu_suspend)
+ stmfd sp!, {r0-r12, lr} @ save registers on stack
+loop:
+ /*b loop*/ @Enable to debug by stepping through code
+
+ /* Put EMIF in self-refresh */
+ ldr r4, emif_pm_ctrl
+ ldr r5, [r4]
+ orr r5, r5, #0x200
+ str r5, [r4]
+
+ /* Disable SDRC and Control Module */
+ ldr r4, cm_iclken1_core
+ ldr r5, clk_core_disable
+ str r5, [r4]
+wait_sdrc_ok:
+ ldr r4, cm_idlest1_core
+ ldr r5, [r4]
+ and r5, r5, #0x2
+ cmp r5, #0x2
+ bne wait_sdrc_ok
+
+ /* Gate DDR Phy clock */
+ ldr r4, omap3517_conf1
+ ldr r5, emif_phy_gate
+ str r5, [r4]
+
+ /* Data memory barrier and Data sync barrier */
+ mov r1, #0
+ mcr p15, 0, r1, c7, c10, 4
+ mcr p15, 0, r1, c7, c10, 5
+
+ wfi
+
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+
+ /* Enable SDRC and Control Module */
+ ldr r4, cm_iclken1_core
+ ldr r5, iclk_core_enable
+ str r5, [r4]
+
+ /* Enable DDR Phy Clock */
+ ldr r4, omap3517_conf1
+ ldr r5, emif_phy_enable
+ str r5, [r4]
+
+ /* Take EMIF out of self-refresh */
+ ldr r4, emif_pm_ctrl
+ ldr r5, [r4]
+ bic r5, r5, #0x200
+ str r5, [r4]
+
+ ldmfd sp!, {r0-r12, pc} @ restore regs and return
+
+clk_core_disable:
+ .word 0x0
+iclk_core_enable:
+ .word 0x42
+emif_phy_gate:
+ .word 0x2620
+emif_phy_enable:
+ .word 0x8620
+cm_idlest1_core:
+ .word CM_IDLEST1_CORE_V
+cm_clkst_core:
+ .word CM_CLKST_CORE_V
+emif_pm_ctrl:
+ .word EMIF_PM_CTR_V
+cm_iclken1_core:
+ .word CM_ICLKEN1_CORE_V
+omap3517_conf1:
+ .word OMAP3517_CONF1_REG_V
+ENTRY(omap3517_cpu_suspend_sz)
+ .word . - omap3517_cpu_suspend
+
+/* Function to call rom code to save secure ram context */
+ENTRY(omap3517_save_secure_ram_context)
+ stmfd sp!, {r1-r12, lr} @ save registers on stack
+save_secure_ram_debug:
+ /* b save_secure_ram_debug */ @ enable to debug save code
+ ldmfd sp!, {r1-r12, pc}
+ENTRY(omap3517_save_secure_ram_context_sz)
+ .word . - omap3517_save_secure_ram_context
+
+/* Function call to get the restore pointer for resume from OFF */
+ENTRY(omap3517_get_restore_pointer)
+ stmfd sp!, {lr} @ save registers on stack
+ ldmfd sp!, {pc} @ restore regs and return
+ENTRY(omap3517_get_restore_pointer_sz)
+ .word . - omap3517_get_restore_pointer
+
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Abhilash K V, Ranjith Lohithakshan
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 <ranjithl@ti.com>
Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
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_setup(struct clockdomain *clkdm, void *unused)
void omap_push_sram_idle(void)
{
- _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
+ if (cpu_is_omap3505() || cpu_is_omap3517())
+ _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
+ omap3517_cpu_suspend_sz);
+ else
+ _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
if (omap_type() != OMAP2_DEVICE_TYPE_GP)
- _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
- save_secure_ram_context_sz);
+ if (cpu_is_omap3505() || cpu_is_omap3517())
+ _omap_save_secure_sram = omap_sram_push(
+ omap3517_save_secure_ram_context,
+ omap3517_save_secure_ram_context_sz);
+ else
+ _omap_save_secure_sram = omap_sram_push(
+ save_secure_ram_context,
+ save_secure_ram_context_sz);
}
static void __init pm_errata_configure(void)
diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
new file mode 100644
index 0000000..3fceefc
--- /dev/null
+++ b/arch/arm/mach-omap2/sleep3517.S
@@ -0,0 +1,144 @@
+/*
+/* linux/arch/arm/mach-omap2/sleep3517.S
+ *
+ * AM3505/3517 Sleep Code.
+ * Ranjith Lohithakshan <ranjithl@ti.com>
+ *
+ * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+#include <plat/sram.h>
+#include <mach/io.h>
+
+#include "cm2xxx_3xxx.h"
+#include "prm2xxx_3xxx.h"
+#include "sdrc.h"
+#include "control.h"
+
+#define CM_IDLEST1_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, CM_IDLEST1)
+#define CM_CLKST_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, OMAP3430_CM_CLKSTST)
+#define CM_ICLKEN1_CORE_V OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)
+
+#define EMIF_PM_CTR_V OMAP2_L3_IO_ADDRESS(0x6D000038)
+#define OMAP3517_CONF1_REG_V OMAP2_L4_IO_ADDRESS(0x48002584)
+
+/*
+ * Forces OMAP into idle state
+ *
+ * omap34xx_suspend() - This bit of code just executes the WFI
+ * for normal idles.
+ *
+ * Note: This code get's copied to internal SRAM at boot. When the OMAP
+ * wakes up it continues execution at the point it went to sleep.
+ */
+ENTRY(omap3517_cpu_suspend)
+ stmfd sp!, {r0-r12, lr} @ save registers on stack
+loop:
+ /*b loop*/ @Enable to debug by stepping through code
+
+ /* Put EMIF in self-refresh */
+ ldr r4, emif_pm_ctrl
+ ldr r5, [r4]
+ orr r5, r5, #0x200
+ str r5, [r4]
+
+ /* Disable SDRC and Control Module */
+ ldr r4, cm_iclken1_core
+ ldr r5, clk_core_disable
+ str r5, [r4]
+wait_sdrc_ok:
+ ldr r4, cm_idlest1_core
+ ldr r5, [r4]
+ and r5, r5, #0x2
+ cmp r5, #0x2
+ bne wait_sdrc_ok
+
+ /* Gate DDR Phy clock */
+ ldr r4, omap3517_conf1
+ ldr r5, emif_phy_gate
+ str r5, [r4]
+
+ /* Data memory barrier and Data sync barrier */
+ mov r1, #0
+ mcr p15, 0, r1, c7, c10, 4
+ mcr p15, 0, r1, c7, c10, 5
+
+ wfi
+
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+
+ /* Enable SDRC and Control Module */
+ ldr r4, cm_iclken1_core
+ ldr r5, iclk_core_enable
+ str r5, [r4]
+
+ /* Enable DDR Phy Clock */
+ ldr r4, omap3517_conf1
+ ldr r5, emif_phy_enable
+ str r5, [r4]
+
+ /* Take EMIF out of self-refresh */
+ ldr r4, emif_pm_ctrl
+ ldr r5, [r4]
+ bic r5, r5, #0x200
+ str r5, [r4]
+
+ ldmfd sp!, {r0-r12, pc} @ restore regs and return
+
+clk_core_disable:
+ .word 0x0
+iclk_core_enable:
+ .word 0x42
+emif_phy_gate:
+ .word 0x2620
+emif_phy_enable:
+ .word 0x8620
+cm_idlest1_core:
+ .word CM_IDLEST1_CORE_V
+cm_clkst_core:
+ .word CM_CLKST_CORE_V
+emif_pm_ctrl:
+ .word EMIF_PM_CTR_V
+cm_iclken1_core:
+ .word CM_ICLKEN1_CORE_V
+omap3517_conf1:
+ .word OMAP3517_CONF1_REG_V
+ENTRY(omap3517_cpu_suspend_sz)
+ .word . - omap3517_cpu_suspend
+
+/* Function to call rom code to save secure ram context */
+ENTRY(omap3517_save_secure_ram_context)
+ stmfd sp!, {r1-r12, lr} @ save registers on stack
+save_secure_ram_debug:
+ /* b save_secure_ram_debug */ @ enable to debug save code
+ ldmfd sp!, {r1-r12, pc}
+ENTRY(omap3517_save_secure_ram_context_sz)
+ .word . - omap3517_save_secure_ram_context
+
+/* Function call to get the restore pointer for resume from OFF */
+ENTRY(omap3517_get_restore_pointer)
+ stmfd sp!, {lr} @ save registers on stack
+ ldmfd sp!, {pc} @ restore regs and return
+ENTRY(omap3517_get_restore_pointer_sz)
+ .word . - omap3517_get_restore_pointer
+
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 2/4] AM3517: Add armv7-a flag for sleepam3517.S
2011-08-19 11:55 ` Abhilash K V
(?)
@ 2011-08-19 11:55 ` Abhilash K V
-1 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Vaibhav Hiremath, Abhilash K V
From: Vaibhav Hiremath <hvaibhav@ti.com>
Without "armv7-a" this flag build fails with following error -
arch/arm/mach-omap2/sleep3517.S: Assembler messages:
arch/arm/mach-omap2/sleep3517.S:80: Error: selected processor does not
support `wfi'
make[1]: *** [arch/arm/mach-omap2/sleep3517.o] Error 1
make: *** [arch/arm/mach-omap2] Error 2
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/Makefile | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 3fdf086..0e626da 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o
AFLAGS_sleep24xx.o :=-Wa,-march=armv6
AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec)
+AFLAGS_sleep3517.o :=-Wa,-march=armv7-a$(plus_sec)
ifeq ($(CONFIG_PM_VERBOSE),y)
CFLAGS_pm_bus.o += -DDEBUG
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/4] AM3517: Add armv7-a flag for sleepam3517.S
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-arm-kernel
From: Vaibhav Hiremath <hvaibhav@ti.com>
Without "armv7-a" this flag build fails with following error -
arch/arm/mach-omap2/sleep3517.S: Assembler messages:
arch/arm/mach-omap2/sleep3517.S:80: Error: selected processor does not
support `wfi'
make[1]: *** [arch/arm/mach-omap2/sleep3517.o] Error 1
make: *** [arch/arm/mach-omap2] Error 2
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/Makefile | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 3fdf086..0e626da 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o
AFLAGS_sleep24xx.o :=-Wa,-march=armv6
AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec)
+AFLAGS_sleep3517.o :=-Wa,-march=armv7-a$(plus_sec)
ifeq ($(CONFIG_PM_VERBOSE),y)
CFLAGS_pm_bus.o += -DDEBUG
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/4] AM3517: Add armv7-a flag for sleepam3517.S
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Vaibhav Hiremath, Abhilash K V
From: Vaibhav Hiremath <hvaibhav@ti.com>
Without "armv7-a" this flag build fails with following error -
arch/arm/mach-omap2/sleep3517.S: Assembler messages:
arch/arm/mach-omap2/sleep3517.S:80: Error: selected processor does not
support `wfi'
make[1]: *** [arch/arm/mach-omap2/sleep3517.o] Error 1
make: *** [arch/arm/mach-omap2] Error 2
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/Makefile | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 3fdf086..0e626da 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o
AFLAGS_sleep24xx.o :=-Wa,-march=armv6
AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec)
+AFLAGS_sleep3517.o :=-Wa,-march=armv7-a$(plus_sec)
ifeq ($(CONFIG_PM_VERBOSE),y)
CFLAGS_pm_bus.o += -DDEBUG
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/4] pm34xx: Warning FIX related to ambiguous else loop
2011-08-19 11:55 ` Abhilash K V
(?)
@ 2011-08-19 11:55 ` Abhilash K V
-1 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Vaibhav Hiremath, Abhilash K V
From: Vaibhav Hiremath <hvaibhav@ti.com>
Fixes below warning -
arch/arm/mach-omap2/pm34xx.c:1041: warning:
suggest explicit braces to avoid ambiguous 'else
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/pm34xx.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 12af5b9..ab3822b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -856,7 +856,7 @@ void omap_push_sram_idle(void)
else
_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
- if (omap_type() != OMAP2_DEVICE_TYPE_GP)
+ if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
if (cpu_is_omap3505() || cpu_is_omap3517())
_omap_save_secure_sram = omap_sram_push(
omap3517_save_secure_ram_context,
@@ -865,6 +865,7 @@ void omap_push_sram_idle(void)
_omap_save_secure_sram = omap_sram_push(
save_secure_ram_context,
save_secure_ram_context_sz);
+ }
}
static void __init pm_errata_configure(void)
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 3/4] pm34xx: Warning FIX related to ambiguous else loop
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-arm-kernel
From: Vaibhav Hiremath <hvaibhav@ti.com>
Fixes below warning -
arch/arm/mach-omap2/pm34xx.c:1041: warning:
suggest explicit braces to avoid ambiguous 'else
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/pm34xx.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 12af5b9..ab3822b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -856,7 +856,7 @@ void omap_push_sram_idle(void)
else
_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
- if (omap_type() != OMAP2_DEVICE_TYPE_GP)
+ if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
if (cpu_is_omap3505() || cpu_is_omap3517())
_omap_save_secure_sram = omap_sram_push(
omap3517_save_secure_ram_context,
@@ -865,6 +865,7 @@ void omap_push_sram_idle(void)
_omap_save_secure_sram = omap_sram_push(
save_secure_ram_context,
save_secure_ram_context_sz);
+ }
}
static void __init pm_errata_configure(void)
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 3/4] pm34xx: Warning FIX related to ambiguous else loop
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Vaibhav Hiremath, Abhilash K V
From: Vaibhav Hiremath <hvaibhav@ti.com>
Fixes below warning -
arch/arm/mach-omap2/pm34xx.c:1041: warning:
suggest explicit braces to avoid ambiguous 'else
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/pm34xx.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 12af5b9..ab3822b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -856,7 +856,7 @@ void omap_push_sram_idle(void)
else
_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
- if (omap_type() != OMAP2_DEVICE_TYPE_GP)
+ if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
if (cpu_is_omap3505() || cpu_is_omap3517())
_omap_save_secure_sram = omap_sram_push(
omap3517_save_secure_ram_context,
@@ -865,6 +865,7 @@ void omap_push_sram_idle(void)
_omap_save_secure_sram = omap_sram_push(
save_secure_ram_context,
save_secure_ram_context_sz);
+ }
}
static void __init pm_errata_configure(void)
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 4/4] AM3517: Fix suspend-resume sequence
2011-08-19 11:55 ` Abhilash K V
(?)
@ 2011-08-19 11:55 ` Abhilash K V
-1 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Abhilash K V
This patch fixes the crash seen while resuming with i2c devices
enabled. This crash was happening because the interface-clocks
were disabled in the suspend sequence, and not re-enabled on
resumption.
The current patch saves the value of the CM_ICLKEN1_CORE register
before zeroing it out, and restores upon resumption. In AM3517 the
interface clocks are enabled by the clock module ONLY during
initialization, so the suspend sequence (in arch/arm/mach-omap2/
sleep3517.S) has to manually turn it off before executing wfi
and then back on again on returning from wfi, to ensure that all
interface clocks are enabled when control returns to omap_sram_idle()
after waking up from idle.
Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Reviewed-by: Sanjeev Premi <premi@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/sleep3517.S | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
index 3fceefc..070a943 100644
--- a/arch/arm/mach-omap2/sleep3517.S
+++ b/arch/arm/mach-omap2/sleep3517.S
@@ -55,6 +55,9 @@ loop:
/* Disable SDRC and Control Module */
ldr r4, cm_iclken1_core
+ ldr r5, [r4]
+ str r5, iclk_core_enable
+ ldr r4, cm_iclken1_core
ldr r5, clk_core_disable
str r5, [r4]
wait_sdrc_ok:
@@ -108,7 +111,7 @@ wait_sdrc_ok:
clk_core_disable:
.word 0x0
iclk_core_enable:
- .word 0x42
+ .word 0x0
emif_phy_gate:
.word 0x2620
emif_phy_enable:
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 4/4] AM3517: Fix suspend-resume sequence
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-arm-kernel
This patch fixes the crash seen while resuming with i2c devices
enabled. This crash was happening because the interface-clocks
were disabled in the suspend sequence, and not re-enabled on
resumption.
The current patch saves the value of the CM_ICLKEN1_CORE register
before zeroing it out, and restores upon resumption. In AM3517 the
interface clocks are enabled by the clock module ONLY during
initialization, so the suspend sequence (in arch/arm/mach-omap2/
sleep3517.S) has to manually turn it off before executing wfi
and then back on again on returning from wfi, to ensure that all
interface clocks are enabled when control returns to omap_sram_idle()
after waking up from idle.
Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Reviewed-by: Sanjeev Premi <premi@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/sleep3517.S | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
index 3fceefc..070a943 100644
--- a/arch/arm/mach-omap2/sleep3517.S
+++ b/arch/arm/mach-omap2/sleep3517.S
@@ -55,6 +55,9 @@ loop:
/* Disable SDRC and Control Module */
ldr r4, cm_iclken1_core
+ ldr r5, [r4]
+ str r5, iclk_core_enable
+ ldr r4, cm_iclken1_core
ldr r5, clk_core_disable
str r5, [r4]
wait_sdrc_ok:
@@ -108,7 +111,7 @@ wait_sdrc_ok:
clk_core_disable:
.word 0x0
iclk_core_enable:
- .word 0x42
+ .word 0x0
emif_phy_gate:
.word 0x2620
emif_phy_enable:
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 4/4] AM3517: Fix suspend-resume sequence
@ 2011-08-19 11:55 ` Abhilash K V
0 siblings, 0 replies; 36+ messages in thread
From: Abhilash K V @ 2011-08-19 11:55 UTC (permalink / raw)
To: linux-omap, linux-arm-kernel, linux-kernel
Cc: tony, linux, khilman, Abhilash K V
This patch fixes the crash seen while resuming with i2c devices
enabled. This crash was happening because the interface-clocks
were disabled in the suspend sequence, and not re-enabled on
resumption.
The current patch saves the value of the CM_ICLKEN1_CORE register
before zeroing it out, and restores upon resumption. In AM3517 the
interface clocks are enabled by the clock module ONLY during
initialization, so the suspend sequence (in arch/arm/mach-omap2/
sleep3517.S) has to manually turn it off before executing wfi
and then back on again on returning from wfi, to ensure that all
interface clocks are enabled when control returns to omap_sram_idle()
after waking up from idle.
Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
Reviewed-by: Sanjeev Premi <premi@ti.com>
Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
---
arch/arm/mach-omap2/sleep3517.S | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
index 3fceefc..070a943 100644
--- a/arch/arm/mach-omap2/sleep3517.S
+++ b/arch/arm/mach-omap2/sleep3517.S
@@ -55,6 +55,9 @@ loop:
/* Disable SDRC and Control Module */
ldr r4, cm_iclken1_core
+ ldr r5, [r4]
+ str r5, iclk_core_enable
+ ldr r4, cm_iclken1_core
ldr r5, clk_core_disable
str r5, [r4]
wait_sdrc_ok:
@@ -108,7 +111,7 @@ wait_sdrc_ok:
clk_core_disable:
.word 0x0
iclk_core_enable:
- .word 0x42
+ .word 0x0
emif_phy_gate:
.word 0x2620
emif_phy_enable:
--
1.7.1
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 4/4] AM3517: Fix suspend-resume sequence
2011-08-19 11:55 ` Abhilash K V
@ 2011-08-19 19:53 ` Russell King - ARM Linux
-1 siblings, 0 replies; 36+ messages in thread
From: Russell King - ARM Linux @ 2011-08-19 19:53 UTC (permalink / raw)
To: Abhilash K V; +Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, khilman
On Fri, Aug 19, 2011 at 05:25:27PM +0530, Abhilash K V wrote:
> This patch fixes the crash seen while resuming with i2c devices
> enabled. This crash was happening because the interface-clocks
> were disabled in the suspend sequence, and not re-enabled on
> resumption.
> The current patch saves the value of the CM_ICLKEN1_CORE register
> before zeroing it out, and restores upon resumption. In AM3517 the
> interface clocks are enabled by the clock module ONLY during
> initialization, so the suspend sequence (in arch/arm/mach-omap2/
> sleep3517.S) has to manually turn it off before executing wfi
> and then back on again on returning from wfi, to ensure that all
> interface clocks are enabled when control returns to omap_sram_idle()
> after waking up from idle.
We don't need to know that your original implementation had a problem and
you fixed it with a later patch. Just roll the two patches together.
When Linus talks about unnecessary churn, this is one of the reasons it
happens. Please change your workflow to reduce the amount of changes
done - especially when you're submitting patches which create entirely
new files.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/4] AM3517: Fix suspend-resume sequence
@ 2011-08-19 19:53 ` Russell King - ARM Linux
0 siblings, 0 replies; 36+ messages in thread
From: Russell King - ARM Linux @ 2011-08-19 19:53 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 19, 2011 at 05:25:27PM +0530, Abhilash K V wrote:
> This patch fixes the crash seen while resuming with i2c devices
> enabled. This crash was happening because the interface-clocks
> were disabled in the suspend sequence, and not re-enabled on
> resumption.
> The current patch saves the value of the CM_ICLKEN1_CORE register
> before zeroing it out, and restores upon resumption. In AM3517 the
> interface clocks are enabled by the clock module ONLY during
> initialization, so the suspend sequence (in arch/arm/mach-omap2/
> sleep3517.S) has to manually turn it off before executing wfi
> and then back on again on returning from wfi, to ensure that all
> interface clocks are enabled when control returns to omap_sram_idle()
> after waking up from idle.
We don't need to know that your original implementation had a problem and
you fixed it with a later patch. Just roll the two patches together.
When Linus talks about unnecessary churn, this is one of the reasons it
happens. Please change your workflow to reduce the amount of changes
done - especially when you're submitting patches which create entirely
new files.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/4] pm34xx: Warning FIX related to ambiguous else loop
2011-08-19 11:55 ` Abhilash K V
@ 2011-08-30 23:00 ` Kevin Hilman
-1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-08-30 23:00 UTC (permalink / raw)
To: Abhilash K V
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Vaibhav Hiremath
Abhilash K V <abhilash.kv@ti.com> writes:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
>
> Fixes below warning -
>
> arch/arm/mach-omap2/pm34xx.c:1041: warning:
> suggest explicit braces to avoid ambiguous 'else
This warning was introduced in [1/4], so I'd suggest just folding this
fix into the original patch.
Kevin
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
> ---
> arch/arm/mach-omap2/pm34xx.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 12af5b9..ab3822b 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -856,7 +856,7 @@ void omap_push_sram_idle(void)
> else
> _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> - if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> + if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
> if (cpu_is_omap3505() || cpu_is_omap3517())
> _omap_save_secure_sram = omap_sram_push(
> omap3517_save_secure_ram_context,
> @@ -865,6 +865,7 @@ void omap_push_sram_idle(void)
> _omap_save_secure_sram = omap_sram_push(
> save_secure_ram_context,
> save_secure_ram_context_sz);
> + }
> }
>
> static void __init pm_errata_configure(void)
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 3/4] pm34xx: Warning FIX related to ambiguous else loop
@ 2011-08-30 23:00 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-08-30 23:00 UTC (permalink / raw)
To: linux-arm-kernel
Abhilash K V <abhilash.kv@ti.com> writes:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
>
> Fixes below warning -
>
> arch/arm/mach-omap2/pm34xx.c:1041: warning:
> suggest explicit braces to avoid ambiguous 'else
This warning was introduced in [1/4], so I'd suggest just folding this
fix into the original patch.
Kevin
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
> ---
> arch/arm/mach-omap2/pm34xx.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 12af5b9..ab3822b 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -856,7 +856,7 @@ void omap_push_sram_idle(void)
> else
> _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> - if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> + if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
> if (cpu_is_omap3505() || cpu_is_omap3517())
> _omap_save_secure_sram = omap_sram_push(
> omap3517_save_secure_ram_context,
> @@ -865,6 +865,7 @@ void omap_push_sram_idle(void)
> _omap_save_secure_sram = omap_sram_push(
> save_secure_ram_context,
> save_secure_ram_context_sz);
> + }
> }
>
> static void __init pm_errata_configure(void)
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/4] AM3517 : support for suspend/resume
2011-08-19 11:55 ` Abhilash K V
@ 2011-08-19 19:56 ` Russell King - ARM Linux
-1 siblings, 0 replies; 36+ messages in thread
From: Russell King - ARM Linux @ 2011-08-19 19:56 UTC (permalink / raw)
To: Abhilash K V
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, khilman,
Ranjith Lohithakshan
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-08-19 19:56 ` Russell King - ARM Linux
0 siblings, 0 replies; 36+ messages in thread
From: Russell King - ARM Linux @ 2011-08-19 19:56 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/4] AM3517 : support for suspend/resume
2011-08-19 11:55 ` Abhilash K V
(?)
@ 2011-08-30 22:58 ` Kevin Hilman
-1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-08-30 22:58 UTC (permalink / raw)
To: Abhilash K V
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Ranjith Lohithakshan
Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
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_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_setup(struct clockdomain *clkdm, void *unused)
>
> void omap_push_sram_idle(void)
> {
> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
> + omap3517_cpu_suspend_sz);
> + else
> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
> - save_secure_ram_context_sz);
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_save_secure_sram = omap_sram_push(
> + omap3517_save_secure_ram_context,
> + omap3517_save_secure_ram_context_sz);
> + else
> + _omap_save_secure_sram = omap_sram_push(
> + save_secure_ram_context,
> + save_secure_ram_context_sz);
> }
You add a new assembly function for save secure context, but that
function is essentially a nop.
It would be better to just not have a function for these devices.
To that end, it would help to add an OMAP "feature" stating whether or
not the device has secure RAM. Then, instead of the cpu_is_* checks
here, that feature flag can be checked.
>
> static void __init pm_errata_configure(void)
> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
> new file mode 100644
> index 0000000..3fceefc
> --- /dev/null
> +++ b/arch/arm/mach-omap2/sleep3517.S
> @@ -0,0 +1,144 @@
> +/*
> +/* linux/arch/arm/mach-omap2/sleep3517.S
You can leave out filenames in comments. Files tend to move, and the
comments don't get changed and become stale.
Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-08-30 22:58 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-08-30 22:58 UTC (permalink / raw)
To: linux-arm-kernel
Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
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_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_setup(struct clockdomain *clkdm, void *unused)
>
> void omap_push_sram_idle(void)
> {
> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
> + omap3517_cpu_suspend_sz);
> + else
> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
> - save_secure_ram_context_sz);
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_save_secure_sram = omap_sram_push(
> + omap3517_save_secure_ram_context,
> + omap3517_save_secure_ram_context_sz);
> + else
> + _omap_save_secure_sram = omap_sram_push(
> + save_secure_ram_context,
> + save_secure_ram_context_sz);
> }
You add a new assembly function for save secure context, but that
function is essentially a nop.
It would be better to just not have a function for these devices.
To that end, it would help to add an OMAP "feature" stating whether or
not the device has secure RAM. Then, instead of the cpu_is_* checks
here, that feature flag can be checked.
>
> static void __init pm_errata_configure(void)
> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
> new file mode 100644
> index 0000000..3fceefc
> --- /dev/null
> +++ b/arch/arm/mach-omap2/sleep3517.S
> @@ -0,0 +1,144 @@
> +/*
> +/* linux/arch/arm/mach-omap2/sleep3517.S
You can leave out filenames in comments. Files tend to move, and the
comments don't get changed and become stale.
Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-08-30 22:58 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-08-30 22:58 UTC (permalink / raw)
To: Abhilash K V
Cc: linux, Ranjith Lohithakshan, tony, linux-kernel, linux-omap,
linux-arm-kernel
Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
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_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_setup(struct clockdomain *clkdm, void *unused)
>
> void omap_push_sram_idle(void)
> {
> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
> + omap3517_cpu_suspend_sz);
> + else
> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
> - save_secure_ram_context_sz);
> + if (cpu_is_omap3505() || cpu_is_omap3517())
> + _omap_save_secure_sram = omap_sram_push(
> + omap3517_save_secure_ram_context,
> + omap3517_save_secure_ram_context_sz);
> + else
> + _omap_save_secure_sram = omap_sram_push(
> + save_secure_ram_context,
> + save_secure_ram_context_sz);
> }
You add a new assembly function for save secure context, but that
function is essentially a nop.
It would be better to just not have a function for these devices.
To that end, it would help to add an OMAP "feature" stating whether or
not the device has secure RAM. Then, instead of the cpu_is_* checks
here, that feature flag can be checked.
>
> static void __init pm_errata_configure(void)
> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
> new file mode 100644
> index 0000000..3fceefc
> --- /dev/null
> +++ b/arch/arm/mach-omap2/sleep3517.S
> @@ -0,0 +1,144 @@
> +/*
> +/* linux/arch/arm/mach-omap2/sleep3517.S
You can leave out filenames in comments. Files tend to move, and the
comments don't get changed and become stale.
Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [PATCH 1/4] AM3517 : support for suspend/resume
2011-08-30 22:58 ` Kevin Hilman
(?)
@ 2011-09-13 11:31 ` Koyamangalath, Abhilash
-1 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-13 11:31 UTC (permalink / raw)
To: Hilman, Kevin
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
Hi
On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>
> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>
> 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());
>
> 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_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_setup(struct clockdomain *clkdm, void *unused)
>>
>> void omap_push_sram_idle(void)
>> {
>> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
>> + omap3517_cpu_suspend_sz);
>> + else
>> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> omap34xx_cpu_suspend_sz);
>> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
>> - save_secure_ram_context_sz);
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_save_secure_sram = omap_sram_push(
>> + omap3517_save_secure_ram_context,
>> + omap3517_save_secure_ram_context_sz);
>> + else
>> + _omap_save_secure_sram = omap_sram_push(
>> + save_secure_ram_context,
>> + save_secure_ram_context_sz);
>> }
>
> You add a new assembly function for save secure context, but that
> function is essentially a nop.
>
> It would be better to just not have a function for these devices.
>
> To that end, it would help to add an OMAP "feature" stating whether or
> not the device has secure RAM. Then, instead of the cpu_is_* checks
> here, that feature flag can be checked.
>
>>
>> static void __init pm_errata_configure(void)
>> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
>> new file mode 100644
>> index 0000000..3fceefc
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/sleep3517.S
>> @@ -0,0 +1,144 @@
>> +/*
>> +/* linux/arch/arm/mach-omap2/sleep3517.S
>
> You can leave out filenames in comments. Files tend to move, and the
> comments don't get changed and become stale.
>
> Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-13 11:31 ` Koyamangalath, Abhilash
0 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-13 11:31 UTC (permalink / raw)
To: linux-arm-kernel
Hi
On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>
> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>
> 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@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());
>
> 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_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_setup(struct clockdomain *clkdm, void *unused)
>>
>> void omap_push_sram_idle(void)
>> {
>> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
>> + omap3517_cpu_suspend_sz);
>> + else
>> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> omap34xx_cpu_suspend_sz);
>> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
>> - save_secure_ram_context_sz);
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_save_secure_sram = omap_sram_push(
>> + omap3517_save_secure_ram_context,
>> + omap3517_save_secure_ram_context_sz);
>> + else
>> + _omap_save_secure_sram = omap_sram_push(
>> + save_secure_ram_context,
>> + save_secure_ram_context_sz);
>> }
>
> You add a new assembly function for save secure context, but that
> function is essentially a nop.
>
> It would be better to just not have a function for these devices.
>
> To that end, it would help to add an OMAP "feature" stating whether or
> not the device has secure RAM. Then, instead of the cpu_is_* checks
> here, that feature flag can be checked.
>
>>
>> static void __init pm_errata_configure(void)
>> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
>> new file mode 100644
>> index 0000000..3fceefc
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/sleep3517.S
>> @@ -0,0 +1,144 @@
>> +/*
>> +/* linux/arch/arm/mach-omap2/sleep3517.S
>
> You can leave out filenames in comments. Files tend to move, and the
> comments don't get changed and become stale.
>
> Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-13 11:31 ` Koyamangalath, Abhilash
0 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-13 11:31 UTC (permalink / raw)
To: Hilman, Kevin
Cc: linux, Lohithakshan, Ranjith, tony, linux-kernel, linux-omap,
linux-arm-kernel
Hi
On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>
> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>
> 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());
>
> 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_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_setup(struct clockdomain *clkdm, void *unused)
>>
>> void omap_push_sram_idle(void)
>> {
>> - _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_sram_idle = omap_sram_push(omap3517_cpu_suspend,
>> + omap3517_cpu_suspend_sz);
>> + else
>> + _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
>> omap34xx_cpu_suspend_sz);
>> if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>> - _omap_save_secure_sram = omap_sram_push(save_secure_ram_context,
>> - save_secure_ram_context_sz);
>> + if (cpu_is_omap3505() || cpu_is_omap3517())
>> + _omap_save_secure_sram = omap_sram_push(
>> + omap3517_save_secure_ram_context,
>> + omap3517_save_secure_ram_context_sz);
>> + else
>> + _omap_save_secure_sram = omap_sram_push(
>> + save_secure_ram_context,
>> + save_secure_ram_context_sz);
>> }
>
> You add a new assembly function for save secure context, but that
> function is essentially a nop.
>
> It would be better to just not have a function for these devices.
>
> To that end, it would help to add an OMAP "feature" stating whether or
> not the device has secure RAM. Then, instead of the cpu_is_* checks
> here, that feature flag can be checked.
>
>>
>> static void __init pm_errata_configure(void)
>> diff --git a/arch/arm/mach-omap2/sleep3517.S b/arch/arm/mach-omap2/sleep3517.S
>> new file mode 100644
>> index 0000000..3fceefc
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/sleep3517.S
>> @@ -0,0 +1,144 @@
>> +/*
>> +/* linux/arch/arm/mach-omap2/sleep3517.S
>
> You can leave out filenames in comments. Files tend to move, and the
> comments don't get changed and become stale.
>
> Kevin
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/4] AM3517 : support for suspend/resume
2011-09-13 11:31 ` Koyamangalath, Abhilash
(?)
@ 2011-09-13 18:24 ` Kevin Hilman
-1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-13 18:24 UTC (permalink / raw)
To: Koyamangalath, Abhilash
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
Hi Abhilash,
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> Hi
>
> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>
>> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>>
>> 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 <khilman@ti.com>
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 <khilman@ti.com>
---
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 <khilman@deeprootsystems.com>
Date: Wed Dec 8 22:40:40 2010 +0000
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 protect UARTs from being accessed after disabled
during idle, but this causes problems in suspend.
During suspend, the console semaphore is acquired by the console
suspend method (console_suspend()) so the try_acquire_console_sem()
will always fail and suspend will be aborted.
To fix, introduce a check so the console semaphore is only attempted
during idle, and not during suspend. Also use the same check so that
the console semaphore is not prematurely released during resume.
Thanks to Paul Walmsley for suggesting adding the same check during
resume.
Cc: Paul Walmsley <paul@pwsan.com>
Tested-by: Jean Pihet <j-pihet@ti.com>
Tested-by: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-13 18:24 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-13 18:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi Abhilash,
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> Hi
>
> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>
>> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>>
>> 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]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-13 18:24 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-13 18:24 UTC (permalink / raw)
To: Koyamangalath, Abhilash
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
Hi Abhilash,
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> Hi
>
> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>
>> Abhilash K V <abhilash.kv@ti.com> 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 <ranjithl@ti.com>
>>> Reviewed-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>> Signed-off-by: Abhilash K V <abhilash.kv@ti.com>
>>
>> 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 <khilman@ti.com>
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 <khilman@ti.com>
---
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 <khilman@deeprootsystems.com>
Date: Wed Dec 8 22:40:40 2010 +0000
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 protect UARTs from being accessed after disabled
during idle, but this causes problems in suspend.
During suspend, the console semaphore is acquired by the console
suspend method (console_suspend()) so the try_acquire_console_sem()
will always fail and suspend will be aborted.
To fix, introduce a check so the console semaphore is only attempted
during idle, and not during suspend. Also use the same check so that
the console semaphore is not prematurely released during resume.
Thanks to Paul Walmsley for suggesting adding the same check during
resume.
Cc: Paul Walmsley <paul@pwsan.com>
Tested-by: Jean Pihet <j-pihet@ti.com>
Tested-by: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply related [flat|nested] 36+ messages in thread* RE: [PATCH 1/4] AM3517 : support for suspend/resume
2011-09-13 18:24 ` Kevin Hilman
(?)
@ 2011-09-14 13:00 ` Koyamangalath, Abhilash
-1 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-14 13:00 UTC (permalink / raw)
To: Hilman, Kevin
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
hi Kevin
On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
> Hi Abhilash,
>
> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>
>> Hi
>>
>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>
>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>
<snip>
>>>
>>> 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 <khilman@ti.com>
> 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 <khilman@ti.com>
> ---
> 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 <khilman@deeprootsystems.com>
> Date: Wed Dec 8 22:40:40 2010 +0000
>
> 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 protect UARTs from being accessed after disabled
> during idle, but this causes problems in suspend.
>
> During suspend, the console semaphore is acquired by the console
> suspend method (console_suspend()) so the try_acquire_console_sem()
> will always fail and suspend will be aborted.
>
> To fix, introduce a check so the console semaphore is only attempted
> during idle, and not during suspend. Also use the same check so that
> the console semaphore is not prematurely released during resume.
>
> Thanks to Paul Walmsley for suggesting adding the same check during
> resume.
>
> Cc: Paul Walmsley <paul@pwsan.com>
> Tested-by: Jean Pihet <j-pihet@ti.com>
> Tested-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-14 13:00 ` Koyamangalath, Abhilash
0 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-14 13:00 UTC (permalink / raw)
To: linux-arm-kernel
hi Kevin
On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
> Hi Abhilash,
>
> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>
>> Hi
>>
>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>
>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>
<snip>
>>>
>>> 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 <khilman@ti.com>
> 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 <khilman@ti.com>
> ---
> 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 <khilman@deeprootsystems.com>
> Date: Wed Dec 8 22:40:40 2010 +0000
>
> 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 protect UARTs from being accessed after disabled
> during idle, but this causes problems in suspend.
>
> During suspend, the console semaphore is acquired by the console
> suspend method (console_suspend()) so the try_acquire_console_sem()
> will always fail and suspend will be aborted.
>
> To fix, introduce a check so the console semaphore is only attempted
> during idle, and not during suspend. Also use the same check so that
> the console semaphore is not prematurely released during resume.
>
> Thanks to Paul Walmsley for suggesting adding the same check during
> resume.
>
> Cc: Paul Walmsley <paul@pwsan.com>
> Tested-by: Jean Pihet <j-pihet@ti.com>
> Tested-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-14 13:00 ` Koyamangalath, Abhilash
0 siblings, 0 replies; 36+ messages in thread
From: Koyamangalath, Abhilash @ 2011-09-14 13:00 UTC (permalink / raw)
To: Hilman, Kevin
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
hi Kevin
On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
> Hi Abhilash,
>
> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>
>> Hi
>>
>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>
>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>
<snip>
>>>
>>> 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 <khilman@ti.com>
> 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 <khilman@ti.com>
> ---
> 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 <khilman@deeprootsystems.com>
> Date: Wed Dec 8 22:40:40 2010 +0000
>
> 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 protect UARTs from being accessed after disabled
> during idle, but this causes problems in suspend.
>
> During suspend, the console semaphore is acquired by the console
> suspend method (console_suspend()) so the try_acquire_console_sem()
> will always fail and suspend will be aborted.
>
> To fix, introduce a check so the console semaphore is only attempted
> during idle, and not during suspend. Also use the same check so that
> the console semaphore is not prematurely released during resume.
>
> Thanks to Paul Walmsley for suggesting adding the same check during
> resume.
>
> Cc: Paul Walmsley <paul@pwsan.com>
> Tested-by: Jean Pihet <j-pihet@ti.com>
> Tested-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/4] AM3517 : support for suspend/resume
2011-09-14 13:00 ` Koyamangalath, Abhilash
(?)
@ 2011-09-15 17:09 ` Kevin Hilman
-1 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-15 17:09 UTC (permalink / raw)
To: Koyamangalath, Abhilash
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> hi Kevin
>
> On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
>> Hi Abhilash,
>>
>> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>>
>>> Hi
>>>
>>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>>
>>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>>
> <snip>
>>>>
>>>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-15 17:09 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-15 17:09 UTC (permalink / raw)
To: linux-arm-kernel
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> hi Kevin
>
> On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
>> Hi Abhilash,
>>
>> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>>
>>> Hi
>>>
>>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>>
>>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>>
> <snip>
>>>>
>>>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 1/4] AM3517 : support for suspend/resume
@ 2011-09-15 17:09 ` Kevin Hilman
0 siblings, 0 replies; 36+ messages in thread
From: Kevin Hilman @ 2011-09-15 17:09 UTC (permalink / raw)
To: Koyamangalath, Abhilash
Cc: linux-omap, linux-arm-kernel, linux-kernel, tony, linux,
Lohithakshan, Ranjith
"Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
> hi Kevin
>
> On Tue, Sep 13, 2011 at 11:54 PM, Hilman, Kevin wrote:
>> Hi Abhilash,
>>
>> "Koyamangalath, Abhilash" <abhilash.kv@ti.com> writes:
>>
>>> Hi
>>>
>>> On Wed, Aug 31, 2011 at 4:28 AM, Hilman, Kevin wrote:
>>>>
>>>> Abhilash K V <abhilash.kv@ti.com> writes:
>>>>
> <snip>
>>>>
>>>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread