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

RP2040 SPI HAL Improvements #222

Merged
merged 6 commits into from
Jul 29, 2024
Merged

RP2040 SPI HAL Improvements #222

merged 6 commits into from
Jul 29, 2024

Conversation

haydenridd
Copy link
Collaborator

@haydenridd haydenridd commented Jul 17, 2024

Relatively minor improvements, but the highlights:

  • Minor renaming for clarity similar to my I2C PR
  • Support for all the different frame formats the peripheral supports
  • Support for all the different bit widths the peripheral supports
  • TX FIFO preloading for performance improvements:

Example transaction before:
RigolDS1

Example transaction with preloading (no gap!):
RigolDS0

  • Removed configuration of GPIO pins in peripheral driver
    • This actually had some immediate benefits, as the way the RP2040 peripheral controls the CSN pin automatically is a bit odd, it toggles CSN high after every byte when a GPIO is configured to .spi function:

RigolDS2
- For the flash chip I tested this driver on, this would actually make it unusable was I forced to use this
- Instead, I manually drive CSN via a GPIO, which works perfectly well
- Not configuring the GPIOs within the peripheral driver gives users of the HAL this kind of flexibility

The full test suite I wrote for this driver can be found here:
https://github.com/haydenridd/microzig/tree/rp2040-spi

Copy link
Contributor

@ikskuh ikskuh left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far. Do we need clock_config to be comptime known. If not, we can remove the comptime config requirement on the apply fn

bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
bsp/raspberrypi/rp2040/src/hal/spi.zig Outdated Show resolved Hide resolved
@haydenridd
Copy link
Collaborator Author

@ikskuh Requested changes applied!

Copy link
Contributor

@ikskuh ikskuh left a comment

Choose a reason for hiding this comment

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

In general, i'm pretty happy now. Can you maybe add an example where you use a non-u8 type for data width?

@haydenridd
Copy link
Collaborator Author

For sure! I'll add an example then merge :)

@haydenridd haydenridd merged commit c780381 into ZigEmbeddedGroup:main Jul 29, 2024
3 checks passed
@haydenridd haydenridd deleted the rp2040-spi-hal-improvements branch July 29, 2024 17:07
EliSauder pushed a commit to EliSauder/microzig that referenced this pull request Aug 27, 2024
- Changed num() to from_instance_number() for clarity
- Added support for all SPI modes + bit widths
- Implemented TX FIFO "pre-loading" for improved performance
- Update example with SPI
- Renamed functions to _blocking for consistency among HALs
- Changed instance access style
- Changed data_width to DataWidthBits to reduce confusion + need for checks
- Other minor odds and ends per PR review
- Added example of using data width other than 8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants