periph/spi: power regression #6652
Conversation
| /* disable device and put it back to sleep */ | ||
| dev(bus)->CTRLA.reg &= ~(SERCOM_SPI_CTRLA_ENABLE); | ||
| while (dev(bus)->SYNCBUSY.reg & SERCOM_SPI_SYNCBUSY_ENABLE) {} | ||
| poweroff(bus); |
There was a problem hiding this comment.
Does it make any difference if you leave the poweroff(bus); line in?
There was a problem hiding this comment.
I don't think I tested that explicitly. I'll be back in the lab in about 4 hours and I'll test that then.
There was a problem hiding this comment.
If you leave the poweroff(bus) in there, the power consumption remains low. It appears that it is clearing ENABLE that causes problems.
There was a problem hiding this comment.
Furthermore if you put the first two lines back in (clearing ENABLE) but not the poweroff(bus), the power consumption goes high. So I don't think it is a poweroff() happening too soon after disable.
| @@ -117,6 +118,10 @@ int spi_acquire(spi_t bus, spi_cs_t cs, spi_mode_t mode, spi_clk_t clk) | |||
| /* power on the device */ | |||
| poweron(bus); | |||
|
|
|||
| /* disable the device */ | |||
| dev(bus)->CTRLA.reg &= ~(SERCOM_SPI_CTRLA_ENABLE); | |||
| while (dev(bus)->SYNCBUSY.reg & SERCOM_SPI_SYNCBUSY_ENABLE) {} | |||
There was a problem hiding this comment.
Taken the fact of basically moving this from spi_release to spi_aquire makes me wonder if there is some combination where the SPI device gets configured but is enabled.
The following registers are enable-protected, meaning that they can only be written when the SPI is disabled (CTRL.ENABLE is zero):
- Control A register (CTRLA), except Enable (CTRLA.ENABLE) and Software Reset (CTRLA.SWRST)
- Control B register (CTRLB), except Receiver Enable (CTRLB.RXEN)
- Baud register (BAUD)
- Address register (ADDR)
There was a problem hiding this comment.
If I am understanding you right, a counterexample is that if I put the disable inside the spi_release, but leave the disable also in spi_acquire, the power consumption goes up again. So I don't think it is the fact that spi_acquire is being called while the device is enabled.
|
I have seen a similar issue with the SPI on Kinetis. There is a leakage current of some hundreds of microamps if the clock gate to the SPI module is turned off without first telling the SPI hardware via the module disable bit in one of the control registers. It's a completely different CPU, but I imagine on that hardware at least that the spi module needs a few clock cycles to turn off properly, which it can't of the clock is just gated off. Maybe there's something similar at play here? |
|
@gebart but if you put the disable lines in spi_release but do not call poweroff() the power consumption still goes high. It seems it is the disabling of the SPI device not the clock domain that is causing the problems. |
|
Hi It is NOT SPI-specific issue and it is NOT leakage current. If you have any digital input with integrated Schmitt trigger (most DIs have it) connected to MCU GPIO without external pull-up/pull-down and then switch MCU to STOP or STANDBY with GPIOs disabled (in AIN or other High-Z mode), this input will be de facto left floated. Which means, Schmitt trigger will switch randomly between 0 and 1 according to some external noise on a pin, consuming a lot of energy — as CMOS logic consumes when it switches. To achieve lowest possible power consumption, you have to: b) if you have any external devices connected to MCU, DO NOT switch corresponding digital outputs to High-Z mode. That means, for SPI devices keep NSS high, SCK and MOSI high or low, for UART devices keep TX high, et cetera. c) if you need STANDBY mode (when GPIO is unpowered in most MCUs), use external pull-ups or pull-downs. After that, you do not need to keep any specific interfaces (SPI, UART, etc.) clocked or even powered in sleep. We implemented automatic configuration of commonly used interfaces in our firmware, check lpm_before_i_go_to_sleep() here: https://github.com/unwireddevices/RIOT/blob/stm32l1_lpm/cpu/stm32l1/lpm_arch.c With STM32L151CCU6, we have 1.65 uA in STOP + RTC mode. |
|
@olegart with respect, I am well aware of that phenomenon, you are preaching to the choir. That only applies to the MISO line, and it is only responsible for about 70uA of current. It was I who filed #5868. I put a note in this diff saying that while changing the MISO to IN_PD was necessary and does reduce power consumption, that doing so in isolation only removes 70uA. To be clear, that was the very first line I changed, and the power went from 1530 +- 10 uA to about 1440 +- uA so there is clearly something else at play. Also @olegart, our system current with this patch is 2.56 +- 0.02 uA, without needing a manual "stop-the-world" function. So with respect, this definitely IS an SPI issue, and it probably is not just a floating pin. |
|
But @olegart does make a good point that there is no benefit to disabling the SPI module on spi_release. Sure we can remove the APBC clock but do we need the disable? We can just disable it in acquire before we mess with the baud. If I made a table of empirical power consumption in every sleep mode on the SAMR21 with and without disabling the SPI module, would that be enough to convince the maintainers that we should merge a variant of this? My hunch is that it doesn't save power and it clearly causes problems. |
|
Oh, yes. my mistake. Sincere apologies. As far as I understand Atmel SAM architecture, there's no need to configure PU/PD on unused pins; disabling input buffer (setting INEN bit to 0) when CPU is going to sleep is enough. Also, it should be the preferred method of reducing power consumption — PD may cause additional power consumption in sleep if there's an external pull-up for some reason. Those actions should be taken by power manager, not SPI driver, more or less as we implemented it. Regarding SPI power consumption, additional tests are needed to understand the reason behind it. Disabling peripheral should not cause power drain by itself. As I understood, you don't have external SPI devices connected. Can you please switch all GPIOs in use by SPI (MISO, MOSI, NSS, SCK) manually to digital input mode with INEN = 0 before putting MCU to sleep and check power consumption again? |
|
The SAMR21 has a radio connected on the SPI bus internally in the package, so the bus is used. |
|
Ok. so you need to manually configure before sleep:
What's the power consumption in this case? If it's down to 2 uA, it's clearly GPIO issue. If not, it's probably internal SPI issue. |
|
With:
Power consumption: 2.56uA With:
Power consumption: 1.4 mA |
|
I would love to see a table of empirical measurements of the power consumption. |
|
Did anybody fix this in the latest release or is the problem still there? |
|
Ok I verified the problem is still there in 2017.04. Are no power consumption tests done on a release? How could something this massive slip through? I have rebased this fix on top of 2017.04. |
|
Sorry for jumping into this discussion so late.
Not really (or only very sporadically on the iotlab). This is a huge problem that we are aware off and that we are working on (but not quite the priority that I would wish for). The goal is indeed to include power measurements into the CI system, and as the next-gen CI (Murdock 2) is ready now, this is one of the next steps. Also there are some other conceptual gaps we have to close, most prominently Regarding this issue: seems like I missed enabling the PD resistor for the MISO line when remodeling the SPI driver, there was no reason besides slppyness on my side for this -> #7037 for quick re-merge The issue with disabling the peripheral before clock-gating is another topic. From the datasheet I was under the impression, that we would achieve the best energy savings when properly disabling the peripheral before clock gating it. But I guess if practice shows that this makes matters worse, I'd say we go with the proposed solution and only disable the peripheral for a short amount of time while configuring the bus clock speed. @immesys would this work? |
|
Yeah, the PR in its form here is exactly what I use on my systems and I get low power out of it. The SPI also works well without any problems. We have a set of changes to xtimer that make it suitable for low power on samr21, if you are interested I could try put together a PR for your perusal. The biggest is that switch the source to OSCULP using RTT instead of a high power timer. |
|
Oh, missed yesterday that the PR is already doing what I wrote (miss-read the diff and somehow had in mind that the disable is done in I would be o.k. to merge this PR (almost) as is -> just the |
cpu/sam0_common/periph/spi.c
Outdated
| @@ -102,7 +102,8 @@ void spi_init(spi_t bus) | |||
|
|
|||
| void spi_init_pins(spi_t bus) | |||
| { | |||
| gpio_init(spi_config[bus].miso_pin, GPIO_IN); | |||
| //MISO must always have PD/PU, see #5968. This is a ~65uA difference | |||
There was a problem hiding this comment.
for github formalities: please change to c-style comment, or remove these two lines in favor of #7037.
There was a problem hiding this comment.
Ok, fixed. I changed this comment style and combined the commits for cleaner merging.
thomaseichinger
added
the
CI: ready for build
|
@immesys thanks for putting pressure on the low power aspect. I think it makes sense to go with the full change set here. CI tests passed, as soon as @haukepetersen approves the your fixes we are ready to go. @kaspar030 should we port this to the 2017.04 branch too? |
|
ACK and to |
This PR is more of a request for comments, although it does merge and it does work. To be honest I need help working out why the problem exists in the first place.
#4780 reworked the SPI interface but also introduced a massive power consumption regression. Before the PR, idle power on the SAMR21 was at 2.7 uA on my platform. After the PR the deep sleep (lowest power mode) is at 1.5 mA.
I rewrote the code several times blending in parts of the old code and parts of the new code and eventually discovered the following minimal change to get the old power behavior back. This is actually two changes in one. The MISO pin as IN_PD is a regression same as last time in #5868, but that's only about 65uA, not 1500uA.
Changing the code to not disable the SPI device, however, fixes the problem. We also need to then introduce code to disable it during acquire so that the baud rate changes don't require sync, but the main change is removing the shutdown in spi_release.
To be clear: without this patch, deepest sleep = 1.5mA, with the patch, deepest sleep = 2.7uA on a SAMR21 with no other significant components on the board.
But no matter how long I stare at the code I don't really see what is wrong with what is there or why the patch makes such a big difference. I assure you my testing methodology is sound, and I have verified that the device works properly (radio TX/RX) with or without the patch.
Hopefully @haukepetersen or @kaspar030 can help here?