spi/s3c64xx diagnostic and PM updates

Enable and disable the clocks to the SPI controller using runtime PM. This serves the dual purpose of reducing power consumption a little and letting the core know when the device is idle.

What about using autosuspend instead? If a client is doing a lot of closely-spaced SPI transactions on a relatively flat device tree, might the resulting runtime suspend/resume overhead between each transaction become noticeable?

Although the hardware supports interrupts we're not currently using them at all since for small transfers the overhead is greater than that for busy waiting and for large transfers we have interrupts from the DMA. This means that if the hardware reports an error (especially one which might not stall transfer) we might miss it.

Take a first pass at dealing with such errors by enabling the interrupt if we can and logging the errors if they happen. Ideally we'd report the error via the affected transfer but since we're in master mode it's very difficult to trigger errors at present and this code is much simpler.

Signed-off-by Mark Brown<[email protected]>

Acked-by Linus Walleij<[email protected]>

---

drivers/spi/spi-s3c64xx.c static int __init s3c64xx_spi_probe(struct platform_device *pdev) · line 1003

Quoted 3 lines · Mark Brown

drivers/spi/spi-s3c64xx.c +2 -1

-	int ret;
+	int ret, irq;
+	char clk_name[16];

clk_name seems to be unused, otherwise looks good.

Quoted 7 lines · Sylwester Nawrocki

drivers/spi/spi-s3c64xx.c static int __init s3c64xx_spi_probe(struct platform_device *pdev) · line 1003

Quoted 3 lines · Mark Brown

drivers/spi/spi-s3c64xx.c +2 -1

-	int ret;
+	int ret, irq;
+	char clk_name[16];

clk_name seems to be unused, otherwise looks good.

That's git cherry-pick being too clever for me when I pulled this out of my -next branch for Grant. clk_name is used in -next by some updates which I guess Kukjin is carrying, I hadn't noticed they weren't in the SPI tree. I don't know if it's better to just leave things as they are for now given that it'll all come out in the merge window?

Although the hardware supports interrupts we're not currently using them at all since for small transfers the overhead is greater than that for busy waiting and for large transfers we have interrupts from the DMA. This means that if the hardware reports an error (especially one which might not stall transfer) we might miss it.

Take a first pass at dealing with such errors by enabling the interrupt if we can and logging the errors if they happen. Ideally we'd report the error via the affected transfer but since we're in master mode it's very difficult to trigger errors at present and this code is much simpler.

Signed-off-by Mark Brown <broonie-yzvPICuk2AATkU/[email protected]>

Acked-by Linus Walleij <[email protected]>

---
 drivers/spi/spi-s3c64xx.c |   57
+++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

drivers/spi/spi-s3c64xx.c +29 -0

index 019a716..d56066b 100644
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/workqueue.h>
+#include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>

@@ -153,6 +154,7 @@ struct s3c64xx_spi_dma_data {
  * @tx_dmach: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
+ * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.

@@ -930,6 +932,33 @@ setup_exit:
 	return err;
 }
 
