-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
power: Add device idle power management support #13382
Conversation
These changes are the outcome of the discussion in Dev Meeting related to PR: #12172 |
All checks are passing now. Review history of this comment for details about previous failed status. |
Codecov Report
@@ Coverage Diff @@
## master #13382 +/- ##
==========================================
+ Coverage 51.99% 52.02% +0.03%
==========================================
Files 308 308
Lines 45569 45569
Branches 10553 10553
==========================================
+ Hits 23693 23709 +16
+ Misses 17069 17057 -12
+ Partials 4807 4803 -4
Continue to review full report at Codecov.
|
31c9497
to
38f940c
Compare
include/device.h
Outdated
@@ -208,6 +208,7 @@ extern "C" { | |||
|
|||
struct device; | |||
|
|||
typedef void (*device_pm_cb)(struct device *dev, int status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add user-defined context the the callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
@@ -221,7 +222,7 @@ struct device_config { | |||
int (*init)(struct device *device); | |||
#ifdef CONFIG_DEVICE_POWER_MANAGEMENT | |||
int (*device_pm_control)(struct device *device, u32_t command, | |||
void *context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like this:
int (*device_pm_control)(struct device *device, u32_t command, void *arg, device_pm_cb cb, void *context)
typedef void (*device_pm_cb)(struct device *dev, int status, void *context);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
38f940c
to
2db68e2
Compare
@pizi-nordic Can you look the PR once? |
@pizi-nordic I have modified the device idle PM based in async device_set_power_state(). Please let me know if you any concerns with the design. Based on your feedback I will push the sample app. |
@pizi-nordic Can you please review the PR? based on your feedback i will work on sample app. |
@@ -339,7 +339,7 @@ Device Set Power State | |||
|
|||
.. code-block:: c | |||
|
|||
int device_set_power_state(struct device *device, u32_t device_power_state); | |||
int device_set_power_state(struct device *device, u32_t device_power_state, device_pm_cb cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How I can pass custom data to the callback?
(We agreed on a slightly different API: #13382 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int (*device_pm_control)(struct device *device, u32_t command, void *arg, device_pm_cb cb, void *context)
typedef void (*device_pm_cb)(struct device *dev, int status, void *context);
I missed the arg parameter you mentioned in the comment.
How are you planning to use arg parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arg allows to pass some parameters specific for given operation. I not have exact scenario for its usage now, however many times I missed this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sure, will update the PR accordingly.
60a61fa
to
13a21f1
Compare
@@ -339,7 +339,7 @@ Device Set Power State | |||
|
|||
.. code-block:: c | |||
|
|||
int device_set_power_state(struct device *device, u32_t device_power_state); | |||
int device_set_power_state(struct device *device, u32_t device_power_state, void *arg, device_pm_cb cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest swapping cb and arg arguments. This would result in more convenient order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to send the arg to callback() from driver set power function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, This should be opaque value which is passed from device_set_power_state() caller to callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
@@ -0,0 +1,158 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look like the FSM we agreed on during our call.
Do you need some kind of formal design proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I implement the FSM I wanted some clarity on the state transitions.
Consider the following scenario.
- device idle requested suspend
- pm_state change from ACTIVE -> SUSPENDING
- driver tries to suspend the device but fails to do so
- driver calls the callback with an error return code.
In this case, what should we set the device state to?
Also if the pm_state is in SUSPENDING and new request comes for SUSPEND what we should do? should we call the device_set_power_state() with new SUSPEND requests for return?
what action we should for new requests if the current case is SUSPENDING or RESUMING? we can not skip the new requests(ex: SUSPEND) even if it is the same as the previous (ex: SUSPENDING) request because the previous request might fail. It would be better to rely only on the actual state for queuing the requests.
Also, if the current pm_state SUSPENDING and the device were not put suspend yet and but a new request comes for RESUME. How we should handle this case? do we need to change the pm state to RESUMING?
This commit now uses the actual device PM states as pm_states so we don't need any translation of FSM to device states in the device_pm code. I feel this simplifies the whole logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pizi-nordic Please have look at my query above. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot use actual device state because it not covers transition state.
Here is algorithm I have in my mind. I hope it explain all the cases you described.
As you see, the algorithm is trying to reduce number of calls to device_set_power_state().
-
Assumptions:
- Device could change power state only when it is idle. There is no "forcing" power state, as this might render the system non-operational (for example shutting down SPI bus during write to SPI flash will corrupt data). Failure of setting power mode is a failure of device - you cannot rely on it any more.
- Suspending and resuming operations cannot be terminated until complete.
-
Variables:
- FSM state: (ACTIVE, SUSPENDED, SUSPENDING, RESUMING)
- Target state: (ACTIVE, SUSPENDED).
-
Pseudocode:
device_suspend() {
if (target_state == SUSPENDED) {
/* Device will eventually reach the SUSPSENDED state */
return;
}
target_state = SUSPENDED;
trigger_fsm();
}
device_resume() {
if (target_state == ACTIVE) {
/* Device will eventually reach the ACTIVE state */
return;
}
target_state = ACTIVE;
trigger_fsm();
}
trigger_fsm() {
run_through_workqueue(fsm);
}
/*
* As device_set_power_state() is asynchronous, this will no block workqueue.
* Also, workqueue allow us to call device api from thread mode as well as handle devices
* which calls callback directly from device_set_power_state().
*/
fsm() {
switch (fsm_state) {
case SUSPENDING:
case RESUMING:
/* We have to wait */
break;
case ACTIVE:
if (target_state == SUSPENDED) {
device_set_power_state(SUSPENDED, trigger_fsm)
fsm_state = SUSPENDING;
}
break;
case SUSPENDED:
if (target_state == ACTIVE) {
device_set_power_state(ACTIVE, trigger_fsm)
fsm_state = RESUMING;
}
break;
}
broadcast_state(fsm_state);
}
device_wait_suspended() {
while (recv_broadcasted_state() != SUSPENDED) { sleep() }
}
device_wait_active() {
while (recv_broadcasted_state() != ACTIVE) { sleep() }
}
device_suspend_sync() {
device_suspend();
device_wait_suspended();
}
device_resume_sync() {
device_resume();
device_wait_active();
}
This could be extended to cancel work queue job if is is not longer needed, but such shortcut must be carefully checked for race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into it and get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- FSM state: (ACTIVE, SUSPENDED, SUSPENDING, RESUMING)
- Target state: (ACTIVE, SUSPENDED).
device_suspend() {
if (target_state == SUSPENDED) {
/* Device will eventually reach the SUSPSENDED state */
return;
}target_state = SUSPENDED;
trigger_fsm();
}
device_resume() {
if (target_state == ACTIVE) {
/* Device will eventually reach the ACTIVE state */
return;
}target_state = ACTIVE;
trigger_fsm();
}
trigger_fsm() {
run_through_workqueue(fsm);
}
Are you suggesting we should have workquque in the device_pm.c? If so, do we need a saperate thread or we should submit the work on system thread?
fsm() {
switch (fsm_state) {
case SUSPENDING:
case RESUMING:
/* We have to wait */
break;case ACTIVE: if (target_state == SUSPENDED) { device_set_power_state(SUSPENDED, trigger_fsm) fsm_state = SUSPENDING; } break; case SUSPENDED: if (target_state == ACTIVE) { device_set_power_state(ACTIVE, trigger_fsm) fsm_state = RESUMING; } break;
}
broadcast_state(fsm_state);
}
is this the worked logic which maintains the states? also why broadcat fsm state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting we should have workquque in the device_pm.c? If so, do we need a saperate thread or we should submit the work on system thread?
IMHO we can safely use system workqueue. The separate thread was needed earlier because synchronous device_set_power_state() could block for a long time blocking execution of other things. Now, with the asynchronous device_set_power_state() this problem is eliminated, so we can use system thread.
There is a possibility to implement this without any workqueue (fsm executed directly from the caller), but this would require using spinlocks which in my opinion will be overkill.
is this the worked logic which maintains the states?
I am not fully getting this question. If you are asking which entity holds the device state: In the proposed model this is tracked by FSM.
why broadcat fsm state?
Broadcasting the FSM state is needed for blocking version of the device_get/put. Whenever state is changed, a signal is sent. If FSM reached desired state, then you can exit from the blocking function.
Also, the application can track FSM state and run some action if device reached desired state (example: Power off the system if all devices are suspended).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed, at least when it comes to the system workqueue
d3d473f
to
131e782
Compare
@pizi-nordic I have updated the PR as your comments. Please look at the overall flow/design of the code. I will update PR with sample app once you are fine with it. |
@ramakrishnapallala: OK. I will look at the code tomorrow morning. |
489d8d2
to
d14a116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from high level. I started detailed review (see my comments), but this is not finished yet. I will continue tomorrow.
subsys/power/device_pm.c
Outdated
k_sem_take(&dev->config->pm->lock, K_FOREVER); | ||
/* Bring up the device before disabling the Idle PM */ | ||
k_work_submit(&dev->config->pm->work); | ||
dev->config->pm->enable = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving dev->config->pm->enable = false;
before k_work_submit()
. If work queue thread has higher priority than caller thread, the work will be executed before enable flag change and the FSM will not update device state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wil fix it.
subsys/power/device_pm.c
Outdated
} else { | ||
k_poll_signal_check(&dev->pm->signal, | ||
&signaled, &result); | ||
if (result != target_state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, you are waiting here for first signal, which might be not the you are waiting for.
Also, the signal state is not reset, so this will work only once.
d14a116
to
d6b7d61
Compare
@pizi-nordic I have updated the PR, please have a look. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. Needs only some polish to be perfect :)
k_sem_take(&dev->config->pm->lock, K_FOREVER); | ||
dev->config->pm->enable = true; | ||
|
||
/* During the driver init, device can set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we have to clearly write in the docs for devices, that all drivers has to boot the device in suspended state.
Now, all devices are active after initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding in the DEVICE_DEFINE macro description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
LOG_MODULE_DECLARE(power); | ||
|
||
/* Device PM request type */ | ||
#define DEVICE_PM_ASYNC (1 << 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add #define DEVICE_PM_SYNC (0 << 0)
and use this flag later, the code will be easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok will add the macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
"Invalid device PM state requested"); | ||
|
||
if (target_state == DEVICE_PM_ACTIVE_STATE) { | ||
if (atomic_inc(&dev->config->pm->usage) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that usage count management should go into get/put and the device_pm_request() should be called only when necessary. Now we are mixing different tasks in one function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will avoid code duplication but anyways will check it again.
"Invalid device PM state requested"); | ||
|
||
if (target_state == DEVICE_PM_ACTIVE_STATE) { | ||
if (atomic_inc(&dev->config->pm->usage) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (atomic_inc(&dev->config->pm->usage) < 0)
: Are we starting count from -1?
We only have to trigger transition when usage changes from 0 to 1 and from 1 to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just to make sure we are also considering the case where some app/driver might call put() multiple times without calling get().
subsys/power/device_pm.c
Outdated
k_work_submit(&dev->config->pm->work); | ||
|
||
/* Incase of Sync request wait for completion event */ | ||
if (!(pm_flags & DEVICE_PM_ASYNC)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can save one level of indentation and make code easier to read ;-)
if (pm_flags & DEVICE_PM_ASYNC) {
return
}
/* Blocking code here */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
subsys/power/device_pm.c
Outdated
|
||
fsm_out: | ||
k_poll_signal_raise(&dev->config->pm->signal, pm_state); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
/* Set the fsm_state */ | ||
if (*((u32_t *)context) == DEVICE_PM_ACTIVE_STATE) { | ||
atomic_set(&dev->config->pm->fsm_state, | ||
DEVICE_PM_FSM_STATE_ACTIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pass DEVICE_PM_FSM_STATE_ACTIVE/DEVICE_PM_FSM_STATE_SUSPENDED as arg, the decoding here could be replaced by single call to atiomic_set() saving some flash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then again I will have to add temp local variable for storing target fsm state. Can we leave this as it for now.
} | ||
|
||
printk("Async wakeup request queued\n"); | ||
ret = k_poll(&async_evt, 1, K_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to wait in a loop until the state we are requesting is signalled. Other states might be signalled before that time.
ret = dummy_suspend(dev); | ||
} | ||
break; | ||
case DEVICE_PM_GET_POWER_STATE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO get power state API should be removed, but this might go into another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
cb(dev, ret, context, arg); | ||
} | ||
|
||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to differentiate the device_set_power_state() return value and the return code passed to callback:
- If device_set_power_state() return something other that 0, then callback will not be called and operation failed.
- If device_set_power_state() return 0, the operation status is reported through callback which will be/was called.
Also, this has to be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
I have a question regarding the first commit "device: Extend device_set_power_state API to support async requests". The commit message says:
However, the mere fact that some devices may take long time to suspend/resume does not seem like a good enough reason to make Looking at the remaining implementation let's say I have an external audio chip, or the aforementioned gyro and the chip documentation says that after waking the device up by toggling a gpio pin I need to wait 10 ms for the internal power to stabilize before the device can be considered active. The chip does not provide any interrupt to let me know that it's active. I simply need to wait 10 ms. The implementation of the My question is: what kind of real life use case are we trying to solve that requires an async version of The implementation proposed in this PR is complex and seems to go against the very principles Zephyr is based on. According to the Synchronous Calls paragraph of the Device Drivers and Device Model: |
Add framework for device Idle Power Management(IPM) for suspending devices based on device idle. This will help in saving power even while system(CPU) is active. The framework uses device_set_power_state() API set the device power state accordingly based on the usage count. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
d6b7d61
to
33e5e57
Compare
recheck |
@pizi-nordic please review the updated the PR. |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the comments from @pizi-nordic seem to have been addressed, with some notable exceptions regarding memory usage when it comes to what gets stored in the pm structure. I will wait for another round of feedback from @ramakrishnapallala and then we can take a decision for code freeze.
@@ -79,11 +79,11 @@ void main(void) | |||
u32_t p_state; | |||
|
|||
p_state = DEVICE_PM_LOW_POWER_STATE; | |||
device_set_power_state(dev, p_state); | |||
device_set_power_state(dev, p_state, NULL, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, an __ASSERT() here would be safer
static inline void device_pm_disable(struct device *dev) { } | ||
static inline int device_pm_get(struct device *dev) { return 0; } | ||
static inline int device_pm_get_sync(struct device *dev) { return 0; } | ||
static inline int device_pm_put(struct device *dev) { return 0; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any follow-up here @ramakrishnapallala ?
* @brief Disable device idle PM | ||
* | ||
* Called by a device driver to disable device idle power management. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
* | ||
* Called by a device driver to enable device idle power management. | ||
* | ||
* @param dev Pointer to device structure of the specific device driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
@@ -221,7 +222,7 @@ struct device_config { | |||
int (*init)(struct device *device); | |||
#ifdef CONFIG_DEVICE_POWER_MANAGEMENT | |||
int (*device_pm_control)(struct device *device, u32_t command, | |||
void *context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
@@ -0,0 +1,158 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed, at least when it comes to the system workqueue
LOG_MODULE_DECLARE(power); | ||
|
||
/* Device PM request type */ | ||
#define DEVICE_PM_ASYNC (1 << 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
} | ||
} | ||
|
||
static void device_pm_callback(struct device *dev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed by workqueue
|
||
static void pm_work_handler(struct k_work *work) | ||
{ | ||
struct device_pm *pm = CONTAINER_OF(work, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
k_sem_take(&dev->config->pm->lock, K_FOREVER); | ||
dev->config->pm->enable = true; | ||
|
||
/* During the driver init, device can set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
33e5e57
to
1604ed2
Compare
Added test for Device Idle Power Management to invoke device_pm_get/put API's. Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
@pizi-nordic @carlescufi I replied to your comments, can you check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with reservations due to the currently unresolved issues. I believe @pizi-nordic already owns a number of GitHub issues related to improving power management in general, I think we could add one more about device idle improvements.
device: Extend device_set_power_state API to support async requests
power: Add device idle power management support
samples: power: Add test for device Idle PM