Discussion of Power Management

Dear everyone,

I've worked on the power management for the atmega platform at the RIOT Hackathon. While creating PRs for the different parts, I stopped at a critial point I would like to discuss here:

Several functions are used for the power management and stubs are provided in drivers/periph_common/pm.c. However as mentioned in one issue, the weak implementation did *not* work correctly. Therefore we agreed on removing weak definitions for the module and use some #ifdefs instead:

#if !defined( MODULE_PM_LAYERED ) void pm_set_lowest(void) { } #endif

Unfortunately, this does not work because some platforms and especially the stm32_common/kinetis_common/cortexm_common platforms define these functions all over the place!

My first question is there for, is there a good system to implementations for a module such as the PM module at different levels?

E.g. the CPU should always have the possibility to implement specific pm_* functions. A fallback could be provided in the common CPU. Additionally, the pm_layered module uses some of these CPU functions. And finally, the fallback functions in pm.c should be used, if none exist. Is it ok to use #define / #ifdef constructs for these? Any opinions? I think we need a very fine-granular system here. I started to add special features to handle this case:

makefiles/features.inc.mk

DEFAULT_FEATURES += periph_pm periph_pm_off periph_pm_set_lowest periph_pm_reboot

cpu/cortexm_common/Makefile.features FEATURES_PROVIDED += periph_pm_set_lowest

I thought that maybe features can be used to describe which functions are provided. Later on based on include/compile order, missing functions can be added if needed. Any opinions on this?

Best Regards, Robert

Hi Robert,

