drivers/periph: define rtc_mem and implement it for sam0_common #16758

benpicco · 2021-08-19T16:57:40Z

Contribution description

The RTC often provides some memory that can be used to store data across reset / deep sleep.
Expose this memory with a proper API.

On SAM D5x/E5x general purpose registers are shared with the alarm registers.
Since the RTC/RTT interface only exposes a single alarm, we can only use the second pair of General Purpose Registers.

The data sheet for SAM L1x and SAM L2x do not mention such limitations, but according to the data sheet SAM L21 only has 2 General Purpose registers whereas the vendor files have definitions for 4. I didn't yet test if they do indeed all work.

Testing procedure

So far this only has been tested on same54-xpro. Both tests/periph_rtc and tests/periph_rtt have been adapted to make use of the feature and to ensure it does not interfere with normal RTC/RTT alarms.

samd5x
saml21
~~saml1x~~ single set of shared Alarm / GP registers

Issues/PRs references

alternative to #16757 - @ant9000 does this work for your use-case?
previously proposed in #11369

kaspar030 · 2021-08-20T07:16:59Z

PR looks fine!

So you decided making the API suitable for external nvrams is not a good idea?

bikeshedding: maybe "rtc" in the name is limiting, as in, this feature might not be implemented by RTC hardware?

benpicco · 2021-08-20T08:51:21Z

So you decided making the API suitable for external nvrams is not a good idea?

Well we have this with all APIs that are usually implemented on the MCU, but could also be provided via software emulation or external hardware.

We would need a higher level API that sits on top of the periph API / external driver API or switch to a pointer based API interface.

But this is beyond the scope of this PR.

maybe "rtc" in the name is limiting, as in, this feature might not be implemented by RTC hardware?

Maybe, but what's a better name? RTC memory has some characteristics that are different from other types of memory.

it's very small, e.g. here only 8 bytes are usable
it's not memory-mapped like backup_ram

What other implementations are you thinking of?

fjmolinas · 2021-08-23T12:25:43Z

@benpicco are you aware of other hardware with this kind of feature or only SAM so far?

ant9000 · 2021-08-23T12:33:42Z

The data sheet for SAM L1x and SAM L2x do not mention such limitations, but according to the data sheet SAM L21 only has 2 General Purpose registers whereas the vendor files have definitions for 4. I didn't yet test if they do indeed all work.

I've tested with a SAMR34 chip, which consists of a SAML21 CPU (rev B) plus a SX1276 radio. I can read and write all 4 registers without problems.

* [ ]  saml21

Your RTC test passes and prints "RTC mem OK" on saml21 (well, samr34 as said above).

Iv'e tried compiling one of my firmwares (that makes use of ztimer module) against this PR. Without adding module periph_rtc_mem I get the following error:

RIOT/cpu/sam0_common/periph/rtc_rtt.c: In function 'rtt_init':
RIOT/cpu/sam0_common/periph/rtc_rtt.c:329:21: error: 'RTC_GPR_NUM_AVAIL' undeclared (first use in this function); did you mean 'RTC_GPR_NUM'?
     uint32_t backup[RTC_GPR_NUM_AVAIL];
                     ^~~~~~~~~~~~~~~~~
                     RTC_GPR_NUM

~~If I add your new module, I can flash the board but initialization fails somewhere - I don't see the standard RIOT banner.~~
(it was my fault, of course: if I add periph_rtc_mem everything works fine).

alternative to #16757 - @ant9000 does this work for your use-case?

~~The API would perfectly solve my problem - I just need to figure out why my own code breaks just by compiling against it (without further changes).~~

This API indeed solves my problem.

A small note: you're intercepting rtt_init(), but the RTC memory is cleared also by calling rtc_init(). To keep things more readable, I'd rather save/restore the bytes in _ rtt_reset(), so you get all the possible paths at once. What do you think?

Another little glitch: in rtc_mem_read and rtc_mem_write the asserts should really be

assert(offset + len <= RTC_MEM_SIZE);

in order to fully use the available memory.

benpicco · 2021-08-23T14:15:25Z

