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.

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

drivers: spi: RPi Pico PIO SPI code size and byte order. #74117

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

ThreeEights
Copy link
Contributor

@ThreeEights ThreeEights commented Jun 11, 2024

Use minimized PIO code for 3-wire operation.

Input and output buffers are conventionally stored in bus byte order. For 16 and 32 bit transfers, this is effectively big-endian, so txbuf and rxbuf need to be read as such. Those pointers also need to be declared uint8_t * instead of void *.
In addition, tx_count and rx_count are based on dts, and refer to whole transfers (8, 16, or 32 bits), not bytes.

Added rpi_pico.overlay to samples/sensor/magn_polling to demonstrate 32-bit word size, and updated the README.rst to make it independent of the specific sensor.

Fixes #72954

@ThreeEights
Copy link
Contributor Author

Drat and darn: Testing revealed a timing issue in 3-wire/SIO mode. Previously, the TX logic could count on PIO instruction address being at the stalled pull instruction after the FIFO is empty, but that's not true with the refactored PIO code. Due to the asynchronous nature of the PIO vs the CPU clocks, the test would sometimes fail and the RX logic would reset the state machine before the TX had completed. The fix is to wait until the TX FIFO is empty, then sleep the number of clock ticks necessary to shift out all the data before starting the RX stage.

@ThreeEights
Copy link
Contributor Author

@yonsch @soburi - When you have a chance, could you take a look at this pull request? I'd like to ferret out any issues as soon as we can.

soburi
soburi previously approved these changes Jul 2, 2024
@ThreeEights
Copy link
Contributor Author

This fix is also needed for the next maintenance release of 3.7.

@MaureenHelm MaureenHelm added dev-review To be discussed in dev-review meeting backport v3.7-branch Request backport to the v3.7-branch labels Aug 8, 2024
@MaureenHelm MaureenHelm removed their assignment Aug 8, 2024
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Sensor changes are ok, but would like @yonsch to review the RPi SPI driver changes.

@MaureenHelm MaureenHelm removed the dev-review To be discussed in dev-review meeting label Aug 8, 2024
@ThreeEights
Copy link
Contributor Author

@yonsch - Do you have any further comments on this pull request?

@soburi
Copy link
Member

soburi commented Sep 3, 2024

@yonsch
ping

@MaureenHelm MaureenHelm added the backport v3.6-branch Request backport to the v3.6-branch label Sep 4, 2024
@MaureenHelm
Copy link
Member

@MaureenHelm @yonsch - This also needs the backport v3.6-branch label. (Am I allowed to add labels?)

Added.

@ThreeEights
Copy link
Contributor Author

@yonsch - Please take a look when you get a chance and see if the changes to drivers/spi/spi_rpi_pico_pio.c meet your approval. I took another look at the problem and redid the wait loop between the send and receive in 3-wire mode.
For those trying to follow along: The issue is that the CPU has no way of knowing when the PIO's state machine has finished clocking out the sent data. Since the SPI clock is typically running at a slower speed than the CPU (and thus PIO) clocks, some delay is needed to prevent the CPU from disabling the state machine (in order to switch to receive state) before the SPI clock is finished. My solution was to include a delay of approximately one SPI clock cycle, to allow the state machine to complete before being reset.

@ThreeEights
Copy link
Contributor Author

@MaureenHelm @soburi - I had to rebase my changes onto the tip of the main branch for the mainline code. There's one update to samples/sensor/magn_polling/README.rst to fix a merge conflict, and the redone synchronization loop in drivers/spi/spi_rpi_pico_pio.c. Can you please review my changes when you have an opportunity?

drivers/spi/spi_rpi_pico_pio.c Outdated Show resolved Hide resolved
@ThreeEights
Copy link
Contributor Author

@yonsch - Have you had a chance to look at this again?

@ajf58
Copy link
Contributor

ajf58 commented Oct 16, 2024

LGTM.

Whilst a concern has been raised about the use of k_sleep, I'd prefer to have correctly working (but perhaps not optimal) code in main, rather than keeping the status quo of main having a known bug in it. The PR serves as a record of this discussion, and someone could raise a new issue against the use of k_sleep, if required.

@ThreeEights ThreeEights force-pushed the spi-rpi-pico-pio-endian branch 4 times, most recently from 7af5c53 to 7e57bee Compare October 28, 2024 18:12
Use minimized PIO code for 3-wire operation.

Input and output buffers are conventionally stored in bus byte order.
For 16 and 32 bit transfers, this is effectively big-endian, so
txbuf and rxbuf need to be read as such.  Those pointers also need
to be declared uint8_t * instead of void *.
In addition, tx_count and rx_count are based on dts, and refer to
whole transfers (8, 16, or 32 bits), not bytes.

Added rpi_pico.overlay to samples/sensor/magn_polling to demonstrate
32-bit word size, and updated the README.rst to make it independent
of the specific sensor.

Clean up compliance check failures.
Fix typos.
Synchronize 3-wire TX and RX cycles.
Simplify state machine synchronization
Minimize SPI bus delay time in 3-wire mode
Move clock delay to PIO code and remove k_sleep

Signed-off-by: Steve Boylan <[email protected]>
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

Sorry for the late response.
I was busy preparing to speak at the open-source summit in Japan.

LGTM

@nashif nashif merged commit d0aced3 into zephyrproject-rtos:main Nov 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Samples Samples area: Sensors Sensors area: SPI SPI bus backport v3.6-branch Request backport to the v3.6-branch backport v3.7-branch Request backport to the v3.7-branch platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drivers: spi: spi_rpi_pico_pio: Byte order is incorrect
8 participants