Discussion:
[PATCH v4 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)
Marek Szyprowski
2016-09-29 08:12:29 UTC
Permalink
Hello,

This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce 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 situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).

Patch requires fourth version of Rafeal's "Functional dependencies
between devices" patchset, which is available here:
https://www.mail-archive.com/linux-***@vger.kernel.org/msg1241473.html

If one wants to test this patchset, I've provided a branch with all needed
patches (some fixes for Exynos4 FIMC-IS driver are needed):
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v4

Patches are based on vanilla v4.8-rc8 kernel with Rafael's device
dependencies v4 patchset applied.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v4:
- 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 developped
by Rafael J. Wysocki

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


Patch summary:

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

drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 134 deletions(-)
--
1.9.1
Marek Szyprowski
2016-09-29 08:12:30 UTC
Permalink
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 <***@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 33dcc29ec200..dfb44034b4ee 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
Marek Szyprowski
2016-09-29 08:12:31 UTC
Permalink
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active and can save/restore its state
instead of being enabled all the time. This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.

Signed-off-by: Marek Szyprowski <***@samsung.com>
---
drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 131 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index dfb44034b4ee..c8926e030713 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -206,6 +206,7 @@ static const struct sysmmu_fault_info sysmmu_v5_faults[] = {
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 */
};

/*
@@ -237,8 +238,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 +252,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 +370,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,40 +416,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");
- } else {
- dev_dbg(data->sysmmu, "%d times left to disable\n",
- data->activations);
- }
-
+ data->active = false;
spin_unlock_irqrestore(&data->lock, flags);

- return disabled;
+ __sysmmu_disable_clocks(data);
}

static void __sysmmu_init_config(struct sysmmu_drvdata *data)
@@ -484,17 +445,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
@@ -505,48 +468,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)
-{
- int ret = 0;
- unsigned long flags;
-
- spin_lock_irqsave(&data->lock, flags);
- if (set_sysmmu_active(data)) {
- data->pgtable = pgtable;
- data->domain = domain;
-
- __sysmmu_enable_nocount(data);
-
- dev_dbg(data->sysmmu, "Enabled\n");
- } else {
- ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
-
- dev_dbg(data->sysmmu, "already enabled\n");
- }
-
- if (WARN_ON(ret < 0))
- set_sysmmu_inactive(data); /* decrement count */
-
- spin_unlock_irqrestore(&data->lock, flags);
-
- return ret;
-}
-
static void sysmmu_tlb_invalidate_flpdcache(struct sysmmu_drvdata *data,
sysmmu_iova_t iova)
{
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,
@@ -555,7 +488,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);
@@ -656,40 +589,55 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
}

pm_runtime_enable(dev);
-
of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);

return 0;
}

-#ifdef CONFIG_PM_SLEEP
static int exynos_sysmmu_suspend(struct device *dev)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+ struct exynos_iommu_owner *owner;

- dev_dbg(dev, "suspend\n");
- if (is_sysmmu_active(data)) {
- __sysmmu_disable_nocount(data);
- pm_runtime_put(dev);
+ if (!data->master)
+ return 0;
+
+ owner = data->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)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
+ struct exynos_iommu_owner *owner;

- dev_dbg(dev, "resume\n");
- if (is_sysmmu_active(data)) {
- pm_runtime_get_sync(dev);
- __sysmmu_enable_nocount(data);
+ if (!data->master)
+ return 0;
+
+ owner = data->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 = {
@@ -793,9 +741,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) {
- if (__sysmmu_disable(data))
- data->master = NULL;
+ 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);
@@ -829,31 +780,32 @@ 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;

+ mutex_lock(&owner->rpm_lock);
+
+ list_for_each_entry(data, &owner->controllers, owner_node) {
+ if (pm_runtime_active(data->sysmmu))
+ __sysmmu_disable(data);
+ }
+
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;
- }
+ spin_lock(&data->lock);
+ data->pgtable = 0;
+ data->domain = NULL;
+ list_del_init(&data->domain_node);
+ spin_unlock(&data->lock);
}
+ owner->domain = NULL;
spin_unlock_irqrestore(&domain->lock, flags);

