boards/sam[r/d]21-xpro: prefer XOSC32K for RTC/RTT (GCLK2) by fjmolinas · Pull Request #13306 · RIOT-OS/RIOT

fjmolinas · 2020-02-06T14:31:39Z

Contribution description

For samr21-xpro, GCLK2 is used to clock RTC and RTT. By default OSCULP32K is preferred to clock GCLK2. On other sam0 boards the actual clock source can be chosen independent of GCLK but not for samr.

OSCULP32K needs calibration and although:

OSCULP32K.CALIB is automatically loaded from Flash Factory Calibration during startup, and is used to compensate for process variation, as described in the Electrical Characteristics. The calibration value can be overridden by the user by writing to OSCULP32K.CALIB.

from practical use the clock still needs calibration leading to drifts in RTC and RTT, samr21-xpro has an external oscillator with a precise value out of the box, so this PR changes the current configuration to prefer that clock as GCLK2 32.768Khz clock source.

Testing procedure

Look at the times of each print, this can optionally be verified with an oscilloscope (I did it).

on master:

2020-02-06 15:28:53,357 # START
2020-02-06 15:28:53,358 # 
2020-02-06 15:28:53,360 # RIOT RTT low-level driver test
2020-02-06 15:28:53,364 # This test will display 'Hello' every 5 seconds
2020-02-06 15:28:53,364 # 
2020-02-06 15:28:53,367 # Initializing the RTT driver
2020-02-06 15:28:53,368 # RTT now: 2
2020-02-06 15:28:53,372 # Setting initial alarm to now + 5 s (163842)
2020-02-06 15:28:53,376 # Done setting up the RTT, wait for many Hellos
2020-02-06 15:28:58,352 # Hello
2020-02-06 15:29:03,337 # Hello
2020-02-06 15:29:08,321 # Hello
2020-02-06 15:29:13,306 # Hello
2020-02-06 15:29:18,290 # Hello

on master using calibration, actual frequency of 32867Khz:

2020-02-06 15:30:12,951 # START
2020-02-06 15:30:12,951 # 
2020-02-06 15:30:12,954 # RIOT RTT low-level driver test
2020-02-06 15:30:12,958 # This test will display 'Hello' every 5 seconds
2020-02-06 15:30:12,958 # 
2020-02-06 15:30:12,960 # Initializing the RTT driver
2020-02-06 15:30:12,962 # RTT now: 2
2020-02-06 15:30:12,966 # Setting initial alarm to now + 5 s (164337)
2020-02-06 15:30:12,970 # Done setting up the RTT, wait for many Hellos
2020-02-06 15:30:17,961 # Hello
2020-02-06 15:30:22,960 # Hello
2020-02-06 15:30:27,960 # Hello
2020-02-06 15:30:32,960 # Hello
2020-02-06 15:30:37,959 # Hello
2020-02-06 15:30:42,959 # Hello
2020-02-06 15:30:47,958 # Hello

this pr:

2020-02-06 15:27:47,102 # RIOT RTT low-level driver test
2020-02-06 15:27:47,106 # This test will display 'Hello' every 5 seconds
2020-02-06 15:27:47,106 # 
2020-02-06 15:27:47,108 # Initializing the RTT driver
2020-02-06 15:27:47,110 # RTT now: 2
2020-02-06 15:27:47,117 # Setting initial alarm to now + 5 s (163842)
2020-02-06 15:27:47,118 # Done setting up the RTT, wait for many Hellos
2020-02-06 15:27:52,109 # Hello
2020-02-06 15:27:57,109 # Hello
2020-02-06 15:28:02,109 # Hello
2020-02-06 15:28:07,109 # Hello

fjmolinas

ups

boards/samr21-xpro/include/periph_conf.h

benpicco

Thank you, I noticed that too.
The RTC/RTT would drift when not using the external oscillator.

I find it a bit weird that we signal the availability of an external oscillator by setting GEN2_ULP32K to 0, but that is for another PR.

fjmolinas · 2020-02-06T14:42:34Z

I find it a bit weird that we signal the availability of an external oscillator by setting GEN2_ULP32K to 0, but that is for another PR.

I can change the variable name, but that means changing more code, and I wanted to get input from people using these cpu more like @benpicco and @dylad.

benpicco · 2020-02-06T14:46:14Z

Yes changing the variable name should be a separate PR.
I manually had to set GEN2_ULP32K to 0 for my at86rf215 range test that was RTT synced and would drift too much without this. (I connected the new radio module to samr21-xpros for convenience)

I wanted to PR this too, but was puzzled why it was explicitly set to 1 in the board config.

dylad · 2020-02-06T14:48:09Z

I agree that external oscillator should be used rather than the ULP32K by default.

benpicco · 2020-02-06T14:50:54Z

Do samd21-xpro and hamilton have an external oscillator? They also define GEN2_ULP32K to 1, but I would expect the -xpro board to have one.

fjmolinas · 2020-02-06T14:52:13Z

Do samd21-xpro and hamilton have an external oscillator? They also define GEN2_ULP32K to 1, but I would expect the -xpro board to have one.

atmega256rfr2-xpro does.

fjmolinas · 2020-02-06T14:52:31Z

atmega256rfr2-xpro does.

Not the same cpu, but maybe all xpro do?

benpicco · 2020-02-06T14:52:58Z

I see a big oscillator right on the board when I look at pictures of it.

dylad · 2020-02-06T14:53:40Z

samd21-xpro has an external 32khz for sure, dunno about hamilton.

dylad · 2020-02-06T14:54:28Z

For the record:
http://ww1.microchip.com/downloads/en/devicedoc/atmel-42220-samd21-xplained-pro_user-guide.pdf
page 14.

benpicco · 2020-02-06T15:01:17Z

I think ham ilt on also got it wrong:

#elif CLOCK_USE_XOSC32_DFLL
/* Settings for 32 kHz external oscillator and 48 MHz DFLL */
#define CLOCK_CORECLOCK     (48000000U)
#define CLOCK_XOSC32K       (32768UL)
#define CLOCK_8MHZ          (1)
#define GEN2_ULP32K         (1)

From the comments I would assume there to be an external oscillator (and setting GEN2_ULP32K is just the think everyone does if there is an external oscillator).

dylad · 2020-02-06T15:02:11Z

Ok asking the real question:
Should we deprecate (or remove) the Hamilton board ?
I don't know if any maintainers have access to one and I don't find any site to buy it. What do you think ?

fjmolinas · 2020-02-06T15:03:24Z

Ok asking the real question:
Should we deprecate (or remove) the Hamilton board ?
I don't know if any maintainers have access to one and I don't find any site to buy it. What do you think ?

I have one, but find it very painful to use stdio_rtt so I haven't really used it.

fjmolinas · 2020-02-06T15:06:02Z

@Hyungsin you contributed the BOARD, maybe you know if it has an external oscilator?

benpicco · 2020-02-06T15:11:30Z

The patch applies cleanly to samd21-xpro and hamilton too, so I'd say it was just copy & paste.

samd21-xpro.patch

From e522739706f384465756fae4e5188b392a68d1b2 Mon Sep 17 00:00:00 2001
From: Francisco Molina <[email protected]>
Date: Thu, 6 Feb 2020 15:20:08 +0100
Subject: [PATCH] boards/samd21-xpro/include/periph_conf: use XOSC32K

---
 boards/samd21-xpro/include/periph_conf.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/boards/samd21-xpro/include/periph_conf.h b/boards/samd21-xpro/include/periph_conf.h
index 9bfcb97d656d..4bab39837322 100644
--- a/boards/samd21-xpro/include/periph_conf.h
+++ b/boards/samd21-xpro/include/periph_conf.h
@@ -63,7 +63,16 @@ extern "C" {
  *
  * @{
  */
-#define CLOCK_USE_PLL       (1)
+#define CLOCK_USE_PLL               (1)
+#define CLOCK_USE_XOSC32_DFLL       (0)
+/*
+ * 0: use XOSC32K (always 32.768kHz) to clock GCLK2
+ * 1: use OSCULP32K factory calibrated (~32.768kHz) to clock GCLK2
+ *
+ * OSCULP32K is factory calibrated to be around 32.768kHz but this values can
+ * be of by a couple of % points, so prefer XOSC32K as default configuration.
+ */
+#define GEN2_ULP32K                 (0)
 
 #if CLOCK_USE_PLL
 /* edit these values to adjust the PLL output frequency */
@@ -75,8 +84,6 @@ extern "C" {
 /* Settings for 32 kHz external oscillator and 48 MHz DFLL */
 #define CLOCK_CORECLOCK     (48000000U)
 #define CLOCK_XOSC32K       (32768UL)
-#define CLOCK_8MHZ          (1)
-#define GEN2_ULP32K         (1)
 #else
 /* edit this value to your needs */
 #define CLOCK_DIV           (1U)

hamilton.patch

From e522739706f384465756fae4e5188b392a68d1b2 Mon Sep 17 00:00:00 2001
From: Francisco Molina <[email protected]>
Date: Thu, 6 Feb 2020 15:20:08 +0100
Subject: [PATCH] boards/hamilton/include/periph_conf: use XOSC32K

---
 boards/hamilton/include/periph_conf.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/boards/hamilton/include/periph_conf.h b/boards/hamilton/include/periph_conf.h
index 9bfcb97d656d..4bab39837322 100644
--- a/boards/hamilton/include/periph_conf.h
+++ b/boards/hamilton/include/periph_conf.h
@@ -63,7 +63,16 @@ extern "C" {
  *
  * @{
  */
-#define CLOCK_USE_PLL       (1)
+#define CLOCK_USE_PLL               (1)
+#define CLOCK_USE_XOSC32_DFLL       (0)
+/*
+ * 0: use XOSC32K (always 32.768kHz) to clock GCLK2
+ * 1: use OSCULP32K factory calibrated (~32.768kHz) to clock GCLK2
+ *
+ * OSCULP32K is factory calibrated to be around 32.768kHz but this values can
+ * be of by a couple of % points, so prefer XOSC32K as default configuration.
+ */
+#define GEN2_ULP32K                 (0)
 
 #if CLOCK_USE_PLL
 /* edit these values to adjust the PLL output frequency */
@@ -77,8 +86,6 @@ extern "C" {
 /* Settings for 32 kHz external oscillator and 48 MHz DFLL */
 #define CLOCK_CORECLOCK     (48000000U)
 #define CLOCK_XOSC32K       (32768UL)
-#define CLOCK_8MHZ          (1)
-#define GEN2_ULP32K         (1)
 #else
 /* edit this value to your needs */
 #define CLOCK_DIV           (1U)

fjmolinas · 2020-02-06T15:33:24Z

@benpicco done, does you ACK still apply?

benpicco · 2020-02-06T15:46:36Z

ACK is still valid.

Hyungsin · 2020-02-06T17:56:53Z

maybe you know if it has an external oscilator?

No external oscillator on hamilton

fjmolinas · 2020-02-07T15:13:56Z

Do we really have to run the WDT off the internal oscillator? When I did a manual test the watchdog would work as expected, not sure how the test script arrived at a negative value.

Running from XOSC32K currently uses a long start up time, I'm guessing that is the issue. It should be fixed now.

multiple changes

fjmolinas · 2020-02-07T15:15:56Z

cpu/samd21/cpu.c

    while (!(SYSCTRL->PCLKSR.reg & SYSCTRL_PCLKSR_OSC8MRDY)) {}
 #endif

+#if CLOCK_USE_XOSC32_DFLL || !GEN2_ULP32K || !GEN3_ULP32K


In a subsequent PR this can be easily replaced by a CLOCK_USE_XOSC32K or similar

fjmolinas · 2020-02-07T15:18:21Z

In the datasheet: CLK_WDT is intended to be sourced from the clock of the internal ultra-low-power (ULP) oscillator., so I would leave the default configuration for now, but now it can be set by the user.

fjmolinas · 2020-02-07T15:19:37Z

cpu/samd21/cpu.c

+ * @note    Override this value in your boards periph_conf.h file
+ *          if a different start up time is to be used.
+ */
+#define XOSC32_STARTUP_TIME      6


This does not need to be 6 for RTT, but maybe for DFLL it does? I didn't change it because I didn't understand the implications, but at least made it configurable.

benpicco · 2020-02-07T15:30:12Z

Thank you for the cleanup!

benpicco · 2020-02-07T17:35:50Z

run_test/examples/micropython/samr21-xpro:gnu and run_test/examples/suit_update/nrf52dk:gnu are failing?!
I'm not buying that - let's give it another go.

benpicco · 2020-02-11T09:38:24Z

@kaspar030 I somehow doubt a change in samd21 code is causing a test failure on nrf52 and esp32.

Code works locally, let's ignore those tests.

fjmolinas · 2020-02-11T13:55:08Z

All green @benpicco

benpicco

Good improvement to the accuracy of the RTC/RTT.
Tested on samr21-xpro.

kaspar030 · 2020-02-21T10:26:28Z

git bisect tells me this broke examples/micropython. Could you take a look?

dylad · 2020-02-21T10:30:45Z

@kaspar030 could you elaborate a bit on it ? Is there any link to the CI or something ?

benpicco · 2020-02-21T10:36:36Z

Looks like startup is taking longer now:

2020-02-21 11:34:31,574 # NameError: name 'ps' isn't defined // I was just bashing the enter key while pressing the reset button
2020-02-21 11:34:34,022 #  main(): This is RIOT! (Version: 2020.04-devel-539-g3e7dd)
2020-02-21 11:34:34,022 # -- Executing boot.py
2020-02-21 11:34:34,031 # boot.py: MicroPython says hello!

~2.5s to boot

kaspar030 · 2020-02-21T10:44:49Z

I was investigating this nightly error: https://ci.riot-os.org/RIOT-OS/RIOT/master/86fcc359947d3647a8d924533ab603177b6f509d/output/run_test/examples/micropython/samr21-xpro:gnu.txt

I did a git bisect testing something like BOARD=samr21-xpro make -C examples/micropython clean all test -j8.

kaspar030 · 2020-02-21T10:46:58Z

~2.5s to boot

Could be that the test needs to be syncronized, and as is just fails because it reboots in the middle of the test.

fjmolinas added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: boards Area: Board ports labels Feb 6, 2020

fjmolinas requested review from aabadie and dylad February 6, 2020 14:31

fjmolinas commented Feb 6, 2020

View reviewed changes

boards/samr21-xpro/include/periph_conf.h Outdated Show resolved Hide resolved

boards/samr21-xpro/include/periph_conf.h Outdated Show resolved Hide resolved

benpicco previously approved these changes Feb 6, 2020

View reviewed changes

fjmolinas force-pushed the pr_samr21_use_xosc branch from 08be875 to e522739 Compare February 6, 2020 14:51

fjmolinas force-pushed the pr_samr21_use_xosc branch from e522739 to 429c250 Compare February 6, 2020 15:32

fjmolinas changed the title ~~boards/samr21-xpro/include/periph_conf: use XOSC32K~~ boards: prefer XOSC32K for RT% (GCLK2) on samd21 boards Feb 6, 2020

fjmolinas commented Feb 7, 2020

View reviewed changes

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 7, 2020

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 7, 2020

benpicco approved these changes Feb 11, 2020

View reviewed changes

benpicco merged commit 33291ad into RIOT-OS:master Feb 11, 2020

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

This was referenced Feb 21, 2020

cpu/esp*: code deduplication and cleanup #12955

Merged

cpu/samd21: fix long startup times #13498

Merged

fjmolinas deleted the pr_samr21_use_xosc branch March 23, 2020 15:56

Conversation

fjmolinas commented Feb 6, 2020

Contribution description

Testing procedure

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylad commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020

Uh oh!

dylad commented Feb 6, 2020

Uh oh!

dylad commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020

Uh oh!

dylad commented Feb 6, 2020

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Feb 6, 2020

Uh oh!

benpicco commented Feb 6, 2020

Uh oh!

Hyungsin commented Feb 6, 2020

Uh oh!

fjmolinas commented Feb 7, 2020

Uh oh!

fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 7, 2020

Uh oh!

fjmolinas Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 7, 2020

Uh oh!

benpicco commented Feb 7, 2020

Uh oh!

benpicco commented Feb 11, 2020

Uh oh!

fjmolinas commented Feb 11, 2020

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Feb 21, 2020

Uh oh!

dylad commented Feb 21, 2020

Uh oh!

benpicco commented Feb 21, 2020

Uh oh!

kaspar030 commented Feb 21, 2020

Uh oh!

kaspar030 commented Feb 21, 2020

Uh oh!

benpicco commented Feb 6, 2020 •

edited

Loading

benpicco commented Feb 6, 2020 •

edited

Loading