Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1393234 > unrolled thread

[PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support

Started byPramod Gurav <pramod.gurav@linaro.org>
First post2016-05-03 11:40 +0200
Last post2016-05-04 05:40 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support Pramod Gurav <pramod.gurav@linaro.org> - 2016-05-03 11:40 +0200
    [PATCH v2 2/2] MAINTAINERS: Update the files to include the Qualcomm DMA folder Pramod Gurav <pramod.gurav@linaro.org> - 2016-05-03 11:40 +0200
    Re: [PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support Manish Badarkhe <badarkhe.manish@gmail.com> - 2016-05-03 12:00 +0200
      Re: [PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support Pramod Gurav <pramod.gurav@linaro.org> - 2016-05-03 13:40 +0200
        Re: [PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support Manish Badarkhe <badarkhe.manish@gmail.com> - 2016-05-04 05:40 +0200

#1393234 — [PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support

FromPramod Gurav <pramod.gurav@linaro.org>
Date2016-05-03 11:40 +0200
Subject[PATCH v2 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support
Message-ID<ruEYa-5FG-5@gated-at.bofh.it>
Adds pm_runtime support for BAM DMA so that clock
is enabled only when there is a transaction going on to help
save power.

Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
---
Changes since v1:
- Removed unnecessary extra line additions and remavals

 drivers/dma/qcom/bam_dma.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index 5b427c4..f2a8b17 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -48,6 +48,7 @@
 #include <linux/of_dma.h>
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
+#include <linux/pm_runtime.h>
 
 #include "../dmaengine.h"
 #include "../virt-dma.h"
@@ -58,6 +59,8 @@ struct bam_desc_hw {
 	u16 flags;
 };
 
+#define BAM_DMA_AUTOSUSPEND_DELAY 100
+
 #define DESC_FLAG_INT BIT(15)
 #define DESC_FLAG_EOT BIT(14)
 #define DESC_FLAG_EOB BIT(13)
@@ -535,6 +538,7 @@ static void bam_free_chan(struct dma_chan *chan)
 		return;
 	}
 
+	pm_runtime_get_sync(bdev->dev);
 	spin_lock_irqsave(&bchan->vc.lock, flags);
 	bam_reset_channel(bchan);
 	spin_unlock_irqrestore(&bchan->vc.lock, flags);
@@ -550,6 +554,8 @@ static void bam_free_chan(struct dma_chan *chan)
 
 	/* disable irq */
 	writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN));
+	pm_runtime_mark_last_busy(bdev->dev);
+	pm_runtime_put_autosuspend(bdev->dev);
 }
 
 /**
@@ -696,10 +702,13 @@ static int bam_pause(struct dma_chan *chan)
 	struct bam_device *bdev = bchan->bdev;
 	unsigned long flag;
 
+	pm_runtime_get_sync(bdev->dev);
 	spin_lock_irqsave(&bchan->vc.lock, flag);
 	writel_relaxed(1, bam_addr(bdev, bchan->id, BAM_P_HALT));
 	bchan->paused = 1;
 	spin_unlock_irqrestore(&bchan->vc.lock, flag);
+	pm_runtime_mark_last_busy(bdev->dev);
+	pm_runtime_put_autosuspend(bdev->dev);
 
 	return 0;
 }
@@ -715,10 +724,13 @@ static int bam_resume(struct dma_chan *chan)
 	struct bam_device *bdev = bchan->bdev;
 	unsigned long flag;
 
+	pm_runtime_get_sync(bdev->dev);
 	spin_lock_irqsave(&bchan->vc.lock, flag);
 	writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
 	bchan->paused = 0;
 	spin_unlock_irqrestore(&bchan->vc.lock, flag);
+	pm_runtime_mark_last_busy(bdev->dev);
+	pm_runtime_put_autosuspend(bdev->dev);
 
 	return 0;
 }
@@ -967,6 +979,9 @@ static void dma_tasklet(unsigned long data)
 			bam_start_dma(bchan);
 		spin_unlock_irqrestore(&bchan->vc.lock, flags);
 	}
+
+	pm_runtime_mark_last_busy(bdev->dev);
+	pm_runtime_put_autosuspend(bdev->dev);
 }
 
 /**
@@ -978,8 +993,12 @@ static void dma_tasklet(unsigned long data)
 static void bam_issue_pending(struct dma_chan *chan)
 {
 	struct bam_chan *bchan = to_bam_chan(chan);
+	struct bam_device *bdev = bchan->bdev;
 	unsigned long flags;
 
+	if (pm_runtime_status_suspended(bdev->dev))
+		pm_runtime_get_sync(bdev->dev);
+
 	spin_lock_irqsave(&bchan->vc.lock, flags);
 
 	/* if work pending and idle, start a transaction */
@@ -1210,6 +1229,13 @@ static int bam_dma_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unregister_dma;
 
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, BAM_DMA_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 err_unregister_dma:
@@ -1252,16 +1278,76 @@ static int bam_dma_remove(struct platform_device *pdev)
 
 	tasklet_kill(&bdev->task);
 
+	pm_runtime_get_sync(&pdev->dev);
 	clk_disable_unprepare(bdev->bamclk);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+
+	return 0;
+}
+
+static int bam_dma_runtime_suspend(struct device *dev)
+{
+	struct bam_device *bdev = dev_get_drvdata(dev);
+
+	clk_disable(bdev->bamclk);
+
+	return 0;
+}
+
+static int bam_dma_runtime_resume(struct device *dev)
+{
+	struct bam_device *bdev = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_enable(bdev->bamclk);
+	if (ret < 0) {
+		dev_err(dev, "clk_enable failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#ifdef CONFIG_PM_SLEEP
+static int bam_dma_suspend(struct device *dev)
+{
+	struct bam_device *bdev = dev_get_drvdata(dev);
+
+	pm_runtime_force_suspend(dev);
+
+	clk_unprepare(bdev->bamclk);
 
 	return 0;
 }
 
+static int bam_dma_resume(struct device *dev)
+{
+	struct bam_device *bdev = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare(bdev->bamclk);
+	if (ret)
+		return ret;
+
+	pm_runtime_force_resume(dev);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops bam_dma_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(bam_dma_suspend, bam_dma_resume)
+	SET_RUNTIME_PM_OPS(bam_dma_runtime_suspend, bam_dma_runtime_resume,
+				NULL)
+};
+
 static struct platform_driver bam_dma_driver = {
 	.probe = bam_dma_probe,
 	.remove = bam_dma_remove,
 	.driver = {
 		.name = "bam-dma-engine",
+		.pm = &bam_dma_pm_ops,
 		.of_match_table = bam_of_match,
 	},
 };
-- 
1.8.2.1

[toc] | [next] | [standalone]

#1393236 — [PATCH v2 2/2] MAINTAINERS: Update the files to include the Qualcomm DMA folder

FromPramod Gurav <pramod.gurav@linaro.org>
Date2016-05-03 11:40 +0200
Subject[PATCH v2 2/2] MAINTAINERS: Update the files to include the Qualcomm DMA folder
Message-ID<ruEYa-5FG-13@gated-at.bofh.it>
In reply to#1393234
Recently all qcom dma drivers were moved a separate directory. Update
the files to include the same

Signed-off-by: Pramod Gurav <pramod.gurav@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d826f1b..2327f92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1407,6 +1407,7 @@ L:	linux-arm-msm@vger.kernel.org
 L:	linux-soc@vger.kernel.org
 S:	Maintained
 F:	arch/arm/mach-qcom/
+F:	drivers/dma/qcom/
 F:	drivers/soc/qcom/
 F:	drivers/tty/serial/msm_serial.h
 F:	drivers/tty/serial/msm_serial.c
-- 
1.8.2.1

[toc] | [prev] | [next] | [standalone]

#1393259

FromManish Badarkhe <badarkhe.manish@gmail.com>
Date2016-05-03 12:00 +0200
Message-ID<ruFhw-5Sv-13@gated-at.bofh.it>
In reply to#1393234
Hi Pramod

> @@ -715,10 +724,13 @@ static int bam_resume(struct dma_chan *chan)
>         struct bam_device *bdev = bchan->bdev;
>         unsigned long flag;
>
> +       pm_runtime_get_sync(bdev->dev);
>         spin_lock_irqsave(&bchan->vc.lock, flag);
>         writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
>         bchan->paused = 0;
>         spin_unlock_irqrestore(&bchan->vc.lock, flag);
> +       pm_runtime_mark_last_busy(bdev->dev);
> +       pm_runtime_put_autosuspend(bdev->dev);
>
>         return 0;
>  }

Why this function simply return 'success' without any error capture?

> @@ -1252,16 +1278,76 @@ static int bam_dma_remove(struct platform_device *pdev)
>
>         tasklet_kill(&bdev->task);
>
> +       pm_runtime_get_sync(&pdev->dev);
>         clk_disable_unprepare(bdev->bamclk);
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +       pm_runtime_set_suspended(&pdev->dev);
> +
> +       return 0;
> +}

Why this function simply return 'success' without any error capture?

> +static int bam_dma_runtime_suspend(struct device *dev)
> +{
> +       struct bam_device *bdev = dev_get_drvdata(dev);
> +
> +       clk_disable(bdev->bamclk);
> +
> +       return 0;
> +}

Why this function simply return 'success' without any error capture?


> +#ifdef CONFIG_PM_SLEEP
> +static int bam_dma_suspend(struct device *dev)
> +{
> +       struct bam_device *bdev = dev_get_drvdata(dev);
> +
> +       pm_runtime_force_suspend(dev);
> +
> +       clk_unprepare(bdev->bamclk);
>
>         return 0;
>  }

Why this function simply return 'success' without any error capture?

Regards
Manish Badarkhe

[toc] | [prev] | [next] | [standalone]

#1393346

FromPramod Gurav <pramod.gurav@linaro.org>
Date2016-05-03 13:40 +0200
Message-ID<ruGQh-7ml-3@gated-at.bofh.it>
In reply to#1393259
Hi Manish,

Thanks for review.

On 3 May 2016 at 15:21, Manish Badarkhe <badarkhe.manish@gmail.com> wrote:
> Hi Pramod
>
>> @@ -715,10 +724,13 @@ static int bam_resume(struct dma_chan *chan)
>>         struct bam_device *bdev = bchan->bdev;
>>         unsigned long flag;
>>
>> +       pm_runtime_get_sync(bdev->dev);
>>         spin_lock_irqsave(&bchan->vc.lock, flag);
>>         writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
>>         bchan->paused = 0;
>>         spin_unlock_irqrestore(&bchan->vc.lock, flag);
>> +       pm_runtime_mark_last_busy(bdev->dev);
>> +       pm_runtime_put_autosuspend(bdev->dev);
>>
>>         return 0;
>>  }
>
> Why this function simply return 'success' without any error capture?
>
I should check return of pm_runtime_get_sync.

