Skip to content

pm: Include disabled states in search inside pm_state_get#89041

Merged
kartben merged 1 commit intozephyrproject-rtos:mainfrom
nordic-krch:pm_force_fix2
May 5, 2025
Merged

pm: Include disabled states in search inside pm_state_get#89041
kartben merged 1 commit intozephyrproject-rtos:mainfrom
nordic-krch:pm_force_fix2

Conversation

@nordic-krch
Copy link
Copy Markdown
Contributor

pm_state_get() is used to get a PM state that can be forced. Disabled states (not included in automatic state selection) can also be forced thus they should be included in search inside pm_state_get().

@ceolin
Copy link
Copy Markdown
Member

ceolin commented Apr 25, 2025

The change behavior looks good to me (thanks !). There is though some tests breaking because the implementation is assuming

cpus {
          power-states {

I am ok with it, since the other way would need to check per cpu. Can you please update failing tests ?

Copy link
Copy Markdown
Contributor

@tmleman tmleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nordic-krch, thank you for the quick response and help in solving the problem. I have one more request for you, when you update this PR, please do a rebase. One of your patches has already been merged, so after the rebase, I will be able to prepare a PR for SOF pointing to this branch, which will trigger the execution of SOF's CI on these changes.

Comment thread subsys/pm/state.c
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed solution is okay and solves my problem. However, I wasn't able to build them immediately with SOF:

zephyr/subsys/pm/state.c:64:2: warning: implicit declaration of function 'DT_N_S_cpus_S_power_states_FOREACH_CHILD' is invalid in C99 [-Wimplicit-function-declaration]
        DT_FOREACH_CHILD(DT_PATH(cpus, power_states), DEFINE_DISABLED_PM_STATE)
        ^
zephyr/include/zephyr/devicetree.h:3111:2: note: expanded from macro 'DT_FOREACH_CHILD'
        DT_CAT(node_id, _FOREACH_CHILD)(fn)
        ^
zephyr/include/zephyr/devicetree.h:5269:24: note: expanded from macro 'DT_CAT'
#define DT_CAT(a1, a2) a1 ## a2
                       ^
<scratch space>:102:1: note: expanded from here
DT_N_S_cpus_S_power_states_FOREACH_CHILD
^
zephyr/subsys/pm/state.c:64:48: error: use of undeclared identifier 'DEFINE_DISABLED_PM_STATE'
        DT_FOREACH_CHILD(DT_PATH(cpus, power_states), DEFINE_DISABLED_PM_STATE)
                                                      ^
zephyr/subsys/pm/state.c:90:26: error: invalid application of 'sizeof' to an incomplete type 'const struct pm_state_info []'
        for (uint8_t i = 0; i < ARRAY_SIZE(disabled_states); i++) {
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
zephyr/include/zephyr/sys/util.h:122:38: note: expanded from macro 'ARRAY_SIZE'
        ((size_t) (IS_ARRAY(array) + (sizeof(array) / sizeof((array)[0]))))
                                            ^~~~~~~
1 warning and 2 errors generated.

Below are the changes I had to make:

diff --git a/subsys/pm/state.c b/subsys/pm/state.c
index d0e8ad3da92..86f935399b0 100644
--- a/subsys/pm/state.c
+++ b/subsys/pm/state.c
@@ -57,11 +57,11 @@ static const uint8_t states_per_cpu[] = {
 };

 #define DEFINE_DISABLED_PM_STATE(node) \
-       IF_ENABLED(DT_NODE_HAS_STATUS(node, disabled), (PM_STATE_INFO_DT_INIT(node),))
+       COND_CODE_1(DT_NODE_HAS_STATUS(node, disabled), (PM_STATE_INFO_DT_INIT(node),), ())

 /* Array with all states which are disabled but can be forced. */
 static const struct pm_state_info disabled_states[] = {
-       DT_FOREACH_CHILD(DT_PATH(cpus, power_states), DEFINE_DISABLED_PM_STATE)
+       DT_FOREACH_CHILD(DT_PATH(power_states), DEFINE_DISABLED_PM_STATE)
 };

 uint8_t pm_state_cpu_get_all(uint8_t cpu, const struct pm_state_info **states)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So power-states in DT are defined in root and not in cpus?
I've updated patch to look for power states in /power_states or /cpus/power_states.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So power-states in DT are defined in root and not in cpus?

Yes.

But from what I see, this is the case only for one platform. Initially, it was more widespread but was changed in pull request #60749. The ACE30 platform was integrated into the main branch later and this was not caught in the review. I made a PR that fixes this so that the structure is consistent across all platforms (#89492).

pm_state_get() is used to get a PM state that can be forced.
Disabled states (not included in automatic state selection) can
also be forced thus they should be included in search inside
pm_state_get().

Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no>
@nordic-krch
Copy link
Copy Markdown
Contributor Author

@ceolin I've updated patch to include case when no power states are defined in DT.

@kartben kartben merged commit 8ad4447 into zephyrproject-rtos:main May 5, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants