sam0_common: make RTC implementation common across all sam0 MCUs #11317
Conversation
benpicco
force-pushed
the
sam0-rtc
branch
2 times, most recently
from
050d76b to
adf611f
Compare
cpu/sam0_common/periph/rtc.c
Outdated
| #else | ||
| RTC->MODE2.CTRL.bit.ENABLE = 0; | ||
| #endif | ||
| #ifdef REG_RTC_MODE2_SYNCBUSY |
There was a problem hiding this comment.
I would replace these #ifdef REG_RTC_MODE2_SYNCBUSY block by a static or static inline function here. Easier to read and more compact.
cpu/sam0_common/periph/rtc.c
Outdated
| /* DISABLE RTC MASTER */ | ||
| rtc_poweroff(); | ||
|
|
||
| rtc_clock_setup(); |
There was a problem hiding this comment.
Please call rtc_clock_setup(); before rtc_poweroff();
SAML10-XPRO get stuck within rtc_poweroff(); which is a normal behavior since we're waiting for a bit but the IP is not clocked.
Strangely, it works so far for SAMD21, SAML21 and SAML11. Atmel black magic ?
There was a problem hiding this comment.
Thank you for testing!
btw, do we need to call rtc_poweroff() and rtc_reset() here at all?
e.g. the RTC keeps ticking in HIBERNATE / BACKUP deep sleep mode and can wake the CPU (which triggers a reset).
I was wondering why the RTC was still starting at 0 after that - only to discover that it is being reset in rtc_init()!
Is this the intended behavior? If not, I'd much rather just remove those calls here.
There was a problem hiding this comment.
@benpicco I have no strong opinion about reset yet. But rtc_poweroff() is mandatary for at least SAML21.
CTRLA register at least is "Enable-Protected" which means we cannot modify it after the ENABLE bit has beent set. This is why we must use rtc_poweroff(); before configuring our rtc.
Try to run the current code on SAML21, it should hang.
There was a problem hiding this comment.
Ok, I've changed the call order and made the RTC persist across reboots.
I might as well remove rtc_reset() then as it's currently unused.
Only tested on the samr21 as I don't have a saml21.
dylad
added
Reviewed: 1-fundamentals
| while (RTC->MODE2.CTRLA.bit.SWRST); | ||
| void rtc_init(void) | ||
| { | ||
| rtc_poweron(); |
There was a problem hiding this comment.
I don't think it's useful to enable it right know as we will disable it a few line after.
I would say to power_off at the beginning of rtc_init and power_on at the end.
There was a problem hiding this comment.
Well you mentioned it is necessary for the device to be powered for rtc_clock_setup() to work on saml10.
Nothing should have enabled the RTC before this function is called, so it would stall again.
There was a problem hiding this comment.
Well you mentioned it is necessary for the device to be powered for rtc_clock_setup() to work on saml10.
I said to call rtc_clock_setup() before rtc_poweroff().
In fact, the current code still hangs for SAML10 because we're stuck in wait_syncbusy. Enable or disable RTC has no effect because RTC IP is not feed by the required clock so the syncbusy bit will not change. Thus, rtc_poweron() is not needed here and we should setup the clock in priority. I'm sorry if I wasn't clear enough I hope my explanations get better here.
In fact, we should Setup Clock -> Disable RTC module (because of Enabled Protected regs) -> Init RTC -> Enable RTC module
There was a problem hiding this comment.
Ok, let's try it again then. I think having the poweron/poweroff functions wrap the ENABLE bit is a bit confusing, I've wrapped them in a separate function now and made the poweron/poweroff functions actually disable the peripheral in PM/MCLK like it is done in the RTT driver.
cpu/sam0_common/periph/rtc.c
Outdated
| @@ -41,13 +43,50 @@ static rtc_state_t rtc_callback; | |||
| * Thanks to this, the user will be able to set time in 2000's*/ | |||
| static uint16_t reference_year = 100; | |||
|
|
|||
| void rtc_init(void) | |||
| static void wait_syncbusy(void) | |||
There was a problem hiding this comment.
static function should be named with _ prefix
_wait_syncbusy()
This is the same for other static functions within this page.
There was a problem hiding this comment.
See our coding convention if something is unclear to you
|
@benpicco New changes work fine on arduino-zero but I have issue with SAML10 / SAML11 and SAML21. |
The currently supported SAM0 MCUs (samd21, saml21, saml1x) share the same RTC peripheral, yet each of them carries it's own copy of the RTC driver. Unify the drivers and move them to sam0_common.
dylad
added
CI: ready for build
|
here we go. |
Contribution description
The currently supported SAM0 MCUs (samd21, saml21, saml1x) share the same RTC peripheral, yet each of them carries it's own copy of the RTC driver.
Unify the drivers and move them to sam0_common.
Testing procedure
Only tested it on samr21-xpro with the
defaultexample andrtc settime&rtc setalarm.Issues/PRs references
#11305 will also use this implementation.