Enable wakeup with uart shell from PM2 on RW612, fix uart issues #89760

decsny · 2025-05-10T02:18:39Z

Support UART wakeup from PM2 with shell on RW612.

Changes:

Power subsys:
- Some refactor to policy_device_lock.c
- New API to check if a state will disable a device
Flexcomm:
- New standard clock names for flexcomms which are "default" and "sleep" clocks. The default is as it was before for normal use. The sleep clock is an indication for clock control driver to switch to a clock source available for the flexcomm in sleep modes.
- Enablement code for configuring the flexcomm UART as a wakeup.
RW612:
- Clock main clock from LPOSC in PM2.
- Add the flexcomm sleep clock names to DT.
- Support flexcomm clock switching in LPC syscon clock control driver used by RW612.
- Make the shell uart of FRDM_RW612 (flexcomm3) a wakeup source by default.

decsny · 2025-05-10T02:23:09Z

@bjarki-andreasen this PR kind of hinges on the new API I added to the power_device_lock interface, which is in the 2nd commit, can I get your feedback about that one before I open this up to wider review and start dealing with compliance/twisters/etc. And the first commit is a refactor in that subsystem file as well so you might also be interested there. The 4th commit is showing the use case I needed something like this for

bjarki-andreasen · 2025-05-12T07:05:18Z

@bjarki-andreasen this PR kind of hinges on the new API I added to the power_device_lock interface, which is in the 2nd commit, can I get your feedback about that one before I open this up to wider review and start dealing with compliance/twisters/etc. And the first commit is a refactor in that subsystem file as well so you might also be interested there. The 4th commit is showing the use case I needed something like this for

TL:DR, API makes sense and is needed here, we need a bit more to make it truly transparent to the user I think :)

You have definitely discovered an issue I have not considered at all, dealing with devices that are "suspended", but are still "active" enough to wake the system :) A driver registering to pm state change events is ok, and a driver being able to check it a state would disable it is also ok, so I think the API is reasonable, and needed for this usecase :)

From a users perspective, how would this work? pm_device_runtime_resume() will keep it active, so it would wake the system with "normal" IRQs, transmit works etc. pm_device_runtime_suspend() will suspend it, to a point where the system can enter PM2, and lastly, if the driver is marked as an enabled wakeup source, it will keep itself powered enough to wake the system even as PM2 is entered.

I'm just wondering, what if PM3 is desired? the UART being a wakeup source should prevent PM3 in this case right? so it would need to have a new set of zephyr,disabling-states, something like zephyr,stop-states or something to match the "suspended but wakeup capable" state, maybe it should even be an actual device state like PM_DEVICE_STATE_SUSPENDED is :)

decsny · 2025-05-12T15:09:24Z

TL:DR, API makes sense and is needed here, we need a bit more to make it truly transparent to the user I think :)

TL:DR, I give you a rambling response about various things.

A driver registering to pm state change events is ok, and a driver being able to check it a state would disable it is also ok, so I think the API is reasonable, and needed for this usecase :)

Yes, I think this API could be generally useful to users of Zephyr as well and is not just niche for this one type of case in this PR. An alternative I considered is to provide an API which gives some kind of list of states which would disable a device, as this might have less runtime performance overhead than doing these looping checks on every notification. But for this both the implementation and interface seemed like it would be a lot more awkward and obviously then involves user needing to storing more data in memory for each device and recall it later. I didn't want to go that route but I present the idea to you now in case you disagree with the option I chose.

You have definitely discovered an issue I have not considered at all, dealing with devices that are "suspended", but are still "active" enough to wake the system :)

Yes, it's a bit strange for me to understand as well from the documentation, about what it means how it says that the peripheral will be in "retention state" yet acts like it is active enough still to wake up.

