[PATCH v9 0/4] amba/dma: pl330: add Power Management support

[PATCH v9 0/4] amba/dma: pl330: add Power Management support

[Krzysztof Kozlowski]

Hi,


Changes since v8:
=================
1. Remove the pm_runtime_is_irq_safe() wrapper (patch 1).
2. Patch 2/4 (amba): Store current irq_safe during runtime suspend.
   Resume will mirror suspend's work. Add a macro for
   safe access of irq_safe if CONFIG_PM_RUNTIME is unset.
   Dropped reviewed-by Ulf Hansson (major changes in patch).

Changes since v7:
=================
1. Add reviewed-by Ulf Hansson (patches 3, 4 and 5).
2. Patch 2/5: Fix missing return in amba_pclk_prepare() (suggested by
   Ulf Hansson).
3. Rebased on next-20141020.

Changes since v6:
=================
1. Add patch 5 removing the amba_pclk_*able macros.
2. Patch 2/5: Remove IS_ERR, use static inline functions instead
   of macros.
3. Patch 4/5: Force runtime suspend/resume in system sleep
   callbacks. Put with autosuspend at end of probe instead of no_idle.
   Suggested by Ulf Hansson.

Changes since v5:
=================
1. Patch 1/4: Add Ulf Hansson's reviewed-by.
2. Patch 4/4: Use PM runtime autosuspend (suggested by Ulf Hansson).
3. Rebase on next-20140922. 

Changes since v4:
1. Patch 3/4: Explicitly initialize amba_device.irq_safe field after
   probing driver (suggested by Russell King).

Changes since v3:
1. Patch 1/4: Document new API in Documentation/power/runtime_pm.txt
   (pointed by Alan Stern).

Changes since v2:
1. Add patch 1 (PM / Runtime: Add getter for querying the IRQ safe option)
2. Add patch 2 (amba: Add helper macros for (un)preparing AMBA clock)
3. Patch 3/4: Rewrite the idea. If IRQ safe runtime PM is set then
   do not unprepare/prepare the clocks. Suggested by Russell King.
4. Patch 4/4: During system sleep unprepare the clock.

Changes since v1:
1. Add patch 1 (amba: Allow AMBA drivers to use their own runtime PM).
2. Patch 2/2: Apply Michal Simek's suggestions.
3. Patch 2/2: Fix atomic context safeness in pl330_issue_pending().


Description:
============
This patchset adds runtime and system PM to the pl330 driver.

The runtime PM of pl330 driver requires interrupt safe suspend/resume
callbacks which is in conflict with current amba bus driver.
The latter also unprepares and prepares the AMBA bus clock which
is not safe for atomic context.

The patchset solves this in patch 2/4 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)


Best regards,
Krzysztof Kozlowski


Krzysztof Kozlowski (4):
  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

 drivers/amba/bus.c       | 15 ++++++--
 drivers/dma/pl330.c      | 94 +++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/amba/bus.h | 13 ++++---
 3 files changed, 111 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v9 1/4] 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>
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

[PATCH v9 2/4] 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 during runtime suspend. During runtime
resume deal with clocks according to stored value.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/amba/bus.c       | 24 ++++++++++++++++++++----
 include/linux/amba/bus.h |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1b5be3..27ec8882ec8e 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -85,6 +85,13 @@ static struct device_attribute amba_dev_attrs[] = {
 };
 
 #ifdef CONFIG_PM
