arch/arm: introduce the pre-stack/RAM init hook

[nvlsianpu]

Introduce hook for customize reset.S code even before stack is initialized or RAM is accessed. Hook can be enabled using CONFIG_SOC_EARLY_RESET_HOOK=y.
Hook implementation is by soc_start_hook() function which should be provided by custom code.

For my case this hook can be use for implementing S2RAM resume operation bridge by the zephyr-rtos based bootloader: for such case the startu code is redirected to application startup code via application's reset vector call.
In the hook proram does recognition of whether this is the regular CPU boot or the S2RAM resume boot. For regular boot the hook returns to reset.S code flow. For S2RAM resume boot the hooks performs jump to well-known (for bootloader) application reset vector.

              resume/reboot                                               reboot==T
[S2RAM sleep]-------------->[ boot (reset.S) ]---->[soc_early_reset_hook]---------->[ continue regular boot ]
                                                          |  
                                                          |      resume==T
                                                          '----------------------> [ s2ram resume ]

This PR supersede #96290 as it is more generic an SoC independent.

[nashif]

we have a standard way for defining hooks, please use that instead (include/zephyr/platform/hooks.h)

[nvlsianpu]

@nashif I followed you and the others review comments,

[bjarki-andreasen]

This hook is also useful for applying trims/factory calibrations to RAM blocks before they are accessed which is the case for some Nordic SoCs.

[mathieuchopstm]

Not strictly related to the PR, but hooks could use a documentation page or something to clarify the exact expectations. This applies more specifically to SOC_RESET_HOOK which is arch-specific, but merely documented as:

zephyr/kernel/Kconfig.init

Lines 9 to 15 in 36bc2f3

	config SOC_RESET_HOOK
	bool "Run early SoC reset hook"
	help
	Run an early SoC reset hook.
	A custom hook soc_reset_hook() is executed at the beginning of the
	startup code (__start). soc_reset_hook() must be implemented by the SoC.

At first read, this can be understood to imply that reset vector must look like:

z_arm_reset:
#if CONFIG_SOC_RESET_HOOK
    bl soc_reset_hook
#endif
    ... do other reset stuff ...

which is (more or less) what we're introducing here... except it is expected that the "reset hook" runs with a stack configured - and possibly some registers reconfigured too if INIT_HW.

(Another remark: soc_reset_hook is currently only implemented for AArch32 in all profiles - maybe the arch-specific hooks should be gated behind a HAS_xxx_HOOK symbol)

[mathieuchopstm]

Ignoring the typos, we can't say "before RAM is initialized" here because it implies the hook doesn't exist when !CONFIG_XIP.

Really the only difference between this hook and the existing soc_reset_hook() is that the latter executes on a stack: why not call this soc_early_reset_hook()?

Also, I'm not sure if using "SoC-specific" branding here is good: my understanding is that e.g. MCUboot would provide (arch-specific) implementation of this, but it would be shared across all SoC vendors?

Ignoring rename, proposal (+ typo fixes):

Suggested change

	config SOC_PRE_RAM_HOOK
	bool "Run early SoC pre-RAM reset hook"
	help
	Run an early SoC reset hook, before RAM get initialize.
	A custom hook soc_pre_ram_hook() is executed at the beginning of the
	startup code (__start), even before any stack is initiated
	or RAM is accessed. soc_pre_ram_hook() must be implemented by the SoC.
	config SOC_PRE_RAM_HOOK
	bool "Run SoC-specific early reset hook"
	help
	Run a SoC-specific hook early in the reset/startup code (__start).
	A custom hook soc_pre_ram_hook() will be executed at the beginning
	of the architecture-specific startup code, after hardware has been
	configured as required by CONFIG_INIT_ARCH_HW_AT_BOOT.
	Returning from this hook is not mandatory: it can be used to implement
	special logic to resume execution in some scenarios (for example, when
	a bootloader is used).
	The stack pointer register is considered as scratch by the caller of
	this hook. In particular, this means that the hook is NOT allowed to
	"push" any data using this stack pointer. However, the hook may use
	an implementation-specific area as stack if desired; in such case,
	the original value of the stack pointer needs not to be "restored"
	before returning control to the hook's caller.
	Additional constraints may be imposed on the hook by the architecture.

(the last line is foreshadowing for architectures that don't have a designated return address register... but I think that's only x86? maybe it can be omitted)

[mathieuchopstm]

Outside PR scope, but then I would change SOC_RESET_HOOK to be described as called after "soc_pre_ram_hook", has a valid stack [with size XXX if that may be portable].

And I'd reword all symbols to use the Run SoC-specific XXX hook formulation. Though again there's some clarification here to be made about who can implement what hook: maybe some should be reserved for SoC-specific code only, whereas some are more likely to be used by application-specific code.

[wearyzen]

In particular, this means that the hook is NOT allowed to
"push" any data using this stack pointer.

I don't think we need to impose this restriction. soc_resume_hook, as I would like to call it, can do anything they like (e.g. changing or pushing stuff on stack) if they don't intend to return.
pls ignore this, the next line covers that I think.

[nvlsianpu]

used your suggestion on Kconfig help.

[mathieuchopstm]

In particular, this means that the hook is NOT allowed to
"push" any data using this stack pointer.

I don't think we need to impose this restriction. soc_resume_hook, as I would like to call it, can do anything they like (e.g. changing or pushing stuff on stack) if they don't intend to return.

The whole reason for these PRs was that one wanted arch_pm_s2ram_resume called on a different stack so it sounded like a reasonable required. I couldn't think of a reason to run code in a hook before arch_pm_s2ram_resume[1] other than "I want to override S2RAM resume in an application-specific manner - e.g., I'm a bootloader and will hand over control to loaded image", which would require a specific stack.

