* [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support
@ 2014-11-12 14:32 Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option Krzysztof Kozlowski
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Hi,
Changes since v10:
==================
1. Bring back wrapper pm_runtime_is_irq_safe() (patch 1/6). This
replaces patch 1/5 from v9 (moving dev->power.irq_safe out of
CONFIG_PM_RUNTIME).
2. Patch 3/6 amba: Don't store current irq_safe during runtime suspend
(suggested by Rafael J. Wysocki).
Because of this change I dropped reviewed-by tags from this patch.
3. Patch 4/6 pl330: Add only runtime PM, without system PM (minor
changes to the patch so I retained the revewed/acked tags).
Add ack from Vinod.
4. Add patch 5/6 pl330: Add system PM to the pl330 which *does not*
depends on runtime PM. I followed the discusions on LKML trying to
reach consensus.
Reasons tosplit previous patch into two patches (1st: runtime,
2nd: system PM):
- easier review,
- the first part (runtime PM) could be applied independently to
system PM support.
6. Patch 6/6 amba: reworked the amba_pclk_* macros into inlines. Previously
they were not used but now patch 5/6 depends on them.
7. Rebase on next-20141112.
Changes since v9:
=================
1. Add patch 1/5: Move dev->power.irq_safe out of CONFIG_PM_RUNTIME.
If CONFIG_PM_RUNTIME is not set, amba bus driver must still know
whether child driver set irq_safe or not. Suggested by Ulf Hansson.
Changes since v8:
=================
1. Remove the pm_runtime_is_irq_safe() wrapper (patch 1).
2. Patch 2/4 (amba): Store current irq_safe during runtime suspend.
Resume will mirror suspend's work. Add a macro for
safe access of irq_safe if CONFIG_PM_RUNTIME is unset.
Dropped reviewed-by Ulf Hansson (major changes in patch).
Changes since v7:
=================
1. Add reviewed-by Ulf Hansson (patches 3, 4 and 5).
2. Patch 2/5: Fix missing return in amba_pclk_prepare() (suggested by
Ulf Hansson).
3. Rebased on next-20141020.
Changes since v6:
=================
1. Add patch 5 removing the amba_pclk_*able macros.
2. Patch 2/5: Remove IS_ERR, use static inline functions instead
of macros.
3. Patch 4/5: Force runtime suspend/resume in system sleep
callbacks. Put with autosuspend at end of probe instead of no_idle.
Suggested by Ulf Hansson.
Changes since v5:
=================
1. Patch 1/4: Add Ulf Hansson's reviewed-by.
2. Patch 4/4: Use PM runtime autosuspend (suggested by Ulf Hansson).
3. Rebase on next-20140922.
Changes since v4:
1. Patch 3/4: Explicitly initialize amba_device.irq_safe field after
probing driver (suggested by Russell King).
Changes since v3:
1. Patch 1/4: Document new API in Documentation/power/runtime_pm.txt
(pointed by Alan Stern).
Changes since v2:
1. Add patch 1 (PM / Runtime: Add getter for querying the IRQ safe option)
2. Add patch 2 (amba: Add helper macros for (un)preparing AMBA clock)
3. Patch 3/4: Rewrite the idea. If IRQ safe runtime PM is set then
do not unprepare/prepare the clocks. Suggested by Russell King.
4. Patch 4/4: During system sleep unprepare the clock.
Changes since v1:
1. Add patch 1 (amba: Allow AMBA drivers to use their own runtime PM).
2. Patch 2/2: Apply Michal Simek's suggestions.
3. Patch 2/2: Fix atomic context safeness in pl330_issue_pending().
Description:
============
This patchset adds runtime and system PM to the pl330 driver.
The runtime PM of pl330 driver requires interrupt safe suspend/resume
callbacks which is in conflict with current amba bus driver.
The latter also unprepares and prepares the AMBA bus clock which
is not safe for atomic context.
The patchset solves this in patch 3/6 by handling clocks in different
way if device driver set interrupt safe runtime PM.
Comments are welcome.
Tested on board with pl330 DMA driver:
- Trats2 (Exynos4212)
Best regards,
Krzysztof Kozlowski
Krzysztof Kozlowski (6):
PM / Runtime: Add getter for querying the IRQ safe option
amba: Add helpers for (un)preparing AMBA clock
amba: Don't unprepare the clocks if device driver wants IRQ safe
runtime PM
dmaengine: pl330: Add runtime Power Management support
dmaengine: pl330: Add PM sleep support
amba: Use inlines instead of macros for amba_pclk_enable/disable
Documentation/power/runtime_pm.txt | 4 ++
drivers/amba/bus.c | 15 ++++--
drivers/dma/pl330.c | 95 ++++++++++++++++++++++++++++++++++++--
include/linux/amba/bus.h | 22 +++++++--
include/linux/pm_runtime.h | 6 +++
5 files changed, 130 insertions(+), 12 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
2014-11-13 1:34 ` Rafael J. Wysocki
2014-11-12 14:32 ` [PATCH v11 2/6] amba: Add helpers for (un)preparing AMBA clock Krzysztof Kozlowski
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime
PM IRQ safe was set or not.
Various bus drivers implementing runtime PM may use choose to suspend
differently based on IRQ safeness status of child driver (e.g. do not
unprepare the clock if IRQ safe is not set).
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Documentation/power/runtime_pm.txt | 4 ++++
include/linux/pm_runtime.h | 6 ++++++
2 files changed, 10 insertions(+)
diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index 0e5ea26b255a..44fe1d28a163 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
- set the power.irq_safe flag for the device, causing the runtime-PM
callbacks to be invoked with interrupts off
+ bool pm_runtime_is_irq_safe(struct device *dev);
+ - return true if power.irq_safe flag was set for the device, causing
+ the runtime-PM callbacks to be invoked with interrupts off
+
void pm_runtime_mark_last_busy(struct device *dev);
- set the power.last_busy field to the current time
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 367f49b9a1c9..44d74f0f182e 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev)
ACCESS_ONCE(dev->power.last_busy) = jiffies;
}
+static inline bool pm_runtime_is_irq_safe(struct device *dev)
+{
+ return dev->power.irq_safe;
+}
+
#else /* !CONFIG_PM_RUNTIME */
static inline int __pm_runtime_idle(struct device *dev, int rpmflags)
@@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev) { return false; }
static inline void pm_runtime_no_callbacks(struct device *dev) {}
static inline void pm_runtime_irq_safe(struct device *dev) {}
+static inline bool pm_runtime_is_irq_safe(struct device *dev) { return false; }
static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; }
static inline void pm_runtime_mark_last_busy(struct device *dev) {}
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 2/6] amba: Add helpers for (un)preparing AMBA clock
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM Krzysztof Kozlowski
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Add amba_pclk_prepare() and amba_pclk_unprepare() inline functions for
handling the AMBA bus clock by device drivers.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
include/linux/amba/bus.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index d024bd9c9d9b..2afc618b15ce 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -98,6 +98,16 @@ void amba_release_regions(struct amba_device *);
#define amba_pclk_disable(d) \
do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
+static inline int amba_pclk_prepare(struct amba_device *dev)
+{
+ return clk_prepare(dev->pclk);
+}
+
+static inline void amba_pclk_unprepare(struct amba_device *dev)
+{
+ clk_unprepare(dev->pclk);
+}
+
/* Some drivers don't use the struct amba_device */
#define AMBA_CONFIG_BITS(a) (((a) >> 24) & 0xff)
#define AMBA_REV_BITS(a) (((a) >> 20) & 0x0f)
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 2/6] amba: Add helpers for (un)preparing AMBA clock Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
2014-11-12 15:29 ` Ulf Hansson
2014-11-12 14:32 ` [PATCH v11 4/6] dmaengine: pl330: Add runtime Power Management support Krzysztof Kozlowski
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
The AMBA bus driver defines runtime Power Management functions which
disable and unprepare AMBA bus clock. This is problematic for runtime PM
because unpreparing a clock might sleep so it is not interrupt safe.
However some drivers may want to implement runtime PM functions in
interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
bus driver should only disable/enable the clock in runtime suspend and
resume callbacks.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
drivers/amba/bus.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index a4ac490dd784..c2153a1d1981 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -95,8 +95,12 @@ static int amba_pm_runtime_suspend(struct device *dev)
struct amba_device *pcdev = to_amba_device(dev);
int ret = pm_generic_runtime_suspend(dev);
- if (ret == 0 && dev->driver)
- clk_disable_unprepare(pcdev->pclk);
+ if (ret == 0 && dev->driver) {
+ if (pm_runtime_is_irq_safe(dev))
+ clk_disable(pcdev->pclk);
+ else
+ clk_disable_unprepare(pcdev->pclk);
+ }
return ret;
}
@@ -107,7 +111,10 @@ static int amba_pm_runtime_resume(struct device *dev)
int ret;
if (dev->driver) {
- ret = clk_prepare_enable(pcdev->pclk);
+ if (pm_runtime_is_irq_safe(dev))
+ ret = clk_enable(pcdev->pclk);
+ else
+ ret = clk_prepare_enable(pcdev->pclk);
/* Failure is probably fatal to the system, but... */
if (ret)
return ret;
@@ -115,7 +122,7 @@ static int amba_pm_runtime_resume(struct device *dev)
return pm_generic_runtime_resume(dev);
}
-#endif
+#endif /* CONFIG_PM */
static const struct dev_pm_ops amba_pm = {
.suspend = pm_generic_suspend,
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 4/6] dmaengine: pl330: Add runtime Power Management support
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
` (2 preceding siblings ...)
2014-11-12 14:32 ` [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 6/6] amba: Use inlines instead of macros for amba_pclk_enable/disable Krzysztof Kozlowski
5 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
This patch adds 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.
Driver uses runtime PM callbacks from amba/bus.c driver only.
Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Vinod Koul <vinod.koul@intel.com>
---
drivers/dma/pl330.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 54 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 83e2257c324a..c3bd3584f261 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
@@ -265,6 +266,9 @@ static unsigned cmd_line;
#define NR_DEFAULT_DESC 16
+/* Delay for runtime PM autosuspend, ms */
+#define PL330_AUTOSUSPEND_DELAY 20
+
/* Populated by the PL330 core driver for DMA API driver's info */
struct pl330_config {
u32 periph_id;
@@ -1958,6 +1962,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 +1977,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 +2002,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 +2022,12 @@ static void pl330_tasklet(unsigned long data)
}
}
spin_unlock_irqrestore(&pch->lock, flags);
+
+ /* If work list empty, power down */
+ if (power_down) {
+ pm_runtime_mark_last_busy(pch->dmac->ddma.dev);
+ pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
+ }
}
bool pl330_filter(struct dma_chan *chan, void *param)
@@ -2073,6 +2097,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 +2124,15 @@ 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_mark_last_busy(pl330->ddma.dev);
+ pm_runtime_put_autosuspend(pl330->ddma.dev);
break;
case DMA_SLAVE_CONFIG:
slave_config = (struct dma_slave_config *)arg;
@@ -2138,6 +2168,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 +2178,8 @@ 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_mark_last_busy(pch->dmac->ddma.dev);
+ pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
}
static enum dma_status
@@ -2162,6 +2195,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);
@@ -2739,6 +2781,12 @@ 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_use_autosuspend(&adev->dev);
+ pm_runtime_set_autosuspend_delay(&adev->dev, PL330_AUTOSUSPEND_DELAY);
+ pm_runtime_mark_last_busy(&adev->dev);
+ pm_runtime_put_autosuspend(&adev->dev);
+
return 0;
probe_err3:
/* Idle the DMAC */
@@ -2765,6 +2813,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);
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
` (3 preceding siblings ...)
2014-11-12 14:32 ` [PATCH v11 4/6] dmaengine: pl330: Add runtime Power Management support Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
2014-11-12 15:28 ` Ulf Hansson
2014-11-13 1:37 ` Rafael J. Wysocki
2014-11-12 14:32 ` [PATCH v11 6/6] amba: Use inlines instead of macros for amba_pclk_enable/disable Krzysztof Kozlowski
5 siblings, 2 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Add system suspend/resume capabilities to the pl330 driver so the amba
bus clock could be also unprepared to conserve energy.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index c3bd3584f261..fd71777fc565 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
return 0;
}
+/*
+ * Runtime PM callbacks are provided by amba/bus.c driver.
+ *
+ * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
+ * bus driver will only disable/enable the clock in runtime PM callbacks.
+ */
+static int __maybe_unused pl330_suspend(struct device *dev)
+{
+ struct amba_device *pcdev = to_amba_device(dev);
+
+ if (!pm_runtime_status_suspended(dev)) {
+ /* amba did not disable the clock */
+ 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);
+ int ret;
+
+ ret = amba_pclk_prepare(pcdev);
+ if (ret)
+ return ret;
+
+ if (!pm_runtime_status_suspended(dev))
+ ret = amba_pclk_enable(pcdev);
+
+ return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
+
static int
pl330_probe(struct amba_device *adev, const struct amba_id *id)
{
@@ -2853,6 +2889,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v11 6/6] amba: Use inlines instead of macros for amba_pclk_enable/disable
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
` (4 preceding siblings ...)
2014-11-12 14:32 ` [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support Krzysztof Kozlowski
@ 2014-11-12 14:32 ` Krzysztof Kozlowski
5 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 14:32 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Ulf Hansson, Alan Stern, linux-pm,
linux-kernel, dmaengine
Cc: Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski
Replace the amba_pclk_enable and amba_pclk_disable macros with static
inline functions and remove checks for IS_ERR. The amba bus clock won't
be ERR because probe would fail before the use of these functions.
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
include/linux/amba/bus.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 2afc618b15ce..7daba00ddabd 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -92,11 +92,15 @@ struct amba_device *amba_find_device(const char *, struct device *, unsigned int
int amba_request_regions(struct amba_device *, const char *);
void amba_release_regions(struct amba_device *);
-#define amba_pclk_enable(d) \
- (IS_ERR((d)->pclk) ? 0 : clk_enable((d)->pclk))
+static inline int amba_pclk_enable(struct amba_device *dev)
+{
+ return clk_enable(dev->pclk);
+}
-#define amba_pclk_disable(d) \
- do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
+static inline void amba_pclk_disable(struct amba_device *dev)
+{
+ clk_disable(dev->pclk);
+}
static inline int amba_pclk_prepare(struct amba_device *dev)
{
--
1.9.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-12 14:32 ` [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support Krzysztof Kozlowski
@ 2014-11-12 15:28 ` Ulf Hansson
2014-11-12 15:53 ` Krzysztof Kozlowski
2014-11-13 1:37 ` Rafael J. Wysocki
1 sibling, 1 reply; 15+ messages in thread
From: Ulf Hansson @ 2014-11-12 15:28 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Alan Stern, linux-pm, linux-kernel,
dmaengine, Kevin Hilman, Laurent Pinchart, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> Add system suspend/resume capabilities to the pl330 driver so the amba
> bus clock could be also unprepared to conserve energy.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c3bd3584f261..fd71777fc565 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> return 0;
> }
>
> +/*
> + * Runtime PM callbacks are provided by amba/bus.c driver.
> + *
> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> + * bus driver will only disable/enable the clock in runtime PM callbacks.
> + */
> +static int __maybe_unused pl330_suspend(struct device *dev)
> +{
> + struct amba_device *pcdev = to_amba_device(dev);
> +
> + if (!pm_runtime_status_suspended(dev)) {
No, this wont work. Consider this scenario:
pm_runtime_status_suspended() returns "true", which means you consider
the device to be runtime PM suspended, thus you will unprepare the
clock below.
Later, a call to pm_runtime_get_sync() is invoked from "somewhere" and
before this driver's system PM resume callback has been invoked. That
will trigger the runtime PM resume callback to be invoked, causing
clock unbalance issues.
You need a pm_runtime_disable() prior checking the runtime PM status
using pm_runtime_status_suspended(). That will prevent this from
happen.
I guess you may see where this is leading. I think
pm_runtime_force_suspend|resume() is well suited for this situation.
> + /* amba did not disable the clock */
> + 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);
> + int ret;
> +
> + ret = amba_pclk_prepare(pcdev);
> + if (ret)
> + return ret;
> +
> + if (!pm_runtime_status_suspended(dev))
> + ret = amba_pclk_enable(pcdev);
> +
> + return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
> +
> static int
> pl330_probe(struct amba_device *adev, const struct amba_id *id)
> {
> @@ -2853,6 +2889,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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM
2014-11-12 14:32 ` [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM Krzysztof Kozlowski
@ 2014-11-12 15:29 ` Ulf Hansson
0 siblings, 0 replies; 15+ messages in thread
From: Ulf Hansson @ 2014-11-12 15:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Alan Stern, linux-pm, linux-kernel,
dmaengine, Kevin Hilman, Laurent Pinchart, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
>
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/amba/bus.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index a4ac490dd784..c2153a1d1981 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -95,8 +95,12 @@ static int amba_pm_runtime_suspend(struct device *dev)
> struct amba_device *pcdev = to_amba_device(dev);
> int ret = pm_generic_runtime_suspend(dev);
>
> - if (ret == 0 && dev->driver)
> - clk_disable_unprepare(pcdev->pclk);
> + if (ret == 0 && dev->driver) {
> + if (pm_runtime_is_irq_safe(dev))
> + clk_disable(pcdev->pclk);
> + else
> + clk_disable_unprepare(pcdev->pclk);
> + }
>
> return ret;
> }
> @@ -107,7 +111,10 @@ static int amba_pm_runtime_resume(struct device *dev)
> int ret;
>
> if (dev->driver) {
> - ret = clk_prepare_enable(pcdev->pclk);
> + if (pm_runtime_is_irq_safe(dev))
> + ret = clk_enable(pcdev->pclk);
> + else
> + ret = clk_prepare_enable(pcdev->pclk);
> /* Failure is probably fatal to the system, but... */
> if (ret)
> return ret;
> @@ -115,7 +122,7 @@ static int amba_pm_runtime_resume(struct device *dev)
>
> return pm_generic_runtime_resume(dev);
> }
> -#endif
> +#endif /* CONFIG_PM */
>
> static const struct dev_pm_ops amba_pm = {
> .suspend = pm_generic_suspend,
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-12 15:28 ` Ulf Hansson
@ 2014-11-12 15:53 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-12 15:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Alan Stern, linux-pm, linux-kernel,
dmaengine, Kevin Hilman, Laurent Pinchart, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
On śro, 2014-11-12 at 16:28 +0100, Ulf Hansson wrote:
> On 12 November 2014 15:32, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > Add system suspend/resume capabilities to the pl330 driver so the amba
> > bus clock could be also unprepared to conserve energy.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> > drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index c3bd3584f261..fd71777fc565 100644
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> > return 0;
> > }
> >
> > +/*
> > + * Runtime PM callbacks are provided by amba/bus.c driver.
> > + *
> > + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> > + * bus driver will only disable/enable the clock in runtime PM callbacks.
> > + */
> > +static int __maybe_unused pl330_suspend(struct device *dev)
> > +{
> > + struct amba_device *pcdev = to_amba_device(dev);
> > +
> > + if (!pm_runtime_status_suspended(dev)) {
>
> No, this wont work. Consider this scenario:
>
> pm_runtime_status_suspended() returns "true", which means you consider
> the device to be runtime PM suspended, thus you will unprepare the
> clock below.
>
> Later, a call to pm_runtime_get_sync() is invoked from "somewhere" and
> before this driver's system PM resume callback has been invoked. That
> will trigger the runtime PM resume callback to be invoked, causing
> clock unbalance issues.
If during system resume the device runtime resume is called BEFORE
normal resume of device, then you're right: clock will be unbalanced.
But such case does not make sense. pm_runtime_get_sync() is called when
someone wants to use the dmaengine/pl330... but the pl330 driver is
still suspended in that time!
If runtime resume is called AFTER normal resume of device - then it is
fine with my code.
> You need a pm_runtime_disable() prior checking the runtime PM status
> using pm_runtime_status_suspended(). That will prevent this from
> happen.
>
> I guess you may see where this is leading. I think
> pm_runtime_force_suspend|resume() is well suited for this situation.
I've been there... :)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option
2014-11-12 14:32 ` [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option Krzysztof Kozlowski
@ 2014-11-13 1:34 ` Rafael J. Wysocki
2014-11-14 8:14 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2014-11-13 1:34 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Len Brown, Pavel Machek, Russell King, Dan Williams, Vinod Koul,
Ulf Hansson, Alan Stern, linux-pm, linux-kernel, dmaengine,
Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On Wednesday, November 12, 2014 03:32:23 PM Krzysztof Kozlowski wrote:
> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime
> PM IRQ safe was set or not.
>
> Various bus drivers implementing runtime PM may use choose to suspend
> differently based on IRQ safeness status of child driver (e.g. do not
> unprepare the clock if IRQ safe is not set).
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
I'm fine with this one, please feel free to add my ACK if that needs to go
through a different tree.
> ---
> Documentation/power/runtime_pm.txt | 4 ++++
> include/linux/pm_runtime.h | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
> index 0e5ea26b255a..44fe1d28a163 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
> - set the power.irq_safe flag for the device, causing the runtime-PM
> callbacks to be invoked with interrupts off
>
> + bool pm_runtime_is_irq_safe(struct device *dev);
> + - return true if power.irq_safe flag was set for the device, causing
> + the runtime-PM callbacks to be invoked with interrupts off
> +
> void pm_runtime_mark_last_busy(struct device *dev);
> - set the power.last_busy field to the current time
>
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 367f49b9a1c9..44d74f0f182e 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev)
> ACCESS_ONCE(dev->power.last_busy) = jiffies;
> }
>
> +static inline bool pm_runtime_is_irq_safe(struct device *dev)
> +{
> + return dev->power.irq_safe;
> +}
> +
> #else /* !CONFIG_PM_RUNTIME */
>
> static inline int __pm_runtime_idle(struct device *dev, int rpmflags)
> @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>
> static inline void pm_runtime_no_callbacks(struct device *dev) {}
> static inline void pm_runtime_irq_safe(struct device *dev) {}
> +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return false; }
>
> static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; }
> static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-12 14:32 ` [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support Krzysztof Kozlowski
2014-11-12 15:28 ` Ulf Hansson
@ 2014-11-13 1:37 ` Rafael J. Wysocki
2014-11-13 9:03 ` Ulf Hansson
1 sibling, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2014-11-13 1:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Len Brown, Pavel Machek, Russell King, Dan Williams, Vinod Koul,
Ulf Hansson, Alan Stern, linux-pm, linux-kernel, dmaengine,
Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote:
> Add system suspend/resume capabilities to the pl330 driver so the amba
> bus clock could be also unprepared to conserve energy.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c3bd3584f261..fd71777fc565 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> return 0;
> }
>
> +/*
> + * Runtime PM callbacks are provided by amba/bus.c driver.
> + *
> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> + * bus driver will only disable/enable the clock in runtime PM callbacks.
> + */
> +static int __maybe_unused pl330_suspend(struct device *dev)
> +{
> + struct amba_device *pcdev = to_amba_device(dev);
> +
> + if (!pm_runtime_status_suspended(dev)) {
It generally isn't safe to call that in .suspend(), because the status may still
change in theory. On the other hand, if you do that in .suspend_late(), you'll
be safe from that.
> + /* amba did not disable the clock */
> + 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);
> + int ret;
> +
> + ret = amba_pclk_prepare(pcdev);
> + if (ret)
> + return ret;
> +
> + if (!pm_runtime_status_suspended(dev))
> + ret = amba_pclk_enable(pcdev);
> +
> + return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
> +
> static int
> pl330_probe(struct amba_device *adev, const struct amba_id *id)
> {
> @@ -2853,6 +2889,7 @@ static struct amba_driver pl330_driver = {
> .drv = {
> .owner = THIS_MODULE,
> .name = "dma-pl330",
> + .pm = &pl330_pm,
> },
> .id_table = pl330_ids,
> .probe = pl330_probe,
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-13 1:37 ` Rafael J. Wysocki
@ 2014-11-13 9:03 ` Ulf Hansson
2014-11-14 8:13 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Ulf Hansson @ 2014-11-13 9:03 UTC (permalink / raw)
To: Rafael J. Wysocki, Krzysztof Kozlowski
Cc: Len Brown, Pavel Machek, Russell King, Dan Williams, Vinod Koul,
Alan Stern, linux-pm, linux-kernel, dmaengine, Kevin Hilman,
Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On 13 November 2014 02:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote:
>> Add system suspend/resume capabilities to the pl330 driver so the amba
>> bus clock could be also unprepared to conserve energy.
>>
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> ---
>> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index c3bd3584f261..fd71777fc565 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
>> return 0;
>> }
>>
>> +/*
>> + * Runtime PM callbacks are provided by amba/bus.c driver.
>> + *
>> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
>> + * bus driver will only disable/enable the clock in runtime PM callbacks.
>> + */
>> +static int __maybe_unused pl330_suspend(struct device *dev)
>> +{
>> + struct amba_device *pcdev = to_amba_device(dev);
>> +
>> + if (!pm_runtime_status_suspended(dev)) {
>
> It generally isn't safe to call that in .suspend(), because the status may still
> change in theory. On the other hand, if you do that in .suspend_late(), you'll
> be safe from that.
>
Just to clarify that statement; it's safe in a ->suspend_late()
callback, because runtime PM has been disabled by the PM core.
That's also the reason why the pm_runtime_force_suspend() helper is
disabling runtime PM, such it may be used in some of the earlier
phases of the system PM callbacks.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support
2014-11-13 9:03 ` Ulf Hansson
@ 2014-11-14 8:13 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-14 8:13 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Len Brown, Pavel Machek, Russell King,
Dan Williams, Vinod Koul, Alan Stern, linux-pm, linux-kernel,
dmaengine, Kevin Hilman, Laurent Pinchart, Kyungmin Park,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
On czw, 2014-11-13 at 10:03 +0100, Ulf Hansson wrote:
> On 13 November 2014 02:37, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, November 12, 2014 03:32:27 PM Krzysztof Kozlowski wrote:
> >> Add system suspend/resume capabilities to the pl330 driver so the amba
> >> bus clock could be also unprepared to conserve energy.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >> ---
> >> drivers/dma/pl330.c | 37 +++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 37 insertions(+)
> >>
> >> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> >> index c3bd3584f261..fd71777fc565 100644
> >> --- a/drivers/dma/pl330.c
> >> +++ b/drivers/dma/pl330.c
> >> @@ -2627,6 +2627,42 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> >> return 0;
> >> }
> >>
> >> +/*
> >> + * Runtime PM callbacks are provided by amba/bus.c driver.
> >> + *
> >> + * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
> >> + * bus driver will only disable/enable the clock in runtime PM callbacks.
> >> + */
> >> +static int __maybe_unused pl330_suspend(struct device *dev)
> >> +{
> >> + struct amba_device *pcdev = to_amba_device(dev);
> >> +
> >> + if (!pm_runtime_status_suspended(dev)) {
> >
> > It generally isn't safe to call that in .suspend(), because the status may still
> > change in theory. On the other hand, if you do that in .suspend_late(), you'll
> > be safe from that.
> >
>
> Just to clarify that statement; it's safe in a ->suspend_late()
> callback, because runtime PM has been disabled by the PM core.
>
> That's also the reason why the pm_runtime_force_suspend() helper is
> disabling runtime PM, such it may be used in some of the earlier
> phases of the system PM callbacks.
So actually only pm_runtime_disable() is missing here (as you mentioned
earlier)?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option
2014-11-13 1:34 ` Rafael J. Wysocki
@ 2014-11-14 8:14 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2014-11-14 8:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Pavel Machek, Russell King, Dan Williams, Vinod Koul,
Ulf Hansson, Alan Stern, linux-pm, linux-kernel, dmaengine,
Kevin Hilman, Laurent Pinchart, Kyungmin Park, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On czw, 2014-11-13 at 02:34 +0100, Rafael J. Wysocki wrote:
> On Wednesday, November 12, 2014 03:32:23 PM Krzysztof Kozlowski wrote:
> > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime
> > PM IRQ safe was set or not.
> >
> > Various bus drivers implementing runtime PM may use choose to suspend
> > differently based on IRQ safeness status of child driver (e.g. do not
> > unprepare the clock if IRQ safe is not set).
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> I'm fine with this one, please feel free to add my ACK if that needs to go
> through a different tree.
Thanks!
With your ack I'll push patchset through Russell's patch system.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-11-14 8:14 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-12 14:32 [PATCH v11 0/6] amba/dmaengine: pl330: add Power Management support Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 1/6] PM / Runtime: Add getter for querying the IRQ safe option Krzysztof Kozlowski
2014-11-13 1:34 ` Rafael J. Wysocki
2014-11-14 8:14 ` Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 2/6] amba: Add helpers for (un)preparing AMBA clock Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 3/6] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM Krzysztof Kozlowski
2014-11-12 15:29 ` Ulf Hansson
2014-11-12 14:32 ` [PATCH v11 4/6] dmaengine: pl330: Add runtime Power Management support Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 5/6] dmaengine: pl330: Add PM sleep support Krzysztof Kozlowski
2014-11-12 15:28 ` Ulf Hansson
2014-11-12 15:53 ` Krzysztof Kozlowski
2014-11-13 1:37 ` Rafael J. Wysocki
2014-11-13 9:03 ` Ulf Hansson
2014-11-14 8:13 ` Krzysztof Kozlowski
2014-11-12 14:32 ` [PATCH v11 6/6] amba: Use inlines instead of macros for amba_pclk_enable/disable Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).