-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add Power Management support to TI cc23x0 SoC #91401
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
base: main
Are you sure you want to change the base?
Add Power Management support to TI cc23x0 SoC #91401
Conversation
c1c1969
to
956ad83
Compare
956ad83
to
f6397be
Compare
f6397be
to
b7b48d6
Compare
|
Hello @andyross @nordic-krch @vaishnavachath Would you mind reviewing this PR please when you are available ? |
Hi , @andyross @nordic-krch @teburd @vaishnavachath @vanti can you review when you have time ? |
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.
Will have a deeper look later, but some preliminary comments.
lfxosc: lf_xosc { | ||
compatible = "ti,cc23x0-lf-xosc"; | ||
}; |
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.
Wouldn't it be cleaner to declare it inside the cc23x0.dtsi and mark it as disabled and then enable it here?
Because as far as I can understand it, there is the possibility for a low frequency oscillator on all MCU's, but it is only placed here on the devboard.
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 stand for Low Frequency External Oscillator which hardware component which could or could not be populated on different board with cc2340r5 SoCs.
External oscillator (LFXT): external 32.768 kHz crystal oscillator connected across the X32P input and
X32N output pins.
case PM_STATE_STANDBY: | ||
pm_cc23x0_enter_standby(); | ||
break; |
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.
Wouldn't it make more sense to have the distinction here:
case PM_STATE_STANDBY: | |
pm_cc23x0_enter_standby(); | |
break; | |
case PM_STATE_STANDBY: | |
#ifdef CONFIG_CC23X0_RTC_TIMER | |
Power_sleep(PowerLPF3_STANDBY); | |
#else | |
pm_cc23x0_enter_standby(); | |
#endif | |
break; |
Then you could also don't compile the function and anything related to it based on the kconfig option.
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.
@gumulka your point here is to avoid having content of pm_cc23x0_enter_standby()
compiled ?
/* Reset timer and start from 0 */ | ||
HWREG(RTC_BASE + RTC_O_CTL) = 0x1; | ||
HWREG(RTC_BASE + RTC_O_CH0CC8U) = ticks; |
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 did not quite understand this, but could it be, that this will make the calculation of the elapsed tick inaccurate, because the timer was reset and the last_rtc_count was not?
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 am not sure I get your point. Can you elaborate if you think there is some kind of issue here ?
From datasheet :
RTC counter reset. Writing 1 to this bit will reset the RTC counter,
and cause it to resume counting from 0x0
0h = No effect
1h = Reset the timer.
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 believe this code to be correct, but in the function sys_clock_elapsed
you calculate the elapsed time as elapsed_rtc = current_rtc_count - last_rtc_count;
If you reset the timer, then the value for last_rtc_count
becomes irrelvant, as the timer itself was reset, making the calculations for sys_clock_elapsed
wrong.
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 else case in get_elapsed_ticks_rtc
address what you describe here, can you take look :
else {
return ((0xFFFFFFFF - last_rtc_count) + current_rtc_count);
}
default 32768 | ||
|
||
config SYS_CLOCK_TICKS_PER_SEC | ||
default 125375 |
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.
Where does this value come from? It seems unusually large.
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.
It was calculated , it is not a magic number. It is result from ticks which sys clock is making calculated to correct resolution
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 you share how it was calculated in the commit message or as a 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.
As RTC have 8us usable minimum resolution this result in 125000 ticks per second. 375 is compensation for the drift and round up/down because in normal operation mode RTC (and AON domain) is getting 1/2 of main 48Mhz clock which is 24Mhz.
b7b48d6
to
8d51cd5
Compare
196d044
to
eece3c2
Compare
With last update of the PR one more commit is added to solve UART PM issue which cause CI to fail with latest rebase on main |
Hi , @andyross @nordic-krch @vaishnavachath @vanti @dcpleung can you review when you have time ? |
Add support for RTC as timer for cc23x0 SoC. Signed-off-by: Stoyan Bogdanov <[email protected]>
Add choice menu where could be selected between RTC or SYSTIM for system timer. Signed-off-by: Stoyan Bogdanov <[email protected]>
In case RTC is used for system timer, it should not be used as counter device. Dependency restricts counter driver to work only with SYSTIM. Signed-off-by: Stoyan Bogdanov <[email protected]>
Add conditonal definition for RTC and SYSTIM with different values for both of them respecting clock speed and ticks per minute. Signed-off-by: Stoyan Bogdanov <[email protected]>
External 32.768 kHz crystal oscillator (LFXT) connected across the X32P input and X32N output pins. Signed-off-by: Julien Panis <[email protected]>
Enable external 32 kHz crystal oscillator. Signed-off-by: Julien Panis <[email protected]>
Add power management capabilities for cc23x0: - runtime-idle - standby - soft-off Signed-off-by: Stoyan Bogdanov <[email protected]>
In power management, add conditions to handle the case where RTC is used as main timer. Signed-off-by: Stoyan Bogdanov <[email protected]>
In power management, add support to take into account the alarms set in RTC. Alarm from RTC is processed like any other from SYSTIM. This prevents from missing interrupts. Signed-off-by: Stoyan Bogdanov <[email protected]>
Add support for PM to cc23x0 SoC. Signed-off-by: Stoyan Bogdanov <[email protected]> Signed-off-by: Julien Panis <[email protected]>
Replace CONFIG_PM with CONFIG_PM_DEVICE to include pm_lock in cc23x0 UART driver in struct uart_cc23x0_data Signed-off-by: Stoyan Bogdanov <[email protected]>
a584b8d
to
5f9dc67
Compare
re-assigning to TI SimpleLink maintainer. |
|
/* PM_STATE_SOFT_OFF can be entered only by calling pm_state_force */ | ||
state2: state2 { | ||
compatible = "zephyr,power-state"; | ||
power-state-name = "soft-off"; | ||
min-residency-us = <0>; | ||
exit-latency-us = <0>; | ||
status = "disabled"; | ||
}; |
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.
sys_poweroff()
exists now which is an API which actually has a clear, non violating promise, please consider using it instead of reusing the PM framework, since its designed to work with states which don't loose system context, which soft off violates (hence the need to force the system into this 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.
It might be part of the future work done on this SoC, thanks for the suggestion. At that point I would like avoid major changes if not strictly necessary since they will slow down this PR even more and it is blocking 4 more pending PRs.
This series adds Power Management support to TI cc23x0 SoC.
Datasheet: https://www.ti.com/lit/ds/symlink/cc2340r5.pdf