[PATCH v8 0/5] amba/dma: pl330: add Power Management support

[PATCH v8 0/5] amba/dma: pl330: add Power Management support

[Krzysztof Kozlowski]

Hi,


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/5 by handling clocks in different
way if device driver set interrupt safe runtime PM.

Any comments are welcome.


Tested on board with pl330 DMA driver:
 - Trats2 (Exynos4212)


TODO/ideas:
===========
Consider renaming existing pm_runtime_irq_safe() function to keep
up with naming convention after adding pm_runtime_is_irq_safe() in
patch 1/5.
This naming:
 - pm_runtime_irq_safe() as setter,
 - pm_runtime_is_irq_safe() as getter,
is contradictory to naming of other PM runtime functions, e.g.:
 - pm_runtime_set_active() as setter,
 - pm_runtime_active() as getter.
I didn't change the naming of pm_runtime_irq_safe() because I wanted
to minimize the impact of the patches.


Best regards,
Krzysztof Kozlowski


Krzysztof Kozlowski (5):
  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
  dma: pl330: add Power Management support
  amba: Remove unused amba_pclk_enable/disable macros

 Documentation/power/runtime_pm.txt |  4 ++
 drivers/amba/bus.c                 | 29 ++++++++++--
 drivers/dma/pl330.c                | 94 ++++++++++++++++++++++++++++++++++++--
 include/linux/amba/bus.h           | 13 ++++--
 include/linux/pm_runtime.h         |  6 +++
 5 files changed, 134 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[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 f32ce5419573..397b81593142 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

[PATCH v8 2/5] amba: Add helpers for (un)preparing AMBA clock

[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>
---
 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 c324f5700d1a..ac02f9bd63dc 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -97,6 +97,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

[PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[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.

Detect the device driver behavior after calling its probe function and
store it. During runtime suspend/resume deal with clocks according to
stored value.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c       | 29 +++++++++++++++++++++++++----
 include/linux/amba/bus.h |  1 +
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1b5be3..474434e1b1b9 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -95,8 +95,18 @@ 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) {
+		/*
+		 * Drivers should not change pm_runtime_irq_safe()
+		 * after probe.
+		 */
+		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
+
+		if (pcdev->irq_safe)
+			clk_disable(pcdev->pclk);
+		else
+			clk_disable_unprepare(pcdev->pclk);
+	}
 
 	return ret;
 }
@@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
-		ret = clk_prepare_enable(pcdev->pclk);
+		/*
+		 * Drivers should not change pm_runtime_irq_safe()
+		 * after probe.
+		 */
+		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
+
+		if (pcdev->irq_safe)
+			ret = clk_enable(pcdev->pclk);
+		else
+			ret = clk_prepare_enable(pcdev->pclk);
 		/* Failure is probably fatal to the system, but... */
 		if (ret)
 			return ret;
@@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
 		pm_runtime_enable(dev);
 
 		ret = pcdrv->probe(pcdev, id);
-		if (ret == 0)
+		if (ret == 0) {
+			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
 			break;
+		}
 
 		pm_runtime_disable(dev);
 		pm_runtime_set_suspended(dev);
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index ac02f9bd63dc..c4bae79851fb 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	unsigned int		irq_safe:1;
 };
 
 struct amba_driver {
-- 
1.9.1

[PATCH v8 4/5] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

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>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/dma/pl330.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa74a10..b123431e62ed 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);
 
@@ -2585,6 +2627,41 @@ 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);
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	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);
+
+	/*
+	 * TODO: Idea for future. The device should not be woken up after
+	 * system resume if it is not needed. It could stay runtime suspended
+	 * waiting for DMA requests. However for safe suspend and resume we
+	 * forcibly resume the device here.
+	 */
+	return pm_runtime_force_resume(dev);
+}
+
+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 +2815,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 */
@@ -2764,6 +2847,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 +2887,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

[PATCH v8 5/5] amba: Remove unused amba_pclk_enable/disable macros

[Krzysztof Kozlowski]

Remove the amba_pclk_enable and amba_pclk_disable macros because they
are not used by the drivers.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/amba/bus.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index c4bae79851fb..566adf0e0412 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -92,12 +92,6 @@ 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))
-
-#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);
-- 
1.9.1

Re: [PATCH v8 2/5] amba: Add helpers for (un)preparing AMBA clock

[Ulf Hansson]

On 20 October 2014 11:04, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 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 c324f5700d1a..ac02f9bd63dc 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -97,6 +97,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
>

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Krzysztof Kozlowski]

On pon, 2014-10-20 at 11:04 +0200, 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>

Rafael, Len, Pavel,

Is proposed API ok? Do you have any comments?

I'll upload whole patchset to Russell's patch tracking system. However
an ack from PM maintainer is probably needed.

Best regards,
Krzysztof


> ---
>  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 f32ce5419573..397b81593142 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) {}

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Ulf Hansson]

On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> On pon, 2014-10-20 at 11:04 +0200, 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>
>
> Rafael, Len, Pavel,
>
> Is proposed API ok? Do you have any comments?
>
> I'll upload whole patchset to Russell's patch tracking system. However
> an ack from PM maintainer is probably needed.

I would actually prefer the opposite. It's better if we can take these
patches through Rafaels tree.

1) I have posted patches for the amba bus, these may have merge
conflicts with your changes.
2) We may start using the new runtime PM API within genpd as of now.

Kind regards
Uffe

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Russell King - ARM Linux]

On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote:
> On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > On pon, 2014-10-20 at 11:04 +0200, 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>
> >
> > Rafael, Len, Pavel,
> >
> > Is proposed API ok? Do you have any comments?
> >
> > I'll upload whole patchset to Russell's patch tracking system. However
> > an ack from PM maintainer is probably needed.
> 
> I would actually prefer the opposite. It's better if we can take these
> patches through Rafaels tree.
> 
> 1) I have posted patches for the amba bus, these may have merge
> conflicts with your changes.
> 2) We may start using the new runtime PM API within genpd as of now.

Stop this broken thinking.  The solution is simple.

Put the generic runtime PM changes into the PM tree in a separate /clean/
branch.  Merge them into the PM-next branch.  Notify me where I can
pull that /clean/ branch.

I pull that clean branch into my tree, and then apply the AMBA specific
patches on top.

That means Rafael has the PM changes, I have the dependent PM changes,
and I also have the AMBA changes, and no one ends up carrying changes
which are inappropriate.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Krzysztof Kozlowski]