(I'll CC the list, this may be interesting to others)

The main problem is that it is NOT sufficient to provide pm_layered and periph_pm. As the various CPUs provide different implementations.

Actually, that is sufficient.

The various defines look something like this:

cpu/saml21: cpu/cortexm_common, cpu/sam0_common   defines pm_set()

cpu/samd21: cpu/cortexm_common, cpu/sam0_common   defines pm_set()

boards/mulle: cpu/k60

cpu/k60: cpu/cortexm_common, cpu/kinetis_common

cpu/cortexm_common:   defines pm_set(), pm_set_lowest(), pm_reboot()

cpu/kinetis_common:   defines pm_set()

All of them who define pm_set() support pm_layered. cortexm_common just provides a fallback for pm_set_lowest() which at least sets the ARM CPU into idle.

These are only two examples of conflicts. The question here would be why do we have multiple "common" CPUs and where should the sleep modes be implemented?

The common cpus are nested, e.g., k60 -> kinetis_common -> cortexm_common.

This is how I envision it:

- if a CPU has a proper pm_set(), it depends on pm_layered - if not, possibly e.g., kinetis_common has pm_set(), then that should depend on pm_layered - if not, the cpu (or e.g., kinetis_common) might depend on cortexm_periph_pm_fallback (where the current cortexm weak defines should go) - if the cpu (or any common ancestor) has it's own way of power management, it implements pm_set_lowest() - if not, it depends on periph_pm_fallback (which should wrap current drivers/periph_common/pm.c)

That should cover all cases, right?

I guess I could just remove the periph/pm.c from kinetis_common?

Yes! kinetis_common should use the cortexm_fallback, they're equivalent.

Kaspar

Hi Kaspar,

thanks for the reply. See answers inline

All of them who define pm_set() support pm_layered.

Makes sense. **CPUs should always define what they are going to use!**

cortexm_common just provides a fallback for pm_set_lowest() which at least sets the ARM CPU into idle.

Oh, damn! It's not defining a pm_set. You are completely right, this is how it should be! Got a little confused here.

I wonder why the kinetis_common cpu defines a pm_set, but cortexm_common does not? Because the function is cortexm_sleep(0), which should be available to all cortexm_common-based boards/cpus? So why not merge these two?

This is how I envision it:

- if a CPU has a proper pm_set(), it depends on pm_layered

ACK. pm_layered does everything, except for pm_set. pm_layered will then do all other stuff.

- if not, possibly e.g., kinetis_common has pm_set(), then that should depend on pm_layered

This means that kinetis_common should provide a module **kinetis_common_pm** that provides pm_set. The CPU should then depend on this module. Additionally, it should select pm_layered.

- if not, the cpu (or e.g., kinetis_common) might depend on cortexm_periph_pm_fallback (where the current cortexm weak defines should go)

Also sounds good.

- if the cpu (or any common ancestor) has it's own way of power management, it implements pm_set_lowest()

Absolutely. Then we just don't use any of the existing pm_* modules. This is the case that can always be done.

- if not, it depends on periph_pm_fallback (which should wrap current drivers/periph_common/pm.c)

In this case we use the existing pm_* functions from drivers/periph_common/pm.c, but I would move them to the module I created (pm_fallback?).

I am working on a pm_fallback module (moved it from drivers/periph/pm.c to sys/pm_fallback/pm.c) - is that the right location? Or where should it be located?

That should cover all cases, right?

I think so. Let's discuss the naming and location of the fallback modules across CPUs and RIOT in general and I will take care of this.

- Robert

Hi Robert,

- if not, possibly e.g., kinetis_common has pm_set(), then that should depend on pm_layered

This means that kinetis_common should provide a module **kinetis_common_pm** that provides pm_set. The CPU should then depend on this module. Additionally, it should select pm_layered.

Yes!

- if not, it depends on periph_pm_fallback (which should wrap current drivers/periph_common/pm.c)

In this case we use the existing pm_* functions from drivers/periph_common/pm.c, but I would move them to the module I created (pm_fallback?).

Perfect!

I am working on a pm_fallback module (moved it from drivers/periph/pm.c to sys/pm_fallback/pm.c) - is that the right location? Or where should it be located?

I think as the periph common code is in drivers/, so should the fallback code.

How about using submodules within drivers/periph_common/Makefile? E.g.,

- rename drivers/periph_common/pm.c to pm_fallback.c - add     # exclude submodule sources from *.c wildcard source selection     SRC := $(pm_fallback.c,$(wildcard *.c))   to drivers/periph_common/Makefile

- add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk

That would compile pm_fallback *only* if periph_common_pm_fallback is selected. (with #5757, all periph files will become submodules...)

kinetis_common/periph/pm.c can just go (and be replaced with a dependency on the cortem_common code).

Kaspar

Hi Kaspar,

Hi Robert,

- if not, possibly e.g., kinetis_common has pm_set(), then that should depend on pm_layered

This means that kinetis_common should provide a module **kinetis_common_pm** that provides pm_set. The CPU should then depend on this module. Additionally, it should select pm_layered.

Yes!

- if not, it depends on periph_pm_fallback (which should wrap current drivers/periph_common/pm.c)

In this case we use the existing pm_* functions from drivers/periph_common/pm.c, but I would move them to the module I created (pm_fallback?).

Perfect!

I am working on a pm_fallback module (moved it from drivers/periph/pm.c to sys/pm_fallback/pm.c) - is that the right location? Or where should it be located?

I think as the periph common code is in drivers/, so should the fallback code.

Just asked because pm_layered is in sys/ :wink:

How about using submodules within drivers/periph_common/Makefile? E.g.,

- rename drivers/periph_common/pm.c to pm_fallback.c - add     # exclude submodule sources from *.c wildcard source selection     SRC := $(pm_fallback.c,$(wildcard *.c))   to drivers/periph_common/Makefile

- add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk

That would compile pm_fallback *only* if periph_common_pm_fallback is selected. (with #5757, all periph files will become submodules...)

This does not work for me, should it be SRC := $(filter-out pm_fallback.c,$(wildcard *.c)) ?

In any case, if I added USE_MODULES += periph_common_pm_fallback, it will not get compiled. Any idea why?

kinetis_common/periph/pm.c can just go (and be replaced with a dependency on the cortem_common code).

I will check that

- Robert

Hi,

    SRC := $(pm_fallback.c,$(wildcard *.c))   to drivers/periph_common/Makefile

- add "PSEUDOMODULES += periph_common_%" to makefiles/pseudomodules.mk

That would compile pm_fallback *only* if periph_common_pm_fallback is selected. (with #5757, all periph files will become submodules...)

This does not work for me, should it be SRC := $(filter-out pm_fallback.c,$(wildcard *.c)) ?

Ah, yes.

In any case, if I added USE_MODULES += periph_common_pm_fallback, it will not get compiled. Any idea why?

Did you use "USEMODULE" instead of "USE_MODULES"? Maybe the periph_common directory doesn't get selected. But we can't make periph_common_pm_fallback depend on periph_common, that might have side effects.

How about drivers/pm_fallback/... for now?

Kaspar

Hi,

In any case, if I added USE_MODULES += periph_common_pm_fallback, it will not get compiled. Any idea why?

Did you use "USEMODULE" instead of "USE_MODULES"?

Yes, did not work :frowning:

Maybe the periph_common directory doesn't get selected. But we can't make periph_common_pm_fallback depend on periph_common, that might have side effects.

How about drivers/pm_fallback/... for now?

Then I'd vote for moving pm_layered to drivers as well? Or should be make it sys/pm_fallback/?

- Robert