cpu/nrf5x_common: fix ztimer issue on warm-boot#20665
Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom May 13, 2024
Merged
cpu/nrf5x_common: fix ztimer issue on warm-boot#20665benpicco merged 1 commit intoRIOT-OS:masterfrom
benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
325d202 to
a49543b
Compare
benpicco
reviewed
May 13, 2024
benpicco
reviewed
May 13, 2024
| { | ||
| /* abort if LF clock is already running */ | ||
| if (NRF_CLOCK->LFCLKSTAT & CLOCK_LFCLKSTAT_STATE_Msk) { | ||
| if (clock_lf_running) { |
Contributor
There was a problem hiding this comment.
Is there a case where this function would be called more than once anyway?
I think we could easily drop that check.
It's only called in rtt_init() which is run once, the call in nimble_riot.c could be guarded with a if (!IS_USED(MODULE_PERIPH_RTT))
Contributor
Author
There was a problem hiding this comment.
Moving this check to a guard in nimble_riot.c moves logic to somewhere where it doesn't belong to. And the product specification explicitly mentions that changing clock parameters while the clock is running is not allowed.
benpicco
approved these changes
May 13, 2024
174bb1e to
350399d
Compare
Contributor
Author
|
Thanks! |
mguetschow
added a commit
to mguetschow/RIOT-exercises
that referenced
this pull request
Apr 10, 2025
not needed anymore since RIOT-OS/RIOT#20665
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
We have been experiencing problems when using
ztimeron thefeather-nrf52840-senseboards under certain compiler versions (see inetrg/exercises#5 (comment) and following) after warm-boot (directly after flashing or after pressing the reset button on the board), while after a cold-boot (un- and re-plugging) the board would behave normally.I've spent quite some time debugging this issue and finally found the culprit: the
NRF_CLOCK->LFCLKSTATis apparently latched for some short time after a softreset (see this comment by Nordic, sadly it is not reported as an erratum) and wrongly reports the low-frequency clock to be running just after a reboot. Inclock_start_lf()ofcpu/nrf5x_common/clock.c, starting the clock is skipped iff the register reports the clock to be running. Apparently different compiler versions (reproducibly) lead to slightly different startup timings, which would explain why the issue only happens with some toolchain versions.The linked discussion suggests to add a check for the reset cause and ignoring the (wrong) state of the LFCLK after a soft reset. To avoid the interdependence with the
RESETREASregister (which might need to be read and cleared in a future version of RIOT or be expected to be untouched by some application code), I propose a different approach with this PR: Adding a static variable that remembers whether the LFCLK has been started (by RIOT) or not. If anyone has a better suggestion for a work-around, please shout out :)The above linked discussion is about nRF52832 (locally confirmed with an
nrf52dk), but I can confirm this issue exists on nRF52840-based boards (I have testedfeather-nrf52840-sense,feather-nrf52840-express,nrf52840dkandnrf52840dongle) as well. I have also testedcalliope-mini(with nRF51) where I didn't see the problem, but the suggested workaround should not harm unless the user application code tinkers with LFCLK itself.While the issue exists on all tested nRF52840-based boards, it has become especially apparent on
feather-nrf52840-sensesince the reset button triggers the bootloader, which itself triggers a soft-reset into the application. But with the following diff, it is reproducible on the other nRF52840-boards as well.Testing procedure
Apply the following diff to current
master:Compile with an affected compiler version (debian stable
arm-gcc-none-eabi 12.2.1is known to have the problem) and flash to some nRF52840 or nRF52832 based board: After the first soft-reset (either triggered from the bootloader in case of the Adafruit boards or triggered by the application code), the reported ztimer difference will be zero and the application will hang duringztimer_sleepas time is not advancing.With the proposed workaround added, the code should behave as expected.
Issues/PRs references
inetrg/exercises#5