Re: [PATCH v2 12/13] iommu/rockchip: Add runtime PM support

[Tomasz Figa]

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx> wrote:
> When the power domain is powered off, the IOMMU cannot be accessed and
> register programming must be deferred until the power domain becomes
> enabled.
>
> Add runtime PM support, and use runtime PM device link from IOMMU to
> master to startup and shutdown IOMMU.
[snip]
> @@ -875,28 +889,19 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
>
>  static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
>  {
> -       return dev->archdata.iommu;
> +       struct rk_iommudata *data = dev->archdata.iommu;
> +
> +       return data ? data->iommu : NULL;
>  }

Is this change intentionally added to this patch? I see this
potentially relevant for the previous patch in this series.

[snip]
> +static int rk_iommu_startup(struct rk_iommu *iommu)
>  {
> -       struct rk_iommu *iommu;
> +       struct iommu_domain *domain = iommu->domain;
>         struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
> -       unsigned long flags;
>         int ret, i;
>
> -       /*
> -        * Allow 'virtual devices' (e.g., drm) to attach to domain.
> -        * Such a device does not belong to an iommu group.
> -        */
> -       iommu = rk_iommu_from_dev(dev);
> -       if (!iommu)
> -               return 0;
> -
> -       if (iommu->domain)
> -               rk_iommu_detach_device(iommu->domain, dev);
> -
>         ret = rk_iommu_enable_clocks(iommu);
>         if (ret)
>                 return ret;
>

Don't we need to check here (and in _shutdown() too) if we have a
domain attached?

> +       mutex_lock(&iommu->pm_mutex);
>         ret = rk_iommu_enable_stall(iommu);
>         if (ret)
> -               goto err_disable_clocks;
> +               goto err_unlock_mutex;
[snip]
> +       iommu->domain = NULL;
> +
> +       spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> +       list_del_init(&iommu->node);
> +       spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> +
> +       if (pm_runtime_get_if_in_use(iommu->dev) > 0) {

Actually, if the above call returns -EINVAL, don't we still need to
call rk_iommu_shutdown(), because it just means runtime PM is disabled
and the IOMMU is always powered on?

> +               rk_iommu_shutdown(iommu);
> +               pm_runtime_put(iommu->dev);
> +       }
> +}
[snip]
> +       spin_lock_irqsave(&rk_domain->iommus_lock, flags);
> +       list_add_tail(&iommu->node, &rk_domain->iommus);
> +       spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
> +
> +       if (pm_runtime_get_if_in_use(iommu->dev) <= 0)

Ditto.

> +               return 0;
> +
> +       ret = rk_iommu_startup(iommu);
> +       if (ret)
> +               rk_iommu_detach_device(data->domain, dev);
[snip]
> @@ -1108,7 +1175,9 @@ static int rk_iommu_of_xlate(struct device *dev,
>                 return -ENODEV;
>         }
>
> -       dev->archdata.iommu = platform_get_drvdata(iommu_dev);
> +       data->iommu = platform_get_drvdata(iommu_dev);
> +       dev->archdata.iommu = data;
> +

I think this change might be mistakenly squashed to this patch instead
of previous.

Best regards,
Tomasz