PM_DEVICE_POWER_DOMAIN refactor #89372
Conversation
github-actions bot requested review from JordanYates, ceolin, decsny, galak, nashif, rruuaanng, teburd and tmleman 
bjarki-andreasen force-pushed the pm-power-domain-refactor branch 2 times, most recently from 646281a to 495362c Compare github-actions bot added area: GPIO platform: Renesas SmartBond github-actions bot requested review from andrzej-kaczmarek, blauret, henrikbrixandersen, ioannis-karachalios, mnkp and ydamigos bjarki-andreasen force-pushed the pm-power-domain-refactor branch from 0cb8a8e to 91227b1 Compare 
There was a problem hiding this comment.
The PR looks really good to me. The change request is specifically over some DT related nits. And I have one question about one of the other power domain drivers which is not being touched on this PR at the moment (power_domain_soc_state_change). Because that one in particular seems like the biggest refactor target of any of the existing in tree ones, given the goal of this PR is to remove code about calling turn on/off actions on domain children. That's basically all that driver's code seems to be for from what I can tell.
| /* Disable unused nodes on power_mode3_domain */ | ||
| &dma0 { | ||
| status = "disabled"; | ||
| }; | ||
| | ||
| &flexcomm1 { | ||
| status = "disabled"; | ||
| }; |
There was a problem hiding this comment.
this change is fine, and it's maybe a bigger ask from this PR but since this RW is one of the few in tree users of power domain, would it make sense on this PR to fix that soc since the power domain is described incorrectly right now?
If not I guess nxp maintainers will fix it later.. but to be clear what I'm seeing is maybe an issue on that platform which I was not aware of before because I was not involved here, is that PM3 on that chip is not a domain but a power state, and should not be described as a domain? I'm not sure about this though because I am actually basically just now getting called in to get a handle on the power architecture of Zephyr due to problems we are having with this specific chip in Zephyr when users enable CONFIG_PM.... apparently the current state of things is not working out
It seems like instead there should be a domain representing the digital region of the chip that I think should use the new default driver you are adding in this PR? And that domain should mark the zephyr suspend (called pm2 on chip) and standby (called pm3 on the chip) states as the states that disable the power domain in DT? And all of these peripherals in this overlay being disabled here are part of that domain (except the analog dac and adc, but for now we can pretend they are at minimum because the digital domain is the most affected by power state transitions on this chip)
I actually think this current driver we have written here for our current conception of the "power domain" is maybe completely backwards and maybe not even valid due to it is representing a specific mode transition and not a physical (or conceptual) domain/power region? https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/power_domain/power_domain_soc_state_change.c . I noticed also that you refactored some of the other power domain drivers regarding this children handling on/off due to the redudancy/reimplementing. I think maybe this particular driver I linked doesn't even need to exist now.
I understand if you don't want to touch that power domain driver I linked on this PR, since I think it's kind of a vendor specific mess, even though it looks it was given a generic zephyr compatible, but at least if you have any guidance or clarity about this at least I will greatly appreciate it.
scripts/dts/gen_defines.py Outdated
| out_dt_define(f"{node.z_path_id}_POWER_DOMAIN_CHILDREN_LEN_OKAY", len(pd_child_z_path_ids)) | ||
| out_dt_define(f"{node.z_path_id}_POWER_DOMAIN_CHILDREN_FOREACH_OKAY(fn)", | ||
| " ".join( | ||
| f"fn(DT_{z_path_id})" | ||
| for z_path_id in pd_child_z_path_ids)) | ||
| out_dt_define(f"{node.z_path_id}_POWER_DOMAIN_CHILDREN_FOREACH_OKAY_SEP(fn, sep)", | ||
| " DT_DEBRACKET_INTERNAL sep ".join( | ||
| f"fn(DT_{z_path_id})" | ||
| for z_path_id in pd_child_z_path_ids)) | ||
| out_dt_define(f"{node.z_path_id}_POWER_DOMAIN_CHILDREN_FOREACH_OKAY_VARGS(fn, ...)", | ||
| " ".join( | ||
| f"fn(DT_{z_path_id}, __VA_ARGS__)" | ||
| for z_path_id in pd_child_z_path_ids)) | ||
| out_dt_define(f"{node.z_path_id}_POWER_DOMAIN_CHILDREN_FOREACH_OKAY_SEP_VARGS(fn, sep, ...)", | ||
| " DT_DEBRACKET_INTERNAL sep ".join( | ||
| f"fn(DT_{z_path_id}, __VA_ARGS__)" | ||
| for z_path_id in pd_child_z_path_ids)) | ||
| |
There was a problem hiding this comment.
I think this addition should warrant update the macros.bnf description for this change similar to like pinctrl has done for example
There was a problem hiding this comment.
added entry to macros.bnf
There was a problem hiding this comment.
I think this is what vnd,reserved-compat is specifically for already, so this new one shouldn't be needed
There was a problem hiding this comment.
nice, changed to use existing binding
There was a problem hiding this comment.
vnd,reserved-node node is specifically used to test devices with status reserved :) Added back the device-base one
There was a problem hiding this comment.
zephyr/tests/lib/devicetree/api/src/main.c
Line 377 in e96bf44
include/zephyr/devicetree.h Outdated
| * @brief Variant of DT_POWER_DOMAIN_CHILDREN_FOREACH_OKAY with VARGS | ||
| * | ||
| * @see DT_POWER_DOMAIN_CHILDREN_FOREACH_OKAY() | ||
| * | ||
| * @details The macro @p fn must take one parameter, which will be a node | ||
| * identifier, followed by variadic args. The macro is expanded once for | ||
| * every power domain child with status "okay". | ||
| * | ||
| * @param node_id Power domain's node identifier | ||
| * @param fn Macro to invoke | ||
| * @param ... Variadic args passed to @p fn | ||
| */ | ||
| #define DT_POWER_DOMAIN_CHILDREN_FOREACH_OKAY_VARGS(node_id, fn, ...) \ | ||
| DT_CAT(node_id, _POWER_DOMAIN_CHILDREN_FOREACH_OKAY_VARGS)(fn, __VA_ARGS__) |
There was a problem hiding this comment.
other subsystems put the DT macros in include/zephyr/devicetree/ subfolder with their own header, can't this go that route as well?
There was a problem hiding this comment.
moved to its own header
| And one more question , is are you targeting this PR to merge before 4.2 freeze point? |
| @decsny I'm all for refactoring the soc_state_change power domain and the board using it as part of this PR, it is indeed "backwards" compared to the power domain behavior I envision with this PR. I just did not want to "rock the boat" too much at once :) |
I'm working with an "as soon as possible" timeline to refactor pm device in general, so probably yes :D |
The power_domain_gpio driver was reimplementing the behavior of pm_device_children_action_run(). Replace reimplementation with pm_device_children_action_run(). Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
The gpio_smartbond driver incorrectly uses the static PM_DEVICE_DEFINE() and PM_DEVICE_GET() macros when creating a driver instance from a devicetree instance number. Update to use PM_DEVICE_DT_INST_DEFINE() and PM_DEVICE_DT_INST_GET() macros. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Validate that device dep children indeed belong to domain when iterating dependent devices from pm_device_children_action_run(). Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Introduce helpers pm_device_domain_children_turn_on() and pm_device_domain_children_turn_off() to be called from PM_DEVICE_RUNTIME when after resuming and before suspending a power domain. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
The APIs pm_policy_device_power_lock_get and pm_policy_device_power_lock_put are not mocked properly as they should be excluded based on CONFIG_PM_POLICY_DEVICE_CONSTRAINTS, not CONFIG_PM. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
bjarki-andreasen force-pushed the pm-power-domain-refactor branch from 0fa6876 to f3b4976 Compare | Please review the following individual PRs this PR depends on / cherry picks |
Move device power domain and pm policy device logic to PM_DEVICE_RUNTIME. Specifically: - When resuming a device, if successful, get pm policy power lock for device, and notify any device power domain children of the new state of the device by calling TURN_ON. - When suspending a device, first notify power domain children that device will be suspended by calling TURN_OFF, then put pm policy device lock, then try to suspend the device. If failure, notify children that device will remain resumed, and get pm policy device lock again. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
PM_DEVICE_RUNTIME now implements power domain child actions. Remove this duplicate logic from the device power domain driver. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
PM_DEVICE_RUNTIME now implements power domain child actions. Remove this duplicate logic from the device power domain driver. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Add default power domain driver which is useful for locking system sleep states based on a group of devices. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Use the default power domain driver instead of creating a bespoke one within the test suite. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Add support for pm device runtime to sample. Simply resume the temperature device before using it. If PM_DEVICE_RUNTIME is not enabled, device is assumed resumed on boot. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Implement pm device runtime for p3t1755 sensor. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
bjarki-andreasen force-pushed the pm-power-domain-refactor branch from f3b4976 to f8e07b0 Compare Update board to use default power domain for the "peripheral" domain, which when resumed prevents SoC from entering suspend and standby states as these power down peripherals. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Enable PM_DEVICE_RUNTIME and explicitlly enable dependencies required for the power domains and device drivers power management to function. For now, this requires manually enabling PM_DEVICE_RUNTIME for every device used in the sample by adding the devicetree prop zephyr,pm-device-runtime-auto to make the drivers actually adhere to PM_DEVICE_RUNTIME... Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
pm_device_runtime has inconsistent behavior regarding getting and putting a device's domain. This commit aligns it to get the domain only once, once device is resumed, and put only once, once device is suspended. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
bjarki-andreasen force-pushed the pm-power-domain-refactor branch from f8e07b0 to 4019fa7 Compare PM_DEVICE_SYSTEM_MANAGED nor PM_DEVICE_RUNTIME is not supported by test suite, so disable any config which results in either being enabled. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
PM_DEVICE_SYSTEM_MANAGED nor PM_DEVICE_RUNTIME is not supported by test suite, so disable any config which results in either being enabled. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
|
| This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
| @bjarki-andreasen what is the status of this ? |
| currently I'm working towards standardizing on PM DEVICE RUNTIME with PRs like #93720 since power domains depend on PM DEVICE RUNTIME, so this PR is a bit less prioritized |
| @bjarki-andreasen can you help me understand what's the status of this now? It seems like this PR is required to have a coherent PM device runtime management. We are kind of blocked waiting for this as we are wanting to enable multiple SOC right now in the paradigm you're trying to lead everybody towards. It seems like some of the commits on this PR are actually merged from other PRs? Can we do a rebase and see what's actually left in this PR? |
4 participants
This PR refactors PM_DEVICE_POWER_DOMAIN to be simpler and more scalable, moving power domain and device constraint logic from device drivers to PM_DEVICE_RUNTIME.
Depends on and cherry picks the following: