cpu/sam0*: enhance power saving by switching EIC clock source during STANDBY mode

[jue89]

Contribution description

This is a follow up PR of issue #13406.

As discussed, the 16MHz clock is set to ONDEMAND mode, so the main clock is only active if it is actually required by the system.

The External Interrupt Controller (EIC) is clocked with the low power 32kHz clock in STANDBY mode.

Testing procedure

diff --git a/cpu/saml21/periph/pm.c b/cpu/saml21/periph/pm.c
index ff2e3c5e9..4147afb73 100644
--- a/cpu/saml21/periph/pm.c
+++ b/cpu/saml21/periph/pm.c
@@ -21,7 +21,7 @@
 
 #include "periph/pm.h"
 
-#define ENABLE_DEBUG (0)
+#define ENABLE_DEBUG (1)
 #include "debug.h"
 
 void pm_set(unsigned mode)
diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9ba..5f5d3447a 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -12,6 +12,9 @@ RIOTBASE ?= $(CURDIR)/../..
 # development process:
 DEVELHELP ?= 1
 
+USEMODULE += periph_gpio_irq
+
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1
 
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a..bdce69402 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,15 @@
  */
 
 #include <stdio.h>
+#include "board.h"
+#include "periph/pm.h"
+#include "periph/gpio.h"
+
+void btn (void * ctx)
+{
+    (void) ctx;
+    puts("Huh?");
+}
 
 int main(void)
 {
@@ -28,5 +37,9 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);
 
+    gpio_init_int(BTN0_PIN, BTN0_MODE, GPIO_FALLING, btn, NULL);
+
+    pm_set(1);
+
     return 0;
 }

2020-02-19 18:48:31,966 # main(): This is RIOT! (Version: 2018.10-RC1-6870-gae2b8-feature/saml21-pm-low-power)
2020-02-19 18:48:31,967 # Hello World!
2020-02-19 18:48:31,972 # You are running RIOT on a(n) samr30-xpro board.
2020-02-19 18:48:31,975 # This board features a(n) saml21 MCU.
2020-02-19 18:48:31,978 # pm_set(): setting STANDBY mode.
2020-02-19 18:48:31,981 # pm_set(): reduce EIC clock speed
2020-02-19 18:48:35,580 # Huh?
2020-02-19 18:48:35,584 # pm_set(): switch EIC clock back to fast speed
2020-02-19 18:48:37,097 # Huh?
2020-02-19 18:48:37,250 # Huh?

Issues/PRs references

Issue #13406

[dylad]

I understand the need for this PR but I think the EIC clock input source should be configurable by user (in periph_conf.h ?)

Moreover this could also work for the BACKUP mode IIRC.

Maybe we could create one define per sleep mode to allow clock source selection per sleep mode ?

#define INPUT_CLK_IDLE (ULP32K or GCLK_EIC)
#define INPUT_CLK_STANDBY (ULP32K or GCLK_EIC)
#define INPUT_CLK_BACKUP  (ULP32K or GCLK_EIC)

With better names of course but that's the spirit. What do you think ?

[jue89]

Thank you for your feedback!

The EIC is located in the power domain PDTOP. If I understand the datasheet correctly, PDTOP is switched off in BACKUP mode. If we want wake-up events in BACKUP mode raised from external pins, we have to use some special pins that are connected to the RSTC. Long story short, we do not need INPUT_CLK_BACKUP.

Furthermore, I thought about the clock selection for IDLE mode. Can you come up with a use case were it is useful to drive the EIC with the slow clock? Is this really required?

I would rather make the switch to ULP32K in STANDBY mode optional. This could be beneficial for applications where the current draw isn't a problem at all. (But on the other hand: why would they go into STANDBY mode?) I don't know, how many cycles are lost for the switch. We are also waiting for the EIC to sync our requested change into its clock domain (while (EIC->SYNCBUSY.bit.ENABLE) {}).

