diff mbox

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

Message ID 1476948173-21093-8-git-send-email-m.szyprowski@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Marek Szyprowski Oct. 20, 2016, 7:22 a.m. UTC
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(-)

Comments

Sricharan Ramabadhran Oct. 23, 2016, 9:49 a.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Oct. 24, 2016, 5:30 a.m. UTC | #2
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
Sricharan Ramabadhran Oct. 24, 2016, 12:29 p.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Oct. 24, 2016, 12:39 p.m. UTC | #4
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
Sricharan Ramabadhran Oct. 25, 2016, 6:53 a.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain Nov. 7, 2016, 9:47 p.m. UTC | #6
On Thu, Oct 20, 2016 at 09:22:53AM +0200, Marek Szyprowski wrote:
> This patch uses recently introduced device dependency links to track the
> runtime pm state of the master's device. This way each SYSMMU controller
> is set to runtime active only when its master's device is active and can
> restore or save its state instead of being activated all the time when
> attached to the given master device. This way SYSMMU controllers no longer
> prevents respective power domains to be turned off when master's device
> is not being used.

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

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

  Luis

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


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

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

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

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

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

Nope, none of that solution deals with runtime pm.

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

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

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

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


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

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

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

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

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

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

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

Thanks,

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

Rafael note:

The one he asked here.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner Nov. 16, 2016, 9:30 a.m. UTC | #14
On Thu, Nov 10, 2016 at 12:56:14AM +0100, Rafael J. Wysocki wrote:
> The idea, roughly, is that if there is a single on/off switch acting
> on multiple devices, you can (a) set up a PM domain tracking all of
> those device's runtime PM invocations and (b) maintaining a reference
> counter of devices still not suspended.  This way it would only turn
> the switch off when all of the devices in question had been suspended.
> Analogously, it would turn the switch on before resuming the first
> device in the domain.  Of course, that code isn't available as a
> library, you would need to implement it (or use genpd, but chances are
> it is too heavy weight for the job).

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

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

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

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

Thanks,

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

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

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

Prototype of device_link_add:

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

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

Best regards,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Nov. 21, 2016, 1:11 p.m. UTC | #16
Hi Lukas,


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

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

Best regards
diff mbox

Patch

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;
 }