RIOT-OS
periph/spi: add support for printing and testing SPI clock rate
Contribution description
Re-open #14768 because I needed it yesterday...
The current test does not allow to test all baud rates supported by the hardware and driver. On the nucleo-f334r8 board:
- 0: 100 KHz -> 281.250 KHz
- 1: 400 KHz -> 281.250 KHz
- 2: 1 MHz -> 562.500 KHz
- 3: 5 MHz -> 4.5MHz
- 4: 10 MHz -> 9 MHz
It was not possible to make tests with others supported speeds (1.125 MHz, 2.25 MHz, 18 MHz, and 36 MHz).
One might think that this is a limitation of the RIOT SPI driver but this is no longer the case for the STM32 CPU family...
In order to not poke CPU configuration in application / test code it was first proposed that the spi_acquire() function return the resulting speed in #15904 but this nullified the benefit of #15902.
Choosing the right prescaler value for an arbitrary speed is also a time-consuming operation which is not needed to be performed each time the bus is acquired.
The stm32 implementation used fixed-point arithmetic and caching to minimize performance impact but caching is inefficient in some use cases i.e. two peripherals with different speed sharing the same bus.
Two functions have been added to the API:
- spi_get_clk() chooses or computes the spi_clk_t value needed to spi_acquire so caching is moved to the application level.
- spi_get_freq() return the actual frequency from a given spi_clk_t
I added arbitrary speed in all implementations where it was missing.
I had to add a bus parameter to the spi_get_*() functions because it was necessary for implementations where multiple devices can have different clock sources ie sam0 and stm32. This broke the SPI_CLK_* macros that were reverted to an enum.
For efm32 (which use the gecko SDK) and lm4f120 (which use the stellaris peripheral driver library) implementations spi_clk_get() computes the prescaler as in the SDK / library but return the actual frequency (no modification to spi_acquire().
Testing procedure
Build tests/periph_spi on all hardware architectures
- [x] atmega (atmega1284p)
- [x] atxmega (atxmega-a1-xplained)
- [x] cc2538 (omote)
- [x] efm32 (slstk3401a)
- [x] esp (esp8266-esp-12x)
- [x] fe310 (hifive1)
- [x] kinetis (openlabs-kw41z-mini)
- [x] lm4f120 (ek-lm4f120xl)
- [x] lpc23xx (msba2)
- [x] msp430fxyz (z1)
- [x] nrf51 (nrf6310)
- [x] nrf52 (nrf52840dk)
- [x] qn908x (qn9080dk)
- [x] sam0_common (same54-xpro)
- [x] sam3 (arduino-due)
- [x] stm32 (nucleo-g474re)
Test frequency printing and check there with an oscilloscope
- [x] atmega - atmega1284p - possible frequencies: 8 MHz / 21..7 - measures ok
- [ ] atxmega - possible frequencies: CLOCK_CORECLOCK / 21..7
- [ ] cc2538 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32512
- [ ] efm32 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32768
- [x] esp - esp8266-esp-12x - possible frequencies: 40 MHz / 1..8192 - measures ok
- [ ] fe310 - possible frequencies: cpu_freq() / 2 / 1..4096
- [ ] kinetis - possible frequencies: CLOCK_BUSCLOCK / 4..229376
- [ ] lm4f120 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32512
- [ ] lpc23xx - possible frequencies: CLOCK_CORECLOCK / 21..4 / 1..127
- [ ] msp430fxyz - possible frequencies: CLOCK_CMCLK / [1|2]..65535
- [ ] nrf51 - possible frequencies: 125 KHz, 250 KHz, 500KHz, 1 MHz, 2 MHz, 4 MHz, 8 MHz
- [ ] nrf52 - possible frequencies: 125 KHz, 250 KHz, 500KHz, 1 MHz, 2 MHz, 4 MHz, 8 MHz
- [ ] qn908x - possible frequencies: AHB bus clock (32MHz) / 1..65536
- [ ] sam0 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..256
- [ ] sam3 - possible frequencies: CLOCK_CORECLOCK / 1..255
- [x] stm32 - possible frequencies: bus_clock / 21..8 - nucleo-g474re: 644 KHz -> 85 MHz - measures ok
Issues/PRs references
Include #15902. Include and improve #16811. See also #15904.
My only issue is that if this information is needed, we should have an API for that - not poke CPU configuration in application / test code.
My only issue is that if this information is needed, we should have an API for that - not poke CPU configuration in application / test code.
I totally agree with that.
Some tests:
nucleo-f746zg
init
2021-09-01 04:35:48,096 # init
2021-09-01 04:35:48,101 # usage: init <dev> <mode> <clk> [cs port] [cs pin]
2021-09-01 04:35:48,101 # dev:
2021-09-01 04:35:48,103 # 0: SPI_DEV(0)
2021-09-01 04:35:48,104 # 1: SPI_DEV(1)
2021-09-01 04:35:48,105 # mode:
2021-09-01 04:35:48,109 # 0: POL:0, PHASE:0 - on first rising edge
2021-09-01 04:35:48,112 # 1: POL:0, PHASE:1 - on second rising edge
2021-09-01 04:35:48,116 # 2: POL:1, PHASE:0 - on first falling edge
2021-09-01 04:35:48,120 # 3: POL:1, PHASE:1 - on second falling edge
2021-09-01 04:35:48,120 # clk:
2021-09-01 04:35:48,123 # 0: SPI_CLK_100KHZ (421875 Hz)
2021-09-01 04:35:48,126 # 1: SPI_CLK_400KHZ (421875 Hz)
2021-09-01 04:35:48,128 # 2: SPI_CLK_1MHZ (843750 Hz)
2021-09-01 04:35:48,131 # 3: SPI_CLK_5MHZ (3375000 Hz)
2021-09-01 04:35:48,133 # 4: SPI_CLK_10MHZ (6750000 Hz)
2021-09-01 04:35:48,136 # n: arbitrary value in Hz
2021-09-01 04:35:48,137 # cs port:
2021-09-01 04:35:48,142 # Port of the CS pin, set to -1 for hardware chip select
2021-09-01 04:35:48,143 # cs pin:
2021-09-01 04:35:48,149 # Pin used for chip select. If hardware chip select is enabled,
2021-09-01 04:35:48,153 # this value specifies the internal HWCS line
init 0 0 1700000 0 0
2021-09-01 04:36:11,845 # init 0 0 1700000 0 0
2021-09-01 04:36:11,852 # SPI_DEV(0) initialized: mode: 0, clk: 1700000(1687500 Hz),cs_port: 0, cs_pin: 0
> init 0 0 14000000 0 0
2021-09-01 04:37:16,051 # init 0 0 14000000 0 0
2021-09-01 04:37:16,058 # SPI_DEV(0) initialized: mode: 0, clk: 14000000(13500000 Hz),cs_port: 0, cs_pin: 0
> init 0 0 27000000 0 0
2021-09-01 04:37:41,326 # init 0 0 27000000 0 0
2021-09-01 04:37:41,333 # SPI_DEV(0) initialized: mode: 0, clk: 27000000(27000000 Hz),cs_port: 0, cs_pin: 0
> init 0 0 54000000 0 0
2021-09-01 04:37:51,521 # init 0 0 54000000 0 0
2021-09-01 04:37:51,529 # SPI_DEV(0) initialized: mode: 0, clk: 54000000(54000000 Hz),cs_port: 0, cs_pin: 0
> init 0 0 108000000 0 0
2021-09-01 04:38:09,621 # init 0 0 108000000 0 0
2021-09-01 04:38:09,628 # SPI_DEV(0) initialized: mode: 0, clk: 108000000(54000000 Hz),cs_port: 0, cs_pin: 0
All clock rates are accessible now.
esp8266-esp-12x
init
2021-09-01 04:41:04,630 # init
2021-09-01 04:41:04,654 # usage: init <dev> <mode> <clk> [cs port] [cs pin]
2021-09-01 04:41:04,654 # dev:
2021-09-01 04:41:04,655 # 0: SPI_DEV(0)
2021-09-01 04:41:04,655 # mode:
2021-09-01 04:41:04,656 # 0: POL:0, PHASE:0 - on first rising edge
2021-09-01 04:41:04,657 # 1: POL:0, PHASE:1 - on second rising edge
2021-09-01 04:41:04,658 # 2: POL:1, PHASE:0 - on first falling edge
2021-09-01 04:41:04,659 # 3: POL:1, PHASE:1 - on second falling edge
2021-09-01 04:41:04,674 # clk:
2021-09-01 04:41:04,675 # 0: SPI_CLK_100KHZ (100000 Hz)
2021-09-01 04:41:04,676 # 1: SPI_CLK_400KHZ (400000 Hz)
2021-09-01 04:41:04,677 # 2: SPI_CLK_1MHZ (1000000 Hz)
2021-09-01 04:41:04,678 # 3: SPI_CLK_5MHZ (5000000 Hz)
2021-09-01 04:41:04,679 # 4: SPI_CLK_10MHZ (10000000 Hz)
2021-09-01 04:41:04,679 # cs port:
2021-09-01 04:41:04,681 # Port of the CS pin, set to -1 for hardware chip select
2021-09-01 04:41:04,681 # cs pin:
2021-09-01 04:41:04,683 # Pin used for chip select. If hardware chip select is enabled,
2021-09-01 04:41:04,685 # this value specifies the internal HWCS line
> init 0 0 5 0 0
2021-09-01 04:42:07,981 # init 0 0 5 0 0
2021-09-01 04:42:07,984 # error: invalid bus speed specified
The driver do not supports arbitrary bus speed.
atmega1284p
2021-09-01 04:50:41,813 # init
2021-09-01 04:50:41,865 # usage: init <dev> <mode> <clk> [cs port] [cs pin]
2021-09-01 04:50:41,871 # dev:
2021-09-01 04:50:41,888 # 0: SPI_DEV(0)
2021-09-01 04:50:41,895 # mode:
2021-09-01 04:50:41,940 # 0: POL:0, PHASE:0 - on first rising edge
2021-09-01 04:50:41,986 # 1: POL:0, PHASE:1 - on second rising edge
2021-09-01 04:50:42,031 # 2: POL:1, PHASE:0 - on first falling edge
2021-09-01 04:50:42,078 # 3: POL:1, PHASE:1 - on second falling edge
2021-09-01 04:50:42,084 # clk:
2021-09-01 04:50:42,117 # 0: SPI_CLK_100KHZ (62500 Hz)
2021-09-01 04:50:42,150 # 1: SPI_CLK_400KHZ (250000 Hz)
2021-09-01 04:50:42,181 # 2: SPI_CLK_1MHZ (500000 Hz)
2021-09-01 04:50:42,213 # 3: SPI_CLK_5MHZ (2000000 Hz)
2021-09-01 04:50:42,247 # 4: SPI_CLK_10MHZ (4000000 Hz)
2021-09-01 04:50:42,257 # cs port:
2021-09-01 04:50:42,316 # Port of the CS pin, set to -1 for hardware chip select
2021-09-01 04:50:42,326 # cs pin:
2021-09-01 04:50:42,392 # Pin used for chip select. If hardware chip select is enabled,
2021-09-01 04:50:42,440 # this value specifies the internal HWCS line
The resulting speeds displayed are half of the targeted speeds because the spi_clk_t enum from cpu/atmega_common/periph_cpu.h has been written assuming a master clock speed of 16 MHz, but this card operates at only 8 MHz. So this which is displayed is what you really get on the SCK PIN.
Other architectures
I have not the hardware to make tests on other architectures.
Ping me if you want any SPI tests run on the HiL CI. We support clock speed checking, I think there is a bit of a rework that could use this. ping @MarcelStenzel
Generally in favor of this one, but its a big change, I don't have an oscilloscope, but I do have a bunch of BOARDs with spi devices attached so I could do testing of that kind. I can do some more testing next tuesday.
@MrKevinWeiss could HIL setup help in testing all of this?
This is what is was built for.
@MarcelStenzel was reworking the SPI tests to get around some limitations of PHiLIP and DUT hardware but we can give it a go now. Pretty soon I can boot up rack 5 and add another 8 boards to the mix.
| PASS | FAIL | SKIP |
|---|---|---|
| 0 | 20 | 2 |
Tests are expected to fail. There are numbers on the actual SPI clock measurements hidden in the logs.
Here is an example of logs showing the actual time measurements
Do you have a link to a master run to compare to?
If there is an API change I would have to adapt the robot tests to match that though...
@hugueslarrive could you rewrite the PR description since this PR is now going further than the initial intent?
If there is an API change I would have to adapt the robot tests to match that though...
Yes I think there are new functions here to test, how hard would it be to add new tests for this? @hugueslarrive @maribu could you give @MrKevinWeiss an idea of what kind of new tests would be needed in the HIL? This would make this PR getting in sooooo much easier.
Ya I can take a look... I was hoping to offload to @MarcelStenzel but I think he doesn't have time to do all my requests.
fyi the failures are occurring in the nucleos due to the assert in the spi_get_clk function. It seems I cannot do that much exploratory testing of clocks without either overriding or disabling the assert functions.
I am wondering if a min/max would be better than the assert, especially since there are no default speeds per board?
I am wondering if a min/max would be better than the assert, especially since there are no default speeds per board?
The API guarantees you that the picked clock will not be higher than the requested to avoid driving the bus out of spec. This is based on the observation that datasheets usually contain an upper bound for the SPI clock, but either no lower bound or a lower bound as low as 100 kHz or so.
So: You should never experience a blown assertion for too high frequencies (the actual clock will just saturate when maxed out). But when you request e.g. 1 Hz as SPI clock, you'll get a blown assertion.
From the user point of view this is IMO ideal and there is no need for min / max.
For testing: I would start with a reasonably high clock (e.g. 1 MHz) and increase by 50%. Only if the returned clock config differs from the last run, it makes sense to actually test. If the clock config hasn't changed for the last 12 iterations (so was increased by more than factor 129), it should be pretty safe to assume that the maximum frequency was reached.
What about adding an enum or getter function to see the min/max available?
I can think of applications that would try to communicate at a high speed and if failures occur to lower it. For example, if something needs to communicate through varying EMC conditions.
OK, you convinced me that adding a getter function for the maximum and minimum SPI frequency makes sense. (It will have to be a function or a function-like macro, since some MCUs support different frequencies for their SPI peripherals. E.g. they might have an high speed QSPI that can be used to connect flash to it that is used as regular SPI in RIOT, and a normal SPI (without Quad-) that runs at a lower clock.)
The reason I was skeptical to this is that I observed that niche features in periph APIs often lead to some of the implementations not implementing it consistently, correctly, or at all. For that reason I'm always in favor of keeping peripheral APIs as slim as possible and rather implement convenience features on top of it.
But I agree that dynamically adapting the clock frequency is something where you don't want to run in an assert(), but rather just keep the SPI at the lowest frequency and hope for conditions to improve over time.
I don't think that a getter for a maximum frequency is strictly needed, but as it is trivial to automatically test its correctness, it should be a piece of cake to maintain.
The assert essentially removed the possibility of setting a lowest frequency.
As long as I have that I am happy. With this update we should also be able to verify if the frequency now.
I also don't need a max frequency but thought, if we have enums (and it is in fact trivial) then why not.
@maribu @MrKevinWeiss would be really nice to get this one in this release, is there still some testing missing?
| PASS | FAIL | SKIP |
|---|---|---|
| 1 | 27 | 3 |
✅ slstk3401a
| PASS | FAIL | SKIP |
|---|---|---|
| 1 | 0 | 0 |
| TEST | RESULT |
|---|---|
| tests/periph_spi | ✅ pass |
@maribu @MrKevinWeiss @hugueslarrive lets not let this change diverge too much, maybe we can directly push the required changes?
- rebase
- remove assert and implement getter functions?
- something else?
Sounds great. I cannot highlight enough how cool this would be to have in the release :)
At least for EPSs I can say that it works like a charm. I have tested different clocks that work even though ALL SPI tests in HIL test output failed.
Sounds great. I cannot highlight enough how cool this would be to have in the release :)
I agree, this can also allow us to tighten up the tolerances for the SPI clock tests. Again, the only issue I have is the asserts when trying to set a specific clock rate.
even though ALL SPI tests in HIL test output failed
Yup, you should ignore these results. The default speed for most tests is 100k, since that fails the assertion all tests fail.
It seems that we need testers for different platforms. I would guess that @hugueslarrive does not have the hardware available for all platforms.