Skip to content

Specify and document PM_DEVICE behavior, including PM_DEVICE=n and device_deinit() #90275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bjarki-andreasen
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen commented May 21, 2025

With the introduction of device_deinit(), we now have a path for suspending a device driver even if DEVICE_PM is not enabled, like we have for resume on device driver init, and we need to be able to ensure safe device_deinit() in case DEVICE_PM is enabled. For now, this is a simple check ensuring the device is not in use. The test suite for pm_device_driver_init() has been extended to cover the new API.

Copy link
Collaborator

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

What is the purpose of this function? It seems odd that it is running functions when CONFIG_PM_DEVICE=n but doesn't do anything when CONFIG_PM_DEVICE=y?

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented May 21, 2025

What is the purpose of this function? It seems odd that it is running functions when CONFIG_PM_DEVICE=n but doesn't do anything when CONFIG_PM_DEVICE=y?

Check out subsys/pm/device.c for now it simply ensures the device is not in use (suspended or off) to prevent deinit of a device which is in use. Even if it did nothing if CONFIG_PM_DEVICE=n, that would still be fine IMO, its there to allow device_deinit() to undo what device_init() did through pm_device_driver_init()

Copy link
Collaborator

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

For now, this is a simple check ensuring the device is not in use

What is stopping a final implementation from being implemented now? The function should have defined semantics, and they should be implemented properly when the function is introduced, not modified at some point in "the future".

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented May 22, 2025

For now, this is a simple check ensuring the device is not in use

What is stopping a final implementation from being implemented now? The function should have defined semantics, and they should be implemented properly when the function is introduced, not modified at some point in "the future".

This is all we need for now, I don't think there is a "final" implementation, its just a hook for doing whatever is required of PM at a given time :)

return rc;
}

rc = action_cb(dev, PM_DEVICE_ACTION_TURN_OFF);
Copy link
Collaborator

@JordanYates JordanYates May 22, 2025

Choose a reason for hiding this comment

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

This doesn't seem right, the device is not literally being turned off, just the software construct destroyed.
This would lead to GPIO's being put into incorrect states if used generically, which the PM API is.

Overloading the meaning of PM_DEVICE_ACTION_TURN_OFF is a bad idea IMO.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen May 22, 2025

Choose a reason for hiding this comment

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

I'm okay with removing the call to TURN_OFF, given that all resources like GPIOs are released as part of device_deinit(), I think you are right that it makes little sense to act as if the driver is being turned off before being deinitialized.

Copy link
Member

Choose a reason for hiding this comment

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

Agree about it

@bjarki-andreasen bjarki-andreasen force-pushed the pm-device-driver-deinit branch from 4ea0b00 to 0f5a01c Compare May 22, 2025 12:20
@bjarki-andreasen bjarki-andreasen force-pushed the pm-device-driver-deinit branch from 0f5a01c to 4a6a1d6 Compare May 25, 2025 18:55
ceolin
ceolin previously approved these changes May 26, 2025
@bjarki-andreasen
Copy link
Collaborator Author

@JordanYates could you revisit this one?

Copy link
Collaborator

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

It's still not clear to me how this helps to reliably implement the requirements of the device_deinit API.

* When a device is de-initialized, it will release any resources it has
* acquired (e.g. pins, memory, clocks, DMA channels, etc.) and its status will
* be left as in its reset state.
*
* @warning It is the responsibility of the caller to ensure that the device is
* ready to be de-initialized.

It's on the caller to determine whether the device can be de-initialized, not the implementation.

Why is STATE_SUSPENDED any more likely to be the "reset state" of a device than STATE_ACTIVE? In many cases the suspend state is explicitly not the reset state of a device.

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented May 29, 2025

It's still not clear to me how this helps to reliably implement the requirements of the device_deinit API.

* When a device is de-initialized, it will release any resources it has
* acquired (e.g. pins, memory, clocks, DMA channels, etc.) and its status will
* be left as in its reset state.
*
* @warning It is the responsibility of the caller to ensure that the device is
* ready to be de-initialized.

It's on the caller to determine whether the device can be de-initialized, not the implementation.

This to my knowledge was intended to clearly indicate that there is no reference counting or other tracking of who is using the device built in to the device model (which there was with an early version of device_deinit()), not that no checks are allowed to make sure it makes sense. Otherwise surely

