[PATCH 0/3] Exynos IOMMU: proper runtime pm support

[PATCH 0/3] Exynos IOMMU: proper runtime pm support

[Marek Szyprowski]

Hello,

This patch series finally implements proper runtime pm support in Exynos
IOMMU driver. This has been achieved by adding runtime pm notifiers,
which lets SYSMMU controller to follow it's master device (the device
which actually performs DMA transaction) runtime pm. The main idea
behind this solution is an observation that any DMA activity from master
device can be done only when master device is active, thus when master
device is suspended SYSMMU controller device can also be suspended.

This patchset finally solves the situation that power domains are always
enabled, because all SYSMMU controllers (which belongs to those domains)
are permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

This patchset requires my previous changes to Exynos IOMMU driver
submitted in the "Exynos IOMMU: improve clock management" thread:
http://www.spinics.net/lists/arm-kernel/msg505695.html  

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Krzysztof Kozlowski (1):
  PM / Runtime: Add notifiers for device runtime PM events

Marek Szyprowski (2):
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Add proper runtime pm support

 drivers/base/power/generic_ops.c |   9 ++
 drivers/base/power/power.h       |   6 ++
 drivers/base/power/runtime.c     |  34 ++++++-
 drivers/iommu/exynos-iommu.c     | 214 ++++++++++++++++++---------------------
 include/linux/pm.h               |   2 +
 include/linux/pm_runtime.h       |  51 ++++++++++
 6 files changed, 201 insertions(+), 115 deletions(-)

-- 
1.9.2

[PATCH 1/3] PM / Runtime: Add notifiers for device runtime PM events

[Marek Szyprowski]

From: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Allow drivers registering for certain runtime PM events of other
devices. Some drivers in power domain are more or less coupled. When one
driver is suspending (thus leading to power domain being also turned
off) the other might have to perform some necessary steps. For example
Exynos IOMMU has to save its context.

Based on previous work of Sylwester Nawrocki <s.nawrocki@samsung.com>.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/base/power/generic_ops.c |  9 +++++++
 drivers/base/power/power.h       |  6 +++++
 drivers/base/power/runtime.c     | 34 +++++++++++++++++++++++++--
 include/linux/pm.h               |  2 ++
 include/linux/pm_runtime.h       | 51 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 07c3c4a9522d..f0838229b781 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -10,6 +10,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/export.h>
 #include <linux/suspend.h>
+#include "power.h"
 
 #ifdef CONFIG_PM
 /**
@@ -25,8 +26,12 @@ int pm_generic_runtime_suspend(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int ret;
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_PRE);
+
 	ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_POST);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
@@ -44,8 +49,12 @@ int pm_generic_runtime_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	int ret;
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_PRE);
+
 	ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_POST);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 50e30e7b059d..30b6319ce96c 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -1,4 +1,5 @@
 #include <linux/pm_qos.h>
+#include <linux/pm_runtime.h>
 
 static inline void device_pm_init_common(struct device *dev)
 {
@@ -20,6 +21,7 @@ static inline void pm_runtime_early_init(struct device *dev)
 extern void pm_runtime_init(struct device *dev);
 extern void pm_runtime_reinit(struct device *dev);
 extern void pm_runtime_remove(struct device *dev);
+extern int pm_runtime_notifier_call(struct device *dev, enum rpm_event event);
 
 struct wake_irq {
 	struct device *dev;
@@ -87,6 +89,10 @@ static inline void pm_runtime_early_init(struct device *dev)
 static inline void pm_runtime_init(struct device *dev) {}
 static inline void pm_runtime_reinit(struct device *dev) {}
 static inline void pm_runtime_remove(struct device *dev) {}
+static inline pm_runtime_notifier_call(struct device *dev, enum rpm_event event)
+{
+	return 0;
+}
 
 static inline int dpm_sysfs_add(struct device *dev) { return 0; }
 static inline void dpm_sysfs_remove(struct device *dev) {}
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b74690418504..3a5637ca8400 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -227,6 +227,27 @@ void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_set_memalloc_noio);
 
+int pm_runtime_notifier_call(struct device *dev, enum rpm_event event)
+{
+	return atomic_notifier_call_chain(&dev->power.runtime_notifier,
+					  event, dev);
+}
+
+int pm_runtime_register_notifier(struct device *dev, struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&dev->power.runtime_notifier,
+					      nb);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_register_notifier);
+
+int pm_runtime_unregister_notifier(struct device *dev,
+				    struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&dev->power.runtime_notifier,
+						nb);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_unregister_notifier);
+
 /**
  * rpm_check_suspend_allowed - Test whether a device may be suspended.
  * @dev: Device to test.
@@ -1174,6 +1195,7 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
 		goto out;
 	}
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_DISABLE_PRE);
 	/*
 	 * Wake up the device if there's a resume request pending, because that
 	 * means there probably is some I/O to process and disabling runtime PM
@@ -1195,6 +1217,7 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
 	if (!dev->power.disable_depth++)
 		__pm_runtime_barrier(dev);
 
+	pm_runtime_notifier_call(dev, RPM_EVENT_DISABLE_POST);
  out:
 	spin_unlock_irq(&dev->power.lock);
 }
@@ -1210,10 +1233,16 @@ void pm_runtime_enable(struct device *dev)
 
 	spin_lock_irqsave(&dev->power.lock, flags);
 
-	if (dev->power.disable_depth > 0)
+	if (dev->power.disable_depth > 0) {
+		if (dev->power.disable_depth == 1)
+			pm_runtime_notifier_call(dev, RPM_EVENT_ENABLE_PRE);
 		dev->power.disable_depth--;
-	else
+	} else {
 		dev_warn(dev, "Unbalanced %s!\n", __func__);
+	}
+
+	if (dev->power.disable_depth == 0)
+		pm_runtime_notifier_call(dev, RPM_EVENT_ENABLE_POST);
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }
@@ -1411,6 +1440,7 @@ void pm_runtime_init(struct device *dev)
 			(unsigned long)dev);
 
 	init_waitqueue_head(&dev->power.wait_queue);
+	ATOMIC_INIT_NOTIFIER_HEAD(&dev->power.runtime_notifier);
 }
 
 /**
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 06eb353182ab..bdf6eaaf62ec 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -27,6 +27,7 @@
 #include <linux/wait.h>
 #include <linux/timer.h>
 #include <linux/completion.h>
+#include <linux/notifier.h>
 
 /*
  * Callbacks for platform drivers to implement.
@@ -604,6 +605,7 @@ struct dev_pm_info {
 	unsigned long		active_jiffies;
 	unsigned long		suspended_jiffies;
 	unsigned long		accounting_timestamp;
+	struct atomic_notifier_head	runtime_notifier;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 2e14d2667b6c..82d41a6a6f5c 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -23,6 +23,43 @@
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
 
+/**
+ * Runtime PM notifier events.
+ *
+ * TODO: RPM_EVENT_SUSPEND/RPM_EVENT_RESUME are supported
+ * currently only with generic power domains. They won't be executed
+ * in other cases. Fix this.
+ *
+ * RPM_EVENT_SUSPEND_PRE	Before calling device suspend callback
+ *				and before turning domain off
+ * RPM_EVENT_SUSPEND_POST	After suspending device and before turning
+ *				domain off
+ *
+ * RPM_EVENT_RESUME_PRE		Before calling device resume callback
+ *				but after turning domain on
+ * RPM_EVENT_RESUME_POST	After resuming device
+ *
+ * RPM_EVENT_ENABLE_PRE		Before effectively enabling runtime PM
+ *				(the pm_runtime_enable() call must undo the
+ *				last pm_runtime_disable() call)
+ * RPM_EVENT_ENABLE_POST	After effectively enabling runtime PM
+ *
+ * RPM_EVENT_DISABLE_PRE	Before effectively disabling runtime PM
+ *				(the first pm_runtime_disable() call)
+ * RPM_EVENT_DISABLE_POST	After effectively disabling runtime PM
+ */
+enum rpm_event {
+	RPM_EVENT_SUSPEND_PRE,
+	RPM_EVENT_SUSPEND_POST,
+	RPM_EVENT_RESUME_PRE,
+	RPM_EVENT_RESUME_POST,
+	RPM_EVENT_ENABLE_PRE,
+	RPM_EVENT_ENABLE_POST,
+	RPM_EVENT_DISABLE_PRE,
+	RPM_EVENT_DISABLE_POST,
+	RPM_EVENT_NUM,
+};
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 
@@ -55,6 +92,10 @@ extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
 extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
+extern int pm_runtime_register_notifier(struct device *dev,
+					struct notifier_block *nb);
+extern int pm_runtime_unregister_notifier(struct device *dev,
+					   struct notifier_block *nb);
 
 static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 {
@@ -186,6 +227,16 @@ static inline unsigned long pm_runtime_autosuspend_expiration(
 				struct device *dev) { return 0; }
 static inline void pm_runtime_set_memalloc_noio(struct device *dev,
 						bool enable){}
+static inline int pm_runtime_register_notifier(struct device *dev,
+					struct notifier_block *nb)
+{
+	return -ENOSYS;
+}
+static innline int pm_runtime_unregister_notifier(struct device *dev,
+					   struct notifier_block *nb)
+{
+	return 0;
+};
 
 #endif /* !CONFIG_PM */
 
-- 
1.9.2

[PATCH 2/3] iommu/exynos: Remove excessive, useless debug

[Marek Szyprowski]

Remove excessive, useless debug about skipping TLB invalidation, which
is a normal situation when more aggressive power management is enabled.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 633e6d023c0d..9d1a14f88891 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -574,9 +574,6 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 			sysmmu_unblock(data);
 		}
 		clk_disable(data->clk_master);
-	} else {
-		dev_dbg(data->master,
-			"disabled. Skipping TLB invalidation @ %#x\n", iova);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
 }
-- 
1.9.2

[PATCH 3/3] iommu/exynos: Add proper runtime pm support

[Marek Szyprowski]

This patch uses recently introduced runtime pm notifiers to track the
runtime pm state of the master's device. This way each SYSMMU controller
knows when its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 211 +++++++++++++++++++++----------------------
 1 file changed, 101 insertions(+), 110 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 9d1a14f88891..de4126787c41 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/dma-iommu.h>
+#include <linux/pm_domain.h>
 
 typedef u32 sysmmu_iova_t;
 typedef u32 sysmmu_pte_t;
@@ -233,13 +234,14 @@ struct sysmmu_drvdata {
 	struct clk *aclk;		/* SYSMMU's aclk clock */
 	struct clk *pclk;		/* SYSMMU's pclk clock */
 	struct clk *clk_master;		/* master's device clock */
-	int activations;		/* number of calls to sysmmu_enable */
 	spinlock_t lock;		/* lock for modyfying state */
+	int active;			/* current status */
 	struct exynos_iommu_domain *domain; /* domain we belong to */
 	struct list_head domain_node;	/* node for domain clients list */
 	struct list_head owner_node;	/* node for owner controllers list */
 	phys_addr_t pgtable;		/* assigned page table structure */
 	unsigned int version;		/* our version */
+	struct notifier_block pm_nb;	/* for tracking master's runtime pm */
 };
 
 static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -247,25 +249,6 @@ static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
 	return container_of(dom, struct exynos_iommu_domain, domain);
 }
 
-static bool set_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU was not active previously
-	   and it needs to be initialized */
-	return ++data->activations == 1;
-}
-
-static bool set_sysmmu_inactive(struct sysmmu_drvdata *data)
-{
-	/* return true if the System MMU is needed to be disabled */
-	BUG_ON(data->activations < 1);
-	return --data->activations == 0;
-}
-
-static bool is_sysmmu_active(struct sysmmu_drvdata *data)
-{
-	return data->activations > 0;
-}
-
 static void sysmmu_unblock(struct sysmmu_drvdata *data)
 {
 	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
@@ -384,7 +367,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	unsigned short reg_status, reg_clear;
 	int ret = -ENOSYS;
 
-	WARN_ON(!is_sysmmu_active(data));
+	WARN_ON(!data->active);
 
 	if (MMU_MAJ_VER(data->version) < 5) {
 		reg_status = REG_INT_STATUS;
@@ -440,32 +423,6 @@ static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
 	__sysmmu_disable_clocks(data);
 }
 
-static bool __sysmmu_disable(struct sysmmu_drvdata *data)
-{
-	bool disabled;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-
-	disabled = set_sysmmu_inactive(data);
-
-	if (disabled) {
-		data->pgtable = 0;
-		data->domain = NULL;
-
-		__sysmmu_disable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Disabled\n");
-	} else  {
-		dev_dbg(data->sysmmu, "%d times left to disable\n",
-					data->activations);
-	}
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return disabled;
-}
-
 static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 {
 	unsigned int cfg;
@@ -501,34 +458,6 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
 	clk_disable(data->clk_master);
 }
 
-static int __sysmmu_enable(struct sysmmu_drvdata *data, phys_addr_t pgtable,
-			   struct exynos_iommu_domain *domain)
-{
-	int ret = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&data->lock, flags);
-	if (set_sysmmu_active(data)) {
-		data->pgtable = pgtable;
-		data->domain = domain;
-
-		__sysmmu_enable_nocount(data);
-
-		dev_dbg(data->sysmmu, "Enabled\n");
-	} else {
-		ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
-
-		dev_dbg(data->sysmmu, "already enabled\n");
-	}
-
-	if (WARN_ON(ret < 0))
-		set_sysmmu_inactive(data); /* decrement count */
-
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return ret;
-}
-
 static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 					    sysmmu_iova_t iova)
 {
@@ -536,7 +465,7 @@ static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 
 
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data) && data->version >= MAKE_MMU_VER(3, 3)) {
+	if (data->active && data->version >= MAKE_MMU_VER(3, 3)) {
 		clk_enable(data->clk_master);
 		__sysmmu_tlb_invalidate_entry(data, iova, 1);
 		clk_disable(data->clk_master);
@@ -551,7 +480,7 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 	unsigned long flags;
 
 	spin_lock_irqsave(&data->lock, flags);
-	if (is_sysmmu_active(data)) {
+	if (data->active) {
 		unsigned int num_inv = 1;
 
 		clk_enable(data->clk_master);
@@ -580,6 +509,60 @@ static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
 
 static struct iommu_ops exynos_iommu_ops;
 
+static void sysmmu_restore_state(struct sysmmu_drvdata *data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->master) {
+		pm_runtime_disable(data->sysmmu);
+		pm_runtime_set_active(data->sysmmu);
+		pm_runtime_enable(data->sysmmu);
+		__sysmmu_enable_nocount(data);
+		data->active = true;
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void sysmmu_save_state(struct sysmmu_drvdata *data)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+	if (data->master) {
+		__sysmmu_disable_nocount(data);
+		pm_runtime_disable(data->sysmmu);
+		pm_runtime_set_suspended(data->sysmmu);
+		pm_runtime_enable(data->sysmmu);
+		data->active = false;
+	}
+	spin_unlock_irqrestore(&data->lock, flags);
+
+}
+
+static int sysmmu_runtime_genpd_event(struct notifier_block *this,
+				unsigned long event, void *ptr)
+{
+	struct sysmmu_drvdata *data = container_of(this, struct sysmmu_drvdata,
+						   pm_nb);
+
+	switch (event) {
+
+	case RPM_EVENT_RESUME_PRE:
+		dev_dbg(data->sysmmu, "restoring state on %s device pm request\n",
+			dev_name(data->master));
+		sysmmu_restore_state(data);
+		break;
+	case RPM_EVENT_SUSPEND_POST:
+		dev_dbg(data->sysmmu, "saving state on %s device pm request\n",
+			dev_name(data->master));
+		sysmmu_save_state(data);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -639,6 +622,7 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 		return PTR_ERR(data->clk_master);
 
 	data->sysmmu = dev;
+	data->pm_nb.notifier_call = sysmmu_runtime_genpd_event;
 	spin_lock_init(&data->lock);
 
 	platform_set_drvdata(pdev, data);
@@ -651,6 +635,7 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 			PG_ENT_SHIFT = SYSMMU_V5_PG_ENT_SHIFT;
 	}
 
+	pm_runtime_no_callbacks(dev);
 	pm_runtime_enable(dev);
 
 	of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);
@@ -664,10 +649,9 @@ static int exynos_sysmmu_suspend(struct device *dev)
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 
 	dev_dbg(dev, "suspend\n");
-	if (is_sysmmu_active(data)) {
+	if (data->active)
 		__sysmmu_disable_nocount(data);
-		pm_runtime_put(dev);
-	}
+
 	return 0;
 }
 
@@ -676,10 +660,9 @@ static int exynos_sysmmu_resume(struct device *dev)
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 
 	dev_dbg(dev, "resume\n");
-	if (is_sysmmu_active(data)) {
-		pm_runtime_get_sync(dev);
+	if (data->active)
 		__sysmmu_enable_nocount(data);
-	}
+
 	return 0;
 }
 #endif
@@ -789,8 +772,11 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 	spin_lock_irqsave(&domain->lock, flags);
 
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		if (__sysmmu_disable(data))
-			data->master = NULL;
+		if (data->active)
+			sysmmu_save_state(data);
+		data->master = NULL;
+		data->pgtable = 0;
+		data->domain = NULL;
 		list_del_init(&data->domain_node);
 	}
 
@@ -830,20 +816,27 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
 		return;
 
+	list_for_each_entry(data, &owner->controllers, owner_node) {
+		pm_runtime_unregister_notifier(dev, &data->pm_nb);
+		if (pm_runtime_enabled(dev) && pm_runtime_active(dev))
+			sysmmu_save_state(data);
+	}
+
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
+		spin_lock(&data->lock);
 		if (data->master == dev) {
-			if (__sysmmu_disable(data)) {
-				data->master = NULL;
-				list_del_init(&data->domain_node);
-			}
-			pm_runtime_put(data->sysmmu);
 			found = true;
+			data->master = NULL;
+			data->pgtable = 0;
+			data->domain = NULL;
+			list_del_init(&data->domain_node);
 		}
+		spin_unlock(&data->lock);
 	}
+	owner->domain = NULL;
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-	owner->domain = NULL;
 
 	if (found)
 		dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n",
@@ -860,7 +853,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	struct sysmmu_drvdata *data;
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	unsigned long flags;
-	int ret = -ENODEV;
 
 	if (!has_sysmmu(dev))
 		return -ENODEV;
@@ -868,29 +860,28 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	if (owner->domain)
 		exynos_iommu_detach_device(owner->domain, dev);
 
+	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(data, &owner->controllers, owner_node) {
-		pm_runtime_get_sync(data->sysmmu);
-		ret = __sysmmu_enable(data, pagetable, domain);
-		if (ret >= 0) {
-			data->master = dev;
-
-			spin_lock_irqsave(&domain->lock, flags);
-			list_add_tail(&data->domain_node, &domain->clients);
-			spin_unlock_irqrestore(&domain->lock, flags);
-		}
+		spin_lock(&data->lock);
+		data->master = dev;
+		data->pgtable = pagetable;
+		data->domain = domain;
+		list_add_tail(&data->domain_node, &domain->clients);
+		spin_unlock(&data->lock);
 	}
+	owner->domain = iommu_domain;
+	spin_unlock_irqrestore(&domain->lock, flags);
 
-	if (ret < 0) {
-		dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n",
-					__func__, &pagetable);
-		return ret;
-	}
+	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n",
+		__func__, &pagetable);
 
