stm32l1/pm: add low power modes for stm32l1 #8403

fjmolinas · 2018-01-22T18:56:58Z

Contribution description

This PR is mostly based on work by @olegart (permission to take over and pr given offline), it adds stand_by, stop and sleep mode for stm32l1. Tested on nucleo-l152, achieves 0.6uA on stop_mode, 0.3uA on stand_by and 50uA on sleep_mode.

Issues/PRs references

Depends on #8402 and #8401

kYc0o

I have some comments in the concepts introduced here. I'm not sure if putting everything systematically in low power before sleep is the way to go, but I feel that is a bit overkill.

kYc0o · 2018-01-22T22:21:38Z

cpu/stm32_common/periph/pm.c

+        port->ODR &= ~(1 << pin_num);
+    }
+
+    // lpm_portmask_system[((uint32_t)&port>> 10) & 0x0f] |= 1 << pin;


Please remove C++ comment style.

kYc0o · 2018-01-22T22:23:59Z

cpu/stm32_common/periph/pm.c

+        lpm_gpio_pupdr[i] = port->PUPDR;
+        lpm_gpio_otyper[i] = (uint16_t)(port->OTYPER & 0xFFFF);
+        lpm_gpio_odr[i] = (uint16_t)(port->ODR & 0xFFFF);
+	}


Are you sure you want to put ALL ports in low power? It seems to me a bit overkill and might slow down the process of sleeping. I think it can be better to leave this option to the user who will set the power modes of the peripherals he/she uses.

kYc0o · 2018-01-22T22:30:13Z

cpu/stm32_common/periph/pm.c

+}
+
+/* restore GPIO settings */
+static void lpm_when_i_wake_up (void) {


Same here as in the "prepare for sleep" function. Better to configure each peripheral separately and not all at once.

kYc0o · 2018-01-22T22:32:15Z

cpu/stm32_common/periph/pm.c

+	lpm_portmask_user[port] &= ~(uint16_t)(1<<pin);
+}
+
+void lpm_before_i_go_to_sleep(void){


Maybe you can add in the API the special action to take for the peripheral you want to put "ready to sleep". I know this had some discussion in the past to try to generalise this concept but there's no consensus afair.

kYc0o

There are some style comments and code duplication but looks good so far. Will test soon.

kYc0o · 2018-01-22T22:34:39Z

cpu/stm32l1/periph/pm.c

+ * @}
+ */
+
+#include "cpu.h" // For numbers of ports


Please remove C++ comment.

kYc0o · 2018-01-22T22:35:01Z

cpu/stm32l1/periph/pm.c

