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
>>> ---
>>> 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
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
---
drivers/iommu/exynos-iommu.c | 45 +++-
1 file changed, 36 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index a959443e6f33..5e6d7bbf9b70 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -206,6 +206,7 @@ struct sysmmu_fault_info {
struct exynos_iommu_owner {
struct list_head controllers; /* list of sysmmu_drvdata.owner_node */
struct iommu_domain *domain;/* domain this device is attached */
+ struct mutex rpm_lock; /* for runtime pm of all sysmmus */
};
/*
@@ -594,40 +595,46 @@ static int __init exynos_sysmmu_probe(struct
platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM_SLEEP
-static int exynos_sysmmu_suspend(struct device *dev)
+static int __maybe_unused exynos_sysmmu_suspend(struct device *dev)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
struct device *master = data->master;
if (master) {
- pm_runtime_put(dev);
+ struct exynos_iommu_owner *owner = master->archdata.iommu;
+
+ mutex_lock(&owner->rpm_lock);
More of a device link question,
To understand, i see that with device link + runtime, the supplier
callbacks are not called for irqsafe clients, even if supplier is irqsafe.
Why so ?
Frankly I didn't care about irqsafe runtime pm, because there is no such
need
for Exynos platform and its drivers. Exynos power domain driver also doesn't
support irqsafe mode.
if (data->domain) {
dev_dbg(data->sysmmu, "saving state\n");
__sysmmu_disable(data);
}
+ mutex_unlock(&owner->rpm_lock);
}
return 0;
}
-static int exynos_sysmmu_resume(struct device *dev)
+static int __maybe_unused exynos_sysmmu_resume(struct device *dev)
{
struct sysmmu_drvdata *data = dev_get_drvdata(dev);
struct device *master = data->master;
if (master) {
- pm_runtime_get_sync(dev);
+ struct exynos_iommu_owner *owner = master->archdata.iommu;
+
+ mutex_lock(&owner->rpm_lock);
if (data->domain) {
dev_dbg(data->sysmmu, "restoring state\n");
__sysmmu_enable(data);
}
+ mutex_unlock(&owner->rpm_lock);
}
return 0;
}
-#endif
static const struct dev_pm_ops sysmmu_pm_ops = {
- SET_LATE_SYSTEM_SLEEP_PM_OPS(exynos_sysmmu_suspend,
exynos_sysmmu_resume)
+ SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+pm_runtime_force_resume)
};
Is this needed to be LATE_SYSTEM_SLEEP_PM_OPS with device links to take care
of the order ?
Hmmm. LASE_SYSTEM_SLEEP_PM_OPS is a left over from the previous versions
of the driver,
which doesn't use device links. You are right, that "normal"
SYSTEM_SLEEP_PM_OPS should
be enough assuming that device links will take care of the proper call
sequence between
consumer and supplier device.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
RE: [PATCH v5 6/7] iommu/exynos: Add runtime pm support
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
>---
> 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
