nRF5x: Fix WFE entry

[TacoGrandeTX]

• Remove check on Interrupt Control and State Reg (ICSR) as this could prevent low power entry
• Add DSB before WFE for architectural compliance
• Remove dead code for nRF51

Description

Gating entry to WFE on the ICSR can prevent the core from entering low power mode if there is a stale pending interrupt, so the check of the ICSR needs to be removed.

We should also add a DSB instruction before executing WFE to be architecturally compliant with the Arm architecture. This ensures that all outstanding memory transactions are complete before entering low power mode.

Some dead preprocessor FPU-related code was removed for the nRF51 (which is based on Cortex-M0).

See #5647 and #8224 for backing reference.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

[TacoGrandeTX]

@kjbracey-arm - sorry but old PR auto-closed on me so migrated it here

[pan-]

Gating entry to WFE on the ICSR can prevent the core from entering low power mode if there is a stale pending interrupt, so the check of the ICSR needs to be removed.

I must be missing something here but it seems this patch allows the core to sleep while there is interrupts pending. Going to sleep unconditionally is not the semantic expected by the sleep manager:

mbed-os/hal/mbed_sleep_manager.c

Lines 208 to 227 in 8bf51be

	void sleep_manager_sleep_auto(void)
	{
	#ifdef MBED_SLEEP_TRACING_ENABLED
	sleep_tracker_print_stats();
	#endif
	core_util_critical_section_enter();
	us_timestamp_t start = read_us();
	bool deep = false;
	// debug profile should keep debuggers attached, no deep sleep allowed
	#ifdef MBED_DEBUG
	hal_sleep();
	#else
	if (sleep_manager_can_deep_sleep()) {
	deep = true;
	hal_deepsleep();
	} else {
	hal_sleep();
	}
	#endif

Any interrupt raised between the entry of the critical section and the first call to __WFE won't get serviced until the processor is woken up from the second __WFE.

[pan-]

What happens to interrupts that became pending before the first call to __WFE ?

[kjbracey]

@pan- Mainly because others are already using the "SEV; WFE; WFE;" approach, I was assuming that the "SEVONPEND" meant that an interrupt being pending was a wakeup, rather than that the act of entering the pending state was the wakeup.

Reading and re-reading the WFE bits of the ARMv7 ARM and comparing with the WFI bits, I can't fully convince myself either way, but I agree it does tend to read more as "entering pending".

But that can't be right - it would be useless - there would be no race-free way to catch things entering pending just before WFE. Unless it actually sets the event register when entering pending, to get a subsequent WFE to exit immediately. But that's not what it says - it talks about it being a "wakeup event" which is distinct from the event register.

[pan-]

@kjbracey-arm The more I think about it, the more I wonder if it wouldn't be easier to have a single __WFE() statement as hal_sleep is called in an infinite loop.

That whole thing is confusing and hard to get right; it would be useful to establish patterns regards to WFE and WFI usage and validate them with our RTX and Cortex M experts so we can refer to these patterns when necessary.

[kjbracey]

I wonder if it wouldn't be easier to have a single __WFE() statement as hal_sleep is called in an infinite loop.

A single WFE statement is still unsafe, if "entering pending" is the wakeup. The exception could have started pending between the "core_util_enter_critical" (that masks application exceptions on this platform) and the WFE, so the WFE could have missed it.

If that was the case, the original if (IPSR says no exceptions pending) WFE; form would have reduced the race, but there would be still a 1-instruction (plus potential SoftDevice IRQ) race window between that test and the WFE.

I don't think there is a safe form unless "entering pending" sets the Event Register or "being pending" is a wakeup condition.

Yes, this needs referral - the only definitely correct patterns I know are WFE with IRQs enabled (SEVONPEND=0) and WFI with IRQs disabled. I've never seen SEVONPEND used.

[kjbracey]

Okay, I've had a moment of clarity. I had missed this important line from the ARMv7 ARM:

Any WFE wakeup event, or the execution of an exception return instruction, sets the Event Register

So it seems clear how it works to me now, and @pan- is almost certainly right, the SEV; WFE; WFE; sequence will swallow an event already raised by SEVONPEND - which effectively does what its name implies.

We need to start thinking of this and writing it like a WFE spinlock, not a WFI sleep, and it needs to be tolerant of spurious events.

I think a safe sequence is:

 // SEVONPEND must be set before core_util_enter_critical_section - do it on init?
 core_util_critical_section_enter(); // mask off mbed OS IRQs
 // enter hal_sleep()
 while (!(ICSR & MBED_OS_PENDING_MASK)) {
     DSB; WFE;
 }
 // exit hal_sleep()
 core_util_critical_section_exit(); // enable mbed OS IRQs, trigger pending

The loop does a few jobs:

• makes it tolerant of the Event Register being set on entry due to a recent SVC or whatever - it may well always be set in practice
• avoids unnecessary waking of mbed OS+RTX if the CPU is woken from the WFE for a non mbed-OS reason (eg SoftDevice IRQ)
• it also would hide wakes due to SEV from another CPU - you're not interested if idling; WFI would ignore them.

[TacoGrandeTX]

@pan On your original comment:

Any interrupt raised between the entry of the critical section and the
first call to __WFE won't get serviced until the processor is woken up from the second __WFE.

You are correct in that if an IRQ asserts after we have entered the critical region then it will be pended until we wake from the second WFE. So this adds latency to the servicing of that interrupt. We removed the check on the ICSR as we thought it wasn't needed (see dialogue here: #8224). However, in hindsight we do need it to avoid the extra latency.

@kjbracey-arm With SEVONPEND any new pending interrupt triggers an event and wakes up the processor, even if the interrupt is disabled or has insufficient priority to cause exception entry. Taken from:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/BABGGICD.html

If we do wake up for a new pending interrupt that won't get serviced that isn't optimal but no worse than we currently have.

One thing to bear in mind that this code only runs when the SoftDevice is not enabled. Otherwise all power management is handled by the SD. So we shouldn't care about SD IRQs... right?

Can you clarify what is the MBED_OS_PENDING_MASK?

For some historical context - most other devices don't use WFE for sleep, but the nRF5x seems to have done so from the beginning. If we want the change with least risk I suggest we go back to #8224 which only adds a DSB before the first SEV and changes the ICSR mask value:
#define SCB_ICSR_WFE_MASK (SCB_ICSR_VECTACTIVE_Msk | SCB_ICSR_VECTPENDING_Msk)

More aggressive measures can be taken later if deemed necessary.

[kjbracey]

Can you clarify what is the MBED_OS_PENDING_MASK?

Sorry, that's me getting confused - I was thinking that was a bitmask of which IRQs were pending, and hence you could limit it to mbed OS relevant things. I think maybe I've been confused on this point:

One thing to bear in mind that this code only runs when the SoftDevice is not enabled

If the SoftDevice isn't active for this case, then why the special handling at all? Can't you just use PRIMASK for critical section and then use WFI as normal?

Was my earlier understanding that you agreed with wrong? #8224 (comment)

Rereading the code, I'm wondering if core_util_critical_section_enter is actually a __irq_disable in this case.

If so, then we just need to get rid of all of this SEVONPEND stuff and make it a WFI.

You are correct in that if an IRQ asserts after we have entered the critical region then it will be pended until we wake from the second WFE.

My reading is that you won't wake from the second WFE.

WFI is a level-based wake. It is (roughly speaking) woken by an exception being asserted and ready to go as soon as PRIMASK is released. Races are avoided by PRIMASK being set while it is used.

WFE is an single-shot-event-based wake. It is (roughly speaking) woken by the Event Register being already set or becoming set. Races are avoided by the use of the Event Register to remember events. The SEV/WFE here clears the register, thus forgetting the already-occurred wakeup event.

My reading is that this is the sequence:

• Mess with exception enables (or is it just really PRIMASK=1?)
• IRQ Exception becomes pending - SEVONPEND makes this a wake event that sets the Event Register
• SEV - sets Event Register again
• WFE - clears Event Register
• WFE - Event Register is not set, so waits for the next IRQ

My original comments were based on the assumption that WFE with SEVONPEND must wake due to an exception /being/ pending, as that would be the only way to be race-free, if the pending exceptions weren't literally setting the Event Register.

However, that level-based behaviour would not be consistent with everything else about WFE, and I've now confirmed that exceptions entering pending do actually set the Event Register, like all WFE wake events. Given that, it seems clear that WFE is consistent, treating the entering as an event, as the very name "SEVONPEND" suggests. Forcibly clearing the Event Register with a SEV+WFE will undo the SEVONPEND.

[kjbracey]

For some historical context - most other devices don't use WFE for sleep, but the nRF5x seems to have done so from the beginning.

IIRC, we did change quite a few from WFE to WFI recently. I think nRF52 was left as-is due to the belief it had the special enter_critical behaviour.

Rereading again, the enter_critical is currently special whenever SOFTDEVICE_ENABLED is true, so the special WFE code is needed to cover the case where that is true and softdevice_handler_is_enabled() is false - I assume that can happen?

If you can't/don't want to change that case to use PRIMASK for enter_critical, so you need to use WFE not WFI, I'm still not happy with the original #8224 for code for a couple of reasons

• the SEV;WFE; approach is overkill, and broken if ever done as SEV;WFE;WFE; without a pending test. "Clearing" the event register is a bad habit, and I don't want to endorse the "SEV;WFE;" pattern. If you are having a test anyway, then the standard while (!condition) WFE; spinlock form does the job more simply, and with fewer bad effects in a general system.

• I'm also not 100% sure from docs that the "VECTPENDING" will work as you hope - the manual says "The exception number of the highest priority pending and enabled interrupt."

This knowledge base says "VECTPENDING is not intended for application code use cases; the design intent is to provide an indication during debug about what the processor expects to do next."

It would seem more logical to check the interrupt pending status for the things you disabled:

static inline bool app_irq_pending(void)
{
    MBED_ASSERT(hal_in_critical_section()); // so __irq_masks is meaningful
    return (NVIC->ICPR[0] & nrf_nvic_state.__irq_masks[0]) || 
           (NVIC->ICPR[1] & nrf_nvic_state.__irq_masks[1]);
}

while (!app_irq_pending())
    WFE;

That does the effective test I previously mistakenly tried to do with the ICSR - those __irq_masks are what I meant by "MBED_OS_PENDING_MASK"

[TacoGrandeTX]

@kjbracey-arm This has moved beyond my comfort zone / level of experience with Mbed. It's probably best if we shelve this PR and someone from the Core OS team makes another to handle this. I'll route the JIRA back through MBOTRIAGE with some additional notes.

This knowledge base says "VECTPENDING is not intended for application code use cases; the design intent is to provide an indication during debug about what the processor expects to do next."

I'll get clarification on that article as I have some contacts on the Cortex-M IP team. I'm going to go out on a limb and say the last sentence is not correct as this register lives in the System Control Space and the ARM states there are no usage constraints on the register. However in debug state some of the register fields might read differently.