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.

Sign up for GitHub

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

Jump to bottom

subsys: pm: add check for device busy in policy #61726

Merged
merged 1 commit into from
Sep 15, 2023
Merged

subsys: pm: add check for device busy in policy #61726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions soc/x86/intel_ish/intel_ish5/pm/Kconfig.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ config GDT_RESERVED_NUM_ENTRIES
config REBOOT
default y

config PM_NEED_ALL_DEVICES_IDLE
default y

endif
7 changes: 7 additions & 0 deletions subsys/pm/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ config PM_S2RAM
help
This option enables suspend-to-RAM (S2RAM).

config PM_NEED_ALL_DEVICES_IDLE
bool "System Low Power Mode Needs All Devices Idle"
depends on PM_DEVICE && !SMP
help
When this option is enabled, check that no devices are busy before
entering into system low power mode.

choice PM_POLICY
prompt "Idle State Power Management Policy"
default PM_POLICY_DEFAULT
Expand Down
7 changes: 7 additions & 0 deletions subsys/pm/policy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <zephyr/sys/time_units.h>
#include <zephyr/sys/atomic.h>
#include <zephyr/toolchain.h>
#include <zephyr/pm/device.h>

#if DT_HAS_COMPAT_STATUS_OKAY(zephyr_power_state)

Expand Down Expand Up @@ -136,6 +137,12 @@ const struct pm_state_info *pm_policy_next_state(uint8_t cpu, int32_t ticks)
uint8_t num_cpu_states;
const struct pm_state_info *cpu_states;

#ifdef CONFIG_PM_NEED_ALL_DEVICES_IDLE
if (pm_device_is_any_busy()) {
Copy link
Member

Choose a reason for hiding this comment

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

Also, take a look at the state locking API. It's the device driver code that needs to prevent system sleep if it's busy doing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out Intel ISH platform requires all devices are not busy to enter into system low power. Add state locking in every device driver may not be a suitable solution. Is it possible to add a Kconfig option in pm that enable such property, for example config NEED_ALL_DEVICES_UNBUSY, if this config is y then runs this pm_device_is_any_busy() checking in policy.c?

Copy link
Collaborator

Choose a reason for hiding this comment

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

here is a bad case:

  • a driver sets device busy, starts one transaction, then waits the completion semaphore with a long timeout or even FOREVER.
  • Zephyr runs to idle thread and gets a long idle time, so PM policy returns a deep sleep state.
  • PM suspends all other devices, then SoC goes into very low PM state, maybe suspend to disk.
  • SoC detects there's device interrupt pending, so wakes up immediately.
  • do all resume works.
  • device interrupt gets served.
    in such a case, low-power states should be blocked. Any driver should avoid it. It's like a common thing. It seems not good for all drivers to call pm state lock APIs.

Copy link
Member

Choose a reason for hiding this comment

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

If your device is waiting on something, and needs to prevent deep sleep, simply lock deep sleep states while waiting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me that most drivers have similar working model:

  • set register to trigger a transaction.
  • wait complete semaphore which will be given in ISR.
  • complete the transaction.
    all those drivers should call PM state lock APIs to lock all low power states after step 1. as a common driver, I've afraid they should call PM state lock API multi times to lock PM_STATE_SUSPEND_TO_IDLE, PM_STATE_STANDBY, PM_STATE_SUSPEND_TO_RAM and PM_STATE_SUSPEND_TO_DISK, then call the API multi times to unlock them after transaction done.
    this seems not good at all.

Copy link
Member

Choose a reason for hiding this comment

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

current solution doesn't solve the problems I mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

It is fine for me having this option wrapped around a Kconfig option if that is a common needed for devices in a platform. If it is particular to one device, I do stay with the state lock solution, but if that is common thing for the platform, I think this solution is reasonable, it less error prone.

Copy link
Collaborator

@kwd-doodling kwd-doodling Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

all ISH devices need it, except three on AON domain, HPET/RTC/IPC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

Copy link
Member

Choose a reason for hiding this comment

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

If ISH has more specific cases in term of pm_state selection, You can always define platform-specific policy CONFIG_PM_POLICY_CUSTOM.

agree, why not use a custom policy instead of change the default policy. PM_NEED_ALL_DEVICES_IDLE seems to be very specialized and soon enough we will be wondering what is the usecase behind it without too much context and details in the documentation. See https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/subsys/pm/power_mgmt/src/main.c#L222C29-L222C49 for an example of a custom policy.

return NULL;
}
#endif

if (ticks != K_TICKS_FOREVER) {
cyc = k_ticks_to_cyc_ceil32(ticks);
}
Expand Down
Loading