-	owner->domain = iommu_domain;
-	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
-		__func__, &pagetable, (ret == 0) ? "" : ", again");
+	list_for_each_entry(data, &owner->controllers, owner_node) {
+		pm_runtime_register_notifier(dev, &data->pm_nb);
+		if (pm_runtime_enabled(dev) && pm_runtime_active(dev))
+			sysmmu_restore_state(data);
+	}
 
-	return ret;
+	return 0;
 }
 
 static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
-- 
1.9.2

Re: [PATCH 1/3] PM / Runtime: Add notifiers for device runtime PM events

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 12:25 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> From: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>
> Allow drivers registering for certain runtime PM events of other
> devices. Some drivers in power domain are more or less coupled. When one
> driver is suspending (thus leading to power domain being also turned
> off) the other might have to perform some necessary steps. For example
> Exynos IOMMU has to save its context.
>
> Based on previous work of Sylwester Nawrocki <s.nawrocki@samsung.com>.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

No, this is not the right way to address this and using notifiers for
that is just wrong (because of the potential ordering issues).

Also, the problem is not limited to runtime PM, but also to system
suspend/resume and initialization/shutdown.

I posted a series of device dependencies patches a few months ago that
might help to address this problem, but there was almost no interest
in it at that time.

Thanks,
Rafael


> ---
>  drivers/base/power/generic_ops.c |  9 +++++++
>  drivers/base/power/power.h       |  6 +++++
>  drivers/base/power/runtime.c     | 34 +++++++++++++++++++++++++--
>  include/linux/pm.h               |  2 ++
>  include/linux/pm_runtime.h       | 51 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index 07c3c4a9522d..f0838229b781 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -10,6 +10,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/export.h>
>  #include <linux/suspend.h>
> +#include "power.h"
>
>  #ifdef CONFIG_PM
>  /**
> @@ -25,8 +26,12 @@ int pm_generic_runtime_suspend(struct device *dev)
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>         int ret;
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_PRE);
> +
>         ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_POST);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
> @@ -44,8 +49,12 @@ int pm_generic_runtime_resume(struct device *dev)
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>         int ret;
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_PRE);
> +
>         ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_POST);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 50e30e7b059d..30b6319ce96c 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -1,4 +1,5 @@
>  #include <linux/pm_qos.h>
> +#include <linux/pm_runtime.h>
>
>  static inline void device_pm_init_common(struct device *dev)
>  {
> @@ -20,6 +21,7 @@ static inline void pm_runtime_early_init(struct device *dev)
>  extern void pm_runtime_init(struct device *dev);
>  extern void pm_runtime_reinit(struct device *dev);
>  extern void pm_runtime_remove(struct device *dev);
> +extern int pm_runtime_notifier_call(struct device *dev, enum rpm_event event);
>
>  struct wake_irq {
>         struct device *dev;
> @@ -87,6 +89,10 @@ static inline void pm_runtime_early_init(struct device *dev)
>  static inline void pm_runtime_init(struct device *dev) {}
>  static inline void pm_runtime_reinit(struct device *dev) {}
>  static inline void pm_runtime_remove(struct device *dev) {}
> +static inline pm_runtime_notifier_call(struct device *dev, enum rpm_event event)
> +{
> +       return 0;
> +}
>
>  static inline int dpm_sysfs_add(struct device *dev) { return 0; }
>  static inline void dpm_sysfs_remove(struct device *dev) {}
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b74690418504..3a5637ca8400 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -227,6 +227,27 @@ void pm_runtime_set_memalloc_noio(struct device *dev, bool enable)
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_set_memalloc_noio);
>
> +int pm_runtime_notifier_call(struct device *dev, enum rpm_event event)
> +{
> +       return atomic_notifier_call_chain(&dev->power.runtime_notifier,
> +                                         event, dev);
> +}
> +
> +int pm_runtime_register_notifier(struct device *dev, struct notifier_block *nb)
> +{
> +       return atomic_notifier_chain_register(&dev->power.runtime_notifier,
> +                                             nb);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_register_notifier);
> +
> +int pm_runtime_unregister_notifier(struct device *dev,
> +                                   struct notifier_block *nb)
> +{
> +       return atomic_notifier_chain_unregister(&dev->power.runtime_notifier,
> +                                               nb);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_unregister_notifier);
> +
>  /**
>   * rpm_check_suspend_allowed - Test whether a device may be suspended.
>   * @dev: Device to test.
> @@ -1174,6 +1195,7 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
>                 goto out;
>         }
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_DISABLE_PRE);
>         /*
>          * Wake up the device if there's a resume request pending, because that
>          * means there probably is some I/O to process and disabling runtime PM
> @@ -1195,6 +1217,7 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
>         if (!dev->power.disable_depth++)
>                 __pm_runtime_barrier(dev);
>
> +       pm_runtime_notifier_call(dev, RPM_EVENT_DISABLE_POST);
>   out:
>         spin_unlock_irq(&dev->power.lock);
>  }
> @@ -1210,10 +1233,16 @@ void pm_runtime_enable(struct device *dev)
>
>         spin_lock_irqsave(&dev->power.lock, flags);
>
> -       if (dev->power.disable_depth > 0)
> +       if (dev->power.disable_depth > 0) {
> +               if (dev->power.disable_depth == 1)
> +                       pm_runtime_notifier_call(dev, RPM_EVENT_ENABLE_PRE);
>                 dev->power.disable_depth--;
> -       else
> +       } else {
>                 dev_warn(dev, "Unbalanced %s!\n", __func__);
> +       }
> +
> +       if (dev->power.disable_depth == 0)
> +               pm_runtime_notifier_call(dev, RPM_EVENT_ENABLE_POST);
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>  }
> @@ -1411,6 +1440,7 @@ void pm_runtime_init(struct device *dev)
>                         (unsigned long)dev);
>
>         init_waitqueue_head(&dev->power.wait_queue);
> +       ATOMIC_INIT_NOTIFIER_HEAD(&dev->power.runtime_notifier);
>  }
>
>  /**
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 06eb353182ab..bdf6eaaf62ec 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -27,6 +27,7 @@
>  #include <linux/wait.h>
>  #include <linux/timer.h>
>  #include <linux/completion.h>
> +#include <linux/notifier.h>
>
>  /*
>   * Callbacks for platform drivers to implement.
> @@ -604,6 +605,7 @@ struct dev_pm_info {
>         unsigned long           active_jiffies;
>         unsigned long           suspended_jiffies;
>         unsigned long           accounting_timestamp;
> +       struct atomic_notifier_head     runtime_notifier;
>  #endif
>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>         void (*set_latency_tolerance)(struct device *, s32);
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 2e14d2667b6c..82d41a6a6f5c 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -23,6 +23,43 @@
>                                             usage_count */
>  #define RPM_AUTO               0x08    /* Use autosuspend_delay */
>
> +/**
> + * Runtime PM notifier events.
> + *
> + * TODO: RPM_EVENT_SUSPEND/RPM_EVENT_RESUME are supported
> + * currently only with generic power domains. They won't be executed
> + * in other cases. Fix this.
> + *
> + * RPM_EVENT_SUSPEND_PRE       Before calling device suspend callback
> + *                             and before turning domain off
> + * RPM_EVENT_SUSPEND_POST      After suspending device and before turning
> + *                             domain off
> + *
> + * RPM_EVENT_RESUME_PRE                Before calling device resume callback
> + *                             but after turning domain on
> + * RPM_EVENT_RESUME_POST       After resuming device
> + *
> + * RPM_EVENT_ENABLE_PRE                Before effectively enabling runtime PM
> + *                             (the pm_runtime_enable() call must undo the
> + *                             last pm_runtime_disable() call)
> + * RPM_EVENT_ENABLE_POST       After effectively enabling runtime PM
> + *
> + * RPM_EVENT_DISABLE_PRE       Before effectively disabling runtime PM
> + *                             (the first pm_runtime_disable() call)
> + * RPM_EVENT_DISABLE_POST      After effectively disabling runtime PM
> + */
> +enum rpm_event {
> +       RPM_EVENT_SUSPEND_PRE,
> +       RPM_EVENT_SUSPEND_POST,
> +       RPM_EVENT_RESUME_PRE,
> +       RPM_EVENT_RESUME_POST,
> +       RPM_EVENT_ENABLE_PRE,
> +       RPM_EVENT_ENABLE_POST,
> +       RPM_EVENT_DISABLE_PRE,
> +       RPM_EVENT_DISABLE_POST,
> +       RPM_EVENT_NUM,
> +};
> +
>  #ifdef CONFIG_PM
>  extern struct workqueue_struct *pm_wq;
>
> @@ -55,6 +92,10 @@ extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
>  extern void pm_runtime_update_max_time_suspended(struct device *dev,
>                                                  s64 delta_ns);
>  extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
> +extern int pm_runtime_register_notifier(struct device *dev,
> +                                       struct notifier_block *nb);
> +extern int pm_runtime_unregister_notifier(struct device *dev,
> +                                          struct notifier_block *nb);
>
>  static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
>  {
> @@ -186,6 +227,16 @@ static inline unsigned long pm_runtime_autosuspend_expiration(
>                                 struct device *dev) { return 0; }
>  static inline void pm_runtime_set_memalloc_noio(struct device *dev,
>                                                 bool enable){}
> +static inline int pm_runtime_register_notifier(struct device *dev,
> +                                       struct notifier_block *nb)
> +{
> +       return -ENOSYS;
> +}
> +static innline int pm_runtime_unregister_notifier(struct device *dev,
> +                                          struct notifier_block *nb)
> +{
> +       return 0;
> +};
>
>  #endif /* !CONFIG_PM */
>
> --

Re: [PATCH 1/3] PM / Runtime: Add notifiers for device runtime PM events

[Marek Szyprowski]

Hi Rafael,


On 2016-06-08 19:18, Rafael J. Wysocki wrote:
> On Wed, Jun 8, 2016 at 12:25 PM, Marek Szyprowski
> <m.szyprowski@samsung.com>  wrote:
>> From: Krzysztof Kozlowski<k.kozlowski@samsung.com>
>>
>> Allow drivers registering for certain runtime PM events of other
>> devices. Some drivers in power domain are more or less coupled. When one
>> driver is suspending (thus leading to power domain being also turned
>> off) the other might have to perform some necessary steps. For example
>> Exynos IOMMU has to save its context.
>>
>> Based on previous work of Sylwester Nawrocki<s.nawrocki@samsung.com>.
>>
>> Signed-off-by: Krzysztof Kozlowski<k.kozlowski@samsung.com>
>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
> No, this is not the right way to address this and using notifiers for
> that is just wrong (because of the potential ordering issues).
>
> Also, the problem is not limited to runtime PM, but also to system
> suspend/resume and initialization/shutdown.
>
> I posted a series of device dependencies patches a few months ago that
> might help to address this problem, but there was almost no interest
> in it at that time.

I spent some time digging for your patches. Sadly no list archives had
them all and I finally found them only in the linux-pm patchwork. This
may explain why there was almost no interest in them.

After some debugging I've managed to get it working for my case. I will
include your patches in my v2 patchset together with the fixes needed to
get it working.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland