nRF: Always use sd_nvic_critical_region calls

The motivation for doing this is so that we can allow
common_hal_mcu_disable_interrupts in IRQ context, something that works
on other ports, but not on nRF with SD enabled.  This is because
when SD is enabled, calling sd_softdevice_is_enabled in the context
of an interrupt with priority 2 or 3 causes a HardFault.  We have chosen
to give the USB interrupt priority 2 on nRF, the highest priority that
is compatible with SD.

Since at least SoftDevice s130 v2.0.1, sd_nvic_critical_region_enter/exit
have been implemented as inline functions and are safe to call even if
softdevice is not enabled.  Reference kindly provided by danh:
 https://devzone.nordicsemi.com/f/nordic-q-a/29553/sd_nvic_critical_region_enter-exit-missing-in-s130-v2

Switching to these as the default/only way to enable/disable interrupts
simplifies things, and fixes several problems and potential problems:
 * Interrupts at priority 2 or 3 could not call common_hal_mcu_disable_interrupts
   because the call to sd_softdevice_is_enabled would HardFault
 * Hypothetically, the state of sd_softdevice_is_enabled
   could change from the disable to the enable call, meaning the calls
   would not match (__disable_irq() could be balanced with
   sd_nvic_critical_region_exit).

This also fixes a problem I believe would exist if disable() were called
twice when SD is enabled.  There is a single "is_nested_critical_region"
flag, and the second call would set it to 1.  Both of the enable()
calls that followed would call critical_region_exit(1), and interrupts
would not properly be reenabled.  In the new version of the code,
we use our own nesting_count value to track the intended state, so
now nested disable()s only call critical_region_enter() once, only
updating is_nested_critical_region once; and only the second enable()
call will call critical_region_exit, with the right value of i_n_c_r.

Finally, in port_sleep_until_interrupt, if !sd_enabled, we really do
need to __disable_irq, rather than using the common_hal_mcu routines;
the reason why is documented in a comment.

2 files changed, 32 insertions, 25 deletions

diff --git a/ports/nrf/common-hal/microcontroller/__init__.c b/ports/nrf/common-hal/microcontroller/__init__.c
index f5caf68ef..06aac9409 100644
--- a/ports/nrf/common-hal/microcontroller/__init__.c
+++ b/ports/nrf/common-hal/microcontroller/__init__.c
@@ -50,36 +50,34 @@ void common_hal_mcu_delay_us(uint32_t delay) {
 
 static volatile uint32_t nesting_count = 0;
 static uint8_t is_nested_critical_region;
-static uint8_t sd_is_enabled = false;
 void common_hal_mcu_disable_interrupts() {
-    sd_softdevice_is_enabled(&sd_is_enabled);
-    if (sd_is_enabled) {
+    if (nesting_count == 0) {
+        // Unlike __disable_irq(), this should only be called the first time
+        // "is_nested_critical_region" is sd's equivalent of our nesting count
+        // so a nested call would store 0 in the global and make the later
+        // exit call not actually reenable interrupts
+        //
+        // This only disables interrupts of priority 2 through 7; levels 0, 1,
+        // and 4, are exclusive to softdevice and should never be used, so
+        // this limitation is not important.
         sd_nvic_critical_region_enter(&is_nested_critical_region);
-    } else {
-        __disable_irq();
-        __DMB();
-        nesting_count++;
     }
+    __DMB();
+    nesting_count++;
 }
 
 void common_hal_mcu_enable_interrupts() {
-    // Don't check here if SD is enabled, because we'll crash if interrupts
-    // were turned off and sd_softdevice_is_enabled is called.
-    if (sd_is_enabled) {
-        sd_nvic_critical_region_exit(is_nested_critical_region);
-    } else {
-        if (nesting_count == 0) {
-            // This is very very bad because it means there was mismatched disable/enables so we
-            // crash.
-            reset_into_safe_mode(HARD_CRASH);
-        }
-        nesting_count--;
-        if (nesting_count > 0) {
-            return;
-        }
-        __DMB();
-        __enable_irq();
+    if (nesting_count == 0) {
+        // This is very very bad because it means there was mismatched disable/enables so we
+        // crash.
+        reset_into_safe_mode(HARD_CRASH);
     }
+    nesting_count--;
+    if (nesting_count > 0) {
+        return;
+    }
+    __DMB();
+    sd_nvic_critical_region_exit(is_nested_critical_region);
 }
 
 void common_hal_mcu_on_next_reset(mcu_runmode_t runmode) {
diff --git a/ports/nrf/supervisor/port.c b/ports/nrf/supervisor/port.c
index aee2d63e1..e681e6825 100644
--- a/ports/nrf/supervisor/port.c
+++ b/ports/nrf/supervisor/port.c
@@ -313,12 +313,21 @@ void port_sleep_until_interrupt(void) {
         // instruction will returned as long as an interrupt is
         // available, even though the actual handler won't fire until
         // we re-enable interrupts.
-        common_hal_mcu_disable_interrupts();
+        //
+        // We do not use common_hal_mcu_disable_interrupts here because
+        // we truly require that interrupts be disabled, while
+        // common_hal_mcu_disable_interrupts actually just masks the
+        // interrupts that are not required to allow the softdevice to
+        // function (whether or not SD is enabled)
+        int nested = __get_PRIMASK();
+        __disable_irq();
         if (!tud_task_event_ready()) {
             __DSB();
             __WFI();
         }
-        common_hal_mcu_enable_interrupts();
+        if (!nested) {
+            __enable_irq();
+        }
     }
 }