The Nordic RAM init thing just came to mind, but my assumption is that it's written in ASM and doesn't need a stack either? Is there really a use case where you both (1) need to run before arch_pm_s2ram_resume (i.e., execute on coldboot and warmboot path) and (2) are allowed to use the stack (i.e., you are not doing RAM initialization)?

I'm not against relaxing the requirement though (but then we need to decide if it is allowed to trash the sp - noting that we didn't allow trashing caller-called registers (i.e., r4~r11), though this could be said explicitly to make it extra clear you can't overwrite these)

[1] That should have been a point to consider for naming/description, but I overlook that (though it was in the back of my head).

[bjarki-andreasen]

to be clear though, we have not applied the trim yet, and nothing has broken yet so it seems the defaults are stable enough, but according to specs trims should be applied first.

[wearyzen]

I missed your previous comment but yes if there are other use cases then we can go with soc_early_reset_hook to be consistent with naming that other hooks have.
Also, I still think that socs should be allowed to use the stack whether or not they return from the hook. We just need a clear description of the hook to say something like hook is called immediately after reset so use stack wisely if the hook intends to return to continue with arch's resume/reset activity .

[mathieuchopstm]

My two cents: we should keep the "no stack" restriction and, if a usecase where stack is needed and soc_reset_hook isn't sufficient appears, the decision can be reconsidered. But I won't block on this point.

For the name, soc_early_reset_hook is fine by me. As documentation, I would say:

Warning: since the hook executes before the architecture-specific resume code,
it gets to run during cold boot (reset) and warm boot (resume). Care must be
taken about operations performed by the hook, especially during warm boot where
most/all RAM contents should remain unmodified. For example, whether the stack
area provided to the hook may be written to is implementation-specific.

Maybe we need to introduce a arch_pm_check_mark for usage by the hook (w/o "clear marker" semantics - de facto, this would be a k_is_warmboot 🙂)

BTW, I would update the documentation for soc_reset_hook to indicate it runs after the architecture-specific resume path - and thus isn't executed on warm boot path.

[wearyzen]

the name and stack requirement is fine by me, we'll cross the bridge when we get there :)
but I would be careful with the arch_pm_check_mark because pm_s2ram_mark_check_and_clear is what got us here in the first place.

[mathieuchopstm]

Right - s2ram_mark_check could require restrictions to be imposed on it which would be painful (i.e., "no stack usage => must be ASM" - which FTR Nordic folks were willing to do in the first place, and is done on STM32WB0 too now)

[bjarki-andreasen]

I think this looks good

[nvlsianpu]

changed hook name to soc_early_reset_hook, description updated.

[wearyzen]

minor nit but otherwise LGTM:

Suggested change

	* SoC-specific initialization. Its execution might takes place even before
	* the stack is initialized or data RAM is accessed.
	* Refer to relevant architecture zephyr-rtos startup implementation for more details.
	* SoC-specific initialization. Refer to :kconfig:option:`SOC_EARLY_RESET_HOOK` and relevant architecture zephyr-rtos startup implementation for more details.

[mathieuchopstm]

The two sentences are contradictory: either you can

use stack wisely

or

the stack pointer register should be considered as not initialized [...and you are] NOT allowed to push any data using this stack pointer value

Since it seems we agreed upon "no stack usage allowed" - and to avoid repetitions - I propose to drop the paragraph entirely:

Suggested change

	The hook is called immediately after reset so use stack wisely if
	the hook intends to return to continue with arch's resume/reset activity.

[nvlsianpu]

applied

[mathieuchopstm]

Nit: align lines to similar length (+ small reword):

Suggested change

	The stack pointer register should be considered as not initialized by the caller of
	this hook. In particular, this means that the hook is NOT allowed to
	"push" any data using this stack pointer value. However, the hook may use
	an implementation-specific area as stack if desired; in such case,
	the original value of the stack pointer needs not to be "restored"
	before returning control to the hook's caller.
	The stack pointer register should be considered as not initialized upon
	call to this hook. In particular, this means that the hook is NOT allowed
	to "push" any data using this stack pointer value. However, the hook may
	use an implementation-specific area as stack if desired; in such case,
	the original value of the stack pointer needs not to be "restored" before
	returning control to the hook's caller.

[nvlsianpu]

applied

[mathieuchopstm]

Also:

[mathieuchopstm]

Nit: I'd update SOC_RESET_HOOK while at it

config SOC_RESET_HOOK
-	bool "Run early SoC reset hook"
+	bool "Run SoC-specific reset hook"
	help
-	  Run an early SoC reset hook.
+	  Run a SoC-specific hook in the reset/startup code (__start).

-	  A custom hook soc_reset_hook() is executed at the beginning of the
-	  startup code (__start). soc_reset_hook() must be implemented by the SoC.
+	  A custom hook soc_reset_hook() will be executed near the beginning
+	  of the architecture-specific startup code, after hardware has been
+	  configured (as required by CONFIG_INIT_ARCH_HW_AT_BOOT), a stack 
+	  pointer has been loaded and the architecture's own resume logic
+	  has executed (if CONFIG_PM_S2RAM is enabled). Because this hook
+	  runs after the resume logic, it is not called when the system
+	  resumes from a suspend-to-RAM power state.

[nvlsianpu]

added as 2nd commit

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[andyross]

No reason not to +1 as this is harmless. But I think the real issue here is that there's tension between the "architecture reset vector", which is owned by arch/arm, and "platform reset", which is squishier and demands board-specific code.

On other architectures, the responsibility can be inverted: the SOC code takes the initial reset and calls into the OS when ready, using the arch layer as a "toolkit" for stuff that needs to be initialized but otherwise controlling the flow. The various adsp platforms look like this (because they have complicated initialization that actually starts across a PCI bus in a Linux driver). It might be something to consider more generally.