Re: [PATCH v3 1/2] dmaengine: qcom-bam-dma: Add pm_runtime support
From: Andy Gross <hidden>
Date: 2016-05-04 20:58:01
Also in:
linux-arm-msm, lkml
On Wed, May 04, 2016 at 02:12:02PM +0530, Pramod Gurav wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- Changes in v2: - Handled return values of pm_runtime_get_sync to return error - Rework remove function drivers/dma/qcom/bam_dma.c | 98 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-)diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index 5b427c4..da64511 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)@@ -528,11 +531,14 @@ static void bam_free_chan(struct dma_chan *chan) u32 val; unsigned long flags; + if (pm_runtime_get_sync(bdev->dev)) + return; + vchan_free_chan_resources(to_virt_chan(chan)); if (bchan->curr_txd) { dev_err(bchan->bdev->dev, "Cannot free busy channel\n"); - return; + goto err; } spin_lock_irqsave(&bchan->vc.lock, flags);@@ -550,6 +556,10 @@ static void bam_free_chan(struct dma_chan *chan) /* disable irq */ writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN)); + +err: + pm_runtime_mark_last_busy(bdev->dev); + pm_runtime_put_autosuspend(bdev->dev); } /**@@ -695,11 +705,18 @@ static int bam_pause(struct dma_chan *chan) struct bam_chan *bchan = to_bam_chan(chan); struct bam_device *bdev = bchan->bdev; unsigned long flag; + int ret; + + ret = pm_runtime_get_sync(bdev->dev); + if (ret) + return ret; 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; }@@ -714,11 +731,17 @@ static int bam_resume(struct dma_chan *chan) struct bam_chan *bchan = to_bam_chan(chan); struct bam_device *bdev = bchan->bdev; unsigned long flag; + int ret; + ret = pm_runtime_get_sync(bdev->dev); + if (ret) + return ret; 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 +990,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);
I think we shouldnt be doing this unless we have nothing else on the lists. Perhaps the bam_start_dma function can be the place where we decide to do the get_sync and the autosuspend.
quoted hunk ↗ jump to hunk
} /**@@ -978,8 +1004,13 @@ 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)) + if (pm_runtime_get_sync(bdev->dev)) + return; +
We should only do this if our lists were empty before. That means we weren't doing any work.
quoted hunk ↗ jump to hunk
spin_lock_irqsave(&bchan->vc.lock, flags); /* if work pending and idle, start a transaction */@@ -1210,6 +1241,13 @@ static int bam_dma_probe(struct platform_device *pdev) if (ret) goto err_unregister_dma; + pm_runtime_irq_safe(&pdev->dev);
I wonder if this is really necessary. We don't directly access any pm runtime calls from irq context.
+ pm_runtime_set_autosuspend_delay(&pdev->dev, BAM_DMA_AUTOSUSPEND_DELAY); + pm_runtime_use_autosuspend(&pdev->dev);
If we use the autosuspend, we have to make sure we never pull the rug out from under the device if there is a pending transaction. So that being the case, I think if we only do the autosuspend after the lists are empty, we are going to be ok. But we can't be allowing autosuspend while the controller is working.
quoted hunk ↗ jump to hunk
+ pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + return 0; err_unregister_dma:@@ -1230,6 +1268,8 @@ static int bam_dma_remove(struct platform_device *pdev) struct bam_device *bdev = platform_get_drvdata(pdev); u32 i; + pm_runtime_force_suspend(&pdev->dev); + of_dma_controller_free(pdev->dev.of_node); dma_async_device_unregister(&bdev->common);@@ -1257,11 +1297,67 @@ static int bam_dma_remove(struct platform_device *pdev) 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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html