-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] SPI HAL Specification #7671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general questions, I like the clean-up
hal/rfcs/0000-spi-overhaul.md
Outdated
### Inconsistencies | ||
|
||
- The type used in block transfer functions vary between `(const) void*` and `(const) char*`. | ||
- Some method have a parameter to set the width of the symbols while it is part of the `spi_format` arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how it could be (data bits dictate the data type aka width) however in the current api it is separated. application buffer can have diff width than data bits set in peripheral.
Thus would rephrase this to "width in buffers could be removed and use data bits to set the data type" ?
hal/rfcs/0000-spi-overhaul.md
Outdated
void spi_init(spi_t *obj, bool is_slave, PINName MISO, PINName MOSI, PINName MCLK, PINName SS); | ||
void spi_format(spi_t *obj, uint8_t bits, spi_mode_t mode, spi_bit_ordering_t bit_ordering); | ||
uint32_t spi_frequency(spi_t *obj, uint32_t hz); | ||
void spi_transfer(spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fill_symbol
why is this a pointer ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the type used for data (void *).
This also cover future use case of large symbols (32+bits).
hal/rfcs/0000-spi-overhaul.md
Outdated
void spi_format(spi_t *obj, uint8_t bits, spi_mode_t mode, spi_bit_ordering_t bit_ordering); | ||
uint32_t spi_frequency(spi_t *obj, uint32_t hz); | ||
void spi_transfer(spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol); | ||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DMAUsage
can be removed, or we keep it and have DMA later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK the story around DMA is not very clear so it was left here to avoid introducing unexpected breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the many changes that are proposed here, why not remove this ?
hal/rfcs/0000-spi-overhaul.md
Outdated
| `spi_busy(spi)` | removed | This method is never used outside of some of the hal implementations. | ||
| `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`. | ||
| `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. | ||
| `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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how IRQ will work?
As its now, C++ handler calls C to get events and invokes handler if any happened. How would trampoline work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a nice flow diagram, can we add this to the document as well. Just the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
Waiting for the example implementation, consistent with the specification, for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions and comments
hal/rfcs/0000-spi-overhaul.md
Outdated
## Description | ||
|
||
This updates targets the SPI HAL as the first step of a wide plan for all HAL APIs. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This update targets
or
These updates target
?
hal/rfcs/0000-spi-overhaul.md
Outdated
- Some method have misleading naming (e.g. `spi_slave_receive` and `spi_slave_read`). | ||
- The slave and master API are "almost" identical. There is no `spi_mater_read` nor `spi_master_receive`. | ||
- `spi_format` should use an enum for the mode. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 question about SPI API is why there are 2 different Rx and Tx lengths and if this really makes sense to keep those 2 arguments (was previously mentioned in #2599)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know about some devices that require such behaviour. For example the PN512 (an NFC frontend) is one of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the PN512 spec you pointed to, I don' t understand how you will use different Tx and Rx length. Can you explain ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example of the NFC PN512 front end is just a case where you would have 1 byte of padding which means you will use length of N+1 to get N bytes, with a byte padding on the last Tx and a useless byte on the first Rx. But driver doesn't need a different tx_len and rx_len.
If I look at PN512 driver here https://github.com/wsrf2009/contactless-driver/blob/master/mod/src/pn512.c it actually uses a spi_write_then_read API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, having the flexibility of only reading/writing the bytes you need (by skipping some leading/trailing bytes) can help prevent having to carry an extra copy or split a transfer into two or there separate ones.
For instance if I have buffers A and B and want to carry out this transfer:
TX: A0 A1 X
RX: X B0 B1
If TX/RX lengths are the same and I can't skip bytes, I'll have to either:
1 - Copy my buffer A into a three byte long buffer, do the transfer and store results in the same buffer, then copy two bytes to my buffer B
2 - Split my transfer into three transfers
When buffers start being quite big this can be a bit problematic in terms of memory/processing costs. Splitting the transfer has a performance impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@donatieng With the proposed API you would still have to allocated a 3 bytes buffer for reception as there is no "skip" parameter. Is that Ok or should we consider adding this new arguments ?
(( we might also consider packing arguments in a new structures as it would start to be a bit crowded in this arglist )).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience using the current API, I've found that either it's a small transfer, in which case a couple of extra bytes aren't significant, or it's a big transfer, in which case splitting into multiple parts isn't significant.
hal/rfcs/0000-spi-overhaul.md
Outdated
* testing. | ||
*/ | ||
uint32_t maximum_frequency; | ||
/** Each bit represents the corresponding word length. lsb => 1bit, msb => 32bit. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this definition
I thought symbol width was considered earlier - is this something different ?
Is this actually a list of supported widths ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, for example, if the peripheral supports 8 bits, 12bits to 16bits and 32bits, the value should be 0x8000F880
.
hal/rfcs/0000-spi-overhaul.md
Outdated
void spi_format(spi_t *obj, uint8_t bits, spi_mode_t mode, spi_bit_ordering_t bit_ordering); | ||
uint32_t spi_frequency(spi_t *obj, uint32_t hz); | ||
void spi_transfer(spi_t *obj, const void *tx, uint32_t tx_len, void *rx, uint32_t rx_len, const void *fill_symbol); | ||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the many changes that are proposed here, why not remove this ?
hal/rfcs/0000-spi-overhaul.md
Outdated
- At least a symbol width of 8bit must be supported. | ||
- The supported frequency range must include the range [0.2..2] MHz. | ||
- The shortest part of the duty cycle must not be shorter than 50% of the expected period. | ||
- `spi_init()` initializes the pins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is the actual SPI peripheral initialization expected ?
This was already not clear before and still not clear here.
is there a mandatory list of calls to init , format, set_frequency ... before the HW isinitialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the many changes that are proposed here, why not remove this ?
What do you refer to by "this" ?
is there a mandatory list of calls to init , format, set_frequency ... before the HW isinitialized.
I updated the document to add details about that. With the new details, at least one call of each method is required before HW is initialized. However, this is open to discussion as it would also make sense to have spi_init to set up default parameters and then eventually calling spi_format
and spi_frequency
to change specific parameters.
hal/rfcs/0000-spi-overhaul.md
Outdated
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. | ||
|
||
## Drawbacks | ||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well one major drawback I see is to make changes to something stable and working ok - so I would say risk of regressions to be mitigated with good test covearage :-)
hal/rfcs/0000-spi-overhaul.md
Outdated
-- | ||
|
||
## Alternative solutions | ||
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most, if not all, of the MBED drivers and applications I have seen are using the GPIO ...
but the Hw supports using HW SS as well, so maybe few users are relying on it, I don't know :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, ss
hw handling could be optional in master mode but needs to be used in slave mode.
This alternative solution is probably no longer relevant since SPI_COUNT
and spi_get_module
would now allow the C++ driver to determine which peripheral is in use.
We would then be able to have C++ driver creating 1 instance per slave while at HAL level we would only have 1 instance per peripheral.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No drivers use the SPI API SS it because not all platforms support it, meaning it's effectively useless.
It's typically only implemented when the SPI peripheral supports it directly in hardware. If the HAL implementers were required to make the API work using internal GPIO if necessary, then drivers could use it.
hal/rfcs/0000-spi-overhaul.md
Outdated
- SPI addresses slave in `spi_init` using the chip select. | ||
- I²C addresses slave in transfer functions using the address. | ||
See also: [#7358](https://github.com/ARMmbed/mbed-os/issues/7358) | ||
- How do we handle half-duplex transfer ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a 3 wires SPI transfer working fine today in ST - I'd like avoiding breaking this as well
See: https://os.mbed.com/teams/ST-Expansion-SW-Team/code/X_NUCLEO_IKS01A2_SPI3W/
this needs to be verified as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see in the code, even though the component individually supports a SPI3W interface, only the i²c interface is used. The user manual seems to confirm that.
Do you have a driver example or a demo application available on github or mbed.org that could reference in the use-case section of this RFC ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the code that makes use of the 3 wires I think.
https://os.mbed.com/teams/ST-Expansion-SW-Team/code/X_NUCLEO_COMMON_SPI3W/file/e4f6488865d1/DevSPI3W/SPI3W.cpp/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LMESTM. Indeed that is the library that the NUCLEO_IKS01A2_SPI3W depends on.
What I meant is do we have a shield that we could use to test this use case ?
The IKS01A2 (user manual UM2121) only uses the I²C interface for that matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ithinuel OK. Indeed there is a shield but it needs to be wired:
Test application for 3/4-wires SPI using Nucleo-L476RG and STEVAL-MET001V1 LPS22H pressure&temperature sensor board. As the STEVAL-MET001V1 board don't have the arduino connector it should be wire connected to Nucleo board. The GPIO pins to be connected are in the main.cpp file (in order to allow sensor reset the STEVAL power is from Nucleo A0).
hal/rfcs/0000-spi-overhaul.md
Outdated
|
||
| Partner | #of Implementation | details | | ||
| --- | --- | --- | | ||
| ST | 1 | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes only 1 driver common to all devices .. but that took a lot of efforts to stabilize : I am a bit worried about the required efforts needed to implement this proposal - we need to ensure proper non-regression tests are in place !
hal/rfcs/0000-spi-overhaul.md
Outdated
|
||
## Motivation | ||
|
||
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)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API change request does not require a full rework of the API. Do you have other customers / users issues that motivates the changes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ithinuel Similar question about the end-game of this PR to #7426 (comment). |
@cmonr |
@ithinuel - @geky wrote "Our SPI raises and lowers CS every call to write. For some protocols (such as SPI flash), multiple write calls need to be performed while CS is held, otherwise the protocol resets." I suggest adding SPI driver APIs such as the 'read' api below (taken from QSPI driver) will fulfill what @geky suggested, and will no longer make require SPI-Flash Block Device to control CS on its own. /** Write to QSPI peripheral using custom write instruction |
@offirko I am not sure to understand what the read/write api you mentioned. What we can propose is to make hardware management of CS optional. Changes in the driver are out of scope of this RFC as it is only about the HAL API. |
As @ithinuel suggested this is out of scope, but still I would like to mention that it is better to have control of CS with driver using SPI bus (SPI-Flash block / SD-driver) instead of SPI layer. As the requirement and usage is different for each driver. In case of SD driver, along with multiple transfer we need to finish sequence of commands and check state before releasing SPI bus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
@ithinuel ok I will post my feedback. I have also asked Licio who wrote the sensor driver to have a look as well. |
|
That would presumably be handled by you calling |
As far as I can see, the correct and necessary answer is to hold CS during SPI::lock(). Anyone manually controlling the CS must also be calling SPI::lock() at the same time - or the system will break when there's a thread switch and someone else uses the bus. I suspect there may be quite a bit of code that doesn't do this - this is broken, for example:
If a thread switch happens at any point there, and there's another slave on the bus being used by another thread, that other thread will trash your SPI slave cos your chip select is on. The only correct way to write that is:
So it makes sense that the
And for a single transfer the implicit lock would be sufficient anyway:
You only need to put in the explicit locks when chaining multiple transfers that need sustained CS. |
I see an issue with this if we clear CS in every unlock (no problem if counter is maintained for locks and cs = 0 is set only on last unlock)
|
@deepikabhavnani @kjbracey-arm These concerns applies to the driver layer. Maybe should we move that discussion to an appropriate "tracking issue" ? |
Well, we need to make sure the HAL supports anything being considered for SPI.cpp, as it is the HAL's only user. In this case, I think the key points for HAL API from the above are:
|
@kjbracey-arm Would that be ok to:
|
I think that's just too limited to be worthwhile - the need to hold CS on between back-to-back transactions is too common, and I think SPI.cpp should be supporting that (via I'm beginning to think that maybe this is the simplest thing to do - just drop the HAL SS support. Only concern - are there potentially any platforms or boards where the SS pin on an SPI bus /can't/ be controlled via GPIO APIs? Where someone is currently passing SS to SPI.cpp and on to HAL for per-transfer assertion, and that's the only way it can work? |
For the SPI Slave use case, handling the SS "manual" through GPIO will be too slow on small targets and requires to be managed in HW. The question about HW support for SPI is only for the master mode and looks like a "nice to have" feature to me as is does not cost much as all targets reviewed during HW analysis do support hw management for SS. |
Yes, I'm only talking about master mode here. By "do support hw management for SS", you mean the ability to auto-assert during a transfer?
I'm doubtful - if the support was dropped altogether at all in the HAL, we would just change SPI.cpp to drive GPIO instead during transfers, and then we get the "hold it asserted" between transfer option at basically no cost, with no HAL dependency. Software drive is easier, probably gives smaller code as we'll have GPIO code in anyway, and only cost is a little bit of performance, and I guess slightly-less-clean signals on the bus. |
All commit have been squashed together, the documents have been moved to Ready to merge to the feature branch. |
@ithinuel It's PRs like this that make me wish that we had something to indicate when we could skip CI. Unfortunately, because we don't, we need to put this through Jenkins CI... @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test |
@cmonr - this passed CI, but may still need a review (there have been seeral commits since the last reviews) |
Doc changes look fine
I don;t see build triggered on this PR yet (missing something? ) Also it says 10 checks passed. Rest 4 Build, test, exporters.. not in list |
Going to run CI since discussion appear to have settled. /morph build |
Correct. The smallrt CI tests passed (mainly Travis), but Jenkins CI hadn't been started. |
Build : SUCCESSBuild number : 3102 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2709 |
Test : SUCCESSBuild number : 2891 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Interested to see how this pans out.
Description
This PR add the Request For Comment document for the SPI HAL API overhaul.
This document is meant to serve as design document for defining what we plan for the API.
Document readable here : Rendered
Pull request type