mcxw7x: Add Power Management support #96891
Conversation
mmahadevan108 force-pushed the Power_MCXW71 branch from 69dda9d to e3a628e Compare 
soc/nxp/mcx/mcxw/mcxw7xx/soc.h Outdated
| | ||
| #undef NXP_ENABLE_WAKEUP_SIGNAL | ||
| #define NXP_ENABLE_WAKEUP_SIGNAL(sig) WUU_SetInternalWakeUpModulesConfig(WUU0,\ | ||
| sig, kWUU_InternalModuleInterrupt); |
There was a problem hiding this comment.
Hello @bjarki-andreasen, it seems that the management of wakeup sources in zephyr is currently quite fragmented? Some wakeup sources are managed through device drivers, while others are managed at the SoC level (soc.c/.h). I would like to ask the power management maintainer if there are any plans to introduce a standardized/systematized wakeup source management system. Thanks.
There was a problem hiding this comment.
@ZhaoxiangJin it was a discussion a lot the last few years. but what you should know here is in our case that we intend to manage all of them by device driver. this soc.h define idea is to have a common api across all nxp platform that the nxp driver can call. but yes, this approach is just how nxp does it, but I think it will work for us, and is not exposed actually to client code. issue only would arise if NXP platform for some reason used some non-nxp driver for an soc peripheral or vice versa (which, is not impossible, since some IP can come from third party designer that is on many different vendor SOC, but usually still there are changes and they write their own driver pretending it's not third party is what I have noticed)
the way that is common in all of zephyr to declare/configure if something is a wakeup is by putting the wakeup-source property on it's node in DT.
soc/nxp/mcx/mcxw/mcxw7xx/power.c Outdated
| void nxp_mcxw7x_power_init(void) | ||
| { | ||
| /* Enable LPTMR0 as wakeup source */ | ||
| NXP_ENABLE_WAKEUP_SIGNAL(0); |
There was a problem hiding this comment.
The mcxw7x has many wakeup sources. Users may need GPIO wake up, RTC wake up, or LPTMR wake up. However, based on the current approach, users cannot configure them. They can only refer to the SDK implementation or the SoC RM to know how to enable the wakeup sources they need here. Does this go against the Zephyr concept of configure hardware features based on the devicetree?
Currently, Zephyr does not have a wakeup source management subsystem. My view is that we need to describe the WUU information in the wakup source peripheral node, and then configure the WUU in the driver based on the information in the device tree.
As a SoC vendor, we need to enable all these wakeup sources in SoC.dts and driver. The user can use /delete-property/ is-wakeup-source; to remove wakeup sources they don't need in the board.dts or application.dts.
There was a problem hiding this comment.
The mcxw7x has many wakeup sources. Users may need GPIO wake up, RTC wake up, or LPTMR wake up. However, based on the current approach, users cannot configure them. They can only refer to the SDK implementation or the SoC RM to know how to enable the wakeup sources they need here. Does this go against the Zephyr concept of configure hardware features based on the devicetree?
I think the reason mahesh put this here is because the lptmr in this case is chosen as the kernel timer when CONFIG_PM=y (hence building this function), so it is a system requirement that it should be a wakeup source, otherwise the system doesn't function. So it should not be left up to user configurability in this case. The LPTMR though can also be used just as a normal counter (not system timer) and in that case the wakeup-source property for configuration would be relevant and the counter driver should be responsible to call this macro to configure the wakeup source conditionally. But for system timer, it is not conditional, it is mandatory.
But, speaking of this line, @mmahadevan108 , this 0 is a magic number, is there not some identifier from the SDK that can mean LPTMR0 wakeup signal?
Currently, Zephyr does not have a wakeup source management subsystem. My view is that we need to describe the WUU information in the wakup source peripheral node, and then configure the WUU in the driver based on the information in the device tree. As a SoC vendor, we need to enable all these wakeup sources in SoC.dts and driver. The user can use
/delete-property/ is-wakeup-source;to remove wakeup sources they don't need in the board.dts or application.dts.
No, I don't agree to this approach, nothing should be a wakeup by default, except the system timer. User should be explicit about what causes wakeup for their system, not try to figure out all the ones they need to disable, that is just annoying.
As for the discussion about how to do generic wakeup sources in zephyr, I think the current paradigm is well understood enough and sufficient , and we should avoid over architecting and causing churn across the whole project unless we really decide its actually needed, not just for pedantic reasons. But in an ideal world, probably each node would be putting some controller+data style info using phandle arrays in DT similar to the resets and interrupts and clocks property, about how to configure the wakeups, especially if one device can have different wakeup sources. But I think we should cross that bridge when we come to it, not just for the sake of it.
There was a problem hiding this comment.
I can move this to the driver level. I have created PR #96996 for such cases where the wakeup signal is not mapped to the interrupt number.
mmahadevan108 force-pushed the Power_MCXW71 branch 2 times, most recently from 5ea2df0 to 1c69397 Compare soc/nxp/mcx/mcxw/mcxw7xx/power.c Outdated
| /* Set MAIN_CORE and MAIN_WAKE power domain into sleep mode. */ | ||
| config.clock_mode = kCMC_GateAllSystemClocksEnterLowPowerMode; | ||
| config.main_domain = kCMC_SleepMode; | ||
| config.wake_domain = kCMC_SleepMode; | ||
| CMC_EnterLowPowerMode(CMC0, &config); |
There was a problem hiding this comment.
we should not be setting domain states in the SOC power driver, this is backwards. the state of the domains should be handled by domain drivers and the result will affect what actual SOC state is selected in the first place based on device runtime PM constraints. cc @bjarki-andreasen
There was a problem hiding this comment.
Optimally the devicetree would contain the power domains which manage the main and wake domains, if anything needs them to be resumed, the power domains would be resumed directly (thus not in sleep mode) and the sleep mode which can only be entered if they are suspended, would have been blocked by the drivers/power domains with pm_policy_state_lock_get() or similar.
That said, some hardware is just quirky, it may make no sense to manage these power domains independently, which is maybe hinted at by the CMC_EnterLowPowerMode(CMC0, &config); taking one config which configures all of them together, so this may just be the way to enter one particular sleep mode.
mmahadevan108 force-pushed the Power_MCXW71 branch 3 times, most recently from 3418f90 to 4fc5b3b Compare 
zephyrbot requested review from DerekSnell, EmilioCBen, George-Stefan, MaochenWang1, PetervdPerk-NXP, axelnxp, bperseghetti, butok, dbaluta, dleach02 and manuargue 
| sleep_optimized: sleep-optimized { | ||
| compatible = "zephyr,power-state"; | ||
| power-state-name = "suspend-to-idle"; | ||
| substate-id = <0>; | ||
| min-residency-us = <500>; | ||
| exit-latency-us = <10>; | ||
| }; |
There was a problem hiding this comment.
What testing is done with this state? It looks like you disable the NBU in power.c . This is really not how we are supposed to be doing power management, by randomly hardcoding things being disabled in power.c. Do you even know if it recovers after disable? Is runtime PM enabled in HCI driver? I don't see any power domains defined to manage this. This looks like an untested technical debt you are adding. Like I said before, I only included this to you in a brainstorming teams comment before we apparently decided that we are not doing device PM on this PR, so I don't think we should be doing this here. But if you really tested it, then please explain what testing you did.
There was a problem hiding this comment.
This was tested using tests/subsys/pm/power_mgmt_soc
There was a problem hiding this comment.
Okay so you did not test anything regarding the state you programmed in then which you programmed that it disables the radio NBU. Those need to be tested with the BLE samples at least entering this "sleep optimized" power mode. My recommendation is that since I don't know of any requirement or request for this, that you just remove this state for now, instead of just adding another unvalidated surface for bugs.
There was a problem hiding this comment.
I have deleted min-residency-us value as this was picked randomly by me. We can re-add this at a later time.
There was a problem hiding this comment.
I wouldn't expect deep sleep LP state to work on BLE with this enablement yet. Radio wakeup sources will be needed. As Declan mentioned at least sleep modes shall be working with this.
| /* Set MAIN_CORE and MAIN_WAKE power domain into Deep Sleep Mode. */ | ||
| config.clock_mode = kCMC_GateAllSystemClocksEnterLowPowerMode; | ||
| config.main_domain = kCMC_DeepSleepMode; | ||
| config.wake_domain = kCMC_DeepSleepMode; |
There was a problem hiding this comment.
this dependency needs to be expressed in DT. It's fine for now to hardcode disabling them in this state, but we need to describe the connection of these domains on being disabled by this state in DT. If you want, we can do it in a later PR since I think this platform still does not have device PM generally enabled anyways.
| | ||
| #define WUU_WAKEUP_LPTMR_IDX 0 | ||
| | ||
| void mcxw7xx_set_wakeup(int32_t sig) |
There was a problem hiding this comment.
seems only LPTMR is used as wakeup source, will the external pin be enabled as wake up source?
There was a problem hiding this comment.
Yes, we will need to add a WUU driver for external pins.
mmahadevan108 force-pushed the Power_MCXW71 branch from 4fc5b3b to 5a27483 Compare mmahadevan108 force-pushed the Power_MCXW71 branch 2 times, most recently from c1d37af to 6b9a922 Compare Add support for power management states Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
Add bindings for the power related modules. Use the bindings Kconfig to pull in SDK drivers for cmc, spc, vbat and wuu. Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
mmahadevan108 force-pushed the Power_MCXW71 branch from 6b9a922 to 1063bb7 Compare Add PM support for MCXW SoC's Signed-off-by: Mahesh Mahadevan <mahesh.mahadevan@nxp.com>
|
8 participants
Add support for power management states
Signed-off-by: Mahesh Mahadevan mahesh.mahadevan@nxp.com