On pią, 2014-10-31 at 10:29 +0100, Ulf Hansson wrote:
> On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > On pon, 2014-10-20 at 11:04 +0200, 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>
> >
> > Rafael, Len, Pavel,
> >
> > Is proposed API ok? Do you have any comments?
> >
> > I'll upload whole patchset to Russell's patch tracking system. However
> > an ack from PM maintainer is probably needed.
> 
> I would actually prefer the opposite. It's better if we can take these
> patches through Rafaels tree.
> 
> 1) I have posted patches for the amba bus, these may have merge
> conflicts with your changes.
> 2) We may start using the new runtime PM API within genpd as of now.

I don't mind and it makes sense to me.
This way I need an ack from Russell or Vinod...


Best regards,
Krzysztof

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Ulf Hansson]

On 31 October 2014 10:33, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote:
>> On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
>> > On pon, 2014-10-20 at 11:04 +0200, 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>
>> >
>> > Rafael, Len, Pavel,
>> >
>> > Is proposed API ok? Do you have any comments?
>> >
>> > I'll upload whole patchset to Russell's patch tracking system. However
>> > an ack from PM maintainer is probably needed.
>>
>> I would actually prefer the opposite. It's better if we can take these
>> patches through Rafaels tree.
>>
>> 1) I have posted patches for the amba bus, these may have merge
>> conflicts with your changes.
>> 2) We may start using the new runtime PM API within genpd as of now.
>
> Stop this broken thinking.  The solution is simple.
>
> Put the generic runtime PM changes into the PM tree in a separate /clean/
> branch.  Merge them into the PM-next branch.  Notify me where I can
> pull that /clean/ branch.
>
> I pull that clean branch into my tree, and then apply the AMBA specific
> patches on top.
>
> That means Rafael has the PM changes, I have the dependent PM changes,
> and I also have the AMBA changes, and no one ends up carrying changes
> which are inappropriate.

Okay, I have no concerns with this approach.

I have also checked the potential upcoming merge conflict in the amba
bus, since I doubt you will pull in bigger changes from his tree!?
Anyway, that conflict should be trivial to resolve if that happens.

Kind regards
Uffe

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Pavel Machek]

On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> On pon, 2014-10-20 at 11:04 +0200, 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>
> 
> Rafael, Len, Pavel,
> 
> Is proposed API ok? Do you have any comments?
> 
> I'll upload whole patchset to Russell's patch tracking system. However
> an ack from PM maintainer is probably needed.

I don't like the API. Having callbacks work in different context (irq
/ noirq) based on what another function reports is ugly.

What is the penalty if we always decide callbacks are not IRQ safe?
								Pavel

> > --- 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
> >  
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Krzysztof Kozlowski]

On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> > On pon, 2014-10-20 at 11:04 +0200, 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>
> > 
> > Rafael, Len, Pavel,
> > 
> > Is proposed API ok? Do you have any comments?
> > 
> > I'll upload whole patchset to Russell's patch tracking system. However
> > an ack from PM maintainer is probably needed.
> 
> I don't like the API. Having callbacks work in different context (irq
> / noirq) based on what another function reports is ugly.
> 
> What is the penalty if we always decide callbacks are not IRQ safe?

Then pm_runtime_get_sync() could not be called in atomic context. The
pl330 runtime PM would have to be completely reworked because one
pm_runtime_get_sync() is called in device_issue_pending which cannot
sleep (at least in non preemptible kernels). Probably this can be solved
some way... 

Best regards,
Krzysztof


> 
> > > --- 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
> > >  

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Russell King - ARM Linux]

On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote:
> [CC list trimmed + added Kevin Hilman]
> 
> On Monday, October 20, 2014 11:04:44 AM 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>
> 
> So why do we need to add the wrapper?
> 
> And it goes kind of against the intention which was to set irq_safe when
> we knew that the callbacks were safe to be executed from interrupt context
> and not when we wished that to be the case.

This was provided in the covering email - I quote:

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/5 by handling clocks in different
way if device driver set interrupt safe runtime PM.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Rafael J. Wysocki]

[CC list trimmed + added Kevin Hilman]

On Monday, October 20, 2014 11:04:44 AM 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>

So why do we need to add the wrapper?

And it goes kind of against the intention which was to set irq_safe when
we knew that the callbacks were safe to be executed from interrupt context
and not when we wished that to be the case.

> ---
>  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 f32ce5419573..397b81593142 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.

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Laurent Pinchart]

Hi Krzysztof,

On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote:
> On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> >> On pon, 2014-10-20 at 11:04 +0200, 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>
> >> 
> >> Rafael, Len, Pavel,
> >> 
> >> Is proposed API ok? Do you have any comments?
> >> 
> >> I'll upload whole patchset to Russell's patch tracking system. However
> >> an ack from PM maintainer is probably needed.
> > 
> > I don't like the API. Having callbacks work in different context (irq
> > / noirq) based on what another function reports is ugly.
> > 
> > What is the penalty if we always decide callbacks are not IRQ safe?
> 
> Then pm_runtime_get_sync() could not be called in atomic context. The
> pl330 runtime PM would have to be completely reworked because one
> pm_runtime_get_sync() is called in device_issue_pending which cannot
> sleep (at least in non preemptible kernels). Probably this can be solved
> some way...

Many other drivers suffer from the same problem. While I won't reject your 
proposed fix, I would prefer a more generic approach.

One option that has been discussed previously was to use a work queue to delay 
starting the DMA transfer to an interruptible context where 
pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
even that won't work in all cases as the DMA slave might need the transfer to 
be started before enabling part of its hardware (OMAP audio seem to be such a 
case).

I've heard a rumor of a possible DMA engine rework to forbid calling the 
descriptor preparation API from atomic context. This could be used as a base 
to implement runtime PM, as DMA slave drivers should not prepare descriptors 
if they don't need to use them. However that's a long term plan, and we need a 
solution sooner than that.

I've been toying with the idea of adding explicit open/close (or whatever we 
would call them) operations to the DMA engine API. Those would be used by DMA 
slave drivers to signal that they will start/stop using the DMA engine.

If (1) we must start the DMA synchronously with a DMA slave call, (2) need to 
sleep to handle PM, and (3) don't want to keep the DMA engine powered for as 
long as one channel is requested, then we need to turn at least preparation as 
not callable in atomic context, or introduce a new operation.

[1] http://www.spinics.net/lists/dmaengine/msg01548.html

> >>> --- 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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Rafael J. Wysocki]

