Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm

[Ohad Ben-Cohen]

Hi Omar,

On Wed, Nov 14, 2012 at 4:34 AM, Omar Ramirez Luna <omar.luna@xxxxxxxxxx> wrote:
> Use runtime PM functionality interfaced with hwmod enable/idle
> functions, to replace direct clock operations and sysconfig
> handling.
>
> Dues to reset sequence, pm_runtime_put_sync must be used, to avoid
> possible operations with the module under reset.

There are some changes here that might not be trivial to understand in
hindsight; any chance you can add more explanations (even only in the
commit log) regarding:

> @@ -160,11 +160,10 @@ static int iommu_enable(struct omap_iommu *obj)
...
> -       clk_enable(obj->clk);
> +       pm_runtime_get_sync(obj->dev);
>
>         err = arch_iommu->enable(obj);
>
> -       clk_disable(obj->clk);
>         return err;
>  }

Why do we remove clk_disable here (instead of replacing it with a _put
variant) ?

> @@ -306,7 +303,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>         if (!obj || !obj->nr_tlb_entries || !e)
>                 return -EINVAL;
>
> -       clk_enable(obj->clk);
> +       pm_runtime_get_sync(obj->dev);

If iommu_enable no longer disables obj->clk before returning, do we
really need to call ->get here (and in all the other similar
instances) ?

> @@ -816,9 +813,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
>         if (!obj->refcount)
>                 return IRQ_NONE;
>
> -       clk_enable(obj->clk);
>         errs = iommu_report_fault(obj, &da);
> -       clk_disable(obj->clk);

Why do we remove the clk_ invocations here (instead of replacing them
with get/put variants) ?

Most of the above questions imply this patch not only converts the
iommu to runtime PM, but may carry additional changes that may imply
previous implementation is sub-optimal. I hope we can clearly document
the motivation behind these changes too (maybe even consider
extracting them to a different patch ?).

> @@ -990,6 +981,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>                 goto err_irq;
>         platform_set_drvdata(pdev, obj);
>
> +       pm_runtime_irq_safe(obj->dev);

Let's also document why _irq_safe is needed here ?

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/