>> @@ -1252,16 +1278,76 @@ static int bam_dma_remove(struct platform_device *pdev)
>>
>>         tasklet_kill(&bdev->task);
>>
>> +       pm_runtime_get_sync(&pdev->dev);
>>         clk_disable_unprepare(bdev->bamclk);
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +       pm_runtime_set_suspended(&pdev->dev);
>> +
>> +       return 0;
>> +}
>
> Why this function simply return 'success' without any error capture?
Need to handle it better.

>
>> +static int bam_dma_runtime_suspend(struct device *dev)
>> +{
>> +       struct bam_device *bdev = dev_get_drvdata(dev);
>> +
>> +       clk_disable(bdev->bamclk);
>> +
>> +       return 0;
>> +}
>
> Why this function simply return 'success' without any error capture?
If probe has succeeded means bdev->bamclk is not NULL and error hence
clk_disable wont return anything.

>
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int bam_dma_suspend(struct device *dev)
>> +{
>> +       struct bam_device *bdev = dev_get_drvdata(dev);
>> +
>> +       pm_runtime_force_suspend(dev);
>> +
>> +       clk_unprepare(bdev->bamclk);
>>
>>         return 0;
>>  }
>
> Why this function simply return 'success' without any error capture?
Same logic applies here as well, as pm_runtime_force_suspend and
clk_unprepare may not return error in our case once probe is through.

>
> Regards
> Manish Badarkhe

[toc] | [prev] | [next] | [standalone]

#1393926

FromManish Badarkhe <badarkhe.manish@gmail.com>
Date2016-05-04 05:40 +0200
Message-ID<ruVPj-4b0-5@gated-at.bofh.it>
In reply to#1393346
Hi Pramod

>>> @@ -715,10 +724,13 @@ static int bam_resume(struct dma_chan *chan)
>>>         struct bam_device *bdev = bchan->bdev;
>>>         unsigned long flag;
>>>
>>> +       pm_runtime_get_sync(bdev->dev);
>>>         spin_lock_irqsave(&bchan->vc.lock, flag);
>>>         writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_HALT));
>>>         bchan->paused = 0;
>>>         spin_unlock_irqrestore(&bchan->vc.lock, flag);
>>> +       pm_runtime_mark_last_busy(bdev->dev);
>>> +       pm_runtime_put_autosuspend(bdev->dev);
>>>
>>>         return 0;
>>>  }
>>
>> Why this function simply return 'success' without any error capture?
>>
> I should check return of pm_runtime_get_sync.

Ok, fine.

>>> @@ -1252,16 +1278,76 @@ static int bam_dma_remove(struct platform_device *pdev)
>>>
>>>         tasklet_kill(&bdev->task);
>>>
>>> +       pm_runtime_get_sync(&pdev->dev);
>>>         clk_disable_unprepare(bdev->bamclk);
>>> +       pm_runtime_disable(&pdev->dev);
>>> +       pm_runtime_put_noidle(&pdev->dev);
>>> +       pm_runtime_set_suspended(&pdev->dev);
>>> +
>>> +       return 0;
>>> +}
>>
>> Why this function simply return 'success' without any error capture?
> Need to handle it better.

Yes, need to handle __pm_runtime__ errors.

>>
>>> +static int bam_dma_runtime_suspend(struct device *dev)
>>> +{
>>> +       struct bam_device *bdev = dev_get_drvdata(dev);
>>> +
>>> +       clk_disable(bdev->bamclk);
>>> +
>>> +       return 0;
>>> +}
>>
>> Why this function simply return 'success' without any error capture?
> If probe has succeeded means bdev->bamclk is not NULL and error hence
> clk_disable wont return anything.

got it.

>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int bam_dma_suspend(struct device *dev)
>>> +{
>>> +       struct bam_device *bdev = dev_get_drvdata(dev);
>>> +
>>> +       pm_runtime_force_suspend(dev);
>>> +
>>> +       clk_unprepare(bdev->bamclk);
>>>
>>>         return 0;
>>>  }
>>
>> Why this function simply return 'success' without any error capture?
> Same logic applies here as well, as pm_runtime_force_suspend and
> clk_unprepare may not return error in our case once probe is through.

got it.


Regards
Manish Badarkhe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web