pm: Add option to have synchronous runtime power management #67424

nordic-krch · 2024-01-10T11:04:53Z

Current solution assumes that power management operations may be blocking, asynchronous and take unknown amount of time. Due to this assumption runtime PM API cannot be effectively used from the interrupt context. Zephyr has few driver APIs which can be used from an interrupt context (UART async and polling, rtio work also targets asynchronous ops) and now use of runtime PM is limited in those cases. Given that In many cases suspending or resuming of a device is limited to just a few register writes having whole infrastructure with semaphore, event and work queue is like using sledgehammer to crack a nut.

Patch introduces a new type of PM device - synchronous PM. If device is specified as capable of synchronous PM operations then device runtime getting and putting is executed in the critical section. In that case, runtime API can be used from an interrupt context. Additionally, this approach reduces RAM needed for PM device (104 -> 20 bytes of RAM on ARM Cortex-M) because PM data structure is limited and does not contain work,event and semaphore.

If driver has synchronous PM operations (like only changing pinctrl state) then PM device can be defined as

PM_DEVICE_SYNC_DEFINE(test_driver, test_driver_action); // instead of PM_DEVICE_DEFINE

PM_DEVICE_SYNC_DT_DEFINE(UARTE(idx), uarte_nrfx_pm_action) // instead of PM_DEVICE_DT_DEFINE

Runtime API stays the same. Synchronous PM device property is detected inside the API.

JordanYates · 2024-01-11T22:47:14Z

The feature makes a lot of sense, I'm not a fan of the name however.

The current basic operations are described as synchronous and asynchronous depending on whether the pm_device_runtime_* call blocks or not. This now introduces a second synchronous meaning, whether the transition is run directly by the call or deferred out to a workqueue. Maybe borrow terminology from ISR and call it direct?

nordic-krch · 2024-01-12T07:26:59Z

@JordanYates thanks for the comment. I knew already that name is wrong and was hoping to get some good suggestions in review. direct sounds good to me.

ceolin

Thanks for the pr. I think it makes sense, but we need better nomenclature / clarification here.

The current implementation supports synchronous calls and can be called from ISR. So, this pr is not introducing a new feature. What it is doing is saving resources for devices with no complex power management. It is reducing the size of pm struct for these devices and avoid additional cycles when they are suspending / resuming.

But this makes things confuse because no PM_DEVICE_FLAG_SYNC_LOCKED_CAPABLE devices can also be called from ISR ...

