Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support #73747

ioannis-karachalios · 2024-06-04T21:07:19Z

This PR requires #73822.

ydamigos · 2024-06-05T12:12:54Z

drivers/spi/spi_smartbond.c

+		__ASSERT(((uint32_t)data->ctx.rx_buf & 0x1) == 0, "Unaligned RX BUF");
+		*(uint16_t *)data->ctx.rx_buf = cfg->regs->SPI_RX_TX_REG;


Could we use sys_put_le16()?

ydamigos · 2024-06-05T12:13:28Z

drivers/spi/spi_smartbond.c

+		__ASSERT(((uint32_t)data->ctx.rx_buf & 0x3) == 0, "Unaligned RX BUF");
+		*(uint32_t *)data->ctx.rx_buf = cfg->regs->SPI_RX_TX_REG;


Could we use sys_put_le32()?

ydamigos · 2024-06-05T12:31:23Z

dts/bindings/spi/renesas,smartbond-spi.yaml

+  tx-dma-channel:
+    type: int
+    description: |
+      DMA channel to be reserved for SPI TX trasmissions.
+      It is imperative that a TX channel be assigned
+      an odd number that is [1, 3, 5, 7].
+
+  rx-dma-channel:
+    type: int
+    description: |
+      DMA channel to be reserved for SPI RX receptions.
+      It is imperative that a RX channel be assigned
+      an even number that is [0, 2, 4, 6].


DA1469x Datasheet version 3.3 mentions in section 23.3.2 Peripheral to Memory Transfer:
Map the peripheral to the selected channels pair (DMA_REQ_MUX_REG[DMAxy_SEL]).

Should we use an array property dma-channel-pair to avoid misuses (e.g. RX DMA channel 0 and TX DMA channel 3)?

DMA channels and controller should be retrieved from the dmas property as commented by @decsny.

ydamigos · 2024-06-05T12:55:02Z

drivers/spi/spi_smartbond.c

+		spi_smartbond_pm_policy_state_lock_put(data);
+#endif
+	}
+	spi_context_cs_control(ctx, false);


we should move it before line 1042, shouldn't we?

In fact, the #CS line should be de-asserted within ISR when all transfers are exercised. I also removed the spi_context_wait_for_completion as this should return immediately when in asynchronous transfers (as we discussed the naming is a bit misleading).

decsny · 2024-06-05T16:24:44Z

dts/bindings/spi/renesas,smartbond-spi.yaml

+      It is imperative that a TX channel be assigned
+      an odd number that is [1, 3, 5, 7].
+
+  rx-dma-channel:


this should be coming from a dmas property, why does smartbond DMA not use #dma-cells?

You are right. DMA channels and controller are now retrieved via DT_INST_DMAS_CELL_BY_NAME and DT_INST_DMAS_CTLR_BY_NAME macros respectively.

tbursztyka · 2024-06-06T08:15:39Z

drivers/spi/spi_smartbond.c

@@ -168,11 +584,6 @@ static int spi_smartbond_configure(const struct spi_smartbond_cfg *cfg,
 		return -ENOTSUP;
 	}

