-
Notifications
You must be signed in to change notification settings - Fork 2k
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
sam0/rtc_rtt: optimizations to get around the painful long syncwaits #18920
Conversation
99754ca
to
884de59
Compare
cpu/sam0_common/periph/rtc_rtt.c
Outdated
_rtt_clear_alarm(); | ||
|
||
/* make sure that preceding changes have been applied */ | ||
_wait_syncbusy(); |
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 don't think that's how it works. You need to sync before reading or writing, otherwise the call (and peripheral bus) blocks. I got confused
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.
Yes, I sync before I'm writing.
The current master syncs after writing to the ALARM/COMP register.
Or am I misunderstanding something here?
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, so the reference manual says:
When executing an operation that requires synchronization, the Synchronization Busy bit in the Status register
(STATUS.SYNCBUSY) will be set immediately, and cleared when synchronization is complete. The Synchronization
Ready interrupt can be used to signal when synchronization is complete. This can be accessed via the Synchronization
Ready Interrupt Flag in the Interrupt Flag Status and Clear register (INTFLAG.SYNCRDY).
If an operation that requires synchronization is executed while STATUS.SYNCBUSY is one, the bus will be stalled. All
operations will complete successfully, but the CPU will be stalled and interrupts will be pending as long as the bus is
stalled.
So the reason to sync after writing the register is to not have a consecutive call stall the bus.
But, if waiting before writing the register, the waiting can be skipped if no operation is in progress.
The following bits need synchronization when written:
- Software Reset bit in the Control register (CTRL.SWRST)
- Enable bit in the Control register (CTRL.ENABLE)
The following registers need synchronization when written:
- The Counter Value register (COUNT)
- The Clock Value register (CLOCK)
- The Counter Period register (PER)
- The Compare n Value registers (COMPn)
- The Alarm n Value registers (ALARMn)
- The Frequency Correction register (FREQCORR)
- The Alarm n Mask register (MASKn)
Write-synchronization is denoted by the Write-Synchronization property in the register description.
The following registers need synchronization when read:
- The Counter Value register (COUNT)
- The Clock Value register (CLOCK)
Read-synchronization is denoted by the Read-Synchronization property in the register description.
Seems like the MODE2.INTENCLR
register is not part of this
(I had it the wrong way in my head, I thought you'd need to sync the clock, then there'd be a time window where an op (read/write) can complete.)
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.
Basically, we cloud skip the _wait_syncbusy()
because the write will sync us anyway?
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.
no, when we don't do the active _wait_syncbusy()
, we risk blocking all ISRs and the whole AHB.
884de59
to
fb8d0d8
Compare
rtc_set_alarm() / rtt_set_alarm() are heavily used by ztimer during ISR. This will reduce time spent during ISR drastically. We trust that the peripheral is able to propagate the alarm asynchronously.
fb8d0d8
to
268bdfe
Compare
Murdock results✔️ PASSED 268bdfe sam0/rtc_rtt: don't block until set_alarm has been propagated to periph
ArtifactsThis only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now. |
I took the branch from #18795 and made some measurements: Without this PR
With this PR merged on top
Yay, I guess :-) |
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.
Nice!
That makes periph_rtt
a vialble ZTIMER backend on sam0 - I might be able to drop the ztimer_no_periph_rtt
after all 😃
(still have to solve the Deep Sleep issue).
I also tested this on samr21-xpro
(since the old sam0 generation often was a bit special wrt RTT) but works fine in both tests/periph_rtt
and tests/periph_rtc
.
Thank you both for reviewing and testing this PR. I reduces pain on our side, as well :-) |
Contribution description
Instead of blocking until a alarm is set, we just trust that the alarm gets propagated down to the peripheral asynchronously.
Testing procedure
TBD
Issues/PRs references
#18883