Add SPI bitwidths to ST targets where supported #14020
Conversation
pea-pod
force-pushed
the
stm-spi-more-bits
branch
from
85772ed to
7bb0392
Compare
|
@pea-pod, thank you for your changes. |
pea-pod
force-pushed
the
stm-spi-more-bits
branch
from
7bb0392 to
fb21e3a
Compare
mergify
bot
dismissed
0xc0170’s stale review
Pull request has been modified.
|
I updated this a moment ago to correct the capabilities structure that gets returned for the @jeromecoutant: Does the ST implementation of the SPI peripheral do "transfers" or bytes? I could not tell with the little I looked, but I will keep looking in the mean time. I might need to do some more updating to do tests for current data widths. |
|
Checked also SPI FPGA test with STM32L4S target: | B_L4S5I_IOT01A-ARMC6 | B_L4S5I_IOT01A | hal-tests-tests-mbed_hal_fpga_ci_test_shield-spi | SPI - symbol size testing (12) | 0 | 1 | FAIL | 0.3 | Note it is not a regression as test was skipped before. |
targets/TARGET_STM/stm_spi_api.c
Outdated
| #if defined(TARGET_STM32F0) || defined(TARGET_STM32F3) || defined(TARGET_STM32F7) || \ | ||
| defined(TARGET_STM32G0) || defined(TARGET_STM32G4) || defined(TARGET_STM32L4) || \ | ||
| defined(TARGET_STM32L5) || defined(TARGET_STM32WB) || defined(TARGET_STM32H7) | ||
| case 4: | ||
| DataSize = SPI_DATASIZE_4BIT; | ||
| break; | ||
| case 5: | ||
| DataSize = SPI_DATASIZE_5BIT; |
There was a problem hiding this comment.
What do you think about code like:
switch (bits) {
#if defined(SPI_DATASIZE_4BIT)
case 4:
DataSize = SPI_DATASIZE_4BIT;
break;
#endif
#if defined(SPI_DATASIZE_4BIT)
case 5:
DataSize = SPI_DATASIZE_5BIT;
break;
#endif
...
There was a problem hiding this comment.
It's a little more verbose on the #if flags, but I think it follows your general style better. Plus, it has the benefit of not having to port in new families when they hit, assuming that the semantics remain the same.
While looking at #13941 regarding the default output for async transfers, I noticed that there was at least one place where the count passed to the structure was transfers and not bytes. I will look for those locations and update them similarly.
targets/TARGET_STM/stm_spi_api.c
Outdated
| #if defined(TARGET_STM32F0) || defined(TARGET_STM32F3) || defined(TARGET_STM32F7) || \ | ||
| defined(TARGET_STM32G0) || defined(TARGET_STM32G4) || defined(TARGET_STM32L4) || \ | ||
| defined(TARGET_STM32L5) || defined(TARGET_STM32WB) | ||
| cap->word_length = 0x0000FFF8; // 4 through 16 bit symbols, inclusive | ||
| #elif defined(TARGET_STM32H7) | ||
| cap->word_length = 0xFFFFFFF8; // 4 through 32 bit symbols, inclusive | ||
| #elif defined(TARGET_STM32F1) || defined(TARGET_STM32F2) || defined(TARGET_STM32F4) || \ | ||
| defined(TARGET_STM32L0) || defined(TARGET_STM32L1) |
There was a problem hiding this comment.
Maybe value could be defined in each subfamily in each TARGET_STM32xx/spi_device.h for ex ?
There was a problem hiding this comment.
I would not mind necessarily. However, there is some precedence for putting specific feature items in here. For example on line 332, you find this: #if defined SPI1_BASE, which is then followed by function calls. I understand that this is different from a value, on the other hand, the value is only used here.
I am neutral on this though.
|
2 other comments:
|
Doc should be wrong as FPGA test is verifying 24 and 32 bits... |
|
@jeromecoutant IF that is the cas,e can you send a pull request fixing the comment? |
|
@0xc0170 Would you like for me to include it in this PR? Either way is fine. My proposed change I made locally only was to change it from "4 to 16" (or close enough) to "4 to 32, target dependent". |
|
@pea-pod yes, please do |
|
Once updated, let us know, we can start CI |
pea-pod
force-pushed
the
stm-spi-more-bits
branch
from
fb21e3a to
9badc9f
Compare
|
@0xc0170 I have pushed changes based on @jeromecoutant's comments. @jeromecoutant: On the @0xc0170 and @jeromecoutant: One more urgent issue with the mbed HAL API: The int spi_master_block_write(spi_t *obj, const char *tx_buffer, int tx_length,
char *rx_buffer, int rx_length, char write_fill)
{
struct spi_s *spiobj = SPI_S(obj);
SPI_HandleTypeDef *handle = &(spiobj->handle);
int total = (tx_length > rx_length) ? tx_length : rx_length;
if (handle->Init.Direction == SPI_DIRECTION_2LINES) {
for (int i = 0; i < total; i++) {
char out = (i < tx_length) ? tx_buffer[i] : write_fill;
char in = spi_master_write(obj, out);
...I could be wrong on this, but I do not think this particular problem is specific to ST's mbed SPI HAL implementation only. |
pea-pod
force-pushed
the
stm-spi-more-bits
branch
2 times, most recently
from
28a0cad to
bbe7094
Compare
|
@jeromecoutant I'm sorry, but I managed to not commit a typo fix somehow. I don't know how, but it happened. Anyway, it'll need another restart once the checks clear. Sorry. |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
|
CI restarted |
0xc0170
added
release-type: patch
|
I set this to patch release (target update). |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
@MarceloSalazar Please review this change |
|
This is something that has been addressed in the overhauled SPI API on https://github.com/ARMmbed/mbed-os/blob/feature-hal-spec-spi/hal/spi_api.h. @donatieng What's the status of this feature branch/overhauled API. And if it's still relevant, how do we proceed with this PR ? |
|
@ithinuel If your comment is regarding the PR itself (and not the issue I found that others addressed on the feature branch), I believe my PR should go through. My changes only open up a number of ST targets to additional bitwidths. But it does not change existing behavior. The 16 bit transfer size existed before and already has these issues. This just lets me do things like talk to certain displays without additional pins today through mbed without waiting on a feature branch to hit master. If this is in regards to updating the feature branch somehow, well, I would still prefer to have mine go through so at least more of my Nucleo features are out-of-the-box with mbed. If it is slightly incongruous with the feature branch, I do not think it too far outside where the API is going. The way ST handled it being something other than 8 or 16 was that it picked it. My PR doesn't change that or attempt to fix or work around the API. For what it's worth, I would be willing to help get the SPI spec branch moving forward, but I would ask that this go forward in the meantime, regardless. |
|
Ugh. Sometimes I don't write words well. "The way ST handled it being something other than 8 or 16 was that it picked it." In real English, I mean that ST's HAL implementation of the format function is to force any entry into one of the two values: 8 or 16. My PR doesn't alter this, except you can now pick 4 or 15 on the devices that support it. If the word length is set outside of the bounds, the behavior is the same. In the current system, if it is not 16 bits, it is set to 8. Now you can make it 5 if the family peripheral supports it. If it does not, it will be set to 8. |
0xc0170
added
ready for merge
needs: review
and removed
needs: CI
ready for merge
labels
|
@ithinuel Please let us know what we shall do with this one. |
|
@0xc0170 I don't know, hence the question to @donatieng 😉 |
|
This PR looks good to me, if @LMESTM and @jeromecoutant are happy, I'm happy! Thanks @pea-pod. |
mbedmain
added
release-version: 6.7.0
Release-pending
and removed
release-type: patch
Summary of changes
Adds
#if definedguards with a switch statement to allow all the number of data width values per family, as per each target'sstm32YYxx_hal_spi.hfile.Families not added yet to the list (for example, the WL series) will error out during compilation so that the appropriate values can be set.
Impact of changes
Targets that support data widths besides 8 and 16 are now supported to their maximum number of valid values. 9 bit SPI (used on many display drivers) can now be supported through mbed. Targets that only support 8 and 16 bit SPI will not be changed. One target family even allows for up to 32 bit data widths (the H7 series) and is supported as well.
Migration actions required
None.
Documentation
Updates
SPI.hdoc comment to reflect capability and target dependent nature.Pull request type
Test results
Reviewers
@jeromecoutant