-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Update snvs rtc for generic rtc driver and enable it for RT10xx boards #97551
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
Update snvs rtc for generic rtc driver and enable it for RT10xx boards #97551
Conversation
0cac71e
to
b033e53
Compare
drivers/counter/counter_mcux_snvs.c
Outdated
SNVS_HP_RTC_EnableInterrupts(config->base, kSNVS_RTC_AlarmInterrupt); | ||
#ifdef MCUX_SNVS_SRTC | ||
SNVS_LP_SRTC_EnableInterrupts(config->base, kSNVS_SRTC_AlarmInterrupt); | ||
#endif |
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.
nit: you could #define SNVS_ALARM_INERRUPT or something to the right value in one place instead of repeating this #ifdef MCUX_SNVS_SRTC in multiple places of the file.
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.
Updated: add SRTC helper macros.
drivers/counter/counter_mcux_snvs.c
Outdated
cb = data->alarm_hp_rtc_callback; | ||
data->alarm_hp_rtc_callback = 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 think I don't have the context, but why are you setting the stored callback pointer to 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.
Counter API semantics (counter.h) say alarms are single-shot: “After expiration … channel is considered available and can be set again.” Drivers must clear their “alarm active” state so counter_set_channel_alarm can succeed again. Calling mcux_snvs_cancel_alarm() inside the ISR is not ideal (it busy-waits on HPCR/LPCR bits). Nulling the stored callback is the more common way, many Zephyr drivers do this way(e.g., mcux_rtc.c, mcux_lpc_rtc.c, mcux_gpt.c, mcux_ctimer.c).
mcux_snvs_cancel_alarm(dev, 0); | ||
cb = data->alarm_hp_rtc_callback; | ||
data->alarm_hp_rtc_callback = NULL; | ||
cb(dev, 0, current, data->alarm_hp_rtc_user_data); |
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.
check for null pointer first?
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.
Updated.
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 2018, NXP | |||
* Copyright (c) 2018,2025 NXP |
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.
remove (c) and put a space after the comma.
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.
Updated.
dts/arm/nxp/nxp_rt10xx.dtsi
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2017,2023,2024 NXP | |||
* Copyright 2017,2023,2024,2025 NXP |
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.
put spaces after commas
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.
Updated to "2017, 2023-2025".
dvp_fpc24_interface: &csi {}; | ||
|
||
&snvs_rtc { | ||
status = "okay"; |
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 one looks redundant, it looks not disabled in the soc dtsi
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.
Removed.
# | ||
# Copyright 2025 NXP | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
|
||
CONFIG_RTC_ALARM=y | ||
CONFIG_RTC_INIT_PRIORITY=70 |
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.
IMO squash overlay to previous (board) commit, we dont want to bloat the commit log with habit of putting an overlay for a test of a feature in another commit as where the feature is added, this is really not necessary, it's a routine basic part of enabling the feature on a board
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 have enabled all RT10xx boards, so I reorganize the commits. Please review again.
# | ||
# Copyright 2025 NXP | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
|
||
CONFIG_RTC_ALARM=y | ||
CONFIG_RTC_INIT_PRIORITY=70 | ||
CONFIG_TEST_RTC_ALARM_TIME_MASK=255 |
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.
two commits each for an overlay, squash to the board commit. there should only be 2 commits on this PR, not 4. We don't need a commit for every file added, that is too much. A commit is for a reviewable and isolatable and complete unit of work. These tiny overlay is part of the work of enabling the feature on the board.
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 have enabled all RT10xx boards, so I reorganize the commits. Please review again.
b033e53
to
089bd2d
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.
The board using the "virtual" RTC to the rtc api test suite must be redundant, it should work with any counter, and should be tested using FFF. If the counter test suites pass, the RTC API test will also pass if the rtc_counter has been tested well enough.
@Holt-Sun since the test overlay are all the same, i would look to make a testcase in the .yaml instead, I think bjarki is blocking because all the overlay can get annoying |
089bd2d
to
9029b4a
Compare
Not exactly, the rtc_counter APIs have limitation, like it cannot support frequency not equal 1. When I enable a new platform, the counter test cases all run passed but RTC test cases run failed. |
Good suggestion, updated. |
9029b4a
to
0873a20
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.
ok, I like this better, though still not a fan of testing a purely software feature with real hardware, better than not testing it, but it makes it hard to see if an issue is with the hardware or rtc counter wrapper if something fails
@Holt-Sun Please add |
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.
Blocking on adding the rtc to the board.yaml as mentioned above.
1. Add SRTC helper macros. 2. Implement start/stop APIs. Signed-off-by: Holt Sun <[email protected]>
The counter_rtc child node is for rtc-counter. Signed-off-by: Holt Sun <[email protected]>
enable SNVS counter_rtc for mimxrt10xx board. Signed-off-by: Holt Sun <[email protected]>
Add "rtc" into supported board.yaml. Signed-off-by: Holt Sun <[email protected]>
enable alarm and init priority for mimxrt10xx sample/drivers/rtc Signed-off-by: Holt Sun <[email protected]>
649e73c
0873a20
to
649e73c
Compare
649e73c
to
8ca451e
Compare
add drivers.rtc.rtc_api.rtc_counter test case for counter rtc. Signed-off-by: Holt Sun <[email protected]>
8ca451e
to
20f6af6
Compare
|
Update snvs rtc for generic rtc driver.
Enable it for RT10xx boards: samples and tests.