And a further complication for this particular peripheral, which is not addressed on this PR, is that it apparently can be used to wake up even from deeper sleep modes, although I'm not sure if this is possible on this platform despite the information being in the reference manual as this could be a copy paste error. But for this uart IP generally, I guess it has the ability to wakeup the system partially in order to trigger some events on some kind of mux fabric (cc @carlescufi btw) without causing a wakeup interrupt to the core. The idea I guess therefore being that it can support a situation where the uart receives some data and triggers a DMA to handle receiving it to memory all without waking up the processor core or executing any software at all. I'm not sure how to even begin approaching that type of use case on Zephyr but it's not something that I think is important right now to us (nxp maintainers).

I'm just wondering, what if PM3 is desired? the UART being a wakeup source should prevent PM3 in this case right?

If wakeup sources are determined solely by the DT right now, then I don't personally think that this is how the wakeup-source property should be interpreted. That is to say, wakeup-source should indicate that the wakeup capability of that device is on or allowed, but not that that device should always be able to wake up the system from every state. At some point, the user has to be an embedded system engineer and read the reference manual of their SOC and know how they are meant to wake up the hardware from the states, and IMO Zephyr should just make it easier to express what they already know they want to do. And keeping in mind that once again if the DT property is the only indication of wakeup sources, DT is already not meant to be portable between platforms, that's actually the purpose is to be one of the few things that should be changed between platforms. So I don't think there is any reason to say, that wakeup-source needs to mean exactly the same thing from all software modes on all devices on all system hardware. For example, if a user needs for example to be able to wakeup from PM3 from uart activity on RW612, then they should know that they need to design their board to connect the UART RX line also to the one of the two special MCU wakeup pin, just like is done on the FRDM development board. (Note: I have not verified that functionality for this PR, right now only focusing on PM2)

so it would need to have a new set of zephyr,disabling-states, something like zephyr,stop-states or something to match the "suspended but wakeup capable" state, maybe it should even be an actual device state like PM_DEVICE_STATE_SUSPENDED is :)

For this situation on RW, it actually is partially a case as I said above of unclear documentation. The RM says that all the peripherals are put into "retention state" and from programmer perspective it is certainly true that for the most part they stop working. I may need to track down who is the chip architects to confirm want "retention" means, but what I have found, at least with the uart, is that maybe what actually happens is just that the clock to the peripherals is lost due to most of the clock generators on the system being gated in PM2. The motivation on this PR for example to change the main clock source during PM2 is because that sets up the clock tree in the one possible configuration for a clock to reach the flexcomm. So maybe it is actually inaccurate to say that the peripheral UART (or even the whole peripheral domain) is actually disabled in PM2, it's just practically going to be the case. Now with a PR like this it starts to muddy which part of the system (power driver, user DT, clock control driver, device driver) is responsible for knowing how the clock tree works and what is the state of it in various power modes. (cc @danieldegrasse). On the other hand, the UART in PM2 is not able to communicate to it's partner due to the changed clock source changing the baud rate, so maybe it is accurate to call it disabled.

From a users perspective, how would this work? pm_device_runtime_resume() will keep it active, so it would wake the system with "normal" IRQs, transmit works etc. pm_device_runtime_suspend() will suspend it, to a point where the system can enter PM2, and lastly, if the driver is marked as an enabled wakeup source, it will keep itself powered enough to wake the system even as PM2 is entered.

First of all, I'm assuming by resume and suspend, you meant get and put.

I think, it's not a perfectly clean solution on this PR, and maybe will need changed someday, but what I think is my answer for this right now is basically like the following:

pm_device_runtime_get called on the uart device will prevent the system from entering PM2, as you said. This is because the UART cannot function normally for the application/board setup in PM2 due to UART communication relying on the agreed upon baud rate, which, as seen in this PR, will change as as result of entering PM2 due to the faster clock source not being available. So if user intends to transmit or have a lot of incoming communication (to the point that the repeated wakeups and sleeps would overload the system, and they know that), then they will want to just lock out PM2, or generally, any state which would not allow the UART to communicate to it's partner normally.

pm_device_runtime_put called on the UART device could allow system to enter PM2. This would be when from user/system perspective we don't expect or know of any UART activity to happen.

