Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/ztimer: add power management for ztimer clocks #13722

Merged
merged 2 commits into from
May 1, 2020
Merged

sys/ztimer: add power management for ztimer clocks #13722

merged 2 commits into from
May 1, 2020

Conversation

jue89
Copy link
Contributor

@jue89 jue89 commented Mar 26, 2020

Contribution description

This is a follow-up of #13585. After some discussion with @kaspar030 I came up with an implementation of his approach.

The basic idea / assumptions behind this PR:

  • On some platforms, the power consumption can be reduced dramatically by disabling high frequency hardware timers. Those timers are used by the ZTIMER_USEC clock.
  • The high frequency timers require a certain pm mode to be active. Lower pm modes deactivate those timers.
  • ztimer knows if any ztimer_t is running on the ZTIMER_USEC clock.
  • ztimer blocks that certain pm mode, if a ztimer_t requires the ZTIMER_USEC clock and unblocks it if no ztimer_t is running on ZTIMER_USEC.

Testing procedure

I'll describe the testing procedure later if the over-all architecture is considered acceptable

Issues/PRs references

Closes #13585
Closes #13580

@jue89 jue89 mentioned this pull request Mar 26, 2020
Closed
@kaspar030 kaspar030 added the Area: timers Area: timer subsystems label Mar 26, 2020
@jue89 jue89 mentioned this pull request Mar 30, 2020
Closed
@tcschmidt tcschmidt requested a review from roberthartung March 30, 2020 18:14
roberthartung
roberthartung reviewed Mar 31, 2020
View reviewed changes
sys/include/ztimer.h Outdated
@@ -297,6 +297,9 @@ struct ztimer_clock {
uint32_t lower_last; /**< timer value at last now() call */
ztimer_now_t checkpoint; /**< cumulated time at last now() call */
#endif
#if MODULE_PM_LAYERED
int8_t required_pm_mode; /**< min. pm mode required for the clock to run */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about using -1 here as a value. 0 is idle and then we have four additional modes. I would be a fan of 255 as an invalid mode, as it is also "below" the deepest sleep mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay good point.

Thinking about this a second time, I would rather define ZTIMER_NO_REQUIRED_PM_MODE and use this instead of checking magic numbers. Would you agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

roberthartung
roberthartung reviewed Mar 31, 2020
View reviewed changes
@roberthartung
Copy link
Member

Without knowing all the details about ztimer this looks straightforward. However, is it possible to have the same physical hardware timer for the usec and msec timer? If so, it could be possible to specify different required pm modes.

@jue89
Copy link
Contributor Author

jue89 commented Mar 31, 2020

Thank you for looking into this PR!

Without knowing all the details about ztimer this looks straightforward. However, is it possible to have the same physical hardware timer for the usec and msec timer? If so, it could be possible to specify different required pm modes.

Yes, by default ZTIMER_MSEC derives its tick from ZTIMER_USEC. So both are relying on the same hardware timer. In this case, the ZTIMER_MSEC clock becomes the user of the ZTIMER_USEC clock and, thus, would not just block the pm modes associated to ZTIMER_MSEC but also block those of ZTIMER_USEC.

If you want to have a ZTIMER_MSEC that is independent of ZTIMER_USEC - which is more or less necessary to gain any benefit from this PR - you have to stick ZTIMER_MSEC to a dedicated RTT timer.

@kaspar030
Copy link
Contributor

Sorry for not giving feedback earlier. I think we should try as proposed here. I thought I could do some actual power measurement, but most of my gear is unavailable due to the quarantine. :(

@jue89
Copy link
Contributor Author

jue89 commented Mar 31, 2020

@kaspar030 don't worry. I also have to stick with the power measurement included in the SAMR30-XPRO board. It is not that accurate, but I can tell if the board is drawing 10uA or 100uA.

@jue89 jue89 mentioned this pull request Apr 2, 2020
Closed
@jue89
Copy link
Contributor Author

jue89 commented Apr 22, 2020

Is there anything I could do to support the review of this PR?

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 23, 2020
kaspar030
kaspar030 approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine.

Tested on nucleo-f401re by pimping tests/periph_pm using this patch, which sets two timers. (patch assumes that mode 1 is the one to block).

ACK.

@kaspar030
Copy link
Contributor

@jue89 please rebase and squash!

@jue89 jue89 force-pushed the feature/ztimer_pm-layered branch from 7fd71dc to 73e2261 Compare April 29, 2020 10:56
@benpicco benpicco merged commit f42b4b4 into RIOT-OS:master May 1, 2020
This was referenced May 4, 2020
Closed
Closed
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
@maribu maribu mentioned this pull request Jul 20, 2020
Closed
@kfessel kfessel mentioned this pull request Mar 5, 2021
Merged
@jue89 jue89 mentioned this pull request Apr 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthartung roberthartung roberthartung left review comments

@kaspar030 kaspar030 kaspar030 approved these changes

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

Assignees
No one assigned
Labels
Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.

ztimer and pm_layered
5 participants
@jue89 @roberthartung @kaspar030 @miri64 @benpicco