cpu/saml21: add interaction with pm_layered for peripheral drivers #18821
Conversation
jue89
added
Type: enhancement
github-actions
bot
added
Area: sys
|
@benpicco I touched your |
jue89
added
the
CI: ready for build
Murdock results✔️ PASSED 73e6886 cpu/saml21: define required power modes
ArtifactsThis 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. |
|
Will test! btw, I don't think DMA transfers are a problem as the API is still blocking. |
The thread calling |
|
SPI in DMA mode is done, as well. Maybe it's a good idea to split off the |
jue89
force-pushed
the
feature/saml21_pheriph_pm
branch
from
70dc352
to
d564e1b
Compare
jue89
added
State: waiting for other PR
jue89
requested review from
kaspar030,
dylad,
biboc,
benpicco and
keestux
as code owners
benpicco
removed
the
State: waiting for other PR
jue89
force-pushed
the
feature/saml21_pheriph_pm
branch
from
d564e1b
to
04a0bf8
Compare
|
I'd say this one ready to be reviewed. #18825 is merged and I rebased this one on master. One question: When to block IRQs? I'm not sure. Most periph driver don't block any IRQs and trust they are used by just one thread. But I don't know if this assumption is correct. Especially the gpio driver might be used by several threads. |
jue89
removed
the
CI: ready for build
|
Shall we block this PR until the discussion @kfessel brought up in #17607 is resolved? If this gets merged, Just to recap:
|
jue89
added
the
CI: ready for build
| * @{ | ||
| */ | ||
| #define SAM0_GPIO_PM_BLOCK SAML21_PM_MODE_BACKUP /**< GPIO IRQs require STANDBY mode */ | ||
| #define SAM0_RTCRTT_PM_BLOCK SAML21_PM_MODE_BACKUP /**< RTC/TRR require STANDBY mode */ |
There was a problem hiding this comment.
I'm pretty sure the RTT/RTC is always on as it can be used to wake from BACKUP sleep
The Backup Power Domain (PDBACKUP) is always on, except in the off sleep mode. It contains the 32KHz oscillator sources, the Supply Controller, the Reset Controller, the Real Time Counter, and the Power Manager itself.
There was a problem hiding this comment.
Yes, right. But we're loosing context. Callback functions are never called, then ;-)
Maybe we can work with two cases:
- A callback function has been specified (i.e. nun-NULL) with the
rtt_set_overflow_cb(),rtt_set_arlarm(),rtc_set_alarm(). -> Block BACKUP - No callback function he been specified -> Don't block a power state at all.
Would that fit your use case?
There was a problem hiding this comment.
Ah yes of course.
But doesn't that mean BACKUP mode should never be entered automatically by pm_layered as we always lose all state there - it can only be entered explicitly.
There was a problem hiding this comment.
If everything is setup correctly, BACKUP will be blocked by some peripherals. We don't need the default-blocked here ... I would say this is already the case?!
There was a problem hiding this comment.
But if BACKUP must always be blocked, why add code to the drivers to block it and not just block it by default?
There was a problem hiding this comment.
I tell the GPIO driver to stop listening for IRQs. And the RTC driver is also turned off.
But then how are you going to wake up from Deep Sleep?
Maybe we need an additional API function for that. For the GPIO driver we could have gpio_init_wake(pin, …) and for the RTC/RTT driver we could have rtx_set_wakeup(offset)?
The former would certainly be cleaner than the current approach where if gpio_init_int() is called on a pin that is capable of waking the system from Deep Sleep, it will just configure it to do so. (The advantage of this approach was that it required no driver change, if you connect your accelerometer to a wake pin and set the threshold even, it would just wake you up from Deep Sleep without the need to configure anything).
There was a problem hiding this comment.
I see ... yeah, we need the RTC ;-)
The correct way to do it (code size optimization out of the way) is to block and unblock in the rtx_set_alarm() / rtx_clear_alarm() resp. rtt_set_overflow_cb() / rtt_clear_overflow_cb() method and have a dedicated rtx_set_wakeup() method, right?
There was a problem hiding this comment.
39bf4a2 tries to demonstrate that. The RTC/RTT driver is kinda crowded ...
There was a problem hiding this comment.
ROM size for tests/periph_rtc on the samr30-xpro:
# Driver from current master copied in place
text data bss dec hex filename
13836 160 2316 16312 3fb8 /home/jue/Projects/ssv/RIOT/tests/periph_rtc/bin/samr30-xpro/tests_periph_rtc.elf
# With SAM0_RTCRTT_PM_BLOCK undefined (i.e. no pm in this driver)
text data bss dec hex filename
13836 160 2316 16312 3fb8 /home/jue/Projects/ssv/RIOT/tests/periph_rtc/bin/samr30-xpro/tests_periph_rtc.elf
# With SAM0_RTCRTT_PM_BLOCK defined
text data bss dec hex filename
13928 160 2316 16404 4014 /home/jue/Projects/ssv/RIOT/tests/periph_rtc/bin/samr30-xpro/tests_periph_rtc.elf
There was a problem hiding this comment.
Regarding gpio_init_int vs. wake-up. I'd prefer to have a special gpio_wake_enable() / gpio_wake_disable() interface. It's a special feature that boards/cpus may provide. This separation would give users the option to opt-out. On top we can do proper power management ;-)
What do you think?
|
May I squash? All those fixup commits become hard to handle ;-) |
|
Sure, go ahead with squashing |
Otherwise we get stuck in an endless loop ...
periph_cpu.h should define the required pm modes. Additionally, some CPUs require a certain pm mode in USB IDLE mode.
Due to the RIOT_EPOCH of 2020 this overflow will happen in year 2084. It would be scary if IoT devices are still around then. We can save RAM and ROM. Furthermore, this overflow handling should block BACKUP power mode in order to keep track of the reference year.
In DMA mode SPI transfers are carried out by HW. We need to block certain pm modes during transfer.
We can get rid initially blocked pm modes \o/
jue89
force-pushed
the
feature/saml21_pheriph_pm
branch
from
39bf4a2
to
73e6886
Compare
|
I can confirm that USB still works on |
There was a problem hiding this comment.
Looks good to me.
I don't quite sure about the ramifications this will have, but efm32 seems to be a good canary.
I'll give this a try on our saml21 based sensor/app just to see if things crash & burn horribly (but since we use ztimer_no_periph_rtt probably nothing will change).
|
Thank you a lot for your effort looking into this and all the valuable input! It improved the implementation significantly, especially the RTC/RTT part. Do you want to try this out on your sensors before we merge? Or shall we merge and fix problems later - if they occur? |
No (this one is right from my pov) I think timer_start and stop doing pm_block / unblock is good (this PR) (this does not block irq for 2us but 2x1us) But i don't think ztimer should stop the timer (the other PR) if it does not care if the timer is running in background-> just do pm in ztimer |
|
Last test on our sensor with Works for me! ;-) |
|
Thank you again :) |
Contribution description
Currently, RIOT just runs with by-default blocked power modes. This PR changes this for the saml21 family: All peripheral drivers now block and unblock pm modes depending on whether they are used or nor. In combination with #17607 this will get us near to seamless power management
Current state of this PR:
periph_timerperiph_uartperiph_gpioperiph_usbdevperiph_rtcperiph_rttperiph_spi(DMA transfers ...)Affected boards (using
saml21):Testing procedure
periph_timerFlash
test/periph_timeron asaml21board:periph_gpioFlash
test/periph_gpioon asaml21board:periph_usbdevFlash
tests/usbus_cdc_ecmon asaml21board:periph_uartFlash
tests/periph_uarton thesamr30-xproboard:Changes to the
periph_conf.h:Bridge PB03 + PB22.
periph_spiFlash
tests/periph_spiwith DMA support on thesamr30-xproboard:Issues/PRs references
My talk from the last RIOT summit: https://www.youtube.com/watch?v=N2etFVR5mfg
Waiting for #18825