From ab6e7a68679c12b5ececf144c169349bf26cb025 Mon Sep 17 00:00:00 2001 From: Sudan Landge Date: Thu, 9 Jan 2025 19:57:05 +0000 Subject: [PATCH 1/3] arch: arm: fix: pm_s2ram with CONFIG_DEBUG_THREAD_INFO What is the change? - Fixes #83660 allowing device to now enter suspend mode even if CONFIG_DEBUG_THREAD_INFO is enabled. Why is this needed? - z_sys_post_kernel was cleared as part of #d778d5c to "allow debuggers to display the correct thread state after the first 3 instructions have run". This is not required while resuming from suspend and it prevents the device from entering suspend so, move it out of resume path. Signed-off-by: Sudan Landge --- arch/arm/core/cortex_m/reset.S | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/arch/arm/core/cortex_m/reset.S b/arch/arm/core/cortex_m/reset.S index bc75ccfceaa..742a3fe3d65 100644 --- a/arch/arm/core/cortex_m/reset.S +++ b/arch/arm/core/cortex_m/reset.S @@ -68,13 +68,6 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,z_arm_reset) */ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start) -#if defined(CONFIG_DEBUG_THREAD_INFO) - /* Clear z_sys_post_kernel flag for RTOS aware debuggers */ - movs.n r0, #0 - ldr r1, =z_sys_post_kernel - strb r0, [r1] -#endif /* CONFIG_DEBUG_THREAD_INFO */ - #if defined(CONFIG_INIT_ARCH_HW_AT_BOOT) /* Reset CONTROL register */ movs.n r0, #0 @@ -93,6 +86,18 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start) bl arch_pm_s2ram_resume #endif /* CONFIG_PM_S2RAM */ + /* Note: Make sure that variables like z_sys_post_kernel + * are set after the call to arch_pm_s2ram_resume + * to avoid any issues with suspend/resume path. + * Refer issue #83660 for more details. + */ +#if defined(CONFIG_DEBUG_THREAD_INFO) + /* Clear z_sys_post_kernel flag for RTOS aware debuggers */ + movs.n r0, #0 + ldr r1, =z_sys_post_kernel + strb r0, [r1] +#endif /* CONFIG_DEBUG_THREAD_INFO */ + #if defined(CONFIG_SOC_RESET_HOOK) bl soc_reset_hook #endif From 9d23066a4667577ab3cab0a2eabf73687cf2fbc1 Mon Sep 17 00:00:00 2001 From: Sudan Landge Date: Thu, 9 Jan 2025 23:58:45 +0000 Subject: [PATCH 2/3] arch: arm: refactor pm_s2ram What is the change? - APIs `pm_s2ram_mark_set` and `pm_s2ram_mark_check_and_clear`, used to set/check pm_s2ram magic marker, no longer have Arm specific limitations. Why is this needed? - These APIs are generic and should not have arch specific dependency. Signed-off-by: Sudan Landge --- arch/arm/core/cortex_m/reset.S | 39 +++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/arch/arm/core/cortex_m/reset.S b/arch/arm/core/cortex_m/reset.S index 742a3fe3d65..c3f9362eca8 100644 --- a/arch/arm/core/cortex_m/reset.S +++ b/arch/arm/core/cortex_m/reset.S @@ -21,6 +21,7 @@ _ASM_FILE_PROLOGUE GTEXT(z_arm_reset) GTEXT(z_early_memset) GDATA(z_interrupt_stacks) +GDATA(z_main_stack) #if defined(CONFIG_DEBUG_THREAD_INFO) GDATA(z_sys_post_kernel) #endif @@ -29,7 +30,6 @@ GTEXT(soc_reset_hook) #endif #if defined(CONFIG_INIT_ARCH_HW_AT_BOOT) GTEXT(z_arm_init_arch_hw_at_boot) -GDATA(z_main_stack) #endif #if defined(CONFIG_PM_S2RAM) GTEXT(arch_pm_s2ram_resume) @@ -83,9 +83,44 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start) #endif /* CONFIG_INIT_ARCH_HW_AT_BOOT */ #if defined(CONFIG_PM_S2RAM) + /* + * Temporarily set MSP to interrupt stack so that arch_pm_s2ram_resume can + * use stack for calling pm_s2ram_mark_check_and_clear. + * This is safe because suspend never being called from an ISR ensures that + * interrupt stack was not live during suspend. + * Note: + * if resuming from suspend, MSP is restored from cpu context + * if not resuming, MSP will be set back to z_main_stack for proper init + * And, apart from arch_pm_s2ram_resume being able to use the stack for + * a short while, there is no change in behavior in either of the paths. + */ + ldr r0, =z_interrupt_stacks + CONFIG_ISR_STACK_SIZE + MPU_GUARD_ALIGN_AND_SIZE + msr msp, r0 + bl arch_pm_s2ram_resume #endif /* CONFIG_PM_S2RAM */ + /* + * MSP needs to be set to main stack for following scenarios: + * - If CONFIG_PM_S2RAM is enabled, MSP is set to z_interrupt_stacks + * for resume functions to make use of a stack. However, + * if the device was not suspended and if resume functions return, + * MSP needs to be set back to z_main_stack to ensure proper + * initialization. + * - If CONFIG_PM_S2RAM is not enabled but CONFIG_INIT_ARCH_HW_AT_BOOT is, + * MSP needs to be set to z_main_stack for proper initialization in case + * device was loaded through chain loading or a debugger, as the initial + * value of MSP would be anything that the previous image loaded. + * - If CONFIG_INIT_STACKS is enabled, we need to ensure MSP is not set + * to z_interrupt_stacks, so we set it to z_main_stack. + * + * Since these scenarios cover most of the cases, we set MSP to + * z_main_stack here. + * + */ + ldr r0, =z_main_stack + CONFIG_MAIN_STACK_SIZE + msr msp, r0 + /* Note: Make sure that variables like z_sys_post_kernel * are set after the call to arch_pm_s2ram_resume * to avoid any issues with suspend/resume path. @@ -110,8 +145,6 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start) str r0, [r1] dsb #endif /* CONFIG_CPU_HAS_ARM_MPU */ - ldr r0, =z_main_stack + CONFIG_MAIN_STACK_SIZE - msr msp, r0 /* Initialize core architecture registers and system blocks */ bl z_arm_init_arch_hw_at_boot From 18786e20e9cb987c24e0002cb42d2adf89e3691a Mon Sep 17 00:00:00 2001 From: Sudan Landge Date: Tue, 14 Jan 2025 00:01:18 +0000 Subject: [PATCH 3/3] Revert "arch: arm: cortex_m: pm_s2ram: Rework S2RAM mark functions" What is the change? - This reverts commit 474d4c3249c190b3261bc128ec1847e0a4f7b176 Why do we need this change? - This commit was added because Cortex-M didn't have a valid stack to make required functionality work however, the previous commit fixes this and makes interrupt stack available for use. This removes Arm specific limitation from these generic APIs so revert the commit to reflect the same. Signed-off-by: Sudan Landge --- arch/arm/core/cortex_m/pm_s2ram.S | 23 ++++++++------- arch/arm/core/cortex_m/pm_s2ram.c | 40 ++++++--------------------- include/zephyr/arch/common/pm_s2ram.h | 10 ------- 3 files changed, 19 insertions(+), 54 deletions(-) diff --git a/arch/arm/core/cortex_m/pm_s2ram.S b/arch/arm/core/cortex_m/pm_s2ram.S index d15bfea0432..1722e32136e 100644 --- a/arch/arm/core/cortex_m/pm_s2ram.S +++ b/arch/arm/core/cortex_m/pm_s2ram.S @@ -178,9 +178,7 @@ SECTION_FUNC(TEXT, arch_pm_s2ram_suspend) /* * Mark entering suspend to RAM. */ - mov r1, lr - bl pm_s2ram_mark_set - mov lr, r1 + bl pm_s2ram_mark_set /* * Call the system_off function passed as parameter. This should never @@ -199,9 +197,7 @@ SECTION_FUNC(TEXT, arch_pm_s2ram_suspend) /* * Reset the marking of suspend to RAM, return is ignored. */ - mov r1, lr - bl pm_s2ram_mark_check_and_clear - mov lr, r1 + bl pm_s2ram_mark_check_and_clear /* Move the stored return value of system_off back to r0, * setting it as return value for this function. @@ -216,13 +212,16 @@ GTEXT(arch_pm_s2ram_resume) SECTION_FUNC(TEXT, arch_pm_s2ram_resume) /* * Check if reset occurred after suspending to RAM. + * Store LR to ensure we can continue boot when we are not suspended + * to RAM. In addition to LR, R0 is pushed too, to ensure "SP mod 8 = 0", + * as stated by ARM rule 6.2.1.2 for AAPCS32. */ - mov r1, lr - bl pm_s2ram_mark_check_and_clear - mov lr, r1 - cmp r0, #0x1 - beq resume - bx lr + push {r0, lr} + bl pm_s2ram_mark_check_and_clear + cmp r0, #0x1 + pop {r0, lr} + beq resume + bx lr resume: /* diff --git a/arch/arm/core/cortex_m/pm_s2ram.c b/arch/arm/core/cortex_m/pm_s2ram.c index b7fe5d9b626..2657d48dc32 100644 --- a/arch/arm/core/cortex_m/pm_s2ram.c +++ b/arch/arm/core/cortex_m/pm_s2ram.c @@ -22,44 +22,20 @@ __noinit _cpu_context_t _cpu_context; */ static __noinit uint32_t marker; -void __attribute__((naked)) pm_s2ram_mark_set(void) +void pm_s2ram_mark_set(void) { - __asm__ volatile( - /* Set the marker to MAGIC value */ - "str %[_magic_val], [%[_marker]]\n" - - "bx lr\n" - : - : [_magic_val] "r"(MAGIC), [_marker] "r"(&marker) - : "r1", "r4", "memory"); + marker = MAGIC; } -bool __attribute__((naked)) pm_s2ram_mark_check_and_clear(void) +bool pm_s2ram_mark_check_and_clear(void) { - __asm__ volatile( - /* Set return value to 0 */ - "mov r0, #0\n" - - /* Check the marker */ - "ldr r3, [%[_marker]]\n" - "cmp r3, %[_magic_val]\n" - "bne exit\n" - - /* - * Reset the marker - */ - "str r0, [%[_marker]]\n" + if (marker == MAGIC) { + marker = 0; - /* - * Set return value to 1 - */ - "mov r0, #1\n" + return true; + } - "exit:\n" - "bx lr\n" - : - : [_magic_val] "r"(MAGIC), [_marker] "r"(&marker) - : "r0", "r1", "r3", "r4", "memory"); + return false; } #endif /* CONFIG_PM_S2RAM_CUSTOM_MARKING */ diff --git a/include/zephyr/arch/common/pm_s2ram.h b/include/zephyr/arch/common/pm_s2ram.h index ad9ab8ad7a4..34c544c769b 100644 --- a/include/zephyr/arch/common/pm_s2ram.h +++ b/include/zephyr/arch/common/pm_s2ram.h @@ -65,11 +65,6 @@ int arch_pm_s2ram_suspend(pm_s2ram_system_off_fn_t system_off); * * Default implementation is setting a magic word in RAM. CONFIG_PM_S2RAM_CUSTOM_MARKING * allows custom implementation. - * The following requirements must be fulfilled: - * - the function cannot use stack (asm function or function with 'naked' attribute) - * - the content of the R1 and R4 registers must remain unchanged - * - returning from the function should be performed with the `bx lr` instruction - * */ void pm_s2ram_mark_set(void); @@ -81,11 +76,6 @@ void pm_s2ram_mark_set(void); * * Default implementation is checking a magic word in RAM. CONFIG_PM_S2RAM_CUSTOM_MARKING * allows custom implementation. - * The following requirements must be fulfilled: - * - the function cannot use stack (most likely asm function) - * - the content of the R1 and R4 registers must remain unchanged - * - the function's return value is passed by R0 register - * - returning from the function should be performed with the `bx lr` instruction * * @retval true if marking is found which indicates resuming after suspend-to-RAM. * @retval false if marking is not found which indicates standard boot.