Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1504576 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 0/7] Exynos IOMMU: proper runtime PM support (use device  dependencies) Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200
    [PATCH v5 1/7] iommu/exynos: Remove excessive, useless debug Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200
    [PATCH v5 6/7] iommu/exynos: Add runtime pm support Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200
      RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support "Sricharan" <sricharan@codeaurora.org> - 2016-10-22 08:00 +0200
        Re: [PATCH v5 6/7] iommu/exynos: Add runtime pm support Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-24 07:30 +0200
          RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support "Sricharan" <sricharan@codeaurora.org> - 2016-10-24 14:20 +0200
    [PATCH v5 3/7] iommu/exynos: Simplify internal enable/disable functions Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200
    [PATCH v5 7/7] iommu/exynos: Use device dependency links to control  runtime pm Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200
      RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control        runtime pm "Sricharan" <sricharan@codeaurora.org> - 2016-10-23 12:00 +0200
        Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to  control runtime pm Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-24 07:40 +0200
          RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm "Sricharan" <sricharan@codeaurora.org> - 2016-10-24 14:30 +0200
            Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to  control runtime pm Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-24 14:40 +0200
              RE: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm "Sricharan" <sricharan@codeaurora.org> - 2016-10-25 09:00 +0200
    [PATCH v5 2/7] iommu/exynos: Remove dead code Marek Szyprowski <m.szyprowski@samsung.com> - 2016-10-20 09:30 +0200

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

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

[toc] | [next] | [standalone]

#1504577 — [PATCH v5 1/7] iommu/exynos: Remove excessive, useless debug

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

__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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web