Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cpu/stm32: Fix periph_spi operation in non-DMA mode #19431

Merged
merged 1 commit into from
Apr 3, 2023
Merged

cpu/stm32: Fix periph_spi operation in non-DMA mode #19431

merged 1 commit into from
Apr 3, 2023

Conversation

maribu
Copy link
Member

@maribu maribu commented Mar 29, 2023

Contribution description

The driver previously failed to reliably clear the RXNE bit, resulting in the next transfer to incorrectly read a stale register value. This was noticed with the SD card SPI driver on an STM32F4, in which the 0xff byte of the previous byte transfer was returned instead of the actual status byte, throwing the SD card driver off the rails.

Testing procedure

Connecting an SD card via SPI to a Nucleo-2F429ZI should now result is almost reliable operation.

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Mar 29, 2023
@github-actions github-actions bot added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Mar 29, 2023
@riot-ci
Copy link

riot-ci commented Mar 29, 2023 •
edited
Loading

Murdock results

✔️ PASSED

b2199bb cpu/stm32: Fix periph_spi operation in non-DMA mode

Success Failures Total Runtime
6882 0 6882 09m:17s

Artifacts

@maribu
Copy link
Member Author

maribu commented Mar 29, 2023

Initializing SPI connected transceivers on our STM32F7 based testbed hardware still works. At least the cc110x driver verifies the configuration it has written on init. So no super obvious regressions.

@benpicco benpicco requested a review from MrKevinWeiss March 30, 2023 01:29
benpicco
benpicco reviewed Mar 30, 2023
View reviewed changes
cpu/stm32/periph/spi.c Outdated Show resolved Hide resolved
benpicco
benpicco approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MrKevinWeiss can you give it a spin on your HiL test setup?

@maribu
cpu/stm32: Fix periph_spi operation in non-DMA mode
b2199bb 
The driver previously failed to reliably clear the RXNE bit, resulting
in the next transfer to incorrectly read a stale register value. This
was noticed with the SD card SPI driver on an STM32F4, in which the
0xff byte of the previous byte transfer was returned instead of the
actual status byte, throwing the SD card driver off the rails.
@maribu maribu force-pushed the cpu/stm32/periph/spi_non_dma branch from a24dfbe to b2199bb Compare March 30, 2023 08:36
@benpicco benpicco added this to the Release 2023.04 milestone Apr 3, 2023
@MrKevinWeiss
Copy link
Contributor

Working on it, this should be in before the hard feature freeze.

@MrKevinWeiss
Copy link
Contributor

Running it on the nucleo-f303re seems to be OK.

==============================================================================
tests_periph_spi                                                              
==============================================================================
tests_periph_spi.Periph Spi Base :: Verify basic functionality of the perip...
==============================================================================
Acquire and Release Should Succeed :: Verify SPI acquire and relea... | PASS |
------------------------------------------------------------------------------
Acquire after Release Should Succeed :: Verify acquiring an SPI bu... | PASS |
------------------------------------------------------------------------------
Double Acquire Should Timeout :: Verify that acquiring a locked SP... | PASS |
------------------------------------------------------------------------------
tests_periph_spi.Periph Spi Base :: Verify basic functionality of ... | PASS |
3 tests, 3 passed, 0 failed
==============================================================================
tests_periph_spi.Periph Spi Transfer :: Verify functionality of transfering...
==============================================================================
Transfer Single Bytes Should Succeed :: Sends a single byte two ti... | PASS |
------------------------------------------------------------------------------
Transfer Multiple Bytes Should Succeed :: Sends 1, 2 and 16 bytes ... | PASS |
------------------------------------------------------------------------------
Transfer Single Reg Should Succeed :: Send a byte to a given regis... | PASS |
------------------------------------------------------------------------------
Transfer Multiple Regs Should Succeed :: Sends 1, 2 and 16 bytes s... | PASS |
------------------------------------------------------------------------------
tests_periph_spi.Periph Spi Transfer :: Verify functionality of tr... | PASS |
4 tests, 4 passed, 0 failed
==============================================================================
tests_periph_spi.Periph Spi Modes :: Verify functionality of all 4 modes of...
==============================================================================
Transfer Bytes Mode 0 Should Succeed :: Transfers Bytes in SPI mod... | PASS |
------------------------------------------------------------------------------
Transfer Bytes Mode 1 Should Succeed :: Transfers Bytes in SPI mod... | PASS |
------------------------------------------------------------------------------
Transfer Bytes Mode 2 Should Succeed :: Transfers Bytes in SPI mod... | PASS |
------------------------------------------------------------------------------
Transfer Bytes Mode 3 Should Succeed :: Transfers Bytes in SPI mod... | PASS |
------------------------------------------------------------------------------
tests_periph_spi.Periph Spi Modes :: Verify functionality of all 4... | PASS |

@MrKevinWeiss
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 3, 2023

Build succeeded:

@bors bors bot merged commit 985a38c into RIOT-OS:master Apr 3, 2023
@maribu maribu deleted the cpu/stm32/periph/spi_non_dma branch April 4, 2023 06:44
@maribu
Copy link
Member Author

maribu commented Apr 4, 2023

Thanks :)

@maribu maribu mentioned this pull request May 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2023.04
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @riot-ci @MrKevinWeiss @benpicco