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

dimitrije-lilic · 2024-09-12T14:03:48Z

Implements the SPIO RTIO API. Refactors internal transcive
fucntios to work with both existing SPI API and RTIO functions.

When SPI_RTIO is enabled the spi_transcieve call translates
the request into an RTIO transaction placed in the queue
that device will execute.

Add overlay for APARD32690 board and add a new test case to
test RTIO functionalities of SPI MAX32 driver.

Signed-off-by: Dimitrije Lilic dimitrije.lilic@orioninc.com

github-actions · 2024-09-12T14:04:30Z

Hello @dimitrije-lilic, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

teburd · 2024-09-12T14:47:48Z

Will take a look, thanks!

I know @ubieda was looking at creating a spi rtio context similar to the i2c rtio context to encompass a lot of the common functionality in #77190 which would likely help avoid some code duplication across drivers

MaureenHelm

Thanks @dimitrije-lilic !

Please have a look at the compliance check, there are some long lines and tabs/spaces issues that need to be fixed.

drivers/spi/spi_max32.c

tests/drivers/spi/spi_loopback/boards/apard32690_max32690_m4.overlay

tests/drivers/spi/spi_loopback/testcase.yaml

ubieda · 2024-09-16T18:41:39Z

FYI - #77136 has been merged and can now be incorporated into this PR!

dimitrije-lilic · 2024-09-17T08:58:53Z

Hi @ubieda, thanks for the update, I'll add those changes to align my PR with your refactors

MaureenHelm · 2024-09-23T15:23:03Z

drivers/spi/spi_max32.c

@@ -287,7 +344,7 @@ static int spi_max32_transceive(const struct device *dev)
 spi_context_update_rx(ctx, 1, len);
 }
 #endif
-
+ spi_spin_unlock(dev, key);


Unlocking a second time?

This was moved before line 346, to not cause issues when interrupt drive

MaureenHelm · 2024-09-23T15:27:31Z

drivers/spi/spi_max32.c

-#define DEFINE_SPI_MAX32(_num) \
- PINCTRL_DT_INST_DEFINE(_num); \
- SPI_MAX32_IRQ_CONFIG_FUNC(_num) \
- static const struct max32_spi_config max32_spi_config_##_num = { \
+#define DEFINE_SPI_MAX32_RTIO(_num) SPI_RTIO_DEFINE(max32_spi_rtio_##_num, \
+ CONFIG_SPI_MAX32_RTIO_SQ_SIZE, \
+ CONFIG_SPI_MAX32_RTIO_CQ_SIZE)
+
+#define DEFINE_SPI_MAX32(_num) \
+ PINCTRL_DT_INST_DEFINE(_num); \
+ SPI_MAX32_IRQ_CONFIG_FUNC(_num) \
+ COND_CODE_1(CONFIG_SPI_RTIO, (DEFINE_SPI_MAX32_RTIO(_num)), ()); \
+ static const struct max32_spi_config max32_spi_config_##_num = { \
 .regs = (mxc_spi_regs_t *)DT_INST_REG_ADDR(_num), \
 .pctrl = PINCTRL_DT_INST_DEV_CONFIG_GET(_num), \
 .clock = DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(_num)), \
 .perclk.bus = DT_INST_CLOCKS_CELL(_num, offset), \
 .perclk.bit = DT_INST_CLOCKS_CELL(_num, bit), \
 MAX32_SPI_DMA_INIT(_num) SPI_MAX32_CONFIG_IRQ_FUNC(_num)}; \
- static struct max32_spi_data max32_spi_data_##_num = {  \
+ static struct max32_spi_data max32_spi_data_##_num = { \
 SPI_CONTEXT_INIT_LOCK(max32_spi_data_##_num, ctx), \
 SPI_CONTEXT_INIT_SYNC(max32_spi_data_##_num, ctx), \
- SPI_CONTEXT_CS_GPIOS_INITIALIZE(DT_DRV_INST(_num), ctx)}; \
- DEVICE_DT_INST_DEFINE(_num, spi_max32_init, NULL, &max32_spi_data_##_num, \
- &max32_spi_config_##_num, PRE_KERNEL_2, CONFIG_SPI_INIT_PRIORITY, \
+ SPI_CONTEXT_CS_GPIOS_INITIALIZE(DT_DRV_INST(_num), ctx) \
+ IF_ENABLED(CONFIG_SPI_RTIO, (.rtio_ctx = &max32_spi_rtio_##_num))}; \
+ DEVICE_DT_INST_DEFINE(_num, spi_max32_init, NULL, &max32_spi_data_##_num, \
+ &max32_spi_config_##_num, PRE_KERNEL_2, CONFIG_SPI_INIT_PRIORITY, \


Some spurious whitespace changes are adding noise to the diff. Can you revert those please?

Should be reverted now

teburd

Driver looks pretty good, maybe missing some coverage for some ops here, and at least to me having a single start transceive function that takes the buffer pointers and a size to transceive would make the most sense to cover both the blocking/non-blocking cases. But this is your driver and you will be maintaining it, so what makes best sense to you is the best option.

teburd · 2024-09-24T14:28:18Z

drivers/spi/spi_max32.c

@@ -242,19 +272,45 @@ static int spi_max32_transceive(const struct device *dev)
 dfs_shift = spi_max32_get_dfs_shift(ctx);

 len = spi_context_max_continuous_chunk(ctx);
+#ifdef CONFIG_SPI_RTIO
+ if (sqe->op == RTIO_OP_TINY_TX) {


missing a switch/case here perhaps to cover RX/TX/TINY_TX/TXRX?

Hi Tom, I agree on this part, its now split to have switch when using RTIO config and old code. I think now its more readable, if you can take a look and let me know if this is ok?

teburd · 2024-09-24T14:29:00Z

drivers/spi/spi_max32.c

+ case RTIO_OP_TX:
+ case RTIO_OP_TINY_TX:
+ case RTIO_OP_TXRX:
+ ret = spi_max32_transceive(dev);


transceive might be a bit cleaner if given the buffer pair to work on, given NULL for tx or rx if unidirection perhaps.

Since initially transceive is able to handle all the cases, i didn't want to change core functionality of the drive, but i think now this is covered with previous comment code update

teburd

Looks good to me!

ubieda

I may be missing something, but I'm under the impression the SPI RTIO is working with sync transfers only?

Please push back if you disagree.

ubieda · 2024-09-26T15:24:52Z

drivers/spi/spi_max32.c

+ if (ret == 0) {
+ spi_max32_iodev_complete(dev, 0);
+ }


Shouldn't this be handled asynchronously, otherwise this is a blocking context?

Hi @ubieda,
I've reviewed what you said here and took another look at spi_mcux_lpspi. I've modified code to do the async part using Interrupts can you please take a look and see if this is ok?

ubieda · 2024-09-26T15:28:12Z

drivers/spi/Kconfig.max32

@@ -23,4 +23,22 @@ config SPI_MAX32_DMA
 help
 Enable DMA support for MAX32 MCU SPI driver.

+if SPI_RTIO


Shouldn't we rely on CONFIG_SPI_MAX32_INTERRUPT?

teburd · 2024-09-26T15:42:24Z

drivers/spi/spi_max32.c

+ data->req.rxData = sqe->txrx.rx_buf;
+ data->req.txLen = len >> dfs_shift;
+ data->req.rxLen = len >> dfs_shift;
+ if (!data->req.rxData) {


This shouldn't be needed actually, op_txrx should guarantee both a tx and rx buffer of equivalent length.

Implements the SPIO RTIO API. Refactors internal transcive fucntios to work with both existing SPI API and RTIO functions. When SPI_RTIO is enabled the spi_transcieve call translates the request into an RTIO transaction placed in the queue that device will execute. Include the latest refacor changes of RTIO. Signed-off-by: Dimitrije Lilic <dimitrije.lilic@orioninc.com>

Add overlay for APARD32690 board and add a new test case to test RTIO functionalities of SPI MAX32 driver. Signed-off-by: Dimitrije Lilic <dimitrije.lilic@orioninc.com>

zephyrbot added platform: ADI Analog Devices, Inc. area: SPI SPI bus labels Sep 12, 2024

zephyrbot requested review from galak, MaureenHelm, microbuilder, ozersa, tbursztyka, teburd and ttmut September 12, 2024 14:04

zephyrbot assigned tbursztyka Sep 12, 2024

teburd mentioned this pull request Sep 12, 2024

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

Merged

dimitrije-lilic force-pushed the spi_max32_rtio_driver_support branch from 5507933 to f2cdf27 Compare September 13, 2024 10:29

MaureenHelm requested changes Sep 13, 2024

View reviewed changes

dimitrije-lilic force-pushed the spi_max32_rtio_driver_support branch 3 times, most recently from 3375387 to 83a268d Compare September 16, 2024 17:44

ubieda self-requested a review September 16, 2024 18:42

dimitrije-lilic force-pushed the spi_max32_rtio_driver_support branch from 83a268d to f006c69 Compare September 17, 2024 15:30

dimitrije-lilic requested a review from MaureenHelm September 18, 2024 17:28

MaureenHelm reviewed Sep 23, 2024

View reviewed changes

teburd reviewed Sep 24, 2024

View reviewed changes

dimitrije-lilic force-pushed the spi_max32_rtio_driver_support branch from f006c69 to 37844f8 Compare September 26, 2024 09:52

dimitrije-lilic requested review from MaureenHelm and teburd September 26, 2024 09:57

teburd previously approved these changes Sep 26, 2024

View reviewed changes

tbursztyka previously approved these changes Sep 26, 2024

View reviewed changes

ubieda requested changes Sep 26, 2024

View reviewed changes

teburd reviewed Sep 26, 2024

View reviewed changes

dimitrije-lilic added 2 commits September 27, 2024 16:52

tests: drivers: spi: spi_loopback: Support RTIO tests for APARD32690

1f1335c

Add overlay for APARD32690 board and add a new test case to test RTIO functionalities of SPI MAX32 driver. Signed-off-by: Dimitrije Lilic <dimitrije.lilic@orioninc.com>

dimitrije-lilic dismissed stale reviews from tbursztyka and teburd via 1f1335c September 27, 2024 14:54

dimitrije-lilic force-pushed the spi_max32_rtio_driver_support branch from 37844f8 to 1f1335c Compare September 27, 2024 14:54

dimitrije-lilic requested review from teburd, ubieda and tbursztyka September 27, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dimitrije-lilic commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

teburd commented Sep 12, 2024 •

edited

Loading

MaureenHelm left a comment

ubieda commented Sep 16, 2024

dimitrije-lilic commented Sep 17, 2024

MaureenHelm Sep 23, 2024

dimitrije-lilic Sep 26, 2024

MaureenHelm Sep 23, 2024

dimitrije-lilic Sep 26, 2024

teburd left a comment

teburd Sep 24, 2024

dimitrije-lilic Sep 26, 2024

teburd Sep 24, 2024

dimitrije-lilic Sep 26, 2024

teburd left a comment

ubieda left a comment •

edited

Loading

ubieda Sep 26, 2024 •

edited

Loading

teburd Sep 26, 2024

dimitrije-lilic Sep 27, 2024

ubieda Sep 26, 2024

teburd Sep 26, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

dimitrije-lilic commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

teburd commented Sep 12, 2024 • edited Loading

MaureenHelm left a comment

Choose a reason for hiding this comment

ubieda commented Sep 16, 2024

dimitrije-lilic commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

ubieda left a comment • edited Loading

Choose a reason for hiding this comment

ubieda Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

teburd commented Sep 12, 2024 •

edited

Loading

ubieda left a comment •

edited

Loading

ubieda Sep 26, 2024 •

edited

Loading

teburd Sep 26, 2024 •

edited

Loading