Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arch: arm: cortex_m: pm_s2ram: add support for ARMv6-M and ARMv7-M #76412

Merged
merged 4 commits into from
Nov 16, 2024

Conversation

mathieuchopstm
Copy link
Collaborator

This PR makes the ARM implementation of suspend-to-RAM compatible with the ARMv6-M and ARMv7-M architectures.

Tested on Cortex-M0+ (ARMv6-M) with a minimal example that invokes arch_pm_s2ram_suspend directly; CPU registers appear to be properly restored after resuming from the low-power mode.

Not tested on ARMv7-M due to lack of hardware.

@mathieuchopstm
Copy link
Collaborator Author

Rebased on main to trigger CI again (failure was due to faulty code in main).

@HesselM
Copy link
Contributor

HesselM commented Aug 12, 2024

I've done a very (limited) test on armv7-M (m4) and it seems to work. I can't do a full test as we are still working on fully implementing s2ram for our platform, which is the qn9090 soc from NXP.

@ithinuel
Copy link
Collaborator

I apologize for the delay in providing a review. I am not familiar with all the registers saved/restored in this module.

Some of the things that confuse me a bit are for example saving & restoring EPSR which cannot be accessed by MSR/MRS, and IPSR which is read only.

@mathieuchopstm
Copy link
Collaborator Author

I apologize for the delay in providing a review. I am not familiar with all the registers saved/restored in this module.

No problem 😄

Some of the things that confuse me a bit are for example saving & restoring EPSR which cannot be accessed by MSR/MRS, and IPSR which is read only.

I agree with you that some things performed by this module appear incorrect or useless, but I kept it as-is to not introduce functional changes. I'm fine with removing them though.

I am preparing a reworked version of this PR to make review easier and address the points raised by @HesselM ...

@mathieuchopstm mathieuchopstm force-pushed the pm_s2ram_armv6_compat branch 2 times, most recently from 2ed09e5 to 1fed460 Compare August 26, 2024 08:00
@mathieuchopstm
Copy link
Collaborator Author

Patch v2 (+ rebase on top of 2590c48):

  • split in separate commits to ease review
  • address issues pointed out by @HesselM

Copy link
Collaborator

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

Thank you Mathieu for the updated version.
If you don’t mind addressing the few points listed bellow, I think this will be in a good shape for approval 👍

arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
arch/arm/core/offsets/offsets_aarch32.c Outdated Show resolved Hide resolved
@mathieuchopstm
Copy link
Collaborator Author

mathieuchopstm commented Aug 27, 2024

Patch v3 (rebased on main, just in case...)

  • fix indentation mistake causing CI review comment
  • add commit that removes backup/restore of IPSR and EPSR
  • backup/restore Xsplim registers for ARMv8-M Baseline too (CONFIG_ARMV8_M_BASELINE)

arch/arm/core/offsets/offsets_aarch32.c Outdated Show resolved Hide resolved
ithinuel
ithinuel previously approved these changes Aug 27, 2024
Copy link
Collaborator

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

LGTM. Not actually tested myself on any hardware so we may have overlooked some corner cases. Still, this is already more portable than the current implementation 👍

@mathieuchopstm
Copy link
Collaborator Author

Noticed what seems like an error introduced in #77277 when rebasing:

https://github.com/zephyrproject-rtos/zephyr/pull/77277/files#r1732765743

@erwango erwango requested a review from HesselM August 28, 2024 09:50
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@mmahadevan108
Copy link
Collaborator

@HesselM, can you approve this PR if this looks good to you.

arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
arch/arm/core/cortex_m/pm_s2ram.S Outdated Show resolved Hide resolved
Remove all xPSR-related registers from struct __cpu_context, and the
associated save/restore code in S2RAM code, as they are not needed:

* EPSR and IPSR are read-only - they cannot be "restored"
* Bits N, V, Z, C, V, Q, and GE (if DSP Extension is implemented) of APSR
  could be restored, but this is not needed as the AAPCS indicates these
  bits to be "undefined on entry to or return from a public interface"

Signed-off-by: Mathieu Choplain <[email protected]>
Use macros to wrap the interaction between the assembly code and the
struct __cpu_context. This helps making the assembly more readable.

Signed-off-by: Mathieu Choplain <[email protected]>
Wrap the CPU register save/restore operations (GPR and special registers)
in macros to make core logic simpler to follow. This is also a preparatory
step to introduce ARMv6-M and ARMv7-M support.

Signed-off-by: Mathieu Choplain <[email protected]>
Extend the ARM M-profile suspend-to-RAM implementation to be compatible
with all versions of the M-profile supported by Zephyr: ARMv6-M, ARMv7-M,
and ARMv8-M Baseline.

Signed-off-by: Mathieu Choplain <[email protected]>
@mathieuchopstm
Copy link
Collaborator Author

Patch v2:

  • ignore APSR in addition to EPSR and IPSR
    • condition flags are volatile and don't need to be saved/restored
  • splim registers are now gated behind CONFIG_CPU_CORTEX_M_HAS_SPLIM
  • align style with the rest of the file
  • use C macros for everything for compatibility with all assemblers

Builds with dummy PM implementation for STM32WB0x series (ARMv6M / Cortex-M0+)

I did not incorporate any revert of 978e5b9 but wouldn't mind doing so.

@mrkhldn
Copy link
Contributor

mrkhldn commented Nov 12, 2024

Seems to work on M7

@dkalowsk dkalowsk added this to the v4.1.0 milestone Nov 16, 2024
@nashif nashif merged commit f27323a into zephyrproject-rtos:main Nov 16, 2024
22 checks passed
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.

9 participants