What I think can be a good route is, devices without this flag can not longer be called from ISR (anyways it is risky right now since we don't have any hint if the pm->cb call will block, and renaming SYNC_LOCKED with something more explicit like ISR_SAFE or so.

ceolin · 2024-01-12T21:49:55Z

subsys/pm/device_runtime.c

+	bool enabled = flags & BIT(PM_DEVICE_FLAG_RUNTIME_ENABLED);
+
+	if (!enabled) {
+		return 0;
+	}
+


you can avoid this check moving this function call in pm_device_runtime_get

ceolin · 2024-01-12T21:51:58Z

subsys/pm/device_runtime.c

+	if (pm->base.usage == 1) {
+		ret = pm->base.action_cb(dev, PM_DEVICE_ACTION_RESUME);
+		if (ret < 0) {
+			return ret;


this will make the reference count unbalanced. You can change the check for if (pm->base.usage == 0) and increment the reference after this call succeed.

ceolin · 2024-01-12T21:52:48Z

subsys/pm/device_runtime.c

+		}
+		pm->base.state = PM_DEVICE_STATE_ACTIVE;
+		if (flags & BIT(PM_DEVICE_FLAG_PD_CLAIMED)) {
+			const struct device *domain = PM_DOMAIN(&pm->base);


the domain has to be get before the device when getting and after when putting.

ceolin · 2024-01-12T22:17:57Z

include/zephyr/pm/device.h

+ * be performed in the critical section as they are short and non-blocking.
+ * Runtime PM API can be used from any context in that case.
+ */
+struct pm_device_rt_sync {


maybe call these structs

struct pm_base; struct pm_device; struct pm_device_isr;

?

isn't pm_base to generic. maybe pm_device_base?

yep ... pm_device_base sounds better.

struct pm_device_base; struct pm_device; struct pm_device_isr;

ceolin · 2024-01-12T23:59:43Z

include/zephyr/pm/device.h

+	static struct pm_device_rt Z_PM_DEVICE_NAME(dev_id) =	\
+	Z_PM_DEVICE_INIT(Z_PM_DEVICE_NAME(dev_id), node_id, pm_action_cb)
+
+#define Z_PM_DEVICE_SYNC_DEFINE(node_id, dev_id, pm_action_cb)				\


Can't we avoid additional macros ...
Maybe something around ...

#define PM_DEVICE_DEFINE(dev_id, pm_action_cb, ...) \ COND_CODE_1(Z_IS_EMPTY(__VA_ARGS__), \ (Z_PM_DEVICE_DEFINE(DT_INVALID_NODE, dev_id, pm_action_cb)), \ (Z_PM_DEVICE_SYNC_DEFINE(DT_INVALID_NODE, dev_id, pm_action_cb)))

Sounds good to me if we extend API macro PM_DEVICE_DEFINE(node, dev, cb, ...) I am not sure what would be the additional argument. Should it be something like PM_DEVICE_DEFINE(..., PM_DEVICE_TYPE_<name>) where flag can indicate generic device or the new one and for backward compatibility no argument would mean the generic runtime pm?

Extended that was. Optional flag can be provided to indicate ISR safe type.
PM_DEVICE_DEVICE(dev, cb) standard device
PM_DEVICE_DEVICE(dev, cb, 0) standard device
PM_DEVICE_DEVICE(dev, cb, PM_DEVICE_ISR_SAFE) ISR safe device

PM_DEVICE_ISR_SAFE is just defined as 1.

nordic-krch · 2024-01-16T07:30:54Z

@ceolin

The current implementation supports synchronous calls and can be called from ISR. So, this pr is not introducing a new feature.

Current implementation supports putting from isr (async put) but getting may fail if we interrupt another getting operation (it will fail on taking locking semaphore). If operation is not reliable then we cannot fully claim that it is supported.

nordic-krch · 2024-01-16T09:53:03Z

Would be good to agree on the name then. DIRECT(proposed by @JordanYates) ISR_SAFE (proposed by @ceolin ). I don't have strong opinions.

ceolin · 2024-01-16T17:21:55Z

@ceolin

The current implementation supports synchronous calls and can be called from ISR. So, this pr is not introducing a new feature.

Current implementation supports putting from isr (async put) but getting may fail if we interrupt another getting operation (it will fail on taking locking semaphore). If operation is not reliable then we cannot fully claim that it is supported.

That is how you take it, the way I see is that the feature is supported and that is its behavior when called from ISR. But, that is not really the point I wanted to make. I agree with you that this proposed approach is better and gives up something more realiable / deterministic.

Note that naming is not the only thing I've pointed, I think that if we have this explicit option, we should prohibit devices not using it from be get/put from ISR.

In many cases suspending or resuming of a device is limited to just a few register writes. Current solution assumes that those operations may be blocking, asynchronous and take a lot of time. Due to this assumption runtime PM API cannot be effectively used from the interrupt context. Zephyr has few driver APIs which can be used from an interrupt context and now use of runtime PM is limited in those cases. Patch introduces a new type of PM device - synchronous PM. If device is specified as capable of synchronous PM operations then device runtime getting and putting is executed in the critical section. In that case, runtime API can be used from an interrupt context. Additionally, this approach reduces RAM needed for PM device (104 -> 20 bytes of RAM on ARM Cortex-M). Signed-off-by: Krzysztof Chruściński <[email protected]>

Extended test to support case when PM device is defined as synchronous. Signed-off-by: Krzysztof Chruściński <[email protected]>

Extend macros from creating a PM device with an optional argument which indicate whether type of device is ISR_SAFE or not. Signed-off-by: Krzysztof Chruściński <[email protected]>

carlescufi · 2024-02-01T13:49:09Z

@ceolin please take another look

ceolin

I think some internal apis can have their name changed, from sync to isr to avoid confusion, but this can be done later since does not change the behavior or public interface.

Thanks a lot for putting this together !

marc-hb · 2024-02-16T07:46:54Z

include/zephyr/device.h

@@ -900,7 +906,7 @@ static inline bool z_impl_device_is_ready(const struct device *dev)
 		.state = (state_),                                             \
 		.data = (data_),                                               \
 		IF_ENABLED(CONFIG_DEVICE_DEPS, (.deps = (deps_),)) /**/        \
-		IF_ENABLED(CONFIG_PM_DEVICE, (.pm = (pm_),)) /**/              \
+		IF_ENABLED(CONFIG_PM_DEVICE, (.pm_base = (pm_),)) /**/         \


Looks like this broke gcc 4.2.0 again.

Seems very similar to #68118

-c zephyr/soc/xtensa/intel_adsp/common/mem_window.c zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62: error: unknown field ‘pm_base’ specified in initializer cc1: warnings being treated as errors zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62: warning: missing braces around initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:62: warning: (near initialization for ‘__device_dts_ord_66.<anonymous>’) zephyr/soc/xtensa/intel_adsp/common/mem_window.c:65: error: unknown field ‘pm_base’ specified in initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:65: warning: missing braces around initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:65: warning: (near initialization for ‘__device_dts_ord_67.<anonymous>’) zephyr/soc/xtensa/intel_adsp/common/mem_window.c:68: error: unknown field ‘pm_base’ specified in initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:68: warning: missing braces around initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:68: warning: (near initialization for ‘__device_dts_ord_68.<anonymous>’) zephyr/soc/xtensa/intel_adsp/common/mem_window.c:71: error: unknown field ‘pm_base’ specified in initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:71: warning: missing braces around initializer zephyr/soc/xtensa/intel_adsp/common/mem_window.c:71: warning: (near initialization for ‘__device_dts_ord_69.<anonymous>’)

Fix submitted:

pm: use braces for .pm_base inside anonymous union initializer #69270

nordic-krch requested review from gmarull, ceolin and dcpleung January 10, 2024 11:04

zephyrbot added area: Device Model area: Power Management labels Jan 10, 2024

zephyrbot requested review from JordanYates, nashif, tbursztyka, teburd and tmleman January 10, 2024 11:05

zephyrbot assigned ceolin Jan 10, 2024

nordic-krch force-pushed the pm_runtime_sync branch from f136987 to d0aa117 Compare January 10, 2024 13:35

ceolin requested changes Jan 12, 2024

View reviewed changes

nordic-krch force-pushed the pm_runtime_sync branch from d0aa117 to db501ed Compare January 16, 2024 09:41

nordic-krch force-pushed the pm_runtime_sync branch from db501ed to 0481ae1 Compare January 18, 2024 07:31

carlescufi requested a review from ceolin January 18, 2024 12:36

nordic-krch force-pushed the pm_runtime_sync branch 2 times, most recently from 1f8b8ed to 3385fd7 Compare January 29, 2024 08:04

zephyrbot added the area: Kernel label Jan 29, 2024

zephyrbot requested review from andyross, cfriedt, npitre and peter-mitsis January 29, 2024 08:04

nordic-krch added 2 commits January 29, 2024 10:18

tests: pm: device_runtime_api: Extend with synchronous device

984165f

Extended test to support case when PM device is defined as synchronous. Signed-off-by: Krzysztof Chruściński <[email protected]>

pm: device_runtime: Extend existing API with ISR_SAFE mode

791e654

Extend macros from creating a PM device with an optional argument which indicate whether type of device is ISR_SAFE or not. Signed-off-by: Krzysztof Chruściński <[email protected]>

nordic-krch force-pushed the pm_runtime_sync branch from 3385fd7 to 791e654 Compare January 29, 2024 09:18

ceolin approved these changes Feb 1, 2024

View reviewed changes

carlescufi approved these changes Feb 1, 2024

View reviewed changes

carlescufi merged commit ee13631 into zephyrproject-rtos:main Feb 1, 2024
26 checks passed

marc-hb mentioned this pull request Feb 16, 2024

[BUG] Zephyr baseline update blocked thesofproject/sof#8818

Closed

marc-hb reviewed Feb 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pm: Add option to have synchronous runtime power management #67424

pm: Add option to have synchronous runtime power management #67424

nordic-krch commented Jan 10, 2024

JordanYates commented Jan 11, 2024

nordic-krch commented Jan 12, 2024

ceolin left a comment

ceolin Jan 12, 2024

ceolin Jan 12, 2024

ceolin Jan 12, 2024

ceolin Jan 12, 2024

nordic-krch Jan 16, 2024

ceolin Jan 22, 2024

nordic-krch Jan 29, 2024

ceolin Jan 12, 2024

nordic-krch Jan 16, 2024

nordic-krch Jan 29, 2024

nordic-krch commented Jan 16, 2024

nordic-krch commented Jan 16, 2024

ceolin commented Jan 16, 2024

carlescufi commented Feb 1, 2024

ceolin left a comment

marc-hb Feb 16, 2024

marc-hb Feb 21, 2024

pm: Add option to have synchronous runtime power management #67424

pm: Add option to have synchronous runtime power management #67424

Conversation

nordic-krch commented Jan 10, 2024

JordanYates commented Jan 11, 2024

nordic-krch commented Jan 12, 2024

ceolin left a 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

nordic-krch commented Jan 16, 2024

nordic-krch commented Jan 16, 2024

ceolin commented Jan 16, 2024

carlescufi commented Feb 1, 2024

ceolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment