[PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device dependencies)

[PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device dependencies)

[Marek Szyprowski]

Hello,

This is another update of the patchset for adding proper runtime PM
support to Exynos IOMMU driver. This has been achieved by using recently
introduced device links, which lets SYSMMU controller's runtime PM to
follow master's device runtime PM state (the device which actually
performs DMA transaction). 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 solves the problem of all power domains being always
enabled. It happened, because all SYSMMU controllers (which belongs to
the same domains) are permanently kept active, because existing driver
was simplified and kept SYSMMU device active all the time after
initialization and attaching to the master device.

Patch requires fifth version of Rafeal's "Functional dependencies
between devices" patchset, which is available here:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1246897.html
or as git branch:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git device-links-test

If one wants to test this patchset, I've provided a branch with all needed
patches (a small fix for Exynos4 FIMC-IS DTS is still needed):
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.9-iommu-pm-v5

Patches are based on vanilla v4.9-rc1 kernel with Rafael's device
dependencies v5 patchset applied.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v5:
- split main patch into several small changes for easier review (requested
  by Luis Rodriquez)
- fixed usage of runtime_pm_active, now it is guarded by pm_runtime_get_noresume()
  and pm_runtime_put() pair

v4: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1241601.html
- rebased on top of v4 of device dependencies/links patchset, what resolved
  system hang on reboot

v3: http://www.spinics.net/lists/linux-samsung-soc/msg55256.html
- rebased on top of latest device dependencies/links patchset
- added proper locking between runtime pm, iommu_attach/detach and sysmmu
  enable/disable(added per iommu owner device's rpm lock)

v2: http://www.spinics.net/lists/arm-kernel/msg512082.html
- replaced PM notifiers with generic device dependencies/links developed
  by Rafael J. Wysocki

v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version


Patch summary:

Marek Szyprowski (7):
  iommu/exynos: Remove excessive, useless debug
  iommu/exynos: Remove dead code
  iommu/exynos: Simplify internal enable/disable functions
  iommu/exynos: Set master device once on boot
  iommu/exynos: Rework and fix internal locking
  iommu/exynos: Add runtime pm support
  iommu/exynos: Use device dependency links to control runtime pm

 drivers/iommu/exynos-iommu.c | 230 ++++++++++++++++++-------------------------
 1 file changed, 95 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH v5 1/7] 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 30808e91b775..8ba0d6049a63 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -578,9 +578,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.1

[PATCH v5 2/7] iommu/exynos: Remove dead code

[Marek Szyprowski]

__sysmmu_enable/disable functions were designed to do ref-count based
operations, but current code always calls them only once, so the code for
checking the conditions and invalid conditions can be simply removed
without any influence to the driver operation.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 8ba0d6049a63..4056228b8e5f 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -460,9 +460,6 @@ static bool __sysmmu_disable(struct sysmmu_drvdata *data)
 		__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);
@@ -508,29 +505,18 @@ static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
 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;
+	return 0;
 }
 
 static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
@@ -793,8 +779,8 @@ 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;
+		__sysmmu_disable(data);
+		data->master = NULL;
 		list_del_init(&data->domain_node);
 	}
 
@@ -829,31 +815,23 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	phys_addr_t pagetable = virt_to_phys(domain->pgtable);
 	struct sysmmu_drvdata *data, *next;
 	unsigned long flags;
-	bool found = false;
 
 	if (!has_sysmmu(dev) || owner->domain != iommu_domain)
 		return;
 
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		if (data->master == dev) {
-			if (__sysmmu_disable(data)) {
-				data->master = NULL;
-				list_del_init(&data->domain_node);
-			}
-			pm_runtime_put(data->sysmmu);
-			found = true;
-		}
+		__sysmmu_disable(data);
+		data->master = NULL;
+		list_del_init(&data->domain_node);
+		pm_runtime_put(data->sysmmu);
 	}
 	spin_unlock_irqrestore(&domain->lock, flags);
 
 	owner->domain = NULL;
 
-	if (found)
-		dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n",
-					__func__, &pagetable);
-	else
-		dev_err(dev, "%s: No IOMMU is attached\n", __func__);
+	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
+		&pagetable);
 }
 
 static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
@@ -864,7 +842,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;
@@ -874,27 +851,19 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 
 	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;
+		__sysmmu_enable(data, pagetable, domain);
+		data->master = dev;
 
-			spin_lock_irqsave(&domain->lock, flags);
-			list_add_tail(&data->domain_node, &domain->clients);
-			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;
+		spin_lock_irqsave(&domain->lock, flags);
+		list_add_tail(&data->domain_node, &domain->clients);
+		spin_unlock_irqrestore(&domain->lock, flags);
 	}
 
 	owner->domain = iommu_domain;
-	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
-		__func__, &pagetable, (ret == 0) ? "" : ", again");
+	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
+		&pagetable);
 
-	return ret;
+	return 0;
 }
 
 static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
-- 
1.9.1

[PATCH v5 3/7] iommu/exynos: Simplify internal enable/disable functions

[Marek Szyprowski]

Remove remaining leftovers of the ref-count related code in the
__sysmmu_enable/disable functions inline __sysmmu_enable/disable_nocount
to them. Suspend/resume callbacks now checks if master device is set for
given SYSMMU controller instead of relying on the activation count.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 4056228b8e5f..f45b274513cc 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -237,8 +237,8 @@ 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 */
@@ -251,25 +251,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);
@@ -388,7 +369,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;
@@ -434,37 +415,19 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __sysmmu_disable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_disable(struct sysmmu_drvdata *data)
 {
+	unsigned long flags;
+
 	clk_enable(data->clk_master);
 
+	spin_lock_irqsave(&data->lock, flags);
 	writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
 	writel(0, data->sfrbase + REG_MMU_CFG);
-
-	__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");
-	}
-
+	data->active = false;
 	spin_unlock_irqrestore(&data->lock, flags);
 
-	return disabled;
+	__sysmmu_disable_clocks(data);
 }
 
 static void __sysmmu_init_config(struct sysmmu_drvdata *data)
@@ -481,17 +444,19 @@ static void __sysmmu_init_config(struct sysmmu_drvdata *data)
 	writel(cfg, data->sfrbase + REG_MMU_CFG);
 }
 
-static void __sysmmu_enable_nocount(struct sysmmu_drvdata *data)
+static void __sysmmu_enable(struct sysmmu_drvdata *data)
 {
+	unsigned long flags;
+
 	__sysmmu_enable_clocks(data);
 
+	spin_lock_irqsave(&data->lock, flags);
 	writel(CTRL_BLOCK, data->sfrbase + REG_MMU_CTRL);
-
 	__sysmmu_init_config(data);
-
 	__sysmmu_set_ptbase(data, data->pgtable);
-
 	writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
+	data->active = true;
+	spin_unlock_irqrestore(&data->lock, flags);
 
 	/*
 	 * SYSMMU driver keeps master's clock enabled only for the short
@@ -502,37 +467,18 @@ 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)
-{
-	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");
-	}
-	spin_unlock_irqrestore(&data->lock, flags);
-
-	return 0;
-}
-
 static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
 					    sysmmu_iova_t iova)
 {
 	unsigned long flags;
 
-
 	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);
 	}
 	spin_unlock_irqrestore(&data->lock, flags);
-
 }
 
 static void sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
@@ -541,7 +487,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);
@@ -652,10 +598,11 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 static int exynos_sysmmu_suspend(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct device *master = data->master;
 
 	dev_dbg(dev, "suspend\n");
-	if (is_sysmmu_active(data)) {
-		__sysmmu_disable_nocount(data);
+	if (master) {
+		__sysmmu_disable(data);
 		pm_runtime_put(dev);
 	}
 	return 0;
@@ -664,11 +611,12 @@ static int exynos_sysmmu_suspend(struct device *dev)
 static int exynos_sysmmu_resume(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+	struct device *master = data->master;
 
 	dev_dbg(dev, "resume\n");
-	if (is_sysmmu_active(data)) {
+	if (master) {
 		pm_runtime_get_sync(dev);
-		__sysmmu_enable_nocount(data);
+		__sysmmu_enable(data);
 	}
 	return 0;
 }
@@ -780,6 +728,8 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
 		__sysmmu_disable(data);
+		data->pgtable = 0;
+		data->domain = NULL;
 		data->master = NULL;
 		list_del_init(&data->domain_node);
 	}
@@ -823,6 +773,8 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
 		__sysmmu_disable(data);
 		data->master = NULL;
+		data->pgtable = 0;
+		data->domain = NULL;
 		list_del_init(&data->domain_node);
 		pm_runtime_put(data->sysmmu);
 	}
@@ -850,8 +802,10 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 		exynos_iommu_detach_device(owner->domain, dev);
 
 	list_for_each_entry(data, &owner->controllers, owner_node) {
+		data->pgtable = pagetable;
+		data->domain = domain;
 		pm_runtime_get_sync(data->sysmmu);
-		__sysmmu_enable(data, pagetable, domain);
+		__sysmmu_enable(data);
 		data->master = dev;
 
 		spin_lock_irqsave(&domain->lock, flags);
-- 
1.9.1

[PATCH v5 4/7] iommu/exynos: Set master device once on boot

[Marek Szyprowski]

To avoid possible races, set master device pointer in each SYSMMU
controller once on boot. Suspend/resume callbacks now properly relies on
the configured iommu domain to enable or disable SYSMMU controller.
While changing the code, also update the sleep debug messages and make
them conditional.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index f45b274513cc..28e570b53672 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -600,10 +600,12 @@ static int exynos_sysmmu_suspend(struct device *dev)
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 	struct device *master = data->master;
 
-	dev_dbg(dev, "suspend\n");
 	if (master) {
-		__sysmmu_disable(data);
 		pm_runtime_put(dev);
+		if (data->domain) {
+			dev_dbg(data->sysmmu, "saving state\n");
+			__sysmmu_disable(data);
+		}
 	}
 	return 0;
 }
@@ -613,10 +615,12 @@ static int exynos_sysmmu_resume(struct device *dev)
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 	struct device *master = data->master;
 
-	dev_dbg(dev, "resume\n");
 	if (master) {
 		pm_runtime_get_sync(dev);
-		__sysmmu_enable(data);
+		if (data->domain) {
+			dev_dbg(data->sysmmu, "restoring state\n");
+			__sysmmu_enable(data);
+		}
 	}
 	return 0;
 }
@@ -730,7 +734,6 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)
 		__sysmmu_disable(data);
 		data->pgtable = 0;
 		data->domain = NULL;
-		data->master = NULL;
 		list_del_init(&data->domain_node);
 	}
 
@@ -772,7 +775,6 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
 		__sysmmu_disable(data);
-		data->master = NULL;
 		data->pgtable = 0;
 		data->domain = NULL;
 		list_del_init(&data->domain_node);
@@ -806,7 +808,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 		data->domain = domain;
 		pm_runtime_get_sync(data->sysmmu);
 		__sysmmu_enable(data);
-		data->master = dev;
 
 		spin_lock_irqsave(&domain->lock, flags);
 		list_add_tail(&data->domain_node, &domain->clients);
@@ -1192,6 +1193,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 	}
 
 	list_add_tail(&data->owner_node, &owner->controllers);
+	data->master = dev;
 	return 0;
 }
 
-- 
1.9.1

[PATCH v5 5/7] iommu/exynos: Rework and fix internal locking

[Marek Szyprowski]

This patch reworks locking in the exynos_iommu_attach/detach_device
functions to ensure that all entries of the sysmmu_drvdata and
exynos_iommu_owner structure are updated under the respective spinlocks,
while runtime pm functions are called without any spinlocks held.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 28e570b53672..a959443e6f33 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -731,10 +731,12 @@ 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) {
+		spin_lock(&data->lock);
 		__sysmmu_disable(data);
 		data->pgtable = 0;
 		data->domain = NULL;
 		list_del_init(&data->domain_node);
+		spin_unlock(&data->lock);
 	}
 
 	spin_unlock_irqrestore(&domain->lock, flags);
@@ -772,17 +774,22 @@ 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) {
+		__sysmmu_disable(data);
+		pm_runtime_put(data->sysmmu);
+	}
+
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry_safe(data, next, &domain->clients, domain_node) {
-		__sysmmu_disable(data);
+		spin_lock(&data->lock);
 		data->pgtable = 0;
 		data->domain = NULL;
 		list_del_init(&data->domain_node);
-		pm_runtime_put(data->sysmmu);
+		spin_unlock(&data->lock);
 	}
+	owner->domain = NULL;
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-	owner->domain = NULL;
 
 	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
 		&pagetable);
@@ -803,18 +810,22 @@ 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) {
+		spin_lock(&data->lock);
 		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);
+
+	list_for_each_entry(data, &owner->controllers, owner_node) {
 		pm_runtime_get_sync(data->sysmmu);
 		__sysmmu_enable(data);
-
-		spin_lock_irqsave(&domain->lock, flags);
-		list_add_tail(&data->domain_node, &domain->clients);
-		spin_unlock_irqrestore(&domain->lock, flags);
 	}
 
-	owner->domain = iommu_domain;
 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
 		&pagetable);
 
-- 
1.9.1

[PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Marek Szyprowski]

This patch adds runtime pm implementation, which is based on previous
suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
from the runtime pm callbacks. System sleep callbacks relies on generic
pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
internal state consistency, additional lock for runtime pm transitions
was introduced.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index a959443e6f33..5e6d7bbf9b70 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -206,6 +206,7 @@ struct sysmmu_fault_info {
 struct exynos_iommu_owner {
 	struct list_head controllers;	/* list of sysmmu_drvdata.owner_node */
 	struct iommu_domain *domain;	/* domain this device is attached */
+	struct mutex rpm_lock;		/* for runtime pm of all sysmmus */
 };
 
 /*
@@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int exynos_sysmmu_suspend(struct device *dev)
+static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 	struct device *master = data->master;
 
 	if (master) {
-		pm_runtime_put(dev);
+		struct exynos_iommu_owner *owner = master->archdata.iommu;
+
+		mutex_lock(&owner->rpm_lock);
 		if (data->domain) {
 			dev_dbg(data->sysmmu, "saving state\n");
 			__sysmmu_disable(data);
 		}
+		mutex_unlock(&owner->rpm_lock);
 	}
 	return 0;
 }
 
-static int exynos_sysmmu_resume(struct device *dev)
+static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
 {
 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
 	struct device *master = data->master;
 
 	if (master) {
-		pm_runtime_get_sync(dev);
+		struct exynos_iommu_owner *owner = master->archdata.iommu;
+
+		mutex_lock(&owner->rpm_lock);
 		if (data->domain) {
 			dev_dbg(data->sysmmu, "restoring state\n");
 			__sysmmu_enable(data);
 		}
+		mutex_unlock(&owner->rpm_lock);
 	}
 	return 0;
 }
-#endif
 
 static const struct dev_pm_ops sysmmu_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
+	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				     pm_runtime_force_resume)
 };
 
 static const struct of_device_id sysmmu_of_match[] __initconst = {
@@ -775,7 +782,15 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 		return;
 
 	list_for_each_entry(data, &owner->controllers, owner_node) {
-		__sysmmu_disable(data);
+		pm_runtime_put_sync(data->sysmmu);
+	}
+
+	mutex_lock(&owner->rpm_lock);
+
+	list_for_each_entry(data, &owner->controllers, owner_node) {
+		pm_runtime_get_noresume(data->sysmmu);
+		if (pm_runtime_active(data->sysmmu))
+			__sysmmu_disable(data);
 		pm_runtime_put(data->sysmmu);
 	}
 
@@ -790,6 +805,7 @@ static void exynos_iommu_detach_device(struct iommu_domain *iommu_domain,
 	owner->domain = NULL;
 	spin_unlock_irqrestore(&domain->lock, flags);
 
+	mutex_unlock(&owner->rpm_lock);
 
 	dev_dbg(dev, "%s: Detached IOMMU with pgtable %pa\n", __func__,
 		&pagetable);
@@ -810,6 +826,8 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	if (owner->domain)
 		exynos_iommu_detach_device(owner->domain, dev);
 
+	mutex_lock(&owner->rpm_lock);
+
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(data, &owner->controllers, owner_node) {
 		spin_lock(&data->lock);
@@ -822,8 +840,16 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	spin_unlock_irqrestore(&domain->lock, flags);
 
 	list_for_each_entry(data, &owner->controllers, owner_node) {
+		pm_runtime_get_noresume(data->sysmmu);
+		if (pm_runtime_active(data->sysmmu))
+			__sysmmu_enable(data);
+		pm_runtime_put(data->sysmmu);
+	}
+
+	mutex_unlock(&owner->rpm_lock);
+
+	list_for_each_entry(data, &owner->controllers, owner_node) {
 		pm_runtime_get_sync(data->sysmmu);
-		__sysmmu_enable(data);
 	}
 
 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
@@ -1200,6 +1226,7 @@ static int exynos_iommu_of_xlate(struct device *dev,
 			return -ENOMEM;
 
 		INIT_LIST_HEAD(&owner->controllers);
+		mutex_init(&owner->rpm_lock);
 		dev->archdata.iommu = owner;
 	}
 
-- 
1.9.1

RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Sricharan]

Hi Marek,

>This patch adds runtime pm implementation, which is based on previous
>suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
>from the runtime pm callbacks. System sleep callbacks relies on generic
>pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
>internal state consistency, additional lock for runtime pm transitions
>was introduced.
>
>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>---
> drivers/iommu/exynos-iommu.c | 45 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 36 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>index a959443e6f33..5e6d7bbf9b70 100644
>--- a/drivers/iommu/exynos-iommu.c
>+++ b/drivers/iommu/exynos-iommu.c
>@@ -206,6 +206,7 @@ struct sysmmu_fault_info {
> struct exynos_iommu_owner {
> 	struct list_head controllers;	/* list of sysmmu_drvdata.owner_node */
> 	struct iommu_domain *domain;	/* domain this device is attached */
>+	struct mutex rpm_lock;		/* for runtime pm of all sysmmus */
> };
>
> /*
>@@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
> 	return 0;
> }
>
>-#ifdef CONFIG_PM_SLEEP
>-static int exynos_sysmmu_suspend(struct device *dev)
>+static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
> {
> 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> 	struct device *master = data->master;
>
> 	if (master) {
>-		pm_runtime_put(dev);
>+		struct exynos_iommu_owner *owner = master->archdata.iommu;
>+
>+		mutex_lock(&owner->rpm_lock);
More of a device link question,
To understand, i see that with device link + runtime, the supplier
callbacks are not called for irqsafe clients, even if supplier is irqsafe.
Why so ?

> 		if (data->domain) {
> 			dev_dbg(data->sysmmu, "saving state\n");
> 			__sysmmu_disable(data);
> 		}
>+		mutex_unlock(&owner->rpm_lock);
> 	}
> 	return 0;
> }
>
>-static int exynos_sysmmu_resume(struct device *dev)
>+static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
> {
> 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
> 	struct device *master = data->master;
>
> 	if (master) {
>-		pm_runtime_get_sync(dev);
>+		struct exynos_iommu_owner *owner = master->archdata.iommu;
>+
>+		mutex_lock(&owner->rpm_lock);
> 		if (data->domain) {
> 			dev_dbg(data->sysmmu, "restoring state\n");
> 			__sysmmu_enable(data);
> 		}
>+		mutex_unlock(&owner->rpm_lock);
> 	}
> 	return 0;
> }
>-#endif
>
> static const struct dev_pm_ops sysmmu_pm_ops = {
>-	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
>+	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
>+	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>+				     pm_runtime_force_resume)
> };
 Is this needed to be LATE_SYSTEM_SLEEP_PM_OPS with device links to take care
  of the order ?

Regards,
 Sricharan

Re: [PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Marek Szyprowski]

Hi Sricharan


On 2016-10-22 07:50, Sricharan wrote:
>
>> This patch adds runtime pm implementation, which is based on previous
>> suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
> > from the runtime pm callbacks. System sleep callbacks relies on generic
>> pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
>> internal state consistency, additional lock for runtime pm transitions
>> was introduced.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> drivers/iommu/exynos-iommu.c | 45 +++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index a959443e6f33..5e6d7bbf9b70 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -206,6 +206,7 @@ struct sysmmu_fault_info {
>> struct exynos_iommu_owner {
>> 	struct list_head controllers;	/* list of sysmmu_drvdata.owner_node */
>> 	struct iommu_domain *domain;	/* domain this device is attached */
>> +	struct mutex rpm_lock;		/* for runtime pm of all sysmmus */
>> };
>>
>> /*
>> @@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>> 	return 0;
>> }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> -static int exynos_sysmmu_suspend(struct device *dev)
>> +static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
>> {
>> 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>> 	struct device *master = data->master;
>>
>> 	if (master) {
>> -		pm_runtime_put(dev);
>> +		struct exynos_iommu_owner *owner = master->archdata.iommu;
>> +
>> +		mutex_lock(&owner->rpm_lock);
> More of a device link question,
> To understand, i see that with device link + runtime, the supplier
> callbacks are not called for irqsafe clients, even if supplier is irqsafe.
> Why so ?

Frankly I didn't care about irqsafe runtime pm, because there is no such 
need
for Exynos platform and its drivers. Exynos power domain driver also doesn't
support irqsafe mode.

>
>> 		if (data->domain) {
>> 			dev_dbg(data->sysmmu, "saving state\n");
>> 			__sysmmu_disable(data);
>> 		}
>> +		mutex_unlock(&owner->rpm_lock);
>> 	}
>> 	return 0;
>> }
>>
>> -static int exynos_sysmmu_resume(struct device *dev)
>> +static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
>> {
>> 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>> 	struct device *master = data->master;
>>
>> 	if (master) {
>> -		pm_runtime_get_sync(dev);
>> +		struct exynos_iommu_owner *owner = master->archdata.iommu;
>> +
>> +		mutex_lock(&owner->rpm_lock);
>> 		if (data->domain) {
>> 			dev_dbg(data->sysmmu, "restoring state\n");
>> 			__sysmmu_enable(data);
>> 		}
>> +		mutex_unlock(&owner->rpm_lock);
>> 	}
>> 	return 0;
>> }
>> -#endif
>>
>> static const struct dev_pm_ops sysmmu_pm_ops = {
>> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume)
>> +	SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
>> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +				     pm_runtime_force_resume)
>> };
>   Is this needed to be LATE_SYSTEM_SLEEP_PM_OPS with device links to take care
>    of the order ?

Hmmm. LASE_SYSTEM_SLEEP_PM_OPS is a left over from the previous versions 
of the driver,
which doesn't use device links. You are right, that "normal" 
SYSTEM_SLEEP_PM_OPS should
be enough assuming that device links will take care of the proper call 
sequence between
consumer and supplier device.

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

RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support

[Sricharan]

Hi Marek,

>Hi Sricharan
>
>
>On 2016-10-22 07:50, Sricharan wrote:
>>
>>> This patch adds runtime pm implementation, which is based on previous
>>> suspend/resume code. SYSMMU controller is now being enabled/disabled mainly
>> > from the runtime pm callbacks. System sleep callbacks relies on generic
>>> pm_runtime_force_suspend/pm_runtime_force_resume helpers. To ensure
>>> internal state consistency, additional lock for runtime pm transitions
>>> was introduced.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>> drivers/iommu/exynos-iommu.c | 45 +++++++++++++++++++++++++++++++++++---------
>>> 1 file changed, 36 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>> index a959443e6f33..5e6d7bbf9b70 100644
>>> --- a/drivers/iommu/exynos-iommu.c
>>> +++ b/drivers/iommu/exynos-iommu.c
>>> @@ -206,6 +206,7 @@ struct sysmmu_fault_info {
>>> struct exynos_iommu_owner {
>>> 	struct list_head controllers;	/* list of sysmmu_drvdata.owner_node */
>>> 	struct iommu_domain *domain;	/* domain this device is attached */
>>> +	struct mutex rpm_lock;		/* for runtime pm of all sysmmus */
>>> };
>>>
>>> /*
>>> @@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>>> 	return 0;
>>> }
>>>
>>> -#ifdef CONFIG_PM_SLEEP
>>> -static int exynos_sysmmu_suspend(struct device *dev)
>>> +static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
>>> {
>>> 	struct sysmmu_drvdata *data = dev_get_drvdata(dev);
>>> 	struct device *master = data->master;
>>>
>>> 	if (master) {
>>> -		pm_runtime_put(dev);
>>> +		struct exynos_iommu_owner *owner = master->archdata.iommu;
>>> +
>>> +		mutex_lock(&owner->rpm_lock);
>> More of a device link question,
>> To understand, i see that with device link + runtime, the supplier
>> callbacks are not called for irqsafe clients, even if supplier is irqsafe.
>> Why so ?
>
>Frankly I didn't care about irqsafe runtime pm, because there is no such
>need
>for Exynos platform and its drivers. Exynos power domain driver also doesn't
>support irqsafe mode.
  ok, i asked this because, i was doing the same thing for arm-smmu driver
   and thought that when we depend on device-link for doing the runtime pm,
   then it might not work for irqsafe master. Probably i can ask this on device link
    series post.

Regards,
 Sricharan

[PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Marek Szyprowski]

This patch uses recently introduced device dependency links to track the
runtime pm state of the master's device. This way each SYSMMU controller
is set to runtime active only when its master's device is active and can
restore or save its state instead of being activated all the time when
attached to the given master device. This way SYSMMU controllers no longer
prevents respective power domains to be turned off when master's device
is not being used.

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

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 5e6d7bbf9b70..59b4f2ce4f5f 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
-	}
-
 	mutex_lock(&owner->rpm_lock);
 
 	list_for_each_entry(data, &owner->controllers, owner_node) {
@@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 
 	mutex_unlock(&owner->rpm_lock);
 
-	list_for_each_entry(data, &owner->controllers, owner_node) {
-		pm_runtime_get_sync(data->sysmmu);
-	}
-
 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
 		&pagetable);
 
@@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
 
 	list_add_tail(&data->owner_node, &owner->controllers);
 	data->master = dev;
+
+	/*
+	 * SYSMMU will be runtime activated via device link (dependency) to its
+	 * master device, so there are no direct calls to pm_runtime_get/put
+	 * in this driver.
+	 */
+	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
+
 	return 0;
 }
 