I expanded the test so now the whole RTC memory is read & written - before I only write the first two words.
Would be interesting if it still works on saml21.

If I add your new module, I can flash the board but initialization fails somewhere - I don't see the standard RIOT banner.
(it was my fault, of course: if I add periph_rtc_mem everything works fine).

Nah that was my fault as well - the driver should still work without periph_rtc_mem 😉

A small note: you're intercepting rtt_init(), but the RTC memory is cleared also by calling rtc_init(). To keep things more readable, I'd rather save/restore the bytes in _ rtt_reset(), so you get all the possible paths at once. What do you think?

The rtt_reset() in rtc_init() is only called if the RTC was not previously in RTC mode, e.g. when a different firmware was flashed or the device was not connected to a power source before.

Another little glitch: in rtc_mem_read and rtc_mem_write the asserts should really be
assert(offset + len <= RTC_MEM_SIZE);
in order to fully use the available memory.

Thank you, fixed!

ant9000 · 2021-08-23T14:49:38Z

I expanded the test so now the whole RTC memory is read & written - before I only write the first two words.
Would be interesting if it still works on saml21.

I can confirm that on SAMR34 your code works (it would on SAML21 too, then).

dylad · 2021-08-31T18:42:38Z

@ant9000 @benpicco
Which PRs should be in ?
this one or #16757 ?

benpicco · 2021-09-01T10:17:15Z

This one should provide a more general solution to the problem that #16757 was set to solve

dylad · 2021-09-02T18:12:22Z

As always, SAML1X complain.

2021-09-02 20:11:35,316 # Alarm!
2021-09-02 20:11:37,316 # Alarm!
2021-09-02 20:11:39,316 # Alarm!
2021-09-02 20:11:41,316 # Alarm!
2021-09-02 20:11:41,321 # RTC mem content does not match
2021-09-02 20:11:41,322 # 52 - 00
2021-09-02 20:11:41,323 # 49 - 00
2021-09-02 20:11:41,323 # 4f - 00
2021-09-02 20:11:41,324 # 54 - 00

dylad · 2021-09-02T18:18:01Z

SAME54-XPRO also complains.

benpicco · 2021-09-02T18:19:25Z

Ah SAM L1x has the same RTC peripheral as SAM D5x (GP and Alarm registers are shared), but there is no second set of GP/Alarm registers. The RTC only supports a single alarm.

Since we use that alarm already, we can't use the GP register to store data.

benpicco · 2021-09-02T18:24:18Z

SAME54-XPRO also complains.

Mine says it's fine

2021-09-02 20:23:24,414 # main(): This is RIOT! (Version: 2021.10-devel-536-g3dba1-periph/rtc_mem)
2021-09-02 20:23:24,414 # 
2021-09-02 20:23:24,417 # RIOT RTC low-level driver test
2021-09-02 20:23:24,422 # This test will display 'Alarm!' every 2 seconds for 4 times
2021-09-02 20:23:24,453 # RTC mem OK
2021-09-02 20:23:24,457 #   Setting clock to   2020-02-28 23:59:57
2021-09-02 20:23:24,463 # Clock value is now   2020-02-28 23:59:57
2021-09-02 20:23:24,466 #   Setting alarm to   2020-02-28 23:59:59
2021-09-02 20:23:24,473 #    Alarm is set to   2020-02-28 23:59:59
2021-09-02 20:23:24,476 #   Alarm cleared at   2020-02-28 23:59:57
2021-09-02 20:23:26,480 #        No alarm at   2020-02-28 23:59:59
2021-09-02 20:23:26,486 #   Setting alarm to   2020-02-29 00:00:01
2021-09-02 20:23:26,486 # 
2021-09-02 20:23:29,427 # Alarm!
2021-09-02 20:23:31,427 # Alarm!
2021-09-02 20:23:33,427 # Alarm!
2021-09-02 20:23:35,427 # Alarm!
2021-09-02 20:23:35,430 # RTC mem OK

dylad · 2021-09-02T18:26:14Z

On my side, SAME54-XPRO returns:

2021-09-02 20:25:19,875 # Help: Press s to start test, r to print it is ready
s
2021-09-02 20:25:22,168 # START
2021-09-02 20:25:22,176 # main(): This is RIOT! (Version: 2021.04-devel-2400-g3dba1-periph/rtc_mem)
2021-09-02 20:25:22,177 # 
2021-09-02 20:25:22,178 # RIOT RTC low-level driver test
2021-09-02 20:25:22,182 # This test will display 'Alarm!' every 2 seconds for 4 times
2021-09-02 20:25:22,188 # RTC mem content does not match
2021-09-02 20:25:22,189 # 52 - 00
2021-09-02 20:25:22,189 # 49 - 00
2021-09-02 20:25:22,190 # 4f - 00
2021-09-02 20:25:22,192 # 54 - 00
2021-09-02 20:25:22,194 #   Setting clock to   2020-02-28 23:59:57
2021-09-02 20:25:22,201 # Clock value is now   2020-02-28 23:59:57
2021-09-02 20:25:22,204 #   Setting alarm to   2020-02-28 23:59:59
2021-09-02 20:25:22,210 #    Alarm is set to   2020-02-28 23:59:59
2021-09-02 20:25:22,214 #   Alarm cleared at   2020-02-28 23:59:57
2021-09-02 20:25:24,218 #        No alarm at   2020-02-28 23:59:59
2021-09-02 20:25:24,224 #   Setting alarm to   2020-02-29 00:00:01
2021-09-02 20:25:24,225 # 
2021-09-02 20:25:27,101 # Alarm!
2021-09-02 20:25:29,101 # Alarm!
2021-09-02 20:25:31,101 # Alarm!
2021-09-02 20:25:33,101 # Alarm!
2021-09-02 20:25:33,106 # RTC mem content does not match
2021-09-02 20:25:33,107 # 52 - 00
2021-09-02 20:25:33,108 # 49 - 00
2021-09-02 20:25:33,109 # 4f - 00
2021-09-02 20:25:33,110 # 54 - 00

dylad · 2021-09-02T18:30:49Z

Just unplug/replug and seems to work now...

dylad · 2021-09-02T18:34:34Z

Why did you disable support for SAML1X in latest commit ?

benpicco · 2021-09-02T18:35:09Z

It can't be used together with RTC / RTT alarms as there is only a single set of GP/Alarm registers

dylad · 2021-09-02T18:58:19Z

Please squash !

fjmolinas · 2021-09-03T07:32:52Z

drivers/include/periph/rtc_mem.h

+#ifdef __cplusplus
+extern "C" {
+#endif
+


Do we need to tie this to rtc? For stm32 this could be used regardless of the rtc.

If STM32 have persistent GP registers across reset we can extend and move this interface somewhere later.

dylad

ACK.
Tested work on SAME54 and SAML21.
Now that SAML1X is out, let's merge this.

fjmolinas · 2021-09-03T07:36:36Z

drivers/include/periph/rtc_mem.h

+/**
+ * @brief Write to RTC memory
+ *
+ * @param[in] offset    Offset to the start of RTC memory in bytes
+ * @param[in] data      Source buffer
+ * @param[in] len       Amount of bytes to write
+ */
+void rtc_mem_write(unsigned offset, const void *data, size_t len);


Can we make the behaviour explicit when attempts to write out of bounds? Will it write what it can or fail?

I added a @note

fjmolinas

Some comments on the api, see above

tests/periph_rtc/main.c

miri64 · 2021-09-03T09:38:10Z

See #16803 for why I removed the CI: ready for build label.

fjmolinas

ACK, changes addressed

benpicco requested review from aabadie, cgundogan, dylad, fjmolinas, jia200x, keestux, leandrolanzieri, MichelRottleuthner, MrKevinWeiss and smlng as code owners August 19, 2021 16:57

github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Aug 19, 2021

benpicco added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 19, 2021

benpicco requested a review from kaspar030 August 19, 2021 16:58

benpicco changed the title ~~Periph/rtc mem~~ drivers/periph: define rtc_mem and implement it for sam0_common Aug 19, 2021

benpicco force-pushed the periph/rtc_mem branch from eb05241 to f1b57cf Compare August 19, 2021 17:02

dylad self-assigned this Aug 31, 2021

dylad added this to the Release 2021.10 milestone Aug 31, 2021

benpicco force-pushed the periph/rtc_mem branch from 84633dd to c40b1c4 Compare September 2, 2021 09:45

benpicco force-pushed the periph/rtc_mem branch from 3dba105 to 8e5cd31 Compare September 2, 2021 18:59

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 2, 2021

fjmolinas reviewed Sep 3, 2021

View reviewed changes

dylad approved these changes Sep 3, 2021

View reviewed changes

fjmolinas reviewed Sep 3, 2021

View reviewed changes

fjmolinas requested changes Sep 3, 2021

View reviewed changes

fjmolinas reviewed Sep 3, 2021

View reviewed changes

tests/periph_rtc/main.c Outdated Show resolved Hide resolved

fjmolinas mentioned this pull request Sep 3, 2021

cpu/stm32: add rtc_mem #16802

Merged

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 3, 2021

fjmolinas approved these changes Sep 3, 2021

View reviewed changes

benpicco added 4 commits September 3, 2021 11:43

drivers/periph: define periph_rtc_mem

0ee0c5e

cpu/sam0_common: implement periph_rtc_mem

3fbf473

tests/periph_rtt: test for periph_rtc_mem

c467f05

tests/periph_rtc: test for periph_rtc_mem

c8d94e8

benpicco force-pushed the periph/rtc_mem branch from 842732f to c8d94e8 Compare September 3, 2021 09:43

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 3, 2021

benpicco merged commit 3b30432 into RIOT-OS:master Sep 3, 2021

benpicco deleted the periph/rtc_mem branch September 3, 2021 12:59

benpicco mentioned this pull request Sep 14, 2021

ieee802154_security: Nonce is reused after reboot #16844

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/periph: define rtc_mem and implement it for sam0_common #16758

drivers/periph: define rtc_mem and implement it for sam0_common #16758

benpicco commented Aug 19, 2021 •

edited

Loading

kaspar030 commented Aug 20, 2021

benpicco commented Aug 20, 2021

fjmolinas commented Aug 23, 2021

ant9000 commented Aug 23, 2021 •

edited

Loading

benpicco commented Aug 23, 2021 •

edited

Loading

ant9000 commented Aug 23, 2021

dylad commented Aug 31, 2021

benpicco commented Sep 1, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

benpicco commented Sep 2, 2021

benpicco commented Sep 2, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

benpicco commented Sep 2, 2021

dylad commented Sep 2, 2021

fjmolinas Sep 3, 2021

dylad Sep 3, 2021

dylad left a comment

fjmolinas Sep 3, 2021

benpicco Sep 3, 2021

fjmolinas left a comment

miri64 commented Sep 3, 2021

fjmolinas left a comment

drivers/periph: define rtc_mem and implement it for sam0_common #16758

drivers/periph: define rtc_mem and implement it for sam0_common #16758

Conversation

benpicco commented Aug 19, 2021 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

kaspar030 commented Aug 20, 2021

benpicco commented Aug 20, 2021

fjmolinas commented Aug 23, 2021

ant9000 commented Aug 23, 2021 • edited Loading

benpicco commented Aug 23, 2021 • edited Loading

ant9000 commented Aug 23, 2021

dylad commented Aug 31, 2021

benpicco commented Sep 1, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

benpicco commented Sep 2, 2021

benpicco commented Sep 2, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

dylad commented Sep 2, 2021

benpicco commented Sep 2, 2021

dylad commented Sep 2, 2021

fjmolinas Sep 3, 2021

Choose a reason for hiding this comment

dylad Sep 3, 2021

Choose a reason for hiding this comment

dylad left a comment

Choose a reason for hiding this comment

fjmolinas Sep 3, 2021

Choose a reason for hiding this comment

benpicco Sep 3, 2021

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

miri64 commented Sep 3, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

benpicco commented Aug 19, 2021 •

edited

Loading

ant9000 commented Aug 23, 2021 •

edited

Loading

benpicco commented Aug 23, 2021 •

edited

Loading