+static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
+{
+	struct s3c64xx_spi_driver_data *sdd = data;
+	struct spi_master *spi = sdd->master;
+	unsigned int val;
+
+	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
+		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
+
+	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
+
+	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+		dev_err(&spi->dev, "RX overrun\n");
+	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "RX underrun\n");
+	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+		dev_err(&spi->dev, "TX overrun\n");
+	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+		dev_err(&spi->dev, "TX underrun\n");
+
+	return IRQ_HANDLED;
+}
+
 static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int
 channel)
 {

drivers/spi/spi-s3c64xx.c +2 -1

 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;

@@ -970,7 +999,8 @@ static int __init s3c64xx_spi_probe(struct
 platform_device *pdev)
 	struct s3c64xx_spi_driver_data *sdd;
 	struct s3c64xx_spi_info *sci;
 	struct spi_master *master;
-	int ret;
+	int ret, irq;
+	char clk_name[16];
 
 	if (pdev->id < 0) {

drivers/spi/spi-s3c64xx.c +6 -0

 		dev_err(&pdev->dev,

@@ -1010,6 +1040,12 @@ static int __init s3c64xx_spi_probe(struct
 platform_device *pdev)
 		return -ENXIO;
 	}
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_warn(&pdev->dev, "Failed to get IRQ: %d\n", irq);
+		return irq;
+	}
+
 	master = spi_alloc_master(&pdev->dev,
 				sizeof(struct s3c64xx_spi_driver_data));

drivers/spi/spi-s3c64xx.c +0 -0

 	if (master == NULL) {

drivers/spi/spi-s3c64xx.c +11 -1

+	if (ret != 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
+			irq, ret);
+		goto err8;
+	}
+
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	if (spi_register_master(master)) {
 		dev_err(&pdev->dev, "cannot register SPI master\n");
 		ret = -EBUSY;
-		goto err8;
+		goto err9;
 	}
 

drivers/spi/spi-s3c64xx.c +2 -0

 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d "

@@ -1119,6 +1166,8 @@ static int __init s3c64xx_spi_probe(struct
 platform_device *pdev)
 
 	return 0;
 
+err9:
+	free_irq(irq, sdd);
 err8:
 	destroy_workqueue(sdd->workqueue);

err7:

drivers/spi/spi-s3c64xx.c +4 -0

@@ -1157,6 +1206,10 @@ static int s3c64xx_spi_remove(struct platform_device
 *pdev)
 
 	spi_unregister_master(master);
 
+	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
+
+	free_irq(platform_get_irq(pdev, 0), sdd);
+
 	destroy_workqueue(sdd->workqueue);
 

clk_disable(sdd->src_clk);
--
1.7.7.3

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
spi-devel-general mailing list spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

drivers/spi/spi-s3c64xx.c static int __init s3c64xx_spi_probe(struct platform_device *pdev) · line 1143

Quoted 1 line

drivers/spi/spi-s3c64xx.c +1 -0

+	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);

Could we have a threaded irq instead and there are prints and that may get executed in interrupt context.

On Sat, Jan 28, 2012 at 10:30:36PM +0530, Shubhrajyoti Datta wrote:

Please delete irrelevant context from your replies.

Quoted 4 lines

Quoted 1 line

+   ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);

Could we have a threaded irq instead and there are prints and that may get executed in interrupt context.

So, clearly we *could*.  However I think that's not going to be as helpful.  These errors are never supposed to happen (and if they do indicate a fairly serious problem) so the performance impact shouldn't be our primary concern and if they do happen then my suspicion is that the timing information will be very important.  I expect that knowing exactly when the error occurred is likely to help diagnose what caused it by virtue of knowing what other things were going on in the system at that time.

The following changes since commit 805a6af8dba5dfdd35ec35dc52ec0122400b2610:

 Linux 3.2 (2012-01-04 15:55:44 -0800)

are available in the git repository at:
 git://git.kernel.org/pub/scm/linux/kernel/git/broonie/misc.git spi/s3c64xx

They've all been posted several times before but seem to have got dropped on the floor somewhere along the line.

Mark Brown (3):
     spi/s3c64xx: Log error interrupts
     spi/s3c64xx: Convert to dev_pm_ops
     spi/s3c64xx: Implement runtime PM support

 drivers/spi/spi-s3c64xx.c |  115 ++++++++++++++++++++++++++++++++++++++++----

 1 files changed, 104 insertions(+), 11 deletions(-)

Pushed out to git://git.secretlab.ca/git/linux-2.6.git spi/merge. I'll ask Linus to pull in a couple of days after it has hit linux-next.

Quoted 3 lines · Grant Likely

Pushed out to git://git.secretlab.ca/git/linux-2.6.git spi/merge. I'll ask Linus to pull in a couple of days after it has hit linux-next.

Thanks. Obviously it'd be nice to get it into 3.3 but I don't see a huge rush and it did miss the merge window - it doesn't fix any bugs and without a bunch of other work in cpuidle that's not been done yet the power gains from the runtime PM are vanishingly small.