- owner->domain = NULL;
+ mutex_unlock(&owner->rpm_lock);

- 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 +816,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;
@@ -872,29 +823,30 @@ 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) {
- pm_runtime_get_sync(data->sysmmu);
- ret = __sysmmu_enable(data, pagetable, domain);
- if (ret >= 0) {
- data->master = dev;
-
- spin_lock_irqsave(&domain->lock, flags);
- list_add_tail(&data->domain_node, &domain->clients);
- spin_unlock_irqrestore(&domain->lock, flags);
- }
+ spin_lock(&data->lock);
+ data->pgtable = pagetable;
+ data->domain = domain;
+ list_add_tail(&data->domain_node, &domain->clients);
+ spin_unlock(&data->lock);
}
+ owner->domain = iommu_domain;
+ spin_unlock_irqrestore(&domain->lock, flags);

- if (ret < 0) {
- dev_err(dev, "%s: Failed to attach IOMMU with pgtable %pa\n",
- __func__, &pagetable);
- return ret;
+ list_for_each_entry(data, &owner->controllers, owner_node) {
+ if (pm_runtime_active(data->sysmmu))
+ __sysmmu_enable(data);
}

- owner->domain = iommu_domain;
- dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa %s\n",
- __func__, &pagetable, (ret == 0) ? "" : ", again");
+ mutex_unlock(&owner->rpm_lock);

- return ret;
+ dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
+ &pagetable);
+
+ return 0;
}

static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
@@ -1265,10 +1217,21 @@ 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;
}

