OS Managed Power Management framework #9416

ramakrishnapallala · 2018-08-14T08:02:53Z

arch: arm: nordic_nrf: Add an API to check for valid PM state
subsys: power: Add support for OS managed Power Management framework
samples: subsys: Add sample app to demo OS managed PM

Fixes #8255

codecov-io · 2018-08-14T08:36:51Z

Codecov Report

Merging #9416 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9416   +/-   ##
=======================================
  Coverage   52.15%   52.15%           
=======================================
  Files         212      212           
  Lines       25916    25916           
  Branches     5582     5582           
=======================================
  Hits        13517    13517           
  Misses      10149    10149           
  Partials     2250     2250

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469bd39...7f6d3e5. Read the comment docs.

pizi-nordic · 2018-08-14T10:22:46Z

arch/arm/soc/nordic_nrf/nrf52/power.c

+#if defined(CONFIG_SYS_POWER_DEEP_SLEEP)
+	case SYS_POWER_STATE_DEEP_SLEEP:
+#endif
+		lps_flag = true;


Could we just do return true; here?

Yes will do.

pizi-nordic · 2018-08-14T10:22:56Z

arch/arm/soc/nordic_nrf/nrf52/power.c

+		lps_flag = true;
+		break;
+	default:
+		lps_flag = false;


Could we just do return false; here?

yes, will do.

pizi-nordic · 2018-08-14T11:03:29Z

samples/boards/nrf52/power_mgr_os/README.rst

+   transparent to devices. SOC and devices do not lose context in this Mode.
+   SOC supports the following two Low Power states :
+
+   A. CONSTANT LATENCY LOW POWER MODE : This Low Power State triggers CONSTLAT


I think that presented approach does not fit well into nRF5x architecture.
These SoCs have only 2 global states: Running and System OFF.

All other power management depends on the peripherals configuration. Mentioned 16MHz clock is requested when there is an peripheral depending on it. For example, it could be turned on by SPI when data transfer is performed or by enabling constant latency mode in PPI.

There is one exception from this rule: Software could select different clocks sources for so called high frequency and low frequency clocks. In both cases clock could came from crystal oscillator or RC oscillator. Usually this is done basing on simple rules:

If you need precise high frequency clock use crystal oscillator. Otherwise, you should stick on RC oscillator as it requires less power. For example stable high frequency clock is needed for stable carrier in radio peripheral. Depending on application it could be also turned on when precise time measurement is needed or stable timing is required on external interfaces (UART, PWM, I2S, PDM etc.)

You can save few cents from BoM (and release 2 GPIO pins) if you will use RC oscillator for a low frequency clock. However if you need low power or precise timing in peripherals running on 32kHz clock, you should use crystal oscillator.

As you see, there is no way to define "one fit all" power management approach. IMHO we should focus on good interface to peripheral power state management, which will allow application to select most suitable power management scheme.

@pizi-nordic this is sample application documentation which i reused from existing sample. I will double check and fix the gaps.

Also on the PM states, from the documentation i saw that the nrf5x has support for two active power saving states (constant latency mode and low power mode).

The idea is to support pluggable PM policies based on the target application, in this base PR, I have added a simple policy to demonstrate the overall flows.

Are you referring the "Sub power modes" in http://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.nrf52832.ps.v1.1%2Fpower.html ?

If so, you see that mentioned power modes only affect PPI interconnect delay and CPU wakeup time. Changing these settings based on predicted sleep time, which is done in the example, is not a thing which we can recommend as these settings are in fact driven by the real-time requirements of application (we might break operation of PPI-based solutions relying on constant event latency if we switch to low power mode in runtime).

Despite of the fact, that this feature is a bit hard to show in nRF5x family, I think, that OS-level power management is a important feature which have to be implemented in Zephyr.

pizi-nordic · 2018-08-14T11:52:24Z

subsys/power/policy.c

+#include <soc.h>
+#include <soc_power.h>
+
+#define SYS_LOG_LEVEL CONFIG_SYS_LOG_PM_LEVEL


