drivers: spi: siwx91x: Add siwx91x SPI primary driver #88212

smalae · 2025-04-06T19:10:07Z

Implement SPI primary driver for siwx91x device

jerome-pouiller · 2025-04-07T10:22:30Z

Hello @smalae,
The commit is large enough and touch to several components. Could you split it in atomic parts:

changes in the the clock framework
the SPI driver by itself
the changes in the DT
tests additions

jerome-pouiller · 2025-04-07T10:25:22Z

dts/arm/silabs/siwg917.dtsi

@@ -296,6 +296,17 @@
 			prescaler = <1>;
 			status = "disabled";
 		};
+
+		gspi: spi@45030000 {


I rather use the generic names for the DT labels: spi0: spi@45030000 {

drivers/clock_control/clock_control_silabs_siwx91x.c

drivers/spi/spi_silabs_siwx91x_gspi.c

drivers/clock_control/clock_control_silabs_siwx91x.c

jerome-pouiller · 2025-04-09T16:06:22Z

drivers/spi/Kconfig.silabs_siwx91x_gspi

+if SPI_SILABS_SIWX91X_GSPI
+
+config SPI_SILABS_SIWX91X_GSPI_DMA
+	bool "SIWX91X MCU GSPI DMA Support"


Since this is a sub-option of SPI_SILABS_SIWX91X_GSPI, we can just say DMA Support (that's said, we have to clean up the names of all our drivers)

jerome-pouiller · 2025-04-09T16:08:11Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+#ifdef CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA
+	struct gspi_siwx91x_dma_channel dma_rx;
+	struct gspi_siwx91x_dma_channel dma_tx;
+#endif /* CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA */


The comment is is not required if the condition is obvious.

jerome-pouiller · 2025-04-09T16:12:52Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+	int chan_nb;
+	struct dma_block_config dma_descriptors[CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA_MAX_BLOCKS];
+};
+#endif /* CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA */


You can define this struct unconditionally.

jerome-pouiller · 2025-04-09T16:14:44Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+	int ret;
+#ifdef CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA
+	int channel_filter;
+#endif /* CONFIG_SPI_SILABS_SIWX91X_GSPI_DMA */


You can use __maybe_unused rather than this #ifdef.

jerome-pouiller · 2025-04-09T16:40:11Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+		dma_release_channel(data->dma_tx.dma_dev, data->dma_tx.chan_nb);
+
+		/* Configure RX DMA channel */
+		if (data->dma_rx.chan_nb == GSPI_RX_UDMA_CH_NUM) {


This only works on siwx917. If the GSPI is used in another soc, this value will probably change. This is the job of the DT to provide this value. The driver can't verify it.

jerome-pouiller · 2025-04-09T16:40:49Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+		}
+
+		/* Configure TX DMA channel */
+		if (data->dma_tx.chan_nb == GSPI_TX_UDMA_CH_NUM) {


See my previous comment.

jerome-pouiller · 2025-04-09T16:43:25Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+		}
+
+		transaction_len -=
+			gspi_siwx91x_fill_desc(cfg, desc, NULL, transaction_len, is_tx, dfs);


Nitpick: we prefer to break between the arguments (as you did with gspi_siwx91x_fill_desc()):

transaction_len -= gspi_siwx91x_fill_desc(cfg, desc, NULL, transaction_len, is_tx, dfs);

jerome-pouiller · 2025-04-09T16:45:39Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+
+	if (spi_context_tx_buf_on(&data->ctx)) {
+		if (dfs == 1) {
+			tx_frame = UNALIGNED_GET((uint8_t *)(data->ctx.tx_buf));


UNALIGNED_GET() is not required with uint8_t.

Removed for uint8_t

jerome-pouiller · 2025-04-09T16:46:19Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+	uint16_t rx_frame;
+
+	tx_frame = gspi_siwx91x_next_tx(data, dfs);
+	gspi_siwx91x_send(cfg, (uint32_t)tx_frame);


I think the cast is not required.

tbursztyka · 2025-04-09T18:56:19Z

drivers/spi/spi_silabs_siwx91x_gspi.c

+		ret = gspi_siwx91x_transceive_dma(dev, config);
+	} else if (!asynchronous) {
+		/* Perform synchronous polling transceive */
+		ret = gspi_siwx91x_transceive_polling_sync(dev, &data->ctx);


spi_transceive_async() is not about being irq based (or in reverse: spi_transceive() is not meant to be in polling mode).

The only difference on the 2 is that the _async will return asap, and use a callback to signal when the transfer is done, while the other is just a blocking call: but it does not mention anywhere that it should be polling. This is a common misunderstanding, I guess we'll need no detail that in the api file.

So remove the polling stuff, just use your dma version (an irq based if dma is not available would be nice).

Thank you for the suggestion! I agree that using the DMA version for the sync API is the right approach. I've implemented this change, replacing the default polling mechanism for synchronous operations. However, switching to synchronous with IRQs, instead of DMA, presents a challenge. Large data transfers can trigger a lot of IRQs, which is not an issue when DMA is used. Is it acceptable to retain polling as a fallback for cases where the DMA component isn't available? This will allow users to quickly perform basic driver checks without requiring the DMA component.

Clock driver changes required for initializing the SPI clock for the siwx91x driver Signed-off-by: Sai Santhosh Malae <[email protected]>

jerome-pouiller

The code is pleasant to read. My comments are mostly nitpicking.

drivers/spi/Kconfig.silabs_siwx91x_gspi

drivers/spi/spi_silabs_siwx91x_gspi.c

drivers/spi/Kconfig.silabs_siwx91x_gspi

drivers/spi/spi_silabs_siwx91x_gspi.c

Implement SPI driver for siwx91x device Signed-off-by: Sai Santhosh Malae <[email protected]>

1. Create a YAML file for SPI node 2. Add SPI node in the siwx917.dtsi Signed-off-by: Sai Santhosh Malae <[email protected]>

1. Add board config and overlay files 2. Update testcase.yaml Signed-off-by: Sai Santhosh Malae <[email protected]>

github-actions bot added area: SPI SPI bus area: Clock Control platform: Silabs Silicon Labs labels Apr 6, 2025

github-actions bot requested review from asmellby, jerome-pouiller, jhedberg, Martinhoff-maker, nordic-krch, tbursztyka and teburd April 6, 2025 19:11

github-actions bot assigned tbursztyka Apr 6, 2025

jerome-pouiller reviewed Apr 7, 2025

View reviewed changes

Martinhoff-maker reviewed Apr 7, 2025

View reviewed changes

smalae force-pushed the silabs_siwx91x_spi_driver branch from 5cde70b to 6a096b8 Compare April 9, 2025 12:05

smalae requested review from jerome-pouiller and Martinhoff-maker April 9, 2025 12:10

jerome-pouiller reviewed Apr 9, 2025

View reviewed changes

tbursztyka reviewed Apr 9, 2025

View reviewed changes

jerome-pouiller self-requested a review April 10, 2025 10:13

drivers: spi: siwx91x: SPI clock initialization for siwx91x

b609366

Clock driver changes required for initializing the SPI clock for the siwx91x driver Signed-off-by: Sai Santhosh Malae <[email protected]>

smalae force-pushed the silabs_siwx91x_spi_driver branch from 6a096b8 to 1bc2624 Compare April 23, 2025 07:05

smalae requested a review from tbursztyka April 23, 2025 07:16

smalae closed this Apr 23, 2025

smalae reopened this Apr 23, 2025

jerome-pouiller reviewed Apr 23, 2025

View reviewed changes

smalae added 3 commits April 24, 2025 11:39

drivers: spi: siwx91x: Add siwx91x SPI primary driver

e02f6e1

Implement SPI driver for siwx91x device Signed-off-by: Sai Santhosh Malae <[email protected]>

drivers: spi: siwx91x: DTS changes for siwx91x SPI driver

b985ba2

1. Create a YAML file for SPI node 2. Add SPI node in the siwx917.dtsi Signed-off-by: Sai Santhosh Malae <[email protected]>

drivers: spi: siwx91x: spi_loopback test support

c7a6854

1. Add board config and overlay files 2. Update testcase.yaml Signed-off-by: Sai Santhosh Malae <[email protected]>

smalae force-pushed the silabs_siwx91x_spi_driver branch from 1bc2624 to c7a6854 Compare April 24, 2025 06:11

smalae requested a review from jerome-pouiller April 24, 2025 06:17

jhedberg mentioned this pull request Apr 24, 2025

Contributor Nomination: Sai Santhosh Malae #89002

Closed

jerome-pouiller approved these changes Apr 24, 2025

View reviewed changes

jhedberg approved these changes Apr 24, 2025

View reviewed changes

Martinhoff-maker approved these changes Apr 29, 2025

View reviewed changes

jhedberg self-assigned this Apr 30, 2025

kartben merged commit 8e57cc6 into zephyrproject-rtos:main Apr 30, 2025
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: spi: siwx91x: Add siwx91x SPI primary driver #88212

drivers: spi: siwx91x: Add siwx91x SPI primary driver #88212

smalae commented Apr 6, 2025 •

edited

Loading

jerome-pouiller commented Apr 7, 2025

jerome-pouiller Apr 7, 2025

smalae Apr 9, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025 •

edited

Loading

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller Apr 9, 2025

smalae Apr 23, 2025

tbursztyka Apr 9, 2025

smalae Apr 23, 2025

jerome-pouiller left a comment

drivers: spi: siwx91x: Add siwx91x SPI primary driver #88212

drivers: spi: siwx91x: Add siwx91x SPI primary driver #88212

Conversation

smalae commented Apr 6, 2025 • edited Loading

jerome-pouiller commented Apr 7, 2025

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

jerome-pouiller Apr 9, 2025 • 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

jerome-pouiller left a comment

Choose a reason for hiding this comment

smalae commented Apr 6, 2025 •

edited

Loading

jerome-pouiller Apr 9, 2025 •

edited

Loading