+ * @name Available power modes
+ */
+enum pm_mode {
+    PM_OFF = 0,            /**< MCU is off */


Please align doxygen comments.

kYc0o · 2018-01-22T22:36:32Z

cpu/stm32l1/periph/pm.c

+    port->OTYPER &= ~(1 << pin_num);
+    if (value) {
+        port->ODR |= (1 << pin_num);
+    } else {


else { goes at the next line. See our coding conventions for more information.

kYc0o · 2018-01-22T22:37:30Z

cpu/stm32l1/periph/pm.c

+        int pin_num = _pin_num(spi_config[i].sclk_pin);
+
+        /* check if alternate functions are enabled for that device*/
+        if (((port->AFR[(pin_num > 7) ? 1 : 0] >> (pin_num * 4)) & 0x0f)  == spi_config[i].af) {


This line is very confusing. Can you factor it out a bit?

This one is to check that SPI port is actually in use (corresponding pin is in AF mode) before reconfiguring it.

But the more I think of it — the more I tend to agree with you the code is too complicated and mostly unnecessary to achieve low power.

Thanks for the explanation! But what I meant is that there is an usage of the ternary operator inside an if statement, which makes the whole line a bit complicated to read, plus the 4 which I don't really know what it means.

Regarding the function, I wouldn't say it's unnecessary, the function can be very helpful to configure the low power mode on this peripheral but what it's overhead is to do it systematically, regardless of the current configuration a user might have done.

My point is, there is actually no need to configure peripherals for low power on STM32. Clocks are stopped in STOP mode, peripheral controllers are stopped, and GPIOs will preserve the state they were left in.

This function was to set SPI GPIO state, which is unnecessary as the same state should be already set by SPI controller itself, and it's not changing when entering STOP or SLEEP.

So the code can be removed.

Ok I got it, thanks again for the clarification!

kYc0o · 2018-01-22T22:38:46Z

cpu/stm32l1/periph/pm.c

+    xtimer_init();
+#endif
+
+    // /* Recalculate stdio UART baudrate */


Please remove //.

kYc0o · 2018-01-22T22:42:10Z

cpu/stm32l1/periph/pm.c

+
+            /* Request Wait For Interrupt */
+            __DSB();
+            __WFI();


Some parts of the code above are already implemented on cortexm_sleep function. Please remove the duplicates and call the function instead. The cases below too.

In cortexm_sleep it is followed by irq_restore(state);

As a rule, you want to restore core and peripheral clocks before enabling and processing any interrupts, especially when waking up from deep sleep mode with its 65 kHz core clock. So either clock switching should be added to cortexm_sleep or irq_restore removed from it.

I have the same issue as stated by @olegart

You can still do it when the function cortexm_sleep returns, as other implementations do if I'm not mistaken. However I think there is a problem using irq_restore since on current master is broken for nucleo-l152. My thoughts were that lot's of things were not implemented (power modes, clocking, etc.) so the problem could be anywhere.

I'll try to make more testing on this and figure out where it breaks and see if we can still reuse the code from cortex_common, which is the ideal.

CPU was woken from sleep by some interrupt and it will enter IRQ routine immediately after irq_restore, and with current cortexm_sleep it will happen before clocks are restored.

If it was STOP mode, default clock after it is 4 MHz MSI, so probably you wouldn't notice some slowdown inside IRQ processing, but when using DEEP SLEEP, core clock after wakeup may be as low as 65 kHz (it should be set before entering DEEP SLEEP and it affects sleep mode power consumption).

Been there, done that :) The clock must be restored before re-enabling interrupts.

If you get a HardFault right after __WFI(), it's a problem of clock initialization routine (clk_init), we had that problem with RIOT some time ago — somewhere around end of 2016, I believe, and then we rewrote clk_init for our needs, all the sequence strictly according to RM0038, and the problem was gone.

Yes. Just tested it and I found that. Interestingly the problem is not present on other devices, and I was also wondering if the missing configuration of FAULTMASK (see comment) was also in the origin of the problem.

Thus, I agree to keep this code, but maybe we can factor out the calls to __DSB() and __WFI() which belong to the Cortex-M architecture and are independent of the manufacturer (ST here).

Actually, in cortexm_sleep, the irq_restore call enables irq only if pm_set was called with interrupt enabled, which is not the case if called from pm_layered (

RIOT/sys/pm_layered/pm.c

Line 61 in ae437cd

unsigned state = irq_disable();

).
The behavior you describe (interrupt being served before re-enabling clocks) is fixed since #7385, but this is not the case anymore.

@kYc0o something like a cortexm_wfi() ?

I was referring to what @vincent-d just said.

But in that case there would be a bunch a irq_disable/restore calls, 3 of each if using pm_layered. Addressed it anyhow.

kYc0o · 2018-01-22T22:45:17Z

cpu/stm32_common/periph/pm.c

@@ -29,6 +29,8 @@
 #include "stmclk.h"
 #endif

+extern void clk_init(void);


As long as I can see, this function doesn't exist anymore.

It does for L-series CPUs, although defined as static. For F-series, clock initialization was moved to stmclk

Yep, forgot to delete

kYc0o · 2018-01-22T22:47:17Z

Please prefix your commits and (un)squash accordingly. It's a bit complicated to review commits which are being modified by subsequent commits.

olegart · 2018-01-22T23:48:04Z

Hi @fjmolinas. Thanks for PR, I'm not sure if we'll ever get to it ourselves :)

But the more I think of it, the more I tend to agree with @kYc0o that our code is overcomplicated in before_i_go_to_sleep and when_i_wake_up functions. What we did is we tried to automatically reconfigure all GPIO to minimize power consumption:

unused GPIOs should be configured as AIN
interfaces' outputs should be configured as DOUTs with inactive state (1 for UART TX, 1 for SPI CS, 0 for MOSI, etc)
GPIOs with registered interrupts should be left as is

But now it seems to me that was a safe (and it works!) but ineffective way. The better way is

All GPIOs should be configured as AIN on startup to minimize power losses on floating pins.
Interfaces-related inputs (SPI MISO, UART RX, etc) should be configured with pull-ups/pull-downs according to inactive state — otherwise, e.g. if you have console UART RX in DIN mode without pull-up it will be floating when no UART device is connected to it. This should be done by corresponding drivers.
Interfaces-related outputs (SPI CLK/CS/MOSI, UART TX, etc.) will keep their state in STOP and all SLEEP modes, so no need to reconfigure it.
User should take care of any GPIO he configured manually.

So, to make it short:

throw away our before-sleep/after-sleep code
in cpu_init:

    /* switch all GPIOs to AIN mode to minimize power consumption */
    uint8_t i;
    GPIO_TypeDef *port;
    for (i = 0; i < cpu_ports_number; i++) {
        port = (GPIO_TypeDef *)(GPIOA_BASE + i*(GPIOB_BASE - GPIOA_BASE));
        port->MODER = 0xffffffff;
}

change UART, SPI and other related drivers (except I2C, it has external pull-ups anyway) to enable pull-ups/pull-downs on their input GPIOs

This should work as well, at least now (3 am in the morning, but anyway...) I can't see why it shouldn't. As I said, our code was a safe way to go, but it really is overcomplicated.

olegart · 2018-01-22T23:57:25Z

BTW, CPU_NUMBER_OF_PORTS doesn't have to be defined, there's a better way — it can be calculated by probing corresponding addresses while masking BusFault interrupt (probing non-existing peripheral address will trigger it).

Here is cpu_check_address probing function: https://github.com/unwireddevices/RIOT/blob/loralan-public/cpu/cortexm_common/cortexm_init.c

Here is implementation: https://github.com/unwireddevices/RIOT/blob/loralan-public/cpu/stm32l1/cpu.c

/* determine available ports */
    cpu_ports_number = 0;
    for (uint32_t i = GPIOA_BASE; i<=GPIOG_BASE; i += (GPIOB_BASE - GPIOA_BASE)) {
        if (cpu_check_address((char *)i)) {
            cpu_ports_number++;
        } else {
            break;
        }
}

(there are functions to determine RAM, flash, and EEPROM sizes as well)

It works on any Cortex-M3/Cortex-M4 CPU. On Cortex-M0 it doesn't as it has no BusFault, but I believe we'll implement it there using HardFault and some assembler magic too.

fjmolinas · 2018-01-23T15:06:51Z

@olegart In regards to the cpu_number_of_ports thing, it's true it can be done that way but that means exposing a global variable witch isn't ideal, what do you think @kYc0o better to have a define or use @olegart suggestion.

olegart · 2018-01-23T15:12:00Z

@fjmolinas actually with simplified code you only need cpu_number_of_ports in a single function on CPU startup, setting all GPIOs to AIN mode. So there will be no need for global var; for some possible other uses it can be wrapped in get_cpu_ports_number() function too.

Otherwise, incorrect define will lead to HardFault or high power consumption in STOP mode.

fjmolinas · 2018-01-23T15:39:28Z

@olegart you are right, will do. Addressed a bunch of the refactors suggestion. The state of some interfaces is therefore in hands of individual drivers, and doesn't guarantee lowest power (Interfaces-related outputs (SPI CLK/CS/MOSI, UART TX, etc. pointed out by @olegart). Regarding MISO state not sure what would be the desired state if PU or PD, I feel like it could depend on what driver is connected to it; @kYc0o thoughts?.

On a different note, the error travis is throwing has to be handled manually right?

kYc0o · 2018-01-24T00:22:27Z

Regarding MISO state not sure what would be the desired state if PU or PD, I feel like it could depend on what driver is connected to it; @kYc0o thoughts?.

Is it strictly necessary to configure it? Otherwise I'd say PU.

kYc0o · 2018-01-24T00:29:22Z

Regarding travis, I tried to rebase your PR manually and I got in trouble, so please recheck that your commits are not squashed and they are not overriding each other. I can help you on that afterwards if needed.

I was testing again the PR and nucleo-l152 is still broken because of LSI selection for RTC. Can you take a look at that? I made a PR #8370 but it's not enough, as @olegart suggested, it's better to add a timeout to tell if a LSE is present. However, even if LSI is selected the RTC is broken on that board.

olegart · 2018-01-24T07:18:48Z

Is it strictly necessary to configure it? Otherwise I'd say PU.

Doesn't actually matter PU or PD, any decent slave must keep its MISO in high impedance mode when not selected by CS. Or it can be configured as Analog Input (AIN) while not in use, then it doesn't need pulling.

kaspar030 · 2018-01-24T10:22:58Z

cpu/stm32l1/periph/pm.c

@@ -0,0 +1,160 @@
+/*


A quick glimpse makes me feel that this could be integrated a lot more into the existing stm32 pm code.

I actually thought the same and tried it but it looked really ugly. Lots of #ifdef's everywhere. The architectures for power management between F and L are really different (registers, options, modes, etc.), so I'd say it deserves it's own implementation, even if it's similar.

So, is it possible to have only two implementations then? One stm32f pm and one stm32l pm?

So, is it possible to have only two implementations then? One stm32f pm and one stm32l pm?

Is what I think is done here, isn't it? Anyways, yes, is what I'd propose.

haven't tested with other stm32l besides l1, I do have a nucleo-l0 somewhere, could test and see. But in that case what would be the suggested structure? ifdef's in stm32_common? It would be really ugly, or split stm32_common into a stm32l_common and stm32f_common? Somehting like that would need more opinions-ACK's

L0-L1-L4 are pretty similar in power management (and other peripherals too). We have plans to port PM on L0 and L4, didn't make it yet, but I don't expect too many ifdefs there.

F-series CPUs are quite different, especially oldest F1 series. Do not expect F1 and F4 to have the same code.

But anyway, someone has to try and do it. I expect clock initialization to have a lot of ifdefs, but procedures before __WFI() can be generalized — they are mostly related to Cortex-M core, not specific vendor peripherals.

P.S. Actually, even STM32 CPUs of the same series are not always fully compatible (e.g. STM32L151CB vs STM32L151CB-A vs STM32L151CC — RTC, EEPROM, some errata points are different), but we tested this LPM implementation on L151CB, L151CB-A, L151CC and L152RE, surprisingly it works.

I can get working on that or merge efforts (@olegart depending on work-load), but I feel that would be a different PR.

I remembered we just bough a couple of L4 too.

@fjmolinas yeah, for sure, that's for future PRs.

P.S. Actually I have a meeting with the client tomorrow evening, so I still have roughly a day to test and fix LPM on STM32L071KBU6 :)

…_initialization

fjmolinas · 2019-04-08T17:05:10Z

#close in favor of #11359

fjmolinas added 8 commits January 19, 2018 11:37

Initial adapt to master of stm32l1 lpm

a3d2074

Add sleep, standy, idle mode, missing proper stmclk lib

7749652

Refactor stm32l1_clk to use stmclk.h, add msi option

8eddc69

Add uart disabling, missing resseting

1598143

Add external baudrate setting to uart

3046d18

Merge branch 'stm32_uart' into stm32l1_lpm

0fca586

cpu/stm32l1 add lpm support to stm32l1 & nucleo-l1

7b38e7b

remove extra power modes

618ab91

This was referenced Jan 22, 2018

stm32_common/uart: add baudrate recalculation based on clk settings #8401

Closed

stm32l1/stmclk: refactor clck init to use stmclk.h, add msi option #8402

Closed

Merge branch 'master' into stm32l1_lpm

8993ab9

kYc0o requested changes Jan 22, 2018

View reviewed changes

stm32l1/periph/pm: remove extra functions, fix comments style

142c433

fjmolinas added 3 commits January 23, 2018 12:49

stm32l1/periph/pm: move uart, spi to corresponding driver

67fd3f1

cpu/cortexm_common: add function to check memmory address validity

18b7acc

stm32l1/periph/pm: move gpio AIN init to cpu_init

dd45b91

kaspar030 requested changes Jan 24, 2018

View reviewed changes

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: pm Area: (Low) power management labels Jan 25, 2018

--fixup: refactor to use cortexm_sleep, factor out xtimer and uart re…

4190b00

…_initialization

kYc0o mentioned this pull request Feb 6, 2018

cpu/cortexm: add __NOP(); after __WFI(); for stm32l152 to avoid hardfault #8518

Merged

--fixup: void cast to unused variable in cpu_check_address

b301778

fjmolinas mentioned this pull request Apr 19, 2018

examples/lorawan: add autonomous lorawan class A device example #8789

Merged

tests/periph_pm: add a test for periph_pm

05e8461

fjmolinas mentioned this pull request Mar 12, 2019

cpu/stm32l1: add stop and standby modes, adds pm_layered #11159

Merged

fjmolinas mentioned this pull request Apr 8, 2019

cpu/stm32: optimize power consumption #11359

Closed

fjmolinas closed this Apr 8, 2019

fjmolinas mentioned this pull request Jun 28, 2019

cpu/stm32: optimize stop mode for stm32f* #11758

Merged

fjmolinas mentioned this pull request Jul 12, 2019

cpu/stm32l1: optimize power consumption #11832

Merged

fjmolinas deleted the stm32l1_lpm branch August 7, 2019 15:46

stm32l1/pm: add low power modes for stm32l1 #8403

stm32l1/pm: add low power modes for stm32l1 #8403

Conversation

fjmolinas commented Jan 22, 2018 • edited Loading

Contribution description

Issues/PRs references

kYc0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o commented Jan 22, 2018

olegart commented Jan 22, 2018 • edited Loading

olegart commented Jan 22, 2018

fjmolinas commented Jan 23, 2018

olegart commented Jan 23, 2018

fjmolinas commented Jan 23, 2018 • edited Loading

kYc0o commented Jan 24, 2018

kYc0o commented Jan 24, 2018

olegart commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegart Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

fjmolinas Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjmolinas commented Apr 8, 2019

fjmolinas commented Jan 22, 2018 •

edited

Loading

fjmolinas Jan 25, 2018 •

edited

Loading

olegart commented Jan 22, 2018 •

edited

Loading

fjmolinas commented Jan 23, 2018 •

edited

Loading

olegart Jan 24, 2018 •

edited

Loading

fjmolinas Jan 24, 2018 •

edited

Loading