Skip to content

Commit 432b698

Browse files
ithinuelithinuel
committed
add spi-overhaul rfc document
1 parent 94d1b98 commit 432b698

File tree

1 file changed

+216
-0
lines changed

1 file changed

+216
-0
lines changed

‎hal/rfcs/0000-spi-overhaul.md

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
# HAL overhaul : SPI
2+
3+
## Description
4+
5+
This updates targets the SPI HAL as the first step of a wide plan for all HAL APIs.
6+
7+
## Motivation
8+
9+
This work is part of the effort to bring well defined specification to APIs in `mbed-os`. Some users have shown interest in some addition to the supported features of this API ([bit ordering selection #4789](https://github.com/ARMmbed/mbed-os/issues/4789)).
10+
Analysis of previous work also revealed some inconsistencies in the current SPI HAL API that this RFC aims at fixing. Those inconsistencies are listed in the next chapter.
11+
Finally, `mbed-os` has grown a lot its supported target count and some of the oldest one could benefit quite significantly in maintenance cost from a factorisation of their code base.
12+
13+
### Inconsistencies
14+
15+
- The type used in block transfer functions vary between `(const) void*` and `(const) char*`.
16+
- Some method have a parameter to set the width of the symbols while it is part of the `spi_format` arguments.
17+
- Some method provide a `fill_symbol` parameter as a `char` which would not allow to set a symbol whose width is over 8bits.
18+
- Some method that allow to read more symbol than they write miss an argument to set the `fill_symbol`.
19+
- `spi_master_transfer` takes a `uint32_t` instead of a function pointer type for its `handler` argument.
20+
- Some method have misleading naming (e.g. `spi_slave_receive` and `spi_slave_read`).
21+
- The slave and master API are "almost" identical. There is no `spi_mater_read` nor `spi_master_receive`.
22+
- `spi_format` should use an enum for the mode.
23+
24+
## Use-cases
25+
26+
In order to provide a meaningful API, this RFC will consider the following use cases.
27+
28+
### Use-cases in Mbed-OS
29+
(click for detailed description)
30+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/sd-driver">sd-driver</a></summary><ul><li>initial freq : 100kHz -> 25+MHz</li><li>clock polarity: low</li><li>clock phase : first edge</li><li>word size : 8</li><li>Chip select : manual (could be it ? Is it fine to release CS between request and response ?).</li><li>eventually also change the bus to half-duplex and its width to dual/quad</li></ul></details>
31+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/dataflash-driver">dataflash-driver</a></summary><ul><li>initial freq : 50 -> 85+MHz</li><li>clock polarity: ? default</li><li>clock phase : ? default</li><li>word size : ? default</li><li>Chip select : manual (could be automated as it is sending/receiving byte by byte).</li></ul></details>
32+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/mbed-os/tree/master/features/unsupported/tests/peripherals/C12832">C12832 LCD screen driver</a> and <a href="https://github.com/ARMmbed/mbed-os/tree/master/features/unsupported/tests/peripherals/ADXL345">ADXL345 3 axis accelerometer</a></summary><ul><li>initial freq : 19.2MHz</li><li>clock polarity: high</li><li>clock phase : second edge</li><li>word size : 8bits</li><li>Chip select : manual (could be automated).</li></ul></details>
33+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/wifi-ism43362">ism43362</a> (WiFi module)</summary><ul><li>initial freq : 20MHz</li><li>clock polarity: low</li><li>clock phase : first edge</li><li>word size : 16bits</li><li>Chip select : manual (could it be automated ? There seem to be some special delays)</li></ul></details>
34+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/atmel-rf-driver">atmel-rf</a> (802.15.4)</summary><ul><li>initial freq : 3.75 -> 7.5MHz</li><li>clock polarity: ? default</li><li>clock phase : ? default</li><li>word size : ? default</li><li>Chip select : manual (could be automated)</li></ul></details>
35+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/stm-spirit1-rf-driver">stm-spirit1-rf</a> (Sub-1 GHz RF based on the SPSGRF-868 Module)</summary><ul><li>initial freq : 10MHz</li><li>clock polarity: low</li><li>clock phase : first edge</li><li>word size : 8bits</li><li>Chip select : manual (could it be automated ? There seem to be some special delays)</li></ul></details>
36+
- <details><summary>SPI Master: <a href="https://github.com/ARMmbed/ble-x-nucleo-idb0xa1">ble-x-nucleo-idb0xa1</a> (BLE module)</summary><ul><li>initial freq : 8MHz</li><li>clock polarity: low</li><li>clock phase : first edge</li><li>word size : 8bits</li><li>Chip select : manual (could be automated as it is sending/receiving byte by byte)</li></ul></details>
37+
- <details><summary>SPI Master: <a href="https://os.mbed.com/cookbook/SPI-communication-with-external-ADC-MCP3">ADC</a></summary><ul><li>initial freq : 1MHz</li><li>clock polarity: low</li><li>clock phase : first edge</li><li>word size : 7bits</li><li>Chip select : manual (could it be automated ? There seem to be some special delays)</li><li>The comment in the code is confusing as it refers to "high steady state" and "second edge capture".</li></ul></details>
38+
39+
### Other use cases
40+
- SPI Slave
41+
In this use case the user wants to receive a command frame and send back an answer frame.
42+
Both frames are constructed that way :
43+
44+
| size | frame type | variable length frame | crc
45+
| --- | --- | --- | ---
46+
| 2 bytes | 1 byte | \<size> bytes | 1 byte
47+
48+
Data are sent as 8 bit symbols, little endian, lsb first. The Frame cannot exceed 2048bytes in length.
49+
CS has to stay asserted between request and response.
50+
The master expect to receive the symbol `0xFF` as a place holder.
51+
- Simultaneous transaction over multiple SPI peripheral.
52+
For example: Reading/Writing to an SD Card while using the communication interface over another SPI interface (other peripheral).
53+
- Asynchronous:
54+
These are basically the same the regular ones except that the function call should return immediately and a callback should be triggered once the operation is completed.
55+
- SPI Master block transfer
56+
- SPI Slave block transfer : Not supported in current API
57+
58+
The asynchronous API is particularly important when dealing with multiple interfaces. Indeed, handling simultaneously two SPI transfers would require two thread with a blocking API while only one is required by the asynchronous API saving kilobytes of RAM.
59+
60+
61+
## API Changes
62+
63+
| Old | New | Motivation
64+
| --- | --- | ---
65+
| `spi_format(spi, bits, mode, slave)` | `spi_format(spi, bits, mode, slave, bit_ordering)` | Add the bit ordering as requested by [#4789](https://github.com/ARMmbed/mbed-os/issues/4789)
66+
| `spi_format(..., int bits, ...)` | `spi_format(..., uint8_t bits, ...)` | `bits` becomes an unsigned 8 bits integer as the value is in the range \[1; 32].
67+
| `spi_format(..., int mode, ...)` | `spi_format(..., spi_mode_t mode, ...)` | SPI mode becomes an enum to sanitize the "magic integer"
68+
| `spi_format(..., int slave, ...)` | `spi_init(..., bool is_slave, ...)` | `slave` becomes a boolean called `is_slave` as it is only a binary value.</br>This argument is also moved to `spi_init()` because it is required to determine if `SS` should be ignored or not.
69+
| `spi_frequency(..., int hz)` | `spi_frequency(..., uint32_t hz)` | `hz` becomes unsigned as a negative does not make sense in this context.
70+
| ̀`void spi_frequency(...)` | `uint32_t spi_frequency(...)` | ̀`spi_frequency()` now returns the actual frequency that the peripheral will be generating to allow a user adjust its strategy in case the target cannot be reached.
71+
| `uint8_t spi_get_module(spi_t *)` | `SPIName spi_get_module(spi_pin_t *)` | This change will ease tracking of SPI instanced by using a know unique identifier for each peripheral.</br>This will be used by the Driver to Identify the various peripheral and allow multiple SPI transfer on different SPI peripherals.</br>The type change allows to call `spi_get_module()` before calling `spi_init()`
72+
| - | `spi_get_capabilities(...)` | This method is introduce to help select, configure and run test appropriate to the tested target's peripheral.
73+
| - | `#define SPI_COUNT (___)` | Introduce a macro defined in a header to tell the driver how many peripheral are available on this target. This information will be used by the driver to allocate a global mutex and owner table (one per peripheral).
74+
| `spi_slave_*` and `spi_master_*` | only `spi_*` | Unifying `spi_slave` a `spi_master` API to provide a single way to interact with the peripheral.
75+
| `spi_master_transfer(...)` | renamed to `spi_transfer_async(...)` | This change is to explicitly state that this method is meant for async operations.
76+
| `spi_master_block_write(...)` | renamed to `spi_transfer(...)` | This change is reflect more accurately what the method is actually doing.</br>`_block_write` only implies emission where the in fact that it may also receive data.
77+
| `spi_transfer_async(...)` and `spi_transfer(...);` | changes in arguments list | They now share the same list of arguments (`_async` as some more to handle the asynchronous part) :</br>`spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol`
78+
| `spi_transfer_async(..., uint32_t handler, ...)` | becomes `spi_transfer_async(..., spi_async_handler_f handler)`</br> with `void (*spi_async_handler_f)(spi_t *obj, void *ctx, spi_async_event_t event)` | This change sanitizes the function pointer
79+
| - | introduce `void *ctx` as an argument of `spi_transfer_async` | This argument shall be passed to the invocation of the event handler.
80+
| `void spi_transfer_async(...)` | `bool spi_transfer_async(...)` | `spi_transfer_async(...)` now returns a true if the transfer was scheduled and false otherwise.</br>E.g.: if the peripheral is already busy with another transfer.
81+
| `spi_transfer_async(..., uint32_t event, ...)` | argument removed | the callback will now be invoked on any event with the event as an argument.
82+
| `spi_busy(spi)` | removed | This method is never used outside of some of the hal implementations.
83+
| `spi_master_write(...)` `spi_slave_write(...)` `spi_slave_read(...)` `spi_slave_receive(...)` | removed | Writing data symbol by symbol is very inefficient and should not be encouraged. It can still be achieved by passing a pointer to the symbol to `spi_transfer`.
84+
| `spi_active(spit_t *obj)` | removed | as the async call back is now always invoked on async operation termination (unless cancelled), this status can be tracked from driver layer without any hal request.
85+
| `uint32_t spi_irq_handler_asynch(spi_t *obj)` | removed | as the event is now passed as an argument to the callback this method is no longer required.
86+
87+
### The new API
88+
89+
```c
90+
typedef struct {
91+
/** Minimum frequency supported must be set by target device and it will be assessed during
92+
* testing.
93+
*/
94+
uint32_t minimum_frequency;
95+
/** Maximum frequency supported must be set by target device and it will be assessed during
96+
* testing.
97+
*/
98+
uint32_t maximum_frequency;
99+
/** Each bit represents the corresponding word length. lsb => 1bit, msb => 32bit. */
100+
uint32_t word_length;
101+
bool support_slave_mode; /**< If true, the device can handle SPI slave mode. */
102+
} spi_capabilities_t;
103+
104+
typedef enum _spi_mode_t {
105+
SPI_MODE_IDLE_LOW_SAMPLE_FIRST_EDGE,
106+
SPI_MODE_IDLE_LOW_SAMPLE_SECOND_EDGE,
107+
SPI_MODE_IDLE_HIGH_SAMPLE_FIRST_EDGE,
108+
SPI_MODE_IDLE_HIGH_SAMPLE_SECOND_EDGE,
109+
} spi_mode_t;
110+
111+
typedef enum _spi_bit_ordering_t {
112+
SPI_BIT_ORDERING_MSB_FIRST,
113+
SPI_BIT_ORDERING_LSB_FIRST,
114+
} spi_bit_ordering_t;
115+
116+
typedef void (*spi_async_handler_f)(spi_t *obj, void *ctx, spi_async_event_t event);
117+
118+
SPIName spi_get_module(PINName MISO, PINName MOSI, PINName MCLK);
119+
void spi_get_capabilities(SPIName name, PINName SS, spi_capabilities_t *cap);
120+
121+
void spi_init(spi_t *obj, bool is_slave, PINName MISO, PINName MOSI, PINName MCLK, PINName SS);
122+
void spi_format(spi_t *obj, uint8_t bits, spi_mode_t mode, spi_bit_ordering_t bit_ordering);
123+
uint32_t spi_frequency(spi_t *obj, uint32_t hz);
124+
void spi_transfer(spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol);
125+
bool spi_transfer_async(spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol, spi_async_handler_f handler, void *ctx, DMAUsage hint);
126+
void spi_transfer_async_abort(spi_t *obj);
127+
void spi_free(spi_t *obj);
128+
```
129+
130+
## Behaviours
131+
### Defined Behaviours
132+
133+
- `spi_get_module()` returns the `SPIName` unique identifier to the peripheral associated to this SPI channel.
134+
- `spi_get_capabilities()` fills the given `spi_capabilities_t` instance
135+
- `spi_get_capabilities()` should consider the `SS` pin when evaluation the `support_slave_mode` capability.
136+
If the given `SS` pin cannot be managed by hardware in slave mode, `support_slave_mode` should be false.
137+
- At least a symbol width of 8bit must be supported.
138+
- The supported frequency range must include the range [0.2..2] MHz.
139+
- The shortest part of the duty cycle must not be shorter than 50% of the expected period.
140+
- `spi_init()` initializes the pins.
141+
- `spi_init()` ignores the `SS` pin if `is_slave` is false.
142+
- `spi_free()` resets the pins to their default state.
143+
- `spi_free()` disables the peripheral clock.
144+
- `spi_format()` sets :
145+
- the number of bits per symbol
146+
- the mode :
147+
0. Clock idle state is *low*, data are sampled when the clock becomes *active* (polarity = 0, phase = 0)
148+
1. Clock idle state is *low*, data are sampled when the clock becomes *inactive* (polarity = 0, phase = 1)
149+
2. Clock idle state is *high*, data are sampled when the clock becomes *active* (polarity = 1, phase = 0)
150+
3. Clock idle state is *high*, data are sampled when the clock becomes *inactive* (polarity = 1, phase = 1)
151+
- the bit ordering (lsb/msb first).
152+
- `spi_frequency()` sets the frequency to use during the transfer.
153+
- `spi_frequency()` returns the actual frequency that will be used.
154+
- `spi_transfer()` :
155+
- writes `tx_len` symbols to the bus.
156+
- reads `rx_len` symbols from the bus.
157+
- if `rx_len` > `tx_len` then it sends `(rx_len-tx_len)` additional `fill_symbol` to the bus.
158+
- if `rx` is NULL then inputs are discarded.
159+
- if `tx` is NULL then `fill_symbol` is used instead.
160+
- `spi_transter_async()` schedules a transfer to be process the same way ̀`spi_transfer()` would have but asynchronously.
161+
- `spi_transter_async()` returns immediately with a boolean indicating whether the transfer was successfully scheduled or not.
162+
- The callback given to `spi_transfer_async()` is invoked when the transfer completes (with a success or an error).
163+
- `spi_transfer_async_abort()` aborts an on-going async transfer.
164+
165+
### Undefined Behaviours
166+
- Calling `spi_init()` multiple times on the same `spi_t` without `spi_free()`'ing it first.
167+
- Calling any method other than `spi_init()` on a non-initialized or freed `spi_t`.
168+
- Passing both MISO and MOSI as NC to `spi_get_module` or `spi_init`.
169+
Only one may be set as NC for simplex/half-duplex configuration.
170+
- Passing SCLK as NC to `spi_get_module` or `spi_init`.
171+
- Passing an invalid pointer as `cap` to `spi_get_capabilities`.
172+
- Passing pins that cannot be on the same peripheral.
173+
- Passing an invalid pointer as `obj` to any method.
174+
- Giving a `SS` pin to `spi_init()` when using in master mode.
175+
SS must be managed by hardware in slave mode and must **NOT** be managed by hardware in master mode.
176+
- Setting a frequency outside of the range given by `spi_get_capabilities()`.
177+
- Setting `bits` in `spi_format` to a value out of the range given by `spi_get_capabilities()`.
178+
- Passing an invalid pointer as `fill_symbol` to `spi_transfer` and `spi_transfer_async` while they would be required by the transfer (`rx_len != tx_len` or `tx==NULL`).
179+
- Passing an invalid pointer as `handler` to `spi_transfer_async`.
180+
- Calling `spi_transfer_async_abort()` while no async transfer is being processed (no transfer or a synchronous transfer).
181+
182+
## Impact on partners' implementations
183+
184+
The new API does not impact partners much as most of them factorise HAL implementation to family level.
185+
For example :
186+
187+
| Partner | #of&nbsp;Implementation | details |
188+
| --- | --- | --- |
189+
| ST | 1 | - |
190+
| Silicon Labs | 1 | EFM32 family |
191+
| Analog device | 2 | They differ by ~20LoC that enable the ability to set the polarity and phase of the clock. They could most probably be merged as a single implementation. |
192+
| Nordic | 2 | The first one for NRF51, the other for NRF52.<br>The functions calls are from what I have seen the same, this could be for Nordic a good opportunity to unify their SPI HAL implementation and bump NRF51’s SDK version. |
193+
| Atmel | 2 | - one for the cortex-m4<br>- one for the cortex-m0+<br>This seems reasonable. |
194+
| Nuvoton | 4 | implementation for 451, 472 and 480 are similar and look like the evolution of the 451.<br>NANO100's implementation has more "tweaks" but is really similar to the others, they could probably all be merged as 1. |
195+
| Maxim | 6 | There's actually only 3 different implementations as :<br>- MAX32620<br>- MAX32600 = MAX32610<br>- MAX32630 = MAX32625 = MAX32620C |
196+
| NXP | 13 | Most of them differ by the pin mapping that is provided in spi_api.c instead of `PeripheralPins.c` as it is done on most of other targets.<br>This number could be easily reduced to 2, maybe 4 in the very worst case. This would still be a huge maintenance gain for NXP DRY’ing their code base. |
197+
| Freescale | 15 | Some of them are exactly the same. This number could be reduce to something around 5 implementation mostly due to the large number of µC generation (K20/KLXX/MCUXPresso etc). |
198+
199+
The partner the most impacted by these changes is definitely Freescale/NXP but overall, in addition to the few bonus added by this new API, it would give partners' team a good opportunity to overhaul their implementations and factorise the code base.
200+
201+
## Drawbacks
202+
--
203+
204+
## Alternative solutions
205+
If it is decided to expose a peripheral interface instead of a "SPI channel to a slave", all `ss` management can be removed from hal and deferred to the driver (handling `ss` as any other GPIO).
206+
This would also reduce differences between SPI Master and SPI Slave use-cases.
207+
208+
## Questions
209+
210+
- To keep consistency between peripherals in terms of design pattern, do we want to expose a peripheral or a communication channel ?
211+
The current SPI driver implementation has 1 instance per slave while i²c driver implementation has 1 instance per peripheral.
212+
- SPI addresses slave in `spi_init` using the chip select.
213+
- I²C addresses slave in transfer functions using the address.
214+
See also: [#7358](https://github.com/ARMmbed/mbed-os/issues/7358)
215+
- How do we handle half-duplex transfer ?
216+

0 commit comments

Comments
 (0)