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

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

[Archit Taneja]

For DSS clock domain to transition from idle to active state. It's necessary
to enable the optional clock DSS_FCLK before we enable the module using the
MODULEMODE bits in the clock domain's CM_DSS_DSS_CLKCTRL register.

This sequence was not followed correctly for the 'dss_hdmi' hwmod and it led
to DSS clock domain not getting out of idle when pm_runtime_get_sync() was
called for hdmi's platform device.

Since the clock domain failed to change it's state to active, the hwmod code
disables any clocks it had enabled before for this hwmod. This led to the clock
'dss_48mhz_clk' gettind disabled.

When hdmi's runtime_resume() op is called, the call to dss_runtime_get()
correctly enables the DSS clock domain this time. However, the clock
'dss_48mhz_clk' is needed for HDMI's PHY to function correctly. Since it was
disabled previously, the driver fails when it tries to enable HDMI's PHY.

Fix this for now by ensuring that dss_runtime_get() is called before we call
pm_runtime_get_sync() for hdmi's platform device. A correct fix for later would
be to modify the DSS related hwmod's mainclks, and also some changes in how
opt clocks are handled in the DSS driver.

This fixes the issue of HDMI not working when it's the default display. The
issue is not seen if any other display is already enabled as the first display
would have correctly enabled the DSS clockdomain.

Signed-off-by: Archit Taneja <archit@ti.com>
---
Changes in v2:
- Rewrite an unclear paragraph in the commit message
- Add comments mentioning that this is a hack

 drivers/video/omap2/dss/hdmi.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index d7aa3b0..a36b934 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -165,9 +165,25 @@ static int hdmi_runtime_get(void)
 
 	DSSDBG("hdmi_runtime_get\n");
 
+	/*
+	 * HACK: Add dss_runtime_get() to ensure DSS clock domain is enabled.
+	 * This should be removed later.
+	 */
+	r = dss_runtime_get();
+	if (r < 0)
+		goto err_get_dss;
+
 	r = pm_runtime_get_sync(&hdmi.pdev->dev);
 	WARN_ON(r < 0);
-	return r < 0 ? r : 0;
+	if (r < 0)
+		goto err_get_hdmi;
+
+	return 0;
+
+err_get_hdmi:
+	dss_runtime_put();
+err_get_dss:
+	return r;
 }
 
 static void hdmi_runtime_put(void)
@@ -178,6 +194,12 @@ static void hdmi_runtime_put(void)
 
 	r = pm_runtime_put_sync(&hdmi.pdev->dev);
 	WARN_ON(r < 0);
+
+	/*
+	 * HACK: This is added to complement the dss_runtime_get() call in
+	 * hdmi_runtime_get(). This should be removed later.
+	 */
+	dss_runtime_put();
 }
 
 int hdmi_init_display(struct omap_dss_device *dssdev)
-- 
1.7.5.4

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

Hi,

On Fri, 2012-02-10 at 11:45 +0530, Archit Taneja wrote:
> For DSS clock domain to transition from idle to active state. It's necessary
> to enable the optional clock DSS_FCLK before we enable the module using the
> MODULEMODE bits in the clock domain's CM_DSS_DSS_CLKCTRL register.
> 
> This sequence was not followed correctly for the 'dss_hdmi' hwmod and it led
> to DSS clock domain not getting out of idle when pm_runtime_get_sync() was
> called for hdmi's platform device.
> 
> Since the clock domain failed to change it's state to active, the hwmod code
> disables any clocks it had enabled before for this hwmod. This led to the clock
> 'dss_48mhz_clk' gettind disabled.
> 
> When hdmi's runtime_resume() op is called, the call to dss_runtime_get()
> correctly enables the DSS clock domain this time. However, the clock
> 'dss_48mhz_clk' is needed for HDMI's PHY to function correctly. Since it was
> disabled previously, the driver fails when it tries to enable HDMI's PHY.
> 
> Fix this for now by ensuring that dss_runtime_get() is called before we call
> pm_runtime_get_sync() for hdmi's platform device. A correct fix for later would
> be to modify the DSS related hwmod's mainclks, and also some changes in how
> opt clocks are handled in the DSS driver.
> 
> This fixes the issue of HDMI not working when it's the default display. The
> issue is not seen if any other display is already enabled as the first display
> would have correctly enabled the DSS clockdomain.

I think this looks fine, it's shouldn't have any side effects and is
easy to remove later.

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

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Cousson, Benoit]

Hi Tomi,

On 2/14/2012 12:57 PM, Tomi Valkeinen wrote:
> Hi,
>
> On Fri, 2012-02-10 at 11:45 +0530, Archit Taneja wrote:
>> For DSS clock domain to transition from idle to active state. It's necessary
>> to enable the optional clock DSS_FCLK before we enable the module using the
>> MODULEMODE bits in the clock domain's CM_DSS_DSS_CLKCTRL register.
>>
>> This sequence was not followed correctly for the 'dss_hdmi' hwmod and it led
>> to DSS clock domain not getting out of idle when pm_runtime_get_sync() was
>> called for hdmi's platform device.
>>
>> Since the clock domain failed to change it's state to active, the hwmod code
>> disables any clocks it had enabled before for this hwmod. This led to the clock
>> 'dss_48mhz_clk' gettind disabled.
>>
>> When hdmi's runtime_resume() op is called, the call to dss_runtime_get()
>> correctly enables the DSS clock domain this time. However, the clock
>> 'dss_48mhz_clk' is needed for HDMI's PHY to function correctly. Since it was
>> disabled previously, the driver fails when it tries to enable HDMI's PHY.
>>
>> Fix this for now by ensuring that dss_runtime_get() is called before we call
>> pm_runtime_get_sync() for hdmi's platform device. A correct fix for later would
>> be to modify the DSS related hwmod's mainclks, and also some changes in how
>> opt clocks are handled in the DSS driver.
>>
>> This fixes the issue of HDMI not working when it's the default display. The
>> issue is not seen if any other display is already enabled as the first display
>> would have correctly enabled the DSS clockdomain.
>
> I think this looks fine, it's shouldn't have any side effects and is
> easy to remove later.
>
> 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.

Implementing that with hwmod is not the right approach since is will 
require to add some more complex OMAP custom stuff in the hwmod fmwk 
whereas the LDM is already able to handle that.

The whole point is that going to device tree, we will have to change the 
responsibility of hwmod to make it focus mainly on OMAP PRCM stuff and 
let the device handles the IRQ/DMA/REG/CLKS resources since it is the 
correct place to do that.


Regards,
Benoit

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

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.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

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.

Archit

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2733 bytes --]

On Tue, 2012-02-14 at 19:00 +0530, 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.

Hmm, are you sure? Not that I checked the code, but isn't MODULEMODE
mapped to a dss clock (was it "dss_clk")?. And so, the clock's refcount
should keep the MODULEMODE enabled/disabled?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Archit Taneja]

On Tuesday 14 February 2012 07:03 PM, Tomi Valkeinen wrote:
> On Tue, 2012-02-14 at 19:00 +0530, 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.
>
> Hmm, are you sure? Not that I checked the code, but isn't MODULEMODE
> mapped to a dss clock (was it "dss_clk")?. And so, the clock's refcount
> should keep the MODULEMODE enabled/disabled?

Yes, that's how we are currently dealing with it and making things work. 
We are forced to represent MODULEMODE as a clock. I forgot to mention 
that in the last mail :)

However, other modules don't do this. modulemode control is taken care 
by hwmod by itself. We just have to fill the hwmod field 
'.prcm.omap4.modulemode' and get done with it. If we try this approach, 
we get into the trouble I mentioned before.

We represent MODULEMODE as dss_fck, and make this the l3 slave clock for 
all DSS hwmods. This way, we ensure that it gets enabled, and we get a 
usecount associated to it. We shouldn't stick to this approach because:

- It isn't exactly correct. MODULEMODE isn't a clock, and others don't 
do it.

- DSS requires a particular sequence of disabling clocks to go into 
lower power states, and with the current approach, this doesn't happen. 
So, DSS doesn't idle, and that's the whole purpose of this :)

Archit

>
>   Tomi
>

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

[Cousson, Benoit]

On 2/14/2012 2:45 PM, Archit Taneja wrote:
> On Tuesday 14 February 2012 07:03 PM, Tomi Valkeinen wrote:
>> On Tue, 2012-02-14 at 19:00 +0530, 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.
>>
>> Hmm, are you sure? Not that I checked the code, but isn't MODULEMODE
>> mapped to a dss clock (was it "dss_clk")?. And so, the clock's refcount
>> should keep the MODULEMODE enabled/disabled?
>
> Yes, that's how we are currently dealing with it and making things work.
> We are forced to represent MODULEMODE as a clock. I forgot to mention
> that in the last mail :)
>
> However, other modules don't do this. modulemode control is taken care
> by hwmod by itself. We just have to fill the hwmod field
> '.prcm.omap4.modulemode' and get done with it. If we try this approach,
> we get into the trouble I mentioned before.
>
> We represent MODULEMODE as dss_fck, and make this the l3 slave clock for
> all DSS hwmods. This way, we ensure that it gets enabled, and we get a
> usecount associated to it. We shouldn't stick to this approach because:
>
> - It isn't exactly correct. MODULEMODE isn't a clock, and others don't
> do it.

Yes, fully agree. This was done like that as a hack to avoid any 
regression because at that time we were not sure if that dependency will 
have to be handled by the hwmod fmwk or by the driver.
Now, I'm sure it should not be handled by the hwmod fmwk :-)

> - DSS requires a particular sequence of disabling clocks to go into
> lower power states, and with the current approach, this doesn't happen.
> So, DSS doesn't idle, and that's the whole purpose of this :)

I'm just curious, what particular sequence is required?

Thanks,
Benoit

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

[Shilimkar, Santosh]

On Tue, Feb 14, 2012 at 9:32 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> On 2/14/2012 2:45 PM, Archit Taneja wrote:
>>
>> On Tuesday 14 February 2012 07:03 PM, Tomi Valkeinen wrote:
>>>
>>> On Tue, 2012-02-14 at 19:00 +0530, 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.
>>>
>>>
>>> Hmm, are you sure? Not that I checked the code, but isn't MODULEMODE
>>> mapped to a dss clock (was it "dss_clk")?. And so, the clock's refcount
>>> should keep the MODULEMODE enabled/disabled?
>>
>>
>> Yes, that's how we are currently dealing with it and making things work.
>> We are forced to represent MODULEMODE as a clock. I forgot to mention
>> that in the last mail :)
>>
>> However, other modules don't do this. modulemode control is taken care
>> by hwmod by itself. We just have to fill the hwmod field
>> '.prcm.omap4.modulemode' and get done with it. If we try this approach,
>> we get into the trouble I mentioned before.
>>
>> We represent MODULEMODE as dss_fck, and make this the l3 slave clock for
>> all DSS hwmods. This way, we ensure that it gets enabled, and we get a
>> usecount associated to it. We shouldn't stick to this approach because:
>>
>> - It isn't exactly correct. MODULEMODE isn't a clock, and others don't
>> do it.
>
>
> Yes, fully agree. This was done like that as a hack to avoid any regression
> because at that time we were not sure if that dependency will have to be
> handled by the hwmod fmwk or by the driver.
> Now, I'm sure it should not be handled by the hwmod fmwk :-)
>
>
>> - DSS requires a particular sequence of disabling clocks to go into
>> lower power states, and with the current approach, this doesn't happen.
>> So, DSS doesn't idle, and that's the whole purpose of this :)
>
>
> I'm just curious, what particular sequence is required?
>
IIRC, the main issue is the order in which functional clock and what
is so called
optional clock enabled ....


While enabling the module
1. Enable optional clock
2. Enabled module mode.

While disabling module
1. Disable module mode
2. Disable optional clock.

I think only above sequence work. Not following above in either
path leads to modules being stuck in transition.

Is that right Archit ?

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

[Archit Taneja]