On Friday, October 31, 2014 11:04:52 PM Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote:
> > [CC list trimmed + added Kevin Hilman]
> > 
> > On Monday, October 20, 2014 11:04:44 AM 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>
> > 
> > So why do we need to add the wrapper?
> > 
> > And it goes kind of against the intention which was to set irq_safe when
> > we knew that the callbacks were safe to be executed from interrupt context
> > and not when we wished that to be the case.
> 
> This was provided in the covering email - I quote:
> 
> 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/5 by handling clocks in different
> way if device driver set interrupt safe runtime PM.

So I'm still unsure why we need the wrapper.  IMHO this check in particular:

	WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));

(and should that be WARN_ON_ONCE(), for that matter?), looks better this way:

	WARN_ON(pcdev->irq_safe != dev->power.irq_safe);

and so on, pretty much.

Besides, these special "irq safe" code paths in the bus type look
considerably ugly to me.  I'd probably use an "irq safe" PM domain for
that device and put it in there instead of doing the

	pcdev->irq_safe = pm_runtime_is_irq_safe(dev);

thing in amba_probe().  But that's just me. :-)

There's one weak point in [3/5], but let me comment it in there.

Rafael

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Rafael J. Wysocki]

On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski 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.
> 
> Detect the device driver behavior after calling its probe function and
> store it. During runtime suspend/resume deal with clocks according to
> stored value.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/amba/bus.c       | 29 +++++++++++++++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..474434e1b1b9 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -95,8 +95,18 @@ 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) {
> +		/*
> +		 * Drivers should not change pm_runtime_irq_safe()
> +		 * after probe.
> +		 */
> +		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> +		if (pcdev->irq_safe)
> +			clk_disable(pcdev->pclk);
> +		else
> +			clk_disable_unprepare(pcdev->pclk);
> +	}
>  
>  	return ret;
>  }
> @@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev)
>  	int ret;
>  
>  	if (dev->driver) {
> -		ret = clk_prepare_enable(pcdev->pclk);
> +		/*
> +		 * Drivers should not change pm_runtime_irq_safe()
> +		 * after probe.
> +		 */
> +		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> +		if (pcdev->irq_safe)
> +			ret = clk_enable(pcdev->pclk);
> +		else
> +			ret = clk_prepare_enable(pcdev->pclk);
>  		/* Failure is probably fatal to the system, but... */
>  		if (ret)
>  			return ret;
> @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
>  		pm_runtime_enable(dev);
>  
>  		ret = pcdrv->probe(pcdev, id);
> -		if (ret == 0)
> +		if (ret == 0) {
> +			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);

This looks racy.

Is it guaranteed that runtime PM callbacks won't be run for the device
after pcdrv->probe() has returned and before setting pcdev->irq_safe?
If not, inconsistent behavior may ensue.

>  			break;
> +		}
>  
>  		pm_runtime_disable(dev);
>  		pm_runtime_set_suspended(dev);
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index ac02f9bd63dc..c4bae79851fb 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>  	struct clk		*pclk;
>  	unsigned int		periphid;
>  	unsigned int		irq[AMBA_NR_IRQS];
> +	unsigned int		irq_safe:1;
>  };
>  
>  struct amba_driver {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Russell King - ARM Linux]

On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> >  		pm_runtime_enable(dev);
> >  
> >  		ret = pcdrv->probe(pcdev, id);
> > -		if (ret == 0)
> > +		if (ret == 0) {
> > +			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> 
> This looks racy.
> 
> Is it guaranteed that runtime PM callbacks won't be run for the device
> after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> If not, inconsistent behavior may ensue.

You are absolutely correct.  So that knocks that idea on its head.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Russell King - ARM Linux]

On Sat, Nov 01, 2014 at 12:55:14AM +0000, Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> > >  		pm_runtime_enable(dev);
> > >  
> > >  		ret = pcdrv->probe(pcdev, id);
> > > -		if (ret == 0)
> > > +		if (ret == 0) {
> > > +			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> > 
> > This looks racy.
> > 
> > Is it guaranteed that runtime PM callbacks won't be run for the device
> > after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> > If not, inconsistent behavior may ensue.
> 
> You are absolutely correct.  So that knocks that idea on its head.

Actually, I think we shouldn't give up hope here.  Currently, we do this:

                pm_runtime_get_noresume(dev);
                pm_runtime_set_active(dev);
                pm_runtime_enable(dev);

                ret = pcdrv->probe(pcdev, id);

What we could do is:

		pm_runtime_get_noresume(dev);
                pm_runtime_get_noresume(dev);
                pm_runtime_set_active(dev);
                pm_runtime_enable(dev);

                ret = pcdrv->probe(pcdev, id);
		if (ret == 0) {
			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
			pm_runtime_put(dev);
			break;
		}

                pm_runtime_disable(dev);
                pm_runtime_set_suspended(dev);
                pm_runtime_put_noidle(dev);
                pm_runtime_put_noidle(dev);

which would ensure that we hold a usecount until after the probe function
has returned.  Would that work?

I'll give you that it's pretty horrid.

Would another possible solution be to remember the irq-safeness in the
suspend handler, and use that in the resume handler?  Resume should
/always/ undo what the suspend handler previously did wrt clk API stuff.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On sob, 2014-11-01 at 01:01 +0000, Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 12:55:14AM +0000, Russell King - ARM Linux wrote:
> > On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote:
> > > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote:
> > > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev)
> > > >  		pm_runtime_enable(dev);
> > > >  
> > > >  		ret = pcdrv->probe(pcdev, id);
> > > > -		if (ret == 0)
> > > > +		if (ret == 0) {
> > > > +			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> > > 
> > > This looks racy.
> > > 
> > > Is it guaranteed that runtime PM callbacks won't be run for the device
> > > after pcdrv->probe() has returned and before setting pcdev->irq_safe?
> > > If not, inconsistent behavior may ensue.
> > 
> > You are absolutely correct.  So that knocks that idea on its head.
> 
> Actually, I think we shouldn't give up hope here.  Currently, we do this:
> 
>                 pm_runtime_get_noresume(dev);
>                 pm_runtime_set_active(dev);
>                 pm_runtime_enable(dev);
> 
>                 ret = pcdrv->probe(pcdev, id);
> 
> What we could do is:
> 
> 		pm_runtime_get_noresume(dev);
>                 pm_runtime_get_noresume(dev);
>                 pm_runtime_set_active(dev);
>                 pm_runtime_enable(dev);
> 
>                 ret = pcdrv->probe(pcdev, id);
> 		if (ret == 0) {
> 			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> 			pm_runtime_put(dev);
> 			break;
> 		}
> 
>                 pm_runtime_disable(dev);
>                 pm_runtime_set_suspended(dev);
>                 pm_runtime_put_noidle(dev);
>                 pm_runtime_put_noidle(dev);
> 
> which would ensure that we hold a usecount until after the probe function
> has returned.  Would that work?
> 
> I'll give you that it's pretty horrid.

> Would another possible solution be to remember the irq-safeness in the
> suspend handler, and use that in the resume handler?  Resume should
> /always/ undo what the suspend handler previously did wrt clk API stuff.

I think the second solution could be more readable. The WARN_ON wouldn't
be needed. However this won't solve the two dual nature of runtime
callbacks.

I wondered also about removing runtime PM callbacks from amba/bus.c
completely and moving this to child drivers. This way runtime PM would
be obvious in each driver case.

Best regards,
Krzysztof

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Krzysztof Kozlowski]