list_add_tail(&data->owner_node, &owner->controllers);
+ data->master = dev;
+
+ /*
+ * SYSMMU will be runtime enabled 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, DEVICE_LINK_AVAILABLE,
+ DEVICE_LINK_PM_RUNTIME);
+
return 0;
}
--
1.9.1
Luis R. Rodriguez
2016-10-06 17:37:56 UTC
Permalink
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?

BTW what is the master device of a SYSMMU? I have no clue about these
IOMMU devices here.
Post by Marek Szyprowski
and can save/restore its state instead of being enabled all the time.
I take it this means currently even if the master device is disabled
(whatever that is) all SYSMMU controllers are kept enabled, is that right?
The issue here is this wastes power? Or what is the issue?
Post by Marek Szyprowski
This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.
So when the master device is idle we want to also remove power from the
controllers ? How much power does this save on a typical device in the
market BTW ?
Post by Marek Szyprowski
---
drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 131 deletions(-)
I'm reviewing the device link patches now but since this is a demo of
use of that I'll note the changes here are pretty large and it makes
it terribly difficult for review. Is there any way this patch can be split
up in to logical atomic pieces that only do one task upon change ?

Luis
Marek Szyprowski
2016-10-10 13:32:06 UTC
Permalink
Hi Luis
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?
instead of keeping SYSMMU controller runtime active all the time.
Post by Luis R. Rodriguez
BTW what is the master device of a SYSMMU? I have no clue about these
IOMMU devices here.
Here is a more detailed description of IOMMU hardware I wrote a few days ago
for Ulf:
http://www.mail-archive.com/linux-***@vger.kernel.org/msg1231006.html

In short: there is a SYSMMU controller and its master device - a device,
which performs DMA operations. That SYSMMU sits in between system memory
and the master device, so it performs mapping of DMA addresses to physical
memory addresses on each DMA operation.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
and can save/restore its state instead of being enabled all the time.
I take it this means currently even if the master device is disabled
(whatever that is) all SYSMMU controllers are kept enabled, is that right?
The issue here is this wastes power? Or what is the issue?
Yes, the issue here is the fact that SYSMMU is kept active all the time,
what in turn prevent the power domain for turning off even if master device
doesn't do anything and is already suspended. This directly (some clocks
enabled) and in-directly (leakage current) causes power looses.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.
So when the master device is idle we want to also remove power from the
controllers ? How much power does this save on a typical device in the
market BTW ?
The main purpose of this patchset is to let power domains to be turned off,
because with the current code all domains are all the time turned on,
because
SYSMMU controllers prevent them from turning them off.

If you want I can measure the power consumption of the idle board with all
domains enabled and disabled if you want to see the numbers. On the
other board
disabling most power domains in idle state (the clocks were already
disabled)
gave me about 20mA savings (at 3.7V), what is a significant value for the
battery powered device.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
---
drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 131 deletions(-)
I'm reviewing the device link patches now but since this is a demo of
use of that I'll note the changes here are pretty large and it makes
it terribly difficult for review. Is there any way this patch can be split
up in to logical atomic pieces that only do one task upon change ?
I will try to split it a bit, but I cannot promise that much can be done
to improve readability for someone not very familiar with the driver
internals.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Luis R. Rodriguez
2016-11-08 22:14:37 UTC
Permalink
Post by Marek Szyprowski
Hi Luis
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?
instead of keeping SYSMMU controller runtime active all the time.
I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ? Because as far as I can tell this was painted to help
with suspend/resume ?
Post by Marek Szyprowski
Post by Luis R. Rodriguez
BTW what is the master device of a SYSMMU? I have no clue about these
IOMMU devices here.
Here is a more detailed description of IOMMU hardware I wrote a few days ago
In short: there is a SYSMMU controller and its master device - a device,
which performs DMA operations. That SYSMMU sits in between system memory
and the master device, so it performs mapping of DMA addresses to physical
memory addresses on each DMA operation.
So you seek a run time power optimization ? Or a fix on suspend? Or both?
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
and can save/restore its state instead of being enabled all the time.
I take it this means currently even if the master device is disabled
(whatever that is) all SYSMMU controllers are kept enabled, is that right?
The issue here is this wastes power? Or what is the issue?
Yes, the issue here is the fact that SYSMMU is kept active all the time,
what in turn prevent the power domain for turning off even if master device
doesn't do anything and is already suspended. This directly (some clocks
enabled) and in-directly (leakage current) causes power looses.
Thanks for the confirmation so really the biggest concern here was run time PM.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.
So when the master device is idle we want to also remove power from the
controllers ? How much power does this save on a typical device in the
market BTW ?
The main purpose of this patchset is to let power domains to be turned off,
because with the current code all domains are all the time turned on,
because
SYSMMU controllers prevent them from turning them off.
I see.. I think the audio folks already addressed this with DAPM, but granted
this was for audio. Then I was also referred to the DRM / Audio component
framework, has this been looked into? v4l folks have v4l async stuff but
its not clear if that help with run time PM. I'm mentioning these given it'd be
silly to re-invent the wheel, additionally if we now have a generic solution
everyone can jump on board with there is quite a bit of work we can do to
dump a lot of old legacy crap.
Post by Marek Szyprowski
If you want I can measure the power consumption of the idle board with all
domains enabled and disabled if you want to see the numbers. On the other
board
disabling most power domains in idle state (the clocks were already
disabled)
gave me about 20mA savings (at 3.7V), what is a significant value for the
battery powered device.
Thanks, this means nothing to me, however it would be value-add to the commit log
as anyone reviewing this can understand what the goal / savings was for exactly.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
---
drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 131 deletions(-)
I'm reviewing the device link patches now but since this is a demo of
use of that I'll note the changes here are pretty large and it makes
it terribly difficult for review. Is there any way this patch can be split
up in to logical atomic pieces that only do one task upon change ?
I will try to split it a bit, but I cannot promise that much can be done
to improve readability for someone not very familiar with the driver
internals.
I've heard this before, I don't buy it but lets see!

Luis
Marek Szyprowski
2016-11-09 15:07:34 UTC
Permalink
Hi Luis,
Post by Luis R. Rodriguez
Post by Marek Szyprowski
Hi Luis
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?
instead of keeping SYSMMU controller runtime active all the time.
I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ?
Yes, it solves both problems, although the suspend/resume was easy to
workaround
just by using LATE_SLEEP_OPS.
Post by Luis R. Rodriguez
Because as far as I can tell this was painted to help
with suspend/resume ?
It also helped to remove the suspend/resume workaround.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
Post by Luis R. Rodriguez
BTW what is the master device of a SYSMMU? I have no clue about these
IOMMU devices here.
Here is a more detailed description of IOMMU hardware I wrote a few days ago
In short: there is a SYSMMU controller and its master device - a device,
which performs DMA operations. That SYSMMU sits in between system memory
and the master device, so it performs mapping of DMA addresses to physical
memory addresses on each DMA operation.
So you seek a run time power optimization ? Or a fix on suspend? Or both?
The main reason for using device links was to implement proper runtime power
optimization.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
and can save/restore its state instead of being enabled all the time.
I take it this means currently even if the master device is disabled
(whatever that is) all SYSMMU controllers are kept enabled, is that right?
The issue here is this wastes power? Or what is the issue?
Yes, the issue here is the fact that SYSMMU is kept active all the time,
what in turn prevent the power domain for turning off even if master device
doesn't do anything and is already suspended. This directly (some clocks
enabled) and in-directly (leakage current) causes power looses.
Thanks for the confirmation so really the biggest concern here was run time PM.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This way SYSMMU controllers no
longer prevents respective power domains to be turned off when master's
device is not used.
So when the master device is idle we want to also remove power from the
controllers ? How much power does this save on a typical device in the
market BTW ?
The main purpose of this patchset is to let power domains to be turned off,
because with the current code all domains are all the time turned on,
because
SYSMMU controllers prevent them from turning them off.
I see.. I think the audio folks already addressed this with DAPM, but granted
this was for audio. Then I was also referred to the DRM / Audio component
framework, has this been looked into? v4l folks have v4l async stuff but
its not clear if that help with run time PM. I'm mentioning these given it'd be
silly to re-invent the wheel, additionally if we now have a generic solution
everyone can jump on board with there is quite a bit of work we can do to
dump a lot of old legacy crap.
Right, probably some workarounds here and there can be removed. However
components
and v4l-async solutions are for resolving only probe and registration
issues and they
are some kind of pool for grouping devices and triggering special
callback once all
requested devices in the pool have probed.
Post by Luis R. Rodriguez
Post by Marek Szyprowski
If you want I can measure the power consumption of the idle board with all
domains enabled and disabled if you want to see the numbers. On the other
board
disabling most power domains in idle state (the clocks were already
disabled)
gave me about 20mA savings (at 3.7V), what is a significant value for the
battery powered device.
Thanks, this means nothing to me, however it would be value-add to the commit log
as anyone reviewing this can understand what the goal / savings was for exactly.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Post by Marek Szyprowski
---
drivers/iommu/exynos-iommu.c | 225 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 131 deletions(-)
I'm reviewing the device link patches now but since this is a demo of
use of that I'll note the changes here are pretty large and it makes
it terribly difficult for review. Is there any way this patch can be split
up in to logical atomic pieces that only do one task upon change ?
I will try to split it a bit, but I cannot promise that much can be done
to improve readability for someone not very familiar with the driver
internals.
I've heard this before, I don't buy it but lets see!
Somehow I managed to split this all-in-one patch into several smaller
changes
in v5 and v6 was posted yesterday ago.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Rafael J. Wysocki
2016-11-10 00:36:29 UTC
Permalink
On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski
Post by Marek Szyprowski
Hi Luis,
Post by Luis R. Rodriguez
Post by Marek Szyprowski
Hi Luis
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?
instead of keeping SYSMMU controller runtime active all the time.
I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ?
Yes, it solves both problems, although the suspend/resume was easy to
workaround just by using LATE_SLEEP_OPS.
Right, but that's just in this particular case, because the dependency
chain is of length 2. :-)

If you had a longer chain, you might in theory use the _noirq() stage
somehow, but that has limitations.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Because as far as I can tell this was painted to help
with suspend/resume ?
It helps with three things, (async) suspend/resume, runtime PM and
shutdown (that last part is the hardest to figure out). The ordering
in which all of these are carried out is analogous and cannot be
determined correctly by the device registration ordering itself in
general (which has been a known fact for years, but some localized
workarounds were put in some places to work around that).

Moreover, even if the list ordering (of dpm_list, for instance) is
correct, it still doesn't guarantee the right ordering of actions that
are carried out asynchronously. They are all started in the list
order, but they may be running in parallel with each other and
complete at different times. For this reason, there needs to be a way
to ensure that, say, the suspend operations for consumer devices
complete before their suppliers will become unavailable and so on.

Both runtime PM and system suspend/resume have this problem. It is
not present in the system shutdown case, but it still helps to get a
correct list ordering (ie. such that won't cause supplier devices to
be shut down before their consumers) in this case too.

Thanks,
Rafael
Luis R. Rodriguez
2016-11-10 01:15:07 UTC
Permalink
Post by Rafael J. Wysocki
On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski
Post by Marek Szyprowski
Hi Luis,
Post by Luis R. Rodriguez
Post by Marek Szyprowski
Hi Luis
Post by Luis R. Rodriguez
Post by Marek Szyprowski
This patch uses recently introduced device links to track the runtime pm
state of the master's device. This way each SYSMMU controller is runtime
activated when its master's device is active
instead of?
instead of keeping SYSMMU controller runtime active all the time.
I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ?
Yes, it solves both problems, although the suspend/resume was easy to
workaround just by using LATE_SLEEP_OPS.
Right, but that's just in this particular case, because the dependency
chain is of length 2. :-)
If you had a longer chain, you might in theory use the _noirq() stage
somehow, but that has limitations.
Post by Marek Szyprowski
Post by Luis R. Rodriguez
Because as far as I can tell this was painted to help
with suspend/resume ?
It helps with three things, (async) suspend/resume, runtime PM and
shutdown (that last part is the hardest to figure out). The ordering
in which all of these are carried out is analogous and cannot be
determined correctly by the device registration ordering itself in
general (which has been a known fact for years, but some localized
workarounds were put in some places to work around that).
Thanks for the clarification, this is due to the implicit sort you
had explained (and I provided notes for on ksummit-discuss) right?

Can you itemize a few of the workarounds that are used today?
As you clarify below, getting this order of device registration
correct does not necessarily guarantee devices will wait for their
provider to be ready.
Post by Rafael J. Wysocki
Moreover, even if the list ordering (of dpm_list, for instance) is
correct, it still doesn't guarantee the right ordering of actions that
are carried out asynchronously. They are all started in the list
order, but they may be running in parallel with each other and
complete at different times. For this reason, there needs to be a way
to ensure that, say, the suspend operations for consumer devices
complete before their suppliers will become unavailable and so on.
Thanks this helps as well!
Post by Rafael J. Wysocki
Both runtime PM and system suspend/resume have this problem. It is
not present in the system shutdown case, but it still helps to get a
correct list ordering (ie. such that won't cause supplier devices to
be shut down before their consumers) in this case too.
Is the fact that its not on shutdown just because we don't care about
being sloppy about shutdown ? Shouldn't some devices care about that
order ?

Luis

Tobias Jakobi
2016-10-05 22:42:05 UTC
Permalink
Hello Marek,

I have applied the new version onto 4.8.0 but I'm seeing this Oops on
shutdown/reboot. However it only shows up with my non-debug config.
[ 897.046373] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
[ 897.046652] Modules linked in: bridge stp llc bnep btrfs xor xor_neon zlib_inflate zlib_deflate raid6_pq s5p_mfc extcon_odroid_usbotg btusb btbcm btintel s5p_jpeg videobuf2_dma_contig v4l2_mem2mem videobuf2_memops videobuf2_v4l2 videobuf2_core
[ 897.068174] CPU: 2 PID: 9437 Comm: reboot Not tainted 4.8.0-vanilla+ #2
[ 897.074772] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 897.080848] task: eea02100 task.stack: cb488000
[ 897.085363] PC is at 0xb886c672
[ 897.088489] LR is at device_shutdown+0x134/0x1c0
[ 897.093087] pc : [<b886c672>] lr : [<c0405b14>] psr: 600a0073
[ 897.093087] sp : cb489e28 ip : 00000000 fp : cb489e4c
[ 897.104547] r10: 00000000 r9 : ee0cfe44 r8 : c0a39020
[ 897.109752] r7 : c0a6d774 r6 : ee0cfe10 r5 : ee08b810 r4 : ee0cfe1c
[ 897.116262] r3 : b886c673 r2 : 00000000 r1 : 00000002 r0 : ee0cfe10
[ 897.122773] Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA Thumb Segment none
[ 897.130151] Control: 10c5387d Table: 6a24404a DAC: 00000051
[ 897.135879] Process reboot (pid: 9437, stack limit = 0xcb488218)
[ 897.141868] Stack: (0xcb489e28 to 0xcb48a000)
[ 897.146209] 9e20: 00000000 00000000 01234567 c0a09d4c 00000010 c0a09a88
[ 897.154371] 9e40: cb489e5c cb489e50 c013eb5c c04059ec cb489e74 cb489e60 c013ec38 c013eb2c
[ 897.162530] 9e60: 01234567 c0a02448 cb489fa4 cb489e78 c013ef08 c013ec30 eb87ca80 00c0d000
[ 897.170698] 9e80: cb489ea4 cb489e90 c01c9660 c01c9434 ef026000 00000000 cb489eb4 cb489ea8
[ 897.178855] 9ea0: c01c977c c01c961c cb489ec4 cb489eb8 c01c9798 c01c9760 cb489f3c cb489ec8
[ 897.187014] 9ec0: c01eeaa4 c01c978c eb87ca80 00000000 ec421210 00c0d000 ed8af3c0 eefd5140
[ 897.195173] 9ee0: ee84a140 eea11e40 cb489f14 cb489ef8 c022bc58 c016cfa0 ed8afa80 ee665c38
[ 897.203333] 9f00: 00000000 eea11e50 cb489f24 cb489f18 c022be1c c022bc20 cb489f5c cb489f28
[ 897.211492] 9f20: c020ce88 c022bdfc 00000020 00000000 cb489f54 eea024f8 c0a39f7c 00000000
[ 897.219651] 9f40: eea02100 c0107ee4 cb488000 00000000 cb489f6c cb489f60 c020cf80 c020cd5c
[ 897.227810] 9f60: cb489f8c cb489f70 c013aa40 c06dd2dc cb488010 c0107ee4 cb489fb0 00040800
[ 897.235969] 9f80: 00000001 be966ce0 00bec008 00000058 c0107ee4 cb488000 00000000 cb489fa8
[ 897.244129] 9fa0: c0107d20 c013ee04 00000001 be966ce0 fee1dead 28121969 01234567 00000010
[ 897.252287] 9fc0: 00000001 be966ce0 00bec008 00000058 00000000 000230e4 00000000 00000000
[ 897.260447] 9fe0: 00023030 be966cc4 00010e9c b6f3f290 200a0050 fee1dead 00000000 00000000
[ 897.271033] [<c04059e0>] (device_shutdown) from [<c013eb5c>] (kernel_restart_prepare+0x3c/0x40)
[ 897.279715] r9:c0a09a88 r8:00000010 r7:c0a09d4c r6:01234567 r5:00000000 r4:00000000
[ 897.287439] [<c013eb20>] (kernel_restart_prepare) from [<c013ec38>] (kernel_restart+0x14/0x58)
[ 897.296036] [<c013ec24>] (kernel_restart) from [<c013ef08>] (SyS_reboot+0x110/0x1f8)
[ 897.303757] r4:c0a02448 r3:01234567
[ 897.307314] [<c013edf8>] (SyS_reboot) from [<c0107d20>] (ret_fast_syscall+0x0/0x3c)
[ 897.314955] r9:cb488000 r8:c0107ee4 r7:00000058 r6:00bec008 r5:be966ce0 r4:00000001
[ 897.322680] Code: bad PC value
[ 897.325940] ---[ end trace 861fd282a7bdc01e ]---
The corresponding config:
https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.8.conf


With best wishes,
Tobias
Hello,
This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce 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 situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).
Patch requires fourth version of Rafeal's "Functional dependencies
If one wants to test this patchset, I've provided a branch with all needed
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v4
Patches are based on vanilla v4.8-rc8 kernel with Rafael's device
dependencies v4 patchset applied.
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
- 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 developped
by Rafael J. Wysocki
v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version
iommu/exynos: Remove excessive, useless debug
iommu/exynos: Add proper runtime pm support
drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 134 deletions(-)
Tobias Jakobi
2016-10-06 16:28:30 UTC
Permalink
Hello again,
Post by Tobias Jakobi
Hello Marek,
I have applied the new version onto 4.8.0 but I'm seeing this Oops on
shutdown/reboot. However it only shows up with my non-debug config.
sorry for the false alarm. That Oops on reboot was due to some other
patch I had applied.


With best wishes,
Tobias
Post by Tobias Jakobi
[ 897.046373] Internal error: Oops: 80000005 [#1] PREEMPT SMP ARM
[ 897.046652] Modules linked in: bridge stp llc bnep btrfs xor xor_neon zlib_inflate zlib_deflate raid6_pq s5p_mfc extcon_odroid_usbotg btusb btbcm btintel s5p_jpeg videobuf2_dma_contig v4l2_mem2mem videobuf2_memops videobuf2_v4l2 videobuf2_core
[ 897.068174] CPU: 2 PID: 9437 Comm: reboot Not tainted 4.8.0-vanilla+ #2
[ 897.074772] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 897.080848] task: eea02100 task.stack: cb488000
[ 897.085363] PC is at 0xb886c672
[ 897.088489] LR is at device_shutdown+0x134/0x1c0
[ 897.093087] pc : [<b886c672>] lr : [<c0405b14>] psr: 600a0073
[ 897.093087] sp : cb489e28 ip : 00000000 fp : cb489e4c
[ 897.104547] r10: 00000000 r9 : ee0cfe44 r8 : c0a39020
[ 897.109752] r7 : c0a6d774 r6 : ee0cfe10 r5 : ee08b810 r4 : ee0cfe1c
[ 897.116262] r3 : b886c673 r2 : 00000000 r1 : 00000002 r0 : ee0cfe10
[ 897.122773] Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA Thumb Segment none
[ 897.130151] Control: 10c5387d Table: 6a24404a DAC: 00000051
[ 897.135879] Process reboot (pid: 9437, stack limit = 0xcb488218)
[ 897.141868] Stack: (0xcb489e28 to 0xcb48a000)
[ 897.146209] 9e20: 00000000 00000000 01234567 c0a09d4c 00000010 c0a09a88
[ 897.154371] 9e40: cb489e5c cb489e50 c013eb5c c04059ec cb489e74 cb489e60 c013ec38 c013eb2c
[ 897.162530] 9e60: 01234567 c0a02448 cb489fa4 cb489e78 c013ef08 c013ec30 eb87ca80 00c0d000
[ 897.170698] 9e80: cb489ea4 cb489e90 c01c9660 c01c9434 ef026000 00000000 cb489eb4 cb489ea8
[ 897.178855] 9ea0: c01c977c c01c961c cb489ec4 cb489eb8 c01c9798 c01c9760 cb489f3c cb489ec8
[ 897.187014] 9ec0: c01eeaa4 c01c978c eb87ca80 00000000 ec421210 00c0d000 ed8af3c0 eefd5140
[ 897.195173] 9ee0: ee84a140 eea11e40 cb489f14 cb489ef8 c022bc58 c016cfa0 ed8afa80 ee665c38
[ 897.203333] 9f00: 00000000 eea11e50 cb489f24 cb489f18 c022be1c c022bc20 cb489f5c cb489f28
[ 897.211492] 9f20: c020ce88 c022bdfc 00000020 00000000 cb489f54 eea024f8 c0a39f7c 00000000
[ 897.219651] 9f40: eea02100 c0107ee4 cb488000 00000000 cb489f6c cb489f60 c020cf80 c020cd5c
[ 897.227810] 9f60: cb489f8c cb489f70 c013aa40 c06dd2dc cb488010 c0107ee4 cb489fb0 00040800
[ 897.235969] 9f80: 00000001 be966ce0 00bec008 00000058 c0107ee4 cb488000 00000000 cb489fa8
[ 897.244129] 9fa0: c0107d20 c013ee04 00000001 be966ce0 fee1dead 28121969 01234567 00000010
[ 897.252287] 9fc0: 00000001 be966ce0 00bec008 00000058 00000000 000230e4 00000000 00000000
[ 897.260447] 9fe0: 00023030 be966cc4 00010e9c b6f3f290 200a0050 fee1dead 00000000 00000000
[ 897.271033] [<c04059e0>] (device_shutdown) from [<c013eb5c>] (kernel_restart_prepare+0x3c/0x40)
[ 897.279715] r9:c0a09a88 r8:00000010 r7:c0a09d4c r6:01234567 r5:00000000 r4:00000000
[ 897.287439] [<c013eb20>] (kernel_restart_prepare) from [<c013ec38>] (kernel_restart+0x14/0x58)
[ 897.296036] [<c013ec24>] (kernel_restart) from [<c013ef08>] (SyS_reboot+0x110/0x1f8)
[ 897.303757] r4:c0a02448 r3:01234567
[ 897.307314] [<c013edf8>] (SyS_reboot) from [<c0107d20>] (ret_fast_syscall+0x0/0x3c)
[ 897.314955] r9:cb488000 r8:c0107ee4 r7:00000058 r6:00bec008 r5:be966ce0 r4:00000001
[ 897.322680] Code: bad PC value
[ 897.325940] ---[ end trace 861fd282a7bdc01e ]---
https://github.com/tobiasjakobi/odroid-environment/blob/master/sourcecode/system/vanilla-4.8.conf
With best wishes,
Tobias
Hello,
This patch series finally implements proper runtime PM support in Exynos
IOMMU driver. This has been achieved by using recently introduce 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 situation that power domains are always enabled,
because all SYSMMU controllers (which belongs to those domains) are
permanently active (because existing driver was simplified and kept
SYSMMU device active all the time after initialization).
Patch requires fourth version of Rafeal's "Functional dependencies
If one wants to test this patchset, I've provided a branch with all needed
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git v4.8-iommu-pm-v4
Patches are based on vanilla v4.8-rc8 kernel with Rafael's device
dependencies v4 patchset applied.
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
- 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 developped
by Rafael J. Wysocki
v1: http://www.spinics.net/lists/arm-kernel/msg509600.html
- initial version
iommu/exynos: Remove excessive, useless debug
iommu/exynos: Add proper runtime pm support
drivers/iommu/exynos-iommu.c | 228 ++++++++++++++++++-------------------------
1 file changed, 94 insertions(+), 134 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...