On Tuesday 14 February 2012 10:29 PM, Shilimkar, Santosh wrote:
> On Tue, Feb 14, 2012 at 9:32 PM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> On 2/14/2012 2:45 PM, Archit Taneja wrote:
>>>
>>> On Tuesday 14 February 2012 07:03 PM, Tomi Valkeinen wrote:
>>>>
>>>> On Tue, 2012-02-14 at 19:00 +0530, 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.
>>>>
>>>>
>>>> Hmm, are you sure? Not that I checked the code, but isn't MODULEMODE
>>>> mapped to a dss clock (was it "dss_clk")?. And so, the clock's refcount
>>>> should keep the MODULEMODE enabled/disabled?
>>>
>>>
>>> Yes, that's how we are currently dealing with it and making things work.
>>> We are forced to represent MODULEMODE as a clock. I forgot to mention
>>> that in the last mail :)
>>>
>>> However, other modules don't do this. modulemode control is taken care
>>> by hwmod by itself. We just have to fill the hwmod field
>>> '.prcm.omap4.modulemode' and get done with it. If we try this approach,
>>> we get into the trouble I mentioned before.
>>>
>>> We represent MODULEMODE as dss_fck, and make this the l3 slave clock for
>>> all DSS hwmods. This way, we ensure that it gets enabled, and we get a
>>> usecount associated to it. We shouldn't stick to this approach because:
>>>
>>> - It isn't exactly correct. MODULEMODE isn't a clock, and others don't
>>> do it.
>>
>>
>> Yes, fully agree. This was done like that as a hack to avoid any regression
>> because at that time we were not sure if that dependency will have to be
>> handled by the hwmod fmwk or by the driver.
>> Now, I'm sure it should not be handled by the hwmod fmwk :-)
>>
>>
>>> - DSS requires a particular sequence of disabling clocks to go into
>>> lower power states, and with the current approach, this doesn't happen.
>>> So, DSS doesn't idle, and that's the whole purpose of this :)
>>
>>
>> I'm just curious, what particular sequence is required?
>>
> IIRC, the main issue is the order in which functional clock and what
> is so called
> optional clock enabled ....
>
>
> While enabling the module
> 1. Enable optional clock
> 2. Enabled module mode.
>
> While disabling module
> 1. Disable module mode
> 2. Disable optional clock.
>
> I think only above sequence work. Not following above in either
> path leads to modules being stuck in transition.
>
> Is that right Archit ?

Yes. Also, this transition failure happens when DSS clock domain is set 
to hardware supervised wakeup(HWAUTO).

The opt clock which requires this ordering with module mode is DSS_FCLK 
by default. However, if some other clock (like DSI PLL, sourced by 
SYS_CLK) is the source for DISPC_FCLK, then that clock is the one which 
needs to be ordered with module mode.

Archit

> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Cousson, Benoit]

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.

Regards,
Benoit

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

[Archit Taneja]

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
>
>

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

[Cousson, Benoit]

On 2/15/2012 1:01 PM, Archit Taneja wrote:
> 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.

Yes, but as you pointed out the current clocks mapping in the DSS is a 
hack that should not be done like that in theory.
As soon as the devices can handle the dependency, we will be able to fix 
the clock mapping in the hwmod data and handle properly the module mode 
only from the dss_core hwmod only.

But I think you have to change the driver first, otherwise it will break 
the DSS. We did that hack previously because it was the only way to 
enable the DSS properly, knowing that disabling it with that method will 
be impossible.

I think that changing the device creation to change the dev->parent 
should be pretty straightforward.

Regards,
Benoit

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On Wed, 2012-02-15 at 13:35 +0100, Cousson, Benoit wrote:

> I think that changing the device creation to change the dev->parent 
> should be pretty straightforward.

That's not possible with the current kernel, right?

We are now using omap_device_build() (in arch/arm/mach-omap2/display.c)
to build the dss devices. Looking at the omap_device.c, the parent will
always forcibly set to omap_device_parent. It'd be nice to be able to
construct the device child-parent relationship the same way with both DT
and non-DT cases.

Or can I create only the dss_core with omap_device_build(), and create
the rest normally with platform device functions, and make dss_core the
parent of the rest? But are the hwmods then handled correctly?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Cousson, Benoit]

+ Kevin

On 2/15/2012 1:51 PM, Tomi Valkeinen wrote:
> On Wed, 2012-02-15 at 13:35 +0100, Cousson, Benoit wrote:
>
>> I think that changing the device creation to change the dev->parent
>> should be pretty straightforward.
>
> That's not possible with the current kernel, right?
>
> We are now using omap_device_build() (in arch/arm/mach-omap2/display.c)
> to build the dss devices. Looking at the omap_device.c, the parent will
> always forcibly set to omap_device_parent. It'd be nice to be able to
> construct the device child-parent relationship the same way with both DT
> and non-DT cases.

I guess this should not be needed anymore since now the whole PM runtime 
stuff is handled using pm_domain.

int omap_device_register(struct platform_device *pdev)
{
	pr_debug("omap_device: %s: registering\n", pdev->name);

	pdev->dev.parent = &omap_device_parent;
	pdev->dev.pm_domain = &omap_device_pm_domain;
	return platform_device_add(pdev);
}