-	if (spi_cfg->operation & SPI_MODE_LOOP) {


Why removing this? (It does not seem to belong to what is being done in this commit.)

Loop operations should now be supported. Should I create a separate commit for this one?

Either another commit or describe in the commit message why it is now available.

That's actually the information I am missing. it's now supported but how? because of DMA or ?
The loop mode is a test mode where a controller can send data to itself, internally, without requiring to short physically MISO/MOSI lines.

Updated. I was under the impression the loop mode can also reflect physical shorting of MISO/MOSI.

tbursztyka · 2024-06-06T08:24:10Z

drivers/spi/spi_smartbond.c

+		spi_context_buffers_setup(ctx, tx_bufs, rx_bufs, data->dfs);
+		spi_context_cs_control(ctx, true);
+
+#ifndef CONFIG_SPI_SMARTBOND_DMA


Are you sure you want to, at built time, enable DMA or interrupt mode as exclusive mode?

What if a controller has a DMA configuration in DTS, but another one has not?

Check how spi_ll_stm32.c does this.

What if a controller has a DMA configuration in DTS, but another one has not?

Fixed, COND_CODE_1 is used to check if dmas cells are defined in DTS.

Are you sure you want to, at built time, enable DMA or interrupt mode as exclusive mode?

I get the point. Since the DMA approach is actually a blocking operation, it should be bound to the blocking API. At the same time non-blocking APIs are available using the interrupt-driven approach. Will update it and let you know once this is done.

it's not a blocking/non-blocking issue (DMA is interrupt based also), I was not clear enough describing the issue I see. It's because you may have 2 SPI controllers defined in DTS, one with DMA and the other without. The issue with your first approach was that you excluded this possibility as soon as the config option for DMA is enabled.

SPI_ASYNC it not tightened to interrupt mode actually, it's enabling part of the SPI API enabling the user to not block on the transceive call.
You can have a blocking transceive call, but still running in interrupt mode. Actually and preferably, all calls should be running under interrupt mode.

Thanks for the thorough analysis provided. It's now clear to me.

ydamigos · 2024-06-07T13:32:07Z

drivers/spi/spi_smartbond.c

+	struct spi_smartbond_data *data = dev->data;
+#endif
+
+	if ((current_mode != mode) SPI_SMARTBOND_DMA_REQ_WORD_ACCESS) {


Suggested change

if ((current_mode != mode) SPI_SMARTBOND_DMA_REQ_WORD_ACCESS) {

if ((current_mode != mode)

#ifdef CONFIG_SPI_SMARTBOND_DMA

|| (data->dfs == 4)

#endif

) {

ydamigos · 2024-06-07T13:36:02Z

drivers/spi/spi_smartbond.c

+#ifdef CONFIG_SPI_SMARTBOND_DMA
+		/*
+		 * Workaround for the controller that cannot generate DMA requests
+		 * for 4-byte bus length or when in RX FIFO mode.
+		 */
+		if ((data->dfs == 4) || (mode == SPI_SMARTBOND_FIFO_MODE_RX_ONLY)) {
+			mode = SPI_SMARTBOND_FIFO_NONE;
+		}
+#endif


Do we need to update comment here?

ydamigos · 2024-06-07T13:44:08Z

drivers/spi/spi_smartbond.c

+	if (data->transfer_mode != transfer_mode_new) {
+		data->transfer_mode = transfer_mode_new;
+	}


Suggested change

if (data->transfer_mode != transfer_mode_new) {

data->transfer_mode = transfer_mode_new;

}

data->transfer_mode = transfer_mode_new;

ydamigos · 2024-06-10T10:59:50Z

drivers/spi/spi_smartbond.c

+		cfg->regs->SPI_CTRL_REG &= ~SPI_SPI_CTRL_REG_SPI_ON_Msk;
+
+#ifdef CONFIG_SPI_SMARTBOND_DMA
+		/*
+		 * Workaround for the controller that cannot generate DMA requests
+		 * for 4-byte bus length.
+		 */
+		if (data->dfs == 4) {
+			mode = SPI_SMARTBOND_FIFO_NONE;
+		}
+#endif
+
+		cfg->regs->SPI_CTRL_REG =
+			((cfg->regs->SPI_CTRL_REG &= ~SPI_SPI_CTRL_REG_SPI_FIFO_MODE_Msk) |
+				((mode << SPI_SPI_CTRL_REG_SPI_FIFO_MODE_Pos) &
+					SPI_SPI_CTRL_REG_SPI_FIFO_MODE_Msk));
+
+
+		if (mode != SPI_SMARTBOND_FIFO_NONE) {
+			cfg->regs->SPI_CTRL_REG &= ~SPI_SPI_CTRL_REG_SPI_DMA_TXREQ_MODE_Msk;
+		} else {
+			cfg->regs->SPI_CTRL_REG |= SPI_SPI_CTRL_REG_SPI_DMA_TXREQ_MODE_Msk;
+		}
+
+		if (is_enabled) {
+			cfg->regs->SPI_CTRL_REG |= SPI_SPI_CTRL_REG_SPI_ON_Msk;


We could avoid writing to register so many times, couldn't we?

This commit should deal with adding support for asynchronous operations. It also adds support for DMA acceleration via a Kconfig variable (enaled by default as DMA should be considered scales faster than the interrupt-driven approach). Signed-off-by: Ioannis Karachalios <[email protected]>

ioannis-karachalios · 2024-06-12T08:06:06Z

@tbursztyka, any other update from your side? We need this PR to be merged till this Friday.

No response while the requests are addressed.

ioannis-karachalios · 2024-06-13T21:32:38Z

@tbursztyka, the SPI acceleration support is quite important for us in the upcoming LTS release and we cannot miss the deadline. Please feel free to open an issue ticket if more work should be done and it will be addressed as bug during the RC2 phase. Sorry for the inconvenience and thanks for your understanding.

ioannis-karachalios marked this pull request as draft June 4, 2024 21:07

zephyrbot added platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond area: DMA Direct Memory Access area: Devicetree Binding PR modifies or adds a Device Tree binding area: SPI SPI bus labels Jun 4, 2024

zephyrbot requested review from andrzej-kaczmarek, blauret, decsny, galak, tbursztyka and teburd June 4, 2024 21:08

zephyrbot assigned andrzej-kaczmarek Jun 4, 2024

ioannis-karachalios requested a review from ydamigos June 4, 2024 21:08

ioannis-karachalios force-pushed the da1469x-spi-async-support branch 4 times, most recently from 03835fb to 294e3ef Compare June 4, 2024 21:47

ioannis-karachalios marked this pull request as ready for review June 4, 2024 21:55

ioannis-karachalios force-pushed the da1469x-spi-async-support branch 3 times, most recently from 89bd785 to c376944 Compare June 5, 2024 04:54

ydamigos requested changes Jun 5, 2024

View reviewed changes

decsny reviewed Jun 5, 2024

View reviewed changes

decsny mentioned this pull request Jun 5, 2024

Renesas smartbond DMA DT does not use dma-cells #73803

Closed

ioannis-karachalios force-pushed the da1469x-spi-async-support branch 2 times, most recently from 794a465 to 322dc36 Compare June 6, 2024 08:05

tbursztyka previously requested changes Jun 6, 2024

View reviewed changes

ioannis-karachalios requested review from decsny and ydamigos June 6, 2024 08:30

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from 322dc36 to d0b7eac Compare June 6, 2024 10:55

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from 8c9a437 to 3e061d6 Compare June 6, 2024 23:48

ioannis-karachalios changed the title ~~Renesas: Smartbond: Add SPI ASYNC Support~~ Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support Jun 7, 2024

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from 3e061d6 to fefd3e3 Compare June 7, 2024 09:28

ioannis-karachalios requested a review from tbursztyka June 7, 2024 09:28

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from fefd3e3 to 642b449 Compare June 7, 2024 10:53

ydamigos requested changes Jun 7, 2024

View reviewed changes

decsny removed the area: Devicetree Binding PR modifies or adds a Device Tree binding label Jun 7, 2024

decsny removed their request for review June 7, 2024 17:24

ioannis-karachalios requested a review from decsny June 7, 2024 17:39

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from 642b449 to ec6cd4b Compare June 7, 2024 17:54

ioannis-karachalios requested a review from ydamigos June 7, 2024 17:56

ioannis-karachalios force-pushed the da1469x-spi-async-support branch 5 times, most recently from 872f3d4 to b564bce Compare June 9, 2024 20:29

ydamigos requested changes Jun 10, 2024

View reviewed changes

ioannis-karachalios force-pushed the da1469x-spi-async-support branch 2 times, most recently from f8c417e to f9b4466 Compare June 10, 2024 15:03

ioannis-karachalios requested a review from ydamigos June 10, 2024 15:03

ioannis-karachalios force-pushed the da1469x-spi-async-support branch from f9b4466 to d8060ed Compare June 10, 2024 15:05

ydamigos approved these changes Jun 11, 2024

View reviewed changes

blauret approved these changes Jun 11, 2024

View reviewed changes

ioannis-karachalios added this to the v3.7.0 milestone Jun 12, 2024

andrzej-kaczmarek approved these changes Jun 12, 2024

View reviewed changes

nashif merged commit 774ed60 into zephyrproject-rtos:main Jun 14, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support #73747

Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support #73747

ioannis-karachalios commented Jun 4, 2024 •

edited

Loading

ydamigos Jun 5, 2024

ioannis-karachalios Jun 6, 2024

ydamigos Jun 5, 2024

ioannis-karachalios Jun 6, 2024

ydamigos Jun 5, 2024

ioannis-karachalios Jun 6, 2024

ydamigos Jun 5, 2024

ioannis-karachalios Jun 6, 2024

decsny Jun 5, 2024

ioannis-karachalios Jun 6, 2024

tbursztyka Jun 6, 2024

ioannis-karachalios Jun 6, 2024

tbursztyka Jun 7, 2024

ioannis-karachalios Jun 7, 2024 •

edited

Loading

tbursztyka Jun 6, 2024

ioannis-karachalios Jun 6, 2024

ioannis-karachalios Jun 7, 2024

tbursztyka Jun 7, 2024

ioannis-karachalios Jun 7, 2024

ydamigos Jun 7, 2024

ioannis-karachalios Jun 7, 2024 •

edited

Loading

ydamigos Jun 7, 2024

ioannis-karachalios Jun 7, 2024

ydamigos Jun 7, 2024

ioannis-karachalios Jun 7, 2024

ydamigos Jun 10, 2024

ioannis-karachalios Jun 10, 2024

ioannis-karachalios commented Jun 12, 2024

ioannis-karachalios commented Jun 13, 2024 •

edited

Loading

		__ASSERT(((uint32_t)data->ctx.rx_buf & 0x1) == 0, "Unaligned RX BUF");
		(uint16_t )data->ctx.rx_buf = cfg->regs->SPI_RX_TX_REG;

		__ASSERT(((uint32_t)data->ctx.rx_buf & 0x3) == 0, "Unaligned RX BUF");
		(uint32_t )data->ctx.rx_buf = cfg->regs->SPI_RX_TX_REG;

Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support #73747

Renesas: Smartbond: Add SPI Non-Blocking & DMA Acceleration Support #73747

Conversation

ioannis-karachalios commented Jun 4, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannis-karachalios Jun 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannis-karachalios Jun 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannis-karachalios commented Jun 12, 2024

ioannis-karachalios commented Jun 13, 2024 • edited Loading

ioannis-karachalios commented Jun 4, 2024 •

edited

Loading

ioannis-karachalios Jun 7, 2024 •

edited

Loading

ioannis-karachalios Jun 7, 2024 •

edited

Loading

ioannis-karachalios commented Jun 13, 2024 •

edited

Loading