stm32-common/spi: allow custom pin modes on spi to minimize power consumption

[aabadie]

Contribution description

This PR is an attempt to provide a way to better optimize power consumption on STM32 by giving a finer control on the GPIO modes configured on each SPI pins (MOSI/MISO/SCLK).

All STM32 but STM32F1 support push-pull (GPIO_OUT) with a pull-up/pull-down configuration. In general, the default is to not use both at the same time but it appears that on STM32, not using pull-up/pull-down with push-pull results in higher power consumption.

Since the choice of pull-up/pull-down is highly correlated to the board configuration, this PR provides a way to configure this in the peripheral configuration struct for each configured SPI periph, for better flexibility. The feature is provided as an extension of periph_spi called periph_spi_gpio_mode (similiar periph_gpio_int with periph_gpio).

As an example, it's applied to nucleo-l152re but maybe this could be applied to all nucleo boards. At least the current approach allows to add this progressively.

Testing procedure

Build and flash an application using SPI and verify that it still work. Another interesting test could be to measure power consumption of this application when the board goes to deep sleep. With this PR, the minimal power consumption is reached, with master, it's always noticeably greater.

Issues/PRs references

Replaces #11384

[aabadie]

Maybe @haukepetersen could give some input on this, since he worked a lot on SPI some time ago.

[fjmolinas]

Although PUSH-PULL shouldn't need PU/PD to achieve GND/VDD I've seen that in stm32 boards when GPIO's are defined as AF an that AF is deactivated (e.g. releasing SPI) the pins do no go completely to GND or VDD. This means that although keeping their inactive state should be enough to minimize consumption some current leak is seen because of the GPIOS having a voltage of ~1mV instead of 0. So I agree with the idea behind this PR although it might only be useful for stm32 (don't know if this problem is present in other families).

I'm not sure on where pin modes are handled though, see comments below.

[fjmolinas]

I think by default this should be set as it is now, so:

        .mosi_pin_mode = (GPIO_OUT),
        .miso_pin_mode = (GPIO_IN),
        .sclk_pin_mode = (GPIO_OUT),

But in concerned driver add FEATURES_OPTIONAL = periph_spi_gpio_mode. And change the gpio mode accordingly.

IMO having it as a default configuration for a board isn't the best. If an spi device uses CPOL=1 you would want to have a PU and not PD. A driver could have pull ups or pull down on the board witch could affect everything, for example some devices can operate ass SPI/I2C and would then have pull ups, if you pull it down by default the current leak will be higher.

I think you could implement this in a way where a device driver can modify the default mosi/miso/sclk pin mode. I'm not sure how this would be implemented... I can think of using CFLAGS (not ideal) or adding an spi function that changes the pins modes having a separate structure for this?

void spi_init_pins_mode(spi_t bus, spi_pin_mode_t mode)
{
    spi_config[bus].mosi_pin_mode = mode.mosi;
    spi_config[bus].miso_pin_mode = mode.miso;
    spi_config[bus].sclk_pin_mode = mode.sclk;
}

// or just

void spi_init_pins_mode(spi_t bus, spi_pin_mode_t mode)
{
    gpio_init(spi_config[bus].mosi_pin, mode.mosi);
    gpio_init(spi_config[bus].miso_pin, mode.miso);
    gpio_init(spi_config[bus].sclk_pin, mode.sclk);
}

There could still be conflicts between spi devices but that is up to the user to realize that using spi_devices with different inactive state is not a good idea.

What do you think?

[aabadie]

What do you think?

I like the idea. I'll provide a patch to this PR.

[aabadie]

@fjmolinas, I extended the SPI interface with a new function spi_init_gpio_mode() (better suggestion welcome) and let the driver manage the right gpio mode directly. It should still work.
Unfortunately, with the current approach, boards still have to explicitly provide periph_spi_gpio_mode. The change is transparent for boards that doesn't expose this. Only nucleo-l151re provides this but we should adapt all LoRa capable boards if we keep things this way.

Let me know what you think of the current strategy.

[fjmolinas]

@aabadie LGTM, I'll test when I can (probably Friday). Maybe someone might give another idea in the meantime.

[fjmolinas]

I tested it on nucleo-l073rz + mbed sx1272 shield but it doesn't work :(. Setting the SPI settings after initializing the the GPIO's functions breaks the AF initialization somehow. But if you re-initiliaze the AF settings after the GPIO's modes it works! Consumption goes from ~90uA to ~10ua for the while system.

[aabadie]

@fjmolinas, sorry for replying late. I finally opted for reinitializing the af from the spi_init_with_gpio_mode function. Let me know what you think.

[fjmolinas]

