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

[Luis R. Rodriguez]

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

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg