cpu/stm32/periph_spi: Fix /CS handling #20084
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maribu
requested review from
aabadie,
fjmolinas,
DipSwitch and
vincent-d
as code owners
maribu
added
the
State: WIP
github-actions
bot
added
Platform: ARM
|
I should check if hardware /CS handling still works correctly, before this get's merged 😅 And may also whether this causes regressions in regard to DMA + SPI. |
15 tasks
|
OK, hardware CS handling seems to just work, it is a pain to test with the self-testing shield as the Nucleo-64 don't have the hardware CS pin routed to Arduino pin D10. |
maribu
requested review from
smlng,
leandrolanzieri and
MichelRottleuthner
as code owners
|
Adding So both hardware /CS handling and DMA still work :) |
maribu
added
Type: bug
The CR2 register was only written to if the settings differ from the reset value. This wasn't actually a bug, since it was cleared in `spi_release()` to the reset value again. Still, it looks like a bug, may cause a pipeline flush due to the branch, and increased `.text` size. So let's get rid of this.
kfessel
reviewed
Nov 22, 2023
maribu
force-pushed
the
stm32_spi
branch
2 times, most recently
from
774bdd5
to
86394c4
Compare
maribu
force-pushed
the
stm32_spi
branch
2 times, most recently
from
2fc1fdc
to
4f475e2
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
https://ci.riot-os.org/details/3fbdeae7e0d249b381a652ebd5bf9a3d << some mcu does not provide the spi mode macros |
|
Indeed. It affects STM32 families with no SPI support yet. I prepared the |
This doesn't change the firmware, since for all STM32 MCUs with an SPI driver the register setting in the mode did match the SPI mode number by chance. But for some STM32 MCUs with no SPI driver yet the register layout is indeed different. This will help to provide an SPI driver for them as well.
Previously, the /CS signal was performed by enabling / disabling the SPI peripheral. This had the disadvantage that clock polarity settings where not applied starting with `spi_acquire()`, as assumed by e.g. the SPI SD card driver, but only just before transmitting data. Now the SPI peripheral is enabled on `spi_acquire()` and only disabled when calling `spi_release()`, and the `SPI_CR2_SSOE` bit in the `CR2` register is used for hardware /CS handling (as supposed to).
With only 8 possible prescalers, we can just loop over the values and shift the clock. In addition to being much easier to read, using shifts over divisions can be a lot faster on CPUs without hardware division. In addition an `assert()` is added that checks if the API contract regarding the SPI frequency is honored. If the requested clock is too low to be generated, we should rather have a blown assertion than hard to trace communication errors. Finally, the term prescaler is used instead of divider, as divider may imply that the frequency is divided by the given value n, but in fact is divided by 2^(n+1).
|
https:/RIOT-OS/RIOT/compare/4f475e2ade954e007249e4bc2665542931317b4d..f4729c28ec0f9ea77bfaf4d4b1a56ad0d48fd72d is the diff combining the two force-pushes. The second one fixes a typo in a comment |
|
Thx :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR contains two cleanup commits and one bug fix commit for the STM32 implementation of the
periph_spidriver:/CSsettings are always applied, not when changed to from the default. (Improves readability, reduces ROM size, avoids a potential CPU pipeline flush due to one branch instruction less)spi_clk_tis provided, since the enumeration values are used as-is to write to the hardware register. The default constant values do happen to match the new values, but now it doesn't look like an accident anymore and is more robust in regard to changing register layoutassert()was added to ensure that the clock divider indeed manages to divide the clock down as requested. Some STM32 boards cannot reach 100 kHz (350 kHz is the lowest e.g. the Nucleo-F446RE can get). In the most common cases the caller ofspi_acquire()will request the highest SPI frequency still supported by the hardware, so ignoring this and using a higher clock is not acceptable.Testing procedure
tests/periph/selftest_shieldnow passes on STM32F3 boards (tested withnucleo-f303re)nucleo-f303reand a logic analyzer connected to the SPI pins and hte hardware /CS pin)nucleo-f446reand the selftesting app)nucleo-f303re)drivers/sdcard_spiwon't work on STM32F3 inmaster, but should now with this PR. I haven't tested this for either branch, thoughIssues/PRs references
#20071