spi: rtio: Refactor common SPI RTIO APIs to use across common drivers #77136

ubieda · 2024-08-16T02:51:31Z

Description

This PR is the result of an analysis performed to the SPI RTIO drivers in the tree (spi_sam.c and spi_mcux_lpspi.c), in which common patterns were detected with the objective of extracting them into a separate file (spi_rtio.c) so that effort is reduced when implementing SPI RTIO drivers moving forward.

Note

Only the spi_mcux_spi.c driver has been refactored. spi_sam.c will be refactored on a separate PR.

Changes

Add SPI RTIO module (spi/rtio.h and spi_rtio.c) with common APIs.
Refactor spi_mcux_lpspi.c to adopt the SPI RTIO module.

Testing

Ran spi_loopback test with and without RTIO enabled:

west build -b mimxrt1010_evk tests/drivers/spi/spi_loopback -- -DCONFIG_SPI_RTIO=y -DCONFIG_SPI_MCUX_LPSPI_DMA=n

west build -b mimxrt1010_evk tests/drivers/spi/spi_loopback -- -DCONFIG_SPI_RTIO=n -DCONFIG_SPI_MCUX_LPSPI_DMA=n

Console output (RTIO variant):

*** Booting Zephyr OS build v3.7.0-1159-g28e8df7cb4e8 ***
Running TESTSUITE spi_loopback
===================================================================
START - test_spi_loopback
I: SPI test on buffers TX/RX 0x20204320/0x20204300, frame size = 8
I: SPI test slow config
I: Start complete multiple
I: Passed
I: Start complete loop
I: Passed
I: Start null tx
I: Passed
I: Start half start
I: Passed
I: Start half end
I: Passed
I: Start every 4
I: Passed
I: Start rx bigger than tx
I: Passed
I: Start complete large transfers
I: Passed
I: SPI test fast config
I: Start complete multiple
I: Passed
I: Start complete loop
I: Passed
I: Start null tx
I: Passed
I: Start half start
I: Passed
I: Start half end
I: Passed
I: Start every 4
I: Passed
I: Start rx bigger than tx
I: Passed
I: Start complete large transfers
I: Passed
I: Start complete loop
I: Passed
I: Start complete loop
I: Passed
I: All tx/rx passed
 PASS - test_spi_loopback in 0.318 seconds
===================================================================
TESTSUITE spi_loopback succeeded
Running TESTSUITE spi_loopback_rtio
===================================================================
START - test_spi_loopback_rtio
I: SPI test on buffers TX/RX 0x20200145/0x20204eb6
I: SPI test slow config
I: Start complete multiple
I: Passed
I: Start complete loop
I: Passed
I: Start null tx
I: Passed
I: Start half start
I: Passed
I: Start half end
I: Passed
I: Start every 4
I: Passed
I: SPI test fast config
I: Start complete multiple
I: Passed
I: Start complete loop
I: Passed
I: Start null tx
I: Passed
I: Start half start
I: Passed
I: Start half end
I: Passed
I: Start every 4
I: Passed
I: All tx/rx passed
 PASS - test_spi_loopback_rtio in 0.045 seconds
===================================================================
TESTSUITE spi_loopback_rtio succeeded

------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [spi_loopback]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.318 seconds
 - PASS - [spi_loopback.test_spi_loopback] duration = 0.318 seconds

SUITE PASS - 100.00% [spi_loopback_rtio]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.045 seconds
 - PASS - [spi_loopback_rtio.test_spi_loopback_rtio] duration = 0.045 seconds

------ TESTSUITE SUMMARY END ------

===================================================================
PROJECT EXECUTION SUCCESSFUL

drivers/spi/spi_rtio.c

tbursztyka · 2024-08-16T12:15:35Z

include/zephyr/drivers/spi/rtio.h

@@ -0,0 +1,114 @@
+/*


I don't think this belongs to include/zephyr, it's not a public API as far as I know. Only spi drivers are meant to use it as it generalizes common parts. Thus its proper place should be in drivers/spi/ .

I don't know why it was done this way in i2c, it's anyway not uncommon to see variations in how things are done depending on the domain. For instance spi_rtio_copy ended in the public spi.h where instead, in i2c in ended up in i2c_rtio.h only via its signature (but public as well), even though in both case: it's meant to be used by drivers only.

Well drivers may exist out of tree, I think this goes to a pr @dcpleung has. I generally believe the application and driver used apis should be public, just not necessarily in the same header. So far we have the one header for the application in include but that’s not very supportive of out of tree drivers.

I saw daniel's PR, it's a good idea. So it could go into include/zephyr/drivers/internal/spi.h then

It hasn’t been sorted out where that stuff should go exactly I don’t think, but yeah something along those lines

I saw daniel's PR, it's a good idea. So it could go into include/zephyr/drivers/internal/spi.h then

Can you provide your rationale behind suggesting that path? I don't see how lumping internal header files behind drivers/internal/ is better than drivers/<area>/*.

In this case, specifically, I don't think we'll want to lump all-things SPI (zephyr/drivers/internal/spi.h) in the same file as SPI RTIO (zephyr/drivers/spi/rtio.h).

This is coming from #77361 and a related larger tree wide PR doing much the same

This is coming from #77361 and a related larger tree wide PR doing much the same

Thanks for the link. By looking at the diff + the comment below: It appears this goes along the same lines?

I would say that, in terms of a hierarchical tree, anything under drivers/serial and drivers/uart should be considered internal if they do not need to be exposed outside this "subtree". One can collapse this subtree into a single node and nothing else outside would be affected.

drivers/spi/spi_mcux_lpspi.c

teburd · 2024-08-16T16:42:02Z

drivers/spi/spi_mcux_lpspi.c

+ if (spi_rtio_submit(rtio_ctx, iodev_sqe)) {
+ spi_mcux_iodev_prepare_start(dev);
+ spi_mcux_iodev_start(dev);
+ }
 }

 static void spi_mcux_iodev_complete(const struct device *dev, int status)


The i2c_rtio_complete does much the same thing and returns true/false from what I recall to inform the driver whether more work should be started. Perhaps spi_rtio_complete could be an equivalent here.

That would reduce the code duplication further.

It's possible that i2c_rtio and spi_rtio do almost the same things entirely and could be converged into bus_rtio, with the blocking call wrappers still being done per bus type (as they each have their own call parameters). That sort of thing can be done after this though.

Just a heads-up: I did come up with a spi_rtio_complete() (see further down); it's just that it's very limited as there's no handling of spi_context to perform spi_context_cs_control() calls.

I can take another look to further optimize it.

At a second pass, I find it a bit hard to optimize (without having spi_rtio deal with the spi_context, or change the API signature to diverge from i2c_rtio's). May we explore this further optimization out of scope for this PR?

It's possible that i2c_rtio and spi_rtio do almost the same things entirely and could be converged into bus_rtio, with the blocking call wrappers still being done per bus type (as they each have their own call parameters). That sort of thing can be done after this though.

BTW - I do agree we'd gain a lot from this bus_rtio refactoring.

ubieda · 2024-08-17T12:40:23Z

I've made one push to move spi_rtio_copy to spi_rtio.h

teburd · 2024-08-26T16:31:50Z

Likely needs a rebase and some conflicts fixed for the const tx buf changes

ubieda · 2024-08-27T01:09:32Z

I've rebased this PR. May I get another review @teburd @tbursztyka?

teburd · 2024-09-12T18:54:33Z

There's a new max32 driver that could really benefit from having this work in #78346

ubieda · 2024-09-12T19:33:06Z

There's a new max32 driver that could really benefit from having this work in #78346

Roger - I'll rebase soon to adress conflicts

As a step to make them common code: spi_rtio.c. Verified this refactorization builds and passes spi_loopback, both with CONFIG_SPI_RTIO enabled, as well as disabled. Tested on mimxrt1010_evk. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Does not seem to be required. This allows hiding away the spin lock APIs. Signed-off-by: Luis Ubieda <luisf@croxel.com>

Extracted common SPI RTIO operations from the spi_mcux_lpspi driver into spi_rtio, which should be common across RTIO drivers. Tested with spi_loopback with and without CONFIG_SPI_RTIO. Ran on mimxrt1010_evk. Also, verified the other SPI RTIO driver (spi_sam) is not broken by these changes (tested building for target: robokit1 with the same conditions as above). Signed-off-by: Luis Ubieda <luisf@croxel.com>

To group all common APIs for SPI RTIO. Signed-off-by: Luis Ubieda <luisf@croxel.com>

ubieda · 2024-09-16T16:51:10Z

I've rebased this PR to resolve conflicts. I've re-tested it on hardware and it looks good.

ubieda added area: SPI SPI bus platform: NXP NXP area: RTIO labels Aug 16, 2024

ubieda force-pushed the ubieda/spi-rtio-refactor-common-apis branch 2 times, most recently from 28e8df7 to 19bb5a4 Compare August 16, 2024 06:26

ubieda marked this pull request as ready for review August 16, 2024 06:26

ubieda requested review from mmahadevan108 and dleach02 as code owners August 16, 2024 06:26

zephyrbot added the platform: NXP Drivers NXP Semiconductors, drivers label Aug 16, 2024

zephyrbot requested review from danieldegrasse, dbaluta, decsny, manuargue, MarkWangChinese, tbursztyka and teburd August 16, 2024 06:27

zephyrbot assigned tbursztyka Aug 16, 2024

ubieda mentioned this pull request Aug 16, 2024

RTIO: Add SPI default submit handler #77141

Merged

tbursztyka requested changes Aug 16, 2024

View reviewed changes

teburd reviewed Aug 16, 2024

View reviewed changes

ubieda force-pushed the ubieda/spi-rtio-refactor-common-apis branch from 19bb5a4 to 9b6d09d Compare August 16, 2024 20:11

ubieda requested review from tbursztyka and teburd August 16, 2024 20:15

zephyrbot added the platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) label Aug 17, 2024

zephyrbot requested review from attie-argentum, mnkp, nandojve, pdgendt and stephanosio August 17, 2024 12:40

ubieda mentioned this pull request Aug 17, 2024

spi: sam: Refactor driver to use SPI RTIO common APIs #77190

Draft

ubieda force-pushed the ubieda/spi-rtio-refactor-common-apis branch from 05fba17 to 097ef3b Compare August 27, 2024 01:05

teburd previously approved these changes Aug 27, 2024

View reviewed changes

ubieda added 4 commits September 16, 2024 12:28

spi: spi_mcux_lpspi: Removed spin lock from iodev_start

f628a15

Does not seem to be required. This allows hiding away the spin lock APIs. Signed-off-by: Luis Ubieda <luisf@croxel.com>

spi: rtio: Move spi_rtio_copy to spi_rtio

48146dc

To group all common APIs for SPI RTIO. Signed-off-by: Luis Ubieda <luisf@croxel.com>

ubieda dismissed teburd’s stale review via 48146dc September 16, 2024 16:40

ubieda force-pushed the ubieda/spi-rtio-refactor-common-apis branch from 097ef3b to 48146dc Compare September 16, 2024 16:40

teburd approved these changes Sep 16, 2024

View reviewed changes

tbursztyka approved these changes Sep 16, 2024

View reviewed changes

carlescufi merged commit 00abc5a into zephyrproject-rtos:main Sep 16, 2024
23 checks passed

ubieda deleted the ubieda/spi-rtio-refactor-common-apis branch September 16, 2024 18:31

ubieda mentioned this pull request Sep 16, 2024

driver:s spi: MAX32 add RTIO support and tests: drivers: spi: spi_loopback: Add support for RTIO tests for APARD32690 board #78346

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spi: rtio: Refactor common SPI RTIO APIs to use across common drivers #77136

spi: rtio: Refactor common SPI RTIO APIs to use across common drivers #77136

ubieda commented Aug 16, 2024 •

edited

Loading

tbursztyka Aug 16, 2024

teburd Aug 16, 2024

tbursztyka Aug 27, 2024

teburd Aug 27, 2024

ubieda Aug 27, 2024 •

edited

Loading

teburd Aug 27, 2024

ubieda Aug 27, 2024

teburd Aug 16, 2024

ubieda Aug 16, 2024 •

edited

Loading

ubieda Aug 16, 2024 •

edited

Loading

ubieda commented Aug 17, 2024

teburd commented Aug 26, 2024

ubieda commented Aug 27, 2024

teburd commented Sep 12, 2024

ubieda commented Sep 12, 2024

ubieda commented Sep 16, 2024

spi: rtio: Refactor common SPI RTIO APIs to use across common drivers #77136

spi: rtio: Refactor common SPI RTIO APIs to use across common drivers #77136

Conversation

ubieda commented Aug 16, 2024 • edited Loading

Description

Changes

Testing

tbursztyka Aug 16, 2024

Choose a reason for hiding this comment

teburd Aug 16, 2024

Choose a reason for hiding this comment

tbursztyka Aug 27, 2024

Choose a reason for hiding this comment

teburd Aug 27, 2024

Choose a reason for hiding this comment

ubieda Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

teburd Aug 27, 2024

Choose a reason for hiding this comment

ubieda Aug 27, 2024

Choose a reason for hiding this comment

teburd Aug 16, 2024

Choose a reason for hiding this comment

ubieda Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

ubieda Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

ubieda commented Aug 17, 2024

teburd commented Aug 26, 2024

ubieda commented Aug 27, 2024

teburd commented Sep 12, 2024

ubieda commented Sep 12, 2024

ubieda commented Sep 16, 2024

ubieda commented Aug 16, 2024 •

edited

Loading

ubieda Aug 27, 2024 •

edited

Loading

ubieda Aug 16, 2024 •

edited

Loading

ubieda Aug 16, 2024 •

edited

Loading