-- 
1.9.1

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,
>This patch uses recently introduced device dependency links to track the
>runtime pm state of the master's device. This way each SYSMMU controller
>is set to runtime active only when its master's device is active and can
>restore or save its state instead of being activated all the time when
>attached to the given master device. This way SYSMMU controllers no longer
>prevents respective power domains to be turned off when master's device
>is not being used.
>
>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>---
> drivers/iommu/exynos-iommu.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>--- a/drivers/iommu/exynos-iommu.c
>+++ b/drivers/iommu/exynos-iommu.c
>@@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
>-	}
>-
> 	mutex_lock(&owner->rpm_lock);
>
> 	list_for_each_entry(data, &owner->controllers, owner_node) {
>@@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>
> 	mutex_unlock(&owner->rpm_lock);
>
>-	list_for_each_entry(data, &owner->controllers, owner_node) {
>-		pm_runtime_get_sync(data->sysmmu);
>-	}
>-
> 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
> 		&pagetable);
>
>@@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>
> 	list_add_tail(&data->owner_node, &owner->controllers);
> 	data->master = dev;
>+
>+	/*
>+	 * SYSMMU will be runtime activated via device link (dependency) to its
>+	 * master device, so there are no direct calls to pm_runtime_get/put
>+	 * in this driver.
>+	 */
>+	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>+
  So in the case of master with multiple sids, this would be called multiple times
  for the same master ?