zephyr/kernel/device.c

Lines 150 to 152 in 600cae6

if (!dev->state->initialized) {
return -EPERM;
}
would not exist :) The function returns a value, it can fail, we are allowed to do additional sanity checking.

Why is STATE_SUSPENDED any more likely to be the "reset state" of a device than STATE_ACTIVE? In many cases the suspend state is explicitly not the reset state of a device.

If CONFIG_PM_DEVICE=n, state is not checked, instead it just calls the suspend action. If CONFIG_PM_DEVICE=y, then device has to be suspended, thus STATE_SUSPENDED or STATE_OFF are explicitly states from which the "reset state" can be safely entered. The only difference between STATE_SUSPENDED and STATE_OFF would be whether the hardware block lost power, and thus was implicitly reset.

@JordanYates
Copy link
Collaborator

As a concrete request, please document what the function is actually supposed to be doing beyond just @brief Deinit a device driver. It is easier to check that an intended behavior makes sense and that an implementation matches the intended behavior than to try and reason about how a particular implementation is supposed to fit into the larger device_deinit API based only on code.

@@ -731,6 +742,11 @@ static inline int pm_device_driver_init(const struct device *dev, pm_device_acti
return 0;
}

static inline int pm_device_driver_deinit(const struct device *dev, pm_device_action_cb_t action_cb)
{
return action_cb(dev, PM_DEVICE_ACTION_SUSPEND);
Copy link
Member

Choose a reason for hiding this comment

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

If PM_DEVICE is not enabled then why is suspend action being called at all? Do I misunderstand something? I thought that the idea is that devices will always be active when PM_DEVICE is disabled.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen May 30, 2025

Choose a reason for hiding this comment

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

This is called when a device is deinitialized, only devices which are initialized are active :) Its for use with device_deinit()

Copy link
Member

Choose a reason for hiding this comment

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

Yes I understand that but it doesn't answer my question, which is that if PM_DEVICE is off, why are we calling suspend action at all in any circumstance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same reason we call TURN_ON and RESUME if PM_DEVICE=n Its to use the same path for resuming and suspending a device, whether PM_DEVICE is enabled or not. Otherwise users would be duplicating code in implementations of device init and deinit in drivers :)

@bjarki-andreasen bjarki-andreasen force-pushed the pm-device-driver-deinit branch from af73637 to 32e5586 Compare June 14, 2025 15:53
@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jun 14, 2025

@decsny I added an entry to cover the case you mentioned here #90946 (comment)

specifically in the example driver's resume function, I added:

           /*
            * Release devices currenty not needed by device. In this case, we
            * are releasing the bus and the enable pin.
            *
            * The device driver would keep the bus ACTIVE while the device is
            * ACTIVE in cases of high throughput or unsolicitet data on the
            * bus, to avoid inefficient RESUME/SUSPEND cycles of the bus
            * for every transaction, and allowing reception of unsolicitet
            * data on busses like UART.
            */

@bjarki-andreasen bjarki-andreasen force-pushed the pm-device-driver-deinit branch 2 times, most recently from c0426c6 to ac7a10a Compare June 14, 2025 15:57
@bjarki-andreasen
Copy link
Collaborator Author

@JordanYates could you take another look?

JordanYates
JordanYates previously approved these changes Jun 17, 2025
@bjarki-andreasen
Copy link
Collaborator Author

@decsny could you take a look?

ceolin
ceolin previously approved these changes Jun 17, 2025
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

Thanks ! It looks great!

@carlescufi carlescufi removed the Architecture Review Discussion in the Architecture WG required label Jun 17, 2025
@@ -628,6 +628,9 @@ bool pm_device_is_powered(const struct device *dev);
* @ref PM_DEVICE_STATE_OFF, or @ref PM_DEVICE_STATE_SUSPENDED if device can
* never be powered off.
*
* @note If CONFIG_PM_DEVICE=n, PM_DEVICE_ACTION_TURN_ON followed by
* PM_DEVICE_ACTION_RESUME will be invoked unconditionally.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "invoked on this device" or something to that effect. Also, I'm pretty sure the note is meant to be saying that will be a result of doing this call. But someone might read this as saying that these actions will happen whether or not this function is called, because it is "unconditional".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about

