periph/spi: add support for printing and testing SPI clock rate

[hugueslarrive]

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().

Drops msp430 support for now (#16727 (comment))

Testing procedure

Build tests/periph_spi on all hardware architectures

• atmega_common (atmega1284p)
• atxmega (atxmega-a1-xplained)
• cc2538 (omote)
• efm32 (slstk3401a)
• esp32 (esp32c3-devkit)
• esp8266 (esp8266-esp-12x)
• fe310 (hifive1)
• gd32v (seeedstudio-gd32)
• kinetis (openlabs-kw41z-mini)
• lm4f120 (ek-lm4f120xl)
• lpc23xx (msba2)
• msp430fxyz (z1)
• nrf51 (nrf6310)
• nrf5x_common (nrf52840dk)
• qn908x (qn9080dk)
• rpx0xx (rpi-pico)
• sam0_common (same54-xpro)
• sam3 (arduino-due)
• stm32 (nucleo-f303k8)

Test frequency printing and check there with an oscilloscope

• atmega_common - possible frequencies: 8 MHz / 2^1..7 - atmega1284p: 62.5 KHz -> 4 MHz - measures ok
• atxmega - possible frequencies: CLOCK_CORECLOCK / 2^1..7
• cc2538 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32512
• efm32 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32768
• esp32 - possible frequencies: CLOCK_CORECLOCK / 5..16384
• esp8266 - possible frequencies: 80 MHz | 80 MHz / 2 / 1..8192 - esp8266-esp-12x: 4.9 KHz -> 80 MHz - measures ok *
• fe310 - possible frequencies: coreclk() / 2 / 1..4096
• gd32v - possible frequencies: bus_clock / 2^1..8
• kinetis - possible frequencies: CLOCK_BUSCLOCK / 4..229376
• lm4f120 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..32512
• lpc23xx - possible frequencies: CLOCK_CORECLOCK / 2^1..4 / 1..127
• msp430fxyz - possible frequencies: SMCLK/ [1|2]..65535
• nrf51 - possible frequencies: 125 KHz, 250 KHz, 500KHz, 1 MHz, 2 MHz, 4 MHz, 8 MHz
• nrf5x_common - possible frequencies: 125 KHz, 250 KHz, 500KHz, 1 MHz, 2 MHz, 4 MHz, 8 MHz
• qn908x - possible frequencies: AHB bus clock (32MHz) / 1..65536
• rpx0xx - possible frequencies: CLOCK_PERIPH / 2..254 / 1..256
• sam0 - possible frequencies: CLOCK_CORECLOCK / 2 / 1..256
• sam3 - possible frequencies: CLOCK_CORECLOCK / 1..255
• stm32 - possible frequencies: bus_clock / 2^1..8 - nucleo-f303k8: 250 KHz -> 32 MHz - measures ok *

*My equipment does not allow me to measure frequencies above 15 MHz.

Issues/PRs references

Include #15902. Include and improve #16811. See also #15904.

[fjmolinas]

Some inline comments, @benpicco you were reviewing the initial PR, anything missing from that one?

[benpicco]

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.

[gschorcht]

Some general comments:

• Instead of making parameter declarations non-const, the SPI frequency in the parameter set should be considered as requested frequency, but the actual SPI frequency should then be stored in the corresponding device descriptor. Removing the const keyword from the parameter set declaration has the disadvantage that the parameter set variable (that can be pretty large) is then placed in RAM instead of ROM.

• I am very impressed how many drivers/packages have to be changed because of the impact of this API change, so I wonder if this is really the right approach. Backwards compatibility is essential for user acceptance. Instead of changing all the hundreds of drivers, you should think again about using an approach as already suggested by @maribu in periph/spi: add support for printing and testing SPI clock rate #16727 (comment). The PR is already pretty large and still not complete. It is so large that can't almost handle it in my browser. The approach suggested by @maribu would allow to split the PR into the API change and driver changes step by step.
  Also the approach mentioned by @maribu in periph/spi: add support for printing and testing SPI clock rate #16727 (comment) to cache the last used SPI frequency settings in the backends and to change the settings only if another frequency is used in spi_acquire wouldn't be a problem and easy to implement.

• I'm also wondering whether the change of the name from spi_clk to spi_freq in the parameter sets are really necessary in the first step.

[hugueslarrive]

@gschorcht Thank you very much for your comments. About the const modifiers, I corrected the drivers from which I had removed it but there are others where I had left it without it causing a compile error so I'll have to see why...

The problem with caching the last used frequency settings in the backend is the case where several devices with different frequencies are used alternatively. Not only this can be a performance killer on architectures with costly prescaler computing, but it's also detrimental to real time, since spi_acquire will take more or less time depending on whether or not another thread has accessed the bus just before. That's why I'm convinced that the caching must be done in each device driver so that spi_acquire remains deterministic.

User acceptance shouldn't be a major issue since this is bus driver is primarily used through other device drivers. It should be transparent in most cases except for those who use their own driver without sharing it.

[maribu]

Sorry for the merge conflict with MSP430. I started rework all periph drivers for that MCU anyway, e.g. it currently only supports a single SPI bus (and at least the MSP430 F2xx family with the USCI based SPI can support multiple SPI interfaces). (And I also would like to look into sharing the USART/USCI peripheral providing the UART/SPI/I2C in the way it is done in nRF5x; right now the MSP430 x1xx family can use one UART plus either one I2C or one SPI, which frankly is pretty scarce.)

I think this PR should just drop SPI support for the MSP430 and stop worrying about that exotic platform. I will eventually add new SPI, UART and I2C implementations anyway. And git makes it pretty easy to base that work on the old code even when it was removed.

[benpicco]

I really don't understand the rationale behind the .spi_clk -> .spi_freq name change.

On most(?) platforms, SPI_CLK_5MHZ is already defined to be MHZ(5) today, so this isn't even a semantic change, but creates a lot of busywork.

[maribu]

User acceptance shouldn't be a major issue since this is bus driver is primarily used through other device drivers. It should be transparent in most cases except for those who use their own driver without sharing it.

I mostly agree with that. Still, users with drivers used as external modules will feel some pain.

Given that 99% of the use cases will either use a single SPI bus or all SPI buses are compatible with the same prescaler settings, providing the backward compatible constants would turn this into a non-breaking API change except for 1% of the use cases. (It might also be possible to pre-calculate prescaler settings for all SPI buses in a struct and pick the right one in spi_acquire(). Or to just always return the prescale for one type of SPI periph and convert the prescaler into the prescaler of other types in spi_acquire() with something as simple as a bit shift or so. This behavior could then be phased out over time when the backward compatible macros are finally removed.)

In addition to not breaking external modules (for at least 99% of the cases), this would also allow to update the drivers independently as follow ups. This would allow distributing the work (also the review tasks) on more shoulders.

Just to be sure I'm not misunderstood: I'm all in for phasing out spi_clk_t constants, especially with the prospect of turning SPI prescaler calculations from run time operations to compile time operations. But I think @gschorcht has a strong point in avoiding breaking changes for users of the SPI bus.

[maribu]

Combining an error code with the clock configuration in a struct looks pretty non-idiomatic C to me. Rather an int spi_get_clk(spi_clk_t *dest, spi_t bus, uint32_t freq_hz) would be more idiomatic to me. But that would conflict with my proposed mode of backward compatibility.

But to be honest: After having seen lots of users of peripheral APIs with return code checking and graceful error handling intended to be done by the caller, I lost all hope that driver developers will ever do this correctly. If spi_get_clk() would just assert() sane settings, we likely get better error handling for less bytes in .text.

Also, the only way to incorrectly call spi_get_clk() to my understanding is calling it with a non-existing value for spi_t bus or with a frequency that is so low that no prescaler can be found. The former clearly is a bug on the caller side, the latter is also difficult to recover from at runtime: If the device attached to a given hardware SPI bus and that bus just cannot generate a frequency compatible with that device, other than falling back to bit-baning SPI in software I have no idea to handle this at runtime. And I'd rather have the application developer than explicitly use the soft SPI than falling back at runtime.

[gschorcht]

Rather an int spi_get_clk(spi_clk_t *dest, spi_t bus, uint32_t freq_hz) would be more idiomatic to me.

👍

If I am correct, the only error code used in spi_clk_t::err is -EDOM? And probably no platform will return 0 as spi_clk_t::clk from spi_get_clk. So why not use as return value 0 in case of error and the clock different from 0 in case of success?

[hugueslarrive]

The qn908x platform can return 0:

    /* The value stored in DIV is always (divider - 1), meaning that a value of
     * 0 divides by 1. */
    return (spi_clk_t){ .clk = (uint16_t)(divider - 1) };

I guess I should not have changed the description from "Opaque type that contains an SPI clock configuration".

The user should not worry about what it contains but just use it to pass the value obtained from spi_get_clk to spi_acquire or spi_get_freq (which returns an int containing the actual frequency value or -EDOM in the idiomatic way).

I will put a better description in the documentation.

[gschorcht]

The qn908x platform can return 0:

🤔 too bad

[hugueslarrive]

@benpicco

I really don't understand the rationale behind the .spi_clk -> .spi_freq name change.

On most(?) platforms, SPI_CLK_5MHZ is already defined to be MHZ(5) today, so this isn't even a semantic change, but creates a lot of busywork.

This is not a name change, spi_freq has been added.

The problem is that spi_acquire needs a prescaler or divider value to be applied in MCU registers, which basically requires the source clock to be divided by the expected frequency, and that doing this computation at each acquire is a big waste of resources. Ideally spi_clk should be renamed spi_div or spi_psc or something like that (which is more related to a period than a frequency) but this won't be the case for all platforms even after this PR so I've left spi_clk for now.

As asking the user to provide a value ready to be applied to the registers is not an option, we provide a getter function that computes this parameter from an expected frequency. We also provide a convenience getter function that returns the actual resulting frequency from this parameter.

So each driver can get the required clock parameter during initialization and store it for later uses.

With this approach you can share the bus between several devices with different frequencies and access them in turn without any overhead.

This was not a marginal issue, as great efforts were made to mitigate it in some major implementations such as stm32.

[hugueslarrive]

Given that 99% of the use cases will either use a single SPI bus or all SPI buses are compatible with the same prescaler settings

The problem is not the use of several spi buses, as their number is known at compile time, so we could easily plan to store one value per bus at compile time. The problem arises when several different devices use the same bus, i.e. a storage device and a network device.

[benpicco]

The problem arises when several different devices use the same bus, i.e. a storage device and a network device.

but in that case all real-time assumptions are out of the window already as any of the two devices might block the bus for an arbitrary amount of time.

[maribu]

The problem arises when several different devices use the same bus, i.e. a storage device and a network device.

but in that case all real-time assumptions are out of the window already as any of the two devices might block the bus for an arbitrary amount of time.

Not really. Real time capable doesn't mean fast, just "in time". Reducing the overhead of spi_acquire() allows splitting larger SPI transactions into multiple smaller ones without too much overhead. This in turn would reduce the worst case time a higher prio thread has to wait for a lower prio thread to release the bus.

IMO reducing the overhead of spi_acquire () is very much beneficial for real time apps.

[hugueslarrive]

@gschorcht I moved this code as is from spi_acquire but I wonder why APB clock rate has to be at least 5 times SPI clock rate. From the reference manual section 7.4: "The maximum output clock frequency of ESP32 GP-SPI master is f apb /2" and even "When the SPI_CLK_EQU_SYSCLK bit in register SPI_CLOCK_REG is set to 1, and the other bits are set to 0, SPI output clock frequency is f apb."

[gschorcht]

This is what I had to learn when I was implementing the support for core clock frequencies lower than 80 MHz down to 2 MHz. For example with

CFLAGS='-DCONFIG_ESP32_DEFAULT_CPU_FREQ_MHZ=40' BOARD=esp32-wroom-32 make -j8 -C tests/periph/spi flash

and wired MOSI-MISO I get for 10 MHz:

> init 0 0 4 0 5
> send hallo
Sent bytes
   0    1    2    3    4 
  0x68 0x65 0x6c 0x6c 0x6f
    h    e    l    l    o 

Received bytes
   0    1    2    3    4 
  0x34 0x32 0xb6 0x36 0x37
    4    2   ??    6    7 

while it works for 5 MHz.

I have the same problem for 2 MHz core clock frequency. While it works for 400 kHz SPI clock frequency, it doesn't work for 1 MHz SPI clock frequency.

That's why I said that the APB clock which corresponds to the core clock for frequencies lower than 80 MHz has to at least 5 times the SPI clock frequency.

[hugueslarrive]

Note that 0x3432b63637 = 0x68656c6c6f >> 1. I observed the same issue on esp8266 for the highest frequency (80MHz with spi_clk_equ_sysclk).

It seems to be linked to spi_miso_delay_mode which should be set to 0 for frequencies higher than clkapb/4 according to the manual section 7.4.2.