I tested on nucleo-l073rz + sx1272 mbed shield. I like the latest version where the pins mode is defined per driver and not per board.

I applied this diff to examples/lorawan and was getting 12uA consumption when in stop mode mode (I also added CFLAGS=-DDISABLE_LORAMAC_DUTYCYCLE since the timers are not running in sleep mode):

diff --git a/examples/lorawan/Makefile b/examples/lorawan/Makefile
index 8265c73ee..23d40c6a9 100644
--- a/examples/lorawan/Makefile
+++ b/examples/lorawan/Makefile
@@ -25,6 +25,7 @@ USEPKG += semtech-loramac
 USEMODULE += $(DRIVER)
 USEMODULE += fmt
 FEATURES_REQUIRED += periph_rtc
+FEATURES_OPTIONAL += periph_eeprom
 
 CFLAGS += -DDEVEUI=\"$(DEVEUI)\" -DAPPEUI=\"$(APPEUI)\" -DAPPKEY=\"$(APPKEY)\"
 
diff --git a/examples/lorawan/main.c b/examples/lorawan/main.c
index 20cc066f3..a73ecaceb 100644
--- a/examples/lorawan/main.c
+++ b/examples/lorawan/main.c
@@ -26,6 +26,7 @@
 #include "msg.h"
 #include "thread.h"
 #include "fmt.h"
+#include "periph/pm.h"
 
 #include "periph/rtc.h"
 
@@ -52,6 +53,7 @@ static void rtc_cb(void *arg)
     (void) arg;
     msg_t msg;
     msg_send(&msg, sender_pid);
+    pm_block(1);
 }
 
 static void _prepare_next_alarm(void)
@@ -62,6 +64,8 @@ static void _prepare_next_alarm(void)
     time.tm_sec += PERIOD;
     mktime(&time);
     rtc_set_alarm(&time, rtc_cb, NULL);
+    semtech_loramac_save_config(&loramac);
+    pm_unblock(1);
 }
 
 static void _send_message(void)
@@ -122,7 +126,8 @@ int main(void)
      * keys.
      */
     puts("Starting join procedure");
-    if (semtech_loramac_join(&loramac, LORAMAC_JOIN_OTAA) != SEMTECH_LORAMAC_JOIN_SUCCEEDED) {
+    uint8_t ret = semtech_loramac_join(&loramac, LORAMAC_JOIN_OTAA);
+    if ((ret != SEMTECH_LORAMAC_JOIN_SUCCEEDED) && (ret != SEMTECH_LORAMAC_ALREADY_JOINED)) {
         puts("Join procedure failed");
         return 1;
     }

IMO this is a good change and although I would like more feedback on the api change this has been going for a while so lets not stall this any longer. I think something like this is necessary for proper low-power.

I would move for now the definition of spi_init_with_gpio_mode to stm32_common/include/periph_cpu_common, and make sure it shows up in doxygen. Once this is done please squash, there are some commits that override each other.

[fjmolinas]

I re-tested applying the same patch. I can see the application working correctly and that the sx272 shield goes down to 8.6uA in sleep mode while de cpu goes down to 1.2uA, the message is correctly sent on every wake-up.

examples/lorawan → DRIVER=sx1272 CFLAGS= DDISABLE_LORAMAC_DUTYCYCLE make BOARD=nucleo-l073rz flash term

2019-08-05 16:27:41,057 - INFO # main(): This is RIOT! (Version: 2019.10-devel-231-g38e53-pr-11542)
2019-08-05 16:27:41,061 - INFO # LoRaWAN Class A low-power application
2019-08-05 16:27:41,064 - INFO # =====================================
2019-08-05 16:27:41,119 - INFO # Starting join procedure
2019-08-05 16:27:41,121 - INFO # Join procedure succeeded
2019-08-05 16:27:41,124 - INFO # Sending: This is RIOT!
2019-08-05 16:28:02,265 - INFO # Sending: This is RIOT!
2019-08-05 16:28:23,406 - INFO # Sending: This is RIOT!
2019-08-05 16:28:44,548 - INFO # Sending: This is RIOT!
2019-08-05 16:29:05,690 - INFO # Sending: This is RIOT!
2019-08-05 16:29:26,831 - INFO # Sending: This is RIOT!
2019-08-05 16:29:47,973 - INFO # Sending: This is RIOT!
2019-08-05 16:30:09,115 - INFO # Sending: This is RIOT!

I also ran the tests in #11915, and all were passing. This looks good to me.
Looking at other CPU's sam0 don't seem to support push-pull with PU/PD but nrf52 do, so I'm ok with the function being in spi.h. ACK