Skip to content

Conversation

nvlsianpu
Copy link
Contributor

For case when the zephyr-rtos application is the bootloader and booted target application had suspended the device, the resume operation would be initiated by the bootloader which could redirect execution to the application S2RAM routines directly.

Thanks to that target application would resume using compiled in S2RAM routines. Such scheme allows the zephyr-rtos based bootloader to not mock the application while does S2RAM resume operation. Therefore no need for keeping compatibility with S2RAM resume mechanism in application. No need to enable PM nor PM_S2RAM anymore in the bootloader.

#endif /* CONFIG_INIT_ARCH_HW_AT_BOOT */

#if defined(CONFIG_PM_S2RAM)
#if defined(CONFIG_PM_S2RAM) || defined(CONFIG_PM_S2RAM_RESUME_INTERMEDIARY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC correctly you are trying to define CONFIG_PM_S2RAM in zephyr while the other intermediary in the bootloader (which is a zephyr application)?
if thats the case why not just 2 different definitions of pm_s2ram_mark_check_and_clear? 1 for zephyr and other for the bootoader and arch_pm_s2ram_resume calls whichever is applicable here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - that also possible. But the I have to enabled CONFIG_PM=y and CONFIG_PM_S2RAM=y, the overwrite SoC original pm_s2ram_mark_check_and_clear implementation. We already did something like you are suggesting on our forks (https://github.com/nrfconnect/sdk-zephyr/pull/3252/files + https://github.com/nrfconnect/sdk-mcuboot/pull/489/files). So that could fly for sure.

I want to avoid CONFIG_PM=y and CONFIG_PM_S2RAM=y which causes unnecessary code compiled in the MCUboot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change have to be in mcuboot? if mcuboot is just a zephyr application then you could define pm_s2ram_mark_check_and_clear in soc.c under flags that enable mcuboot.
IMHO, this change is duplicating what pm_s2ram_mark_check_and_clear was designed for and so doesn't look right.

timer is due to expire.

config PM_S2RAM_RESUME_INTERMEDIARY
bool "Resume intermeniary fo Suspend-to-RAM (S2RAM)"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

power management subsystem of the number of ticks until the next kernel
timer is due to expire.

config PM_S2RAM_RESUME_INTERMEDIARY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is specific to bootloaders, and specific to CORTEX_M, it should not be in the common PM kconfig as it is here. The only knowledge needed by any arch is is whether CONFIG_PM_S2RAM is enabled in the application, if that requires enabling an option like this in the case of CORTEX_M with a zephyr based bootloader, that option should depend on CONFIG_PM_S2RAM, it should not be in PM.

The way to achieve this portably is to use sysbuild. We don't want CONFIG_PM enabled in the bootloader, it means something very different from what the option in this PR does for the bootloader, and we don't need to, we just need to check if the application was built with CONFIG_PM_S2RAM, like we check if the app was built with CONFIG_MCUBOOT_MODE_SINGLE_APP. See https://github.com/zephyrproject-rtos/zephyr/blob/main/share/sysbuild/images/bootloader/Kconfig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where you see check for that CONFIG_MCUBOOT_MODE_SINGLE_APP is set for the application? Isn't rather that mode is selected at sysbuild level and the transferred to subimages (via relevant Kconfigs)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the way to do this without sysbuild would be to select an arch specific kconfig manually when building the mcuboot image, that same config could be selected automatically by sysbuild

Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config ARCH_PM_S2RAM_RESUME
        bool "Include PM S2RAM resume jump"

for case when the zephyr-rtos application is the bootloader and booted
target application had suspended the device, the resume operation would
be initiated by the bootloader which could redirect execution to
the application S2RAM routines directly.
Thanks to that target application would resume using compiled in
S2RAM routines.
Such scheme allows the zephyr-rtos based bootloader to not mock
the application while does S2RAM resume operation.
Therefore no need for keeping compatibility with S2RAM resume
mechanism in application.
No need to enable PM nor PM_S2RAM anymore in the bootloader.

Signed-off-by: Andrzej Puzdrowski <[email protected]>
@nvlsianpu nvlsianpu force-pushed the upstream_s2ram_redirect branch from 8ac3f47 to cdcc1f5 Compare September 24, 2025 13:59
Copy link

Copy link
Contributor

@wearyzen wearyzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at #95914 (comment)

Comment on lines +86 to +94
* @brief Check suspend-to-RAM marking and does mediation.
*
* Function does resume mediation if determines resuming after suspend-to-RAM
* or return so standard boot will be executed.
*
* Implementation is up to given application - usually a bootloader. The function is expected
* to do mediation needed for resuming the application from S2AM state, which usually means no
* return to caller. Usage of this API implementation shall be enabled using
* CONFIG_ARCH_PM_S2RAM_RESUME.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of pm_s2ram_mark_check_and_mediate? What does resume mediation mean? I don't really understand what an implementation is supposed to do with just this description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is expected to do anything what is needed to boot the device from S2RAM sleep. Usually it should jump to the reset vector of the application. This reset vector is known for the bootloader - it is up to bootloader to make this operation reliable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a trampoline to redirect reset.S's bl arch_pm_s2ram_resume to pm_s2ram_mark_check_and_mediate? If so, we don't need assembly for that (and indeed @wearyzen's proposal of a platform hook instead is better)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a part of PM_S2RAM feature compiled in the zephyr-rtos instance. Bootloader should use config CONFIG_ARCH_PM_S2RAM_RESUME and Not CONFIG_PM_S2RAM while the application just the opposite way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key point of my comment was that we don't need a new ASM file for a call trampoline - either inline it in reset.S (i.e., soc_resume_hook which is what we're going towards) or implement it in a C file.

