pm: Make device pm optional per state #60939
Conversation
ceolin
force-pushed
the
pm/device
branch
2 times, most recently
from
8c631d8
to
7959516
Compare
|
I think this is a very nice non-destructive compromise of #60463. The ability to individually disable device PM for peripherals where it might conflict with system PM is a great idea! :) |
Thanks for looking it. |
ceolin
requested review from
nashif,
carlescufi,
galak,
MaureenHelm,
sylvioalves,
dcpleung,
kgugala,
pgielda,
albertofloyd,
mmahadevan108,
dleach02,
ssekar15,
MulinChao and
ChiHuaL
as code owners
There was a problem hiding this comment.
I think this PR doesn't solve the problems in device PM itself. At least with CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE=y you had the guarantee that CONFIG_PM_DEVICE did nothing, now, as soon as you have a single state with zephyr,pm-device-enabled and a device with a blocking PM callback you're screwed again.
The main problem with CONFIG_PM_DEVICE is not that it doesn't allow blocking, but that it treats all devices equally. If we really want to keep this mode, I think it needs its own set of PM callbacks (where you can ask clients to not block), separate from PM device runtime (as on Linux, actually). But on MCUs this PM model doesn't make sense, you typically don't want to deal with devices on every LP state transition. Some MCUs need to perform actions when exiting certain LP states on a per-device basis (e.g. the STOP modes in STM32), but this is something that can be solved in a better way, for example, subscribing to such events as needed. And to me, such a mechanism could likely be re-used to finally get rid of CONFIG_PM_DEVICE. The problem here is that we've never got technical details on how it is being used to discuss a proper way forward, and we've been stuck forever with this.
That is not what this pr aims. The goal here is bring more flexibility for those who wants to use it and not breaking the current behavior. I can restore
That is one reason to allow them to choose only low power states that it should be triggered.
notifiers don't solve it now since they can't block as well (they are called from idle thread)
We can work through this, but there are real use cases. Note that this is bring more flexibility making even easier to not use it if that is wanted. We may work through a better way but this change is just giving the capability to tell whether or not a state triggers device pm. Is removing |
ceolin
changed the title
|
@gmarull ping |
subsys/pm/Kconfig
Outdated
| On system suspend / resume do not trigger the Device PM hooks but | ||
| only rely on Runtime PM to manage the devices power states. | ||
| This option enables the device system managed power management. The | ||
| power management subsystem will suspend devices when it gets idle |
There was a problem hiding this comment.
gets idle -> goes to idle or transitions to idle
|
@JordanYates thanks for looking it ! Have update with suggestions. |
| config PM_DEVICE_RUNTIME_EXCLUSIVE | ||
| depends on PM_DEVICE_RUNTIME | ||
| bool "Use only on Runtime Power Management on system suspend / resume" | ||
| config PM_DEVICE_SYSTEM_MANAGED |
There was a problem hiding this comment.
Isn't this changing the default behaviour when PM_DEVICE=y and PM_DEVICE_RUNTIME=y?
Previously hooks wouldn't be enabled, now they are.
There was a problem hiding this comment.
PM_DEVICE_RUNTIME_EXCLUSIVE=y can be replace with PM_DEVICE_SYSTEM_MANAGED=n.
I believe this is more clear.
PM_DEVICE is the base and need by RUNTIME and SYSTEM_MANAGED. PM_DEVICE is the base to enable pm in a device (aka pm callback)
PM_DEVICE_RUNTIME enables device runtime
PM_DEVICE_SYSTEM_MANAGED tell the pm subsystem to suspend / resume devices when the soc goes idle.
If one does not want device system managed pm, just disable it. We can make default n if PM_DEVICE_RIUNTIME
There was a problem hiding this comment.
I understand it can be replaced, but PM_DEVICE_RUNTIME_EXCLUSIVE=y was the previous default, and PM_DEVICE_SYSTEM_MANAGED=y is the new default, which is the opposite behaviour.
There was a problem hiding this comment.
Right ... but now we have to explicitly set, in DT, which states triggers system managed device pm.
If we want to keep exactly the same behavior, I can change this pr to;
- By default, states trigger device pm when
PM_DEVICE_SYSTEM_MANAGEDis set (and users can disable specific sates setting it in DT) PM_DEVICE_SYSTEM_MANAGEDis disabled (by default) when PM_DEVICE_RUNTIME is enabled.
With these changes we keep the current behavior. I was already thinking about it (especially due dt changes).
@JordanYates what you think ?
|
@ceolin I see you have added this to the v3.6.0 milestone. Do you still intend for this to go in before v3.6.0? It looks like an enhancement to me. |
@henrikbrixandersen it won't. It is to late for it now. I'll wait the merge window open to move it. |
That is option has shown confusing on it is attempt to prevent system pm doing device power management. Lets address this problem properly. Signed-off-by: Flavio Ceolin <[email protected]>
Enable device power management individually per power state. This allows targets tuning which state should trigger device power management or completely disable it simply not enabling it in any power state. Signed-off-by: Flavio Ceolin <[email protected]>
Remove device pm path when there is no is no power state in DT with device pm enabled. This basically does the same thing that was done by PM_DEVICE_RUNTIME_EXCLUSIVE. Signed-off-by: Flavio Ceolin <[email protected]>
PM_DEVICE is not attached to system managed device power management. It is a very common use case targets with device runtime power management that don't want system device power management enabled. We introduce a new symbol (PM_DEVICE_SYSTEM_MANAGED) to explicit control whether or not system device power management should be globally enabled. Signed-off-by: Flavio Ceolin <[email protected]>
Define the power state needed in the test in DT. Signed-off-by: Flavio Ceolin <[email protected]>
Define the power state needed in the test in DT. Signed-off-by: Flavio Ceolin <[email protected]>
Add new information about new property to enable/disable device power management per power state. Signed-off-by: Flavio Ceolin <[email protected]>
Add a new test that disables PM_DEVICE_SYSTEM_MANAGED, since this option is enabled by default. Signed-off-by: Flavio Ceolin <[email protected]>
Bump SoF to a version that does not have reference to PM_DEVICE_RUNTIME_EXCLUSIVE Signed-off-by: Flavio Ceolin <[email protected]>
|
@JordanYates I have kept the current behavior in #70623. Please take a look. @erwango ^ |
|
Closing it in favor of #70623 |
This is a proposal to make device power management (triggered when SoC is idle) completely optional.
CONFIG_PM_DEVICE_RUNTIME_EXCLUSIVENotes:
This draft contains a proposal but it is not complete,
some tests are failing because of the latest commit that completely disable device pm when it is not enabled in DT and we have some tests that don't use DT to check pm API.This is commit is basically an additional optimization to have the same we had withCONFIG_PM_DEVICE_RUNTIME_EXCLUSIVE.In case of consensus, I will restore the current behavior addingzephyr,pm-device-enabledin all power states (across the tree) that are currently triggering device power management, then we can in a case by case disable it if need. But the idea is not breaking the current behavior.Additional context:
st,reinit-power-statesDT property #60369