On sob, 2014-11-01 at 01:42 +0100, Rafael J. Wysocki wrote:
> On Friday, October 31, 2014 11:04:52 PM Russell King - ARM Linux wrote:
> > On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote:
> > > [CC list trimmed + added Kevin Hilman]
> > > 
> > > On Monday, October 20, 2014 11:04:44 AM 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>
> > > 
> > > So why do we need to add the wrapper?
> > > 
> > > And it goes kind of against the intention which was to set irq_safe when
> > > we knew that the callbacks were safe to be executed from interrupt context
> > > and not when we wished that to be the case.
> > 
> > This was provided in the covering email - I quote:
> > 
> > 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/5 by handling clocks in different
> > way if device driver set interrupt safe runtime PM.
> 
> So I'm still unsure why we need the wrapper.  IMHO this check in particular:
> 
> 	WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> 
> (and should that be WARN_ON_ONCE(), for that matter?), looks better this way:
> 
> 	WARN_ON(pcdev->irq_safe != dev->power.irq_safe);
> 
> and so on, pretty much.

I used the wrapper only to hide the actual code behind interface but it
don't really matter to me.

> Besides, these special "irq safe" code paths in the bus type look
> considerably ugly to me.  I'd probably use an "irq safe" PM domain for
> that device and put it in there instead of doing the
> 
> 	pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
> 
> thing in amba_probe().  But that's just me. :-)

The device is not attached to any domain and there is no hardware domain
matching.

Thanks for feedback!

Best regards,
Krzysztof

> 
> There's one weak point in [3/5], but let me comment it in there.
> 
> Rafael

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Krzysztof Kozlowski]

On sob, 2014-11-01 at 02:29 +0200, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote:
> > On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> > >> On pon, 2014-10-20 at 11:04 +0200, 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>
> > >> 
> > >> Rafael, Len, Pavel,
> > >> 
> > >> Is proposed API ok? Do you have any comments?
> > >> 
> > >> I'll upload whole patchset to Russell's patch tracking system. However
> > >> an ack from PM maintainer is probably needed.
> > > 
> > > I don't like the API. Having callbacks work in different context (irq
> > > / noirq) based on what another function reports is ugly.
> > > 
> > > What is the penalty if we always decide callbacks are not IRQ safe?
> > 
> > Then pm_runtime_get_sync() could not be called in atomic context. The
> > pl330 runtime PM would have to be completely reworked because one
> > pm_runtime_get_sync() is called in device_issue_pending which cannot
> > sleep (at least in non preemptible kernels). Probably this can be solved
> > some way...
> 
> Many other drivers suffer from the same problem. While I won't reject your 
> proposed fix, I would prefer a more generic approach.
> 
> One option that has been discussed previously was to use a work queue to delay 
> starting the DMA transfer to an interruptible context where 
> pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
> even that won't work in all cases as the DMA slave might need the transfer to 
> be started before enabling part of its hardware (OMAP audio seem to be such a 
> case).
> 
> I've heard a rumor of a possible DMA engine rework to forbid calling the 
> descriptor preparation API from atomic context. This could be used as a base 
> to implement runtime PM, as DMA slave drivers should not prepare descriptors 
> if they don't need to use them. However that's a long term plan, and we need a 
> solution sooner than that.
> 
> I've been toying with the idea of adding explicit open/close (or whatever we 
> would call them) operations to the DMA engine API. Those would be used by DMA 
> slave drivers to signal that they will start/stop using the DMA engine.
> 
> If (1) we must start the DMA synchronously with a DMA slave call, (2) need to 
> sleep to handle PM, and (3) don't want to keep the DMA engine powered for as 
> long as one channel is requested, then we need to turn at least preparation as 
> not callable in atomic context, or introduce a new operation.
> 
> [1] http://www.spinics.net/lists/dmaengine/msg01548.html

That makes sense. However I am not familiar with DMA core code as much
as I think it would be needed to make such generic changes :). I'll
stick to one driver for now.

Thanks for comments!
Krzysztof

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Russell King - ARM Linux]

On Mon, Nov 03, 2014 at 09:36:45AM +0100, Krzysztof Kozlowski wrote:
> On sob, 2014-11-01 at 01:01 +0000, Russell King - ARM Linux wrote:
> > Would another possible solution be to remember the irq-safeness in the
> > suspend handler, and use that in the resume handler?  Resume should
> > /always/ undo what the suspend handler previously did wrt clk API stuff.
> 
> I think the second solution could be more readable. The WARN_ON wouldn't
> be needed. However this won't solve the two dual nature of runtime
> callbacks.
> 
> I wondered also about removing runtime PM callbacks from amba/bus.c
> completely and moving this to child drivers. This way runtime PM would
> be obvious in each driver case.

That makes it pretty horrid from the point of view of having bus
management code, because we now have the management of the bus clock
split between the bus layer and the device driver.

This is /really/ a problem for runtime PM.  Runtime PM permits there
to be a bus layer involved - and runtime PM can also be coupled up
to PM domains as well.  For all this stuff, the context which the
callbacks are called in depends on whether the driver itself has
marked the device as having IRQ-safe callbacks.

That's fine, but the bus and PM domain level code then /really/ needs
to know what context they're being called in, so they know whether
they can sleep or not, or they must to be written to always use
non-sleeping functions so they work in both contexts.  If we assume
the former, then that implies that the irq-safe flag must never change
state between a suspend and a resume.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Alan Stern]

On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:

> That makes it pretty horrid from the point of view of having bus
> management code, because we now have the management of the bus clock
> split between the bus layer and the device driver.
> 
> This is /really/ a problem for runtime PM.  Runtime PM permits there
> to be a bus layer involved - and runtime PM can also be coupled up
> to PM domains as well.  For all this stuff, the context which the
> callbacks are called in depends on whether the driver itself has
> marked the device as having IRQ-safe callbacks.
> 
> That's fine, but the bus and PM domain level code then /really/ needs
> to know what context they're being called in, so they know whether
> they can sleep or not, or they must to be written to always use
> non-sleeping functions so they work in both contexts.  If we assume
> the former, then that implies that the irq-safe flag must never change
> state between a suspend and a resume.

If a bus subsystem or PM domain is going to allow its drivers to choose
between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
subsystem to come up with a way for drivers to indicate their choice.

I tend to agree with Rafael that testing dev->power.irq_safe should be 
good enough, with no real need for a wrapper.  But the subsystem can 
use a different mechanism if it wants.

Bear in mind, however, that once the irq_safe flag has been set, the 
runtime PM core offers no way to turn it off again.

Alan Stern

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Russell King - ARM Linux]

On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote:
> Bear in mind, however, that once the irq_safe flag has been set, the 
> runtime PM core offers no way to turn it off again.

Ah, I thought it did permit it to change both ways.  In that case, we
don't need to validate that it doesn't change state on each call, and
we can just get away with checking its value.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Vinod Koul]

On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote:
> > On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> > >> On pon, 2014-10-20 at 11:04 +0200, 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>
> > >> 
> > >> Rafael, Len, Pavel,
> > >> 
> > >> Is proposed API ok? Do you have any comments?
> > >> 
> > >> I'll upload whole patchset to Russell's patch tracking system. However
> > >> an ack from PM maintainer is probably needed.
> > > 
> > > I don't like the API. Having callbacks work in different context (irq
> > > / noirq) based on what another function reports is ugly.
> > > 
> > > What is the penalty if we always decide callbacks are not IRQ safe?
> > 
> > Then pm_runtime_get_sync() could not be called in atomic context. The
> > pl330 runtime PM would have to be completely reworked because one
> > pm_runtime_get_sync() is called in device_issue_pending which cannot
> > sleep (at least in non preemptible kernels). Probably this can be solved
> > some way...
> 
> Many other drivers suffer from the same problem. While I won't reject your 
> proposed fix, I would prefer a more generic approach.
> 
> One option that has been discussed previously was to use a work queue to delay 
> starting the DMA transfer to an interruptible context where 
> pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
> even that won't work in all cases as the DMA slave might need the transfer to 
> be started before enabling part of its hardware (OMAP audio seem to be such a 
> case).
> 
> I've heard a rumor of a possible DMA engine rework to forbid calling the 
> descriptor preparation API from atomic context. This could be used as a base 
> to implement runtime PM, as DMA slave drivers should not prepare descriptors 
> if they don't need to use them. However that's a long term plan, and we need a 
> solution sooner than that.

Well it is not a rumour :)

I have been contemplating that now that async_tx will be killed so we dont
have to worry about that usage. For the slave dma usage, we can change the
prepare API to be non atomic. I think the users will be okay with approach.
This way drivers can use runtime pm calls in prepare.

-- 
~Vinod

> 
> I've been toying with the idea of adding explicit open/close (or whatever we 
> would call them) operations to the DMA engine API. Those would be used by DMA 
> slave drivers to signal that they will start/stop using the DMA engine.
> 
> If (1) we must start the DMA synchronously with a DMA slave call, (2) need to 
> sleep to handle PM, and (3) don't want to keep the DMA engine powered for as 
> long as one channel is requested, then we need to turn at least preparation as 
> not callable in atomic context, or introduce a new operation.
> 
> [1] http://www.spinics.net/lists/dmaengine/msg01548.html
> 
> > >>> --- 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
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Laurent Pinchart]

Hi Vinod,

On Monday 03 November 2014 21:57:28 Vinod Koul wrote:
> On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote:
> > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote:
> >> On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> >>> On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> >>>> On pon, 2014-10-20 at 11:04 +0200, 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>
> >>>> 
> >>>> Rafael, Len, Pavel,
> >>>> 
> >>>> Is proposed API ok? Do you have any comments?
> >>>> 
> >>>> I'll upload whole patchset to Russell's patch tracking system.
> >>>> However an ack from PM maintainer is probably needed.
> >>> 
> >>> I don't like the API. Having callbacks work in different context (irq
> >>> / noirq) based on what another function reports is ugly.
> >>> 
> >>> What is the penalty if we always decide callbacks are not IRQ safe?
> >> 
> >> Then pm_runtime_get_sync() could not be called in atomic context. The
> >> pl330 runtime PM would have to be completely reworked because one
> >> pm_runtime_get_sync() is called in device_issue_pending which cannot
> >> sleep (at least in non preemptible kernels). Probably this can be solved
> >> some way...
> > 
> > Many other drivers suffer from the same problem. While I won't reject your
> > proposed fix, I would prefer a more generic approach.
> > 
> > One option that has been discussed previously was to use a work queue to
> > delay starting the DMA transfer to an interruptible context where
> > pm_runtime_get_sync() could be called. However, as Russell pointed out
> > [1],
> > even that won't work in all cases as the DMA slave might need the transfer
> > to be started before enabling part of its hardware (OMAP audio seem to be
> > such a case).
> > 
> > I've heard a rumor of a possible DMA engine rework to forbid calling the
> > descriptor preparation API from atomic context. This could be used as a
> > base to implement runtime PM, as DMA slave drivers should not prepare
> > descriptors if they don't need to use them. However that's a long term
> > plan, and we need a solution sooner than that.
> 
> Well it is not a rumour :)

I didn't want to put too much pressure on you by quoting you on that :-)

> I have been contemplating that now that async_tx will be killed so we dont
> have to worry about that usage. For the slave dma usage, we can change the
> prepare API to be non atomic. I think the users will be okay with approach.
> This way drivers can use runtime pm calls in prepare.

I'd certainly welcome that. There's a couple of users we will need to fix as 
they call the prepare API from an atomic context. I'm thinking about ASoC 
drivers in particular.

Do you have any time line in mind ?

> > I've been toying with the idea of adding explicit open/close (or whatever
> > we would call them) operations to the DMA engine API. Those would be used
> > by DMA slave drivers to signal that they will start/stop using the DMA
> > engine.
> > 
> > If (1) we must start the DMA synchronously with a DMA slave call, (2) need
> > to sleep to handle PM, and (3) don't want to keep the DMA engine powered
> > for as long as one channel is requested, then we need to turn at least
> > preparation as not callable in atomic context, or introduce a new
> > operation.
> > 
> > [1] http://www.spinics.net/lists/dmaengine/msg01548.html
> > 
> > > >>> --- 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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Russell King - ARM Linux]

On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote:
> On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote:
> > Many other drivers suffer from the same problem. While I won't reject your 
> > proposed fix, I would prefer a more generic approach.
> > 
> > One option that has been discussed previously was to use a work queue to delay 
> > starting the DMA transfer to an interruptible context where 
> > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
> > even that won't work in all cases as the DMA slave might need the transfer to 
> > be started before enabling part of its hardware (OMAP audio seem to be such a 
> > case).
> > 
> > I've heard a rumor of a possible DMA engine rework to forbid calling the 
> > descriptor preparation API from atomic context. This could be used as a base 
> > to implement runtime PM, as DMA slave drivers should not prepare descriptors 
> > if they don't need to use them. However that's a long term plan, and we need a 
> > solution sooner than that.
> 
> Well it is not a rumour :)
> 
> I have been contemplating that now that async_tx will be killed so we dont
> have to worry about that usage. For the slave dma usage, we can change the
> prepare API to be non atomic. I think the users will be okay with approach.
> This way drivers can use runtime pm calls in prepare.

Except we /do/ have a fair number of places where the prep calls are made
from atomic contexts, particularly in serial drivers.  You'd need to
introduce a tasklet into almost every serial driver which doesn't
already have one to restart RX DMA after an error or pause.  Eg,

drivers/tty/serial/amba-pl011.c
drivers/tty/serial/pch_uart.c
drivers/tty/serial/imx.c

Probably also:

drivers/net/ethernet/micrel/ks8842.c

There could well be other places as well, I've not gone through and
checked exhaustively.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Kevin Hilman]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

[...]

> Would another possible solution be to remember the irq-safeness in the
> suspend handler, and use that in the resume handler?  Resume should
> /always/ undo what the suspend handler previously did wrt clk API stuff.

This seems more reasonable to me.

Currently, the setting of irq_safe-ness is permanent (there's no exposed
API to unset it after you set it.) so assuming it to be the same across
suspend resume is a safe assumption.

Kevin

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Rafael J. Wysocki]

On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote:
> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:
> 
> > That makes it pretty horrid from the point of view of having bus
> > management code, because we now have the management of the bus clock
> > split between the bus layer and the device driver.
> > 
> > This is /really/ a problem for runtime PM.  Runtime PM permits there
> > to be a bus layer involved - and runtime PM can also be coupled up
> > to PM domains as well.  For all this stuff, the context which the
> > callbacks are called in depends on whether the driver itself has
> > marked the device as having IRQ-safe callbacks.
> > 
> > That's fine, but the bus and PM domain level code then /really/ needs
> > to know what context they're being called in, so they know whether
> > they can sleep or not, or they must to be written to always use
> > non-sleeping functions so they work in both contexts.  If we assume
> > the former, then that implies that the irq-safe flag must never change
> > state between a suspend and a resume.
> 
> If a bus subsystem or PM domain is going to allow its drivers to choose
> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
> subsystem to come up with a way for drivers to indicate their choice.
> 
> I tend to agree with Rafael that testing dev->power.irq_safe should be 
> good enough, with no real need for a wrapper.  But the subsystem can 
> use a different mechanism if it wants.
> 
> Bear in mind, however, that once the irq_safe flag has been set, the 
> runtime PM core offers no way to turn it off again.

There is a problem with it, though.  Say, a driver handles a device that
may or may not be in a power domain.  Or in other words, the power domain
the device is in may or may not be always on.  If the domain is always on,
the runtime PM callbacks are IRQ-safe (they depend on the driver only).
If it isn't, they may not be IRQ-safe.  How's the driver going to decide
whether or not to set power.irq_safe?

Rafael

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On pon, 2014-11-03 at 15:44 +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote:
> > Bear in mind, however, that once the irq_safe flag has been set, the 
> > runtime PM core offers no way to turn it off again.
> 
> Ah, I thought it did permit it to change both ways.  In that case, we
> don't need to validate that it doesn't change state on each call, and
> we can just get away with checking its value.

It cannot be unset but still it could be *set* during runtime (not only
in probe). However that shouldn't happen between suspend-resume calls,
so the solution of undoing suspend's work seems fine.

I'll send a new patch doing this way. Without the wrapper.

Best regards,
Krzysztof

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On wto, 2014-11-04 at 02:57 +0100, Rafael J. Wysocki wrote:
> On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote:
> > On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:
> > 
> > > That makes it pretty horrid from the point of view of having bus
> > > management code, because we now have the management of the bus clock
> > > split between the bus layer and the device driver.
> > > 
> > > This is /really/ a problem for runtime PM.  Runtime PM permits there
> > > to be a bus layer involved - and runtime PM can also be coupled up
> > > to PM domains as well.  For all this stuff, the context which the
> > > callbacks are called in depends on whether the driver itself has
> > > marked the device as having IRQ-safe callbacks.
> > > 
> > > That's fine, but the bus and PM domain level code then /really/ needs
> > > to know what context they're being called in, so they know whether
> > > they can sleep or not, or they must to be written to always use
> > > non-sleeping functions so they work in both contexts.  If we assume
> > > the former, then that implies that the irq-safe flag must never change
> > > state between a suspend and a resume.
> > 
> > If a bus subsystem or PM domain is going to allow its drivers to choose
> > between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
> > subsystem to come up with a way for drivers to indicate their choice.
> > 
> > I tend to agree with Rafael that testing dev->power.irq_safe should be 
> > good enough, with no real need for a wrapper.  But the subsystem can 
> > use a different mechanism if it wants.
> > 
> > Bear in mind, however, that once the irq_safe flag has been set, the 
> > runtime PM core offers no way to turn it off again.
> 
> There is a problem with it, though.  Say, a driver handles a device that
> may or may not be in a power domain.  Or in other words, the power domain
> the device is in may or may not be always on.  If the domain is always on,
> the runtime PM callbacks are IRQ-safe (they depend on the driver only).
> If it isn't, they may not be IRQ-safe.  How's the driver going to decide
> whether or not to set power.irq_safe?

The pl330 driver (being a part of amba bus) always wants IRQ safe
callbacks. However other amba drivers may not. So actually the driver
always sets one or another. The dualism is present only in the
amba/bus.c driver which encapsulates the child drivers.