(the former solution is probably better)

@nvlsianpu
Copy link
Contributor Author

nvlsianpu commented Sep 25, 2025

Please have a look at #95914 (comment)

That PR fixes stacks interference issue. AFIKT S2RAM resume via bootloader is affected by that issue regardless of the existence of this PR solution.

@wearyzen soc_resume_hook is not in the right place for objective of that patch.

@wearyzen
Copy link
Contributor

That PR fixes stacks interference issue. AFIKT S2RAM resume via bootloader is affected by that issue regardless of the existence of this PR solution.

So what I wanted to highlight in that comment was that, this PR should introduce a new soc_resume_hook instead of the current approach and inside this new soc_resume_hook soc's can set MSP to their desired memory and call their desired arch_pm_s2ram_resume and so so.

@nvlsianpu
Copy link
Contributor Author

@wearyzen Something like this will make us able to do that.

#if defined(CONFIG_SOC_RESUME_HOOK)
    bl soc_resume_hook
#endif
#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.
	@@ -113,6 +124,7 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start)
     * 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
#endif

@wearyzen
Copy link
Contributor

This is how I think the solution would look like:

reset.S would call soc_resume_hook()

#if defined(CONFIG_SOC_RESUME_HOOK)
    bl soc_resume_hook
#elif defined(CONFIG_PM_S2RAM)
  ...
  bl arch_pm_s2ram_resume
#endif

in soc.c or any soc specific file:

SECTION_FUNC(TEXT, soc_resume_hook)
	push    {r0, lr}
	/* soc specific stuff which in your case seems to be setting a stack and calling the mediate function */
	ldr r0, =DT_REG_ADDR(DT_NODELABEL(pm_s2ram_stack)) + DT_REG_SIZE(DT_NODELABEL(pm_s2ram_stack))
	msr msp, r0
	bl      pm_s2ram_mark_check_and_mediate
	pop	{r0, pc}

and the pm_s2ram_mark_check_and_mediate goes in the same soc specific file as well.

soc_resume_hook should be added similar to how soc_reset_hook was added (not arch specific) and the only change that goes in arch is calling the soc_resume_hook.

@wearyzen
Copy link
Contributor

@wearyzen Something like this will make us able to do that.

#if defined(CONFIG_SOC_RESUME_HOOK)
    bl soc_resume_hook
#endif
#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.
	@@ -113,6 +124,7 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start)
     * 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
#endif

saw this after I hit my reply but we are thinking on same lines now 😄

@JarmouniA JarmouniA requested a review from nordicjm September 25, 2025 12:04
@nvlsianpu
Copy link
Contributor Author

@wearyzen After rethinking fix in #95914 can't be moved to hook-function. Can the this be covered by asm macro? Is that allowed?

@wearyzen
Copy link
Contributor

@wearyzen After rethinking fix in #95914 can't be moved to hook-function. Can the this be covered by asm macro? Is that allowed?

May I know what the issue is with moving it to the hook-function?

@nvlsianpu
Copy link
Contributor Author

nvlsianpu commented Oct 3, 2025

I'm closing this PR in faworu to more generic approach to problemy I want to solve.

@nvlsianpu
Copy link
Contributor Author

@wearyzen ^^

@nvlsianpu nvlsianpu closed this Oct 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants