pm: optimize resources #39397

gmarull · 2021-10-13T12:24:08Z

It is well known that PM subsystem has never been optimized in terms of
resource usage. The situation is particularly bad in case the PM runtime
API is enabled. What this patch does is to move the responsibility of PM
resource definition to the device driver like this:

Device is responsible to define PM resources, using a new set of
macros: PM_DEVICE_*DEFINE().
DEVICE_*DEFINE macro accepts a reference to the device PM state, which
can be obtained using PM_DEVICE_*REF() set of macros. This
allows device to initialize the dev->pm reference.

This method decouples a bit more PM from devices since devices just keep
a reference to the device PM state. It also means that future PM changes
will have less chances to impact all devices, but only devices that
support PM.

Stats for west build -b nrf52840dk_nrf52840 samples/hello_world -- -DCONFIG_PM=y -DCONFIG_PM_DEVICE=y -DCONFIG_PM_DEVICE_RUNTIME=y

main:

Memory region         Used Size  Region Size  %age Used
           FLASH:       19632 B         1 MB      1.87%
            SRAM:        7360 B       256 KB      2.81%
        IDT_LIST:          0 GB         2 KB      0.00%

This PR:

Memory region         Used Size  Region Size  %age Used
           FLASH:       19188 B         1 MB      1.83%
            SRAM:        6912 B       256 KB      2.64%
        IDT_LIST:          0 GB         2 KB      0.00%

ceolin · 2021-10-13T23:42:05Z

I think get rid of the pm struct with the when PM_DISABLE is passed instead of the callback is a good thing. The other change I have concerns, I think it makes the code more complicated, harder to read and maintain. Instead of jump in optimizations I think we should review the async usage and implementation.

This is not a 👎 but I have doubts if the benefits are greater than the cons.

gmarull · 2021-10-14T09:43:43Z

I think get rid of the pm struct with the when PM_DISABLE is passed instead of the callback is a good thing. The other change I have concerns, I think it makes the code more complicated, harder to read and maintain. Instead of jump in optimizations I think we should review the async usage and implementation.

This is not a -1 but I have doubts if the benefits are greater than the cons.

Since an API change has not been discarded yet (usage of PM_DISABLE), I think it's worth considering all options to try avoiding yet another change in the future. In some other proposals, like pinctrl, optimizing memory usage has been a critical factor on the API design. If savings are considerable, increased complexity could be justified up to a certain point. I think Zephyr should be consistent on this aspect. I'm currently reviewing the runtime API to see if it can be simplified somehow, so this option may become less relevant. Let's see.

yperess

Finally, if we were to add a binding API, we would need a few changes to subsys/pm I would add the following to the build:

zephyr_sources_ifdef(CONFIG_PM    pm.c)

Here I would add an initializer function that will run (probably post kernel). It would iterate through the new section added for the struct pm_device and set the .pm pointers for all the devices which had PM bindings.

(these are just my thoughts/suggestions, please implement as you see fit)

yperess · 2021-10-19T16:55:22Z

include/device.h

+#define SYS_DEVICE_DEFINE(drv_name, init_fn, pm_enable, level, prio)	\
 	DEVICE_DEFINE(Z_SYS_NAME(init_fn), drv_name, init_fn,		\
-		      pm_control_fn,					\
+		      pm_enable,					\


from meeting: leave this as argument, but ignore

yperess · 2021-10-19T16:55:47Z

include/device.h

+#define DEVICE_DEFINE(dev_name, drv_name, init_fn, pm_enable,		\
 		      data_ptr, cfg_ptr, level, prio, api_ptr)		\
 	Z_DEVICE_DEFINE(DT_INVALID_NODE, dev_name, drv_name, init_fn,	\
-			pm_control_fn,					\
+			pm_enable,					\


from meeting: leave this as argument, but ignore

yperess · 2021-10-19T16:56:08Z

include/device.h

+#define DEVICE_DT_DEFINE(node_id, init_fn, pm_enable,			\
 			 data_ptr, cfg_ptr, level, prio,		\
 			 api_ptr, ...)					\
 	Z_DEVICE_DEFINE(node_id, Z_DEVICE_DT_DEV_NAME(node_id),		\
 			DEVICE_DT_NAME(node_id), init_fn,		\
-			pm_control_fn,					\
+			pm_enable,					\


from meeting: leave this as argument, but ignore

yperess · 2021-10-19T17:43:24Z

include/device.h

-		Z_DEVICE_DEFINE_PM_INIT(dev_name, pm_control_fn)
+#define Z_DEVICE_DEFINE_INIT(node_id, dev_name, pm_enable)		\
+	.handles = Z_DEVICE_HANDLE_NAME(node_id, dev_name),		\
+	COND_CODE_1(pm_enable, (Z_DEVICE_DEFINE_PM_INIT(dev_name)), ())


.pm will always be NULL by default until the subsystem runs so no need for COND_CODE_1

yperess · 2021-10-19T17:47:25Z

include/pm/device.h

+#define Z_PM_DEVICE_STATE_DEFINE(node_id, dev_name, pm_control, async)	       \
+	COND_CODE_1(async, (Z_PM_DEVICE_RUNTIME_ASYNC_DEFINE(dev_name);), ())  \
+	static struct pm_device Z_PM_DEVICE_STATE_NAME(dev_name) =	       \
+	Z_PM_DEVICE_INIT(Z_PM_DEVICE_STATE_NAME(dev_name), dev_name, node_id,  \
+			 pm_control, async)
+
+/**
+ * Define device PM state for the given device name.
+ *
+ * @param dev_name Device name.
+ * @param pm_control PM control callback.
+ * @param async #PM_NO_ASYNC or #PM_ASYNC.
+ */
+#define PM_DEVICE_STATE_DEFINE(dev_name, pm_control, async)		       \
+	Z_PM_DEVICE_STATE_DEFINE(DT_INVALID_NODE, dev_name, pm_control, async)
+
+/**
+ * Define device PM state for the given node identifier.
+ *
+ * @param node_id Node identifier.
+ * @param pm_control PM control callback.
+ * @param async #PM_NO_ASYNC or #PM_ASYNC.
+ */
+#define PM_DEVICE_STATE_DT_DEFINE(node_id, pm_control, async)		       \
+	Z_PM_DEVICE_STATE_DEFINE(node_id, Z_DEVICE_DT_DEV_NAME(node_id),       \
+				 pm_control, async)
+
+/**
+ * Define device PM state for the given instance.
+ *
+ * @param idx Instance index.
+ * @param pm_control PM control callback.
+ * @param async #PM_NO_ASYNC or #PM_ASYNC.
+ */
+#define PM_DEVICE_STATE_DT_INST_DEFINE(idx, pm_control, async)		       \
+	Z_PM_DEVICE_STATE_DEFINE(DT_DRV_INST(idx),			       \
+				 Z_DEVICE_DT_DEV_NAME(DT_DRV_INST(idx)),       \
+				 pm_control, async)


This would now be: PM_DEVICE_DT_BIND(dev_node_id, pm_control) along with the other permutations (I believe async will be added later to keep the PR simple)

yperess · 2021-10-19T17:49:17Z

include/pm/device.h

+#define Z_PM_DEVICE_INIT(obj, dev_name, node_id, pm_control_, async)	\
 	{								\
-		INIT_PM_DEVICE_RUNTIME(obj)				\
+		INIT_PM_DEVICE_RUNTIME(obj, dev_name, async)		\


This should now include a const struct device *dev field so we can map from the struct pm_device back up.

yperess · 2021-10-19T17:56:33Z

include/pm/device.h

+ */
+#define Z_PM_DEVICE_STATE_DEFINE(node_id, dev_name, pm_control, async)	       \
+	COND_CODE_1(async, (Z_PM_DEVICE_RUNTIME_ASYNC_DEFINE(dev_name);), ())  \
+	static struct pm_device Z_PM_DEVICE_STATE_NAME(dev_name) =	       \


This should be in its own linker section with a start/end pointer so we can iterate

gmarull · 2021-10-21T08:52:41Z

@yperess I have updated the PR with a simpler approach: a reference to the state is passed to the device definition macros (I think @carlescufi mentioned this option), making the change less impactful (NULL can continue to be used). I'll continue exploring the bind option and see if I can come up with something, or just feel free to work on it/contact me.

keith-zephyr

You also need to update subsys/pm/device.c to change all if (pm->action_cb == NULL) checks to if (pm == NULL)

include/device.h

include/pm/device.h

ceolin

That is the right direction IMHO. I think the late binding is better but that can be done on top of this one.

This change takes the following approach: 1. Create a new `struct device_subsystem_hooks` which will be extended anytime that a new subsystem requires direct access from a device. This will add `sizeof(void*)` to the device struct (less than adding `struct pm_device` per instance. Then an additional pointer per enabled subsystem that hooks to the device struct. 2. The subsystem is responsible for creating the device-to-pm pointer by adding a `const struct device*` to the `struct pm_device` (again a cost of 1 pointer per enabled PM device). The subsystem then registers a `SYS_INIT` which will iterate through the PM structs and set the pointers from the device back to the PM struct. This commit demonstrates a tradeoff. There's a slight increased cost in memory over PR zephyrproject-rtos#39397 in favor of an extensible API that allows device declaration to be separate from subsystems. Signed-off-by: Yuval Peress <peress@google.com>

yperess · 2021-11-04T03:08:19Z

@gmarull @keith-zephyr I finally got things working, see yperess@067f701 for an example. This builds:

$ west build -p -b nrf52840dk_nrf52840 samples/subsys/pm/device_pm/

include/device.h

include/pm/device.h

drivers/gpio/gpio_stm32.c

gmarull · 2021-11-17T14:18:07Z

@yperess can you re-review?

Define the device using DEVICE_DEFINE macro, so that a single option can be used regardless of PM being enabled or not. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

The macro already mentions in the docstrings that PM is not supported: "Invokes DEVICE_DEFINE() with no power management support". This patch removed the PM entry from the macro and ajusts its uses. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

It is well known that PM subsystem has never been optimized in terms of resource usage. The situation is particularly bad in case the PM runtime API is enabled. What this patch does is to move the responsability of PM resource definition to the device like this: - Device is responsible to define PM resources, using a new set of macros: PM_DEVICE_*DEFINE(). - DEVICE_*DEFINE macro accepts a reference to the device PM state, which can be obtained using PM_DEVICE_*REF() set of macros. This allows device to initialize the dev->pm reference. This method decouples a bit more PM from devices since devices just keep a reference to the device PM state. It also means that future PM changes will have less chances to impact all devices, but only devices that support PM. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Port some drivers to the recently introduced macros to showcase its usage and be able to do some initial testing (nRF52840). Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Use PM_DEVICE_STATE_DEFINE to define PM state. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Use recently introduced PM macros to define device PM resources. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Increase main stack size (in 128 bytes) to make these tests run successfully on qemu_x86 platform. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

Raised concerns do not apply to the current approach (bind option has not been implemented at this stage). I'll investigate if it is feasible using some sort of post-processing as we do in other areas.

gmarull · 2021-11-19T09:06:42Z

Confirmed CI passes, dropped buildkite resorce increase patch.

github-actions bot added area: API Changes to public APIs area: Power Management area: Timer Timer labels Oct 13, 2021

gmarull mentioned this pull request Oct 13, 2021

Pm struct optimization #39166

Closed

gmarull requested review from keith-zephyr, JordanYates and ceolin October 13, 2021 12:25

gmarull added the RFC Request For Comments: want input from the community label Oct 13, 2021

gmarull requested a review from carlocaione October 13, 2021 15:26

keith-zephyr mentioned this pull request Oct 14, 2021

Optimize PM structures #39286

Closed

yperess previously requested changes Oct 19, 2021

View reviewed changes

gmarull force-pushed the pm-optimize-runtime branch 3 times, most recently from 42feee5 to 1d4e455 Compare October 21, 2021 08:48

gmarull requested review from carlescufi, nashif and mbolivar-nordic October 21, 2021 10:43

keith-zephyr requested changes Oct 21, 2021

View reviewed changes

include/device.h Outdated Show resolved Hide resolved

gmarull force-pushed the pm-optimize-runtime branch from 1d4e455 to 16f99e3 Compare October 22, 2021 08:42

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Oct 22, 2021

gmarull force-pushed the pm-optimize-runtime branch from 16f99e3 to 980de70 Compare October 22, 2021 08:45

keith-zephyr requested changes Oct 25, 2021

View reviewed changes

include/pm/device.h Outdated Show resolved Hide resolved

include/pm/device.h Outdated Show resolved Hide resolved

include/pm/device.h Outdated Show resolved Hide resolved

gmarull force-pushed the pm-optimize-runtime branch from 980de70 to 822e4df Compare October 26, 2021 09:04

ceolin approved these changes Oct 26, 2021

View reviewed changes

mbolivar-nordic approved these changes Nov 12, 2021

View reviewed changes

keith-zephyr requested changes Nov 16, 2021

View reviewed changes

include/device.h Outdated Show resolved Hide resolved

include/pm/device.h Outdated Show resolved Hide resolved

drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved

gmarull force-pushed the pm-optimize-runtime branch from ec2f66b to 7dffb6d Compare November 16, 2021 18:09

gmarull requested review from andyross and rlubos as code owners November 16, 2021 18:09

gmarull requested review from keith-zephyr and removed request for rlubos and alwa-nordic November 16, 2021 18:12

keith-zephyr approved these changes Nov 16, 2021

View reviewed changes

jhedberg approved these changes Nov 17, 2021

View reviewed changes

gmarull force-pushed the pm-optimize-runtime branch from 7dffb6d to 37bda0d Compare November 17, 2021 10:18

gmarull force-pushed the pm-optimize-runtime branch from 37bda0d to 0b7b32e Compare November 17, 2021 21:21

gmarull requested a review from GTLin08 as a code owner November 17, 2021 21:21

GTLin08 approved these changes Nov 18, 2021

View reviewed changes

gmarull added 7 commits November 18, 2021 10:09

drivers: interrupt_controller: i/loapic: use DEVICE_DEFINE

185ec9f

Define the device using DEVICE_DEFINE macro, so that a single option can be used regardless of PM being enabled or not. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

drivers: use new PM macros

abad930

Port some drivers to the recently introduced macros to showcase its usage and be able to do some initial testing (nRF52840). Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

tests: pm: use new PM macros

32c17c5

Use PM_DEVICE_STATE_DEFINE to define PM state. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

samples: pm: use new PM macros

a93d7cd

Use recently introduced PM macros to define device PM resources. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

tests: posix: common: increase main stack for TLS tests

f128a7e

Increase main stack size (in 128 bytes) to make these tests run successfully on qemu_x86 platform. Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>

gmarull force-pushed the pm-optimize-runtime branch from 0b7b32e to f128a7e Compare November 18, 2021 09:43

gmarull added the DNM This PR should not be merged (Do Not Merge) label Nov 18, 2021

ceolin approved these changes Nov 18, 2021

View reviewed changes

gmarull force-pushed the pm-optimize-runtime branch from 64551db to f128a7e Compare November 19, 2021 09:06

gmarull removed the DNM This PR should not be merged (Do Not Merge) label Nov 19, 2021

carlescufi merged commit a6a296d into zephyrproject-rtos:main Nov 19, 2021

gmarull deleted the pm-optimize-runtime branch November 19, 2021 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pm: optimize resources #39397

pm: optimize resources #39397

gmarull commented Oct 13, 2021 •

edited

ceolin commented Oct 13, 2021

gmarull commented Oct 14, 2021 •

edited

yperess left a comment

yperess Oct 19, 2021

yperess Oct 19, 2021

yperess Oct 19, 2021

yperess Oct 19, 2021

yperess Oct 19, 2021

yperess Oct 19, 2021

yperess Oct 19, 2021

gmarull commented Oct 21, 2021

keith-zephyr left a comment

ceolin left a comment

yperess commented Nov 4, 2021

gmarull commented Nov 17, 2021

gmarull commented Nov 19, 2021

pm: optimize resources #39397

pm: optimize resources #39397

Conversation

gmarull commented Oct 13, 2021 • edited

ceolin commented Oct 13, 2021

gmarull commented Oct 14, 2021 • edited

yperess left a comment

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

yperess Oct 19, 2021

Choose a reason for hiding this comment

gmarull commented Oct 21, 2021

keith-zephyr left a comment

Choose a reason for hiding this comment

ceolin left a comment

Choose a reason for hiding this comment

yperess commented Nov 4, 2021

gmarull commented Nov 17, 2021

gmarull commented Nov 19, 2021

gmarull commented Oct 13, 2021 •

edited

gmarull commented Oct 14, 2021 •

edited