[benpicco]

I think the EIC clock input source should be configurable by user

Mind you that added configuration options add complexity as you end up with more configuration permutations. IMHO it should best be avoided if possible.

Dynamic clock switching should yield the best of both world: You get to detect high frequency events in RUN mode and power savings in STANDBY and wake from BACKUP.

[roberthartung]

I agree with @benpicco here. Power-Saving is always hard, especially if there are many configuration options. Therefore we should have less options and save as much power as possible.

[roberthartung]

This code looks rather similar to the code below. Can we make it an inline function or are there already macros for enable-protected registers?

[benpicco]

I think it would make more sense for this to live inside sam0_common/periph/gpio.c, this should work the same for all sam0s other than samd2x - also in preparation to what is proposed in #13422

[dylad]

How do you want to implement it within sam0_common/periph/gpio.c ?

[jue89]

I would define a function in cpu/sam0_common/include/periph_cpu_common.h that is called in cpu/caml21/periph/pm.c and implemented in sam0_common/periph/gpio.c?! I think this is the solution with the least overhead.

Another solution could be some kind of registry inside of periph/pm.c that holds all required callbacks. sam0_common/periph/gpio.c would register a callback on startup. But this would imply some overhead every time pm_set() is called.

What solution would you prefer @benpicco?

[benpicco]

I had the first option in mind too.
I don't think a dynamic registry is needed here - something like what is used for auto_init would do. (Basically what you already did there)

[roberthartung]

I was talking about a macro here to avoid code duplication as above. I guess there might be more use cases for that macro at some point.

Not sure why we switched to callbacks here?

[jue89]

The code is more related to the GPIO peripheral that must be called upon PM changes. So from a logical point of view this make sense to me to put it in periph/gpio.c.

I already have another PR in mind: We need to mux the SPI pins back to GPIO function before switching to STANDBY oder BACKUP mode. The concept of calling certain callbacks makes sense, imho. Otherwise I have to patch this also inside of pm.c and clutters the whole power management.

What are your concerns regarding this callback implementation?

[roberthartung]

That's my point. I am not talking about the callbacks in any way. I was reviewing the code as is. Which shows code duplication which can be solved by a nice macro and used elsewhere.

If you want to replace the above code with a callback, that's fine, but then the code will simply be added elsewhere and still be duplicated.

[jue89]

Oh now I get you ... sry! Of course, I took your review serious and de-duplicated that bit. I implemented reenable_eic(clock). Thank you for your review. I really appreciate it!

@benpicco and I focused on the question where to put the implementation (with or without code de-duplication). That's why we started talking about something more or less out of focus. We already had some conversation about this topic in other PRs and issues.

[dylad]

The EIC is located in the power domain PDTOP. If I understand the datasheet correctly, PDTOP is switched off in BACKUP mode.

You're right, so there is no need for configuration.

[jue89]

I pushed a fixup ... could you have a look into it, @benpicco? Does this match with the concept you had in mind?

[jue89]

I just force pushed a new approach. I think we should first discuss the overall concept: I wrapped the coretexm_sleep() call for all CPUs relying on cpu/sam0_common. This enables me to call callback functions before and after the Cortex goes sleeping.

If the GPIO peripheral driver is compiled in, its callback functions are called and the driver has the opportunity to switch clocks of the EIC.

If the proposed concept is accepted, I have another PR in mind touching periph/spi.c ... but one step at a time.

Thank you all for your hints and feedback!

[benpicco]

I tested this on same54-xpro, saml21-xpro and samr21-xpro and I get power savings in STANDBY across the board.

On saml21-xpro I get 1.8µA in STANDBY with full memory retention & wake on GPIO.

The structure of sam0_cortexm_sleep() will allow to extend this approach to more peripherals in the future.

[benpicco]

Please squash!

[jue89]

Thank you very much! This merge helped me a lot to move closer to RIOT upstream with the project I am currently working on :)