Skip to content

rtio: SPI support #56125

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
Merged
merged 6 commits into from
Apr 3, 2023
Merged

rtio: SPI support #56125

merged 6 commits into from
Apr 3, 2023

Conversation

@teburd
Copy link
Contributor

@teburd teburd commented Mar 22, 2023 •
edited
Loading

Supporting SPI using a direct mapping of the possible read/write/transceives to RTIO requests

Opens the possibility up of having truly async spi where any number of contexts may submit transactions from any call context without blocking, such as a sensor fifo triggered gpio interrupt as shown in #55383

Related #22494 for SPI
Related #22498 for SPI
Enables finally resolving #1387 in a nice way

Test results running on the tdk robokit

./scripts/twister -X spi_loopback -c -i -v -p tdk_robokit1 --device-testing --device-serial /dev/ttyTDKRobokit1 -T tests/drivers/spi/spi_loopback --west-runner=jlink --west-flash="--reset-after-load"       ✔  10564  16:11:45 
ZEPHYR_BASE unset, using "/home/tburdick/zephyr/zephyr"
Deleting output directory /home/tburdick/zephyr/zephyr/twister-out
INFO    - Using Ninja..
INFO    - Zephyr version: zephyr-v3.3.0-1592-gfb1c719705f7
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/tburdick/zephyr/zephyr/twister-out/testplan.json

Device testing on:

| Platform     | ID   | Serial device       |
|--------------|------|---------------------|
| tdk_robokit1 |      | /dev/ttyTDKRobokit1 |

INFO    - JOBS: 36
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - 5/8 tdk_robokit1              tests/drivers/spi/spi_loopback/drivers.spi.loopback.internal SKIPPED (runtime filter)
INFO    - 6/8 tdk_robokit1              tests/drivers/spi/spi_loopback/drivers.spi.loopback PASSED (device 1.843s)
INFO    - 7/8 tdk_robokit1              tests/drivers/spi/spi_loopback/drivers.spi.loopback.rtio PASSED (device 1.879s)
INFO    - 8/8 tdk_robokit1              tests/drivers/spi/spi_loopback/drivers.spi.sam_spi_dma.loopback PASSED (device 1.992s)

INFO    - 8 test scenarios (8 test instances) selected, 5 configurations skipped (4 by static filter, 1 at runtime).
INFO    - 3 of 8 test configurations passed (100.00%), 0 failed, 0 errored, 5 skipped with 0 warnings in 14.31 seconds
INFO    - In total 6 test cases were executed, 10 skipped on 1 out of total 554 platforms (0.18%)
INFO    - 3 test configurations executed on platforms, 0 test configurations were only built.

Hardware distribution summary:

| Board        | ID   |   Counter |
|--------------|------|-----------|
| tdk_robokit1 |      |         3 |
INFO    - Saving reports...
INFO    - Writing JSON report /home/tburdick/zephyr/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/tburdick/zephyr/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/tburdick/zephyr/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

@teburd teburd force-pushed the rtio_spi branch 9 times, most recently from 4dde6b2 to d71d98b Compare March 22, 2023 22:06
@teburd teburd marked this pull request as ready for review March 23, 2023 01:22
@zephyrbot zephyrbot added area: SPI SPI bus area: RTIO platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) labels Mar 23, 2023
@teburd teburd mentioned this pull request Mar 23, 2023
Open
19 tasks
yperess
yperess requested changes Mar 23, 2023
View reviewed changes
yperess
yperess previously approved these changes Mar 24, 2023
View reviewed changes
@nandojve
Copy link
Member

nandojve commented Mar 24, 2023

Hi @teburd ,

I was looking and I think this model seems to be very intrusive for me at first. Consider that I'm very green on RTIO and maybe I'm lacking in knowledge too.

Questions:
1- Did you considered create another driver instead mix then? I'm asking this because it could be difficult to scale and maintain. I mean, If you look there is a lot of drivers that don't implement the full API and since you are mixing you may not get the best. In this case, you will be tight to each driver model instead create a exclusive one for RTIO too.

2- Did you considered define a API with focus on DMA? I mean, create universal mechanism that all drivers can consume specific DMA APIs, for instance, pass linked lists. So, when someone implement the peripheral driver the RTIO can be just used it if there is DMA available and API have DMA implementation. In this case, with linked lists it is possible to implement at least batch mode and ping pong and you don't need to change RTIO API for I2C and SPI if driver can direct consume linked lists.

Keep in mind that since you are experimenting it could be good have a dedicated SPI driver at this moment. This will help with the experiment phase without break driver for current users. When we confirm that it is fine we can merge versions.

@teburd
Copy link
Contributor Author

teburd commented Mar 25, 2023 •
edited
Loading

@nandojve thank you for taking a look! Responses inline

Hi @teburd ,

I was looking and I think this model seems to be very intrusive for me at first. Consider that I'm very green on RTIO and maybe I'm lacking in knowledge too.

Questions: 1- Did you considered create another driver instead mix then? I'm asking this because it could be difficult to scale and maintain. I mean, If you look there is a lot of drivers that don't implement the full API and since you are mixing you may not get the best. In this case, you will be tight to each driver model instead create a exclusive one for RTIO too.

