Skip to content

Conversation

@ceolin
Copy link
Member

@ceolin ceolin commented Dec 27, 2020

This will replace the pm_ctrl_* API.

@github-actions github-actions bot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Dec 27, 2020
Copy link
Contributor

@pabigot pabigot left a 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.

@ceolin
Copy link
Member Author

ceolin commented Jan 4, 2021

@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 POST_KERNEL ?

@pabigot
Copy link
Contributor

pabigot commented Jan 4, 2021

Why do you think it is bad constraint allowing use this API only after POST_KERNEL ?

When drivers start up pre-kernel they may want to set constraints to prevent themselves from being shut down.

@ceolin
Copy link
Member Author

ceolin commented Jan 26, 2021

@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 ?

Copy link
Contributor

@pabigot pabigot left a 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.

@ceolin
Copy link
Member Author

ceolin commented Jan 27, 2021

@nashif would you mind take a look ? Also, CI failed for not clear reason.

Flavio Ceolin added 4 commits February 4, 2021 10:04
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]>
@ceolin
Copy link
Member Author

ceolin commented Feb 4, 2021

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.

agree, will address it in another pr.

Copy link
Contributor

@andyross andyross left a 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.

@nashif nashif added this to the v2.6.0 milestone Feb 5, 2021
#ifdef CONFIG_PM_STATE_LOCK
/**
* @brief Disable particular power state
* @brief Set a constraint for a power state
Copy link
Member

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 ?

Copy link
Contributor

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 STANDBY you implicitly block transition to SUSPEND_TO_RAM and 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.

@nashif nashif merged commit 7f56bd4 into zephyrproject-rtos:master Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Boards area: Documentation area: I2C area: Power Management area: Samples Samples area: SPI SPI bus area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants