Zephyr,power-state clarify min-residency-us and remove incoherent "consistency" check which was not consistent
#97108
Conversation
zephyrbot
requested review from
bjarki-andreasen,
ceolin,
JordanYates,
mbolivar,
nashif,
rruuaanng,
teburd and
tmleman
decsny
force-pushed
the
idle_states_binding_match
branch
from
5dc05e1 to
60dc9db
Compare
|
|
||
| description: Properties for power management state | ||
| description: | | ||
| Properties for power management idle states. Note that not all of the timing properties will |
There was a problem hiding this comment.
Nonblocking, but would you mind wrapping this to 80 columns? It will look better if we can avoid horizontal scrolling on the generated DT bindings index page for this, and that's more likely if the text is wrapped to a smaller column limit IMO.
| | /-- IDLE1 | ||
| e | /--- | ||
| n | /---- | ||
| e | /--- |
There was a problem hiding this comment.
It's not mine, I took it from the Linux binding
There was a problem hiding this comment.
Is BSD 2-Clause compatible with Apache 2.0?
There was a problem hiding this comment.
https://www.apache.org/legal/resolved.html#category-a
But maybe you are right that I forgot to update the copyright of this file, if ASCII art falls under copyright and software licensing
As for generally following the DT binding of Linux , if using the same DT properties and so on is "redistribution" or whatever and it's not compatible then we were already in trouble a long time ago because we have been trying to do that with pretty much all DT bindings. And a lot of the descriptions of bindings have been taken from Linux.
| Minimum residency duration in microseconds. It is the minimum time for a | ||
| given idle state to be worthwhile energywise. It includes the time to enter | ||
| in this state. | ||
| into this state and the time spent in the state before exiting. It represents |
There was a problem hiding this comment.
and the time spent in the state before exiting is a bit awkward IMO
If we reworded the whole thing:
- Minimum residency duration in microseconds. It is the minimum time for a
- given idle state to be worthwhile energywise. It includes the time to enter
- into this state and the time spent in the state before exiting. It represents
- the "break-even" time to justify the time and power spent
- on the work needed for entering the state based on how much actual energy is saved
- once in the state.
+ Minimum residency duration in microseconds.
+
+ How much time the residency in a given idle state must last for state entry
+ to be worthwhile energywise. It includes the time to enter into this state
+ and represents the "break-even" time to justify the time and power spent
+ on the work needed for entering the state based on how much actual energy
+ is saved once in the state.
| Maximum delay between the signaling of a wake-up event and the CPU | ||
| being able to execute normal code again. If unspecified, it is assumed | ||
| to be entry-latency-us + exit-latency-us, which would be the worst case. | ||
| But the actual wakeup latency may be lower as the intended meaning | ||
| of this value is actually | ||
| "exit-latency-us + entry-latency-us - 'software prep latency us'" | ||
| because the software prep happening before hardware gets committed | ||
| to the idle state could theoretically be aborted. |
There was a problem hiding this comment.
Maybe making the prep time appear on the timeline diagram would help, that's a bit harsh to understand.
|
The changes look pretty good do me :) Just the licensing thing concerns me. |
I agree, please fix this @decsny |
Clarify the min-residency-us property meaning. Signed-off-by: Declan Snyder <[email protected]>
This check is totally wrong, min-residency-us and exit-latency-us are not related to each other at all except that the end of the residency period occurs when the exit starts, so they are totally orthogonal values and should not be checking if one is less than the other or anything like this. Even in the code for the default policy manager, they are added together, so this check is incompatible / in disagreement with that code, so it should be removed. Signed-off-by: Declan Snyder <[email protected]>
decsny
force-pushed
the
idle_states_binding_match
branch
from
60dc9db to
3b68396
Compare
decsny
changed the title
Zephyr,power-state match to linux binding and remove incoherent "consistency" check which was not consistentZephyr,power-state clarify min-residency-us and remove incoherent "consistency" check which was not consistent
|
I updated the PR to remove the new properties and diagrams since the software license of the ASCII art was controversial, and we aren't using the properties yet anyways. The main motivation of this PR really was to remove the inconsistent check about the min-residency-us and clarify the property anyways. |
|
There is some incoherency about the meaning of the timing properties in the power-state binding. Improve the documentation and also fill out the binding to finish matching linux https://www.kernel.org/doc/Documentation/devicetree/bindings/cpu/idle-states.yaml as it was currently an incomplete match. More importantly, remove the check for "power state consistency" as it was in disagreement (inconsistent if you will) with the actual code of the default policy manager. The check considered min-residency-us as including exit-latency-us whereas the zephyr code and linux binding consider them as totally separate values.