spi: spi-imx: use threaded interrupt

Commit Message

Lucas Stach Sept. 1, 2014, 2:35 p.m. UTC

From: Sascha Hauer <[e-mail]>

The i.MX SPI driver fills/flushes the FIFOs in interrupt context. With
longer SPI messages this introduces big latencies in the system. Using
a threaded interrupt avoids this issue.

Signed-off-by: Sascha Hauer <[e-mail]>
Signed-off-by: Lucas Stach <[e-mail]>
---
 drivers/spi/spi-imx.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Mark Brown Sept. 1, 2014, 2:57 p.m. UTC | #1

On Mon, Sep 01, 2014 at 04:35:06PM +0200, Lucas Stach wrote:
> From: Sascha Hauer <[e-mail]>
> 
> The i.MX SPI driver fills/flushes the FIFOs in interrupt context. With
> longer SPI messages this introduces big latencies in the system. Using
> a threaded interrupt avoids this issue.

Adding Marek.  A few things here:

> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 5daff2054ae4..5e40801e1c52 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -672,6 +672,15 @@ static irqreturn_t spi_imx_isr(int irq, void *dev_id)
>  {
>  	struct spi_imx_data *spi_imx = dev_id;
>  
> +	spi_imx->devtype_data->intctrl(spi_imx, 0);
> +
> +	return IRQ_WAKE_THREAD;
> +}

This means we unconditionally defer to the thread which means that we
will add latency in any case where we've not got any more data to place
in the FIFO.  That doesn't seem great.  I'd expect the driver to only
defer in cases where it was going to call one of the FIFO handling
functions (ideally it'd be possible to do some sort of copybreak based
on the amount of data to fill but I'm not sure it's worth the effort of
trying to pick numbers for that) and still complete transfers in hardirq
context in cases where that's all that needs doing.

I'm also not sure why the interrupt needs masking here - the driver
doesn't support shared interrupts so doesn't this just duplicate what
the interrupt subsystem is doing when it implements threading?  As
things stand it looks like the hard interrupt handler may as well just
be omitted.

> @@ -883,8 +892,8 @@ static int spi_imx_probe(struct platform_device *pdev)
>  		goto out_master_put;
>  	}
>  
> -	ret = devm_request_irq(&pdev->dev, spi_imx->irq, spi_imx_isr, 0,
> -			       dev_name(&pdev->dev), spi_imx);
> +	ret = request_threaded_irq(spi_imx->irq, spi_imx_isr,
> +			spi_imx_isr_thread, 0, dev_name(&pdev->dev), spi_imx);
>  	if (ret) {
>  		dev_err(&pdev->dev, "can't get irq%d: %d\n", spi_imx->irq, ret);
>  		goto out_master_put;

You've removed a devm_ but not added any free_irq() calls.  There's a
devm_request_threaded_irq() so you could just use that?

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 5daff2054ae4..5e40801e1c52 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -672,6 +672,15 @@  static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 {
 	struct spi_imx_data *spi_imx = dev_id;
 
+	spi_imx->devtype_data->intctrl(spi_imx, 0);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t spi_imx_isr_thread(int irq, void *dev_id)
+{
+	struct spi_imx_data *spi_imx = dev_id;
+
 	while (spi_imx->devtype_data->rx_available(spi_imx)) {
 		spi_imx->rx(spi_imx);
 		spi_imx->txfifo--;
@@ -679,6 +688,7 @@  static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 
 	if (spi_imx->count) {
 		spi_imx_push(spi_imx);
+		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
 		return IRQ_HANDLED;
 	}
 
@@ -691,7 +701,6 @@  static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
-	spi_imx->devtype_data->intctrl(spi_imx, 0);
 	complete(&spi_imx->xfer_done);
 
 	return IRQ_HANDLED;
@@ -883,8 +892,8 @@  static int spi_imx_probe(struct platform_device *pdev)
 		goto out_master_put;
 	}
 
-	ret = devm_request_irq(&pdev->dev, spi_imx->irq, spi_imx_isr, 0,
-			       dev_name(&pdev->dev), spi_imx);
+	ret = request_threaded_irq(spi_imx->irq, spi_imx_isr,
+			spi_imx_isr_thread, 0, dev_name(&pdev->dev), spi_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't get irq%d: %d\n", spi_imx->irq, ret);
 		goto out_master_put;