| Message ID | 1410857494-15936-5-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 16 September 2014 10:51, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > This patch adds both normal PM suspend/resume support and runtime PM > support to pl330 DMA engine driver. > > The runtime power management for pl330 DMA driver allows gating of AMBA > clock (PDMA) in FSYS clock domain, when the device is not processing any > requests. This is necessary to enter low power modes on Exynos SoCs > (e.g. LPA on Exynos4x12 or W-AFTR on Exynos3250). > > Runtime PM resuming of the device may happen in atomic context (during > call device_issue_pending()) so pm_runtime_irq_safe() is used. This will > lead only to disabling/enabling of the clock but this is sufficient for > gating the clock and for reducing energy usage. > > During system sleep the AMBA bus clock is also unprepared. > > Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/dma/pl330.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 73 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index 4839bfa74a10..9c014874c33d 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -27,6 +27,7 @@ > #include <linux/of.h> > #include <linux/of_dma.h> > #include <linux/err.h> > +#include <linux/pm_runtime.h> > > #include "dmaengine.h" > #define PL330_MAX_CHAN 8 > @@ -1958,6 +1959,7 @@ static void pl330_tasklet(unsigned long data) > struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; > struct dma_pl330_desc *desc, *_dt; > unsigned long flags; > + bool power_down = false; > > spin_lock_irqsave(&pch->lock, flags); > > @@ -1972,10 +1974,17 @@ static void pl330_tasklet(unsigned long data) > /* Try to submit a req imm. next to the last completed cookie */ > fill_queue(pch); > > - /* Make sure the PL330 Channel thread is active */ > - spin_lock(&pch->thread->dmac->lock); > - _start(pch->thread); > - spin_unlock(&pch->thread->dmac->lock); > + if (list_empty(&pch->work_list)) { > + spin_lock(&pch->thread->dmac->lock); > + _stop(pch->thread); > + spin_unlock(&pch->thread->dmac->lock); > + power_down = true; > + } else { > + /* Make sure the PL330 Channel thread is active */ > + spin_lock(&pch->thread->dmac->lock); > + _start(pch->thread); > + spin_unlock(&pch->thread->dmac->lock); > + } > > while (!list_empty(&pch->completed_list)) { > dma_async_tx_callback callback; > @@ -1990,6 +1999,12 @@ static void pl330_tasklet(unsigned long data) > if (pch->cyclic) { > desc->status = PREP; > list_move_tail(&desc->node, &pch->work_list); > + if (power_down) { > + spin_lock(&pch->thread->dmac->lock); > + _start(pch->thread); > + spin_unlock(&pch->thread->dmac->lock); > + power_down = false; > + } > } else { > desc->status = FREE; > list_move_tail(&desc->node, &pch->dmac->desc_pool); > @@ -2004,6 +2019,10 @@ static void pl330_tasklet(unsigned long data) > } > } > spin_unlock_irqrestore(&pch->lock, flags); > + > + /* If work list empty, power down */ > + if (power_down) > + pm_runtime_put(pch->dmac->ddma.dev); > } > > bool pl330_filter(struct dma_chan *chan, void *param) > @@ -2073,6 +2092,7 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned > > switch (cmd) { > case DMA_TERMINATE_ALL: > + pm_runtime_get_sync(pl330->ddma.dev); > spin_lock_irqsave(&pch->lock, flags); > > spin_lock(&pl330->lock); > @@ -2099,10 +2119,14 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned > dma_cookie_complete(&desc->txd); > } > > + if (!list_empty(&pch->work_list)) > + pm_runtime_put(pl330->ddma.dev); > + > list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool); > list_splice_tail_init(&pch->work_list, &pl330->desc_pool); > list_splice_tail_init(&pch->completed_list, &pl330->desc_pool); > spin_unlock_irqrestore(&pch->lock, flags); > + pm_runtime_put(pl330->ddma.dev); > break; > case DMA_SLAVE_CONFIG: > slave_config = (struct dma_slave_config *)arg; > @@ -2138,6 +2162,7 @@ static void pl330_free_chan_resources(struct dma_chan *chan) > > tasklet_kill(&pch->task); > > + pm_runtime_get_sync(pch->dmac->ddma.dev); > spin_lock_irqsave(&pch->lock, flags); > > pl330_release_channel(pch->thread); > @@ -2147,6 +2172,7 @@ static void pl330_free_chan_resources(struct dma_chan *chan) > list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); > > spin_unlock_irqrestore(&pch->lock, flags); > + pm_runtime_put(pch->dmac->ddma.dev); > } > > static enum dma_status > @@ -2162,6 +2188,15 @@ static void pl330_issue_pending(struct dma_chan *chan) > unsigned long flags; > > spin_lock_irqsave(&pch->lock, flags); > + if (list_empty(&pch->work_list)) { > + /* > + * Warn on nothing pending. Empty submitted_list may > + * break our pm_runtime usage counter as it is > + * updated on work_list emptiness status. > + */ > + WARN_ON(list_empty(&pch->submitted_list)); > + pm_runtime_get_sync(pch->dmac->ddma.dev); > + } > list_splice_tail_init(&pch->submitted_list, &pch->work_list); > spin_unlock_irqrestore(&pch->lock, flags); > > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > return 0; > } > > +/* > + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver > + * will only disable/enable the clock in runtime PM suspend/resume. > + */ > +static int __maybe_unused pl330_suspend(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + > + if (!pm_runtime_suspended(dev)) > + amba_pclk_disable(pcdev); I would suggest to use pm_runtime_force_suspend() instead of the above. > + amba_pclk_unprepare(pcdev); > + > + return 0; > +} > + > +static int __maybe_unused pl330_resume(struct device *dev) > +{ > + struct amba_device *pcdev = to_amba_device(dev); > + > + amba_pclk_prepare(pcdev); > + if (!pm_runtime_suspended(dev)) > + return amba_pclk_enable(pcdev); The above if statement could be replaced with pm_runtime_force_resume(). Doing that, means the amba_pclk_enable|disable() don't need to be exported from the AMBA bus header, but entirely handled by the AMBA bus itself. > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); > + > static int > pl330_probe(struct amba_device *adev, const struct amba_id *id) > { > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan, > pcfg->num_peri, pcfg->num_events); > You need pm_runtime_set_active() here as well. > + pm_runtime_irq_safe(&adev->dev); > + pm_runtime_put_noidle(&adev->dev); Why pm_runtime_put_noidle(), that seems like you might end up leaving the device in active state - unless you get some request. Likely not what you want? A final question, have you considered using runtime PM autosuspend feature. Any reason to why not? > + > return 0; > probe_err3: > /* Idle the DMAC */ > @@ -2764,6 +2830,8 @@ static int pl330_remove(struct amba_device *adev) > struct pl330_dmac *pl330 = amba_get_drvdata(adev); > struct dma_pl330_chan *pch, *_p; > > + pm_runtime_get_noresume(pl330->ddma.dev); > + > if (adev->dev.of_node) > of_dma_controller_free(adev->dev.of_node); > > @@ -2802,6 +2870,7 @@ static struct amba_driver pl330_driver = { > .drv = { > .owner = THIS_MODULE, > .name = "dma-pl330", > + .pm = &pl330_pm, > }, > .id_table = pl330_ids, > .probe = pl330_probe, > -- > 1.9.1 > Kind regards Uffe -- 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
On ?ro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote: > On 16 September 2014 10:51, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: [...] > > > > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > > return 0; > > } > > > > +/* > > + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver > > + * will only disable/enable the clock in runtime PM suspend/resume. > > + */ > > +static int __maybe_unused pl330_suspend(struct device *dev) > > +{ > > + struct amba_device *pcdev = to_amba_device(dev); > > + > > + if (!pm_runtime_suspended(dev)) > > + amba_pclk_disable(pcdev); > > I would suggest to use pm_runtime_force_suspend() instead of the above. Sure, I can change it but... (see below) > > > + amba_pclk_unprepare(pcdev); > > + > > + return 0; > > +} > > + > > +static int __maybe_unused pl330_resume(struct device *dev) > > +{ > > + struct amba_device *pcdev = to_amba_device(dev); > > + > > + amba_pclk_prepare(pcdev); > > + if (!pm_runtime_suspended(dev)) > > + return amba_pclk_enable(pcdev); > > The above if statement could be replaced with pm_runtime_force_resume(). But that would lead to runtime resuming the device even when it is not needed. We don't have to fully wakeup the device during resume operation when the device was runtime suspended before suspend. > > Doing that, means the amba_pclk_enable|disable() don't need to be > exported from the AMBA bus header, but entirely handled by the AMBA > bus itself. > > > + > > + return 0; > > +} > > + > > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); > > + > > static int > > pl330_probe(struct amba_device *adev, const struct amba_id *id) > > { > > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > > pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan, > > pcfg->num_peri, pcfg->num_events); > > > > You need pm_runtime_set_active() here as well. This is done by amba/bus.c. Do I have to do it again? > > > + pm_runtime_irq_safe(&adev->dev); > > + pm_runtime_put_noidle(&adev->dev); > > Why pm_runtime_put_noidle(), that seems like you might end up leaving > the device in active state - unless you get some request. Likely not > what you want? Hmmm... I am sorry but I do not get the point here. It could be pm_runtime_put() as well because initially we start with counter incremented by amba/bus.c. > A final question, have you considered using runtime PM autosuspend > feature. Any reason to why not? There is no reason, I just implemented the easier way. I was hoping for such ideas of improvement. I'll add autosuspend. Thanks for feedback, I really appreciate it! Best regards, Krzysztof > > > + > > return 0; > > probe_err3: > > /* Idle the DMAC */ > > @@ -2764,6 +2830,8 @@ static int pl330_remove(struct amba_device *adev) > > struct pl330_dmac *pl330 = amba_get_drvdata(adev); > > struct dma_pl330_chan *pch, *_p; > > > > + pm_runtime_get_noresume(pl330->ddma.dev); > > + > > if (adev->dev.of_node) > > of_dma_controller_free(adev->dev.of_node); > > > > @@ -2802,6 +2870,7 @@ static struct amba_driver pl330_driver = { > > .drv = { > > .owner = THIS_MODULE, > > .name = "dma-pl330", > > + .pm = &pl330_pm, > > }, > > .id_table = pl330_ids, > > .probe = pl330_probe, > > -- > > 1.9.1 > > > > Kind regards > Uffe -- 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
On 18 September 2014 12:09, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > On ?ro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote: >> On 16 September 2014 10:51, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > [...] > >> > >> > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, >> > return 0; >> > } >> > >> > +/* >> > + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver >> > + * will only disable/enable the clock in runtime PM suspend/resume. >> > + */ >> > +static int __maybe_unused pl330_suspend(struct device *dev) >> > +{ >> > + struct amba_device *pcdev = to_amba_device(dev); >> > + >> > + if (!pm_runtime_suspended(dev)) >> > + amba_pclk_disable(pcdev); >> >> I would suggest to use pm_runtime_force_suspend() instead of the above. > > Sure, I can change it but... (see below) > >> >> > + amba_pclk_unprepare(pcdev); >> > + >> > + return 0; >> > +} >> > + >> > +static int __maybe_unused pl330_resume(struct device *dev) >> > +{ >> > + struct amba_device *pcdev = to_amba_device(dev); >> > + >> > + amba_pclk_prepare(pcdev); >> > + if (!pm_runtime_suspended(dev)) >> > + return amba_pclk_enable(pcdev); >> >> The above if statement could be replaced with pm_runtime_force_resume(). > > But that would lead to runtime resuming the device even when it is not > needed. We don't have to fully wakeup the device during resume operation > when the device was runtime suspended before suspend. That's true - but that's not a big issue I believe. I think the below problems are worse. :-) 1) You don't disable runtime PM, thus not preventing the runtime PM callbacks from being invoked. 2) You don't update the runtime PM status, thus not reflecting the actual state of the device. Both the above may cause unbalanced clk_enable|disable calls. Don't get me wrong, I certainly think the "don't do runtime PM resume during system PM resume" is interesting to solve. But that it's a common problem and should be supported by the runtime PM API somehow instead. I have been thinking of extending the pm_runtime_force_resume() API to support this, let's see when that happens. :-) > >> >> Doing that, means the amba_pclk_enable|disable() don't need to be >> exported from the AMBA bus header, but entirely handled by the AMBA >> bus itself. >> >> > + >> > + return 0; >> > +} >> > + >> > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); >> > + >> > static int >> > pl330_probe(struct amba_device *adev, const struct amba_id *id) >> > { >> > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) >> > pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan, >> > pcfg->num_peri, pcfg->num_events); >> > >> >> You need pm_runtime_set_active() here as well. > > This is done by amba/bus.c. Do I have to do it again? Ohh, you are right! No need to add it! > >> >> > + pm_runtime_irq_safe(&adev->dev); >> > + pm_runtime_put_noidle(&adev->dev); >> >> Why pm_runtime_put_noidle(), that seems like you might end up leaving >> the device in active state - unless you get some request. Likely not >> what you want? > > Hmmm... I am sorry but I do not get the point here. It could be > pm_runtime_put() as well because initially we start with counter > incremented by amba/bus.c. The "no_idle" version will prevent the device from going inactive, unless a new cycle of pm_runtime_get() to pm_runtime_put() happens, which there are no guarantees of. Kind regards Uffe -- 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
On wto, 2014-09-23 at 17:39 +0200, Ulf Hansson wrote: > On 18 September 2014 12:09, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > On ?ro, 2014-09-17 at 20:42 +0200, Ulf Hansson wrote: > >> On 16 September 2014 10:51, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > > > [...] > > > >> > > >> > @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, > >> > return 0; > >> > } > >> > > >> > +/* > >> > + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver > >> > + * will only disable/enable the clock in runtime PM suspend/resume. > >> > + */ > >> > +static int __maybe_unused pl330_suspend(struct device *dev) > >> > +{ > >> > + struct amba_device *pcdev = to_amba_device(dev); > >> > + > >> > + if (!pm_runtime_suspended(dev)) > >> > + amba_pclk_disable(pcdev); > >> > >> I would suggest to use pm_runtime_force_suspend() instead of the above. > > > > Sure, I can change it but... (see below) > > > >> > >> > + amba_pclk_unprepare(pcdev); > >> > + > >> > + return 0; > >> > +} > >> > + > >> > +static int __maybe_unused pl330_resume(struct device *dev) > >> > +{ > >> > + struct amba_device *pcdev = to_amba_device(dev); > >> > + > >> > + amba_pclk_prepare(pcdev); > >> > + if (!pm_runtime_suspended(dev)) > >> > + return amba_pclk_enable(pcdev); > >> > >> The above if statement could be replaced with pm_runtime_force_resume(). > > > > But that would lead to runtime resuming the device even when it is not > > needed. We don't have to fully wakeup the device during resume operation > > when the device was runtime suspended before suspend. > > That's true - but that's not a big issue I believe. I think the below > problems are worse. :-) > > 1) You don't disable runtime PM, thus not preventing the runtime PM > callbacks from being invoked. > 2) You don't update the runtime PM status, thus not reflecting the > actual state of the device. > > Both the above may cause unbalanced clk_enable|disable calls. > > Don't get me wrong, I certainly think the "don't do runtime PM resume > during system PM resume" is interesting to solve. But that it's a > common problem and should be supported by the runtime PM API somehow > instead. I have been thinking of extending the > pm_runtime_force_resume() API to support this, let's see when that > happens. :-) OK, I get the point. I'll fix this. > > > >> > >> Doing that, means the amba_pclk_enable|disable() don't need to be > >> exported from the AMBA bus header, but entirely handled by the AMBA > >> bus itself. > >> > >> > + > >> > + return 0; > >> > +} > >> > + > >> > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); > >> > + > >> > static int > >> > pl330_probe(struct amba_device *adev, const struct amba_id *id) > >> > { > >> > @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) > >> > pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan, > >> > pcfg->num_peri, pcfg->num_events); > >> > > >> > >> You need pm_runtime_set_active() here as well. > > > > This is done by amba/bus.c. Do I have to do it again? > > Ohh, you are right! No need to add it! > > > > >> > >> > + pm_runtime_irq_safe(&adev->dev); > >> > + pm_runtime_put_noidle(&adev->dev); > >> > >> Why pm_runtime_put_noidle(), that seems like you might end up leaving > >> the device in active state - unless you get some request. Likely not > >> what you want? > > > > Hmmm... I am sorry but I do not get the point here. It could be > > pm_runtime_put() as well because initially we start with counter > > incremented by amba/bus.c. > > The "no_idle" version will prevent the device from going inactive, > unless a new cycle of pm_runtime_get() to pm_runtime_put() happens, > which there are no guarantees of. Right. Thanks for reviewing the code. I'll send the next version removing also the amba_pclk_enable|disable() macros. Best regards, Krzysztof > > Kind regards > Uffe -- 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
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index 4839bfa74a10..9c014874c33d 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -27,6 +27,7 @@ #include <linux/of.h> #include <linux/of_dma.h> #include <linux/err.h> +#include <linux/pm_runtime.h> #include "dmaengine.h" #define PL330_MAX_CHAN 8 @@ -1958,6 +1959,7 @@ static void pl330_tasklet(unsigned long data) struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data; struct dma_pl330_desc *desc, *_dt; unsigned long flags; + bool power_down = false; spin_lock_irqsave(&pch->lock, flags); @@ -1972,10 +1974,17 @@ static void pl330_tasklet(unsigned long data) /* Try to submit a req imm. next to the last completed cookie */ fill_queue(pch); - /* Make sure the PL330 Channel thread is active */ - spin_lock(&pch->thread->dmac->lock); - _start(pch->thread); - spin_unlock(&pch->thread->dmac->lock); + if (list_empty(&pch->work_list)) { + spin_lock(&pch->thread->dmac->lock); + _stop(pch->thread); + spin_unlock(&pch->thread->dmac->lock); + power_down = true; + } else { + /* Make sure the PL330 Channel thread is active */ + spin_lock(&pch->thread->dmac->lock); + _start(pch->thread); + spin_unlock(&pch->thread->dmac->lock); + } while (!list_empty(&pch->completed_list)) { dma_async_tx_callback callback; @@ -1990,6 +1999,12 @@ static void pl330_tasklet(unsigned long data) if (pch->cyclic) { desc->status = PREP; list_move_tail(&desc->node, &pch->work_list); + if (power_down) { + spin_lock(&pch->thread->dmac->lock); + _start(pch->thread); + spin_unlock(&pch->thread->dmac->lock); + power_down = false; + } } else { desc->status = FREE; list_move_tail(&desc->node, &pch->dmac->desc_pool); @@ -2004,6 +2019,10 @@ static void pl330_tasklet(unsigned long data) } } spin_unlock_irqrestore(&pch->lock, flags); + + /* If work list empty, power down */ + if (power_down) + pm_runtime_put(pch->dmac->ddma.dev); } bool pl330_filter(struct dma_chan *chan, void *param) @@ -2073,6 +2092,7 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned switch (cmd) { case DMA_TERMINATE_ALL: + pm_runtime_get_sync(pl330->ddma.dev); spin_lock_irqsave(&pch->lock, flags); spin_lock(&pl330->lock); @@ -2099,10 +2119,14 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned dma_cookie_complete(&desc->txd); } + if (!list_empty(&pch->work_list)) + pm_runtime_put(pl330->ddma.dev); + list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool); list_splice_tail_init(&pch->work_list, &pl330->desc_pool); list_splice_tail_init(&pch->completed_list, &pl330->desc_pool); spin_unlock_irqrestore(&pch->lock, flags); + pm_runtime_put(pl330->ddma.dev); break; case DMA_SLAVE_CONFIG: slave_config = (struct dma_slave_config *)arg; @@ -2138,6 +2162,7 @@ static void pl330_free_chan_resources(struct dma_chan *chan) tasklet_kill(&pch->task); + pm_runtime_get_sync(pch->dmac->ddma.dev); spin_lock_irqsave(&pch->lock, flags); pl330_release_channel(pch->thread); @@ -2147,6 +2172,7 @@ static void pl330_free_chan_resources(struct dma_chan *chan) list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool); spin_unlock_irqrestore(&pch->lock, flags); + pm_runtime_put(pch->dmac->ddma.dev); } static enum dma_status @@ -2162,6 +2188,15 @@ static void pl330_issue_pending(struct dma_chan *chan) unsigned long flags; spin_lock_irqsave(&pch->lock, flags); + if (list_empty(&pch->work_list)) { + /* + * Warn on nothing pending. Empty submitted_list may + * break our pm_runtime usage counter as it is + * updated on work_list emptiness status. + */ + WARN_ON(list_empty(&pch->submitted_list)); + pm_runtime_get_sync(pch->dmac->ddma.dev); + } list_splice_tail_init(&pch->submitted_list, &pch->work_list); spin_unlock_irqrestore(&pch->lock, flags); @@ -2585,6 +2620,34 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan, return 0; } +/* + * Assume that IRQ safe runtime PM is chosen in probe and amba bus driver + * will only disable/enable the clock in runtime PM suspend/resume. + */ +static int __maybe_unused pl330_suspend(struct device *dev) +{ + struct amba_device *pcdev = to_amba_device(dev); + + if (!pm_runtime_suspended(dev)) + amba_pclk_disable(pcdev); + amba_pclk_unprepare(pcdev); + + return 0; +} + +static int __maybe_unused pl330_resume(struct device *dev) +{ + struct amba_device *pcdev = to_amba_device(dev); + + amba_pclk_prepare(pcdev); + if (!pm_runtime_suspended(dev)) + return amba_pclk_enable(pcdev); + + return 0; +} + +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume); + static int pl330_probe(struct amba_device *adev, const struct amba_id *id) { @@ -2738,6 +2801,9 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id) pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan, pcfg->num_peri, pcfg->num_events); + pm_runtime_irq_safe(&adev->dev); + pm_runtime_put_noidle(&adev->dev); + return 0; probe_err3: /* Idle the DMAC */ @@ -2764,6 +2830,8 @@ static int pl330_remove(struct amba_device *adev) struct pl330_dmac *pl330 = amba_get_drvdata(adev); struct dma_pl330_chan *pch, *_p; + pm_runtime_get_noresume(pl330->ddma.dev); + if (adev->dev.of_node) of_dma_controller_free(adev->dev.of_node); @@ -2802,6 +2870,7 @@ static struct amba_driver pl330_driver = { .drv = { .owner = THIS_MODULE, .name = "dma-pl330", + .pm = &pl330_pm, }, .id_table = pl330_ids, .probe = pl330_probe,
This patch adds both normal PM suspend/resume support and runtime PM support to pl330 DMA engine driver. The runtime power management for pl330 DMA driver allows gating of AMBA clock (PDMA) in FSYS clock domain, when the device is not processing any requests. This is necessary to enter low power modes on Exynos SoCs (e.g. LPA on Exynos4x12 or W-AFTR on Exynos3250). Runtime PM resuming of the device may happen in atomic context (during call device_issue_pending()) so pm_runtime_irq_safe() is used. This will lead only to disabling/enabling of the clock but this is sufficient for gating the clock and for reducing energy usage. During system sleep the AMBA bus clock is also unprepared. Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/dma/pl330.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 4 deletions(-)