@note If CONFIG_PM_DEVICE=n, PM_DEVICE_ACTION_TURN_ON followed by
 * PM_DEVICE_ACTION_RESUME will be invoked on the device by this function.

/**
* @brief Deinit a device driver
*
* @details Ensures device is either SUSPENDED or OFF, and will revert the actions of
Copy link
Member

Choose a reason for hiding this comment

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

reverting the actions would mean it would be able to put it into suspend, since pm_device_driver_init resumes it, no? So maybe a nit here is to say "revert any other actions", or even say "revert any other actions or side effects"

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Jun 18, 2025

Choose a reason for hiding this comment

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

How about

Ensures device is either SUSPENDED or OFF, and will deinitialize the PM_DEVICE context if this is the case.

the * @note below covers the "reverting actions of pm_device_driver_init()" as documented in the new section added by this PR

Comment on lines 648 to 649
* @note If CONFIG_PM_DEVICE=n, PM_DEVICE_ACTION_SUSPEND will be invoked
* unconditionally.
Copy link
Member

Choose a reason for hiding this comment

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

again strange for 2 reasons, first, it says just above that this function first ensures that it is already suspended, and 2nd, im still unclear why suspend action is ever being called if PM_DEVICE=n . My conceptualization of PM_DEVICE=n is that devices will ALWAYS be active in that case. And actually this has serious implications I think to say now that devices can be suspended or have the suspend action invoked on them when PM_DEVICE=n . Because now what, do drivers have to #if CONFIG_PM_DEVICE some code in order to make the distinction between what they want to do with the suspend action depending on the configuration ? This seems relevant especially considering that it seems that pretty much every single pm-managed device driver is now also technically a domain driver and therefore any of these drivers could take actions which depower other devices upon getting suspended.

Copy link
Member

Choose a reason for hiding this comment

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

I find it weird how PM is being implemented when PM_DEVICE=n, i.e. in the header, all pm_device_ calls are NOP when PM_DEVICE=n, this one and pm_device_driver_init however do "something" PM related.
and fully in agreement about the above note as well, if PM_DEVICE=n do suspend using PM constructs?

Is this something that needs to be done outside of PM subsystem or is it just fallback and the primary use is when CONFIG_PM_DEVICE is enabled but where documentation need to be improved?

This probably is following the pre-existing pm_device_driver_init and trying to do pm_device_driver_deinit the same way, but all of this probably applies to pm_device_driver_init as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

again strange for 2 reasons, first, it says just above that this function first ensures that it is already suspended, and 2nd, im still unclear why suspend action is ever being called if PM_DEVICE=n . My conceptualization of PM_DEVICE=n is that devices will ALWAYS be active in that case. And actually this has serious implications I think to say now that devices can be suspended or have the suspend action invoked on them when PM_DEVICE=n . Because now what, do drivers have to #if CONFIG_PM_DEVICE some code in order to make the distinction between what they want to do with the suspend action depending on the configuration ? This seems relevant especially considering that it seems that pretty much every single pm-managed device driver is now also technically a domain driver and therefore any of these drivers could take actions which depower other devices upon getting suspended.

Device drivers do not behave differently whether PM_DEVICE=y or not, no ifdefery is needed.

Does it make sense for a device to be ACTIVE, if no driver is "loaded" for it? I say no :) With device_deinit() a device driver can be "unloaded", better suspend it using the driver before we unload it no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find it weird how PM is being implemented when PM_DEVICE=n, i.e. in the header, all pm_device_ calls are NOP when PM_DEVICE=n, this one and pm_device_driver_init however do "something" PM related.
and fully in agreement about the above note as well, if PM_DEVICE=n do suspend using PM constructs?

Is this something that needs to be done outside of PM subsystem or is it just fallback and the primary use is when CONFIG_PM_DEVICE is enabled but where documentation need to be improved?

This probably is following the pre-existing pm_device_driver_init and trying to do pm_device_driver_deinit the same way, but all of this probably applies to pm_device_driver_init as well.

PM_DEVICE=n does not mean users don't want their devices ACTIVE. We still need to resume devices, and the way to resume a device does not change regardless of when the application or kernel wants the device resumed. PM_DEVICE is all about runtime, it simply exposes the resume/suspend actions the driver will need to do anyway. pm_device_driver_init is a special case, where we get to directly call, thus reuse the drivers resume on init without exposing a hook for runtime use :)

Copy link
Member

Choose a reason for hiding this comment

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

again strange for 2 reasons, first, it says just above that this function first ensures that it is already suspended, and 2nd, im still unclear why suspend action is ever being called if PM_DEVICE=n . My conceptualization of PM_DEVICE=n is that devices will ALWAYS be active in that case. And actually this has serious implications I think to say now that devices can be suspended or have the suspend action invoked on them when PM_DEVICE=n . Because now what, do drivers have to #if CONFIG_PM_DEVICE some code in order to make the distinction between what they want to do with the suspend action depending on the configuration ? This seems relevant especially considering that it seems that pretty much every single pm-managed device driver is now also technically a domain driver and therefore any of these drivers could take actions which depower other devices upon getting suspended.

Device drivers do not behave differently whether PM_DEVICE=y or not, no ifdefery is needed.

We talked before on a different ticket about this and I explicitly made a table to clarify this and you added it to the RFC, which said that the devices never go out of active state when PM_DEVICE=n. That seems to be changing now. And I don't see how you can say that calling SUSPEND action on devices when PM_DEVICE=n makes any sense. The device driver is clearly going to potentially have suspending actions implemented and like you say, they shouldn't have any different behavior expected or ifdeffry based on PM_DEVICE=n. So this PR is seeming like it is just going to cause device PM to happen even when PM_DEVICE=n. It's not making sense to me and I'm not hearing a convincing explanation of why, and also not seeing an explanation of how even a driver is not going to be responding to suspend action with steps to actually suspend if PM_DEVICE=n.

Does it make sense for a device to be ACTIVE, if no driver is "loaded" for it? I say no :) With device_deinit() a device driver can be "unloaded", better suspend it using the driver before we unload it no?

With deinit we're talking about initialization state, not power state. Why are you making an equivalence between a device having a suspended power state and a device being deinitialized? Why coupling those two definitions exactly together? Instead, shouldn't it be symmetrical, where deinit just does the opposite of init, and suspend just does the opposite of resume? Does that mean that device init routine always does the same thing as device resume routine as well? I don't think so. So this is breaking symmetry, is unintuitive, and doesn't seem right to me, it seems to be doing something unexpected from multiple perspectives of different components and layers of the system.

* @param dev Device instance.
* @param action_cb Device PM control callback function.
* @retval 0 if success.
* @retval -EBUSY Device is not SUSPENDED nor OFF
Copy link
Member

Choose a reason for hiding this comment

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

again, this doesn't seem to be true if PM_DEVICE=n as per the note above, otherwise why would it be calling the suspend action? Something feels off/uncomfortable here about the discrepancy in behavior, I'm not sure how to best describe my intuitive feeling though right now because I haven't had the bandwidth to get to deep into this PR

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Jun 17, 2025

Choose a reason for hiding this comment

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

Please read the Device Model without Power Management Support section added to the docs regarding drivers without pm, it explains the rationale :)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not talking about drivers without PM, I'm talking about drivers with PM when PM_DEVICE=n

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, wording is confusing, by Device Model without PM support, its meant as Device Model where internal PM operations are not exposed to the application, so I believe Device Model without Power Management Support is identical to drivers with PM when PM_DEVICE=n

The pm device docs introduction section has been updated to make this meaning explicit :)

Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

lets get a few things clarified before we move forward with this.

* @param dev Device instance.
* @param action_cb Device PM control callback function.
* @retval 0 On success.
* @retval -errno Error code from @a action_cb on failure.
*/
int pm_device_driver_init(const struct device *dev, pm_device_action_cb_t action_cb);

