samd21: spi: fix power regression

[immesys]

tl;dr commit 845ef0d introduced a power consumption regression by leaving the MISO pin floating

Despite this being a one line fix, this took us four days to find, so allow me to elaborate. @Hyungsin and I have been working on getting RIOT running at ~10uA average current for a duty cycling SAMR21 based mote, basically we have #2309 and #5608 along with a set of changes here and there. We got everything working, sampling temperature at 1Hz with ~10uA consumption and we patted ourselves on the back with a "lets quickly rebase and then submit patches upstream". We rebased, (it was just four days of commits) and one of those commits was merging #5743.

Nothing worked.

Ah well, we consoled ourselves, at least this means RIOT is active, this is a good problem to have. So we got everything to compile and redid the platform support only to find that our MCU-only power consumption had gone from 2.5uA in idle to over 70 uA. Somewhere in the more than 10k changed lines, there was a problem. We spent a few days inspecting all the clocking config (we thought there must be a new RUNSTDBY clock). We scoured the PM registers, nothing. Eventually I started bisecting the PR, and discovered that 845ef0d was to blame, but nothing seemed blatantly wrong. Eventually by reverting it line by line we discovered that the new code does not configure the pullup resistor on MISO. Adding that brought us back 👍

Hallelujah:

I guess to make this kind of thing easier to find in future we

a) might want to add power consumption tests of a physical device to the CI
b) might want to split large PR's into mostly-harmless refactor (updating the ASF and moving headers for example) and things that change implementation details, so that problems don't get buried in 10kloc batches.

Now that this is sorted out, I will get back to preparing a PR for low-power SAMR21 platforms.

[Hyungsin]

We are happy to move on to the next step :)

[jnohlgard]

Is it possible to tri-state the miso pin between calls to spi transfer functions instead of an unconditional pull resistor? If there is a slave device driving pulling the miso pin the other direction then there will be a direct leak between vdd and gnd.

[jnohlgard]

My question is only to stimulate discussion and thought around power savings, I won't object to merging this PR. I don't want to hinder the progress on your project.

[immesys]

I don't think it is necessary to try and turn the pullup on/off. We don't do that with I2C where the pullup is normally strong (<5k) and here the pullup/down is nominally 40k so the current during the periods where SPI is active is negligible (80uA) compared to the current of having the processor active and doing SPI.

What would be a greater power savings is if we started doing I2C and SPI using DMA, so that we could turn the CPU off during those transfers. That is a problem for another day :p

edit: necessary is maybe the wrong word. It is probably the right thing to do, but not the most significant at the moment

[PeterKietzmann]

ACK for this PR. Tested SPI on a samr21-xpro. If there is need for further discussions it would be great if you could open an issue.