pm: WIP for power management that uses submodules #7597

roberthartung · 2017-09-13T14:17:17Z

This commit resolves issues with the power management and prepares the PM architecture to be selected by the actual CPU.

kaspar030 · 2017-09-14T12:11:40Z

It seems to me that you didn't reduce the number of duplicate implementations. How about using submodules in the pm_fallback module, which can be independently selected?

roberthartung · 2017-09-14T12:14:36Z

Hi @kaspar030 , the idea was not to reduce duplication, but to resolve some issues with the current selection of pm functions/implementations and to allow the actual cpu to use the implementation it needs.

Submodules sound the way to go for me! Which would you suggest? One for each of pm_off, pm_reboot and pm_set_lowest? So the cpu would e.g. do the following:

USEMODULE += pm_fallback_pm_set_lowest
USEMODULE += pm_stm32_common_pm_off
USEMODULE += pm_stm32_common_pm_reboot

Like this? That also looks like a lot of overhead?

kaspar030 · 2017-09-14T12:17:07Z

Submodules sound the way to go for me! Which would you suggest? One for each of pm_off, pm_reboot and pm_set_lowest?

Yes! Can we apply this to the cpu fallbacks, too?

roberthartung · 2017-09-14T12:17:39Z

@kaspar030 check out my updated comment!

roberthartung · 2017-09-14T12:18:27Z

@kaspar030 That's the question how it would work with the most used CPUs (atmega_common, stm32_common, cortexm_common and nrf5x_common). Let's try to figure it out from my proposal.

kaspar030 · 2017-09-14T12:20:02Z

Like this? That also looks like a lot of overhead?

Hm, it does, but we do want this fine-grained selection. The alternatives are ifdefs with global defines, weak defaults, ...

Let's try!

roberthartung · 2017-09-14T12:21:54Z

Yes, overhead is relative though, because we can just select fine-grained implementations. The main problem is that some CPUs use both stm32_common AND cortexm_common. And we have to review the current pm implementation and see if we can merge it to one of them IMO.

I would really like to avoid any ifdefs/weak defaults if possible.

roberthartung · 2017-09-14T14:56:40Z

@kaspar030 Can you help me with enabling a simple submodule? What do I need? Let's say I want to change pm_fallback to have three submodules: pm_fallback_off, pm_fallback_set_lowest and pm_fallback_reboot. Do I need to cherry-pick / get anything from #7241 ?

kaspar030 · 2017-09-14T15:03:38Z

@kaspar030 Can you help me with enabling a simple submodule? What do I need? Let's say I want to change pm_fallback to have three submodules: pm_fallback_off, pm_fallback_set_lowest and pm_fallback_reboot.

The steps should be

add SUBMODULES := 1 to the pm_fallback Makefile
move the implementations to "off.c", "set_lowest.c", "reboot.c"
add "pm_fallback_%" to makefiles/pseudomodules.mk (or pm_fallback% if there are no always compiled .c files).

See how it is done in core/Makefile.

Do I need to cherry-pick / get anything from #7241 ?

That should be independent. I'll be rebasing it on this one once we're done here. ;)

roberthartung · 2017-09-14T15:11:37Z

@kaspar030 Okay, had a little typo :/

Only problem now is that I have to add USEMODULE += pm_fallback to be able to do the following:

USEMODULE += pm_fallback_off
USEMODULE += pm_fallback_set_lowest
USEMODULE += pm_fallback_reboot

Is there a smarter way to tell the build system that it should include it automatically? How do we do it if we mix it up. I would like to have it like this:

USEMODULE += pm_fallback_set_lowest
USEMODULE += pm_stm32_common_off
USEMODULE += pm_cortexm_common_reboot

but for now we would have to write the following:

USEMODULE += pm_fallback
USEMODULE += pm_fallback_set_lowest

USEMODULE += pm_stm32_common
USEMODULE += pm_stm32_common_off

USEMODULE += pm_cortexm_common
USEMODULE += pm_cortexm_common_reboot

kaspar030 · 2017-09-14T15:14:36Z

Is there a smarter way to tell the build system that it should include it automatically?

Ah, I forgot: add "pm_fallback" as dependency of "pm_falback_%" (e.g., in Makefile.dep):

ifneq (,$(filter pm_fallback_%,$(USEMODULE)))
  USEMODULE += pm_fallback
endif

roberthartung · 2017-09-14T15:17:06Z

Additionally: is there a smart way to select all submodues automatically? e.g. pm_fallback will select _off, _set_lowest and _reboot?

