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.

Sign up for GitHub

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

Jump to bottom

Arch arm aarc32 wfi barriers #23436

Closed
wants to merge 2 commits into from
Closed

Arch arm aarc32 wfi barriers #23436

wants to merge 2 commits into from

Conversation

ioannisg
Copy link
Member

No description provided.

@ioannisg ioannisg requested a review from stephanosio March 13, 2020 10:39
@ioannisg
Copy link
Member Author

@stephanosio can you check - maybe this whole thing does not apply for Cortex-R

@zephyrbot zephyrbot added the area: ARM ARM (32-bit) Architecture label Mar 13, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented Mar 13, 2020 •
edited
Loading

All checks passed.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

ioannisg added 2 commits March 13, 2020 12:48
@ioannisg
arch: arm: aarch32: add synchronization barriers before WFI
f743511 
Following ARM's recommendation, add memory and instruction
synchronization barriers before invoking WFI in arch_cpu_idle.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg
arch: arm: aarch32: correct documentation of arch_cpu_atomic_idle
b5189a7 
z_CpuIdleInit has been renamed to z_arm_cpu_idle_init, so
we need to correct that function's name in the documentation
of arch_cpu_atomic_idle.

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg ioannisg force-pushed the arch_arm_aarc32_wfi_barriers branch from 9983e3b to b5189a7 Compare March 13, 2020 11:50
@tagunil tagunil mentioned this pull request Mar 13, 2020
Closed
@stephanosio stephanosio mentioned this pull request Mar 17, 2020
Merged
@stephanosio
Copy link
Member

stephanosio commented Mar 17, 2020 •
edited
Loading

Rationale for DSB before WFI/WFE on Cortex-M:

  • "It is not a requirement for the processor hardware to drain any pending memory activity before
    suspending execution to enter a sleep mode. Therefore, software has to handle this by adding
    barrier instructions if the sleep mode used could affect data transfer. A DSB should be used to
    ensure that there are no outstanding memory transactions prior to executing the WFI or WFE
    instruction." [1]
  • "A DSB should be used after programming control registers, such as the System Control Register
    (SCR), when the side-effect is needed immediately. For example, a DSB is needed between the
    write to the SCS and the next operation if the SLEEPDEEP setting in the SCR is changed." [1]

Rationale for DSB before WFI/WFE on Cortex-A:

  • "The WFE (Wait For Event) and WFI (Wait For Interrupt) instructions enable you to stop execution
    and enter a low-power state. To ensure that all memory accesses prior to executing WFI or WFE
    have been completed (and made visible to other cores), you must insert a DSB instruction." [2]

Rationale for DSB before WFI/WFE on Cortex-R:

  • "The WFE (Wait For Event) and WFI (Wait For Interrupt) instructions enable you to stop execution
    and enter a low-power state. To ensure that all memory accesses prior to executing WFI or WFE
    have been completed (and made visible to other cores), you must insert a DSB instruction." [3]

[1] ARM AN 321
[2] ARM Cortex-A Series Programmer's Guide
[3] ARM Cortex-R Series Programmer's Guide

@stephanosio
Copy link
Member

Also note that adding ISB before WFI ensures that any pending interrupt is executed before going to sleep, and this will certainly cause the problem described in #23511.

stephanosio
stephanosio reviewed Mar 17, 2020
View reviewed changes
arch/arm/core/aarch32/cpu_idle.S
@@ -73,6 +73,12 @@ SECTION_FUNC(TEXT, arch_cpu_idle)
#error Unknown ARM architecture
#endif /* CONFIG_ARMV6_M_ARMV8_M_BASELINE */

/* Synchronization barriers before calling WFI
* DSB is recommented by architecture definitions
Copy link
Member

@stephanosio stephanosio Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

s/recommented/recommended/

After a quick review of the ARM docs, I would say this is "required," not just "recommended."

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this is only about AARCH32; actually I had only Cortex-M in mind. I interpreted "should" as "recommended" not "required". But it does not really make a difference, since we enforce it here anyways :)

arch/arm/core/aarch32/cpu_idle.S
* ISB flushes the instruction pipeline after setting PRIMASK/BASEPRI
*/
dsb
isb
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if ISB is necessary/required here.

Copy link
Member

Choose a reason for hiding this comment

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

Note that in the context of #23511, CPSID is guaranteed to be self synchronising and using ISB is not required in this particular instance.

Copy link
Member Author

@ioannisg ioannisg Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

agreed about CPSID, but not when setting BASEPRI - this needs ISB if you want to make sure BASEPRI is zero-ed before wfi is executed.

arch/arm/core/aarch32/cpu_idle.S
/* No BASEPRI, call wfe directly (SEVONPEND set in z_CpuIdleInit()) */
/* No BASEPRI, call wfe directly
* (SEVONPEND is set in z_arm_cpu_idle_init())
*/
wfe
Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, we should DSB here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure - can we take this in a separate discussion? This PR only about WFI. Let's fix one thing at a time :)

@ioannisg
Copy link
Member Author

Covered now in #23511

@ioannisg ioannisg closed this Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanosio stephanosio stephanosio left review comments

Assignees
No one assigned
Labels
area: ARM ARM (32-bit) Architecture
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ioannisg @zephyrbot @stephanosio