Such as in the case of the shell where we interact on human timescale and therefore next uart activity is unknown and eons away in cpu time, but we still expect it someday, then user sets wakeup source DT property. If UART in DT has the wakeup-source property, then as seen in this PR, it will call the (eventually platform specific, unless a new abstraction is proposed) wakeup_cfg function, and the (platform agnostic) clock_control_configure function.

For situations like PM3 enabled where the UART wakeup alone is not sufficient to wakeup, user is responsible to know the platform and either 1) not enable PM3, 2) have some other mechanism to know when someone would be able to be using the shell and lock out PM3 from that information, or 3) design the board to wake up from UART activity with the deep sleep wakeup pins.

This is maybe not the best and maybe you are right that it might be cleaner ultimately to describe some other information in DT but for now from nxp perspective I am ok with the situation of things that I have on this PR described above.

bjarki-andreasen · 2025-05-12T19:08:57Z

One of the core design principles I'm adhering to regarding device PM is to keep it as open and flexible as possible, any device can be a power domain, any device can get/put any other device, and indeed, any device should be able to lock and react to any pm state change. I don't think there is a one solution fits all when it comes to the specifics of an SoC, I'm just clinging on to the simple "bottom/up user requests device when needed, drivers and soc do whatever to meet this" philosophy of the PM enhancement RFC :) So far seems promising

mmahadevan108 · 2025-05-13T20:45:29Z

drivers/serial/uart_mcux_flexcomm.c

 		data->pm_policy_state_lock = false;
 		pm_policy_device_power_lock_put(dev);
+	} else {
+		/* can't block systemn workqueue so keep re-submitting until it's done */


NIT, typo extra 'n'

mmahadevan108 · 2025-05-13T21:04:05Z

drivers/serial/uart_mcux_flexcomm.c

+		base->BRG = data->old_brg;
+	}
+}
+#endif /* CONFIG_PM_POLICY_DEVICE_CONSTRAINTS */


Update comment.

dleach02 · 2025-05-13T21:39:50Z

drivers/clock_control/clock_control_mcux_syscon.c

+		static uint32_t frgclksels[4];
+		static uint32_t frgctls[4];
+
+		if (clock_name & 0x80) {


what is the special bit at 0x80?

It's just decoding the arbitrary value given in the DT clock header that is specific for this driver. I could give the bit a macro name in the header

If the device header doesn't have a mask for it, then just put a comment block above it as to what it is for future me/you.

I think I will just make a descriptive macro mask on the next push in the dt binding header

dleach02 · 2025-05-13T21:43:05Z

drivers/serial/Kconfig.mcux_flexcomm

@@ -10,6 +10,7 @@ config UART_MCUX_FLEXCOMM
 	select SERIAL_SUPPORT_INTERRUPT
 	select SERIAL_SUPPORT_ASYNC if \
 		DT_HAS_NXP_LPC_DMA_ENABLED
+	select PM_POLICY_DEVICE_CONSTRAINTS if PM


The title references RW612 where this gets set already if PM is enabled (

zephyr/soc/nxp/rw/Kconfig.defconfig

Line 74 in fbdf4b9

config PM_POLICY_DEVICE_CONSTRAINTS

)

But I'm guessing that since this is shared IP we need this here. We might need to think through a bit this config and why we set it in the RW defconfig... yet still need to set it in the drivers Kconfig

Yes, I put this here because it is not specific to RW, even though RW is the only platform right now in tree that is enabling PM deep enough to disable the UART. But any platform that disables the UART on low power mode will need this dependency to avoid the garbage character.

mmahadevan108 · 2025-05-13T21:43:10Z

drivers/serial/uart_mcux_flexcomm.c

+		data->old_brg = base->BRG;
+		data->old_osr = base->OSR;
+		base->OSR = 8;
+		base->BRG = 0;


How did you decide on these values. A comment would be helpful.

There already is a comment on line 1168-1171 explaining that we are switching to the lowest baud rate.

True, but how did you calculate 8 and 0?

There is no calculation involved, BRG is the baud rate generator register and I put 0 which is the min, and OSR is the clocks per bit register which I put 8 which is the max

It would help if some of this is added to the comment. Not a blocker.

DerekSnell · 2025-05-14T13:54:43Z

drivers/clock_control/clock_control_mcux_syscon.c

update copyright year

DerekSnell · 2025-05-14T14:04:20Z

drivers/serial/uart_mcux_flexcomm.c

update copyright year

DerekSnell · 2025-05-14T14:08:04Z

drivers/serial/uart_mcux_flexcomm.c

+
+	if (data->pm_policy_state_lock) {
+		/* we can't block on TXidle mask in IRQ context so offload */
+		k_work_submit(&data->pm_lock_work);
 	}
 }
 #endif


Suggested change

#endif

#endif /* CONFIG_PM_POLICY_DEVICE_CONSTRAINTS */

soc/nxp/rw/power.c

EmilioCBen · 2025-05-14T18:58:01Z

drivers/serial/uart_mcux_flexcomm.c

@@ -26,6 +27,8 @@
 #include <fsl_inputmux.h>
 #endif

+#define UART_IS_WAKEUP (IS_ENABLED(CONFIG_PM) && DT_ANY_INST_HAS_BOOL_STATUS_OKAY(wakeup_source))


*Nit not specific to UART, is specific to mcux_uart_flexcomm,

Suggested change

#define UART_IS_WAKEUP (IS_ENABLED(CONFIG_PM) && DT_ANY_INST_HAS_BOOL_STATUS_OKAY(wakeup_source))

#define MCUX_UART_IS_WAKEUP (IS_ENABLED(CONFIG_PM) && DT_ANY_INST_HAS_BOOL_STATUS_OKAY(wakeup_source))

I'll put flexcomm but don't like mcux namespace being used in zephyr, since it's not an mcux product, it's just confusing

mmahadevan108 · 2025-05-14T19:45:01Z

drivers/serial/uart_mcux_flexcomm.c

 	k_work_init(&data->pm_lock_work, config->pm_unlock_work_fn);
 #endif

+#if UART_IS_WAKEUP
+	config->wakeup_cfg();
+	pm_notifier_register((struct pm_notifier *)&data->pm_handles);


You probably don't need the cast.

drivers/serial/uart_mcux_flexcomm.c

There is currently a bug where when entering low power modes, there is a garbage character sent if the UART is in the middle of a transmit when entering low power mode. This is because the fifo interrupt happens when last character is pulled by transmitter from fifo and then we unlock PM constraints at that moment, instead of waiting for actual transmitter to become idle. Fix by adding work item to check for this when needed. Signed-off-by: Declan Snyder <[email protected]>

Refactor this code so it has less nesting and is able to share some code between functions, by making a function to find the device constraints object which can be called before doing operations using it. Signed-off-by: Declan Snyder <[email protected]>

Add an API function that can be used by client code to check if a state disables a device. Signed-off-by: Declan Snyder <[email protected]>

On RW, normal configuration has all clock generators gated in PM2. Only the LPOSC is available for main clock source since it is a low power clock. Many of the peripherals on the chip are still "on" and do need a main clock source in order to be effective as wakeup sources to the chip as intended. So we should make this switch for PM2 specifically in order to achieve desired wakeup capabilities. Signed-off-by: Declan Snyder <[email protected]>

Add the feature for the flexcomm uart to be able to be a wakeup source from low power modes. To be able to do this on a relevant platform, the DT node for the UART needs to have the wakeup-source property and define a "sleep" clock. The details of handling the sleep clock and default clock are still platform specific but handled by the clock control driver of the platform, so that this code should be able to be applicable to any platform that follows the same DT definition. Signed-off-by: Declan Snyder <[email protected]>

Following the new feature in the flexcomm driver to be able to wake up from low power mode, the clock control drivers have to handle the platform specific details, so this commit adds to the already ugly mess that is the LPC syscon driver which is used by RW currently and makes the required devicetree changes. Make the console/shell uart on the FRDM_RW612 take advantage of this by default. Signed-off-by: Declan Snyder <[email protected]>

sonarqubecloud · 2025-05-22T21:05:23Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dleach02 · 2025-05-30T16:04:42Z

@mmahadevan108 @DerekSnell if you are good with the responses please give +1

decsny force-pushed the uart_wakeup_shell branch 2 times, most recently from 0abbec4 to edc7974 Compare May 12, 2025 16:34

decsny marked this pull request as ready for review May 12, 2025 16:35

github-actions bot added area: Power Management platform: NXP Drivers NXP Semiconductors, drivers area: Clock Control area: UART Universal Asynchronous Receiver-Transmitter platform: NXP NXP labels May 12, 2025

github-actions bot requested review from bjarki-andreasen, butok, ceolin, dbaluta, dcpleung, DerekSnell, dleach02, EmilioCBen, manuargue, mmahadevan108, nashif, nordic-krch, Raymond0225, teburd and tmleman May 12, 2025 16:36

github-actions bot assigned dleach02 and mmahadevan108 May 12, 2025

decsny force-pushed the uart_wakeup_shell branch from edc7974 to 41fc65e Compare May 13, 2025 16:22

mmahadevan108 reviewed May 13, 2025

View reviewed changes

dleach02 reviewed May 13, 2025

View reviewed changes

mmahadevan108 reviewed May 13, 2025

View reviewed changes

DerekSnell reviewed May 14, 2025

View reviewed changes

EmilioCBen reviewed May 14, 2025

View reviewed changes

mmahadevan108 reviewed May 14, 2025

View reviewed changes

mmahadevan108 reviewed May 15, 2025

View reviewed changes

drivers/serial/uart_mcux_flexcomm.c Show resolved Hide resolved

decsny mentioned this pull request May 21, 2025

PM_DEVICE_POWER_DOMAIN refactor #89372

Open

mmahadevan108 previously approved these changes May 22, 2025

View reviewed changes

decsny added 6 commits May 22, 2025 15:26

pm: Add API to check if a state disables a device

fe1d1f9

Add an API function that can be used by client code to check if a state disables a device. Signed-off-by: Declan Snyder <[email protected]>

decsny dismissed mmahadevan108’s stale review via f1884c6 May 22, 2025 20:36

decsny force-pushed the uart_wakeup_shell branch from 41fc65e to f1884c6 Compare May 22, 2025 20:36

EmilioCBen approved these changes May 23, 2025

View reviewed changes

decsny requested review from mmahadevan108 and DerekSnell May 27, 2025 14:37

DerekSnell mentioned this pull request May 30, 2025

NXP drivers: spi_mcux_flexcomm: Adds PM low-power recovery support #89290

Draft

dleach02 approved these changes May 30, 2025

View reviewed changes

kartben merged commit f1df212 into zephyrproject-rtos:main May 30, 2025
27 checks passed

	#define UART_IS_WAKEUP (IS_ENABLED(CONFIG_PM) && DT_ANY_INST_HAS_BOOL_STATUS_OKAY(wakeup_source))
	#define MCUX_UART_IS_WAKEUP (IS_ENABLED(CONFIG_PM) && DT_ANY_INST_HAS_BOOL_STATUS_OKAY(wakeup_source))

Enable wakeup with uart shell from PM2 on RW612, fix uart issues #89760

Enable wakeup with uart shell from PM2 on RW612, fix uart issues #89760

Conversation

decsny commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarki-andreasen commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny commented May 12, 2025

Uh oh!

bjarki-andreasen commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

decsny May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

decsny May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented May 22, 2025

Quality Gate passed

Uh oh!

dleach02 commented May 30, 2025

Uh oh!

Uh oh!

Uh oh!

decsny commented May 10, 2025 •

edited

Loading

decsny commented May 10, 2025 •

edited

Loading

bjarki-andreasen commented May 12, 2025 •

edited

Loading

bjarki-andreasen commented May 12, 2025 •

edited

Loading

decsny May 14, 2025 •

edited

Loading

decsny May 13, 2025 •

edited

Loading

mmahadevan108 May 13, 2025 •

edited

Loading