kaspar030 · 2017-09-14T15:23:52Z

Where do add it? In each board, cpu, RIOT's Makefile.dep?

If the module is in drivers, drivers/Makefile.dep, otherwise ./Makefile.dep. The module is shared by all (or most) boards, right?

Additionally: is there a smart way to select all submodues automatically? e.g. pm_fallback will select _off, _set_lowest and _reboot?

nope. ;)

roberthartung · 2017-09-14T15:56:47Z

Almost finished. One thing we really have to look at before merging: Can all CPUs that are based on cortexm_common by default use the pm implementation that is present in cortexm_common? That would be great!

roberthartung · 2017-09-14T16:09:58Z

@kaspar030 Pushed a submodule-based version. You can have a look now!

kaspar030 · 2017-09-15T11:10:20Z

cpu/atmega1281/Makefile.include

@@ -3,6 +3,9 @@ export CFLAGS += -DCOREIF_NG=1

 # tell the build system that the CPU depends on the atmega common files
 USEMODULE += atmega_common
+USEMODULE += pm_fallback_set_lowest


can these be moved to atmega_common?

@kaspar030 No, the idea is that the final CPU will always select everything it needs. In particular my implementation of the pm is specific to atmega1284p, as registers and modes and peripheral levels are individual per CPU.
The only thing would be to create a pm_fallback.mk.inc in atmega_common that will be included in Makefile.include of the CPU?

kaspar030 · 2017-09-15T11:10:49Z

cpu/atmega_common/pm/pm.c

@@ -26,6 +26,14 @@
 #include "irq.h"
 #include "periph/pm.h"

+void pm_set_lowest(void) {}


shouldn't this be in the common pm_fallback_set_lowest?

kaspar030 · 2017-09-15T11:10:54Z

cpu/atmega_common/pm/pm.c

@@ -26,6 +26,14 @@
 #include "irq.h"
 #include "periph/pm.h"

+void pm_set_lowest(void) {}
+
+void pm_off(void)


kaspar030 · 2017-09-15T11:11:55Z

cpu/cortexm_common/Makefile.include

@@ -3,5 +3,6 @@ INCLUDES += -I$(RIOTCPU)/cortexm_common/include
 INCLUDES += -I$(RIOTCPU)/cortexm_common/include/vendor

 USEMODULE += cortexm_common_periph
+include $(RIOTCPU)/cortexm_common/Makefile.dep


cannot include Makefile.dep from a different makefile

I think it didn't work without, but I will check again

kaspar030 · 2017-09-15T11:13:03Z

cpu/ezr32wg/Makefile.include

@@ -1,3 +1,7 @@
 export CPU_ARCH = cortex-m4f

+USEMODULE += pm_cortexm_common_set_lowest
+USEMODULE += pm_cortexm_common_off


don't all cortexm boards use the cortexm_common off and reboot?

kaspar030 · 2017-09-15T11:14:31Z

cpu/lm4f120/Makefile.include

 # use common periph functions
 USEMODULE += periph_common
+USEMODULE += pm_cortexm_common_set_lowest


IMO we can generalize this for all cortexm boards:
if not (feature_provided(periph/pm)) -> use ...._common_set_lowest
of make it dependend on whether pm_layered is used.

Sounds good. As mentioned above we have to check if all boards based on cortexm_common AND stm32_common can still use the pm implementation of cortexm_common!

well, pm_reboot() will definitely be the same for all cortexm. "pm_off()" will be cpu dependent. "pm_set_lowest()" will be the same for all cortexm not implementing periph/pm.

where would we implemented such a check to select all cortexm_common fallback if the feature is not provided by the cpu itself?

kaspar030 · 2017-09-15T11:15:15Z

cpu/nrf5x_common/pm/pm.c

@@ -23,6 +23,14 @@

 #include "cpu.h"

+void pm_set_lowest(void) {


use pm_fallback_coretexm_common_

kaspar030 · 2017-09-15T11:16:03Z

drivers/pm_fallback/set_lowest.c

+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+void pm_set_lowest(void) {}


I don't like this file much... ;)

Me neither, but that's the fallback and it was before as well. Any suggestions?

Not with using submodules and without using ifdefs. Let's keep it for now.

pm_set_lowest is only used in core/kernel_init.c and sys/pm_layered/pm.c, maybe we can omit the file and only call pm_set_lowest in kernel_init.c if the CPU/board defines something like #define HAS_PM_SET_LOWEST or something?

roberthartung · 2017-09-25T12:26:33Z

From what I can see, all boards that are based on stm32_common, are also based on cortexm_common. Therefore, as stm32_common provides a pm_layered implementation, all boards based on stm32_common can select the pm_set method from stm32_common and off/reboot from cortexm_common.

find . -name "Makefile.include" | xargs grep -A2 stm32_common

roberthartung · 2017-09-25T12:39:20Z

@kaspar030 Can kinetis_common just use the default cortem_common pm implementation? You defined a single power mode here, which will in the end do the same as pm_set_lowest of pm_layered will call pm_set which calls cortexm_sleep(0), but in the end, this is the same as pm_set_lowest from cortexm_common.

jnohlgard · 2017-09-25T12:51:40Z

Kinetis has 4 power modes which can be used with RIOT without any big intrusive changes. I have a WIP branch for adding them, but I have not yet posted a PR. Please leave the Kinetis specific implementation of pm_set in place to make merging my future PR easier, don't remove it right now.

…ack modules

roberthartung · 2017-09-25T13:27:43Z

@gebart Thanks! The function has been moved to its own module for now to comply with the other implementations. Might change that depending on the feedback receivedhere.
@kaspar030 I just committed all the changes requested.

I dont like that I have to use USEMODULE += in Makefile.features, but this was the only possibility to check if the feature exists.

roberthartung · 2017-09-25T13:41:10Z

I also updated the wiki page on the new power management API and the ideas behind it: https://github.com/RIOT-OS/RIOT/wiki/Power-Management#architecture

cladmi · 2017-10-06T16:17:12Z

I would prefer the version with submodules, because, the required code and the dependencies are explicit.

For me, the problem here, is the current possibility to express dependencies to interfaces which is a build system issue and not a source file issue.

kaspar030 · 2017-10-28T18:51:31Z

Closed in favor of #7726.

@roberthartung Again, thanks a lot for the work you've put in this!

roberthartung added 2 commits September 13, 2017 16:14

WIP: Power Management with selective modules

77c6ede

cpu/stm32_common: adds todo for pm

7c81028

roberthartung mentioned this pull request Sep 14, 2017

pm: fix pm configuration #7308

Closed

roberthartung added 2 commits September 14, 2017 17:59

pm: submodules for pm_fallback

c326065

pm: submodules for cortexm_common and cleanup of pm_layered

fe1ef1a

roberthartung changed the title ~~Power Management: Proposal for an implementation that uses separate modules~~ pm: WIP for power management that uses submodules Sep 14, 2017

This was referenced Sep 14, 2017

Atmega PWM implementation #7542

Closed

Implement power management pm for CPUs #6802

Closed

kaspar030 mentioned this pull request Sep 15, 2017

make: use submodules for periph #7241

Merged

kaspar030 requested changes Sep 15, 2017

View reviewed changes

miri64 assigned kaspar030 Sep 19, 2017

pm: removes unneeded code duplication and automatically selects fallb…

9039dfb

…ack modules

pm: removes redundant file

69d3627

roberthartung mentioned this pull request Sep 26, 2017

pm: adds a c/file-based power management configuration to select required implementations #7648

Closed

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pm Area: (Low) power management State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 28, 2017

kaspar030 closed this Oct 28, 2017

roberthartung mentioned this pull request Feb 7, 2020

github: add CODEOWNERS file #13289

Merged

		@@ -23,6 +23,14 @@

		#include "cpu.h"

		void pm_set_lowest(void) {

pm: WIP for power management that uses submodules #7597

pm: WIP for power management that uses submodules #7597

Conversation

roberthartung commented Sep 13, 2017

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017 • edited Loading

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017

roberthartung commented Sep 14, 2017

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017

roberthartung commented Sep 14, 2017

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017

kaspar030 commented Sep 14, 2017

roberthartung commented Sep 14, 2017 • edited Loading

roberthartung commented Sep 14, 2017

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

roberthartung commented Sep 25, 2017

roberthartung commented Sep 25, 2017

jnohlgard commented Sep 25, 2017

roberthartung commented Sep 25, 2017 • edited Loading

roberthartung commented Sep 25, 2017

cladmi commented Oct 6, 2017

kaspar030 commented Oct 28, 2017

roberthartung commented Sep 14, 2017 •

edited

Loading

roberthartung commented Sep 14, 2017 •

edited

Loading

roberthartung commented Sep 25, 2017 •

edited

Loading