+
+#ifdef CONFIG_PM_RUNTIME
+#define get_pm_runtime_irq_safe(dev)	((dev)->power.irq_safe)
+#else
+#define get_pm_runtime_irq_safe(dev)	1
+#endif
+
 /*
  * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
  * enable/disable the bus clock at runtime PM suspend/resume as this
@@ -95,8 +102,14 @@ 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) {
+		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
+
+		if (pcdev->irq_safe)
+			clk_disable(pcdev->pclk);
+		else
+			clk_disable_unprepare(pcdev->pclk);
+	}
 
 	return ret;
 }
@@ -107,7 +120,10 @@ static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
-		ret = clk_prepare_enable(pcdev->pclk);
+		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;
@@ -115,7 +131,7 @@ static int amba_pm_runtime_resume(struct device *dev)
 
 	return pm_generic_runtime_resume(dev);
 }
-#endif
+#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops amba_pm = {
 	.suspend	= pm_generic_suspend,
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 v9 3/4] 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 v9 4/4] 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 v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Ulf Hansson]

On 4 November 2014 13:52, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
>
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
>
> Detect the device driver behavior during runtime suspend. During runtime
> resume deal with clocks according to stored value.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/amba/bus.c       | 24 ++++++++++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..27ec8882ec8e 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -85,6 +85,13 @@ static struct device_attribute amba_dev_attrs[] = {
>  };
>
>  #ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_RUNTIME
> +#define get_pm_runtime_irq_safe(dev)   ((dev)->power.irq_safe)
> +#else
> +#define get_pm_runtime_irq_safe(dev)   1

No, this doesn't work for those drivers that isn't configured as
irq_safe. The pm_runtime_force_suspend() invoked from the driver's
system PM suspend callback, will expect clocks to be unprepared when
CONFIG_PM_RUNTIME is unset.

To address this issue, in principle we need to know whether the driver
has invoked pm_runtime_irq_safe(), even when CONFIG_PM_RUNTIME is
unset.

This may be solved by the help of PM core:
1) Move the irq_safe member in the struct dev_pm_info, outside the
#ifdef CONFIG_PM_RUNTIME.
2) Let the pm_runtime_irq_safe() function be implemented for CONFIG_PM
instead of CONFIG_PM_RUNTIME.

If the PM core maintainers don't like that approach we can always add
an amba specific wrapper function for pm_runtime_irq_safe() and store
the information needed in the struct amba_device.

Kind regards
Uffe

> +#endif
> +
>  /*
>   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
>   * enable/disable the bus clock at runtime PM suspend/resume as this
> @@ -95,8 +102,14 @@ 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) {
> +               pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> +
> +               if (pcdev->irq_safe)
> +                       clk_disable(pcdev->pclk);
> +               else
> +                       clk_disable_unprepare(pcdev->pclk);
> +       }
>
>         return ret;
>  }
> @@ -107,7 +120,10 @@ static int amba_pm_runtime_resume(struct device *dev)
>         int ret;
>
>         if (dev->driver) {
> -               ret = clk_prepare_enable(pcdev->pclk);
> +               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;
> @@ -115,7 +131,7 @@ static int amba_pm_runtime_resume(struct device *dev)
>
>         return pm_generic_runtime_resume(dev);
>  }
> -#endif
> +#endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops amba_pm = {
>         .suspend        = pm_generic_suspend,
> 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
>

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Pavel Machek]

On Tue 2014-11-04 13:52:48, 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.



>  /*
>   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
>   * enable/disable the bus clock at runtime PM suspend/resume as this
> @@ -95,8 +102,14 @@ 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) {
> +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> +
> +		if (pcdev->irq_safe)
> +			clk_disable(pcdev->pclk);
> +		else
> +			clk_disable_unprepare(pcdev->pclk);
> +	}

So you can handle the case of !pcdev->irq_safe. What is the penalty
for always assuming !pcdev->irq_safe?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> On Tue 2014-11-04 13:52:48, 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.
> 
> 
> 
> >  /*
> >   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> >   * enable/disable the bus clock at runtime PM suspend/resume as this
> > @@ -95,8 +102,14 @@ 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) {
> > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > +
> > +		if (pcdev->irq_safe)
> > +			clk_disable(pcdev->pclk);
> > +		else
> > +			clk_disable_unprepare(pcdev->pclk);
> > +	}
> 
> So you can handle the case of !pcdev->irq_safe. What is the penalty
> for always assuming !pcdev->irq_safe?

The penalty (for pl330 driver) would be that the runtime resume/suspend
cannot happen from atomic context
  => pm_runtime_get_sync() cannot be called from atomic context
    => complete rework of runtime PM for pl330 DMA driver because now
       one of pm_runtime_get_sync() calls is in device_issue_pending
       callback which may not sleep. And by "rework" I also mean that
       I do not know how to do this... yet.

Best regards,
Krzysztof

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On wto, 2014-11-04 at 20:06 +0100, Ulf Hansson wrote:
> On 4 November 2014 13:52, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > The AMBA bus driver defines runtime Power Management functions which
> > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > because unpreparing a clock might sleep so it is not interrupt safe.
> >
> > However some drivers may want to implement runtime PM functions in
> > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > bus driver should only disable/enable the clock in runtime suspend and
> > resume callbacks.
> >
> > Detect the device driver behavior during runtime suspend. During runtime
> > resume deal with clocks according to stored value.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/amba/bus.c       | 24 ++++++++++++++++++++----
> >  include/linux/amba/bus.h |  1 +
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 47bbdc1b5be3..27ec8882ec8e 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -85,6 +85,13 @@ static struct device_attribute amba_dev_attrs[] = {
> >  };
> >
> >  #ifdef CONFIG_PM
> > +
> > +#ifdef CONFIG_PM_RUNTIME
> > +#define get_pm_runtime_irq_safe(dev)   ((dev)->power.irq_safe)
> > +#else
> > +#define get_pm_runtime_irq_safe(dev)   1
> 
> No, this doesn't work for those drivers that isn't configured as
> irq_safe. The pm_runtime_force_suspend() invoked from the driver's
> system PM suspend callback, will expect clocks to be unprepared when
> CONFIG_PM_RUNTIME is unset.

D'oh! You're right... 

> To address this issue, in principle we need to know whether the driver
> has invoked pm_runtime_irq_safe(), even when CONFIG_PM_RUNTIME is
> unset.
> 
> This may be solved by the help of PM core:
> 1) Move the irq_safe member in the struct dev_pm_info, outside the
> #ifdef CONFIG_PM_RUNTIME.
> 2) Let the pm_runtime_irq_safe() function be implemented for CONFIG_PM
> instead of CONFIG_PM_RUNTIME.

Before I'll start implementing this - could PM maintainers share their
opinion on such change?

Best regards,
Krzysztof


> If the PM core maintainers don't like that approach we can always add
> an amba specific wrapper function for pm_runtime_irq_safe() and store
> the information needed in the struct amba_device.
> 
> Kind regards
> Uffe

Re: [PATCH v9 3/4] dma: pl330: add Power Management support

[Vinod Koul]

On Tue, Nov 04, 2014 at 01:52:49PM +0100, Krzysztof Kozlowski wrote:
>  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);
Why do we need _get() here? If we are terminating then channel is already in
use so should be active?

>  		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);
Well else should be error here, so I would expect loud complains in that
case here
> +
>  		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);
Again why do we need _get() in free callback.

>  	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 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);
IIUC this sets .suspend and .resume, aren't you trying to add runtime
support as well?
Did you want UNIVERSAL_DEV_PM_OPS() ?

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

Last please use the right subsystem name, dmaengine is patches.

-- 
~Vinod

Re: [PATCH v9 3/4] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

On śro, 2014-11-05 at 19:31 +0530, Vinod Koul wrote:
> On Tue, Nov 04, 2014 at 01:52:49PM +0100, Krzysztof Kozlowski wrote:
> >  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);
> Why do we need _get() here? If we are terminating then channel is already in
> use so should be active?

The runtime PM is kind a aggressive here so I think the device could be
suspended in that moment. The terminate may happen some time after
channel was used.

> 
> >  		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);
> Well else should be error here, so I would expect loud complains in that
> case here

The 'else' path is fine. Just the terminate command was triggered when
work_list was empty. 

The 'else' here could be triggered easily with:

echo dma0chan0 > /sys/module/dmatest/parameters/channel
echo 1 > /sys/module/dmatest/parameters/run
sleep 3
cat /sys/module/dmatest/parameters/run


> > +
> >  		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);
> Again why do we need _get() in free callback.

Hmmm... I think because of the same reason as terminate_all. It may
happen after some time since last usage. The driver here stops thread
which requires device resumed.

> 
> >  	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 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);
> IIUC this sets .suspend and .resume, aren't you trying to add runtime
> support as well?
> Did you want UNIVERSAL_DEV_PM_OPS() ?

The runtime suspend and resume callbacks are provided by amba/bus.c.

> 
> > +
> >  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
> > 
> 
> Last please use the right subsystem name, dmaengine is patches.

Sure, I just got confused by name of directory.

Thanks for looking at code,
Krzysztof

Re: [PATCH v9 3/4] dma: pl330: add Power Management support

[Vinod Koul]

On Wed, Nov 05, 2014 at 03:21:39PM +0100, Krzysztof Kozlowski wrote:
> > > +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);
> > IIUC this sets .suspend and .resume, aren't you trying to add runtime
> > support as well?
> > Did you want UNIVERSAL_DEV_PM_OPS() ?
> 
> The runtime suspend and resume callbacks are provided by amba/bus.c.
Can you add that as comment in this driver please.

rest looks fine

-- 
~Vinod

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Pavel Machek]

On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > On Tue 2014-11-04 13:52:48, 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.
> > 
> > 
> > 
> > >  /*
> > >   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> > >   * enable/disable the bus clock at runtime PM suspend/resume as this
> > > @@ -95,8 +102,14 @@ 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) {
> > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > +
> > > +		if (pcdev->irq_safe)
> > > +			clk_disable(pcdev->pclk);
> > > +		else
> > > +			clk_disable_unprepare(pcdev->pclk);
> > > +	}
> > 
> > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > for always assuming !pcdev->irq_safe?
> 
> The penalty (for pl330 driver) would be that the runtime resume/suspend
> cannot happen from atomic context
>   => pm_runtime_get_sync() cannot be called from atomic context
>     => complete rework of runtime PM for pl330 DMA driver because now
>        one of pm_runtime_get_sync() calls is in device_issue_pending
>        callback which may not sleep. And by "rework" I also mean that
>        I do not know how to do this... yet.

I still don't get it. You say that you don't know how to handle
!pcdev->irq_safe case... Yet have code above that tries to handle it.

If that case can't be sanely handled, I'd expect
BUG_ON(!pcdev->irq_safe).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On pią, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > On Tue 2014-11-04 13:52:48, 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.
> > > 
> > > 
> > > 
> > > >  /*
> > > >   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> > > >   * enable/disable the bus clock at runtime PM suspend/resume as this
> > > > @@ -95,8 +102,14 @@ 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) {
> > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > +
> > > > +		if (pcdev->irq_safe)
> > > > +			clk_disable(pcdev->pclk);
> > > > +		else
> > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > +	}
> > > 
> > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > for always assuming !pcdev->irq_safe?
> > 
> > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > cannot happen from atomic context
> >   => pm_runtime_get_sync() cannot be called from atomic context
> >     => complete rework of runtime PM for pl330 DMA driver because now
> >        one of pm_runtime_get_sync() calls is in device_issue_pending
> >        callback which may not sleep. And by "rework" I also mean that
> >        I do not know how to do this... yet.
> 
> I still don't get it. You say that you don't know how to handle
> !pcdev->irq_safe case... Yet have code above that tries to handle it.
> 
> If that case can't be sanely handled, I'd expect
> BUG_ON(!pcdev->irq_safe).

Hmmm... I could misunderstand your question. The amba/bus.c driver can
handle both cases. However this varies for child drivers (which use
these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
Other drivers can.

Is it the answer for your question?

Best regards,
Krzysztof

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Pavel Machek]

On Fri 2014-11-07 13:18:03, Krzysztof Kozlowski wrote:
> On pią, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> > On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > > On Tue 2014-11-04 13:52:48, 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.
> > > > 
> > > > 
> > > > 
> > > > >  /*
> > > > >   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> > > > >   * enable/disable the bus clock at runtime PM suspend/resume as this
> > > > > @@ -95,8 +102,14 @@ 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) {
> > > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > > +
> > > > > +		if (pcdev->irq_safe)
> > > > > +			clk_disable(pcdev->pclk);
> > > > > +		else
> > > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > > +	}
> > > > 
> > > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > > for always assuming !pcdev->irq_safe?
> > > 
> > > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > > cannot happen from atomic context
> > >   => pm_runtime_get_sync() cannot be called from atomic context
> > >     => complete rework of runtime PM for pl330 DMA driver because now
> > >        one of pm_runtime_get_sync() calls is in device_issue_pending
> > >        callback which may not sleep. And by "rework" I also mean that
> > >        I do not know how to do this... yet.
> > 
> > I still don't get it. You say that you don't know how to handle
> > !pcdev->irq_safe case... Yet have code above that tries to handle it.
> > 
> > If that case can't be sanely handled, I'd expect
> > BUG_ON(!pcdev->irq_safe).
> 
> Hmmm... I could misunderstand your question. The amba/bus.c driver can
> handle both cases. However this varies for child drivers (which use
> these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
> Other drivers can.

Ok, so pl330 can't handle non-irq-safe callbacks. What about the other
solution preserving consistency -- can we make sure all callbacks are
irq-safe with acceptable penalty?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v9 2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On pią, 2014-11-07 at 13:28 +0100, Pavel Machek wrote:
> On Fri 2014-11-07 13:18:03, Krzysztof Kozlowski wrote:
> > On pią, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> > > On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > > > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > > > On Tue 2014-11-04 13:52:48, 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.
> > > > > 
> > > > > 
> > > > > 
> > > > > >  /*
> > > > > >   * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
> > > > > >   * enable/disable the bus clock at runtime PM suspend/resume as this
> > > > > > @@ -95,8 +102,14 @@ 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) {
> > > > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > > > +
> > > > > > +		if (pcdev->irq_safe)
> > > > > > +			clk_disable(pcdev->pclk);
> > > > > > +		else
> > > > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > > > +	}
> > > > > 
> > > > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > > > for always assuming !pcdev->irq_safe?
> > > > 
> > > > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > > > cannot happen from atomic context
> > > >   => pm_runtime_get_sync() cannot be called from atomic context
> > > >     => complete rework of runtime PM for pl330 DMA driver because now
> > > >        one of pm_runtime_get_sync() calls is in device_issue_pending
> > > >        callback which may not sleep. And by "rework" I also mean that
> > > >        I do not know how to do this... yet.
> > > 
> > > I still don't get it. You say that you don't know how to handle
> > > !pcdev->irq_safe case... Yet have code above that tries to handle it.
> > > 
> > > If that case can't be sanely handled, I'd expect
> > > BUG_ON(!pcdev->irq_safe).
> > 
> > Hmmm... I could misunderstand your question. The amba/bus.c driver can
> > handle both cases. However this varies for child drivers (which use
> > these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
> > Other drivers can.
> 
> Ok, so pl330 can't handle non-irq-safe callbacks. What about the other
> solution preserving consistency -- can we make sure all callbacks are
> irq-safe with acceptable penalty?

Aaahhh, I get it. We could revert 5303c0f46c87 ("ARM: 7916/1: amba: Add
clk_prepare|unprepare in runtime PM callbacks"). After quick grep
through amba drivers it seems that this should work and the only impact
would be that less energy could be saved on other drivers (clock only
disabled, not unprepared).

However someone more experienced in amba drivers should confirm this.

Best regards,
Krzysztof