Re: [PATCH v7 13/14] iommu/rockchip: Add runtime PM support

[Tomasz Figa]

On Tue, Mar 6, 2018 at 7:07 PM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> Hi Jeffy,
>
> It looks like I missed some details of how runtime PM enable works
> before, so we might be able to simplify things. Sorry for not getting
> things right earlier.
>
> On Tue, Mar 6, 2018 at 12:27 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.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
>> ---
>>
>> Changes in v7:
>> Add WARN_ON in irq isr, and modify iommu archdata comment.
>>
>> Changes in v6: None
>> Changes in v5:
>> Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled().
>>
>> Changes in v4: None
>> Changes in v3:
>> Only call startup() and shutdown() when iommu attached.
>> Remove pm_mutex.
>> Check runtime PM disabled.
>> Check pm_runtime in rk_iommu_irq().
>>
>> Changes in v2: None
>>
>>  drivers/iommu/rockchip-iommu.c | 189 ++++++++++++++++++++++++++++++++---------
>>  1 file changed, 148 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
>> index 2448a0528e39..db08978203f7 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/of_iommu.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>>
>> @@ -105,7 +106,14 @@ struct rk_iommu {
>>         struct iommu_domain *domain; /* domain to which iommu is attached */
>>  };
>>
>> +/**
>> + * struct rk_iommudata - iommu archdata of master device.
>> + * @link:      device link with runtime PM integration from the master
>> + *             (consumer) to the IOMMU (supplier).
>> + * @iommu:     IOMMU of the master device.
>> + */
>>  struct rk_iommudata {
>> +       struct device_link *link;
>>         struct rk_iommu *iommu;
>>  };
>>
>> @@ -518,7 +526,13 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
>>         u32 int_status;
>>         dma_addr_t iova;
>>         irqreturn_t ret = IRQ_NONE;
>> -       int i;
>> +       bool need_runtime_put;
>> +       int i, err;
>> +
>> +       err = pm_runtime_get_if_in_use(iommu->dev);
>> +       if (WARN_ON(err <= 0 && err != -EINVAL))
>> +               return ret;
>> +       need_runtime_put = err > 0;
>
> Actually, for our purposes, we can assume that runtime PM enable
> status can be only changed by the driver itself. Looking at the LXR,
> PM core also calls __pm_runtime_disable() before calling
> .suspend_late() callback and pm_runtime_enable() after calling
> .resume_early() callback, but we should be able to ignore this,
> because we handle things in .suspend() callback in this driver.
>
> With this assumption in mind, all we need to do here is:
>
> if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
>     return 0;
>
>>
>>         WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
>>
>> @@ -570,6 +584,9 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
>>
>>         clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>>
>> +       if (need_runtime_put)
>> +               pm_runtime_put(iommu->dev);
>
> if (pm_runtime_enabled())
>         pm_runtime_put(iommu->dev);

Actually, we don't even need this pm_runtime_enabled() check and can
always call pm_runtime_put(), because at this point we would be only
in either of cases:
1) runtime PM compiled in and enabled, so we got the enable count and
need to put it,
2) runtime PM not compiled in, so pm_runtime_put() is a no-op.

>
>> +
>>         return ret;
>>  }
>>
>> @@ -611,10 +628,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain *rk_domain,
>>         spin_lock_irqsave(&rk_domain->iommus_lock, flags);
>>         list_for_each(pos, &rk_domain->iommus) {
>>                 struct rk_iommu *iommu;
>> +               int ret;
>> +
>>                 iommu = list_entry(pos, struct rk_iommu, node);
>> -               WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
>> -               rk_iommu_zap_lines(iommu, iova, size);
>> -               clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>> +
>> +               /* Only zap TLBs of IOMMUs that are powered on. */
>> +               ret = pm_runtime_get_if_in_use(iommu->dev);
>> +               if (ret > 0 || ret == -EINVAL) {
>
> if (pm_runtime_get_if_in_use(iommu->dev)) {
>
>> +                       WARN_ON(clk_bulk_enable(iommu->num_clocks,
>> +                                               iommu->clocks));
>> +                       rk_iommu_zap_lines(iommu, iova, size);
>> +                       clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>
>         if (pm_runtime_enabled(iommu->dev))
>                 pm_runtime_put(iommu->dev);

Same here.

Best regards,
Tomasz