Kevin,

Do we still need to set the dev.parent to omap_device_parent?
I guess the default &platform_bus parent is good enough and potentially 
the DSS children should be able to overwrite that.

> Or can I create only the dss_core with omap_device_build(), and create
> the rest normally with platform device functions, and make dss_core the
> parent of the rest? But are the hwmods then handled correctly?

You can, the only issue if you create a regular platform device is that 
you will miss the automatic pm_runtime support along with the hwmod 
device creation mechanism + clock / PM.

I think we can add an extra parameter to allow changing the omap_device 
parent during omap_device_build.

Regards,
Benoit

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

[Kevin Hilman]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> + Kevin
>
> On 2/15/2012 1:51 PM, Tomi Valkeinen wrote:
>> On Wed, 2012-02-15 at 13:35 +0100, Cousson, Benoit wrote:
>>
>>> I think that changing the device creation to change the dev->parent
>>> should be pretty straightforward.
>>
>> That's not possible with the current kernel, right?
>>
>> We are now using omap_device_build() (in arch/arm/mach-omap2/display.c)
>> to build the dss devices. Looking at the omap_device.c, the parent will
>> always forcibly set to omap_device_parent. It'd be nice to be able to
>> construct the device child-parent relationship the same way with both DT
>> and non-DT cases.
>
> I guess this should not be needed anymore since now the whole PM
> runtime stuff is handled using pm_domain.
>
> int omap_device_register(struct platform_device *pdev)
> {
> 	pr_debug("omap_device: %s: registering\n", pdev->name);
>
> 	pdev->dev.parent = &omap_device_parent;
> 	pdev->dev.pm_domain = &omap_device_pm_domain;
> 	return platform_device_add(pdev);
> }
>
>
> Kevin,
>
> Do we still need to set the dev.parent to omap_device_parent?

Nope.

> I guess the default &platform_bus parent is good enough and
> potentially the DSS children should be able to overwrite that.

Yes, now that we use PM domains, we don't need it.  I just sent a patch
to remove omap_device_parent.

Kevin

>> Or can I create only the dss_core with omap_device_build(), and create
>> the rest normally with platform device functions, and make dss_core the
>> parent of the rest? But are the hwmods then handled correctly?
>
> You can, the only issue if you create a regular platform device is
> that you will miss the automatic pm_runtime support along with the
> hwmod device creation mechanism + clock / PM.
>
> I think we can add an extra parameter to allow changing the
> omap_device parent during omap_device_build.

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, 2012-02-15 at 11:59 -0800, Kevin Hilman wrote:
> "Cousson, Benoit" <b-cousson@ti.com> writes:

> > Kevin,
> >
> > Do we still need to set the dev.parent to omap_device_parent?
> 
> Nope.
> 
> > I guess the default &platform_bus parent is good enough and
> > potentially the DSS children should be able to overwrite that.
> 
> Yes, now that we use PM domains, we don't need it.  I just sent a patch
> to remove omap_device_parent.

But it's still not possible to create a custom parent-child hierarchy
with omap_devices. Or can I change the parent of a platform_device after
it has been created? (doesn't sound very clean even if I can)

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Cousson, Benoit]

On 2/16/2012 9:22 AM, Tomi Valkeinen wrote:
> On Wed, 2012-02-15 at 11:59 -0800, Kevin Hilman wrote:
>> "Cousson, Benoit"<b-cousson@ti.com>  writes:
>
>>> Kevin,
>>>
>>> Do we still need to set the dev.parent to omap_device_parent?
>>
>> Nope.
>>
>>> I guess the default&platform_bus parent is good enough and
>>> potentially the DSS children should be able to overwrite that.
>>
>> Yes, now that we use PM domains, we don't need it.  I just sent a patch
>> to remove omap_device_parent.
>
> But it's still not possible to create a custom parent-child hierarchy
> with omap_devices. Or can I change the parent of a platform_device after
> it has been created? (doesn't sound very clean even if I can)

Nope, it has to be done before platform_device_add.

Thanks to Ohad patch [1] that Tony has just resent, you can use the 
internal omap_device API and thus will be able to change the parent 
before the registration.

Benoit

[1] ARM: OMAP: omap_device: Expose omap_device_{alloc, delete, register}