Date:	Thu, 15 Nov 2012 10:53:45 -0600
From:	Omar Ramirez Luna <omar.luna@...aro.org>
To:	Ohad Ben-Cohen <ohad@...ery.com>
Cc:	Tony Lindgren <tony@...mide.com>, Joerg Roedel <joro@...tes.org>,
	Russell King <linux@....linux.org.uk>,
	Ido Yariv <ido@...ery.com>,
	Mauro Carvalho Chehab <mchehab@...hat.com>,
	Paul Walmsley <paul@...an.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	linux-arm <linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	Felipe Contreras <felipe.contreras@...il.com>
Subject: Re: [PATCH v4 2/2] ARM: OMAP3/4: iommu: adapt to runtime pm

Hi Ohad,

On 14 November 2012 03:54, Ohad Ben-Cohen <ohad@...ery.com> wrote:
> Hi Omar,
>
> On Wed, Nov 14, 2012 at 4:34 AM, Omar Ramirez Luna <omar.luna@...aro.org> 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:

I have discussed exactly the same changes in the list with Felipe, but
yes I did forget to add the explanations (I thought I did in some
version of the patch or cover-letter), but will update this
description.

Below is the discussion just in case, I'll be replying to your
comments anyway ;)

https://patchwork.kernel.org/patch/1585741/

>> @@ -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) ?

Basically, with the previous clk management, the iommu driver assumes
that its clock is shared with its client, which is the case for ipu
and dsp, but I don't like that assumption. So by doing
clock_enable/disable, the functional clock required for translations
it is indirectly provided by the user of the iommu (let's say ipu).
E.g. IPU enables the iommu and maps, at the end of the maps the clock
will be disabled, but given that ipu clock is the same the mmu stays
functional.

By changing this to get_sync only, the mmu stays enabled as long as
the iommu has been requested (except for the power transitions).

>> @@ -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) ?

"You can access this paths through debugfs, some of them doesn't work
if the module is not enabled first, but in future if you just want to
idle the iommu without freeing, these are needed to debug."

>
>> @@ -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) ?

Because in order to get an interrupt from the mmu device it implies
that the mmu was functional already (with a clock), so I don't see how
clk_enable/disable are needed here. Even if you rely on the IPU to
maintain the clock enabled.

> 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 ?).

I didn't want to extract this changes into different patches since
they could be included in this one, otherwise it would look like lines
adding and then deleting runtime pm functions.

I do agree description is missing, again I thought I had done this
somewhere but looks like I didn't, will update these. If you think
these should be different patches please let me know, otherwise I
would like to keep a single *documented* 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 ?

Ok.

Thanks for the comments,

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

Powered by blists - more mailing lists