/**
* @brief Deinit a device driver
Copy link
Member

Choose a reason for hiding this comment

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

that brief probably needs to provide more context, right now it is not different from device_deinit which says: "De-initialize a device"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about

@brief Prepare PM device for device driver deinit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to Prepare PM device for device driver deinit

Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

This is going to need more clarification about what is the difference or not between a device being suspended and deinitialized, right now this PR seems to make them equivalent, which I don't think should be the case. But really the device model maintainers need to weigh in here about this, I don't know what was the use cases for why deinit was added in the first place, and I don't understand why is this PR seemingly equating deinit to a suspend power state in all cases.

Comment on lines 177 to 182
- ``SUSPENDED``: Device hardware is powered, but the device is not needed by the
system. The device should be put into its lowest internal power state,
commonly named "disabled" or "stopped". The device will be powered off if its
power domain is no longer needed by the system. Note that this state is
**NOT** a "low-power"/"partially operable" state, those are configured using
device driver specific APIs, and apply only while the device is ``ACTIVE``.
Copy link
Member

Choose a reason for hiding this comment

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

should you mention that the device's domain children would be powered off here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added mention

Comment on lines 193 to 198
- ``SUSPENDING``: Device hardware is powered, but the device has been enqueued to
be suspended, as it is no longer needed by the system. The device will be
dequeued in case the device becomes needed by the system, otherwise the device
will be suspended. No device driver API calls must occur in this state. Note
that this state is opaque to the device driver (no :c:enum:`pm_device_action`
is called as this state is entered)
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't sound applicable to all systems, should an example be explained? why would there be a queue? this is something I actually never understood and neither has anyone I talked to about it so maybe an explanation of why this state exists, somewhere, would be good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its for pm_device_runtime_put_async(), where the device suspend is delegated to a workqeue, possibly on a delay, that's where the "queue" part comes in. I can extend the docs to clarify this, and/or change it to say "slated to", or "scheduled to" instead of "enqueued" :)

Copy link
Member

Choose a reason for hiding this comment

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

I get that it's being enqueued and the idea is that it will be suspended later, what I don't get is why would this happen. I'm not saying this particular spot would be the best place to explain why, but it seems like somewhere that the reason for the existence of this state could be at least alluded to

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Jun 20, 2025

Choose a reason for hiding this comment

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

added the why and when :)

@@ -158,8 +158,47 @@ a particular device. It is important to emphasize that, although the
state is tracked by the subsystem, it is the responsibility of each device driver
to handle device actions(:c:enum:`pm_device_action`) which change device state.

Each :c:enum:`pm_device_action` have a direct an unambiguous relationship with
a :c:enum:`pm_device_state`.
Each :c:enum:`pm_device_state` state is defined as follows:
Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree with the contents of the documentation changes, and they are good changes, but why is it happening on this PR? what does it have to do with the pm_device_driver_deinit function being added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To understand why pm_device_driver_deinit() is added, its imperative the user understands how PM_DEVICE works to begin with. Without the docs, there is no way for a user to review whether or not the behavior of pm_device_driver_deinit() makes any sense :)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but, user does not call the pm_device_driver_deinit, is my understanding, so again I don't see how is it related to this PR. I think the doc changes are good but it seems to me like a sly attempt to update these docs into a tangentially related PR when really those new definitions should be properly reviewed. I agree with all of it but I don't see how this PR is the best place for it. The PR title is called "introduce pm_device_driver_init, not "introduce new definitions and clarify new paradigms for all the PM device states", people are going to skip over this PR and not review it who might actually only be interested in the latter

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Jun 19, 2025

Choose a reason for hiding this comment

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

There is no "sly attempt" at anything in this PR :) Consider creating a new PR with only the docs, it will quickly stop making sense as pm_device_driver_deinit() is missing... or you could try rewriting the doc part without the deinit part, and then be stuck forever as atleast I would ask why we are missing the deinit usecase :)

I will instead retitle the PR sine it has indeed changed focus a lot from when it was introduced, docs where added way later as a result of requests by jordan

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed title, and discussed PR at seconds arch meeting 20/6/2025

Comment on lines 645 to 646
* pm_device_driver_init() if this is the case. Must be called at the beginning of a
* driver's deinit function.
Copy link
Member

Choose a reason for hiding this comment

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

It says must be called by driver, okay, but is it illegal to be called anywhere else ? if so, should that be clarified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it can not be used outside of a device driver, given it must be passed the device drivers static pm_hook :)

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's fair enough, and the implication is fair enough as well

Comment on lines 648 to 649
* @note If CONFIG_PM_DEVICE=n, PM_DEVICE_ACTION_SUSPEND will be invoked
* unconditionally.
Copy link
Member

Choose a reason for hiding this comment

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

again strange for 2 reasons, first, it says just above that this function first ensures that it is already suspended, and 2nd, im still unclear why suspend action is ever being called if PM_DEVICE=n . My conceptualization of PM_DEVICE=n is that devices will ALWAYS be active in that case. And actually this has serious implications I think to say now that devices can be suspended or have the suspend action invoked on them when PM_DEVICE=n . Because now what, do drivers have to #if CONFIG_PM_DEVICE some code in order to make the distinction between what they want to do with the suspend action depending on the configuration ? This seems relevant especially considering that it seems that pretty much every single pm-managed device driver is now also technically a domain driver and therefore any of these drivers could take actions which depower other devices upon getting suspended.

Device drivers do not behave differently whether PM_DEVICE=y or not, no ifdefery is needed.

We talked before on a different ticket about this and I explicitly made a table to clarify this and you added it to the RFC, which said that the devices never go out of active state when PM_DEVICE=n. That seems to be changing now. And I don't see how you can say that calling SUSPEND action on devices when PM_DEVICE=n makes any sense. The device driver is clearly going to potentially have suspending actions implemented and like you say, they shouldn't have any different behavior expected or ifdeffry based on PM_DEVICE=n. So this PR is seeming like it is just going to cause device PM to happen even when PM_DEVICE=n. It's not making sense to me and I'm not hearing a convincing explanation of why, and also not seeing an explanation of how even a driver is not going to be responding to suspend action with steps to actually suspend if PM_DEVICE=n.

Does it make sense for a device to be ACTIVE, if no driver is "loaded" for it? I say no :) With device_deinit() a device driver can be "unloaded", better suspend it using the driver before we unload it no?

With deinit we're talking about initialization state, not power state. Why are you making an equivalence between a device having a suspended power state and a device being deinitialized? Why coupling those two definitions exactly together? Instead, shouldn't it be symmetrical, where deinit just does the opposite of init, and suspend just does the opposite of resume? Does that mean that device init routine always does the same thing as device resume routine as well? I don't think so. So this is breaking symmetry, is unintuitive, and doesn't seem right to me, it seems to be doing something unexpected from multiple perspectives of different components and layers of the system.

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jun 18, 2025

@decsny Yeah, I had not considered device_deinit() was a thing when I stated that "PM_DEVICE=n means always active", but now I'm blocked since I am working towards supporting device_deinit().

To be fair, PM_DEVICE=n does not mean devices are actually "always active", they are only active, after device_init() has been called. Using zephyr,deferred-init;, this means devices are not really "always active", they could actually never be active if device_init() is never called on a device, regardless of PM_DEVICE=n :)

So, what PM_DEVICE=n really means is: when a device is initialized, it will be put into the active state. device_deinit() thus would intuitively mean when a device is deinitialized, it will be put into the suspended state IMO.

There is a clear usecase for this logic, at runtime, a user may want to deinitialize a device driver to take manual control of the GPIOs the driver used before, or load another driver which uses the same underlying hardware, like nordics UART, SPI and I2C which share the same hardware, or simply a different driver for the same hardware, optimized for low power or something. To switch from UART to SPI, the UART has to be suspended, it can't just be left active when we deinitialize the driver, as this essentially results in undefined behavior.

Issue is, if we go with the PM_DEVICE=n means hardware can never be suspended, then device_deinit() depends on PM_DEVICE=y. This is a conflict since users may want to be able to switch drivers, but don't care about PM at all. using pm_device_driver_deinit() makes drivers do this necessary step, without having every driver copying the suspend logic into their deinit implementations.

device_deinit() was explicitly introduced for usecases like above, quoted from the PR introducing it:

(1) Multi-functional IP (e.g. USART/SPI/I2C)
(2) Re-allocation of device resources (e.g. pin re-assignment)
(3) Runtime interpreters like Micropython (where some devices may be dynamically loaded based on user input)
(4) Bootloaders: to leave some devices in their reset state
(5) See concrete case: "Allow multiple ICE40 on the same SPI bus" https://github.com/zephyrproject-rtos/zephyr/pull/77980, https://github.com/zephyrproject-rtos/zephyr/issues/80010

Usecase 4 for example, would it make sense for device_deinit() to just leave the device ACTIVE? I would expect device be suspendend and the hardware reset for the app to start fresh, as is defined by device_deinit:

* When a device is de-initialized, it will release any resources it has
* acquired (e.g. pins, memory, clocks, DMA channels, etc.) and its status will
* be left as in its reset state.

@nashif
Copy link
Member

nashif commented Jun 18, 2025

init/deinit are not globally seen as power management related operations, this might be the case for some driver and vendor implementations, but is not the general case. Making this behave like power management with CONFIG_PM_DEVICE=n introduces a second device power management mode that relies on init()/deinit().

Why can't all of this be done within PM_DEVICE=y where ACTIVE is achieved with init() and SUSPENDED is achieved with deinit() why do we need to do this outside of PM and introduce yet another class of device power management?

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jun 19, 2025

init/deinit are not globally seen as power management related operations, this might be the case for some driver and vendor implementations, but is not the general case. Making this behave like power management with CONFIG_PM_DEVICE=n introduces a second device power management mode that relies on init()/deinit().

Why can't all of this be done within PM_DEVICE=y where ACTIVE is achieved with init() and SUSPENDED is achieved with deinit() why do we need to do this outside of PM and introduce yet another class of device power management?

All PM_DEVICE=y does, is expose the pm hook outside of the driver. The pm hook implements everything the device driver needs to resume and suspend itself. Device drivers need to resume themselves at some point, to be usable at all. So, what is your solution to this problem, in cases of PM_DEVICE=y and PM_DEVICE=n ?

As it is now (see pm_device_driver_init()), we use the pm hook, unconditionally, internally in the driver, to resume the device right before leaving init if PM_DEVICE=n. Whatever you call this, power management related operations for example, this is simply reusing the same path the driver needs to take to be operational, at all. The same path runtime power management would use if PM_DEVICE=y.

@bjarki-andreasen bjarki-andreasen changed the title Introduce pm_device_driver_deinit() Specify and document PM_DEVICE behavior, including PM_DEVICE=n and device_deinit() Jun 19, 2025
@bjarki-andreasen bjarki-andreasen added this to the v4.2.0 milestone Jun 19, 2025
Add note which describes the behavior of pm_device_driver_init() if
CONFIG_PM_DEVICE=n.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Introduce pm_device_driver_deinit() which is required to fully
support device_deinit(). This function shall be called by drivers
when device_deinit() is called.

If PM_DEVICE is enabled, it will
simply check whether the device is either SUSPENDED or OFF, this
will prevent device_deinit() on a device which is currently in
use, and ensure the device is in the correct state if device_init()
is called later.

If PM_DEVICE is not enabled, it will invoke the SUSPEND action which
mirrors the pm_device_driver_init() behavior. Note that TURN_OFF
action is not called since the device is deinitialized after this
call.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Extend test suite to cover pm_device_driver_deinit() resulting
from call to device_deinit().

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
- Extend the pm device documentation defining and detailing
  expectations of devices in each enum pm_device_state state.

- Extend pm device driver code example with more concise comments
  and include pm_device_driver_init, pm_device_driver_deinit, and
  DEVICE_DT_INST_DEINIT_DEFINE() (device deinit feature)

- Describe the device driver PM states used if PM_DEVICE is not
  enabled.

- Note which states a device driver is initialized from and can
  be deinitialized in and why.

Signed-off-by: Bjarki Arge Andreasen <[email protected]>
@bjarki-andreasen bjarki-andreasen dismissed stale reviews from ceolin and JordanYates via 39a33f5 June 20, 2025 09:55
@bjarki-andreasen bjarki-andreasen force-pushed the pm-device-driver-deinit branch from ac7a10a to 39a33f5 Compare June 20, 2025 09:55
@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Jun 20, 2025

Update

  • Reworded pm device doc introduction to clearly specify that PM_DEVICE is used to expose the device driver's PM operations outside of the driver
  • Extended the SUSPENDED state doc entry to include the case where the device is itself a power domain
  • Expanded the SUSPENDING state doc entry to explain why and when the state is used.
  • Reworded header brief of pm_device_driver_deinit() from "Deinit a device driver" to "Prepare PM device for device driver deinit"
  • Reworded header details of pm_device_driver_deinit() to not state that it "revert the actions of pm_device_driver_init"
  • Reworded header notes for PM_DEVICE=n to replace "invoked unconditionally" with "invoked on the device by this function"

diff: https://github.com/zephyrproject-rtos/zephyr/compare/ac7a10a1ef9302a945584418d95e4c2bba8473b2..39a33f541a9dac4eb8d2163dc7a60e92ec374280

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

8 participants