drivers: spi: gd32: support interrupt-driven mode

[soburi]

Add supporting interrupt-based asynchronous operation for GD32 SPI.

Signed-off-by: TOKITA Hiroshi [email protected]

[nandojve]

Overall LGTM.

[cameled]

In this case, I think #ifdef is more clear. we just want to import the code if config is defined.

[soburi]

Replace #if IS_ENABLED to #ifdef.

[cameled]

@soburi Please help to add this two state check, It will help to improve the transfer stability. Thanks!

The first check use to confirm TBE is set before write frame.
The second check use to confirm last frame transfer is complete.

diff --git a/drivers/spi/spi_gd32.c b/drivers/spi/spi_gd32.c
index 5a5b18b41e..0a0e5e9f26 100644
--- a/drivers/spi/spi_gd32.c
+++ b/drivers/spi/spi_gd32.c
@@ -161,6 +161,10 @@ static int spi_gd32_frame_exchange(const struct device *dev)
 	struct spi_context *ctx = &data->ctx;
 	uint16_t tx_frame = 0U, rx_frame = 0U;
 
+	while ((SPI_STAT(cfg->reg) & SPI_STAT_TBE) == 0) {
+		/* NOP */
+	}
+
 	if (SPI_WORD_SIZE_GET(ctx->config->operation) == 8) {
 		if (spi_context_tx_buf_on(ctx)) {
 			tx_frame = UNALIGNED_GET((uint8_t *)(data->ctx.tx_buf));
@@ -242,6 +246,11 @@ static int spi_gd32_transceive_impl(const struct device *dev,
 #endif
 #endif
 
+	while (!(SPI_STAT(cfg->reg) & SPI_STAT_TBE) ||
+		(SPI_STAT(cfg->reg) & SPI_STAT_TRANS)) {
+		/* Wait until last frame transfer complete. */
+	}
+
 	spi_context_cs_control(&data->ctx, false);
 
 	SPI_CTL0(cfg->reg) &= ~SPI_CTL0_SPIEN;

[cameled]

This is required for SPI_ASYNC, even in a polling case?
As I know, it not required since without interrupt support, async will not work.

[soburi]

In my understanding, SPI_ASYNC is just only notify finish of transfer with signal object.
Potentially it should works with polling implementations.
(I think it is enough useful for running with worker thread.)

For example, this PR work with enabling SPI_ASYNC even if not enabling SPI_GD32_INTERRUPT.

[cameled]

Thanks for the explanation.

[cameled]

Please consider to remove this line, gd32f450i_eval can not pass spi_flash with this.

[soburi]

Could you tell me the details of the problem.

It disables spi after spi_context_wait_for_completion is finished, so it should be okay to remove it.

[soburi]

It seems work correctly even if this line removed.
Once, remove this line.

[cameled]

Use below debug patch to trace gd32 spi isr:

diff --git a/drivers/spi/spi_gd32.c b/drivers/spi/spi_gd32.c
index ca033a9bb4..6d2a8628b5 100644
--- a/drivers/spi/spi_gd32.c
+++ b/drivers/spi/spi_gd32.c
@@ -286,7 +286,9 @@ static void spi_gd32_complete(const struct device *dev, int status)
 {
        struct spi_gd32_data *data = dev->data;
        const struct spi_gd32_config *cfg = dev->config;
+       printk("complete\n");
 
+       SPI_CTL0(cfg->reg) &= ~SPI_CTL0_SPIEN;
        SPI_CTL1(cfg->reg) &= ~(SPI_CTL1_RBNEIE | SPI_CTL1_TBEIE | SPI_CTL1_ERRIE);
 
        spi_context_complete(&data->ctx, status);
@@ -298,15 +300,19 @@ static void spi_gd32_isr(struct device *dev)
        struct spi_gd32_data *data = dev->data;
        int err = 0;
 
+       printk("isr_1: %x\n", SPI_STAT(cfg->reg));
        if ((SPI_STAT(cfg->reg) & SPI_GD32_ERR_MASK) != 0) {
                err = spi_gd32_get_err(cfg);
        } else {
+               printk("exchange\n");
                err = spi_gd32_frame_exchange(dev);
        }
+       printk("isr_2\n");
 
        if (err || !spi_gd32_transfer_ongoing(data)) {
                spi_gd32_complete(dev, err);
        }
+       printk("isr_3\n");
 
        SPI_STAT(cfg->reg) = 0;
 }

On gd32f450i_eval, we can get below output with spi_flash example.

There have an extral isr after the complete funcation have been called.
In the extral isr, it halt at exchange function.
Why isr halt at exchange function? Because we disabled i2c at complete function, RBNE will never be set again.

isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
complete
isr_3
isr_1: 2
exchange

After remove the spi disable line, there have two complete funcation call.

isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
isr_3
isr_1: 2
exchange
isr_2
complete
isr_3
isr_1: 2
exchange
isr_2
complete
isr_3

We can see there have an extral TBE interrupt event though we have cleared the SPI_CTL1_TBEIE bit.

To cover this situation, maybe we can add a on_going check before enter exchange function and prevent re-enter complete function.

[soburi]

Thank you for the explanation.
I got that interrupts are not received correctly by stopping the interrupt SPI in isr.
Eventually, Stop the SPI immediately before completing the transfer with spi_context_release ().
So, removing that line from spi_gd32_complete () seems to be well.

[cameled]

Thanks, gd32f450i_eval works well.

[cameled]

@soburi There have a CI failure, cause by others. Maybe a rebase from main branch can fix it.

[soburi]

@cameled

@soburi There have a CI failure, cause by others. Maybe a rebase from main branch can fix it.

Rebase done, and Fix CI failure. Thanks for notify.