NXP drivers: pwm mcux sctimer: adds PM low power support #89384
Conversation
github-actions
bot
added
area: PWM
github-actions
bot
requested review from
anangl,
butok,
dbaluta,
decsny,
DerekSnell,
dleach02,
EmilioCBen,
henrikbrixandersen,
manuargue,
mmahadevan108 and
Raymond0225
The sctimer relies on the core clock, which is disabled in both suspend and standby. Signed-off-by: Alex Rodriguez <[email protected]>
alrodlim
force-pushed
the
feature/pwm_mcux_sctimer_enable_pm3
branch
from
e95bdef to
e714e85
Compare
drivers/pwm/pwm_mcux_sctimer.c
Outdated
| case PM_DEVICE_ACTION_SUSPEND: | ||
| break; | ||
| case PM_DEVICE_ACTION_TURN_OFF: | ||
| SCTIMER_StopTimer(config->base, kSCTIMER_Counter_U); |
There was a problem hiding this comment.
just wondering why is this needed? if timer is turning off then surely it won't continue counting already?
There was a problem hiding this comment.
Prevent glitches when going into sleep mode. Better to have a controlled shutdown.
There was a problem hiding this comment.
This driver seems like an odd case with respect to PM. If the timer is running it would seem like an odd case to allow the system to go into PM.
alrodlim
force-pushed
the
feature/pwm_mcux_sctimer_enable_pm3
branch
from
e714e85 to
67d7139
Compare
alrodlim
force-pushed
the
feature/pwm_mcux_sctimer_enable_pm3
branch
from
67d7139 to
276bcc0
Compare
| @@ -147,6 +150,10 @@ static int mcux_sctimer_pwm_set_cycles(const struct device *dev, | |||
| if (ret < 0) { | |||
| return ret; | |||
| } | |||
|
|
|||
| /* Block from losing power while PWM output is enabled. */ | |||
| pm_policy_device_power_lock_get(dev); | |||
There was a problem hiding this comment.
Where do we unblock?
There was a problem hiding this comment.
We don't unblock since the PWM runs continuously once enabled and there is not API to cancel it. We don't want it to suddenly stop, especially if it's driving something critical.
There was a problem hiding this comment.
Sometimes it just drives an LED.
I think a request of 0 width pulse is supposed to be interpreted as request to make it stop
There was a problem hiding this comment.
Zephyr now supports a deinit API where user applications can call device_deinit() on a device. Would it make sense for us to add support for this API in this driver. We can release the power lock and deinit SCTimer.
There was a problem hiding this comment.
IMO this seems like another case where only application knows its requirements for using the pwm and driver should not be making assumptions about it and controlling what states are entered primarily from it's unknowing perspective. @alxrey makes a good point that if the pwm is controlling a critical thing then it already must already have critical code which knows when it will be in use or not.
PWM, unlike uart or SPI or something, is not a transactional API, so the driver can't instrinsically know when an operation is "complete" and when it needs to be blocking power states due to a transaction in progress. Things like GPIO and PWM control only makes sense to do from app level, and only app can know when whatever it is doing is still in progress or not, or what the significance is.
As far as implementing deinit specifically for the purpose of allowing PM entry, I think that is completely the wrong architectural direction. AFAIK deinit is supposed to be the opposite of an init, ie return device to default/uninitialized state, which is NOT what we want to do just to enter PM. So first of all, Ideally the device remains initialized and nothing has to change to enter PM except any bare minimum. And secondly, it's not a good design paradigm to overload and couple the meaning and mechanisms of separate functionalities together.
Ping @anangl and @bjarki-andreasen to help resolve this question about what to do or not do regarding PWM device drivers locking or unlocking system power state transitions, is what makes sense as far as the roles and responsibilities of different components of the PM architecture and what makes sense from the perspective of PWM specifically
There was a problem hiding this comment.
PM_DEVICE_RUNTIME has no such limitations :) See #84049 Drivers do indeed not know what the user wants, this is one of the reasons why we are working towards deprecating PM_SYSTEM_MANAGED in the first place. User calls pm_device_runtime_get() when user wants device to be operational. PM_DEVICE_RUNTIME automatically calls pm_policy_device_power_lock_get(), and makes sure a potential power domain, and its power domain etc. is resumed, before resuming device.
All a device driver needs to do, is become operational when PM_DEVICE_ACTION_RESUME is received. The driver does not need to know anything about locking power states, power domains, or do any self resuming/suspending internally based on best guesses of what the user wants. See #89031 and #89372
There was a problem hiding this comment.
I'm not sure it matters what the application is doing. If the IP was configured by a higher layer to start doing PWM stuff, then the driver knows that it can't allow the power mode to go into a state that would disrupt the IP block. And as @bjarki-andreasen noted, setting the pulse to zero is to put the pin into an inactive state which is effectively telling the driver the higher layer is finished using the IP block for PWM functions... the driver can then unblock the PM.
It doesn't really matter what the application is doing though because the system won't go into PM if the application thread is still running. And if the application wants to wake up at some future time to actually do some PWM activity, then the PM system will have the information necessary to determine if there is enough time to go into a power mode before this timed event occurs.
There was a problem hiding this comment.
I still believe the driver should be responsible for managing its own power state as it has full context of the hardware’s behavior/limitations. Shifting this responsibility to the user undermines the purpose of Zephyr's abstraction layers. Forcing users to manage individual power states also increases the risk of runtime bugs such as race conditions or inconsistent suspend/resume handling.
Here’s how this can be addressed:
- The driver calls pm_device_busy_set() when the device is active and clears it with pm_device_busy_clear() when idle. This should prevent the system power manager from suspending the device (needs confirmation).
- If the device loses state or behaves incorrectly in certain low power modes (e.g., PM3), the driver should call pm_policy_state_lock_get() to block entry into those modes.
- The driver should still allow the user to force a suspend if needed for example via pm_device_action_run(PM_DEVICE_ACTION_SUSPEND). The added complexity here is knowing when the system pm is calling suspend vs when the user is calling it, but I think (1) addresses this.
This gives users the flexibility to manage power at a higher level without burdening them with low level details.
As for the PWM specific case of whether to block low power modes during idle (i.e., constant output) states, this can be made configurable using a Kconfig option to accommodate different hardware or application needs.
There was a problem hiding this comment.
I still believe the driver should be responsible for managing its own power state as it has full context of the hardware’s behavior/limitations.
Drivers have never been responsible for managing their own power state and sounds like this is going to be even more true soon. The power state of devices is managed by the power subsystem. Drivers only react to the actions taken by the power subsystem, and give information about the state of their operation.
Shifting this responsibility to the user undermines the purpose of Zephyr's abstraction layers.
I don't see how this is the case. Also, this is not about just "shifting responsibility to the user", the goal here is to make as close to a one to one relationship as possible between knowledge of the system and responsibilities to the system for each component involved. What is ideal is if components that know something that others don't are responsible for what they know and capable to act on that knowledge. THAT is the purpose of the "abstraction layers". (although, this architecture, both in implementation and requirement, is much more complex and nuanced than simple layered abstraction).
Forcing users to manage individual power states also increases the risk of runtime bugs such as race conditions or inconsistent suspend/resume handling.
I don't see how that is the case. There is an atomic usage counter for a reason. If any part of the system is written to use a resource without releasing it, that's not a fault of the architecture, that's just a classic leakage bug. It's like malloc'ing memory without freeing it, which is usually on accident. This could happen in any part of the codebase, whether app level, subsystem level, driver level, anywhere.
Here’s how this can be addressed:
1. The driver calls pm_device_busy_set() when the device is active and clears it with pm_device_busy_clear() when idle. This should prevent the system power manager from suspending the device (needs confirmation).
That API is being removed soon according to power maintainers response to my question about it. It is redundant once this architecture becomes streamlined and system managed device PM is removed. So I don't think we should plan on using it anymore.
2. If the device loses state or behaves incorrectly in certain low power modes (e.g., PM3), the driver should call pm_policy_state_lock_get() to block entry into those modes.
That once again goes back to the question of whether or not a PWM device should be considered "in the middle of an operation" or not if it has had it's pulse width set to 0, and vice versa. To answer that, we really need @anangl (maintainer of PWM) or @henrikbrixandersen (collaborator of PWM) to clarify it. The specific questions are:
1. Should a PWM device outputting a non-zero pulse be allowed to be suspended by PM system?
2. Should a device outputting a 0 pulse be considered as disabled/off and not driving pin or having state?
3. Should the zephyr driver implementation make assumptions about whether or not the user wants to allow the PWM to be suspended or not, or about whether or not the system should transition modes based on the PWM state?.
Personally I think this is completely up to the app to know because 1) a 0 pulse width actually means that the pin is being driven at a constant inactive level, NOT that the PWM is not doing anything and 2) Only the app knows if the PWM being on is actually something that needs to block system PM transitions. For example, maybe it's just driving some RGB LED which turns off when the system goes to sleep, and that could be expected behavior. Why should we assume at driver level that the entire system needs blocked because the PWM is currently a certain width? Why should we assume that if someone passed in 0 width that they don't care about what the PWM is doing anymore? I would imagine that in most SOCs, if a pin is muxed to a peripheral and that peripheral drives the pin, if the peripheral loses power then the pin would be floating? But that's not the same thing as driving the pin at constant inactive level which is what the PWM API says. It would be great if a PWM maintainer clarified this.
3. The driver should still allow the user to force a suspend if needed for example via pm_device_action_run(PM_DEVICE_ACTION_SUSPEND). The added complexity here is knowing when the system pm is calling suspend vs when the user is calling it, but I think (1) addresses this.
The driver is not involved in this, and I am pretty sure that's how it already works. The source doesn't need to be known. And as far as I am understanding, this is also not exactly related to what we are talking about because the device suspend is not the issue here, it's the system mode transition.
This gives users the flexibility to manage power at a higher level without burdening them with low level details.
I actually think what you've described is more complex and detail leaking for a user than just calling a simple hardware agnostic reference count API. It's completely normal in OS to have reference counting specifically for this reason of different components not knowing about each other and not knowing what each other know. All of the APIs are implemented to just take a device pointer and user doesn't need to know any low level details. But having to call these APIs different ways depending on the situation like you're describing would be a result of leaking low level details.
As for the PWM specific case of whether to block low power modes during idle (i.e., constant output) states, this can be made configurable using a Kconfig option to accommodate different hardware or application needs.
That seems like a reasonable suggestion on paper, but adding configurability into the meaning and contract of APIs adds a lot of complexity to both the implementation and the usage of it, by an exponential amount. For example, if you add a configuration like that, now the amount of test cases (at the very least in configuration, but even potentially in code, depending on how the API is changed by the configurability) actually has to double. And each configuration added doubles it again, this is an exponential complexity. So if we already have a clean and unified way to do this at the app level which is agnostic completely to the resource (ie, we could be talking about pwm, uart, shell, display, the reference counting works the same no matter what the resource is), then I don't think it would be advantageous to start making configurations and changing how PM works for every different type of resource supported in zephyr. It would get unwieldy and confusing for users very fast, as well as adding exponential more maintenance effort for the kernel side developers.
There was a problem hiding this comment.
Thanks for the explanation and your points make sense. Just to clarify my earlier point, when I said drivers should manage their own power state, I was specifically referring to the ability to block low power states during critical operations (e.g., active transactions). Shifting this responsibility entirely to the application forces it to be aware of the driver’s internal timing and state, which I believe breaks the abstraction. That said, I understand this is a trade-off being made in favor of a more unified and flexible power management model.
Enables sleep mode (PM3) in RW61x. Once any channel is configured, PWM driver blocks standby and suspend modes idefinitely since there is no API to disable PWM outputs. Signed-off-by: Alex Rodriguez <[email protected]>
Enables System PM and Standby mode to verify that the PWM driver correctly blocks standby entry. Signed-off-by: Alex Rodriguez <[email protected]>
alrodlim
force-pushed
the
feature/pwm_mcux_sctimer_enable_pm3
branch
from
276bcc0 to
b20e33f
Compare
|
Enables Sleep mode (PM3) in RW61x.
Tested tests/drivers/pwm/pwm_api on FRDM-RW612. Verified that RW612 goes into standby mode once all channels are inactive (pulse = 0).