-
Couldn't load subscription status.
- Fork 8.2k
PM: Add constraint API #31013
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
PM: Add constraint API #31013
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we go too far down the path of size-checked increments I'd like to see that discussed.
Also if we're using a mutex we need to document that people can't set constraints from interrupts or before POST_KERNEL. Which, I suspect, is probably a restriction we should not impose.
|
@pabigot thanks for reviewing it. Regarding mutex usage, I can remove them if I assume that the input is always right, but I don't like this idea, right now this is just checking if the given state is a valid one, but we may want to check if the state is defined by the target. I can change it to spin look and allows using from an interrupt context. Why do you think it is bad constraint allowing use this API only after |
When drivers start up pre-kernel they may want to set constraints to prevent themselves from being shut down. |
|
@pabigot I just kept the current implementation but changed API names to align with what you've discussed. Would you mind take a look again ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continue to believe the documentation on this API is horrifically poor (what does "set a constraint" actually mean in terms of its effect on the system behavior?), but as a refactoring PR it's ok.
|
@nashif would you mind take a look ? Also, CI failed for not clear reason. |
Small fixes in state control documentation. Signed-off-by: Flavio Ceolin <[email protected]>
Replace pm_ctrl_* with pm_constraint. Signed-off-by: Flavio Ceolin <[email protected]>
Simplify pm subsystem removing PM_STATE_LOCK option. Constraints API is small and is a key component of power subsystem. Signed-off-by: Flavio Ceolin <[email protected]>
Align documentation with constraint API changes. Signed-off-by: Flavio Ceolin <[email protected]>
agree, will address it in another pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine. I agree that naming cleanup without a doc rewrite might be getting ahead of ourselves. But no reason not to merge.
| #ifdef CONFIG_PM_STATE_LOCK | ||
| /** | ||
| * @brief Disable particular power state | ||
| * @brief Set a constraint for a power state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One point is not clear to me on this API. Not sure if this is the right pace to discuss. Please discard this comment if this isn't.
When setting a constraint on a particular state, should we assume the same constraint should be used on "deeper sleep states", or is the caller expected to call all related states where the constrain apply?
What about substates ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to wait to discuss that, since this PR is just renaming stuff, but:
- I'm not sure if we can assume a strict linear ordering such that if you block a transition to
STANDBYyou implicitly block transition toSUSPEND_TO_RAMand other "lower" states. Need to see what the prior art does. - yes, constraints are going to have to take substates into account, either explicitly or through some other back door.
This will replace the pm_ctrl_* API.