[PATCH 0/3] spi/s3c64xx diagnostic and PM updates

[PATCH 0/3] spi/s3c64xx diagnostic and PM updates

[Mark Brown]

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(-)

[PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

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@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   57 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 019a716..d56066b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -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)
 {
 	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) {
 		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));
 	if (master == NULL) {
@@ -1104,10 +1140,21 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	INIT_WORK(&sdd->work, s3c64xx_spi_work);
 	INIT_LIST_HEAD(&sdd->queue);
 
+	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);
+	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;
 	}
 
 	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:
@@ -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

[PATCH 2/3] spi/s3c64xx: Convert to dev_pm_ops

[Mark Brown]

In preparation for the addition of runtime PM ops.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index d56066b..56dbdf1 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1231,9 +1231,9 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int s3c64xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
+static int s3c64xx_spi_suspend(struct device *dev)
 {
-	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	unsigned long flags;
 
@@ -1253,9 +1253,10 @@ static int s3c64xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
 	return 0;
 }
 
-static int s3c64xx_spi_resume(struct platform_device *pdev)
+static int s3c64xx_spi_resume(struct device *dev)
 {
-	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct platform_device *pdev = to_platform_device(dev);
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
 	unsigned long flags;
@@ -1274,19 +1275,19 @@ static int s3c64xx_spi_resume(struct platform_device *pdev)
 
 	return 0;
 }
-#else
-#define s3c64xx_spi_suspend	NULL
-#define s3c64xx_spi_resume	NULL
 #endif /* CONFIG_PM */
 
+static const struct dev_pm_ops s3c64xx_spi_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(s3c64xx_spi_suspend, s3c64xx_spi_resume)
+};
+
 static struct platform_driver s3c64xx_spi_driver = {
 	.driver = {
 		.name	= "s3c64xx-spi",
 		.owner = THIS_MODULE,
+		.pm = &s3c64xx_spi_pm,
 	},
 	.remove = s3c64xx_spi_remove,
-	.suspend = s3c64xx_spi_suspend,
-	.resume = s3c64xx_spi_resume,
 };
 MODULE_ALIAS("platform:s3c64xx-spi");
 
-- 
1.7.7.3

[PATCH 3/3] spi/s3c64xx: Implement runtime PM support

[Mark Brown]

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.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/spi/spi-s3c64xx.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 56dbdf1..b0b843b 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -25,6 +25,7 @@
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 
 #include <mach/dma.h>
@@ -782,6 +783,8 @@ static void s3c64xx_spi_work(struct work_struct *work)
 	while (!acquire_dma(sdd))
 		msleep(10);
 
+	pm_runtime_get_sync(&sdd->pdev->dev);
+
 	spin_lock_irqsave(&sdd->lock, flags);
 
 	while (!list_empty(&sdd->queue)
@@ -810,6 +813,8 @@ static void s3c64xx_spi_work(struct work_struct *work)
 	/* Free DMA channels */
 	sdd->ops->release(sdd->rx_dma.ch, &s3c64xx_spi_dma_client);
 	sdd->ops->release(sdd->tx_dma.ch, &s3c64xx_spi_dma_client);
+
+	pm_runtime_put(&sdd->pdev->dev);
 }
 
 static int s3c64xx_spi_transfer(struct spi_device *spi,
@@ -892,6 +897,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 		goto setup_exit;
 	}
 
+	pm_runtime_get_sync(&sdd->pdev->dev);
+
 	/* Check if we can provide the requested rate */
 	if (!sci->clk_from_cmu) {
 		u32 psr, speed;
@@ -924,6 +931,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 			err = -EINVAL;
 	}
 
+	pm_runtime_put(&sdd->pdev->dev);
+
 setup_exit:
 
 	/* setup() returns with device de-selected */
@@ -1164,6 +1173,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 					mem_res->end, mem_res->start,
 					sdd->rx_dma.dmach, sdd->tx_dma.dmach);
 
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 err9:
@@ -1197,6 +1208,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	struct resource	*mem_res;
 	unsigned long flags;
 
+	pm_runtime_disable(&pdev->dev);
+
 	spin_lock_irqsave(&sdd->lock, flags);
 	sdd->state |= SUSPND;
 	spin_unlock_irqrestore(&sdd->lock, flags);
