Skip to content
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

cpu/efm32/timer_series2: fix interaction with pm_layered #18814

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

jue89
Copy link
Contributor

@jue89 jue89 commented Oct 27, 2022

Contribution description

While working on #17607, I found two bugs in the periph_timer implementation for Gecko Series 2 EFM32 CPUs:

During timer_start() and timer_stop(), the driver checks whether the timer isn't running yet resp. is currently running. Power modes are just blocked and unblocked in those cases to prevent leaks if timer_start() or timer_stop() is called multiple times in a row.

  1. timer_start() had the wrong logic. pm_block() wasn't called.
  2. timer_stop() has to make sure that the STATUS register is up-to-date.

Testing procedure

Abusing examples/hello-world with #17607 in place:

Diff for hello-world
diff --git a/cpu/efm32/periph/pm.c b/cpu/efm32/periph/pm.c
index 9c60448cef..10ac8017dc 100644
--- a/cpu/efm32/periph/pm.c
+++ b/cpu/efm32/periph/pm.c
@@ -22,7 +22,7 @@
 
 #include "em_emu.h"
 
-#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1
 #include "debug.h"
 
 void pm_set(unsigned mode)
diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9baf..b002792120 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -12,6 +12,8 @@ RIOTBASE ?= $(CURDIR)/../..
 # development process:
 DEVELHELP ?= 1
 
+USEMODULE += ztimer_msec ztimer_usec ztimer_ondemand_timer ztimer_ondemand_strict
+
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1
 
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a0..2de8077627 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,13 @@
  */
 
 #include <stdio.h>
+#include "ztimer.h"
+
+static void cb(void *arg)
+{
+    (void) arg;
+    puts("Hello again!");
+}
 
 int main(void)
 {
@@ -28,5 +35,12 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);
 
+    ztimer_t timer = { .callback = cb };
+    ztimer_set(ZTIMER_USEC, &timer, 2000000);
+
+    ztimer_sleep(ZTIMER_MSEC, 10000);
+
+    puts("Bye!");
+
     return 0;
 }
2022-10-28 00:01:48,602 # main(): This is RIOT! (Version: 2023.01-devel-193-g69a6d7-tmp)
2022-10-28 00:01:48,602 # Hello World!
2022-10-28 00:01:48,607 # You are running RIOT on a(n) xg23-pk6068a board.
2022-10-28 00:01:48,610 # This board features a(n) efm32 MCU.
2022-10-28 00:01:48,615 # [pm] enter EFM32_PM_MODE_EM1
(LETIMER and TIMER are active. EM1 is the lowest available mode, because TIMER requires the HFXTAL)
2022-10-28 00:01:50,612 # Hello again!
2022-10-28 00:01:50,614 # [pm] enter EFM32_PM_MODE_EM2
(Just LETIMER ist active. EM2 is the lowest available mode, because LETIMER requires the LFXTAL)
2022-10-28 00:01:58,614 # Bye!
2022-10-28 00:01:58,614 # [pm] enter EFM32_PM_MODE_EM3
(EM3 is the lowest mode. No peripheral is active anymore ...)

Issues/PRs references

@jue89 jue89 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: cpu Area: CPU/MCU ports labels Oct 27, 2022
@jue89 jue89 requested a review from gschorcht October 27, 2022 22:11
@jue89 jue89 requested a review from basilfx as a code owner October 27, 2022 22:11
@jue89 jue89 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 27, 2022
@github-actions github-actions bot added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Oct 27, 2022
@riot-ci
Copy link

riot-ci commented Oct 27, 2022

Murdock results

✔️ PASSED

54e915e cpu/efm32/timer_series2: sync STATUS register

Success Failures Total Runtime
1996 0 1996 06m:36s

Artifacts

This 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.

cpu/efm32/periph/timer_series2.c Outdated Show resolved Hide resolved
cpu/efm32/periph/timer_series2.c Outdated Show resolved Hide resolved
jue89 added 2 commits October 28, 2022 11:34
The timer hasn't been enabled yet, if the STATUS isn't RUNNING, yet ...
Make sure the STATUS register has been synced between the different clock domains.

Especially the LETIMER needs some time to update the STATUS register after it has been enabled. If the LETIMER is started and stopped with just a short delay, pm_layered gets confused because the timer seems to be inactive ...
@jue89 jue89 force-pushed the fix/efm32_series2_timer branch from 290f8b0 to 54e915e Compare October 28, 2022 09:36
@jue89
Copy link
Contributor Author

jue89 commented Oct 28, 2022

@gschorcht Thank you for reviewing! Does your ACK still apply? I've refactored the methods _letimer_is_running() and _timer_is_running() out.

@jue89
Copy link
Contributor Author

jue89 commented Oct 28, 2022

The testing example above still works with the refactored version of this PR.

@jue89
Copy link
Contributor Author

jue89 commented Nov 2, 2022

@gschorcht may I ask you for a quick look, if your ACK still applies?

@gschorcht
Copy link
Contributor

gschorcht commented Nov 2, 2022

@gschorcht may I ask you for a quick look, if your ACK still applies?

Sorry for the delay, I was quite busy by other tasks the last days. The ACK still applies, the last change is semantically the same.

@gschorcht gschorcht merged commit ccbb304 into RIOT-OS:master Nov 2, 2022
@jue89 jue89 deleted the fix/efm32_series2_timer branch November 2, 2022 16:37
@jue89
Copy link
Contributor Author

jue89 commented Nov 2, 2022

Thank you for reviewing! :-)

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants