Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman <khilman@ti.com>]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin,
>
> On 05/31/2012 03:42 PM, Kevin Hilman wrote:
>> Jon Hunter <jon-hunter@ti.com> writes:
>> 
>>> Hi Kevin, Will,
>>>
>>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>>> Hi Kevin,
>>>>
>>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>>> Basically, I don't like the result when we have to hack around missing
>>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>>
>>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>>> with runtime PM get/put, and the functionality in those functions would
>>>>> be the runtime PM callbacks instead.
>>>>>
>>>>> Will, any objections to armpmu growing runtime PM support?
>>>>
>>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>>> that we currently have and push those function pointers into the arm_pmu,
>>>> so that fits with what you'd like.
>>>>
>>>> The only concern I have is that we need the mutual exclusion even when we
>>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>>> the approach.
>>>
>>> To add a bit more food for thought, I had implemented a quick patch to
>>> add runtime PM support for PMU. You will notice that I have been
>>> conservative on where I have placed the pm_runtime_get/put calls,
>>> because I am not too familiar with the PMU driver to know exactly
>>> where we need to maintain the PMU context. So right now these are just
>>> around the reserve_hardware/release_hardware calls. This works on OMAP
>>> for some quick testing. However, I would need to make sure this does
>>> not break compilation without runtime PM enabled.
>>>
>>> Let me know your thoughts.
>> 
>> That looks good, but I'm curious what would be done in the new
>> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
>> driver needs to be moved into the runtime PM hooks?
>
> For omap4, the plat->runtime_* hooks look like ...
>
> +static int omap4_pmu_runtime_resume(struct device *dev)
> +{
> +       /* configure CTI0 for PMU IRQ routing */
> +       cti_unlock(&omap4_cti[0]);
> +       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
> +       cti_enable(&omap4_cti[0]);
> +
> +       /* configure CTI1 for PMU IRQ routing */
> +       cti_unlock(&omap4_cti[1]);
> +       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
> +       cti_enable(&omap4_cti[1]);
> +
> +       return 0;
> +}
> +
> +static int omap4_pmu_runtime_suspend(struct device *dev)
> +{
> +       cti_disable(&omap4_cti[0]);
> +       cti_disable(&omap4_cti[1]);
> +
> +       return 0;
> +}
>
> This is what I have implemented so far and currently testing. So really
> just using the hooks to configure the cross triggering interface.
>
> Is this what you were thinking?
>

Basically, yes.  

But since I haven't studied the PMU driver closely, I have some dumb
questions.  My concern is that these look bsically like the
plat->irq_[enable|disable] hooks, so I guess the root of my question is
do we need both the irq enable/disable and runtime suspend/resume hooks
in plat?  or can we get by with one set.

Kevin