Best regards,
Krzysztof

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Ulf Hansson]

On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote:
>> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:
>>
>> > That makes it pretty horrid from the point of view of having bus
>> > management code, because we now have the management of the bus clock
>> > split between the bus layer and the device driver.
>> >
>> > This is /really/ a problem for runtime PM.  Runtime PM permits there
>> > to be a bus layer involved - and runtime PM can also be coupled up
>> > to PM domains as well.  For all this stuff, the context which the
>> > callbacks are called in depends on whether the driver itself has
>> > marked the device as having IRQ-safe callbacks.
>> >
>> > That's fine, but the bus and PM domain level code then /really/ needs
>> > to know what context they're being called in, so they know whether
>> > they can sleep or not, or they must to be written to always use
>> > non-sleeping functions so they work in both contexts.  If we assume
>> > the former, then that implies that the irq-safe flag must never change
>> > state between a suspend and a resume.
>>
>> If a bus subsystem or PM domain is going to allow its drivers to choose
>> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
>> subsystem to come up with a way for drivers to indicate their choice.
>>
>> I tend to agree with Rafael that testing dev->power.irq_safe should be
>> good enough, with no real need for a wrapper.  But the subsystem can
>> use a different mechanism if it wants.
>>
>> Bear in mind, however, that once the irq_safe flag has been set, the
>> runtime PM core offers no way to turn it off again.
>
> There is a problem with it, though.  Say, a driver handles a device that
> may or may not be in a power domain.  Or in other words, the power domain
> the device is in may or may not be always on.  If the domain is always on,
> the runtime PM callbacks are IRQ-safe (they depend on the driver only).
> If it isn't, they may not be IRQ-safe.  How's the driver going to decide
> whether or not to set power.irq_safe?

>From my point of view; the decision whether the driver will set the
IRQ safe flag is in principle a software design choice.

Currently genpd isn't able to power off, if one of its devices are IRQ
safe configured. That's a limitation in genpd which we need to fix and
it's on my TODO list.

My point is thus, I don't think the driver should care about PM
domains at all regarding using the IRQ safe option. Does that make
sense?

Kind regards
Uffe

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote:
> On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote:
> >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:
> >>
> >> > That makes it pretty horrid from the point of view of having bus
> >> > management code, because we now have the management of the bus clock
> >> > split between the bus layer and the device driver.
> >> >
> >> > This is /really/ a problem for runtime PM.  Runtime PM permits there
> >> > to be a bus layer involved - and runtime PM can also be coupled up
> >> > to PM domains as well.  For all this stuff, the context which the
> >> > callbacks are called in depends on whether the driver itself has
> >> > marked the device as having IRQ-safe callbacks.
> >> >
> >> > That's fine, but the bus and PM domain level code then /really/ needs
> >> > to know what context they're being called in, so they know whether
> >> > they can sleep or not, or they must to be written to always use
> >> > non-sleeping functions so they work in both contexts.  If we assume
> >> > the former, then that implies that the irq-safe flag must never change
> >> > state between a suspend and a resume.
> >>
> >> If a bus subsystem or PM domain is going to allow its drivers to choose
> >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
> >> subsystem to come up with a way for drivers to indicate their choice.
> >>
> >> I tend to agree with Rafael that testing dev->power.irq_safe should be
> >> good enough, with no real need for a wrapper.  But the subsystem can
> >> use a different mechanism if it wants.
> >>
> >> Bear in mind, however, that once the irq_safe flag has been set, the
> >> runtime PM core offers no way to turn it off again.
> >
> > There is a problem with it, though.  Say, a driver handles a device that
> > may or may not be in a power domain.  Or in other words, the power domain
> > the device is in may or may not be always on.  If the domain is always on,
> > the runtime PM callbacks are IRQ-safe (they depend on the driver only).
> > If it isn't, they may not be IRQ-safe.  How's the driver going to decide
> > whether or not to set power.irq_safe?
> 
> From my point of view; the decision whether the driver will set the
> IRQ safe flag is in principle a software design choice.
> 
> Currently genpd isn't able to power off, if one of its devices are IRQ
> safe configured. That's a limitation in genpd which we need to fix and
> it's on my TODO list.
> 
> My point is thus, I don't think the driver should care about PM
> domains at all regarding using the IRQ safe option. Does that make
> sense?

Yes, it does, and that's the heart of the problem above.  The driver should
not care about wherther or not the device is in a power domain, but it needs
to know that when deciding whether or not to set power.irq_safe.  Catch 22.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v8 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Ulf Hansson]

On 4 November 2014 14:59, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote:
>> On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote:
>> >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote:
>> >>
>> >> > That makes it pretty horrid from the point of view of having bus
>> >> > management code, because we now have the management of the bus clock
>> >> > split between the bus layer and the device driver.
>> >> >
>> >> > This is /really/ a problem for runtime PM.  Runtime PM permits there
>> >> > to be a bus layer involved - and runtime PM can also be coupled up
>> >> > to PM domains as well.  For all this stuff, the context which the
>> >> > callbacks are called in depends on whether the driver itself has
>> >> > marked the device as having IRQ-safe callbacks.
>> >> >
>> >> > That's fine, but the bus and PM domain level code then /really/ needs
>> >> > to know what context they're being called in, so they know whether
>> >> > they can sleep or not, or they must to be written to always use
>> >> > non-sleeping functions so they work in both contexts.  If we assume
>> >> > the former, then that implies that the irq-safe flag must never change
>> >> > state between a suspend and a resume.
>> >>
>> >> If a bus subsystem or PM domain is going to allow its drivers to choose
>> >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the
>> >> subsystem to come up with a way for drivers to indicate their choice.
>> >>
>> >> I tend to agree with Rafael that testing dev->power.irq_safe should be
>> >> good enough, with no real need for a wrapper.  But the subsystem can
>> >> use a different mechanism if it wants.
>> >>
>> >> Bear in mind, however, that once the irq_safe flag has been set, the
>> >> runtime PM core offers no way to turn it off again.
>> >
>> > There is a problem with it, though.  Say, a driver handles a device that
>> > may or may not be in a power domain.  Or in other words, the power domain
>> > the device is in may or may not be always on.  If the domain is always on,
>> > the runtime PM callbacks are IRQ-safe (they depend on the driver only).
>> > If it isn't, they may not be IRQ-safe.  How's the driver going to decide
>> > whether or not to set power.irq_safe?
>>
>> From my point of view; the decision whether the driver will set the
>> IRQ safe flag is in principle a software design choice.
>>
>> Currently genpd isn't able to power off, if one of its devices are IRQ
>> safe configured. That's a limitation in genpd which we need to fix and
>> it's on my TODO list.
>>
>> My point is thus, I don't think the driver should care about PM
>> domains at all regarding using the IRQ safe option. Does that make
>> sense?
>
> Yes, it does, and that's the heart of the problem above.  The driver should
> not care about wherther or not the device is in a power domain, but it needs
> to know that when deciding whether or not to set power.irq_safe.  Catch 22.

Why is it catch22? The problem is supposed to be fixed in the generic
PM domain. How we do that is a different question.

Until genpd get fixed, the driver can still keep using irq_safe if
they want to. It will only lead to limitations if the device is
attached to a genpd.

Kind regards
Uffe

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Vinod Koul]

On Mon, Nov 03, 2014 at 05:04:08PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote:
> > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote:
> > > Many other drivers suffer from the same problem. While I won't reject your 
> > > proposed fix, I would prefer a more generic approach.
> > > 
> > > One option that has been discussed previously was to use a work queue to delay 
> > > starting the DMA transfer to an interruptible context where 
> > > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
> > > even that won't work in all cases as the DMA slave might need the transfer to 
> > > be started before enabling part of its hardware (OMAP audio seem to be such a 
> > > case).
> > > 
> > > I've heard a rumor of a possible DMA engine rework to forbid calling the 
> > > descriptor preparation API from atomic context. This could be used as a base 
> > > to implement runtime PM, as DMA slave drivers should not prepare descriptors 
> > > if they don't need to use them. However that's a long term plan, and we need a 
> > > solution sooner than that.
> > 
> > Well it is not a rumour :)
> > 
> > I have been contemplating that now that async_tx will be killed so we dont
> > have to worry about that usage. For the slave dma usage, we can change the
> > prepare API to be non atomic. I think the users will be okay with approach.
> > This way drivers can use runtime pm calls in prepare.
> 
> Except we /do/ have a fair number of places where the prep calls are made
> from atomic contexts, particularly in serial drivers.  You'd need to
> introduce a tasklet into almost every serial driver which doesn't
> already have one to restart RX DMA after an error or pause.  Eg,
> 
> drivers/tty/serial/amba-pl011.c
> drivers/tty/serial/pch_uart.c
> drivers/tty/serial/imx.c
> 
> Probably also:
> 
> drivers/net/ethernet/micrel/ks8842.c
> 
> There could well be other places as well, I've not gone through and
> checked exhaustively.
Yes that would be required to be done first. Or is there any better
alternative proposal, am all ears

-- 
~Vinod

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Vinod Koul]

On Mon, Nov 03, 2014 at 06:59:37PM +0200, Laurent Pinchart wrote:
> > > Many other drivers suffer from the same problem. While I won't reject your
> > > proposed fix, I would prefer a more generic approach.
> > > 
> > > One option that has been discussed previously was to use a work queue to
> > > delay starting the DMA transfer to an interruptible context where
> > > pm_runtime_get_sync() could be called. However, as Russell pointed out
> > > [1],
> > > even that won't work in all cases as the DMA slave might need the transfer
> > > to be started before enabling part of its hardware (OMAP audio seem to be
> > > such a case).
> > > 
> > > I've heard a rumor of a possible DMA engine rework to forbid calling the
> > > descriptor preparation API from atomic context. This could be used as a
> > > base to implement runtime PM, as DMA slave drivers should not prepare
> > > descriptors if they don't need to use them. However that's a long term
> > > plan, and we need a solution sooner than that.
> > 
> > Well it is not a rumour :)
> 
> I didn't want to put too much pressure on you by quoting you on that :-)
> 
> > I have been contemplating that now that async_tx will be killed so we dont
> > have to worry about that usage. For the slave dma usage, we can change the
> > prepare API to be non atomic. I think the users will be okay with approach.
> > This way drivers can use runtime pm calls in prepare.
> 
> I'd certainly welcome that. There's a couple of users we will need to fix as 
> they call the prepare API from an atomic context. I'm thinking about ASoC 
> drivers in particular.
ASoC is impler to fix as we can do this is ASoC prepare which is not atomic.
So we would need to split prepare and submit on that

> Do you have any time line in mind ?
Not yet, perhpaps the one after next merge window will be good target.

-- 
~Vinod

Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Laurent Pinchart]

Hi Russell,

On Monday 03 November 2014 17:04:08 Russell King - ARM Linux wrote:
> On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote:
> > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote:
> >> Many other drivers suffer from the same problem. While I won't reject
> >> your proposed fix, I would prefer a more generic approach.
> >> 
> >> One option that has been discussed previously was to use a work queue to
> >> delay starting the DMA transfer to an interruptible context where
> >> pm_runtime_get_sync() could be called. However, as Russell pointed out
> >> [1],
> >> even that won't work in all cases as the DMA slave might need the
> >> transfer to be started before enabling part of its hardware (OMAP audio
> >> seem to be such a case).
> >> 
> >> I've heard a rumor of a possible DMA engine rework to forbid calling the
> >> descriptor preparation API from atomic context. This could be used as a
> >> base to implement runtime PM, as DMA slave drivers should not prepare
> >> descriptors if they don't need to use them. However that's a long term
> >> plan, and we need a solution sooner than that.
> > 
> > Well it is not a rumour :)
> > 
> > I have been contemplating that now that async_tx will be killed so we dont
> > have to worry about that usage. For the slave dma usage, we can change the
> > prepare API to be non atomic. I think the users will be okay with
> > approach. This way drivers can use runtime pm calls in prepare.
> 
> Except we /do/ have a fair number of places where the prep calls are made
> from atomic contexts, particularly in serial drivers.  You'd need to
> introduce a tasklet into almost every serial driver which doesn't
> already have one to restart RX DMA after an error or pause.  Eg,
> 
> drivers/tty/serial/amba-pl011.c
> drivers/tty/serial/pch_uart.c
> drivers/tty/serial/imx.c

I wonder whether it would be possible to decouple descriptor allocation and 
descriptor initialization/preparation. If drivers could allocate descriptors 
and reuse them after they complete, not only would it lower the memory 
management pressure by getting rid of alloc/free during transmission, but it 
would also make it possible to easily allocate the transaction descriptors 
beforehand in non-atomic context.

> Probably also:
> 
> drivers/net/ethernet/micrel/ks8842.c
> 
> There could well be other places as well, I've not gone through and
> checked exhaustively.

-- 
Regards,

Laurent Pinchart