* [PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device dependencies) [not found] <CGME20161020072330eucas1p2b09ad8d091171edbac9449815fdc0fb7@eucas1p2.samsung.com> @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device dependencies) @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 ^ permalink raw reply [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072331eucas1p1af7dc7270b0b19168b949f3416eda474@eucas1p1.samsung.com>]
* [PATCH v5 1/7] iommu/exynos: Remove excessive, useless debug @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 1/7] iommu/exynos: Remove excessive, useless debug @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072332eucas1p1d980c1659979bd5bc2918bfc9d40a415@eucas1p1.samsung.com>]
* [PATCH v5 2/7] iommu/exynos: Remove dead code @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa __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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 2/7] iommu/exynos: Remove dead code @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner __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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072332eucas1p26960035de3007724498d59057329683d@eucas1p2.samsung.com>]
* [PATCH v5 3/7] iommu/exynos: Simplify internal enable/disable functions @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 3/7] iommu/exynos: Simplify internal enable/disable functions @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072333eucas1p25b638379091939f10b3c9eb5d89a031e@eucas1p2.samsung.com>]
* [PATCH v5 4/7] iommu/exynos: Set master device once on boot [not found] ` <CGME20161020072333eucas1p25b638379091939f10b3c9eb5d89a031e@eucas1p2.samsung.com> @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072334eucas1p2a159a25a2875611eff208381ebdb2e84@eucas1p2.samsung.com>]
* [PATCH v5 5/7] iommu/exynos: Rework and fix internal locking @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 5/7] iommu/exynos: Rework and fix internal locking @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072335eucas1p209675d6fbf39e5045281e8023fa9d234@eucas1p2.samsung.com>]
* [PATCH v5 6/7] iommu/exynos: Add runtime pm support @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 6/7] iommu/exynos: Add runtime pm support @ 2016-10-20 7:22 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: Tomeu Vizoso, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman, Kevin Hilman, Rafael J. Wysocki, Tomasz Figa, Krzysztof Kozlowski, Inki Dae, Tobias Jakobi, Luis R. Rodriguez, Kukjin Kim, Mark Brown, Lukas Wunner 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support @ 2016-10-22 5:50 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-22 5:50 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support @ 2016-10-22 5:50 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-22 5:50 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: 'Tomeu Vizoso', 'Kevin Hilman', 'Greg Kroah-Hartman', 'Mark Brown', 'Bartlomiej Zolnierkiewicz', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Lukas Wunner' 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> >--- > 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 6/7] iommu/exynos: Add runtime pm support 2016-10-22 5:50 ` Sricharan (?) @ 2016-10-24 5:19 ` Marek Szyprowski 2016-10-24 12:15 ` Sricharan -1 siblings, 1 reply; 50+ messages in thread From: Marek Szyprowski @ 2016-10-24 5:19 UTC (permalink / raw) To: Sricharan, linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support 2016-10-24 5:19 ` Marek Szyprowski @ 2016-10-24 12:15 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-24 12:15 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support @ 2016-10-24 12:15 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-24 12:15 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
[parent not found: <CGME20161020072336eucas1p24a2b020f69b6ae1f55e1760e6e0e94f9@eucas1p2.samsung.com>]
* [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm [not found] ` <CGME20161020072336eucas1p24a2b020f69b6ae1f55e1760e6e0e94f9@eucas1p2.samsung.com> @ 2016-10-20 7:22 ` Marek Szyprowski 2016-10-23 9:49 ` Sricharan 2016-11-07 21:47 ` Luis R. Rodriguez 0 siblings, 2 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-20 7:22 UTC (permalink / raw) To: linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: Marek Szyprowski, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-23 9:49 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-23 9:49 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-23 9:49 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-23 9:49 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: 'Tomeu Vizoso', 'Kevin Hilman', 'Greg Kroah-Hartman', 'Mark Brown', 'Bartlomiej Zolnierkiewicz', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Lukas Wunner' 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> >--- > 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-10-23 9:49 ` Sricharan @ 2016-10-24 5:30 ` Marek Szyprowski -1 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-24 5:30 UTC (permalink / raw) To: Sricharan, linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-24 5:30 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-10-24 5:30 UTC (permalink / raw) To: Sricharan, linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: 'Tomeu Vizoso', 'Kevin Hilman', 'Greg Kroah-Hartman', 'Mark Brown', 'Bartlomiej Zolnierkiewicz', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Lukas Wunner' 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> >> --- >> 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-10-24 5:30 ` Marek Szyprowski @ 2016-10-24 12:29 ` Sricharan -1 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-24 12:29 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-24 12:29 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-24 12:29 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-10-24 12:29 ` Sricharan (?) @ 2016-10-24 12:39 ` Marek Szyprowski 2016-10-25 6:53 ` Sricharan -1 siblings, 1 reply; 50+ messages in thread From: Marek Szyprowski @ 2016-10-24 12:39 UTC (permalink / raw) To: Sricharan, linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-25 6:53 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-25 6:53 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm, linux-kernel, iommu, linux-samsung-soc Cc: 'Tomeu Vizoso', 'Bartlomiej Zolnierkiewicz', 'Greg Kroah-Hartman', 'Kevin Hilman', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Mark Brown', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-10-25 6:53 ` Sricharan 0 siblings, 0 replies; 50+ messages in thread From: Sricharan @ 2016-10-25 6:53 UTC (permalink / raw) To: 'Marek Szyprowski', linux-pm-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA Cc: 'Tomeu Vizoso', 'Kevin Hilman', 'Greg Kroah-Hartman', 'Mark Brown', 'Bartlomiej Zolnierkiewicz', 'Rafael J. Wysocki', 'Tomasz Figa', 'Krzysztof Kozlowski', 'Inki Dae', 'Tobias Jakobi', 'Luis R. Rodriguez', 'Kukjin Kim', 'Lukas Wunner' 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-07 21:47 ` Luis R. Rodriguez 0 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-07 21:47 UTC (permalink / raw) To: Marek Szyprowski Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Luis R. Rodriguez, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-07 21:47 ` Luis R. Rodriguez 0 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-07 21:47 UTC (permalink / raw) To: Marek Szyprowski Cc: Tomasz Figa, Grant Likely, Andrzej Hajda, Laurent Pinchart, Lukas Wunner, Mauro Carvalho Chehab, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA, Kevin Hilman, Bartlomiej Zolnierkiewicz, Luis R. Rodriguez, Krzysztof Kozlowski, Kukjin Kim, linux-pm-u79uwXL29TY76Z2rM5mHXA, Inki Dae, Lars-Peter Clausen, Tomeu Vizoso, Greg Kroah-Hartman, Dmitry Torokhov, Rafael J. Wysocki, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Tobias Jakobi, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Mark 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> > --- > 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-07 21:47 ` Luis R. Rodriguez @ 2016-11-08 7:27 ` Marek Szyprowski -1 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-11-08 7:27 UTC (permalink / raw) To: Luis R. Rodriguez Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-08 7:27 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-11-08 7:27 UTC (permalink / raw) To: Luis R. Rodriguez Cc: linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Lukas Wunner, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-08 7:27 ` Marek Szyprowski @ 2016-11-08 15:30 ` Lukas Wunner -1 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-08 15:30 UTC (permalink / raw) To: Marek Szyprowski Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-08 15:30 ` Lukas Wunner 0 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-08 15:30 UTC (permalink / raw) To: Marek Szyprowski Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-08 15:30 ` Lukas Wunner @ 2016-11-09 23:55 ` Luis R. Rodriguez -1 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-09 23:55 UTC (permalink / raw) To: Lukas Wunner, Rafael J. Wysocki Cc: Marek Szyprowski, Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-09 23:55 ` Luis R. Rodriguez 0 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-09 23:55 UTC (permalink / raw) To: Lukas Wunner, Rafael J. Wysocki Cc: Marek Szyprowski, Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-09 23:55 ` Luis R. Rodriguez @ 2016-11-10 0:05 ` Rafael J. Wysocki -1 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-10 0:05 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-10 0:05 ` Rafael J. Wysocki 0 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-10 0:05 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-10 0:05 ` Rafael J. Wysocki @ 2016-11-10 0:12 ` Luis R. Rodriguez -1 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-10 0:12 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Luis R. Rodriguez, Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-10 0:12 ` Luis R. Rodriguez 0 siblings, 0 replies; 50+ messages in thread From: Luis R. Rodriguez @ 2016-11-10 0:12 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Luis R. Rodriguez, Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-10 0:12 ` Luis R. Rodriguez @ 2016-11-10 0:20 ` Rafael J. Wysocki -1 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-10 0:20 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Rafael J. Wysocki, Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-10 0:20 ` Rafael J. Wysocki 0 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-10 0:20 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Rafael J. Wysocki, Lukas Wunner, Rafael J. Wysocki, Marek Szyprowski, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-08 15:30 ` Lukas Wunner @ 2016-11-09 23:56 ` Rafael J. Wysocki -1 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-09 23:56 UTC (permalink / raw) To: Lukas Wunner Cc: Marek Szyprowski, Luis R. Rodriguez, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-09 23:56 ` Rafael J. Wysocki 0 siblings, 0 replies; 50+ messages in thread From: Rafael J. Wysocki @ 2016-11-09 23:56 UTC (permalink / raw) To: Lukas Wunner Cc: Marek Szyprowski, Luis R. Rodriguez, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-09 23:56 ` Rafael J. Wysocki @ 2016-11-16 9:30 ` Lukas Wunner -1 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-16 9:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Marek Szyprowski, Luis R. Rodriguez, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-16 9:30 ` Lukas Wunner 0 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-16 9:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Marek Szyprowski, Luis R. Rodriguez, Linux PM, Linux Kernel Mailing List, open list:AMD IOMMU (AMD-VI), Linux Samsung SoC, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-08 7:27 ` Marek Szyprowski @ 2016-11-19 11:11 ` Lukas Wunner -1 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-19 11:11 UTC (permalink / raw) To: Marek Szyprowski Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-19 11:11 ` Lukas Wunner 0 siblings, 0 replies; 50+ messages in thread From: Lukas Wunner @ 2016-11-19 11:11 UTC (permalink / raw) To: Marek Szyprowski Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm 2016-11-19 11:11 ` Lukas Wunner @ 2016-11-21 13:11 ` Marek Szyprowski -1 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-11-21 13:11 UTC (permalink / raw) To: Lukas Wunner Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen, Andrzej Hajda, Mauro Carvalho Chehab, Dmitry Torokhov 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm @ 2016-11-21 13:11 ` Marek Szyprowski 0 siblings, 0 replies; 50+ messages in thread From: Marek Szyprowski @ 2016-11-21 13:11 UTC (permalink / raw) To: Lukas Wunner Cc: Luis R. Rodriguez, linux-pm, linux-kernel, iommu, linux-samsung-soc, Joerg Roedel, Inki Dae, Kukjin Kim, Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso, Kevin Hilman, Tobias Jakobi, Tomasz Figa, Grant Likely, Laurent Pinchart, Lars-Peter Clausen 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 ^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2016-11-21 13:11 UTC | newest] Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20161020072330eucas1p2b09ad8d091171edbac9449815fdc0fb7@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device dependencies) Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski [not found] ` <CGME20161020072331eucas1p1af7dc7270b0b19168b949f3416eda474@eucas1p1.samsung.com> 2016-10-20 7:22 ` [PATCH v5 1/7] iommu/exynos: Remove excessive, useless debug Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski [not found] ` <CGME20161020072332eucas1p1d980c1659979bd5bc2918bfc9d40a415@eucas1p1.samsung.com> 2016-10-20 7:22 ` [PATCH v5 2/7] iommu/exynos: Remove dead code Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski [not found] ` <CGME20161020072332eucas1p26960035de3007724498d59057329683d@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 3/7] iommu/exynos: Simplify internal enable/disable functions Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski [not found] ` <CGME20161020072333eucas1p25b638379091939f10b3c9eb5d89a031e@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 4/7] iommu/exynos: Set master device once on boot Marek Szyprowski [not found] ` <CGME20161020072334eucas1p2a159a25a2875611eff208381ebdb2e84@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 5/7] iommu/exynos: Rework and fix internal locking Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski [not found] ` <CGME20161020072335eucas1p209675d6fbf39e5045281e8023fa9d234@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 6/7] iommu/exynos: Add runtime pm support Marek Szyprowski 2016-10-20 7:22 ` Marek Szyprowski 2016-10-22 5:50 ` Sricharan 2016-10-22 5:50 ` Sricharan 2016-10-24 5:19 ` Marek Szyprowski 2016-10-24 12:15 ` Sricharan 2016-10-24 12:15 ` Sricharan [not found] ` <CGME20161020072336eucas1p24a2b020f69b6ae1f55e1760e6e0e94f9@eucas1p2.samsung.com> 2016-10-20 7:22 ` [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm Marek Szyprowski 2016-10-23 9:49 ` Sricharan 2016-10-23 9:49 ` Sricharan 2016-10-24 5:30 ` Marek Szyprowski 2016-10-24 5:30 ` Marek Szyprowski 2016-10-24 12:29 ` Sricharan 2016-10-24 12:29 ` Sricharan 2016-10-24 12:39 ` Marek Szyprowski 2016-10-25 6:53 ` Sricharan 2016-10-25 6:53 ` Sricharan 2016-11-07 21:47 ` Luis R. Rodriguez 2016-11-07 21:47 ` Luis R. Rodriguez 2016-11-08 7:27 ` Marek Szyprowski 2016-11-08 7:27 ` Marek Szyprowski 2016-11-08 15:30 ` Lukas Wunner 2016-11-08 15:30 ` Lukas Wunner 2016-11-09 23:55 ` Luis R. Rodriguez 2016-11-09 23:55 ` Luis R. Rodriguez 2016-11-10 0:05 ` Rafael J. Wysocki 2016-11-10 0:05 ` Rafael J. Wysocki 2016-11-10 0:12 ` Luis R. Rodriguez 2016-11-10 0:12 ` Luis R. Rodriguez 2016-11-10 0:20 ` Rafael J. Wysocki 2016-11-10 0:20 ` Rafael J. Wysocki 2016-11-09 23:56 ` Rafael J. Wysocki 2016-11-09 23:56 ` Rafael J. Wysocki 2016-11-16 9:30 ` Lukas Wunner 2016-11-16 9:30 ` Lukas Wunner 2016-11-19 11:11 ` Lukas Wunner 2016-11-19 11:11 ` Lukas Wunner 2016-11-21 13:11 ` Marek Szyprowski 2016-11-21 13:11 ` Marek Szyprowski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.