Re: [PATCH v2] OMAPDSS: HACK: Ensure DSS clock domain gets out of idle when HDMI is enabled

[Archit Taneja <a0393947@ti.com>]

On Tuesday 14 February 2012 09:11 PM, Cousson, Benoit wrote:
> On 2/14/2012 2:30 PM, Archit Taneja wrote:
>> Hi,
>>
>> On Tuesday 14 February 2012 06:45 PM, Tomi Valkeinen wrote:
>>> On Tue, 2012-02-14 at 13:58 +0100, Cousson, Benoit wrote:
>>>> Hi Tomi,
>>>
>>>>> Benoit, do you think we'll get the MODULEMODE mess cleaned up in the
>>>>> hwmod/clk framework at some point, and the drivers could do without
>>>>> these kinds of hacks? =)
>>>>
>>>> The best way to fix that for my point of view is to go to device tree
>>>> or/and to consider the DSS as the parent of all the DSS modules.
>>>> pm_runtime will then always ensure that the parent is enabled before
>>>> any
>>>> of the child are used.
>>>
>>> Ah, right. Sounds fine to me.
>>>
>>> But is that a proper "fix"? Are we sure the MODULEMODE will then always
>>> be handled correctly? Isn't the core problem still there, it just
>>> doesn't happen with the setup anymore?
>>>
>>> I mean, if we have these special requirements regarding MODULEMODE, and
>>> the code doesn't really know about it, would it get broken easily with
>>> restructuring/changes?
>>>
>>> And no, I don't have any clear idea why/how it would break, but I have
>>> just gotten the impression that the MODULEMODE is not handled quite
>>> properly (and so we have these current problems), and having dss_core as
>>> the parent of other dss modules doesn't really fix that in any way.
>>
>> I agree with that.
>>
>> In the current approach, we have multiple platform devices for DSS, and
>> all of them belong to the same clock domain, and the clock domain has
>> just one MODULEMODE bit field.
>>
>> When shutting off a platform device(by calling pm_runtime_put()), hwmod
>> enables/disables MODULEMODE without taking into mind that other active
>> platform devices may still need it. So, for example, if we have 2
>> platform devices, say dss and dispc, and we have code like:
>>
>> dispc_foo()
>> {
>> pm_runtime_get(dispc_pdev);
>> ...
>> ...
>> pm_runtime_put(dispc_pdev);
>> }
>>
>> dss_foo()
>> {
>> pm_runtime_get(dss_pdev);
>> ...
>> ...
>> dispc_foo(); /* MODULEMODE off after this */
>> ...
>> ...
>> pm_runtime_put(dss_pdev);
>> }
>>
>> This will lead to the situation of one platform device disabling
>> MODULEMODE even though other platform devices need it.
>>
>> This may not be resolved in device tree either. We would need to have
>> some use count mechanism for these bits, or attach MODULEMODE only to
>> one platform device, and don't give others control to enable/disable it.
>
> And this is exactly what the pm_runtime will provide. The fmwk already
> handles reference counting.
> Moreover the dev->parent will increment the power.child_count and thus
> ensure that the parent is always enabled if at least one child is active.
>
> By initializing the dev->parent of each DSS modules to the dss_core, it
> will ensure that the power dependency is managed properly.

Yes, a parent/child relation will ensure the required dependencies. It 
sounds good!

How do we take care of things in the meanwhile? With the current way of 
representing modulemode as a slave clock, the disabling sequence gets 
messed up. In arch/arm/mach-omap2/omap_hwmod.c:

static int _disable_clocks(struct omap_hwmod *oh)
{
	...
	disable mainclk;
	disable slave clocks;
	...
}

For DSS, mainclk is the DSS_FCLK opt clock, and slave clock is 
modulemode bits. We need the opposite sequence to happen here to cleanly 
disable DSS.

Any suggestions?

Thanks,
Archit

>
> Regards,
> Benoit
>
>