drivers: spi: spi_pico_pio: Add basic support for SPI via PIO #60395

ThreeEights · 2023-07-14T15:10:54Z

Add fundamental feature support for RP2040 PIO SPI peripherals. This commit implements synchronous transfer with 8-bit MSB format. Using PIO allows any GPIO pins to be assigned the roles of CS, CLK, MOSI, and MISO.

Optional features not implemented yet:

Interrupt based transfer
DMA transfer
Slave mode
Varying word size
3-wire SPI support
LSB-first

github-actions

Hello @ThreeEights, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

ThreeEights · 2023-07-14T16:05:23Z

I have a sample based on the zephyr/samples/sensor/bme280 sample that demonstrates using the PIO SPI driver. I thought that ought to be a separate commit, but that requires approval of this commit first. One option is to create a new samples/boards/rpi_pico/spi_pio sample, though the only change is the addition of the rpi_pico.overlay and updating the prj.cfg.
The other option is to just update the existing sample with the .overlay. Please advise on the best steps to include sample code.

carlescufi · 2023-07-14T16:26:55Z

@ThreeEights I am curious, why PIO instead of using the Pico's native SPI IP? Regarding your sample questions, @yonsch should be able to answer that.

ThreeEights · 2023-07-14T17:18:46Z

@carlescufi - There are 3 main reasons for adding PIO SPI support, in addition to the two built-in SPI devices:

Any arrangement of GPIO pins can be used instead of the pre-defined pin assignments.
More than 2 SPI devices can be supported, up to 4.
But the biggest reason is that SPI support through PIO is required to work with the CYW43439 chip on the Raspberry Pi Pico W. The hard-wired configuration on that board is not supported by the built-in SPI blocks.

ThreeEights · 2023-07-14T18:53:50Z

@iocapa - Would you be willing to join the review for this pull request?
Thanks,
Steve

ThreeEights · 2023-07-17T21:14:07Z

@fabiobaltieri - I saw you triggered a re-run of the Coding Guidelines checks, but that failed due to apt-get not being able to access the Ubuntu repositories. Is there a way I can restart the job, or do I need assistance from the build team?

Thanks,
Steve

fabiobaltieri · 2023-07-17T21:20:06Z

@fabiobaltieri - I saw you triggered a re-run of the Coding Guidelines checks, but that failed due to apt-get not being able to access the Ubuntu repositories. Is there a way I can restart the job, or do I need assistance from the build team?

Hey, I retried it again, not sure what level of permissions you need to retry the individual checks but you can always do a dummy git commit --amend and push force again to restart the checks. That'll lose the approvals, but if you don't have any that's fine, if you have some I guess ask the reviewers (or #pr-help on Discord). Good call poking me on it, I wasn't tracking this one.

fabiobaltieri · 2023-07-17T21:27:49Z

Hey while we are at it...

But the biggest reason is that SPI support through PIO is required to work with the CYW43439 chip on the Raspberry Pi Pico W. The hard-wired configuration on that board is not supported by the built-in SPI blocks.

Does this actually work on the pico W? Thought it was some odd single-wire SPI-ish thing.

soburi · 2023-07-16T18:25:55Z

drivers/spi/spi_pico_pio.c

Is it cannot obtain frequencies from clock-control?

@soburi - clock-control is not supported on the Raspberry Pi Pico (or Pico W). Those boards use a fixed clock. However, looking back at this, it makes more sense to just use the clock frequency from the devicetree, so I've updated those functions to use that instead of calling into the Pico SDK and pushed the change to GitHub. Thanks for pointing this out.

ThreeEights · 2023-07-17T22:26:06Z

@fabiobaltieri - Thanks! That run completed successfully.

You asked:

Does this actually work on the pico W? Thought it was some odd single-wire SPI-ish thing.

Not yet. This is a work in progress. I wanted to get this baseline out there for review while adding the other pieces needed for the Pico W.

ThreeEights · 2023-07-18T13:22:44Z

@soburi - Thanks for the pointer. Since clock-control didn't appear in any of the SPI drivers I was using for references, I didn't know about it. (There are so many parts of Zephyr I don't know about!) I'll try getting the system clock speed through the API and, if that succeeds, I'll push an update.

yonsch · 2023-07-19T17:03:50Z

@ThreeEights Thanks for this PR, I will take a look later. In the meantime:

But the biggest reason is that SPI support through PIO is required to work with the CYW43439 chip on the Raspberry Pi Pico W. The hard-wired configuration on that board is not supported by the built-in SPI blocks.

The SPI used for the wifi module is a special one wire version of SPI, which I haven't looked at thoroughly enough to say how different it is from normal SPI. It may be similar enough to share code with notmal SPI, but not necessarily so.

In any case, there's another use for a PIO SPI - the PL022 SPI peripheral only supports MSB first communication. The only way to add LSB first SPI is via PIO. So I think any PIO implementation of SPI should support both.

ThreeEights · 2023-07-19T17:13:58Z

@yonsch - Thanks. So far, other than needing 3-wire support, it looks like the CYW43439 SPI is pretty conventional. And LSB support is on my longer list of features to add, though that will be a number of pull requests in the future. For now, I wanted to get this first version of the driver reviewed while I add the additional features needed to support the Pico W.

ThreeEights · 2023-07-24T13:38:04Z

@yonsch - Turns out supporting LSB first just involves flipping one bit in a control register. Do you know of any (inexpensive, readily acquired) peripherals that use SPI in LSB mode? Changing the driver is simple, but I don't want to push a change without being able to test it.

Thanks,
Steve

yonsch · 2023-07-24T14:41:27Z

@yonsch - Turns out supporting LSB first just involves flipping one bit in a control register. Do you know of any (inexpensive, readily acquired) peripherals that use SPI in LSB mode? Changing the driver is simple, but I don't want to push a change without being able to test it.

Thanks,
Steve

I think @petejohanson has a keyboard with LSB first SPI

petejohanson · 2023-07-24T14:49:38Z

Adafruit sells sharp memory display breakouts that use the ls0xx driver and are LSB: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/display/ls0xx.c#L306

https://www.adafruit.com/product/3502

Can often be found on Mouser/DigiKey too.

drivers/spi/spi_rpi_pico_pio.c

yonsch · 2023-08-10T19:00:59Z

drivers/spi/spi_rpi_pico_pio.c

Why is this not handled by pinctrl?

Turns out you're right - it IS handled by pinctrl. That code is redundant; I deleted it.

ThreeEights · 2023-08-10T21:48:55Z

@soburi @fabiobaltieri @yonsch - Yet another update to incorporate review comments from @yonsch. Can someone restart the workflows now that the git conflicts have been resolved?

ThreeEights · 2023-08-18T12:55:22Z

@yonsch - Can you please take a look at my changes and let me know if there's anything else required?
@soburi @fabiobaltieri - Can you please review the latest changes?
Thanks!

ThreeEights · 2023-08-21T14:02:33Z

@yonsch - Could you please review the changes made in response to your review comments? Thanks!

ThreeEights · 2023-08-28T21:01:19Z

@yonsch - I'd like to complete this pull request, preferably before another merge conflict shows up. The last response was over 2 weeks ago. This driver makes minimal use of zephyr/drivers/misc/pio_rpi_pico, so it should be possible to quickly adapt to any changes in #56678. I'm going to ask dev-review to look at this PR and get their recommendation.

yonsch

This seems to need a rebase but otherwise looks good

drivers/spi/CMakeLists.txt

Add fundamental feature support for RP2040 PIO SPI peripherals. This commit implements synchronous transfer with 8-bit MSB format. Using PIO allows any GPIO pins to be assigned the roles of CS, CLK, MOSI, and MISO. Optional features not implemented yet: - Interrupt based transfer - DMA transfer - Slave mode - Varying word size - 3-wire SPI support - LSB-first Updated in response to review comments. Further updates from second round of review. Rename spi_pico_pio.c source to match zephyr/MAINTAINERS.yml Remove unnecessary initialization code. Resolve merge conflicts Signed-off-by: Steve Boylan <[email protected]>

github-actions

Hi @ThreeEights!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

ThreeEights requested review from nashif, tbursztyka and yonsch as code owners July 14, 2023 15:10

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: SPI SPI bus platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Jul 14, 2023

zephyrbot requested review from galak, soburi and teburd July 14, 2023 15:11

zephyrbot assigned tbursztyka Jul 14, 2023

github-actions bot reviewed Jul 14, 2023

View reviewed changes

carlescufi requested a review from anangl July 14, 2023 16:12

ThreeEights force-pushed the rpi-pico-spi-pio branch from a244c6e to 813d35f Compare July 14, 2023 17:10

soburi reviewed Jul 17, 2023

View reviewed changes

ThreeEights force-pushed the rpi-pico-spi-pio branch 2 times, most recently from 0a41457 to bede792 Compare July 19, 2023 13:53

nashif assigned yonsch and unassigned tbursztyka Aug 9, 2023

yonsch requested changes Aug 10, 2023

View reviewed changes

fabiobaltieri removed the DNM This PR should not be merged (Do Not Merge) label Aug 10, 2023

ThreeEights dismissed fabiobaltieri’s stale review via 738d0ae August 10, 2023 21:26

ThreeEights force-pushed the rpi-pico-spi-pio branch from de17a1d to 738d0ae Compare August 10, 2023 21:26

ThreeEights requested a review from yonsch August 11, 2023 13:16

ThreeEights force-pushed the rpi-pico-spi-pio branch 4 times, most recently from b8e6563 to 5fec1a6 Compare August 11, 2023 20:22

fabiobaltieri previously approved these changes Aug 18, 2023

View reviewed changes

soburi previously approved these changes Aug 18, 2023

View reviewed changes

ThreeEights dismissed stale reviews from soburi and fabiobaltieri via 0f1cb89 August 28, 2023 20:46

ThreeEights force-pushed the rpi-pico-spi-pio branch from 95ad4a6 to 6d91c10 Compare August 29, 2023 14:08

yonsch requested changes Aug 29, 2023

View reviewed changes

drivers/spi/CMakeLists.txt Outdated Show resolved Hide resolved

ThreeEights force-pushed the rpi-pico-spi-pio branch from 6d91c10 to 4704ba7 Compare August 29, 2023 21:55

fabiobaltieri approved these changes Aug 29, 2023

View reviewed changes

yonsch approved these changes Sep 1, 2023

View reviewed changes

soburi approved these changes Sep 1, 2023

View reviewed changes

carlescufi merged commit 85cbc7a into zephyrproject-rtos:main Sep 1, 2023

github-actions bot reviewed Sep 1, 2023

View reviewed changes

ThreeEights deleted the rpi-pico-spi-pio branch September 1, 2023 15:10

ThreeEights mentioned this pull request Jan 30, 2025

Grant Triage permission to Contributor Steve Boylan #84933

Closed

drivers: spi: spi_pico_pio: Add basic support for SPI via PIO #60395

drivers: spi: spi_pico_pio: Add basic support for SPI via PIO #60395

Uh oh!

Conversation

ThreeEights commented Jul 14, 2023

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

ThreeEights commented Jul 14, 2023

Uh oh!

carlescufi commented Jul 14, 2023

Uh oh!

ThreeEights commented Jul 14, 2023

Uh oh!

ThreeEights commented Jul 14, 2023

Uh oh!

ThreeEights commented Jul 17, 2023

Uh oh!

fabiobaltieri commented Jul 17, 2023

Uh oh!

fabiobaltieri commented Jul 17, 2023

Uh oh!

soburi Jul 16, 2023

Choose a reason for hiding this comment

Uh oh!

ThreeEights Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

ThreeEights commented Jul 17, 2023

Uh oh!

ThreeEights commented Jul 18, 2023

Uh oh!

yonsch commented Jul 19, 2023

Uh oh!

ThreeEights commented Jul 19, 2023

Uh oh!

ThreeEights commented Jul 24, 2023

Uh oh!

yonsch commented Jul 24, 2023

Uh oh!

petejohanson commented Jul 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yonsch Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

ThreeEights Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

ThreeEights commented Aug 10, 2023

Uh oh!

ThreeEights commented Aug 18, 2023

Uh oh!

ThreeEights commented Aug 21, 2023

Uh oh!

ThreeEights commented Aug 28, 2023

Uh oh!

yonsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants