diff mbox series

[2/4] spi: sprd: Add the SPI irq function for the SPI DMA mode

Message ID ec1741f29c44c436c1086877ff5b208e6b8af45d.1547559542.git.baolin.wang@linaro.org (mailing list archive)
State New, archived
Headers show
Series [1/4] spi: sprd: Fix the error data length in SPI read-only mode | expand

Commit Message

(Exiting) Baolin Wang Jan. 15, 2019, 1:46 p.m. UTC 
From: Lanqing Liu <lanqing.liu@spreadtrum.com>

The SPI irq event will use to complete the SPI work in the SPI DMA mode,
so this patch is a preparation for the following DMA mode support.

Signed-off-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/spi/spi-sprd.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Mark Brown Jan. 15, 2019, 2:24 p.m. UTC | #1 
On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote:

This looks good, just one small issue and a thing to check:

> +static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
> +{
> +	struct sprd_spi *ss = (struct sprd_spi *)data;
> +	u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
> +
> +	if (val & SPRD_SPI_MASK_TX_END) {
> +		writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> +		if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
> +			complete(&ss->xfer_completion);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (val & SPRD_SPI_MASK_RX_END) {
> +		writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> +		complete(&ss->xfer_completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

This will return IRQ_HANDLED no matter if there was an interrupt
actually handled.  That means that if something goes wrong due to some
bug or a hardware change (eg, a new version of the IP) and there's
another interrupt fired we won't clear it and the interrupt core won't
be able to detect that it's a spurious interrupt and use its own error
handling.  It's better to return IRQ_NONE in that case.

> +	ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
> +				0, pdev->name, ss);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
> +			ss->irq, ret);

Are you sure it's safe to use devm_request_irq(), especially when
unloading the driver?  Using it means that we will only disable the
interrupt after the driver's remove function has finished so there's a
danger of an interrupt firing when some of the resources the hander has
used are still in use.  I didn't spot any issues, just something to
check especially with the later patches building on top of this.
(Exiting) Baolin Wang Jan. 16, 2019, 2:21 a.m. UTC | #2 
Hi Mark,

On Tue, 15 Jan 2019 at 22:25, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote:
>
> This looks good, just one small issue and a thing to check:
>
> > +static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
> > +{
> > +     struct sprd_spi *ss = (struct sprd_spi *)data;
> > +     u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
> > +
> > +     if (val & SPRD_SPI_MASK_TX_END) {
> > +             writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > +             if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
> > +                     complete(&ss->xfer_completion);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     if (val & SPRD_SPI_MASK_RX_END) {
> > +             writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > +             complete(&ss->xfer_completion);
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> This will return IRQ_HANDLED no matter if there was an interrupt
> actually handled.  That means that if something goes wrong due to some
> bug or a hardware change (eg, a new version of the IP) and there's
> another interrupt fired we won't clear it and the interrupt core won't
> be able to detect that it's a spurious interrupt and use its own error
> handling.  It's better to return IRQ_NONE in that case.

Yes, you are correct and will fix in next version.

> > +     ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
> > +                             0, pdev->name, ss);
> > +     if (ret)
> > +             dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
> > +                     ss->irq, ret);
>
> Are you sure it's safe to use devm_request_irq(), especially when
> unloading the driver?  Using it means that we will only disable the
> interrupt after the driver's remove function has finished so there's a
> danger of an interrupt firing when some of the resources the hander has
> used are still in use.  I didn't spot any issues, just something to
> check especially with the later patches building on top of this.

Yes, we missed this issue, thanks for pointing out this. After some
discussion with Lanqing, we think we need call the
spi_controller_suspend() manually in remove function to avoid this
issue.

Thanks for your comments.
diff mbox series

Patch

diff --git a/drivers/spi/spi-sprd.c b/drivers/spi/spi-sprd.c
index fa324ce..bfba30b 100644
--- a/drivers/spi/spi-sprd.c
+++ b/drivers/spi/spi-sprd.c
@@ -133,6 +133,7 @@  struct sprd_spi {
 	void __iomem *base;
 	struct device *dev;
 	struct clk *clk;
+	int irq;
 	u32 src_clk;
 	u32 hw_mode;
 	u32 trans_len;
@@ -141,6 +142,7 @@  struct sprd_spi {
 	u32 hw_speed_hz;
 	u32 len;
 	int status;
+	struct completion xfer_completion;
 	const void *tx_buf;
 	void *rx_buf;
 	int (*read_bufs)(struct sprd_spi *ss, u32 len);
@@ -575,6 +577,45 @@  static int sprd_spi_transfer_one(struct spi_controller *sctlr,
 	return ret;
 }
 
+static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
+{
+	struct sprd_spi *ss = (struct sprd_spi *)data;
+	u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
+
+	if (val & SPRD_SPI_MASK_TX_END) {
+		writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
+		if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
+			complete(&ss->xfer_completion);
+		return IRQ_HANDLED;
+	}
+
+	if (val & SPRD_SPI_MASK_RX_END) {
+		writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
+		complete(&ss->xfer_completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int sprd_spi_irq_init(struct platform_device *pdev, struct sprd_spi *ss)
+{
+	int ret;
+
+	ss->irq = platform_get_irq(pdev, 0);
+	if (ss->irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq resource\n");
+		return ss->irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
+				0, pdev->name, ss);
+	if (ret)
+		dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
+			ss->irq, ret);
+
+	return ret;
+}
+
 static int sprd_spi_clk_init(struct platform_device *pdev, struct sprd_spi *ss)
 {
 	struct clk *clk_spi, *clk_parent;
@@ -635,11 +676,16 @@  static int sprd_spi_probe(struct platform_device *pdev)
 	sctlr->max_speed_hz = min_t(u32, ss->src_clk >> 1,
 				    SPRD_SPI_MAX_SPEED_HZ);
 
+	init_completion(&ss->xfer_completion);
 	platform_set_drvdata(pdev, sctlr);
 	ret = sprd_spi_clk_init(pdev, ss);
 	if (ret)
 		goto free_controller;
 
+	ret = sprd_spi_irq_init(pdev, ss);
+	if (ret)
+		goto free_controller;
+
 	ret = clk_prepare_enable(ss->clk);
 	if (ret)
 		goto free_controller;