Regards,
 Sricharan

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Marek Szyprowski]

Hi Sricharan,


On 2016-10-23 11:49, Sricharan wrote:
> Hi Marek,
>> This patch uses recently introduced device dependency links to track the
>> runtime pm state of the master's device. This way each SYSMMU controller
>> is set to runtime active only when its master's device is active and can
>> restore or save its state instead of being activated all the time when
>> attached to the given master device. This way SYSMMU controllers no longer
>> prevents respective power domains to be turned off when master's device
>> is not being used.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> drivers/iommu/exynos-iommu.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
>> -	}
>> -
>> 	mutex_lock(&owner->rpm_lock);
>>
>> 	list_for_each_entry(data, &owner->controllers, owner_node) {
>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>
>> 	mutex_unlock(&owner->rpm_lock);
>>
>> -	list_for_each_entry(data, &owner->controllers, owner_node) {
>> -		pm_runtime_get_sync(data->sysmmu);
>> -	}
>> -
>> 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>> 		&pagetable);
>>
>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>
>> 	list_add_tail(&data->owner_node, &owner->controllers);
>> 	data->master = dev;
>> +
>> +	/*
>> +	 * SYSMMU will be runtime activated via device link (dependency) to its
>> +	 * master device, so there are no direct calls to pm_runtime_get/put
>> +	 * in this driver.
>> +	 */
>> +	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>> +
>    So in the case of master with multiple sids, this would be called multiple times
>    for the same master ?

I don't know what is "multiple sids" case, but if given SYSMMU master 
device (supplier)
has multiple SYSMMU controllers (consumers), then links will be created 
for each SYSMMU
controller. Please note that this code is based on vanilla v4.9-rc1, 
which calls
of_xlate() callback only once for every iommu for given master device. 
Your IOMMU
deferred probe patches change this, but I already posted a fix for 
Exynos IOMMU driver
to handle such case.

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

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,

>>> This patch uses recently introduced device dependency links to track the
>>> runtime pm state of the master's device. This way each SYSMMU controller
>>> is set to runtime active only when its master's device is active and can
>>> restore or save its state instead of being activated all the time when
>>> attached to the given master device. This way SYSMMU controllers no longer
>>> prevents respective power domains to be turned off when master's device
>>> is not being used.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>> drivers/iommu/exynos-iommu.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>>> --- a/drivers/iommu/exynos-iommu.c
>>> +++ b/drivers/iommu/exynos-iommu.c
>>> @@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
>>> -	}
>>> -
>>> 	mutex_lock(&owner->rpm_lock);
>>>
>>> 	list_for_each_entry(data, &owner->controllers, owner_node) {
>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>>
>>> 	mutex_unlock(&owner->rpm_lock);
>>>
>>> -	list_for_each_entry(data, &owner->controllers, owner_node) {
>>> -		pm_runtime_get_sync(data->sysmmu);
>>> -	}
>>> -
>>> 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>>> 		&pagetable);
>>>
>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>>
>>> 	list_add_tail(&data->owner_node, &owner->controllers);
>>> 	data->master = dev;
>>> +
>>> +	/*
>>> +	 * SYSMMU will be runtime activated via device link (dependency) to its
>>> +	 * master device, so there are no direct calls to pm_runtime_get/put
>>> +	 * in this driver.
>>> +	 */
>>> +	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>>> +
>>    So in the case of master with multiple sids, this would be called multiple times
>>    for the same master ?
>
>I don't know what is "multiple sids" case, but if given SYSMMU master
>device (supplier)
>has multiple SYSMMU controllers (consumers), then links will be created
>for each SYSMMU
>controller. Please note that this code is based on vanilla v4.9-rc1,
>which calls
>of_xlate() callback only once for every iommu for given master device.
>Your IOMMU
>deferred probe patches change this, but I already posted a fix for
>Exynos IOMMU driver
>to handle such case.
      By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case,
      so xlate would be called multiples for the same master without deferred
       probing also. But the fix that you showed on the other thread would work
       here as well or maybe if you dont have masters with multiple sids you wont
      have any issues as well.

Regards,
 Sricharan

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Marek Szyprowski]

Hi Sricharan,


On 2016-10-24 14:29, Sricharan wrote:
>>>> This patch uses recently introduced device dependency links to track the
>>>> runtime pm state of the master's device. This way each SYSMMU controller
>>>> is set to runtime active only when its master's device is active and can
>>>> restore or save its state instead of being activated all the time when
>>>> attached to the given master device. This way SYSMMU controllers no longer
>>>> prevents respective power domains to be turned off when master's device
>>>> is not being used.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>> drivers/iommu/exynos-iommu.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>>>> --- a/drivers/iommu/exynos-iommu.c
>>>> +++ b/drivers/iommu/exynos-iommu.c
>>>> @@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
>>>> -	}
>>>> -
>>>> 	mutex_lock(&owner->rpm_lock);
>>>>
>>>> 	list_for_each_entry(data, &owner->controllers, owner_node) {
>>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>>>
>>>> 	mutex_unlock(&owner->rpm_lock);
>>>>
>>>> -	list_for_each_entry(data, &owner->controllers, owner_node) {
>>>> -		pm_runtime_get_sync(data->sysmmu);
>>>> -	}
>>>> -
>>>> 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>>>> 		&pagetable);
>>>>
>>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>>>
>>>> 	list_add_tail(&data->owner_node, &owner->controllers);
>>>> 	data->master = dev;
>>>> +
>>>> +	/*
>>>> +	 * SYSMMU will be runtime activated via device link (dependency) to its
>>>> +	 * master device, so there are no direct calls to pm_runtime_get/put
>>>> +	 * in this driver.
>>>> +	 */
>>>> +	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>>>> +
>>>     So in the case of master with multiple sids, this would be called multiple times
>>>     for the same master ?
>> I don't know what is "multiple sids" case, but if given SYSMMU master
>> device (supplier)
>> has multiple SYSMMU controllers (consumers), then links will be created
>> for each SYSMMU
>> controller. Please note that this code is based on vanilla v4.9-rc1,
>> which calls
>> of_xlate() callback only once for every iommu for given master device.
>> Your IOMMU
>> deferred probe patches change this, but I already posted a fix for
>> Exynos IOMMU driver
>> to handle such case.
>   By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case,
>   so xlate would be called multiples for the same master without deferred
>   probing also. But the fix that you showed on the other thread would work
>   here as well or maybe if you dont have masters with multiple sids you wont
>   have any issues as well.

Exynos SYSMMU driver always use "#iommu-cells = <0>", so it doesn't support
multiple sids. However there is a case with 2 SYSMMU controllers attached
to the same master device: "iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>;".

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

RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Sricharan]

Hi Marek,

>>>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>>>> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
>>>>> --- a/drivers/iommu/exynos-iommu.c
>>>>> +++ b/drivers/iommu/exynos-iommu.c
>>>>> @@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
>>>>> -	}
>>>>> -
>>>>> 	mutex_lock(&owner->rpm_lock);
>>>>>
>>>>> 	list_for_each_entry(data, &owner->controllers, owner_node) {
>>>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>>>>
>>>>> 	mutex_unlock(&owner->rpm_lock);
>>>>>
>>>>> -	list_for_each_entry(data, &owner->controllers, owner_node) {
>>>>> -		pm_runtime_get_sync(data->sysmmu);
>>>>> -	}
>>>>> -
>>>>> 	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>>>>> 		&pagetable);
>>>>>
>>>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>>>>>
>>>>> 	list_add_tail(&data->owner_node, &owner->controllers);
>>>>> 	data->master = dev;
>>>>> +
>>>>> +	/*
>>>>> +	 * SYSMMU will be runtime activated via device link (dependency) to its
>>>>> +	 * master device, so there are no direct calls to pm_runtime_get/put
>>>>> +	 * in this driver.
>>>>> +	 */
>>>>> +	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>>>>> +
>>>>     So in the case of master with multiple sids, this would be called multiple times
>>>>     for the same master ?
>>> I don't know what is "multiple sids" case, but if given SYSMMU master
>>> device (supplier)
>>> has multiple SYSMMU controllers (consumers), then links will be created
>>> for each SYSMMU
>>> controller. Please note that this code is based on vanilla v4.9-rc1,
>>> which calls
>>> of_xlate() callback only once for every iommu for given master device.
>>> Your IOMMU
>>> deferred probe patches change this, but I already posted a fix for
>>> Exynos IOMMU driver
>>> to handle such case.
>>   By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case,
>>   so xlate would be called multiples for the same master without deferred
>>   probing also. But the fix that you showed on the other thread would work
>>   here as well or maybe if you dont have masters with multiple sids you wont
>>   have any issues as well.
>
>Exynos SYSMMU driver always use "#iommu-cells = <0>", so it doesn't support
>multiple sids. However there is a case with 2 SYSMMU controllers attached
>to the same master device: "iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>;".
>
   with iommu-cells = <0> always, its ok. The case of 2 SYSMMU controllers that
   is shown above is fine, as anyways both the suppliers (symmu) needs to linked
    seperately. So it looks all fine.

Regards,
  Sricharan

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Thu, Oct 20, 2016 at 09:22:53AM +0200, Marek Szyprowski wrote:
> This patch uses recently introduced device dependency links to track the
> runtime pm state of the master's device. This way each SYSMMU controller
> is set to runtime active only when its master's device is active and can
> restore or save its state instead of being activated all the time when
> attached to the given master device. This way SYSMMU controllers no longer
> prevents respective power domains to be turned off when master's device
> is not being used.

Its unclear why you need this based on this commit log -- is this
needed only on platforms that lack ACPI and use device tree ? If so
why? If this issue is present also on systems that only use ACPI is
this possibly due to an ACPI firmware bug  or the lack of some semantics
in ACPI to express ordering in a better way? If the issue is device
tree related only is this due to the lack of semantics in device tree
to express some more complex dependency ?

Has there been any review of the existing similar solutions out there
such as the DRM / audio component framework? Would that help ?

  Luis

> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/iommu/exynos-iommu.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -781,10 +781,6 @@ 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_put_sync(data->sysmmu);
> -	}
> -
>  	mutex_lock(&owner->rpm_lock);
>  
>  	list_for_each_entry(data, &owner->controllers, owner_node) {
> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>  
>  	mutex_unlock(&owner->rpm_lock);
>  
> -	list_for_each_entry(data, &owner->controllers, owner_node) {
> -		pm_runtime_get_sync(data->sysmmu);
> -	}
> -
>  	dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>  		&pagetable);
>  
> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  
>  	list_add_tail(&data->owner_node, &owner->controllers);
>  	data->master = dev;
> +
> +	/*
> +	 * SYSMMU will be runtime activated via device link (dependency) to its
> +	 * master device, so there are no direct calls to pm_runtime_get/put
> +	 * in this driver.
> +	 */
> +	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Marek Szyprowski]

Hi Luis,


On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> On Thu, Oct 20, 2016 at 09:22:53AM +0200, Marek Szyprowski wrote:
>> This patch uses recently introduced device dependency links to track the
>> runtime pm state of the master's device. This way each SYSMMU controller
>> is set to runtime active only when its master's device is active and can
>> restore or save its state instead of being activated all the time when
>> attached to the given master device. This way SYSMMU controllers no longer
>> prevents respective power domains to be turned off when master's device
>> is not being used.
> Its unclear why you need this based on this commit log -- is this
> needed only on platforms that lack ACPI and use device tree ?

Nope, it has nothing to device tree nor ACPI. The dependency is a direct
result of the way the devices operate.

> If so
> why? If this issue is present also on systems that only use ACPI is
> this possibly due to an ACPI firmware bug  or the lack of some semantics
> in ACPI to express ordering in a better way? If the issue is device
> tree related only is this due to the lack of semantics in device tree
> to express some more complex dependency ?

The main feature of device links that is used in this patch is enabling
runtime pm dependency between Exynos SYSMMU controller (called it client
device) and the device, for which it implements DMA address translation
(called master device). The assumptions are following:
1. master device driver is completely unaware of the Exynos SYSMMU presence,
    IOMMU is transparently hooked up and managed by DMA-mapping framework
2. SYSMMU belongs to the same power domain as it's master device
3. SYSMMU is optional, master device can fully operate without it, with
    simple DMA address translation (DMA address == physical address)
4. Master device implements runtime pm, what in turn causes respective
    power domain to be turned on/off
5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU
    when its master device is performing DMA operations, so SYSMMU has
    to be runtime active
6. Currently SYSMMU always sets its runtime pm status to active after
    attaching to its master device to ensure proper hardware state. This
    prevents power domain to be turned off, even when master device sets
    its runtime pm status to suspended.
7. Exynos SYSMMU has to be runtime active at the same time when its
    master device is runtime active to it to perform DMA operations and
    allow the power domain to be turned off, when master device is
    runtime suspended.
8. The terms of device links, Exynos SYSMMU is a 'consumer' and master
    device is a 'supplier'.

> Has there been any review of the existing similar solutions out there
> such as the DRM / audio component framework? Would that help ?

Nope, none of that solution deals with runtime pm.

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

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Lukas Wunner]

On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> > Has there been any review of the existing similar solutions out there
> > such as the DRM / audio component framework? Would that help ?
> 
> Nope, none of that solution deals with runtime pm.

Well, they do.  Hybrid graphics laptops often have an HDA controller
on the discrete GPU and I assume that's what Luis meant. There's code
in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:

* When the GPU is powered up/down, the HDA controller's driver is
  instructed to pm_runtime_get/put the HDA device (see call to
  set_audio_state() in vga_switcheroo_set_dynamic_switch()).

* When a runtime PM ref is acquired on the HDA device, the
  GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).


Unfortunately this is all fairly broken, e.g.:

