Skip to content
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 power management states definition #29505

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

ceolin
Copy link
Member

@ceolin ceolin commented Oct 23, 2020

Add definition power management states, these states were inspired by
ACPI specification. This commit is part of the following pr
#27315 from Wentong.

Signed-off-by: Flavio Ceolin flavio.ceolin@intel.com

@github-actions github-actions bot added the area: API Changes to public APIs label Oct 23, 2020
@ceolin
Copy link
Member Author

ceolin commented Oct 23, 2020

This is part of #27315
There is another pending commit replacing the usage of old states with the ones introduced here, but I'm holding it to avoid a huge clash with #29410

@carlescufi carlescufi requested a review from pabigot October 24, 2020 11:34
Copy link
Collaborator

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

In #24228 we were discussing the names; are we going to continue to use Zephyr-specific terminology? I'd hoped we could come closer to using ACPI terminology for system and device power.

@ceolin
Copy link
Member Author

ceolin commented Oct 26, 2020

In #24228 we were discussing the names; are we going to continue to use Zephyr-specific terminology? I'd hoped we could come closer to using ACPI terminology for system and device power.

I thought that this was the outcome of that discussion. I saw your comment about having 1:1 with ACPI, but didn't see any agreement around it. Do we have this granularity need ? I see a trade-off here between clarity and granularity, also how many embedded platforms follow ACPI name convention ? TI CC1352 does not, at least.

@ceolin
Copy link
Member Author

ceolin commented Oct 26, 2020

In #24228 we were discussing the names; are we going to continue to use Zephyr-specific terminology? I'd hoped we could come closer to using ACPI terminology for system and device power.

I thought that this was the outcome of that discussion. I saw your comment about having 1:1 with ACPI, but didn't see any agreement around it. Do we have this granularity need ? I see a trade-off here between clarity and granularity, also how many embedded platforms follow ACPI name convention ? TI CC1352 does not, at least.

btw, I should have mentioned, but these are only the system states, I'll send later devices power states.

@pabigot
Copy link
Collaborator

pabigot commented Oct 26, 2020

I thought that this was the outcome of that discussion. I saw your comment about having 1:1 with ACPI, but didn't see any agreement around it.

I didn't see anybody object. The only documentation I know of for the architecture is @nashif's powerpoint presentation which uses the ACPI S and D states.

I see a trade-off here between clarity and granularity, also how many embedded platforms follow ACPI name convention ?

I do think it makes sense to define Zephyr's behavior in terms of a well-defined and understood architecture such as ACPI. Even if the names aren't used for some reason, the description of the Zephyr-only terms should at least have some relation to existing standards,

@ceolin
Copy link
Member Author

ceolin commented Oct 26, 2020

I thought that this was the outcome of that discussion. I saw your comment about having 1:1 with ACPI, but didn't see any agreement around it.

I didn't see anybody object. The only documentation I know of for the architecture is @nashif's powerpoint presentation which uses the ACPI S and D states.

I see a trade-off here between clarity and granularity, also how many embedded platforms follow ACPI name convention ?

I do think it makes sense to define Zephyr's behavior in terms of a well-defined and understood architecture such as ACPI. Even if the names aren't used for some reason, the description of the Zephyr-only terms should at least have some relation to existing standards,

Lets make it clear, you are proposing we to use ACPI states ? G0-3, S1-6 ... ? I don't have a strong argument against it, just that this seems more desktop/laptop orientated and even linux does not fully follow it, my main concern is that this granularity is not necessary for 99% of our targets, but I see the value of following a standard and having the power to cover that 1%
At this point, I want to have a consensus around it and move forward. Are you proposing 1:1 with ACPI ?

@pabigot
Copy link
Collaborator

pabigot commented Oct 26, 2020

Lets make it clear, you are proposing we to use ACPI states ? G0-3, S1-6 ... ?

I'm proposing we document what we're going to do at the architecture level, then ensure the code matches it. AFAIK that documentation is the power point presentation, which says that Zephyr "platform and device specific PM states needs to be mapped to [ACPI] S and D states respectively" (at least in the version I have). Which suggests we should be using those states as the platform/device agnostic states.

We really need an active architectural lead for this effort. We're not going to get very far if every time we pick this up we change where we're going.

@nashif
Copy link
Member

nashif commented Oct 27, 2020

In #24228 we were discussing the names; are we going to continue to use Zephyr-specific terminology? I'd hoped we could come closer to using ACPI terminology for system and device power.

what terminology are you proposing here? Can you please be specific about the terminology you are asking for? I thought that was something we closed on already and was discussed and addressed in #27315

@pabigot
Copy link
Collaborator

pabigot commented Oct 27, 2020

I really can't keep track of everywhere things are being discussed, and what state they're being left in.

From what I can tell #24228 didn't reach a conclusion. I don't see that #27315 did either; as I recall it simply captured the original proposal using Zephyr names, and started outlining solutions using them. Some discussion in #27315 was targeted at making the state descriptions consistent with ACPI. The PowerPoint presentation also uses ACPI states, and I thought it was the most recent documented and maintained architectural plan for Zephyr. So yes, I was surprised to find that we're back to using Zephyr-specific state names with no reference to existing well-understood architectures.

Beyond the names and definitions, why are these bit fields? That makes it difficult to have a type that can only represent a single state.

This PR is updating Zephyr to record a snapshot of a fragment of a solution. It can't be more than that because we don't have a plan that goes any deeper than some slides. I don't think it's helping us make progress.

@nashif
Copy link
Member

nashif commented Oct 27, 2020

what terminology are you proposing here? Can you please be specific about the terminology you are asking for?

You are still not answering my question:

what terminology are you proposing here? Can you please be specific about the terminology you are asking for?

The states mentioned in the slides (S0... and D0...) actually correspond to what we have here (also see your comment: #24228 (comment)), so please if you think some state should be named differently, be specific what you want to be changed.

@pabigot
Copy link
Collaborator

pabigot commented Oct 27, 2020

Thank you. Then what I want is text in the description of the states that records the correlation between the Zephyr names and the ACPI states.

@nashif
Copy link
Member

nashif commented Oct 27, 2020

Thank you. Then what I want is text in the description of the states that records the correlation between the Zephyr names and the ACPI states.

This can be added as a note per state I guess.

Add definition power management states, these states were inspired by
ACPI specification. This commit is part of the following pr
zephyrproject-rtos#27315 from Wentong.

Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
@pabigot pabigot self-requested a review November 4, 2020 18:08
Copy link
Collaborator

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

Seems good. Thanks.

@carlescufi carlescufi merged commit c609528 into zephyrproject-rtos:master Nov 11, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants