Implement soc-manager-service#699
Implement soc-manager-service#699kurtjd wants to merge 1 commit intoOpenDevicePartnership:v0.2.0from
Conversation
620a40a to
600100c
Compare
tullom
left a comment
There was a problem hiding this comment.
Would it be possible to add some simple unit tests for the SocManager and PowerGuard structs?
|
@kurtjd Can we demonstrate this service on one of soc-ec's platform? |
Yes, this is a good idea, will add some. |
Yes I can do this. |
600100c to
adb2c81
Compare
There was a problem hiding this comment.
Pull request overview
Introduces a new soc-manager-service crate to centralize SoC power-state transitions (ACPI S0/S0ix/S3/S4/S5) and provide a reusable rollback helper for regulator sequencing.
Changes:
- Added
SocManagerwith ACPI power-state transition validation and listener support viaembassy_sync::watch. - Added
PowerGuardhelper to execute regulator ops and rollback on failures. - Wired the new crate into the workspace and lockfile.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
soc-manager-service/src/lib.rs |
Implements SocManager, power-state enum, and listener API backed by Watch. |
soc-manager-service/src/power_guard.rs |
Adds a heapless stack-based rollback mechanism for regulator enable/disable ops. |
soc-manager-service/Cargo.toml |
Defines the new crate and dependencies (including new git deps). |
Cargo.toml |
Adds soc-manager-service to workspace members. |
Cargo.lock |
Records the new crate and its new git dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adb2c81 to
87e7147
Compare
Done. The test coverage isn't exhaustive at the moment but covers the basic functionality. Will add more once some of the interface and functionality is finalized. |
Mentioned offline I had a branch starting to integrate this into one of our test platforms, but I don't have a working version of that platform at the moment so can't test it. Thus tomorrow I will integrate it into the platform I do have working and will share a link to the branch after I've verified it (will share the link internally since it would be on a private repo). |
87e7147 to
93622b6
Compare
Shared link to working integration into one of our hardware platforms. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
93622b6 to
e0f2746
Compare
e0f2746 to
02d95ea
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There were a few suggestions for what the
soc-manager-serviceshould handle, but for several of them I couldn't come up with abstractions I felt satisfied with after thinking about them and experimenting.For now, this PR keeps it simple and focuses on proper power state transitioning and power regulator rollback in event of failure. In its current state it is more of a library than a service as there are no associated tasks, but that will likely change in the future.
Power state transition
I followed the docs from here for my understanding of valid power state transitions. Offline it was discussed that the
embedded-power-sequencetraits be renamed to match ACPI power state names, but in this context, I think it makes sense to keep them as-is. The S0, S3, etc terminology reflects a state, but we can view theembedded-power-sequencetraits as state transitions which are aptly named IMO. Also just added a simple standard interface for allowing multiple listeners to power state changes (since currently platforms are just rolling their own slightly different but ultimately the same implementation for this).Rollback
Made a simple data structure backed by a heapless vec which acts as a stack that regulator operations can be pushed to. In the event of failure, or explicitly called rollback, this just allows us to reverse every prior regulator operation. The idea is this will be used in the
embedded-power-sequencetrait impls for a SoC (the docs contain an example of how I pictured it being used).Other suggestions folks offered and things I need to think about more:
so will need to think about this a bit more.
Resolves #685