[10/18] spi: stm32: wait for completion in transfer_one()

Commit Message

Alain Volmat Aug. 5, 2020, 7:02 a.m. UTC

From: Amelie Delaunay <[e-mail]>

In irq mode, when cpu is under heavy load and spi speed is set too
high, the irq handler is not fast enough to feed the spi FIFOs.
This does not compromises the data tranferred; the spi clock is
temporarily stopped, the transfer takes longer time and the real
speed is much lower than expected.
The caller of transfer_one(), spi_transfer_one_message(), waits for
the end of current transfer. It sets a timeouts with quite a generous
tolerance of +100% + 100ms, but this can still expire.

We could make transfer_one() blocking till the end of the transfer
and bypass the wait/timeout mechanism in spi_transfer_one_message().
But if for some reason, we never get either an error (OVR, SUSP) event
or end of transfer (EOT) event, xfer_completion will never "complete".
That's why a timeout is useful here to avoid a hang. Timeout delay is
deducted from the transfer length, the real speed and the optional delay
we can add between each data frames. Timeout delay is doubled compared to
the theorical transfer duration.

While doing it to address irq mode only, take benefit of the new
code structure and wait also in dma mode so an eventual error can be
returned to the framework.

Signed-off-by: Antonio Borneo <[e-mail]>
Signed-off-by: Amelie Delaunay <[e-mail]>
Signed-off-by: Alain Volmat <[e-mail]>
---
 drivers/spi/spi-stm32.c | 87 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 25 deletions(-)

Comments

Mark Brown Aug. 5, 2020, 10:58 a.m. UTC | #1

On Wed, Aug 05, 2020 at 09:02:05AM +0200, Alain Volmat wrote:

> We could make transfer_one() blocking till the end of the transfer
> and bypass the wait/timeout mechanism in spi_transfer_one_message().
> But if for some reason, we never get either an error (OVR, SUSP) event
> or end of transfer (EOT) event, xfer_completion will never "complete".
> That's why a timeout is useful here to avoid a hang. Timeout delay is
> deducted from the transfer length, the real speed and the optional delay
> we can add between each data frames. Timeout delay is doubled compared to
> the theorical transfer duration.

> While doing it to address irq mode only, take benefit of the new
> code structure and wait also in dma mode so an eventual error can be
> returned to the framework.

I can't tell from this changelog what this change is intended to do
which makes it very difficult to review.  If the timeout is too short
for some systems under extreme load it would be better to provide a
generic way of configuring it rather than open coding timeouts, or
otherwise improve the generic code.

Patch

diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c
index a931c821c280..7e3894455331 100644
--- a/drivers/spi/spi-stm32.c
+++ b/drivers/spi/spi-stm32.c
@@ -240,7 +240,7 @@  struct stm32_spi_cfg {
 	int (*set_mode)(struct stm32_spi *spi, unsigned int comm_type);
 	void (*set_data_idleness)(struct stm32_spi *spi, u32 length);
 	int (*set_number_of_data)(struct stm32_spi *spi, u32 length);
-	void (*transfer_one_dma_start)(struct stm32_spi *spi);
+	int (*transfer_one_dma_start)(struct stm32_spi *spi);
 	void (*dma_rx_cb)(void *data);
 	void (*dma_tx_cb)(void *data);
 	int (*transfer_one_irq)(struct stm32_spi *spi);
@@ -276,6 +276,8 @@  struct stm32_spi_cfg {
  * @dma_tx: dma channel for TX transfer
  * @dma_rx: dma channel for RX transfer
  * @phys_addr: SPI registers physical base address
+ * @xfer_completion: completion to wait for end of transfer
+ * @xfer_status: current transfer status
  */
 struct stm32_spi {
 	struct device *dev;
@@ -303,6 +305,8 @@  struct stm32_spi {
 	struct dma_chan *dma_tx;
 	struct dma_chan *dma_rx;
 	dma_addr_t phys_addr;
+	struct completion xfer_completion;
+	int xfer_status;
 };
 
 static const struct stm32_spi_regspec stm32f4_spi_regspec = {
@@ -863,8 +867,8 @@  static irqreturn_t stm32f4_spi_irq_thread(int irq, void *dev_id)
 	struct spi_master *master = dev_id;
 	struct stm32_spi *spi = spi_master_get_devdata(master);
 
-	spi_finalize_current_transfer(master);
 	stm32f4_spi_disable(spi);
+	complete(&spi->xfer_completion);
 
 	return IRQ_HANDLED;
 }
@@ -920,13 +924,16 @@  static irqreturn_t stm32h7_spi_irq_thread(int irq, void *dev_id)
 		 * If communication is suspended while using DMA, it means
 		 * that something went wrong, so stop the current transfer
 		 */
-		if (spi->cur_usedma)
+		if (spi->cur_usedma) {
+			spi->xfer_status = -EIO;
 			end = true;
+		}
 		ifcr |= STM32H7_SPI_SR_SUSP;
 	}
 
 	if (mask & STM32H7_SPI_SR_OVR) {
 		dev_err(spi->dev, "Overrun: RX data lost\n");
+		spi->xfer_status = -EIO;
 		end = true;
 		ifcr |= STM32H7_SPI_SR_OVR;
 	}
@@ -955,7 +962,7 @@  static irqreturn_t stm32h7_spi_irq_thread(int irq, void *dev_id)
 
 	if (end) {
 		stm32h7_spi_disable(spi);
-		spi_finalize_current_transfer(master);
+		complete(&spi->xfer_completion);
 	}
 
 	return IRQ_HANDLED;
@@ -1026,8 +1033,8 @@  static void stm32f4_spi_dma_tx_cb(void *data)
 	struct stm32_spi *spi = data;
 
 	if (spi->cur_comm == SPI_SIMPLEX_TX || spi->cur_comm == SPI_3WIRE_TX) {
-		spi_finalize_current_transfer(spi->master);
 		stm32f4_spi_disable(spi);
+		complete(&spi->xfer_completion);
 	}
 }
 
@@ -1041,8 +1048,8 @@  static void stm32f4_spi_dma_rx_cb(void *data)
 {
 	struct stm32_spi *spi = data;
 
-	spi_finalize_current_transfer(spi->master);
 	stm32f4_spi_disable(spi);
+	complete(&spi->xfer_completion);
 }
 
 /**
@@ -1124,9 +1131,6 @@  static void stm32_spi_dma_config(struct stm32_spi *spi,
  * stm32f4_spi_transfer_one_irq - transfer a single spi_transfer using
  *				  interrupts
  * @spi: pointer to the spi controller data structure
- *
- * It must returns 0 if the transfer is finished or 1 if the transfer is still
- * in progress.
  */
 static int stm32f4_spi_transfer_one_irq(struct stm32_spi *spi)
 {
@@ -1160,16 +1164,13 @@  static int stm32f4_spi_transfer_one_irq(struct stm32_spi *spi)
 
 	spin_unlock_irqrestore(&spi->lock, flags);
 
-	return 1;
+	return 0;
 }
 
 /**
  * stm32h7_spi_transfer_one_irq - transfer a single spi_transfer using
  *				  interrupts
  * @spi: pointer to the spi controller data structure
- *
- * It must returns 0 if the transfer is finished or 1 if the transfer is still
- * in progress.
  */
 static int stm32h7_spi_transfer_one_irq(struct stm32_spi *spi)
 {
@@ -1202,7 +1203,7 @@  static int stm32h7_spi_transfer_one_irq(struct stm32_spi *spi)
 
 	spin_unlock_irqrestore(&spi->lock, flags);
 
-	return 1;
+	return 0;
 }
 
 /**
@@ -1210,8 +1211,12 @@  static int stm32h7_spi_transfer_one_irq(struct stm32_spi *spi)
  *					transfer using DMA
  * @spi: pointer to the spi controller data structure
  */
-static void stm32f4_spi_transfer_one_dma_start(struct stm32_spi *spi)
+static int stm32f4_spi_transfer_one_dma_start(struct stm32_spi *spi)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&spi->lock, flags);
+
 	/* In DMA mode end of transfer is handled by DMA TX or RX callback. */
 	if (spi->cur_comm == SPI_SIMPLEX_RX || spi->cur_comm == SPI_3WIRE_RX ||
 	    spi->cur_comm == SPI_FULL_DUPLEX) {
@@ -1224,6 +1229,10 @@  static void stm32f4_spi_transfer_one_dma_start(struct stm32_spi *spi)
 	}
 
 	stm32_spi_enable(spi);
+
+	spin_unlock_irqrestore(&spi->lock, flags);
+
+	return 0;
 }
 
 /**
@@ -1231,8 +1240,12 @@  static void stm32f4_spi_transfer_one_dma_start(struct stm32_spi *spi)
  *					transfer using DMA
  * @spi: pointer to the spi controller data structure
  */
-static void stm32h7_spi_transfer_one_dma_start(struct stm32_spi *spi)
+static int stm32h7_spi_transfer_one_dma_start(struct stm32_spi *spi)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&spi->lock, flags);
+
 	/* Enable the interrupts relative to the end of transfer */
 	stm32_spi_set_bits(spi, STM32H7_SPI_IER, STM32H7_SPI_IER_EOTIE |
 						 STM32H7_SPI_IER_TXTFIE |
@@ -1241,15 +1254,16 @@  static void stm32h7_spi_transfer_one_dma_start(struct stm32_spi *spi)
 	stm32_spi_enable(spi);
 
 	stm32_spi_set_bits(spi, STM32H7_SPI_CR1, STM32H7_SPI_CR1_CSTART);
+
+	spin_unlock_irqrestore(&spi->lock, flags);
+
+	return 0;
 }
 
 /**
  * stm32_spi_transfer_one_dma - transfer a single spi_transfer using DMA
  * @spi: pointer to the spi controller data structure
  * @xfer: pointer to the spi_transfer structure
- *
- * It must returns 0 if the transfer is finished or 1 if the transfer is still
- * in progress.
  */
 static int stm32_spi_transfer_one_dma(struct stm32_spi *spi,
 				      struct spi_transfer *xfer)
@@ -1325,12 +1339,9 @@  static int stm32_spi_transfer_one_dma(struct stm32_spi *spi,
 		stm32_spi_set_bits(spi, spi->cfg->regs->dma_tx_en.reg,
 				   spi->cfg->regs->dma_tx_en.mask);
 	}
-
-	spi->cfg->transfer_one_dma_start(spi);
-
 	spin_unlock_irqrestore(&spi->lock, flags);
 
-	return 1;
+	return spi->cfg->transfer_one_dma_start(spi);
 
 dma_submit_error:
 	if (spi->dma_rx)
@@ -1640,6 +1651,7 @@  static int stm32_spi_transfer_one(struct spi_master *master,
 				  struct spi_transfer *transfer)
 {
 	struct stm32_spi *spi = spi_master_get_devdata(master);
+	u32 xfer_time, midi_delay_ns;
 	int ret;
 
 	spi->tx_buf = transfer->tx_buf;
@@ -1656,10 +1668,34 @@  static int stm32_spi_transfer_one(struct spi_master *master,
 		return ret;
 	}
 
+	reinit_completion(&spi->xfer_completion);
+	spi->xfer_status = 0;
+
 	if (spi->cur_usedma)
-		return stm32_spi_transfer_one_dma(spi, transfer);
+		ret = stm32_spi_transfer_one_dma(spi, transfer);
 	else
-		return spi->cfg->transfer_one_irq(spi);
+		ret = spi->cfg->transfer_one_irq(spi);
+
+	if (ret)
+		return ret;
+
+	/* Wait for transfer to complete */
+	xfer_time = spi->cur_xferlen * 8 * MSEC_PER_SEC / spi->cur_speed;
+	midi_delay_ns = spi->cur_xferlen * 8 / spi->cur_bpw * spi->cur_midi;
+	xfer_time += DIV_ROUND_UP(midi_delay_ns, NSEC_PER_MSEC);
+	xfer_time = max(2 * xfer_time, 100U);
+
+	ret = wait_for_completion_timeout(&spi->xfer_completion,
+					  (msecs_to_jiffies(xfer_time)));
+	if (!ret) {
+		dev_err(spi->dev, "SPI transfer timeout (%u ms)\n", xfer_time);
+		spi->xfer_status = -ETIMEDOUT;
+		spi->cfg->disable(spi);
+	}
+
+	spi_finalize_current_transfer(master);
+
+	return spi->xfer_status;
 }
 
 /**
@@ -1810,6 +1846,7 @@  static int stm32_spi_probe(struct platform_device *pdev)
 	spi->dev = &pdev->dev;
 	spi->master = master;
 	spin_lock_init(&spi->lock);
+	init_completion(&spi->xfer_completion);
 
 	spi->cfg = (const struct stm32_spi_cfg *)
 		of_match_device(pdev->dev.driver->of_match_table,