cpu/kinetis: implement power modes for pm_layered by benemorius · Pull Request #13742 · RIOT-OS/RIOT

benemorius · 2020-03-28T00:13:18Z

Contribution description

This implements the remaining power modes for Kinetis platforms. There are now 4 modes ranging from about 10uA to about 5mA depending on the board.

Testing procedure

For this PR I think it should be enough to test that each power mode changes the current consumption. As the peripheral drivers haven't yet been optimized for power in this PR, a reset is required to wake from power modes lower than 3.

I used tests/periph_pm to set each power mode and observed the current consumption. Values will vary for each board but here's what I get with a kw41z-mini:

mode 0: 11 uA
mode 1: 59 uA
mode 2: 274 uA
mode 3: 4.8 mA
~~run: 10mA~~

Note that on Kinetis MCUs it is necessary after flashing to power cycle the board before measuring power consumption, else the debug hardware is forced to remain in the active state in modes 1 and 2 which will give wrong consumption readings.

Issues/PRs references

Split out of #11789

benpicco · 2020-03-29T21:32:37Z

cpu/kinetis/include/periph_cpu.h

+ * @name    Kinetis power mode configuration
+ * @{
+ */
+#define PM_NUM_MODES    (4U)


Is KINETIS_PM_WAIT the default (idle) state?
If you must put #define PM_NUM_MODES (3U) here. (Yes, this is confusing… #13475)

If I'm understanding correctly, the consensus is that we want the highest enterable state to be what ARM calls Sleep which is where everything stays on but the CPU calls wfi(). That would mean PM can't block that mode which means PM can't enter what ARM calls Run mode which is effectively a while(...){} state. Is that right?

I'm not saying it's terribly useful for PM to enter that mode. I'm just making sure I understand. I've been using it this way for years and it never occurred to me to do otherwise.

I've added comments for each power mode to try to remove any confusion from differing terminologies as I'm not certain what you mean by "idle state". Hopefully now you can tell me if it's definitely wrong the way it is.

Hm I think there is a misunderstanding.
From what I read, the default IDLE mode should be KINETIS_PM_WAIT. You can remove case PM_NUM_MODES, that is not needed.

But PM_NUM_MODES only counts the non-IDLE modes (that is all other modes: KINETIS_PM_STOP, KINETIS_PM_VLPS, KINETIS_PM_LLS - those are 3).

Why it often still works with the 'wrong' number is that many implementations have a default: case at their normal IDLE mode, so it will also catch all 'higher' modes.

Ok I think that satisfies my uncertainty about how it should be. I've pushed an update to change it as you said.

Why it often still works with the 'wrong' number is that many implementations have a default: case at their normal IDLE mode, so it will also catch all 'higher' modes.

In this case it worked because there was a PM_NUM_MODES: case to handle it, but that wasn't how it was supposed to work.

cpu/kinetis/periph/pm.c

cpu/kinetis/Makefile.include

benpicco · 2020-06-17T05:42:09Z

A rebase is needed.

fjmolinas

@benemorius I have some kinetis to test this and access to my office to measure this again, if you can rebase and address the couple of comments I can test!

fjmolinas · 2020-06-17T09:17:49Z

cpu/kinetis/include/periph_cpu.h

+#define PM_BLOCK(x) pm_block(x)
+/**
+ * @brief   pm_unblock iff pm_layered is used
+ */
+#define PM_UNBLOCK(x) pm_unblock(x)
+#else
+/* ignore these calls when not using pm_layered */
+#define PM_BLOCK(x)
+#define PM_UNBLOCK(x)


drivers/kw41zrf/include/kw41zrf_intern.h redefines these, can you adapt that to use this defintions?

In agree. But in fact I think we probably need to generalize it further even. Unless I missed something, everything that calls pm_block() and pm_unblock() is going to need some equivalent of these defines to cover the case where pm_layered isn't built. Is that wrong?

I think you are right, currently pm_% is only used in code for platforms that always provide pm_layered or there are a couple of ifdef MODULE_PM_LAYERED, maybe it makes sense to have empty definitions of those functions, but I think this should be in its own PR though, might spark up some unneeded discussion (in the context of this PR)

In the mean time I removed the duplication as you indicated.

benpicco · 2020-06-21T19:52:39Z

Looks good to me - feel free to squash.

fjmolinas · 2020-06-22T08:49:23Z

@benemorius I'm trying to measure power consumption on a frdm-kw41z, have you measured it on that board? I'm trying to get the right pin header configuration, but I think I still have some leakage somehwere, since I can't seem to be able to go lower than 1.mA

benemorius · 2020-06-22T21:32:30Z

Unfortunately I don't have frdm-kw41z. That sure is a lot of jumpers on it.

I saw this thread that might be helpful: https://community.nxp.com/thread/469967

fjmolinas · 2020-06-25T14:01:41Z

Unfortunately I don't have frdm-kw41z. That sure is a lot of jumpers on it.

I saw this thread that might be helpful: https://community.nxp.com/thread/469967

Seems like the errata point 3 is getting in my way, can't do any of the two workarounds right now. I'm going to trust @benemorius on this one.

fjmolinas

ACK!

fjmolinas · 2020-06-25T14:03:10Z

drivers/kw41zrf/include/kw41zrf_intern.h

 /* When the transceiver is not in DSM, this power mode will be blocked.
 * TODO: Change this to symbolic name KINETIS_PM_VLPS when Kinetis power
- * management is merged (https://github.com/RIOT-OS/RIOT/pull/7897) */
+ * management is merged (https://github.com/RIOT-OS/RIOT/pull/13742) */


Hmm just saw this, wrong PR number?

But #13742 is this PR and it introduces KINETIS_PM_VLPS

Whoops, I'm not sure why I updated the PR number instead of executing the TODO. I'll fix that.

found typo in pr number

fjmolinas · 2020-06-26T08:50:57Z

All good now.

fjmolinas

ACK.

fjmolinas · 2020-06-26T08:54:26Z

Thanks for the patience and contributions @benemorius!

benemorius requested review from fjmolinas, kaspar030 and roberthartung as code owners March 28, 2020 00:13

benpicco added Area: pm Area: (Low) power management Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Mar 28, 2020

benpicco self-requested a review March 28, 2020 00:35

benpicco reviewed Mar 29, 2020

View reviewed changes

cpu/kinetis/periph/pm.c Show resolved Hide resolved

leandrolanzieri reviewed Mar 29, 2020

View reviewed changes

cpu/kinetis/Makefile.include Outdated Show resolved Hide resolved

fjmolinas reviewed Jun 17, 2020

View reviewed changes

benemorius force-pushed the pr/kinetis-pm_layered branch from 0f2dc59 to 3af6978 Compare June 17, 2020 21:49

cpu/kinetis: implement power modes for pm_layered

ae7ec96

benemorius force-pushed the pr/kinetis-pm_layered branch from 346db21 to f15ce8b Compare June 22, 2020 02:51

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 22, 2020

fjmolinas previously approved these changes Jun 25, 2020

View reviewed changes

fjmolinas reviewed Jun 25, 2020

View reviewed changes

benemorius mentioned this pull request Jun 25, 2020

[Tracking] kinetis: Implement low power modes #11789

Open

drivers/kw41zrf: remove duplicate PM_UN/BLOCK defines

b8ce88e

benemorius force-pushed the pr/kinetis-pm_layered branch from f15ce8b to b8ce88e Compare June 26, 2020 00:00

fjmolinas approved these changes Jun 26, 2020

View reviewed changes

fjmolinas merged commit a46adc3 into RIOT-OS:master Jun 26, 2020

miri64 added this to the Release 2020.07 milestone Jun 29, 2020

Conversation

benemorius commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benemorius Jun 17, 2020 • 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!

benpicco Jun 17, 2020 • 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!

Uh oh!

Uh oh!

benpicco commented Jun 17, 2020

Uh oh!

fjmolinas 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!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Jun 21, 2020

Uh oh!

fjmolinas commented Jun 22, 2020

Uh oh!

benemorius commented Jun 22, 2020

Uh oh!

fjmolinas commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas 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!

fjmolinas commented Jun 26, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benemorius commented Mar 28, 2020 •

edited

Loading

benemorius Jun 17, 2020 •

edited

Loading

benpicco Jun 17, 2020 •

edited

Loading

fjmolinas commented Jun 25, 2020 •

edited

Loading