| Message ID | 1462268237-29508-1-git-send-email-pramod.gurav@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, }, };
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(+)