@@ -1277,8 +1290,34 @@ static int s3c64xx_spi_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_RUNTIME
+static int s3c64xx_spi_runtime_suspend(struct device *dev)
+{
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+
+	clk_disable(sdd->clk);
+	clk_disable(sdd->src_clk);
+
+	return 0;
+}
+
+static int s3c64xx_spi_runtime_resume(struct device *dev)
+{
+	struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
+
+	clk_enable(sdd->src_clk);
+	clk_enable(sdd->clk);
+
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
 static const struct dev_pm_ops s3c64xx_spi_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(s3c64xx_spi_suspend, s3c64xx_spi_resume)
+	SET_RUNTIME_PM_OPS(s3c64xx_spi_runtime_suspend,
+			   s3c64xx_spi_runtime_resume, NULL)
 };
 
 static struct platform_driver s3c64xx_spi_driver = {
-- 
1.7.7.3

Re: [PATCH 3/3] spi/s3c64xx: Implement runtime PM support

[Bill Gatliff]

Guys:

On Sat, Jan 21, 2012 at 2:24 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> 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?

I'm doing runtime PM on several drivers myself, and that question
keeps coming up a lot.  Especially for devices that are more
complicated to suspend/resume than this one.  But even without the
device-specific overhead, I'm wondering how structure-walking that
goes on behind the pm_runtime() calls might tip the balance of PM
gains the opposite way from what we are seeking.

Not a critique of your patch series per se, but you probably have some
thoughts on all of this too--- and I would love to hear them while
they are still fresh. :)


b.g.
-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH 3/3] spi/s3c64xx: Implement runtime PM support

[Mark Brown]

On Sat, Jan 21, 2012 at 02:41:59PM +0100, Bill Gatliff wrote:

> 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?

It seems very low overhead, particularly in the context of the overhead
of the SPI transactions themselves.  I have sometimes wondered if it
might be an idea to just make the core do something along these lines by
default as there should be very few cases where it's important to have
the suspend happen immediately.

Re: [PATCH 0/3] spi/s3c64xx diagnostic and PM updates

[Grant Likely]

On Sat, Jan 21, 2012 at 6:23 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> 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

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.

g.

>
> 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(-)



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/3] spi/s3c64xx diagnostic and PM updates

[Mark Brown]

On Sat, Jan 21, 2012 at 07:27:23AM -0700, Grant Likely wrote:

> 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.

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Sylwester Nawrocki]

On 01/21/2012 02:24 PM, Mark Brown wrote:
> 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@opensource.wolfsonmicro.com>
> Acked-by: Linus Walleij<linus.walleij@linaro.org>
> ---
...
>   static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
>   {
>   	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];

clk_name seems to be unused, otherwise looks good.

--

Thanks,
Sylwester

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

On Sat, Jan 21, 2012 at 04:27:16PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 02:24 PM, Mark Brown wrote:

> > -	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?

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

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Sylwester Nawrocki]

On 01/21/2012 04:29 PM, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 04:27:16PM +0100, Sylwester Nawrocki wrote:
>> On 01/21/2012 02:24 PM, Mark Brown wrote:
> 
>>> -	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?

Indeed, the orphaned variable is already sitting in -next. I guess it
can be handled during merge.

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

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Shubhrajyoti Datta]

Hi Mark,
Some minor doubts/ questions.

On 1/21/12, Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> 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/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/spi/spi-s3c64xx.c |   57
> +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 019a716..d56066b 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -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)
>  {
>  	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) {
>  		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));
>  	if (master == NULL) {
> @@ -1104,10 +1140,21 @@ static int __init s3c64xx_spi_probe(struct
> platform_device *pdev)
>  	INIT_WORK(&sdd->work, s3c64xx_spi_work);
>  	INIT_LIST_HEAD(&sdd->queue);
>
> +	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.

> +	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;
>  	}
>
>  	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:
> @@ -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
>

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

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

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

Please delete irrelevant context from your replies.

> > +	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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] spi/s3c64xx diagnostic and PM updates

[Grant Likely]

On Sat, Jan 21, 2012 at 03:13:41PM +0000, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 07:27:23AM -0700, Grant Likely wrote:
> 
> > 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.

Okay, I've moved it over to spi/next instead.  It will be ready for the
v3.4 merge window.

g.

Re: [PATCH 1/3] spi/s3c64xx: Log error interrupts

[Shubhrajyoti Datta]

Hi Mark,

On Mon, Jan 30, 2012 at 3:10 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Jan 28, 2012 at 10:30:36PM +0530, Shubhrajyoti Datta wrote:
>
> Please delete irrelevant context from your replies.
>
>> > +   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.
I agree .

My only concern was that incase someone wants to add prints in the irq
that may be handy.
However as you rightly pointed that timing is surely to be more of a concern.
Thanks for the explanation.

[PATCH 1/3] spi/s3c64xx: Log error interrupts

[Mark Brown]

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@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   56 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index dcf7e10..9b16406 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -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)
 {
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
@@ -970,7 +999,7 @@ 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) {
@@ -1006,6 +1035,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));
 	if (master == NULL) {
@@ -1100,10 +1135,21 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	INIT_WORK(&sdd->work, s3c64xx_spi_work);
 	INIT_LIST_HEAD(&sdd->queue);
 
+	ret = request_irq(irq, s3c64xx_spi_irq, 0, "spi-s3c64xx", sdd);
+	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;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d "
@@ -1115,6 +1161,8 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
+err9:
+	free_irq(irq, sdd);
 err8:
 	destroy_workqueue(sdd->workqueue);
 err7:
@@ -1153,6 +1201,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