IMHO new code should use new logger.

Ok will fix it.

pizi-nordic · 2018-08-14T12:58:46Z

subsys/power/Kconfig

+config PM_CONTROL_OS_LPS
+	bool "Platfrom supports LPS"
+	help
+	  This is the maximum number of bytes that the


Is the help string correct?
(please also look on the items below)

copy paste error, will fix it.

pizi-nordic · 2018-08-14T13:02:55Z

subsys/power/device.c

+ * and clock domain dependencies.
+ */
+#ifdef CONFIG_SOC_SERIES_NRF52X
+#define CONFIG_MAX_PM_DEVICES	15


IMHO we should be not forced to count devices on board.

Ok will consider using the device list itself.

pizi-nordic · 2018-08-14T13:04:16Z

subsys/power/device.c

+		 * device in the device list is busy as shown below :
+		 * if(device_busy_check(&device_list[idx])) {do something}
+		 */
+		device_retval[i] = device_set_power_state(&pm_device_list[idx],


Some devices (especially external to the chip) need several ms to enter deeps sleep state.
Are we going to block here waiting for each device on the list?

At present state of things, we will have to wait for the device suspend/resume to return because we cannot enter/exit SoC low power states without properly managing device PM.

In future, we will have support for dynamic device PM based on device idle status and can optimize the logic here to speed up things.

I think, that until blocking still is there, this feature will be not really usable in real-word applications.

pizi-nordic · 2018-08-14T13:07:29Z

subsys/power/device.c

+		if (!device_retval[i]) {
+			int idx = device_ordered_list[i];
+
+			device_set_power_state(&pm_device_list[idx],


The comment above also applies here. I seen an I2C gyroscope chip which needed more than 80ms to wake up.
If we perform device wake-up synchronously, then timing of that operation might be not acceptable.

I think this kind of complexity can be handled in the driver itself, for example, on resume driver can submit worker to do the actual wake up and handle the device status accordingly.

Ok. This will also make the sleep state longer saving more time. What about applications which need small latency?

pizi-nordic · 2018-08-14T13:20:53Z

subsys/power/device.c

+			}
+		}
+
+		if (is_core_dev) { 


I think, that this approach is too simple. For example the external device could be connected through I2C. Then we cannot disable I2C before disabling this external device.

There also might be a bit more complex dependencies: For example one of audio codec I used was connected through I2C, but for its operation needed clock derived form I2S stream. As result in order to communicate with such device we had to start I2S streaming (with silence as a content), then configure codec using I2C and then start transmitting real audio data.

I think that we should be able to handle situation described in the first example (as the second is a bit extreme). Information about dependencies could be obtained from device tree, especially if take care of describing there things like clock tree.

As you pointed rightly, we need to use device tree here to handle the device dependencies which will come as incremental changes in the upcoming patches.

dbkinder · 2018-08-14T15:39:35Z

samples/boards/nrf52/power_mgr_os/README.rst

@@ -0,0 +1,92 @@
+.. _nrf52-power-mgr-sample:


This looks like it's a copy of an existing sample documentation with no obvious changes (including the title): /samples/boards/nrf52/power_mgr/README.rst. The doc build complained that the label at the top of the doc here was the same as another document.

yes David, will fix it.

dbkinder · 2018-08-14T15:41:22Z

samples/boards/nrf52/power_mgr_os/README.rst

+******************************
+
+.. zephyr-app-commands::
+   :zephyr-app: samples/boards/nrf52/power_mgr


... and the sample app you're running is in another folder, not the one with this documentation in it.

Ok will fix it.

Add an API _sys_soc_is_valid_power_state to check if a PM state is valid or not. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

ramakrishnapallala · 2018-08-16T06:20:00Z

@pizi-nordic I have addressed all your comments except the following:

MAX_DEVICE_NUM macro usages => This I will remove while adding device dependency logic based DT.

SYS_LOG: I looked at the ./doc/subsystems/logging/logger.rst but an example in kernel or driver code will be really helpful.

ramakrishnapallala · 2018-08-17T14:55:46Z

@pizi-nordic @dbkinder I have addressed your comments and updated the PR, please have a look.

ramakrishnapallala · 2018-08-20T06:08:21Z

@dbkinder I am getting following warning:

/home/buildslave/src/github.com/zephyrproject-rtos/zephyr/doc/_build/rst/doc/samples/subsys/power/power_mgr/README.rst: WARNING: document isn't included in any toctree

How to fix this?

nashif · 2018-08-20T12:57:34Z

How to fix this?

diff --git a/samples/subsys/power/power.rst b/samples/subsys/power/power.rst
new file mode 100644
index 0000000000..e1dd2cc6ee
--- /dev/null
+++ b/samples/subsys/power/power.rst
@@ -0,0 +1,10 @@
+.. _power_management-samples:
+
+Power Management Samples
+########################
+
+.. toctree::
+   :maxdepth: 1
+   :glob:
+
+   **/*

rgundi · 2018-08-21T06:38:20Z

subsys/power/pm_policy.h

+extern  void sys_pm_create_device_list(void);
+
+/**
+ * @brief Function to supend the devices in PM device list


nitpick - spelling mistake in suspend

will fix it.

rgundi · 2018-08-21T06:40:15Z

subsys/power/policy.c

+#else
+#define SECS_TO_TICKS		100
+#endif
+


Isn't the ticks configurable? So, shouldn't you use a macro function to address it here?

agreed, will fix it.

Add support for OS managed Power Management framework for Zephyr under 'subsys/power'. This framework takes care of implementing the _sys_soc_suspend/_sys_soc_resume API's, a PM policy based on SoC Low Power residencies and also provides necessary API's to do devices suspend and resume. Also add necessary changes to support the existing Application managed Power Management framework. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

Add a sample application to demonstrate OS managed Power Management framework. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

pizi-nordic · 2018-08-21T14:56:21Z

SYS_LOG: I looked at the ./doc/subsystems/logging/logger.rst but an example in kernel or driver code will be really helpful.

Please look at #8816. I think that this is the most comprehensive example. There are also few PRs which are smaller (like #9361), but do not show all features.

nashif · 2018-08-21T15:16:37Z

@pizi-nordic are you ok with the changes otherwise? I think the PR was changed to use new logging subsystem already and we can further optimize the usage in the stabilisation period after this gets merged as part of 1.13

addressed

pizi-nordic · 2018-08-21T15:19:59Z

@pdunaj: FYI

pizi-nordic · 2018-08-21T15:30:54Z

@nashif: The biggest issue what I see is bonding of CPU/PPI latency mode to the predicted sleep time. IMHO we should completely remove code which alters this setting from the nrf52/power.c.

I assume that rest of the pointed things will be solved later in subsequent PRs, so I have no problem with them.

PS: I see that fact that I answered to comments before I looked into the code caused some misunderstanding. I apologize for that.

pizi-nordic · 2018-08-21T15:36:18Z

subsys/power/policy.c

+#ifdef CONFIG_TICKLESS_KERNEL
+#define SECS_TO_TICKS		CONFIG_TICKLESS_KERNEL_TIME_UNIT_IN_MICRO_SECS
+#else
+#define SECS_TO_TICKS		CONFIG_SYS_CLOCK_TICKS_PER_SEC


Such tick <-> time conversion is imprecise on some SoCs (especially nRF5x).
If you need a more precise timing, please use __ticks_to_ms() and _ms_to_ticks(). If not, this is OK.

@pizi-nordic, for now, I will leave it like that. I am taking a note of this point and account this in my future changes.

ramakrishnapallala · 2018-08-22T02:34:53Z

@pizi-nordic
"The biggest issue what I see is bonding of CPU/PPI latency mode to the predicted sleep time. IMHO we should completely remove code which alters this setting from the nrf52/power.c."

_sys_soc_set_power_state from nrf52/power.c is only offering a mechanism to enter different power states(SYS_POWER_STATE_CPU_LPS/SYS_POWER_STATE_CPU_LPS_1) and does not have any info on power state residency or latency requirement. It is the PM policy which is making decisions on which PM state to enter through _sys_soc_suspend which is being defined by the application in the current code. The proposed changes are built on top of them.

As per the proposed changes, the application designer or developer has to define a PM policy which suits his requirements, like if the system/CPU is going to idle for X secs(residency), then the system can enter into LPS. And these are configurable and can be changed based on application needs.

@pizi-nordic If you have any suggestions on the way existing things implemented, I can definately work on them and incorporate them in my upcoming PR's.

pdunaj · 2018-08-22T15:54:33Z

subsys/power/device.c

+	"clk_k32src",
+	"clk_m16src",
+	"sys_clock",
+	"UART_0",


UART may be disabled

ramakrishnapallala requested review from andrewboie, andyross, dbkinder, galak and MaureenHelm as code owners August 14, 2018 08:02

carlescufi requested a review from pizi-nordic August 14, 2018 08:50

pizi-nordic previously requested changes Aug 14, 2018

View reviewed changes

dbkinder previously requested changes Aug 14, 2018

View reviewed changes

arch: arm: nordic_nrf: Add an API to check for valid PM state

c4612d0

Add an API _sys_soc_is_valid_power_state to check if a PM state is valid or not. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

ramakrishnapallala force-pushed the pwr-mgmt-fw branch 2 times, most recently from 4086eba to 97ead6d Compare August 16, 2018 05:16

ramakrishnapallala force-pushed the pwr-mgmt-fw branch from 97ead6d to c50a04b Compare August 17, 2018 14:52

ramakrishnapallala requested review from nashif, rgundi and carlescufi August 17, 2018 14:52

ramakrishnapallala force-pushed the pwr-mgmt-fw branch from c50a04b to d31d784 Compare August 20, 2018 13:28

rgundi reviewed Aug 21, 2018

View reviewed changes

Ramakrishna Pallala added 2 commits August 21, 2018 19:45

samples: subsys: Add sample app to demo OS managed PM

7f6d3e5

Add a sample application to demonstrate OS managed Power Management framework. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

ramakrishnapallala force-pushed the pwr-mgmt-fw branch from d31d784 to 7f6d3e5 Compare August 21, 2018 14:23

ramakrishnapallala changed the title ~~[RFC] OS Managed Power Management framework~~ OS Managed Power Management framework Aug 21, 2018

pizi-nordic reviewed Aug 21, 2018

View reviewed changes

nashif approved these changes Aug 22, 2018

View reviewed changes

nashif merged commit b69d286 into zephyrproject-rtos:master Aug 22, 2018

pdunaj reviewed Aug 22, 2018

View reviewed changes

subsys/power/device.c

"clk_k32src",

"clk_m16src",

"sys_clock",

"UART_0",

Copy link

Collaborator

pdunaj Aug 22, 2018

Choose a reason for hiding this comment

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

UART may be disabled

OS Managed Power Management framework #9416

OS Managed Power Management framework #9416

Conversation

ramakrishnapallala commented Aug 14, 2018 • edited Loading

codecov-io commented Aug 14, 2018 • edited Loading

Codecov Report

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

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

ramakrishnapallala commented Aug 16, 2018

ramakrishnapallala commented Aug 17, 2018

ramakrishnapallala commented Aug 20, 2018

nashif commented Aug 20, 2018

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

pizi-nordic commented Aug 21, 2018

nashif commented Aug 21, 2018

pizi-nordic commented Aug 21, 2018

pizi-nordic commented Aug 21, 2018 • edited Loading

pizi-nordic Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramakrishnapallala commented Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

ramakrishnapallala commented Aug 14, 2018 •

edited

Loading

codecov-io commented Aug 14, 2018 •

edited

Loading

pizi-nordic commented Aug 21, 2018 •

edited

Loading

pizi-nordic Aug 21, 2018 •

edited

Loading

ramakrishnapallala commented Aug 22, 2018 •

edited

Loading