sys/pm_layered: use array representation, get rid of implicit IDLE mode

[benpicco]

Contribution description

PM_NUM_MODES is confusing as it must not include the default IDLE mode.
In #17883 I added the IDLE mode to PM_NUM_MODES of samd5x so it can be blocked too (falling back to polling the CPU), which introduced an introducing.

I think this was done to fit all blockers for the modes into an uint32_t, but is is entirely unnecessary as the blockers are typically accessed as an array anyway.

So always make pm_blocker_t an array to remove the limitation to 4 modes.

Testing procedure

This should retain the current behavior. samd5x was not changed as it already implements the new convention.
native was accidentally already using the new convention, causing it not to enter sleep at all with the current implementation. As a side effect, this is now fixed by this PR.

Issues/PRs references

#13475

[kaspar030]

I think this was done to fit all blockers for the modes into an uint32_t, but is is entirely unnecessary as the blockers are typically accessed as an array anyway.

well, this was done to minimize the irq off phase, and handle blocker changes that happen while pm_set_lowest() is figuring out which mode to set, efficiently.
the current blocker value is copied into a local variable (hopefully into a register), the mode is figured out, then the blocker value is compared again to see if it has changed in the meantime, and the mode is not set in that case.
I wanted to minimize the cases where the scheduler goes into deep-sleep, just to wake up right away.
Maybe premature optimization.

Can we measure the time spent in pm_set_lowest() before and after this PR?

[benpicco]

It's actually a bit faster with the new code:

master

diff --git a/sys/pm_layered/pm.c b/sys/pm_layered/pm.c
index 2d2b0e0aec..27195c21c8 100644
--- a/sys/pm_layered/pm.c
+++ b/sys/pm_layered/pm.c
@@ -44,6 +44,8 @@ static pm_blocker_t pm_blocker = { .val_u32 = PM_BLOCKER_INITIAL };
 
 void pm_set_lowest(void)
 {
+    LED1_ON;
+
     pm_blocker_t blocker = { .val_u32 = atomic_load_u32(&pm_blocker.val_u32) };
     unsigned mode = PM_NUM_MODES;
     while (mode) {
@@ -57,9 +59,11 @@ void pm_set_lowest(void)
     unsigned state = irq_disable();
     if (blocker.val_u32 == pm_blocker.val_u32) {
         DEBUG("pm: setting mode %u\n", mode);
+        LED1_OFF;
         pm_set(mode);
     }
     else {
+        LED1_OFF;
         DEBUG("pm: mode block changed\n");
     }
     irq_restore(state);

this PR

diff --git a/sys/pm_layered/pm.c b/sys/pm_layered/pm.c
index f5645c88f2..d3c798c326 100644
--- a/sys/pm_layered/pm.c
+++ b/sys/pm_layered/pm.c
@@ -59,6 +59,7 @@ static pm_blocker_t pm_blocker = { .val_u8 = PM_BLOCKER_INITIAL };
 
 void pm_set_lowest(void)
 {
+    LED1_ON;
     unsigned mode = PM_NUM_MODES;
 
     /* set lowest mode if blocker is still the same */
@@ -71,6 +72,7 @@ void pm_set_lowest(void)
     }
 
     if (mode != PM_NUM_MODES) {
+        LED1_OFF;
         pm_set(mode);
     }
     irq_restore(state);

I tested with examples/timer_periodic_wakeup on same54-xpro where I set the CPU to run at 1.2 MHz and defined LED1 as a spare GPIO that I connected to the scope.

same54-xpro

diff --git a/boards/same54-xpro/include/board.h b/boards/same54-xpro/include/board.h
index 45f57e455f..9d90648455 100644
--- a/boards/same54-xpro/include/board.h
+++ b/boards/same54-xpro/include/board.h
@@ -59,6 +59,13 @@ extern "C" {
 #define LED0_TOGGLE         (LED_PORT.OUTTGL.reg = LED0_MASK)
 /** @} */
 
+#define LED1_PIN            GPIO_PIN(PC, 7)
+#define LED1_MASK           (1 << 7)
+#define LED1_ON             (LED_PORT.OUTSET.reg = LED1_MASK)
+#define LED1_OFF            (LED_PORT.OUTCLR.reg = LED1_MASK)
+#define LED1_TOGGLE         (LED_PORT.OUTTGL.reg = LED1_MASK)
+
+
 /**
  * @name SW0 (Button) pin definitions
  * @{
diff --git a/boards/same54-xpro/include/periph_conf.h b/boards/same54-xpro/include/periph_conf.h
index fbd66a1024..086e4c4a9a 100644
--- a/boards/same54-xpro/include/periph_conf.h
+++ b/boards/same54-xpro/include/periph_conf.h
@@ -33,7 +33,7 @@ extern "C" {
  *          frequency to 12 MHz.
  */
 #ifndef USE_XOSC_ONLY
-#define USE_XOSC_ONLY       (0)
+#define USE_XOSC_ONLY       (1)
 #endif
 
 /**
@@ -49,7 +49,7 @@ extern "C" {
  */
 #ifndef CLOCK_CORECLOCK
 #if USE_XOSC_ONLY
-#define CLOCK_CORECLOCK     XOSC1_FREQUENCY
+#define CLOCK_CORECLOCK     XOSC1_FREQUENCY/10
 #else
 #define CLOCK_CORECLOCK     MHZ(120)
 #endif

[maribu]

ACK. Looks good to me and doesn't change the logic for when to go to sleep and when not.

[kaspar030]

I tested with examples/timer_periodic_wakeup on same54-xpro where I set the CPU to run at 1.2 MHz and defined LED1 as a spare GPIO that I connected to the scope.

👍

Thanks for testing!

[benpicco]

@OlegHahm can we have this in for the release? It does not change behavior but makes it more consistent.

[maribu]

I don't get this. Why would one decrement a ref counter with no coordination?

It triggers the assertion in pm.c on non-STM32 boards right away, because not every ref counter is higher than 1. But this is bound to break any periph driver than explicitly requested a certain clock.

We should rather try to adapt the periph_uart implementation to not keep a clock required after TX is completed when no RX callback is set.