Enable LPSPI RTIO Support #62057

EmilioCBen · 2023-08-29T20:42:23Z

Tested with the mimxrt1170_evk_cm7 using the tests/spi/spi_loopback test.

bperseghetti · 2023-09-06T08:55:10Z

Probably should also add the VMU_RT1170 to the test.

teburd

Unclear if this works in an asynchronous (event driven) way as would be expected, some clarification is needed

teburd · 2023-09-06T14:14:26Z

drivers/spi/spi_mcux_lpspi.c

+	rx.count = 1;
+
+	transceive(dev, &dt_spec->config, &tx, &rx, 0, 0, 0);
+	if (ret == 0) {


when would this not be 0? It seems like this implies a blocking flow at all times.

What should happen is the transceive above should be asynchronously started here and upon completion the next transceive started.

Or maybe better is the entire transaction is loaded into a hardware queue and started asynchronously, then upon completion things are marked as such.

The only reason this is a blocking operation in the sam driver for small transfers is that it effectively needs to spin tightly on writing/reading out to the spi tx/rx registers... because of the way the IP works without any sort of sensible hardware FIFO available. Doing that with interrupts enabled caused SPI to behave incorrectly. It's a uniquely sam spi issue. LPSPI shouldn't have any of these problems from what I understand of the IP block.

yes, driver should enable dma and spi async if using rtio and call transceive_dma with asynchronous=1 here instead of regular blocking transceive

edit: looks like this driver was never supported with async+dma, so in this case only async is necessary for RTIO, not necessarily DMA, if that is not the focus of this PR

I’m working on adding polling support as well, so if for small transfers it makes more sense to have a tight poll loop, rtio will be able to support that. It should fix the Sam spi issue as well.

for larger transfers dma or peripheral fifos are of course going to be much better.

decsny · 2023-09-08T17:22:06Z

drivers/spi/Kconfig.mcux_lpspi

@@ -19,4 +19,18 @@ config SPI_MCUX_LPSPI_DMA
 	help


add also default y if SPI_RTIO

and maybe select SPI_ASYNC if SPI_RTIO otherwise it seems like the spi_wait_for_completion will block no matter what

tbursztyka · 2023-09-20T08:48:41Z

drivers/spi/spi_mcux_lpspi.c

+	}
+
+	spi_context_cs_control(&data->ctx, true);
+	spi_context_cs_control(&data->ctx, false);


Why these switch on/off of the CS line?

and why the spi_mcux_configure() call, since it's already called below at line 520 ?

Fidally, how these changes fit with RTIO addition in this driver?

Thank you, I don't know where this came from, I have removed this.

FYI you just pushed and it's not removed dont know if it's intentional

Thank you again, it is now removed.

EmilioCBen · 2023-09-28T12:33:40Z

Probably should also add the VMU_RT1170 to the test.

I have now added this to the testcase.yaml

teburd

There's a race in the submit call that needs correcting

Please carefully look over the way submit is written for the sam spi driver and replicate the structure there for now.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/spi/spi_sam.c#L685

Note that a spin lock is taken (serialized single consumer of the request queue), checks if there's ongoing work. If no work is ongoing then pop and start, otherwise do nothing.

teburd · 2023-09-28T13:05:46Z

drivers/spi/spi_mcux_lpspi.c

+	struct spi_mcux_data *data = dev->data;
+	struct rtio_mpsc_node *next = rtio_mpsc_pop(&data->iodev.iodev_sq);
+
+	if (next != NULL) {


next needs to be serialized in a spin lock as it is in the sam driver

It also needs to verify there's no ongoing current work before popping and starting the next thing on the queue.

I wondered about that, but when the LPSPI driver reaches the transceive function, would this not stop the race condition?
https://github.com/zephyrproject-rtos/zephyr/blob/9686f6e000c746538ec7ab78f6d8abf299d24e95/drivers/spi/spi_mcux_lpspi.c#L488C1-L488C1

I wondered about that, but when the LPSPI driver reaches the transceive function, would this not stop the race condition?

looks like it would not, a lot of data is already read and changed before getting to this

Thank you both, I have fixed this and there should no longer be a race condition.

teburd · 2023-09-28T23:25:13Z

drivers/spi/spi_mcux_lpspi.c

+	int ret = 0;
+
+	switch (sqe->op) {
+	case RTIO_OP_TX:


This seems to be mixing transmit and receive in the same spi_buf setup which looks a bit wrong to me

This should no longer be the case.

teburd · 2023-10-03T20:12:07Z

drivers/spi/spi_mcux_lpspi.c

+						 &transfer);
+	spi_spin_unlock(dev, key);
+	if (status != kStatus_Success) {
+		LOG_ERR("Transfer could not start");


rtio_iodev_sqe_err(txn_head, -EIO); or something likely needed here

understood, added this call.

MaureenHelm

Very cool!

tbursztyka · 2023-11-02T12:05:27Z

drivers/spi/Kconfig.mcux_lpspi

@@ -19,4 +19,18 @@ config SPI_MCUX_LPSPI_DMA
 	help
 	  Enable the SPI DMA mode for SPI instances
 	  that enable dma channels in their device tree node.
+
+if SPI_RTIO


you could have a "depends on SPI_RTIO instead of such if/endif.

the if/endif is more scalable syntax

Can you help me understand the advantage of "depends on" over "if/endif"?
Perhaps I missed something here.

Added Kconfig that switches RTIO Support in the LPSPI driver. Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

Provided APIs to support RTIO with the LPSPI driver. Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

Added an overlay file for mimxrt1170_evk_cm33 to test rtio with the spi_loopback test. Added mimxrt1170_evk_cm7 to the testcase.yml file Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

teburd · 2023-11-13T14:04:17Z

@tbursztyka any remaining issues still? it would be nice to have at least two parts supporting this API to further validate it, and I'd love to use something other than a sam e70 :-) Thanks!

dleach02 · 2023-11-15T21:13:56Z

@tbursztyka, can you return to this PR to see if @EmilioCBen has addressed your change request? We would like to get this merged.

teburd

Needs maintainer approval but refreshing my +1

dleach02 · 2023-11-28T17:40:53Z

@tbursztyka please review the changes to determine if your requests have been dealt with

EmilioCBen requested a review from tbursztyka as a code owner August 29, 2023 20:42

zephyrbot added area: SPI SPI bus platform: NXP NXP labels Aug 29, 2023

zephyrbot requested review from bperseghetti, danieldegrasse, dbaluta, decsny, DerekSnell, dleach02, iuliana-prodan, manuargue, mmahadevan108, PetervdPerk-NXP, teburd and yvanderv August 29, 2023 20:42

zephyrbot assigned tbursztyka Aug 29, 2023

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch 2 times, most recently from c29c69c to 658d54e Compare August 29, 2023 21:30

teburd reviewed Sep 6, 2023

View reviewed changes

decsny reviewed Sep 8, 2023

View reviewed changes

tbursztyka requested changes Sep 20, 2023

View reviewed changes

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch from 658d54e to 40d0310 Compare September 28, 2023 12:32

teburd requested changes Sep 28, 2023

View reviewed changes

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch 3 times, most recently from cb51bde to fdb807f Compare September 28, 2023 15:48

teburd reviewed Sep 28, 2023

View reviewed changes

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch from fdb807f to 066ca93 Compare September 29, 2023 17:08

teburd reviewed Oct 3, 2023

View reviewed changes

MaureenHelm previously approved these changes Oct 4, 2023

View reviewed changes

EmilioCBen dismissed stale reviews from MaureenHelm, bperseghetti, teburd, and danieldegrasse via 9a637d6 October 11, 2023 17:15

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch 2 times, most recently from 9a637d6 to 0a305d6 Compare October 11, 2023 19:26

teburd requested a review from tbursztyka October 12, 2023 00:35

tbursztyka reviewed Nov 2, 2023

View reviewed changes

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch from 0a305d6 to 63de139 Compare November 2, 2023 21:15

EmilioCBen added 3 commits November 3, 2023 13:17

drivers: spi: Kconfig.mcux_lpspi: Added RTIO Kconfig for lpspi

a15bad1

Added Kconfig that switches RTIO Support in the LPSPI driver. Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

drivers: spi: spi_mcux_lpspi: Added RTIO Support for LPSPI

e0eb018

Provided APIs to support RTIO with the LPSPI driver. Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

tests: drivers: spi: spi_loopback: added overlay file for mimxrt1170

25204ef

Added an overlay file for mimxrt1170_evk_cm33 to test rtio with the spi_loopback test. Added mimxrt1170_evk_cm7 to the testcase.yml file Signed-off-by: Emilio Benavente <emilio.benavente@nxp.com>

EmilioCBen force-pushed the Enable_LPSPI_RTIO_Support branch from 63de139 to 25204ef Compare November 3, 2023 18:18

dleach02 requested review from tbursztyka, decsny, MaureenHelm, danieldegrasse and bperseghetti November 15, 2023 21:14

bperseghetti approved these changes Nov 15, 2023

View reviewed changes

teburd approved these changes Nov 28, 2023

View reviewed changes

decsny approved these changes Nov 28, 2023

View reviewed changes

dleach02 approved these changes Nov 28, 2023

View reviewed changes

tbursztyka approved these changes Nov 28, 2023

View reviewed changes

dleach02 merged commit 617cb78 into zephyrproject-rtos:main Nov 28, 2023
20 checks passed

teburd mentioned this pull request Jan 12, 2024

NXP enable RTIO for lpspi on rt. #60543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable LPSPI RTIO Support #62057

Enable LPSPI RTIO Support #62057

EmilioCBen commented Aug 29, 2023

bperseghetti commented Sep 6, 2023

teburd left a comment

teburd Sep 6, 2023 •

edited

Loading

decsny Sep 8, 2023 •

edited

Loading

teburd Sep 20, 2023 •

edited

Loading

decsny Sep 8, 2023

decsny Sep 8, 2023 •

edited

Loading

tbursztyka Sep 20, 2023

EmilioCBen Sep 28, 2023

decsny Sep 28, 2023

EmilioCBen Sep 29, 2023

EmilioCBen commented Sep 28, 2023

teburd left a comment

teburd Sep 28, 2023

EmilioCBen Sep 28, 2023

decsny Sep 28, 2023 •

edited

Loading

EmilioCBen Sep 29, 2023

teburd Sep 28, 2023

EmilioCBen Sep 29, 2023

teburd Oct 3, 2023

EmilioCBen Oct 11, 2023

MaureenHelm left a comment

tbursztyka Nov 2, 2023

decsny Nov 2, 2023

EmilioCBen Nov 6, 2023

teburd commented Nov 13, 2023

dleach02 commented Nov 15, 2023

teburd left a comment

dleach02 commented Nov 28, 2023

Enable LPSPI RTIO Support #62057

Enable LPSPI RTIO Support #62057

Conversation

EmilioCBen commented Aug 29, 2023

bperseghetti commented Sep 6, 2023

teburd left a comment

Choose a reason for hiding this comment

teburd Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

decsny Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

teburd Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decsny Sep 8, 2023 • 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

Choose a reason for hiding this comment

EmilioCBen commented Sep 28, 2023

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

decsny Sep 28, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaureenHelm 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

teburd commented Nov 13, 2023

dleach02 commented Nov 15, 2023

teburd left a comment

Choose a reason for hiding this comment

dleach02 commented Nov 28, 2023

teburd Sep 6, 2023 •

edited

Loading

decsny Sep 8, 2023 •

edited

Loading

teburd Sep 20, 2023 •

edited

Loading

decsny Sep 8, 2023 •

edited

Loading

decsny Sep 28, 2023 •

edited

Loading