cpu/stm32f0: add periph_pm support by vincent-d · Pull Request #9521 · RIOT-OS/RIOT

vincent-d · 2018-07-09T12:03:58Z

Contribution description

Add stm32f0 support in stm32_common pm driver.
Tested with a nucleo-f091rc. Didn't check the actual power consumption yet.

Issues/PRs references

None

vincent-d · 2018-07-17T08:36:27Z

Tested on a custom device (stm32f091 based) and works!

kaspar030

minor readability question

kaspar030 · 2018-07-17T08:49:19Z

cpu/stm32_common/periph/pm.c

+    defined(CPU_FAM_STM32F4)
+#define PM_EWUP_CONFIG  (PWR_CSR_EWUP)
+#elif defined(PWR_CSR_EWUP8)
+#define PM_EWUP_CONFIG  (PWR_CSR_EWUP8 | PWR_CSR_EWUP7 | PWR_CSR_EWUP6 | \


Maybe readability can be improved by using a static inline?

static const unsigned _pwr_ewup_config(void) { unsigned tmp = 0; #if defined(PWR_CSR_EWUP8) tmp |= PWR_CSR_EWUP8; #endif #if defined(PWR_CSR_EWUP7) tmp |= PWR_CSR_EWUP7; #endif ... #if defined(PWR_CSR_EWUP1) tmp |= PWR_CSR_EWUP1; #endif return tmp; }

... then use PWR->CSR = _pwr_ewup_config() below.

This should optimize to a single value even when assigning a volatile register.

I'll give a try

vincent-d · 2018-08-08T10:19:20Z

Ping

aabadie · 2018-08-08T11:19:25Z

cpu/stm32_common/periph/pm.c

@@ -1,5 +1,6 @@
 /*
- * Copyright (C) 2016 Kaspar Schleiser <kaspar@schleiser.de>
+ * Copyright (C) 2018 OTA keys S.A.


According to the authors list order, I think the copyright order should be reversed. Otherwise this gives the impression that OTA Keys initially contributed this file, which is not true.

aabadie · 2018-08-08T11:52:06Z

Apart from the copyright comment, the changes look good and work (tested on nucleo-f070rb).

aabadie

ACK

@kaspar030's comment is addressed, so please also squash

vincent-d · 2018-08-08T13:08:34Z

Squashed

aabadie · 2018-08-08T13:49:20Z

Thanks @vincent-d. Now let's wait for @kaspar030's approval (and merge ? :) )

vincent-d · 2018-10-03T09:07:29Z

Ping @kaspar030

smlng · 2018-12-21T12:36:06Z

please rebase, I'll to find a device for testing

smlng · 2018-12-21T12:36:26Z

if good, we can dismiss @kaspar030 review

comment addressed

vincent-d · 2018-12-27T10:12:20Z

Rebased and adapted stm32l0 implementation

vincent-d · 2019-03-13T10:24:49Z

rebased

aabadie · 2019-03-13T10:30:56Z

Ah yes, sorry ;) I'll have a look later.

fjmolinas · 2019-03-13T10:46:34Z

cpu/stm32_common/periph/pm.c

-#else
-            PWR->CSR |= PWR_CSR_EWUP;
 #endif
+            PWR->CSR |= _ewup_config();


I don't think activating all wake-up pins available by default is a good choice. On nucleo boards WKUP2 is connected to user button and VDD which triggers wakeup from STANDBY mode immediately (except for stm32l433rc) (#11167). On other nucleo boards you have wkup pins connected to other peripherals which can also trigger weird resets.

Maybe it would be better if this was handled or at least over-writable for every board. Since wiring is very board specific enabling this for all boards using this cpu is prone to conflicts (in some stml4 base boards uart is connected to a wake-up pin too.) IMO default configuration should be no wkup-pin enabled (cpu can allways wake up from RTC), and then when enabling a wkup pin makes sense, this should be handled on board configuration files. Thoughts @aabadie @vincent-d @kaspar030 ?

it's actually how it's implemented. One can define PM_EWUP_CONFIG to enable the pins depending on their needs.

Your are right, sorry I didn't read the code properly. I like it!

aabadie · 2019-03-13T18:49:15Z

I'll give a last try to this PR tomorrow and will merge it. After I'll adapt #11173.

aabadie

I tested this PR on nucleo-f091rc and STOP mode works although consumption seems quite high (~2mA instead of 7 in normal run mode). By default, the STANDBY mode doesn't work because all wakeup pins are enabled which causes troubles.

This is why I don't like the current default behaviour with wake-up pins. See my comment below.

aabadie · 2019-03-14T08:12:50Z

cpu/stm32_common/periph/pm.c

+    tmp |= PM_EWUP_CONFIG;
+#elif defined(PWR_CSR_EWUP)
+    tmp |= PWR_CSR_EWUP;
+#else


I think this should be done differently.
In the current state, if the user doesn't provide a define for PW_EWUP_CONFIG, then this function will activate all wake-up pins.
This results in an invalid default STANDBY configuration for most of the nucleo boards in this case. For example, on nucleo-f091rc, with this default configuration, the STANDBY mode doesn't work unless one builds the application with CFLAGS=-DPM_EWUP_CONFIG=0.

What about having the possibility to provide the PM_EWUP_CONFIG in the board periph_conf.h and include this file in pm.c ?
Then there's no need for this function, just use the following snippet in the STANDBY case, below:

PWR->CSR |= PM_EWUP_CONFIG

And provide a default PM_EWUP_CONFIG to 0 somewhere in the periph_cpu_common.h file.

I think this has been inherited from the previous versions which used to enable all wake-up pins.

I don't mind changing to use PM_EWUP_CONFIG only.

vincent-d · 2019-03-18T10:44:56Z

I changed the wake-up pins config, and refactored even more.

aabadie · 2019-03-18T11:46:48Z

Thanks for updating @vincent-d, it looks much better like this :)

I think the history of commits should be reworked: one commit introducing the pm code refactoring and one adding the support for F0. That would make history cleaner.

I'll give a try asap.

aabadie

Tested on F0, F4, L0 and L1. It works.

ACK, please squash (see #9521 (comment) for a better git history proposal)

vincent-d · 2019-03-19T16:53:35Z

@aabadie done!

aabadie

ACK

vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Jul 9, 2018

vincent-d requested review from aabadie and kaspar030 July 9, 2018 12:03

kaspar030 previously requested changes Jul 17, 2018

View reviewed changes

aabadie reviewed Aug 8, 2018

View reviewed changes

aabadie approved these changes Aug 8, 2018

View reviewed changes

vincent-d force-pushed the pr/stm32f0_pm branch from fbd9b00 to 6603ab9 Compare August 8, 2018 13:08

vincent-d force-pushed the pr/stm32f0_pm branch from 6603ab9 to c9df230 Compare December 27, 2018 10:11

vincent-d mentioned this pull request Mar 13, 2019

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

Merged

vincent-d force-pushed the pr/stm32f0_pm branch from c9df230 to 05b813e Compare March 13, 2019 10:24

fjmolinas reviewed Mar 13, 2019

View reviewed changes

fjmolinas approved these changes Mar 13, 2019

View reviewed changes

aabadie requested changes Mar 14, 2019

View reviewed changes

This was referenced Mar 14, 2019

cpu/stm32l4: add STOP and STANDBY low-power modes #11173

Closed

cpu/stm32f7: add STOP and STANDBY low-power modes #11178

Closed

aabadie approved these changes Mar 18, 2019

View reviewed changes

Vincent Dupont added 2 commits March 19, 2019 17:51

cpu/stm32_common: refactor pm implementation

e83f271

cpu/stm32f0: add pm_layered support

7590d52

vincent-d force-pushed the pr/stm32f0_pm branch from d6eeb2d to 7590d52 Compare March 19, 2019 16:53

aabadie mentioned this pull request Mar 19, 2019

cpu/stm32: add STOP and STANDBY low-power for stm32f3, unify for all stm32 #11211

Merged

aabadie approved these changes Mar 19, 2019

View reviewed changes

aabadie merged commit d7b3091 into RIOT-OS:master Mar 19, 2019

vincent-d deleted the pr/stm32f0_pm branch March 19, 2019 17:29

danpetry added this to the Release 2019.04 milestone Mar 26, 2019

Conversation

vincent-d commented Jul 9, 2018

Contribution description

Issues/PRs references

Uh oh!

vincent-d commented Jul 17, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Aug 8, 2018

Uh oh!

aabadie commented Aug 8, 2018

Uh oh!

vincent-d commented Oct 3, 2018

Uh oh!

smlng commented Dec 21, 2018

Uh oh!

smlng commented Dec 21, 2018

Uh oh!

vincent-d commented Dec 27, 2018

Uh oh!

vincent-d commented Mar 13, 2019

Uh oh!

aabadie commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 13, 2019

Uh oh!

aabadie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 18, 2019

Uh oh!

aabadie commented Mar 18, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 19, 2019

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

aabadie commented Aug 8, 2018 •

edited

Loading

fjmolinas Mar 13, 2019 •

edited

Loading

aabadie left a comment •

edited

Loading