I can understand and appreciate the apprehension to mix what is marked here as an experimental feature, which it is only until it starts being used, which I don't think will take long. I'm happy to split this into spi_sam.c (existing driver) and spi_sam_rtio.c the driver in this PR with the SPI_RTIO feature enabled if helpful in moving quickly.

2- Did you considered define a API with focus on DMA? I mean, create universal mechanism that all drivers can consume specific DMA APIs, for instance, pass linked lists. So, when someone implement the peripheral driver the RTIO can be just used it if there is DMA available and API have DMA implementation. In this case, with linked lists it is possible to implement at least batch mode and ping pong and you don't need to change RTIO API for I2C and SPI if driver can direct consume linked lists.

In a way this is exactly an struct rtio_iodev is, its an API that provides the single asynchronous submit function, associated with some data. The associated mpsc queue probably should be moved into something like struct rtio_iodev_context much like spi_context, and its something I intend on doing after this.

Keep in mind that since you are experimenting it could be good have a dedicated SPI driver at this moment. This will help with the experiment phase without break driver for current users. When we confirm that it is fine we can merge versions.

I agree! As mentioned, if it makes it easier to experiment without disturbing existing SAM users I'm all for having two C files, spi_sam.c and spi_sam_rtio.c. Let me know if that's what you'd like to see.

Or I guess another possibility would be to layer these things, maybe that's what you are getting at.

spi_sam_rtio.c implements a simple API for RTIO only, and spi_sam.c calls to it to provide the existing SPI API when its enabled. Maybe this is what you meant in a way and I've misunderstood? I haven't considered that option much but it could be interesting to try!

MaureenHelm
MaureenHelm reviewed Mar 30, 2023
View reviewed changes
MaureenHelm
MaureenHelm previously approved these changes Mar 30, 2023
View reviewed changes
pdgendt
pdgendt reviewed Mar 30, 2023
View reviewed changes
@teburd
rtio: Add tiny write op
1b54c9c 
When sending small buffers out it makes sense to copy rather than
reference to avoid having to keep the small buffer around for the
lifetime of the write request.

Adjusts the op numbers to always be +1 from the previously defined op
id making it easier to re-arrange if needed in the future.

Signed-off-by: Tom Burdick <[email protected]>
@teburd teburd force-pushed the rtio_spi branch 2 times, most recently from 2d06b78 to ca0ecd3 Compare March 30, 2023 19:16
teburd added 2 commits March 30, 2023 14:17
@teburd
rtio: Add callback op
c1f5b18 
Adds a callback op to RTIO enabling C logic to be interspersed with
I/O operations. This is not safe from userspace but allowable in kernel
space.

Signed-off-by: Tom Burdick <[email protected]>
@teburd
rtio: Add transceive op
882d2e9 
Adds the transceive op which is needed for full SPI support. Notably
in RTIO transceive is expected to be a balanced buffer pair of the same
length and have non-null tx/rx bufs.

Signed-off-by: Tom Burdick <[email protected]>
pdgendt
pdgendt previously approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

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

LGTM, some compliance fixes needed though.

teburd added 3 commits March 31, 2023 08:03
@teburd
spi: Add RTIO support to SPI
7f56c82 
Provides a macro and submit API for SPI drivers to support RTIO.

A copy function enables compatibility with the existing blocking API
and very easily the existing async API as well.

Signed-off-by: Tom Burdick <[email protected]>
@teburd
spi: SAM add RTIO support
075f56c 
Implements the SPI RTIO API. Refactors many of the internal transfer
functions to work on simple types to allow for both the RTIO and existing
SPI API functions to co-exist.

Removes the unneeded shift_master specialization. Either a polling or DMA
transfer flow is used when used with RTIO as all operations are normalized.

When SPI_RTIO is enabled the spi_transceive blocking call translates
the request into an RTIO transaction placed in the queue of transactions
the device is meant to do.

Signed-off-by: Tom Burdick <[email protected]>
@teburd
tests: SPI loopback with RTIO
a023c98 
Adds the equivalent spi loopback tests using RTIO and a testplan that
uses it. Adds a tdk robokit1 overlay to enable the NOCACHE option so
that DMA transfers correctly work.

Signed-off-by: Tom Burdick <[email protected]>
@carlescufi carlescufi merged commit 06aa17f into zephyrproject-rtos:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pdgendt pdgendt pdgendt approved these changes

@MaureenHelm MaureenHelm MaureenHelm approved these changes

@yperess yperess yperess left review comments

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@nordic-krch nordic-krch Awaiting requested review from nordic-krch

@attie-argentum attie-argentum Awaiting requested review from attie-argentum

@mnkp mnkp Awaiting requested review from mnkp

@nandojve nandojve Awaiting requested review from nandojve

@stephanosio stephanosio Awaiting requested review from stephanosio

Assignees

@tbursztyka tbursztyka

Labels

area: RTIO area: SPI SPI bus platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@teburd @nandojve @pdgendt @MaureenHelm @yperess @tbursztyka @carlescufi @zephyrbot