* If a runtime PM ref on the HDA device is held for more than 5 sec
  (autosuspend delay of the GPU), the GPU will be powered down and
  the HDA device will become inaccessible, regardless of the runtime
  PM ref still being held (because vga_switcheroo_set_dynamic_switch()
  doesn't check the refcount of the HDA device).

* The DRM device is afforded direct-complete but the HDA device is not.
  If the GPU is handled earlier by dpm_suspend(), then runtime PM will
  have been disabled on the GPU and thus the HDA device will fail to
  runtime resume before system sleep.

Rafael's series allows representation of such inter-device dependencies
in the PM core and can thus replace kludgy and broken "solutions" like
the one above.

There's one thing that I haven't understood myself though:  In an e-mail
exchange in September Rafael has argued that the above-mentioned hybrid
graphics use case "isn't a good [example] IMO.  That clearly is a case
when two (or more) devices share power resources controlled by a single
on/off switch.  Which is a clear use case for a PM domain."

The same seems to apply to Marek's SYSMMU use case.  When applying device
links to SYSMMU or hybrid graphics, we select one of the devices in the
PM domain as master and have the other one depend on it as slave, i.e.
a synthetic hierarchical relationship is established.

I've responded to Rafael on September 18 that this can't be solved with
a struct dev_pm_domain, but haven't received a reply since:
https://lkml.org/lkml/2016/9/18/103

Thanks,

Lukas

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> > > Has there been any review of the existing similar solutions out there
> > > such as the DRM / audio component framework? Would that help ?
> > 
> > Nope, none of that solution deals with runtime pm.
> 
> Well, they do.  Hybrid graphics laptops often have an HDA controller
> on the discrete GPU and I assume that's what Luis meant. There's code
> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
> 
> * When the GPU is powered up/down, the HDA controller's driver is
>   instructed to pm_runtime_get/put the HDA device (see call to
>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
> 
> * When a runtime PM ref is acquired on the HDA device, the
>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
> 
> 
> Unfortunately this is all fairly broken, e.g.:
> 
> * If a runtime PM ref on the HDA device is held for more than 5 sec
>   (autosuspend delay of the GPU), the GPU will be powered down and
>   the HDA device will become inaccessible, regardless of the runtime
>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
>   doesn't check the refcount of the HDA device).
> 
> * The DRM device is afforded direct-complete but the HDA device is not.
>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
>   have been disabled on the GPU and thus the HDA device will fail to
>   runtime resume before system sleep.
> 
> Rafael's series allows representation of such inter-device dependencies
> in the PM core and can thus replace kludgy and broken "solutions" like
> the one above.
> 
> There's one thing that I haven't understood myself though:  In an e-mail
> exchange in September Rafael has argued that the above-mentioned hybrid
> graphics use case "isn't a good [example] IMO.  That clearly is a case
> when two (or more) devices share power resources controlled by a single
> on/off switch.  Which is a clear use case for a PM domain."
> 
> The same seems to apply to Marek's SYSMMU use case.  When applying device
> links to SYSMMU or hybrid graphics, we select one of the devices in the
> PM domain as master and have the other one depend on it as slave, i.e.
> a synthetic hierarchical relationship is established.
> 
> I've responded to Rafael on September 18 that this can't be solved with
> a struct dev_pm_domain, but haven't received a reply since:
> https://lkml.org/lkml/2016/9/18/103

Rafael note:

The one he asked here.

  Luis

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Rafael J. Wysocki]

On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
>> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
>> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
>> > > Has there been any review of the existing similar solutions out there
>> > > such as the DRM / audio component framework? Would that help ?
>> >
>> > Nope, none of that solution deals with runtime pm.
>>
>> Well, they do.  Hybrid graphics laptops often have an HDA controller
>> on the discrete GPU and I assume that's what Luis meant. There's code
>> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
>>
>> * When the GPU is powered up/down, the HDA controller's driver is
>>   instructed to pm_runtime_get/put the HDA device (see call to
>>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
>>
>> * When a runtime PM ref is acquired on the HDA device, the
>>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
>>
>>
>> Unfortunately this is all fairly broken, e.g.:
>>
>> * If a runtime PM ref on the HDA device is held for more than 5 sec
>>   (autosuspend delay of the GPU), the GPU will be powered down and
>>   the HDA device will become inaccessible, regardless of the runtime
>>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
>>   doesn't check the refcount of the HDA device).
>>
>> * The DRM device is afforded direct-complete but the HDA device is not.
>>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
>>   have been disabled on the GPU and thus the HDA device will fail to
>>   runtime resume before system sleep.
>>
>> Rafael's series allows representation of such inter-device dependencies
>> in the PM core and can thus replace kludgy and broken "solutions" like
>> the one above.
>>
>> There's one thing that I haven't understood myself though:  In an e-mail
>> exchange in September Rafael has argued that the above-mentioned hybrid
>> graphics use case "isn't a good [example] IMO.  That clearly is a case
>> when two (or more) devices share power resources controlled by a single
>> on/off switch.  Which is a clear use case for a PM domain."
>>
>> The same seems to apply to Marek's SYSMMU use case.  When applying device
>> links to SYSMMU or hybrid graphics, we select one of the devices in the
>> PM domain as master and have the other one depend on it as slave, i.e.
>> a synthetic hierarchical relationship is established.
>>
>> I've responded to Rafael on September 18 that this can't be solved with
>> a struct dev_pm_domain, but haven't received a reply since:
>> https://lkml.org/lkml/2016/9/18/103
>
> Rafael note:
>
> The one he asked here.

OK, so please see the message I've just sent. :-)

The bottom line is that there may be multiple ways to approach a
problem like this and which of them works best really depends on the
particular case.

You seem to be thinking that the device links infrastructure may not
be necessary after all if there are other ways to address the problems
it is intended for, but those other ways may still be less viable
practically due to the complexity involved and similar.  Also they may
lead to code duplication in different places that try to address those
problems in a similar fashion, but slightly differently.

All this is about providing people with reasonably straightforward and
common ways to deal with certain class of problems showing up in
multiple places.  It is not about getting the driver probe ordering
right magically in one go.

Thanks,
Rafael

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Thu, Nov 10, 2016 at 01:05:42AM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
> >> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> >> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> >> > > Has there been any review of the existing similar solutions out there
> >> > > such as the DRM / audio component framework? Would that help ?
> >> >
> >> > Nope, none of that solution deals with runtime pm.
> >>
> >> Well, they do.  Hybrid graphics laptops often have an HDA controller
> >> on the discrete GPU and I assume that's what Luis meant. There's code
> >> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
> >>
> >> * When the GPU is powered up/down, the HDA controller's driver is
> >>   instructed to pm_runtime_get/put the HDA device (see call to
> >>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
> >>
> >> * When a runtime PM ref is acquired on the HDA device, the
> >>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
> >>
> >>
> >> Unfortunately this is all fairly broken, e.g.:
> >>
> >> * If a runtime PM ref on the HDA device is held for more than 5 sec
> >>   (autosuspend delay of the GPU), the GPU will be powered down and
> >>   the HDA device will become inaccessible, regardless of the runtime
> >>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
> >>   doesn't check the refcount of the HDA device).
> >>
> >> * The DRM device is afforded direct-complete but the HDA device is not.
> >>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
> >>   have been disabled on the GPU and thus the HDA device will fail to
> >>   runtime resume before system sleep.
> >>
> >> Rafael's series allows representation of such inter-device dependencies
> >> in the PM core and can thus replace kludgy and broken "solutions" like
> >> the one above.
> >>
> >> There's one thing that I haven't understood myself though:  In an e-mail
> >> exchange in September Rafael has argued that the above-mentioned hybrid
> >> graphics use case "isn't a good [example] IMO.  That clearly is a case
> >> when two (or more) devices share power resources controlled by a single
> >> on/off switch.  Which is a clear use case for a PM domain."
> >>
> >> The same seems to apply to Marek's SYSMMU use case.  When applying device
> >> links to SYSMMU or hybrid graphics, we select one of the devices in the
> >> PM domain as master and have the other one depend on it as slave, i.e.
> >> a synthetic hierarchical relationship is established.
> >>
> >> I've responded to Rafael on September 18 that this can't be solved with
> >> a struct dev_pm_domain, but haven't received a reply since:
> >> https://lkml.org/lkml/2016/9/18/103
> >
> > Rafael note:
> >
> > The one he asked here.
> 
> OK, so please see the message I've just sent. :-)
> 
> The bottom line is that there may be multiple ways to approach a
> problem like this and which of them works best really depends on the
> particular case.
> 
> You seem to be thinking that the device links infrastructure may not
> be necessary after all if there are other ways to address the problems
> it is intended for, but those other ways may still be less viable
> practically due to the complexity involved and similar.  Also they may
> lead to code duplication in different places that try to address those
> problems in a similar fashion, but slightly differently.

I was not arguing that, I have been suggesting that pre-existing solutions
should at least be reviewed and considered, if they are sub-par and
the device links infrastructure is much simpler and provides the same
solution folks should be alert and consider embracing it. If not and
its the other way around and we could generalize the others, it should
mean we could learn from them.

>From what I gather from Plumbers its not clear to me any of this review
was done, that's all. This leaves a series of open questions about those
existing solutions.

> All this is about providing people with reasonably straightforward and
> common ways to deal with certain class of problems showing up in
> multiple places.  It is not about getting the driver probe ordering
> right magically in one go.

Right, I just want us to avoid re-inventing the wheel.

  Luis
> 
> Thanks,
> Rafael
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Rafael J. Wysocki]

On Thu, Nov 10, 2016 at 1:12 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Nov 10, 2016 at 01:05:42AM +0100, Rafael J. Wysocki wrote:
>> On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
>> >> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
>> >> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
>> >> > > Has there been any review of the existing similar solutions out there
>> >> > > such as the DRM / audio component framework? Would that help ?
>> >> >
>> >> > Nope, none of that solution deals with runtime pm.
>> >>
>> >> Well, they do.  Hybrid graphics laptops often have an HDA controller
>> >> on the discrete GPU and I assume that's what Luis meant. There's code
>> >> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
>> >>
>> >> * When the GPU is powered up/down, the HDA controller's driver is
>> >>   instructed to pm_runtime_get/put the HDA device (see call to
>> >>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
>> >>
>> >> * When a runtime PM ref is acquired on the HDA device, the
>> >>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
>> >>
>> >>
>> >> Unfortunately this is all fairly broken, e.g.:
>> >>
>> >> * If a runtime PM ref on the HDA device is held for more than 5 sec
>> >>   (autosuspend delay of the GPU), the GPU will be powered down and
>> >>   the HDA device will become inaccessible, regardless of the runtime
>> >>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
>> >>   doesn't check the refcount of the HDA device).
>> >>
>> >> * The DRM device is afforded direct-complete but the HDA device is not.
>> >>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
>> >>   have been disabled on the GPU and thus the HDA device will fail to
>> >>   runtime resume before system sleep.
>> >>
>> >> Rafael's series allows representation of such inter-device dependencies
>> >> in the PM core and can thus replace kludgy and broken "solutions" like
>> >> the one above.
>> >>
>> >> There's one thing that I haven't understood myself though:  In an e-mail
>> >> exchange in September Rafael has argued that the above-mentioned hybrid
>> >> graphics use case "isn't a good [example] IMO.  That clearly is a case
>> >> when two (or more) devices share power resources controlled by a single
>> >> on/off switch.  Which is a clear use case for a PM domain."
>> >>
>> >> The same seems to apply to Marek's SYSMMU use case.  When applying device
>> >> links to SYSMMU or hybrid graphics, we select one of the devices in the
>> >> PM domain as master and have the other one depend on it as slave, i.e.
>> >> a synthetic hierarchical relationship is established.
>> >>
>> >> I've responded to Rafael on September 18 that this can't be solved with
>> >> a struct dev_pm_domain, but haven't received a reply since:
>> >> https://lkml.org/lkml/2016/9/18/103
>> >
>> > Rafael note:
>> >
>> > The one he asked here.
>>
>> OK, so please see the message I've just sent. :-)
>>
>> The bottom line is that there may be multiple ways to approach a
>> problem like this and which of them works best really depends on the
>> particular case.
>>
>> You seem to be thinking that the device links infrastructure may not
>> be necessary after all if there are other ways to address the problems
>> it is intended for, but those other ways may still be less viable
>> practically due to the complexity involved and similar.  Also they may
>> lead to code duplication in different places that try to address those
>> problems in a similar fashion, but slightly differently.
>
> I was not arguing that, I have been suggesting that pre-existing solutions
> should at least be reviewed and considered, if they are sub-par and
> the device links infrastructure is much simpler and provides the same
> solution folks should be alert and consider embracing it. If not and
> its the other way around and we could generalize the others, it should
> mean we could learn from them.
>
> From what I gather from Plumbers its not clear to me any of this review
> was done, that's all. This leaves a series of open questions about those
> existing solutions.
>
>> All this is about providing people with reasonably straightforward and
>> common ways to deal with certain class of problems showing up in
>> multiple places.  It is not about getting the driver probe ordering
>> right magically in one go.
>
> Right, I just want us to avoid re-inventing the wheel.

Well, actually, you seem to be missing a bit of context. :-)

Something similar to device links as submitted had already been
considered in the 2010-or-so time frame (IIRC), but then we thought
that maybe the extra complexity was not needed after all.  Fast
forward a few years and a few more-or-less failing attempts to address
those problems in different ways and here we go again.  Plus, there
are more apparent problems of the nature in question now, but let me
address that in a different branch of this thread.

Thanks,
Rafael

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Rafael J. Wysocki]

On Tue, Nov 8, 2016 at 4:30 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
>> On 2016-11-07 22:47, Luis R. Rodriguez wrote:
>> > Has there been any review of the existing similar solutions out there
>> > such as the DRM / audio component framework? Would that help ?
>>
>> Nope, none of that solution deals with runtime pm.
>
> Well, they do.  Hybrid graphics laptops often have an HDA controller
> on the discrete GPU and I assume that's what Luis meant. There's code
> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
>
> * When the GPU is powered up/down, the HDA controller's driver is
>   instructed to pm_runtime_get/put the HDA device (see call to
>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
>
> * When a runtime PM ref is acquired on the HDA device, the
>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
>
>
> Unfortunately this is all fairly broken, e.g.:
>
> * If a runtime PM ref on the HDA device is held for more than 5 sec
>   (autosuspend delay of the GPU), the GPU will be powered down and
>   the HDA device will become inaccessible, regardless of the runtime
>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
>   doesn't check the refcount of the HDA device).
>
> * The DRM device is afforded direct-complete but the HDA device is not.
>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
>   have been disabled on the GPU and thus the HDA device will fail to
>   runtime resume before system sleep.
>
> Rafael's series allows representation of such inter-device dependencies
> in the PM core and can thus replace kludgy and broken "solutions" like
> the one above.
>
> There's one thing that I haven't understood myself though:  In an e-mail
> exchange in September Rafael has argued that the above-mentioned hybrid
> graphics use case "isn't a good [example] IMO.  That clearly is a case
> when two (or more) devices share power resources controlled by a single
> on/off switch.  Which is a clear use case for a PM domain."
>
> The same seems to apply to Marek's SYSMMU use case.  When applying device
> links to SYSMMU or hybrid graphics, we select one of the devices in the
> PM domain as master and have the other one depend on it as slave, i.e.
> a synthetic hierarchical relationship is established.
>
> I've responded to Rafael on September 18 that this can't be solved with
> a struct dev_pm_domain, but haven't received a reply since:
> https://lkml.org/lkml/2016/9/18/103

Well, that clearly fell off my radar, sorry about that.

The idea, roughly, is that if there is a single on/off switch acting
on multiple devices, you can (a) set up a PM domain tracking all of
those device's runtime PM invocations and (b) maintaining a reference
counter of devices still not suspended.  This way it would only turn
the switch off when all of the devices in question had been suspended.
Analogously, it would turn the switch on before resuming the first
device in the domain.  Of course, that code isn't available as a
library, you would need to implement it (or use genpd, but chances are
it is too heavy weight for the job).

In theory, it shouldn't really matter where the switch is and how it
is operated as long as the PM domain "driver" knows how to access it.
However, for that to work something would need to bind that "driver"
to the domain and something would need to know which devices needed to
be added to the PM domains during enumeration and the ordering of
things bringup matters a lot here, which is a problem.

So in short, you are right that for the GPU/HDA thing device links
would likely to be a simpler to use and still get the job done.

Thanks,
Rafael

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Lukas Wunner]

On Thu, Nov 10, 2016 at 12:56:14AM +0100, Rafael J. Wysocki wrote:
> The idea, roughly, is that if there is a single on/off switch acting
> on multiple devices, you can (a) set up a PM domain tracking all of
> those device's runtime PM invocations and (b) maintaining a reference
> counter of devices still not suspended.  This way it would only turn
> the switch off when all of the devices in question had been suspended.
> Analogously, it would turn the switch on before resuming the first
> device in the domain.  Of course, that code isn't available as a
> library, you would need to implement it (or use genpd, but chances are
> it is too heavy weight for the job).

My understanding is that the hierarchy of struct generic_pm_domain
is created by the platform on boot.  For an embedded platform, this
is encoded in the device tree, but what about ACPI which doesn't
know anything about struct generic_pm_domain?  I would have to lump
devices into generic_pm_domains after the fact, after the platform
has scanned the buses, but this seems to be forbidden according to
this slide deck, which calls that a "layering violation":

https://events.linuxfoundation.org/images/stories/pdf/lcjp2012_wysocki.pdf

(Quote: "Adding and Removing Devices [...] Supposed to be called by
the platform (calling one of them from a device driver is a layering
violation).")

So it seems that using struct generic_pm_domain is never an option
on ACPI, is that correct?

Thanks,

Lukas

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Lukas Wunner]

On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> > If so
> > why? If this issue is present also on systems that only use ACPI is
> > this possibly due to an ACPI firmware bug  or the lack of some semantics
> > in ACPI to express ordering in a better way? If the issue is device
> > tree related only is this due to the lack of semantics in device tree
> > to express some more complex dependency ?
> 
> The main feature of device links that is used in this patch is enabling
> runtime pm dependency between Exynos SYSMMU controller (called it client
> device) and the device, for which it implements DMA address translation
> (called master device). The assumptions are following:
> 1. master device driver is completely unaware of the Exynos SYSMMU presence,
>    IOMMU is transparently hooked up and managed by DMA-mapping framework
> 2. SYSMMU belongs to the same power domain as it's master device
> 3. SYSMMU is optional, master device can fully operate without it, with
>    simple DMA address translation (DMA address == physical address)
> 4. Master device implements runtime pm, what in turn causes respective
>    power domain to be turned on/off
> 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU
>    when its master device is performing DMA operations, so SYSMMU has
>    to be runtime active
> 6. Currently SYSMMU always sets its runtime pm status to active after
>    attaching to its master device to ensure proper hardware state. This
>    prevents power domain to be turned off, even when master device sets
>    its runtime pm status to suspended.
> 7. Exynos SYSMMU has to be runtime active at the same time when its
>    master device is runtime active to it to perform DMA operations and
>    allow the power domain to be turned off, when master device is
>    runtime suspended.
> 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master
>    device is a 'supplier'.

You seem to have mixed up the consumer and supplier in point 8 above.
Your code is such that the SYSMMU is the supplier and the master device
is the consumer:

	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);

Prototype of device_link_add:

	struct device_link *device_link_add(struct device *consumer,
				            struct device *supplier,
					    u32 flags);

Your code is correct, only point 8 above is wrong.

Best regards,

Lukas

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Marek Szyprowski]

Hi Lukas,


On 2016-11-19 12:11, Lukas Wunner wrote:
> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
>> On 2016-11-07 22:47, Luis R. Rodriguez wrote:
>>> If so
>>> why? If this issue is present also on systems that only use ACPI is
>>> this possibly due to an ACPI firmware bug  or the lack of some semantics
>>> in ACPI to express ordering in a better way? If the issue is device
>>> tree related only is this due to the lack of semantics in device tree
>>> to express some more complex dependency ?
>> The main feature of device links that is used in this patch is enabling
>> runtime pm dependency between Exynos SYSMMU controller (called it client
>> device) and the device, for which it implements DMA address translation
>> (called master device). The assumptions are following:
>> 1. master device driver is completely unaware of the Exynos SYSMMU presence,
>>     IOMMU is transparently hooked up and managed by DMA-mapping framework
>> 2. SYSMMU belongs to the same power domain as it's master device
>> 3. SYSMMU is optional, master device can fully operate without it, with
>>     simple DMA address translation (DMA address == physical address)
>> 4. Master device implements runtime pm, what in turn causes respective
>>     power domain to be turned on/off
>> 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU
>>     when its master device is performing DMA operations, so SYSMMU has
>>     to be runtime active
>> 6. Currently SYSMMU always sets its runtime pm status to active after
>>     attaching to its master device to ensure proper hardware state. This
>>     prevents power domain to be turned off, even when master device sets
>>     its runtime pm status to suspended.
>> 7. Exynos SYSMMU has to be runtime active at the same time when its
>>     master device is runtime active to it to perform DMA operations and
>>     allow the power domain to be turned off, when master device is
>>     runtime suspended.
>> 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master
>>     device is a 'supplier'.
> You seem to have mixed up the consumer and supplier in point 8 above.
> Your code is such that the SYSMMU is the supplier and the master device
> is the consumer:
>
> 	device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
>
> Prototype of device_link_add:
>
> 	struct device_link *device_link_add(struct device *consumer,
> 				            struct device *supplier,
> 					    u32 flags);
>
> Your code is correct, only point 8 above is wrong.

Thanks for checking this. You are right that I mixed up consumer and 
supplier
in point 8. I'm sorry for the confusion.

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