Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Alain Volmat <alain.volmat@st.com>
To:     <broonie@kernel.org>, <amelie.delaunay@st.com>
CC:     <mcoquelin.stm32@gmail.com>, <alexandre.torgue@st.com>,
        <linux-spi@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <fabrice.gasnier@st.com>,
        <alain.volmat@st.com>
Subject: [PATCH 10/18] spi: stm32: wait for completion in transfer_one()
Date:   Wed, 5 Aug 2020 09:02:05 +0200
Message-ID: <1596610933-32599-11-git-send-email-alain.volmat@st.com>
In-Reply-To: <1596610933-32599-1-git-send-email-alain.volmat@st.com>
References: <1596610933-32599-1-git-send-email-alain.volmat@st.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Amelie Delaunay <amelie.delaunay@st.com>

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 <antonio.borneo@st.com>
Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 drivers/spi/spi-stm32.c | 87 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 25 deletions(-)

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,
-- 
2.7.4