OMAP4 DSS clock setup

OMAP4 DSS clock setup

[Tomi Valkeinen]

Hi Benoit, Paul,

I've been discussing with Sumit and Archit to understand how the DSS
clocks are set up on OMAP4. I think I now have some idea how things
work, but I'm still at loss why things are the way they are.

So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
controllable, but it is affected by MODULEMODE bit.

Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
the TRM's DSS_FCLK.

Was that correct?

If so, from DSS driver's perspective, the dss_fck sounds very much like
an interface clock (it's always needed when DSS is used) and dss_dss_clk
sounds very much like functional clock (it's always needed, except if
DSI PLL is used for DSS functional clock).

If "dss_fck" would control DSS_FCLK and "dss_ick" would control
MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
and we wouldn't need any omap4 spesific trickery in the DSS driver.
("dss_dss_clk" wouldn't be needed).

Why are the clocks set up in this strange fashion?

 Tomi

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

Hi Tomi,

On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
> Hi Benoit, Paul,
>
> I've been discussing with Sumit and Archit to understand how the DSS
> clocks are set up on OMAP4. I think I now have some idea how things
> work, but I'm still at loss why things are the way they are.
>
> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
> controllable, but it is affected by MODULEMODE bit.
>
> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
> the TRM's DSS_FCLK.
>
> Was that correct?

Yes. For the moment, but this is not the final state.

> If so, from DSS driver's perspective, the dss_fck sounds very much like
> an interface clock (it's always needed when DSS is used) and dss_dss_clk
> sounds very much like functional clock (it's always needed, except if
> DSI PLL is used for DSS functional clock).
>
> If "dss_fck" would control DSS_FCLK and "dss_ick" would control
> MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
> and we wouldn't need any omap4 spesific trickery in the DSS driver.
> ("dss_dss_clk" wouldn't be needed).

You cannot play with iclk, because this clock is supposed to be handled 
automatically by the HW. This was the case on OMAP2 & 3 as well BTW.

> Why are the clocks set up in this strange fashion?

There are a lot of historical reasons and some changes from OMAP3 to 
OMAP4 that are not well handle by the current clock fmwk / hwmod fmwk.
We are still trying to mimic OMAP2&3 clock management for OMAP4, and in 
fact we should do the opposite.

The issue is due to the confusion with the MODULEMODE and the real input 
clock state.
The MODULEMODE is just enabling the module (using iclk, fclk, opt clock) 
or whatever clk is used as input clock.
In the case of the DSS, several optional clocks can be used as fclk. 
SYS_CLK, DSSCLK or the output of the DSI.
In theory the MODULEMODE should be directly handled by hwmod since it is 
similar to enable / disable the module. Using a clock to manage the 
MODULEMODE is a temporary hack we are still using for the moment but 
that should be removed at some point.

The clock fmwk should be used only to select / enable the proper 
optional clocks.

In theory the current OMAP2 and OMAP3 iclken/fclken at PRCM level should 
be handled as a OMAP4 MODULEMODE and not as 2 clock nodes like it is the 
case today. Then we will have a consistent module / clock management.

Hope this will help and clarify a little bit the current mess :-)

Bottom-line is that you cannot do much at your level, the whole clock + 
hwmod fmwk should be improved.

Benoit

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
> Hi Tomi,
> 
> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
> > Hi Benoit, Paul,
> >
> > I've been discussing with Sumit and Archit to understand how the DSS
> > clocks are set up on OMAP4. I think I now have some idea how things
> > work, but I'm still at loss why things are the way they are.
> >
> > So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
> > clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
> > and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
> > controllable, but it is affected by MODULEMODE bit.
> >
> > Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
> > and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
> > the TRM's DSS_FCLK.
> >
> > Was that correct?
> 
> Yes. For the moment, but this is not the final state.
> 
> > If so, from DSS driver's perspective, the dss_fck sounds very much like
> > an interface clock (it's always needed when DSS is used) and dss_dss_clk
> > sounds very much like functional clock (it's always needed, except if
> > DSI PLL is used for DSS functional clock).
> >
> > If "dss_fck" would control DSS_FCLK and "dss_ick" would control
> > MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
> > and we wouldn't need any omap4 spesific trickery in the DSS driver.
> > ("dss_dss_clk" wouldn't be needed).
> 
> You cannot play with iclk, because this clock is supposed to be handled 
> automatically by the HW. This was the case on OMAP2 & 3 as well BTW.

Right.

But now we have 1) dss_fck, which isn't the DSS_FCLK in TRM, and in fact
not fck at all, or even clock at all 2) dss_dss_clk, a new clock whose
name doesn't match to anything in TRM, and is in fact the DSS_FCLK.

So they both sound like they are confusingly named.

If we had dss_fck matching to DSS_FCLK and dss_ick for MODULEMODE, they
would, in my opinion, be much more understandable and in many ways
relate to the way things are in the HW. Additionally this would match
the way clocks are on OMAP2/3 and things would just work.

So while I understand that neither the current way and my suggestion
exactly match the HW, and also that things are not ready yet and the
clocks will change, I don't understand why such a strange method to name
the clocks was used, when there's a simpler and backward compatible way.

And if this current way to handle OMAP4 DSS clocks is for some reason
better and can't be changed, could the OMAP2/3 clocks also be changed to
keep things consistent between OMAPs? Because the inconsistency between
platforms is the biggest problem for me.

 Tomi

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

On 3/30/2011 1:03 PM, Valkeinen, Tomi wrote:
> On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
>> Hi Tomi,
>>
>> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
>>> Hi Benoit, Paul,
>>>
>>> I've been discussing with Sumit and Archit to understand how the DSS
>>> clocks are set up on OMAP4. I think I now have some idea how things
>>> work, but I'm still at loss why things are the way they are.
>>>
>>> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
>>> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
>>> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
>>> controllable, but it is affected by MODULEMODE bit.
>>>
>>> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
>>> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
>>> the TRM's DSS_FCLK.
>>>
>>> Was that correct?
>>
>> Yes. For the moment, but this is not the final state.
>>
>>> If so, from DSS driver's perspective, the dss_fck sounds very much like
>>> an interface clock (it's always needed when DSS is used) and dss_dss_clk
>>> sounds very much like functional clock (it's always needed, except if
>>> DSI PLL is used for DSS functional clock).

dss_dss_fck is one of the possible functional clocks, hence the optional 
attribute. You can run the DSS only for sys_clk is you want.
That's why the hwmod fmwk cannot choose for you which one you will need 
depending of your panel. It is up to the driver to select the correct one.

>>> If "dss_fck" would control DSS_FCLK and "dss_ick" would control
>>> MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
>>> and we wouldn't need any omap4 spesific trickery in the DSS driver.
>>> ("dss_dss_clk" wouldn't be needed).
>>
>> You cannot play with iclk, because this clock is supposed to be handled
>> automatically by the HW. This was the case on OMAP2&  3 as well BTW.
>
> Right.
>
> But now we have 1) dss_fck, which isn't the DSS_FCLK in TRM, and in fact
> not fck at all, or even clock at all 2) dss_dss_clk, a new clock whose
> name doesn't match to anything in TRM, and is in fact the DSS_FCLK.
>
> So they both sound like they are confusingly named.

The naming convention is simple: opt clock are using the module name + 
the original clock name: "dss_XXX", whereas the modulemode is using the 
module name + fck, because it is for the moment similar to the fclk we 
have on simpler module. iclk_en is not exposed at all on OMAP4.

optional clocks:

static struct clk dss_sys_clk = {
	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
	.enable_bit	= OMAP4430_OPTFCLKEN_SYS_CLK_SHIFT,

static struct clk dss_tv_clk = {
	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
	.enable_bit	= OMAP4430_OPTFCLKEN_TV_CLK_SHIFT,

static struct clk dss_dss_clk = {
	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
	.enable_bit	= OMAP4430_OPTFCLKEN_DSSCLK_SHIFT,

static struct clk dss_48mhz_clk = {
	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
	.enable_bit	= OMAP4430_OPTFCLKEN_48MHZ_CLK_SHIFT,


MODULEMODE:

static struct clk dss_fck = {
	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
	.enable_bit	= OMAP4430_MODULEMODE_SWCTRL,

> If we had dss_fck matching to DSS_FCLK and dss_ick for MODULEMODE, they
> would, in my opinion, be much more understandable and in many ways
> relate to the way things are in the HW. Additionally this would match
> the way clocks are on OMAP2/3 and things would just work.

No, because you should not have to play with iclk at all.
BTW, for my point of view you have such issue because your are still not 
using the pm_runtime API.
As soon as you will switch to pm_runtime, iclk_en and fclk_en (OMAP2&3 
name) or MODULEMODE will be handle by the fmwk. The driver will just 
have to handle the optional clock.

> So while I understand that neither the current way and my suggestion
> exactly match the HW, and also that things are not ready yet and the
> clocks will change, I don't understand why such a strange method to name
> the clocks was used, when there's a simpler and backward compatible way.

The point is that this automated method works for every IPs in the OMAP 
except the DSS that is not managing its clocks like other modules.
Moreover, since the clock fmwk is creating aliases for these clock 
nodes, you should never see at all that dss_dss_clk node that seems to 
bother you.

> And if this current way to handle OMAP4 DSS clocks is for some reason
> better and can't be changed, could the OMAP2/3 clocks also be changed to
> keep things consistent between OMAPs? Because the inconsistency between
> platforms is the biggest problem for me.

As I said previsously, pm_runtime should fix these issues.

Benoit

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

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

On Wed, 2011-03-30 at 14:12 +0200, Cousson, Benoit wrote:
> On 3/30/2011 1:03 PM, Valkeinen, Tomi wrote:
> > On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
> >> Hi Tomi,
> >>
> >> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
> >>> Hi Benoit, Paul,
> >>>
> >>> I've been discussing with Sumit and Archit to understand how the DSS
> >>> clocks are set up on OMAP4. I think I now have some idea how things
> >>> work, but I'm still at loss why things are the way they are.
> >>>
> >>> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
> >>> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
> >>> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
> >>> controllable, but it is affected by MODULEMODE bit.
> >>>
> >>> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
> >>> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
> >>> the TRM's DSS_FCLK.
> >>>
> >>> Was that correct?
> >>
> >> Yes. For the moment, but this is not the final state.
> >>
> >>> If so, from DSS driver's perspective, the dss_fck sounds very much like
> >>> an interface clock (it's always needed when DSS is used) and dss_dss_clk
> >>> sounds very much like functional clock (it's always needed, except if
> >>> DSI PLL is used for DSS functional clock).
> 
> dss_dss_fck is one of the possible functional clocks, hence the optional 
> attribute. You can run the DSS only for sys_clk is you want.

We can? I remember this was possible on OMAP2, but I can't seem to find
it on OMAP4 TRM...

Ah right, you mean sys_clk goes to DSI PLL, and the output from DSI PLL
can be used as functional clock?

We _always_ need DSS_FCLK to get DSS up and running, and to configure
DSI PLL if we want to get the clocks from there. So it's not quite that
optional. But it's true that we can turn it off later.

> That's why the hwmod fmwk cannot choose for you which one you will need 
> depending of your panel. It is up to the driver to select the correct one.

But isn't that DSS internal thing? (I'm looking again at the DSS clock
tree figure). DSS_FCLK from the PRCM should always be the same from
DSS's point of view, it doesn't change. If we use the clock from DSI
PLL, we'll get clock for LCD2_CLK, LCD1_CLK and DISPC_FCLK. But
DSS_FCLK/DSS_CLK is always the same.

> >>> If "dss_fck" would control DSS_FCLK and "dss_ick" would control
> >>> MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
> >>> and we wouldn't need any omap4 spesific trickery in the DSS driver.
> >>> ("dss_dss_clk" wouldn't be needed).
> >>
> >> You cannot play with iclk, because this clock is supposed to be handled
> >> automatically by the HW. This was the case on OMAP2&  3 as well BTW.
> >
> > Right.
> >
> > But now we have 1) dss_fck, which isn't the DSS_FCLK in TRM, and in fact
> > not fck at all, or even clock at all 2) dss_dss_clk, a new clock whose
> > name doesn't match to anything in TRM, and is in fact the DSS_FCLK.
> >
> > So they both sound like they are confusingly named.
> 
> The naming convention is simple: opt clock are using the module name + 
> the original clock name: "dss_XXX", whereas the modulemode is using the 
> module name + fck, because it is for the moment similar to the fclk we 
> have on simpler module. iclk_en is not exposed at all on OMAP4.
> 
> optional clocks:
> 
> static struct clk dss_sys_clk = {
> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
> 	.enable_bit	= OMAP4430_OPTFCLKEN_SYS_CLK_SHIFT,
> 
> static struct clk dss_tv_clk = {
> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
> 	.enable_bit	= OMAP4430_OPTFCLKEN_TV_CLK_SHIFT,
> 
> static struct clk dss_dss_clk = {
> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
> 	.enable_bit	= OMAP4430_OPTFCLKEN_DSSCLK_SHIFT,
> 
> static struct clk dss_48mhz_clk = {
> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
> 	.enable_bit	= OMAP4430_OPTFCLKEN_48MHZ_CLK_SHIFT,
> 
> 
> MODULEMODE:
> 
> static struct clk dss_fck = {
> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
> 	.enable_bit	= OMAP4430_MODULEMODE_SWCTRL,

Okay, the naming makes sense now that you explained the convention.

I think I'm finally starting to get this =).

> > If we had dss_fck matching to DSS_FCLK and dss_ick for MODULEMODE, they
> > would, in my opinion, be much more understandable and in many ways
> > relate to the way things are in the HW. Additionally this would match
> > the way clocks are on OMAP2/3 and things would just work.
> 
> No, because you should not have to play with iclk at all.
> BTW, for my point of view you have such issue because your are still not 
> using the pm_runtime API.
> As soon as you will switch to pm_runtime, iclk_en and fclk_en (OMAP2&3 
> name) or MODULEMODE will be handle by the fmwk. The driver will just 
> have to handle the optional clock.
> 
> > So while I understand that neither the current way and my suggestion
> > exactly match the HW, and also that things are not ready yet and the
> > clocks will change, I don't understand why such a strange method to name
> > the clocks was used, when there's a simpler and backward compatible way.
> 
> The point is that this automated method works for every IPs in the OMAP 
> except the DSS that is not managing its clocks like other modules.

With this do you mean that the SW does not manage clocks like other
modules (should use pm_runtime?), or the HW is somehow different?

> Moreover, since the clock fmwk is creating aliases for these clock 
> nodes, you should never see at all that dss_dss_clk node that seems to 
> bother you.

Oh, that doesn't bother me =). What bothers me is that DSS did not
suddenly work because the clocks were set up in this manner.

Perhaps the aliases should have been setup differently, at least
temporarily, to get things working more easily.

Currently we have these aliases for OMAP4:

"dss_clk" -> dss_dss_clk
"fck" -> dss_fck
"ick" -> dummy_ck

If that would be changed to:

"fck" -> dss_dss_clk
"ick -> dss_fck

The driver would work the same way for all OMAPs.

> > And if this current way to handle OMAP4 DSS clocks is for some reason
> > better and can't be changed, could the OMAP2/3 clocks also be changed to
> > keep things consistent between OMAPs? Because the inconsistency between
> > platforms is the biggest problem for me.
> 
> As I said previsously, pm_runtime should fix these issues.

Ok. I really need to find time to look at that. Sumit has been working
on it, I hope it'll be ready soon =).

Anyway. To get things working for rc2 (DSS currently crashes due to not
enabling dss_clk) we need to either:
1) Change the clock aliases as above
2) Add something like Sumit's patch (attached) to handle dss_clk. While
the patch is not that complex, it feels rather hacky.

What's your opinion?

 Tomi


[-- Attachment #2: 0001-OMAP4-DSS2-add-dss_dss_clk.patch --]
[-- Type: text/x-patch, Size: 2955 bytes --]

>From 0722361a0921296c980cb9011dad42f56c929007 Mon Sep 17 00:00:00 2001
From: Sumit Semwal <sumit.semwal@ti.com>
Date: Tue, 1 Mar 2011 14:23:09 +0530
Subject: [PATCH] OMAP4:DSS2: add dss_dss_clk.

dss_dss_clk is a new clock needed in OMAP4 as an opt-clock.
Adding the same in dss clock handling.

Signed-off-by: Sumit Semwal <sumit.semwal@ti.com>
---
 drivers/video/omap2/dss/dss.c |   33 ++++++++++++++++++++++++++++++---
 1 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 3d0277d..6c1a090 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -67,6 +67,7 @@ static struct {
 	struct clk	*dss_sys_clk;
 	struct clk	*dss_tv_fck;
 	struct clk	*dss_video_fck;
+	struct clk	*dss_dss_clk;
 	unsigned	num_clks_enabled;
 
 	unsigned long	cache_req_pck;
@@ -706,6 +707,7 @@ static int dss_get_clocks(void)
 	dss.dss_sys_clk = NULL;
 	dss.dss_tv_fck = NULL;
 	dss.dss_video_fck = NULL;
+	dss.dss_dss_clk	= NULL;
 
 	r = dss_get_clock(&dss.dss_ick, "ick");
 	if (r)
@@ -738,6 +740,12 @@ static int dss_get_clocks(void)
 			goto err;
 	}
 
+	if (pdata->opt_clock_available("dss_clk")) {
+		r = dss_get_clock(&dss.dss_dss_clk, "dss_clk");
+		if (r)
+			goto err;
+	}
+
 	return 0;
 
 err:
@@ -751,7 +759,8 @@ err:
 		clk_put(dss.dss_tv_fck);
 	if (dss.dss_video_fck)
 		clk_put(dss.dss_video_fck);
-
+	if (dss.dss_dss_clk)
+		clk_put(dss.dss_dss_clk);
 	return r;
 }
 
@@ -763,6 +772,8 @@ static void dss_put_clocks(void)
 		clk_put(dss.dss_tv_fck);
 	if (dss.dss_sys_clk)
 		clk_put(dss.dss_sys_clk);
+	if (dss.dss_dss_clk)
+		clk_put(dss.dss_dss_clk);
 	clk_put(dss.dss_fck);
 	clk_put(dss.dss_ick);
 }
@@ -810,8 +821,16 @@ static void dss_clk_enable_no_ctx(enum dss_clock clks)
 
 	if (clks & DSS_CLK_ICK)
 		clk_enable(dss.dss_ick);
-	if (clks & DSS_CLK_FCK)
+	/*
+	 * XXX: tie dss_dss_clk to FCK - this will change with following
+	 * pm_runtime patches. Needed for OMAP4 boot up due to stricter
+	 * clock cutting in pm framework post 2.6.38-rc5.
+	 */
+	if (clks & DSS_CLK_FCK) {
 		clk_enable(dss.dss_fck);
+		if (dss.dss_dss_clk)
+			clk_enable(dss.dss_dss_clk);
+	}
 	if ((clks & DSS_CLK_SYSCK) && dss.dss_sys_clk)
 		clk_enable(dss.dss_sys_clk);
 	if ((clks & DSS_CLK_TVFCK) && dss.dss_tv_fck)
@@ -838,8 +857,16 @@ static void dss_clk_disable_no_ctx(enum dss_clock clks)
 
 	if (clks & DSS_CLK_ICK)
 		clk_disable(dss.dss_ick);
-	if (clks & DSS_CLK_FCK)
+        /*
+         * XXX: tie dss_dss_clk to FCK - this will change with following
+         * pm_runtime patches. Needed for OMAP4 boot up due to stricter
+         * clock cutting in pm framework post 2.6.38-rc5.
+         */
+	if (clks & DSS_CLK_FCK) {
 		clk_disable(dss.dss_fck);
+		if (dss.dss_dss_clk)
+			clk_disable(dss.dss_dss_clk);
+	}
 	if ((clks & DSS_CLK_SYSCK) && dss.dss_sys_clk)
 		clk_disable(dss.dss_sys_clk);
 	if ((clks & DSS_CLK_TVFCK) && dss.dss_tv_fck)
-- 
1.7.1

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

On 3/30/2011 2:58 PM, Valkeinen, Tomi wrote:
> On Wed, 2011-03-30 at 14:12 +0200, Cousson, Benoit wrote:
>> On 3/30/2011 1:03 PM, Valkeinen, Tomi wrote:
>>> On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
>>>> Hi Tomi,
>>>>
>>>> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
>>>>> Hi Benoit, Paul,
>>>>>
>>>>> I've been discussing with Sumit and Archit to understand how the DSS
>>>>> clocks are set up on OMAP4. I think I now have some idea how things
>>>>> work, but I'm still at loss why things are the way they are.
>>>>>
>>>>> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
>>>>> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
>>>>> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
>>>>> controllable, but it is affected by MODULEMODE bit.
>>>>>
>>>>> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
>>>>> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
>>>>> the TRM's DSS_FCLK.
>>>>>
>>>>> Was that correct?
>>>>
>>>> Yes. For the moment, but this is not the final state.
>>>>
>>>>> If so, from DSS driver's perspective, the dss_fck sounds very much like
>>>>> an interface clock (it's always needed when DSS is used) and dss_dss_clk
>>>>> sounds very much like functional clock (it's always needed, except if
>>>>> DSI PLL is used for DSS functional clock).
>>
>> dss_dss_fck is one of the possible functional clocks, hence the optional
>> attribute. You can run the DSS only for sys_clk is you want.
>
> We can? I remember this was possible on OMAP2, but I can't seem to find
> it on OMAP4 TRM...
>
> Ah right, you mean sys_clk goes to DSI PLL, and the output from DSI PLL
> can be used as functional clock?

Yes, you're right, maybe we can still use the sys_clk directly with a 
parallel QVGA LCD like on OMAP2:-)

> We _always_ need DSS_FCLK to get DSS up and running, and to configure
> DSI PLL if we want to get the clocks from there. So it's not quite that
> optional. But it's true that we can turn it off later.

Yes, that was confirmed by HW folks, as soon as you have one functional 
clock active, you can disable the other one.

>> That's why the hwmod fmwk cannot choose for you which one you will need
>> depending of your panel. It is up to the driver to select the correct one.
>
> But isn't that DSS internal thing? (I'm looking again at the DSS clock
> tree figure). DSS_FCLK from the PRCM should always be the same from
> DSS's point of view, it doesn't change. If we use the clock from DSI
> PLL, we'll get clock for LCD2_CLK, LCD1_CLK and DISPC_FCLK. But
> DSS_FCLK/DSS_CLK is always the same.

Yes, but there is no other need for the DSS_CLK beside these internal 
nodes and the DSI engines. So you can shut it down as soon as an 
alternate clock is available (PLL1_CLK1 or PLL2_CLK1 in your case).

>>>>> If "dss_fck" would control DSS_FCLK and "dss_ick" would control
>>>>> MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
>>>>> and we wouldn't need any omap4 spesific trickery in the DSS driver.
>>>>> ("dss_dss_clk" wouldn't be needed).
>>>>
>>>> You cannot play with iclk, because this clock is supposed to be handled
>>>> automatically by the HW. This was the case on OMAP2&   3 as well BTW.
>>>
>>> Right.
>>>
>>> But now we have 1) dss_fck, which isn't the DSS_FCLK in TRM, and in fact
>>> not fck at all, or even clock at all 2) dss_dss_clk, a new clock whose
>>> name doesn't match to anything in TRM, and is in fact the DSS_FCLK.
>>>
>>> So they both sound like they are confusingly named.
>>
>> The naming convention is simple: opt clock are using the module name +
>> the original clock name: "dss_XXX", whereas the modulemode is using the
>> module name + fck, because it is for the moment similar to the fclk we
>> have on simpler module. iclk_en is not exposed at all on OMAP4.
>>
>> optional clocks:
>>
>> static struct clk dss_sys_clk = {
>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>> 	.enable_bit	= OMAP4430_OPTFCLKEN_SYS_CLK_SHIFT,
>>
>> static struct clk dss_tv_clk = {
>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>> 	.enable_bit	= OMAP4430_OPTFCLKEN_TV_CLK_SHIFT,
>>
>> static struct clk dss_dss_clk = {
>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>> 	.enable_bit	= OMAP4430_OPTFCLKEN_DSSCLK_SHIFT,
>>
>> static struct clk dss_48mhz_clk = {
>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>> 	.enable_bit	= OMAP4430_OPTFCLKEN_48MHZ_CLK_SHIFT,
>>
>>
>> MODULEMODE:
>>
>> static struct clk dss_fck = {
>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>> 	.enable_bit	= OMAP4430_MODULEMODE_SWCTRL,
>
> Okay, the naming makes sense now that you explained the convention.
>
> I think I'm finally starting to get this =).
>
>>> If we had dss_fck matching to DSS_FCLK and dss_ick for MODULEMODE, they
>>> would, in my opinion, be much more understandable and in many ways
>>> relate to the way things are in the HW. Additionally this would match
>>> the way clocks are on OMAP2/3 and things would just work.
>>
>> No, because you should not have to play with iclk at all.
>> BTW, for my point of view you have such issue because your are still not
>> using the pm_runtime API.
>> As soon as you will switch to pm_runtime, iclk_en and fclk_en (OMAP2&3
>> name) or MODULEMODE will be handle by the fmwk. The driver will just
>> have to handle the optional clock.
>>
>>> So while I understand that neither the current way and my suggestion
>>> exactly match the HW, and also that things are not ready yet and the
>>> clocks will change, I don't understand why such a strange method to name
>>> the clocks was used, when there's a simpler and backward compatible way.
>>
>> The point is that this automated method works for every IPs in the OMAP
>> except the DSS that is not managing its clocks like other modules.
>
> With this do you mean that the SW does not manage clocks like other
> modules (should use pm_runtime?), or the HW is somehow different?
>
>> Moreover, since the clock fmwk is creating aliases for these clock
>> nodes, you should never see at all that dss_dss_clk node that seems to
>> bother you.
>
> Oh, that doesn't bother me =). What bothers me is that DSS did not
> suddenly work because the clocks were set up in this manner.
>
> Perhaps the aliases should have been setup differently, at least
> temporarily, to get things working more easily.
>
> Currently we have these aliases for OMAP4:
>
> "dss_clk" ->  dss_dss_clk
> "fck" ->  dss_fck
> "ick" ->  dummy_ck
>
> If that would be changed to:
>
> "fck" ->  dss_dss_clk
> "ick ->  dss_fck
>
> The driver would work the same way for all OMAPs.
>
>>> And if this current way to handle OMAP4 DSS clocks is for some reason
>>> better and can't be changed, could the OMAP2/3 clocks also be changed to
>>> keep things consistent between OMAPs? Because the inconsistency between
>>> platforms is the biggest problem for me.
>>
>> As I said previsously, pm_runtime should fix these issues.
>
> Ok. I really need to find time to look at that. Sumit has been working
> on it, I hope it'll be ready soon =).
>
> Anyway. To get things working for rc2 (DSS currently crashes due to not
> enabling dss_clk) we need to either:
> 1) Change the clock aliases as above

Changing the clock alias for my point of view is like hacking the HW 
definition to fit your need. Even if this is temporary, I do not like 
that approach.

> 2) Add something like Sumit's patch (attached) to handle dss_clk. While
> the patch is not that complex, it feels rather hacky.

Yes, I'd rather hack that issue internally, because this is something 
you should fix as soon as you switch to PM runtime.

> What's your opinion?

Definitively #2.

Benoit

Re: OMAP4 DSS clock setup

[Archit Taneja]

On Wednesday 30 March 2011 06:51 PM, Cousson, Benoit wrote:
> On 3/30/2011 2:58 PM, Valkeinen, Tomi wrote:
>> On Wed, 2011-03-30 at 14:12 +0200, Cousson, Benoit wrote:
>>> On 3/30/2011 1:03 PM, Valkeinen, Tomi wrote:
>>>> On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
>>>>> Hi Tomi,
>>>>>
>>>>> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
>>>>>> Hi Benoit, Paul,
>>>>>>
>>>>>> I've been discussing with Sumit and Archit to understand how the DSS
>>>>>> clocks are set up on OMAP4. I think I now have some idea how things
>>>>>> work, but I'm still at loss why things are the way they are.
>>>>>>
>>>>>> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
>>>>>> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
>>>>>> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
>>>>>> controllable, but it is affected by MODULEMODE bit.
>>>>>>
>>>>>> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
>>>>>> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
>>>>>> the TRM's DSS_FCLK.
>>>>>>
>>>>>> Was that correct?
>>>>>
>>>>> Yes. For the moment, but this is not the final state.
>>>>>
>>>>>> If so, from DSS driver's perspective, the dss_fck sounds very much like
>>>>>> an interface clock (it's always needed when DSS is used) and dss_dss_clk
>>>>>> sounds very much like functional clock (it's always needed, except if
>>>>>> DSI PLL is used for DSS functional clock).
>>>
>>> dss_dss_fck is one of the possible functional clocks, hence the optional
>>> attribute. You can run the DSS only for sys_clk is you want.
>>
>> We can? I remember this was possible on OMAP2, but I can't seem to find
>> it on OMAP4 TRM...
>>
>> Ah right, you mean sys_clk goes to DSI PLL, and the output from DSI PLL
>> can be used as functional clock?
>
> Yes, you're right, maybe we can still use the sys_clk directly with a
> parallel QVGA LCD like on OMAP2:-)
>
>> We _always_ need DSS_FCLK to get DSS up and running, and to configure
>> DSI PLL if we want to get the clocks from there. So it's not quite that
>> optional. But it's true that we can turn it off later.

Just wanted to clarify the issue for myself and summarise:

hwmod and pm runtime ensures that the MODULEMODE bits are set, and 
technically, that should be enough for a module to get up and running. 
But because of the strange design of DSS HW, we need an additional clock 
(DSS_CLK at bootup, could be something else later on) to access DSS 
registers. So we need to enable dss_dss_fclk in our driver in the 
beginning itself to access a register, hence Sumit's patch is needed.

The strange thing is that if dss_dss_fclk is 
off(OMAP4430_OPTFCLKEN_DSSCLK bit is 0) it doesn't mean that the clock 
is surely OFF. Its only OFF when the DPLL per m5x2 divider allows HW 
auto-gating of the clock.

So OPTCLKEN_DSSCLK is in a way a dummy bit which when set, ensures that 
the M5 divider doesn't auto gate. So it isn't exactly a enable/disable bit.

In our tree(2.6.38-rc series), HW auto gating bit was disabled for m5x2 
divider, and hence, it never mattered to us if OPTCLKEN_DSSCLK was 
enabled or disabled. We went on happily without bothering about this opt 
clock.

When things got merged in mainline, the HW auto gating for m5x2 came 
into picture(HSDIVIDER_CLKOUT2_GATE_CTRL in CM_DIV_M5_DPLL_PER), and 
then suddenly dss_dss_fclk became super crucial, and a register access 
depended on it. This was the main reason of the confusion I guess.

Archit

>
> Yes, that was confirmed by HW folks, as soon as you have one functional
> clock active, you can disable the other one.
>
>>> That's why the hwmod fmwk cannot choose for you which one you will need
>>> depending of your panel. It is up to the driver to select the correct one.
>>
>> But isn't that DSS internal thing? (I'm looking again at the DSS clock
>> tree figure). DSS_FCLK from the PRCM should always be the same from
>> DSS's point of view, it doesn't change. If we use the clock from DSI
>> PLL, we'll get clock for LCD2_CLK, LCD1_CLK and DISPC_FCLK. But
>> DSS_FCLK/DSS_CLK is always the same.
>
> Yes, but there is no other need for the DSS_CLK beside these internal
> nodes and the DSI engines. So you can shut it down as soon as an
> alternate clock is available (PLL1_CLK1 or PLL2_CLK1 in your case).
>
>>>>>> If "dss_fck" would control DSS_FCLK and "dss_ick" would control
>>>>>> MODULEMODE, they would be about the same as the clocks in OMAP2 and 3,
>>>>>> and we wouldn't need any omap4 spesific trickery in the DSS driver.
>>>>>> ("dss_dss_clk" wouldn't be needed).
>>>>>
>>>>> You cannot play with iclk, because this clock is supposed to be handled
>>>>> automatically by the HW. This was the case on OMAP2&    3 as well BTW.
>>>>
>>>> Right.
>>>>
>>>> But now we have 1) dss_fck, which isn't the DSS_FCLK in TRM, and in fact
>>>> not fck at all, or even clock at all 2) dss_dss_clk, a new clock whose
>>>> name doesn't match to anything in TRM, and is in fact the DSS_FCLK.
>>>>
>>>> So they both sound like they are confusingly named.
>>>
>>> The naming convention is simple: opt clock are using the module name +
>>> the original clock name: "dss_XXX", whereas the modulemode is using the
>>> module name + fck, because it is for the moment similar to the fclk we
>>> have on simpler module. iclk_en is not exposed at all on OMAP4.
>>>
>>> optional clocks:
>>>
>>> static struct clk dss_sys_clk = {
>>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>>> 	.enable_bit	= OMAP4430_OPTFCLKEN_SYS_CLK_SHIFT,
>>>
>>> static struct clk dss_tv_clk = {
>>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>>> 	.enable_bit	= OMAP4430_OPTFCLKEN_TV_CLK_SHIFT,
>>>
>>> static struct clk dss_dss_clk = {
>>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>>> 	.enable_bit	= OMAP4430_OPTFCLKEN_DSSCLK_SHIFT,
>>>
>>> static struct clk dss_48mhz_clk = {
>>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>>> 	.enable_bit	= OMAP4430_OPTFCLKEN_48MHZ_CLK_SHIFT,
>>>
>>>
>>> MODULEMODE:
>>>
>>> static struct clk dss_fck = {
>>> 	.enable_reg	= OMAP4430_CM_DSS_DSS_CLKCTRL,
>>> 	.enable_bit	= OMAP4430_MODULEMODE_SWCTRL,
>>
>> Okay, the naming makes sense now that you explained the convention.
>>
>> I think I'm finally starting to get this =).
>>
>>>> If we had dss_fck matching to DSS_FCLK and dss_ick for MODULEMODE, they
>>>> would, in my opinion, be much more understandable and in many ways
>>>> relate to the way things are in the HW. Additionally this would match
>>>> the way clocks are on OMAP2/3 and things would just work.
>>>
>>> No, because you should not have to play with iclk at all.
>>> BTW, for my point of view you have such issue because your are still not
>>> using the pm_runtime API.
>>> As soon as you will switch to pm_runtime, iclk_en and fclk_en (OMAP2&3
>>> name) or MODULEMODE will be handle by the fmwk. The driver will just
>>> have to handle the optional clock.
>>>
>>>> So while I understand that neither the current way and my suggestion
>>>> exactly match the HW, and also that things are not ready yet and the
>>>> clocks will change, I don't understand why such a strange method to name
>>>> the clocks was used, when there's a simpler and backward compatible way.
>>>
>>> The point is that this automated method works for every IPs in the OMAP
>>> except the DSS that is not managing its clocks like other modules.
>>
>> With this do you mean that the SW does not manage clocks like other
>> modules (should use pm_runtime?), or the HW is somehow different?
>>
>>> Moreover, since the clock fmwk is creating aliases for these clock
>>> nodes, you should never see at all that dss_dss_clk node that seems to
>>> bother you.
>>
>> Oh, that doesn't bother me =). What bothers me is that DSS did not
>> suddenly work because the clocks were set up in this manner.
>>
>> Perhaps the aliases should have been setup differently, at least
>> temporarily, to get things working more easily.
>>
>> Currently we have these aliases for OMAP4:
>>
>> "dss_clk" ->   dss_dss_clk
>> "fck" ->   dss_fck
>> "ick" ->   dummy_ck
>>
>> If that would be changed to:
>>
>> "fck" ->   dss_dss_clk
>> "ick ->   dss_fck
>>
>> The driver would work the same way for all OMAPs.
>>
>>>> And if this current way to handle OMAP4 DSS clocks is for some reason
>>>> better and can't be changed, could the OMAP2/3 clocks also be changed to
>>>> keep things consistent between OMAPs? Because the inconsistency between
>>>> platforms is the biggest problem for me.
>>>
>>> As I said previsously, pm_runtime should fix these issues.
>>
>> Ok. I really need to find time to look at that. Sumit has been working
>> on it, I hope it'll be ready soon =).
>>
>> Anyway. To get things working for rc2 (DSS currently crashes due to not
>> enabling dss_clk) we need to either:
>> 1) Change the clock aliases as above
>
> Changing the clock alias for my point of view is like hacking the HW
> definition to fit your need. Even if this is temporary, I do not like
> that approach.
>
>> 2) Add something like Sumit's patch (attached) to handle dss_clk. While
>> the patch is not that complex, it feels rather hacky.
>
> Yes, I'd rather hack that issue internally, because this is something
> you should fix as soon as you switch to PM runtime.
>
>> What's your opinion?
>
> Definitively #2.
>
> Benoit
>
>

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

Hi Archit,

On 3/31/2011 8:42 AM, Taneja, Archit wrote:
> On Wednesday 30 March 2011 06:51 PM, Cousson, Benoit wrote:
>> On 3/30/2011 2:58 PM, Valkeinen, Tomi wrote:
>>> On Wed, 2011-03-30 at 14:12 +0200, Cousson, Benoit wrote:
>>>> On 3/30/2011 1:03 PM, Valkeinen, Tomi wrote:
>>>>> On Wed, 2011-03-30 at 11:32 +0200, Cousson, Benoit wrote:
>>>>>> Hi Tomi,
>>>>>>
>>>>>> On 3/30/2011 8:48 AM, Valkeinen, Tomi wrote:
>>>>>>> Hi Benoit, Paul,
>>>>>>>
>>>>>>> I've been discussing with Sumit and Archit to understand how the DSS
>>>>>>> clocks are set up on OMAP4. I think I now have some idea how things
>>>>>>> work, but I'm still at loss why things are the way they are.
>>>>>>>
>>>>>>> So, if I look at OMAP4 TRM, Figure 10-4 DSS Clock Tree, there are two
>>>>>>> clocks in PRCM block that are relevant to this discussion: DSS_L3_ICLK
>>>>>>> and DSS_FCLK. To my understanding DSS_L3_ICLK is not really
>>>>>>> controllable, but it is affected by MODULEMODE bit.
>>>>>>>
>>>>>>> Then we have two relevant clocks defined in clock44xx_data.c: dss_fck
>>>>>>> and dss_dss_clk. dss_fck controls the MODULEMODE bit, and dss_dss_clk is
>>>>>>> the TRM's DSS_FCLK.
>>>>>>>
>>>>>>> Was that correct?
>>>>>>
>>>>>> Yes. For the moment, but this is not the final state.
>>>>>>
>>>>>>> If so, from DSS driver's perspective, the dss_fck sounds very much like
>>>>>>> an interface clock (it's always needed when DSS is used) and dss_dss_clk
>>>>>>> sounds very much like functional clock (it's always needed, except if
>>>>>>> DSI PLL is used for DSS functional clock).
>>>>
>>>> dss_dss_fck is one of the possible functional clocks, hence the optional
>>>> attribute. You can run the DSS only for sys_clk is you want.
>>>
>>> We can? I remember this was possible on OMAP2, but I can't seem to find
>>> it on OMAP4 TRM...
>>>
>>> Ah right, you mean sys_clk goes to DSI PLL, and the output from DSI PLL
>>> can be used as functional clock?
>>
>> Yes, you're right, maybe we can still use the sys_clk directly with a
>> parallel QVGA LCD like on OMAP2:-)
>>
>>> We _always_ need DSS_FCLK to get DSS up and running, and to configure
>>> DSI PLL if we want to get the clocks from there. So it's not quite that
>>> optional. But it's true that we can turn it off later.
>
> Just wanted to clarify the issue for myself and summarise:
>
> hwmod and pm runtime ensures that the MODULEMODE bits are set, and
> technically, that should be enough for a module to get up and running.
> But because of the strange design of DSS HW, we need an additional clock
> (DSS_CLK at bootup, could be something else later on) to access DSS
> registers. So we need to enable dss_dss_fclk in our driver in the
> beginning itself to access a register, hence Sumit's patch is needed.
>
> The strange thing is that if dss_dss_fclk is
> off(OMAP4430_OPTFCLKEN_DSSCLK bit is 0) it doesn't mean that the clock
> is surely OFF. Its only OFF when the DPLL per m5x2 divider allows HW
> auto-gating of the clock.

Hehe, welcome to the PRCM world :-)

> So OPTCLKEN_DSSCLK is in a way a dummy bit which when set, ensures that
> the M5 divider doesn't auto gate. So it isn't exactly a enable/disable bit.

It is the case for most clocks in the system. You know when it is 
enabled, but it will be disabled only when the clock domain or in that 
case the parent clock will do HW auto gating.
In this case there is no intermediate gating because the DSS_CLK is the 
only user of the M5 HW divider output, so gating only the parent is enough.

> In our tree(2.6.38-rc series), HW auto gating bit was disabled for m5x2
> divider, and hence, it never mattered to us if OPTCLKEN_DSSCLK was
> enabled or disabled. We went on happily without bothering about this opt
> clock.
>
> When things got merged in mainline, the HW auto gating for m5x2 came
> into picture(HSDIVIDER_CLKOUT2_GATE_CTRL in CM_DIV_M5_DPLL_PER), and
> then suddenly dss_dss_fclk became super crucial, and a register access
> depended on it. This was the main reason of the confusion I guess.

That make sense now, in theory, all these HS divider should be autogated 
by default, it was not the case previously because we were still in a 
non-PM optimize mode. That's why you have to control the clock at your 
level to ensure that the HW will not gate the parent clock.

Benoit

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Wed, 2011-03-30 at 15:21 +0200, Cousson, Benoit wrote:
> On 3/30/2011 2:58 PM, Valkeinen, Tomi wrote:

> > Anyway. To get things working for rc2 (DSS currently crashes due to not
> > enabling dss_clk) we need to either:
> > 1) Change the clock aliases as above
> 
> Changing the clock alias for my point of view is like hacking the HW 
> definition to fit your need. Even if this is temporary, I do not like 
> that approach.
> 
> > 2) Add something like Sumit's patch (attached) to handle dss_clk. While
> > the patch is not that complex, it feels rather hacky.
> 
> Yes, I'd rather hack that issue internally, because this is something 
> you should fix as soon as you switch to PM runtime.
> 
> > What's your opinion?
> 
> Definitively #2.

Ok, let's go with that. I don't like it but hopefully pm_runtime will
clear the mess from dss driver.

Thanks for your time explaining the clock setup to me =)

 Tomi

Re: OMAP4 DSS clock setup

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1983 bytes --]

Hi Tomi, Benoît, et al,

On Wed, 30 Mar 2011, Tomi Valkeinen wrote:

> Currently we have these aliases for OMAP4:
> 
> "dss_clk" -> dss_dss_clk
> "fck" -> dss_fck
> "ick" -> dummy_ck
>
> If that would be changed to:
>
> "fck" -> dss_dss_clk
> "ick -> dss_fck
>
> The driver would work the same way for all OMAPs.

This looks reasonable to me, and seems to match the TRM's Figure 10-4 "DSS 
Clock Tree".

The current OMAP4 clock data name "dss_fck" is just kind of a fake name 
for that clock in the OMAP4 context.

> Anyway. To get things working for rc2 (DSS currently crashes due to not
> enabling dss_clk) we need to either:
> 1) Change the clock aliases as above
> 2) Add something like Sumit's patch (attached) to handle dss_clk. While
> the patch is not that complex, it feels rather hacky.

Yeah, that patch looks like a hack to me, especially something like 
this:

+       if (pdata->opt_clock_available("dss_clk")) {

Based on the E-mail thread so far, I'd say that changing the clock aliases 
is the way to go for right now.  The clock aliases are not hardware data; 
they just control how the clock hardware is mapped to the drivers.

Of course, at some point soon, those clock aliases are going to go away.  
But hopefully you all will have converted the driver over to PM runtime at 
that point.

Once that happens, there's another problem: omap_hwmod_enable() is defined 
that the hardware registers should be accessible by the MPU after it 
completes.  So by that definition, the hwmod code should be 
enabling/disabling that dss_clk clock too when it enables/idles/shuts down 
the hwmod.  Probably we'd need to mark that struct omap_hwmod_opt_clk with 
some flag.  Then we'd need some kind of way for the DSS to tell the hwmod 
code whether it is or isn't reliant on the PRCM-provided functional clock 
for its internal functional clock.  Maybe something like 
omap_hwmod_{release,require}_system_fclk()?


- Paul

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

Hi Paul, Benoit,

On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:

> Based on the E-mail thread so far, I'd say that changing the clock aliases 
> is the way to go for right now.  The clock aliases are not hardware data; 
> they just control how the clock hardware is mapped to the drivers.

I'd very much prefer this option. Below is a patch for this.

If Benoit doesn't complain too much about this, I'd like to get this
merged as soon as possible, as OMAP4 DSS is currently crashing in the
mainline kernel.

I can either handle it myself if I get your acks, or you can send a pull
request for this if you have some other patches going in also.

> Of course, at some point soon, those clock aliases are going to go away.  
> But hopefully you all will have converted the driver over to PM runtime at 
> that point.
> 
> Once that happens, there's another problem: omap_hwmod_enable() is defined 
> that the hardware registers should be accessible by the MPU after it 
> completes.  So by that definition, the hwmod code should be 
> enabling/disabling that dss_clk clock too when it enables/idles/shuts down 
> the hwmod.  Probably we'd need to mark that struct omap_hwmod_opt_clk with 
> some flag.  Then we'd need some kind of way for the DSS to tell the hwmod 
> code whether it is or isn't reliant on the PRCM-provided functional clock 
> for its internal functional clock.  Maybe something like 
> omap_hwmod_{release,require}_system_fclk()?

Hmm, right. I guess no other HW module has clock setups like this?

Currently DSS can use clk_enable/disable() for the system fclk when its
using DSI PLL for the fclk. So omap_hwmod_{release,require}_system_fclk
sounds like a simple solution to this.

Not directly related, but something I've been wondering about is how to
abstract the DSI/HDMI PLLs in DSS. What do you think, would it be
possible/worth it to create struct clks for the clocks coming out of
those PLLs? These would, of course, be DSS internal clks. I'm not very
familiar with the clock framework, so I don't really have any idea what
this would require and what would be the pros and cons.

---

>From f9999ceb48b2e22217dccc85b33362b6a17e5a00 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Mon, 4 Apr 2011 09:26:19 +0300
Subject: [PATCH] OMAP4: clock data: Change DSS clock aliases

DSS driver has used fck and ick clocks on OMAP2/3 to get DSS HW up and
running, and also to get the pixel clock's source clock rate from the
fck.

On OMAP4 the clock data is set up in a different way, as there's no ick,
dss_fck points to a fake clock which just affects DSS's MODULEMODE, and
dss_dss_clk if the DSS_FCK.

>From DSS driver's point of view the dss_fck sounds like an ick, and
dss_dss_clk is the fck. While this is not entirely correct from HW point
of view, especially for the ick, configuring the clock aliases that way
makes DSS "just work" with OMAP4's clock setup.

In the (hopefully near) future DSS driver will be reworked to use
pm_runtime support which should clean up the clock code.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/clock44xx_data.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
index 276992d..8c96567 100644
--- a/arch/arm/mach-omap2/clock44xx_data.c
+++ b/arch/arm/mach-omap2/clock44xx_data.c
@@ -3116,14 +3116,9 @@ static struct omap_clk omap44xx_clks[] = {
 	CLK(NULL,	"dsp_fck",			&dsp_fck,	CK_443X),
 	CLK("omapdss_dss",	"sys_clk",			&dss_sys_clk,	CK_443X),
 	CLK("omapdss_dss",	"tv_clk",			&dss_tv_clk,	CK_443X),
-	CLK("omapdss_dss",	"dss_clk",			&dss_dss_clk,	CK_443X),
 	CLK("omapdss_dss",	"video_clk",			&dss_48mhz_clk,	CK_443X),
-	CLK("omapdss_dss",	"fck",				&dss_fck,	CK_443X),
-	/*
-	 * On OMAP4, DSS ick is a dummy clock; this is needed for compatibility
-	 * with OMAP2/3.
-	 */
-	CLK("omapdss_dss",	"ick",				&dummy_ck,	CK_443X),
+	CLK("omapdss_dss",	"fck",				&dss_dss_clk,	CK_443X),
+	CLK("omapdss_dss",	"ick",				&dss_fck,	CK_443X),
 	CLK(NULL,	"efuse_ctrl_cust_fck",		&efuse_ctrl_cust_fck,	CK_443X),
 	CLK(NULL,	"emif1_fck",			&emif1_fck,	CK_443X),
 	CLK(NULL,	"emif2_fck",			&emif2_fck,	CK_443X),
-- 
1.7.1

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

Paul, Benoit,

On Mon, 2011-04-04 at 09:53 +0300, Tomi Valkeinen wrote:
> Hi Paul, Benoit,
> 
> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
> 
> > Based on the E-mail thread so far, I'd say that changing the clock aliases 
> > is the way to go for right now.  The clock aliases are not hardware data; 
> > they just control how the clock hardware is mapped to the drivers.
> 
> I'd very much prefer this option. Below is a patch for this.
> 
> If Benoit doesn't complain too much about this, I'd like to get this
> merged as soon as possible, as OMAP4 DSS is currently crashing in the
> mainline kernel.
> 
> I can either handle it myself if I get your acks, or you can send a pull
> request for this if you have some other patches going in also.

Ping. Can I get an ack/nack from you for the patch below?

 Tomi

> > Of course, at some point soon, those clock aliases are going to go away.  
> > But hopefully you all will have converted the driver over to PM runtime at 
> > that point.
> > 
> > Once that happens, there's another problem: omap_hwmod_enable() is defined 
> > that the hardware registers should be accessible by the MPU after it 
> > completes.  So by that definition, the hwmod code should be 
> > enabling/disabling that dss_clk clock too when it enables/idles/shuts down 
> > the hwmod.  Probably we'd need to mark that struct omap_hwmod_opt_clk with 
> > some flag.  Then we'd need some kind of way for the DSS to tell the hwmod 
> > code whether it is or isn't reliant on the PRCM-provided functional clock 
> > for its internal functional clock.  Maybe something like 
> > omap_hwmod_{release,require}_system_fclk()?
> 
> Hmm, right. I guess no other HW module has clock setups like this?
> 
> Currently DSS can use clk_enable/disable() for the system fclk when its
> using DSI PLL for the fclk. So omap_hwmod_{release,require}_system_fclk
> sounds like a simple solution to this.
> 
> Not directly related, but something I've been wondering about is how to
> abstract the DSI/HDMI PLLs in DSS. What do you think, would it be
> possible/worth it to create struct clks for the clocks coming out of
> those PLLs? These would, of course, be DSS internal clks. I'm not very
> familiar with the clock framework, so I don't really have any idea what
> this would require and what would be the pros and cons.
> 
> ---
> 
> From f9999ceb48b2e22217dccc85b33362b6a17e5a00 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Mon, 4 Apr 2011 09:26:19 +0300
> Subject: [PATCH] OMAP4: clock data: Change DSS clock aliases
> 
> DSS driver has used fck and ick clocks on OMAP2/3 to get DSS HW up and
> running, and also to get the pixel clock's source clock rate from the
> fck.
> 
> On OMAP4 the clock data is set up in a different way, as there's no ick,
> dss_fck points to a fake clock which just affects DSS's MODULEMODE, and
> dss_dss_clk if the DSS_FCK.
> 
> From DSS driver's point of view the dss_fck sounds like an ick, and
> dss_dss_clk is the fck. While this is not entirely correct from HW point
> of view, especially for the ick, configuring the clock aliases that way
> makes DSS "just work" with OMAP4's clock setup.
> 
> In the (hopefully near) future DSS driver will be reworked to use
> pm_runtime support which should clean up the clock code.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  arch/arm/mach-omap2/clock44xx_data.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach-omap2/clock44xx_data.c
> index 276992d..8c96567 100644
> --- a/arch/arm/mach-omap2/clock44xx_data.c
> +++ b/arch/arm/mach-omap2/clock44xx_data.c
> @@ -3116,14 +3116,9 @@ static struct omap_clk omap44xx_clks[] = {
>  	CLK(NULL,	"dsp_fck",			&dsp_fck,	CK_443X),
>  	CLK("omapdss_dss",	"sys_clk",			&dss_sys_clk,	CK_443X),
>  	CLK("omapdss_dss",	"tv_clk",			&dss_tv_clk,	CK_443X),
> -	CLK("omapdss_dss",	"dss_clk",			&dss_dss_clk,	CK_443X),
>  	CLK("omapdss_dss",	"video_clk",			&dss_48mhz_clk,	CK_443X),
> -	CLK("omapdss_dss",	"fck",				&dss_fck,	CK_443X),
> -	/*
> -	 * On OMAP4, DSS ick is a dummy clock; this is needed for compatibility
> -	 * with OMAP2/3.
> -	 */
> -	CLK("omapdss_dss",	"ick",				&dummy_ck,	CK_443X),
> +	CLK("omapdss_dss",	"fck",				&dss_dss_clk,	CK_443X),
> +	CLK("omapdss_dss",	"ick",				&dss_fck,	CK_443X),
>  	CLK(NULL,	"efuse_ctrl_cust_fck",		&efuse_ctrl_cust_fck,	CK_443X),
>  	CLK(NULL,	"emif1_fck",			&emif1_fck,	CK_443X),
>  	CLK(NULL,	"emif2_fck",			&emif2_fck,	CK_443X),

Re: OMAP4 DSS clock setup

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2767 bytes --]

Hello Tomi, Benoît,

On Mon, 4 Apr 2011, Tomi Valkeinen wrote:

> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
> 
> > Based on the E-mail thread so far, I'd say that changing the clock aliases 
> > is the way to go for right now.  The clock aliases are not hardware data; 
> > they just control how the clock hardware is mapped to the drivers.
> 
> I'd very much prefer this option. Below is a patch for this.
> 
> If Benoit doesn't complain too much about this, I'd like to get this
> merged as soon as possible, as OMAP4 DSS is currently crashing in the
> mainline kernel.

I will queue your clock aliases patch and attempt to merge it upstream for 
the -rc series.  I am not happy to be disagreeing with Benoît here, but 
this is the rationale that I am using.  (Benoît, Tomi, please feel free to 
correct if there is something blatantly false below.)

1. The integration of the DSS module is an unusual case.  The
   MODULEMODE bits for the DSS bits do not control the DSS "main"
   functional clock (the clock that is needed to access device
   registers); instead, they only control the DSS interface clock.
   (This is because the DSS can use a functional clock that is not
   provided by the OMAP core.)  So calling the DSS MODULEMODE struct
   clk "dss_fck" is not really correct, since it is really controlling
   the interface clock.

2. This patch does not create a precedent for hacking around general
   driver clocking problems in the clock or clock data.  This is only
   needed because the MODULEMODE bits don't control the module
   functional clock in this case.  As I understand it, the only other
   device that this problem could occur with is McBSP, due to
   mcbsp_clks.

3. The existing DSS2 driver clock design apparently works fine when
   this change is implemented[1], so no driver changes will be needed.

4. The proposed DSS driver patch to work around this uses a nasty hack
   that really should be NACK'ed[2].

All this said, we may not be able to merge this change upstream, due to 
the recent unhappiness about the clock data changes.  If Linus rejects it, 
you'll need a "Plan B."

...

Also, I hope you and the other DSS hackers can finish the PM runtime
conversion of the DSS driver soon.  Ideally before any new DSS
features are added.  We really need to be able to separate the OMAP
integration details from the drivers, and right now, hwmod and PM
runtime are the best way we have to do that.

Comments welcome.


- Paul


1. Valkeinen, Tomi.  _Re: OMAP4 DSS clock setup_.  E-mail to 
linux-omap@vger.kernel.org mailing list dated Wed, 30 Mar 2011 05:59:06 
-0700. 
<http://www.mail-archive.com/linux-omap@vger.kernel.org/msg47665.html>

2. ibid.

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

Hi Paul,

On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote:
> Hello Tomi, Benoît,
> 
> On Mon, 4 Apr 2011, Tomi Valkeinen wrote:
> 
> > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
> > 
> > > Based on the E-mail thread so far, I'd say that changing the clock aliases 
> > > is the way to go for right now.  The clock aliases are not hardware data; 
> > > they just control how the clock hardware is mapped to the drivers.
> > 
> > I'd very much prefer this option. Below is a patch for this.
> > 
> > If Benoit doesn't complain too much about this, I'd like to get this
> > merged as soon as possible, as OMAP4 DSS is currently crashing in the
> > mainline kernel.
> 
> I will queue your clock aliases patch and attempt to merge it upstream for 
> the -rc series.  I am not happy to be disagreeing with Benoît here, but 
> this is the rationale that I am using.  (Benoît, Tomi, please feel free to 
> correct if there is something blatantly false below.)
> 
> 1. The integration of the DSS module is an unusual case.  The
>    MODULEMODE bits for the DSS bits do not control the DSS "main"
>    functional clock (the clock that is needed to access device
>    registers); instead, they only control the DSS interface clock.
>    (This is because the DSS can use a functional clock that is not
>    provided by the OMAP core.)  So calling the DSS MODULEMODE struct
>    clk "dss_fck" is not really correct, since it is really controlling
>    the interface clock.

If we don't apply the patch, I think the clock aliases are still broken.
Let's forget the ick/fck thing for a second, and just think the clock
from PRCM which is used as the source clock for pixel clock. That is
currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This
cannot be right, can it? From DSS's point of view that is the same
clock, it should clearly have the same alias on all platforms. I don't
care what the name is as long as it's consistent on all platforms.

And I have to say I still don't quite get it what is the problem with
"fck" name. Or is that meant to be a clock which enables the registers
on the module, and not the clock used for the pixel clock? If so, I'm
fine with having "fck" to be what it is currently, but then we need a
new name for the clock used for pixel clock, which is consistent on all
platforms.

> 2. This patch does not create a precedent for hacking around general
>    driver clocking problems in the clock or clock data.  This is only
>    needed because the MODULEMODE bits don't control the module
>    functional clock in this case.  As I understand it, the only other
>    device that this problem could occur with is McBSP, due to
>    mcbsp_clks.
> 
> 3. The existing DSS2 driver clock design apparently works fine when
>    this change is implemented[1], so no driver changes will be needed.
> 
> 4. The proposed DSS driver patch to work around this uses a nasty hack
>    that really should be NACK'ed[2].
> 
> All this said, we may not be able to merge this change upstream, due to 
> the recent unhappiness about the clock data changes.  If Linus rejects it, 
> you'll need a "Plan B."
> 
> ...
> 
> Also, I hope you and the other DSS hackers can finish the PM runtime
> conversion of the DSS driver soon.  Ideally before any new DSS
> features are added.  We really need to be able to separate the OMAP
> integration details from the drivers, and right now, hwmod and PM
> runtime are the best way we have to do that.

I also started to look at the PM runtime, but it doesn't fix the issue
with the inconsistent clock name I described above.

I also have some questions regarding PM runtime, perhaps I'll just put
them here as they are slightly related:

- Should every DSS module handle their clocks independently, i.e. should
VENC get its own clocks and DSI should get its own? If so, we need a
bunch of new clock aliases so the devices can get their clocks.

- Should every DSS module have their own PM runtime handling? (actually
related to the question above)

- If the modules are handled separately, how should the dependencies be
handled? For example, dss_core's reset will reset all the other modules
also, and most of the submodules need functions from dss_core and
dss_dispc. So should, say, dss_dsi then call functions in core and dispc
to "get" them, i.e. increase their pm runtime use count?

- There seems to be some child/parent relationships in PM runtime.
Should dss_core be the parent for the rest of the DSS modules? This
would at least solve the reset issue easily, I guess.

- How does saving/restoring the registers for OFF mode go with PM
runtime? Should the registers be saved in runtime_suspend(), and
restored in runtime_resume()? Can/should
omap_pm_get_dev_context_loss_count() still be used to optimize the
restoring?

 Tomi


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

Re: OMAP4 DSS clock setup

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3389 bytes --]

Hi

On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote:
> > On Mon, 4 Apr 2011, Tomi Valkeinen wrote:
> > 
> > > On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
> > > 
> > > > Based on the E-mail thread so far, I'd say that changing the clock aliases 
> > > > is the way to go for right now.  The clock aliases are not hardware data; 
> > > > they just control how the clock hardware is mapped to the drivers.
> > > 
> > > I'd very much prefer this option. Below is a patch for this.
> > > 
> > > If Benoit doesn't complain too much about this, I'd like to get this
> > > merged as soon as possible, as OMAP4 DSS is currently crashing in the
> > > mainline kernel.
> > 
> > I will queue your clock aliases patch and attempt to merge it upstream for 
> > the -rc series.  I am not happy to be disagreeing with Benoît here, but 
> > this is the rationale that I am using.  (Benoît, Tomi, please feel free to 
> > correct if there is something blatantly false below.)
> > 
> > 1. The integration of the DSS module is an unusual case.  The
> >    MODULEMODE bits for the DSS bits do not control the DSS "main"
> >    functional clock (the clock that is needed to access device
> >    registers); instead, they only control the DSS interface clock.
> >    (This is because the DSS can use a functional clock that is not
> >    provided by the OMAP core.)  So calling the DSS MODULEMODE struct
> >    clk "dss_fck" is not really correct, since it is really controlling
> >    the interface clock.
> 
> If we don't apply the patch, I think the clock aliases are still broken.
> Let's forget the ick/fck thing for a second, and just think the clock
> from PRCM which is used as the source clock for pixel clock. That is
> currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This
> cannot be right, can it? From DSS's point of view that is the same
> clock, it should clearly have the same alias on all platforms. I don't
> care what the name is as long as it's consistent on all platforms.

Yes, I agree.  That is another good point.

> And I have to say I still don't quite get it what is the problem with
> "fck" name.

After the hwmod/PM runtime conversion, there shouldn't be any clock 
aliases left that are simply called "fck", because it is almost a 
meaningless name.

> Or is that meant to be a clock which enables the registers
> on the module,

After the hwmod/PM runtime conversion, that should have an alias name of 
"main" that is automatically added by the omap_device code.  (Note that it 
does not appear to do so now; that is a bug that needs be fixed.)

> and not the clock used for the pixel clock? If so, I'm fine with having 
> "fck" to be what it is currently, but then we need a new name for the 
> clock used for pixel clock, which is consistent on all platforms.

If there is a separate PRCM-provided clock used only for the pixel clock, 
then that clock should have an alias name of "system_pixel_ck" or 
something similar that is meaningful to the DSS driver.  I think the 
problem in this case is that "dss_dss_clk" is (optionally) used for two 
purposes: optionally as a "main PRCM-provided functional clock" and 
optionally as a system-provided pixel clock.

I'll reply to the rest of your mail in a separate E-mail...


- Paul

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Fri, 2011-04-08 at 08:55 -0600, Paul Walmsley wrote:
> Hi
> 
> On Fri, 8 Apr 2011, Tomi Valkeinen wrote:

> > and not the clock used for the pixel clock? If so, I'm fine with having 
> > "fck" to be what it is currently, but then we need a new name for the 
> > clock used for pixel clock, which is consistent on all platforms.
> 
> If there is a separate PRCM-provided clock used only for the pixel clock, 
> then that clock should have an alias name of "system_pixel_ck" or 
> something similar that is meaningful to the DSS driver.  I think the 
> problem in this case is that "dss_dss_clk" is (optionally) used for two 
> purposes: optionally as a "main PRCM-provided functional clock" and 
> optionally as a system-provided pixel clock.

Not only for pixel clock, but in the end it'll come out as pixel clock.
I'm not sure what exactly is a "functional clock" here. I mean, one
could think it as a basic functionality of DSS to read the pixels,
manipulate them, and output them (with the rate of the pixel clock).

However, I think there is one difference between the clock used just to
enable the DSS registers, and the one used to output pixels: we need to
be able to adjust the rate of the clock. Thus we need to have a common
(omap2/3/4) clock name for it to be able to clk_get() it.

Should that clock name be just the "main" clock provided automatically,
or something else?

 Tomi

Re: OMAP4 DSS clock setup

[Paul Walmsley]

Hi

On Mon, 11 Apr 2011, Tomi Valkeinen wrote:
> On Fri, 2011-04-08 at 08:55 -0600, Paul Walmsley wrote:
> > On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> 
> > > and not the clock used for the pixel clock? If so, I'm fine with having 
> > > "fck" to be what it is currently, but then we need a new name for the 
> > > clock used for pixel clock, which is consistent on all platforms.
> > 
> > If there is a separate PRCM-provided clock used only for the pixel clock, 
> > then that clock should have an alias name of "system_pixel_ck" or 
> > something similar that is meaningful to the DSS driver.  I think the 
> > problem in this case is that "dss_dss_clk" is (optionally) used for two 
> > purposes: optionally as a "main PRCM-provided functional clock" and 
> > optionally as a system-provided pixel clock.
> 
> Not only for pixel clock, but in the end it'll come out as pixel clock.
> I'm not sure what exactly is a "functional clock" here. I mean, one
> could think it as a basic functionality of DSS to read the pixels,
> manipulate them, and output them (with the rate of the pixel clock).

Broadly speaking, a functional clock is defined negatively - 'anything 
that's not an interface clock.'  That's why the term 'functional clock' is 
not so useful by itself.  But when we talk about a module's 'main 
functional clock,' that means the functional clock that is needed for 
register accesses to work. 
 
> However, I think there is one difference between the clock used just to
> enable the DSS registers, and the one used to output pixels: we need to
> be able to adjust the rate of the clock. Thus we need to have a common
> (omap2/3/4) clock name for it to be able to clk_get() it.
>
> Should that clock name be just the "main" clock provided automatically,
> or something else?

Are you referring here to the system DPLL and its output dividers, or are 
you referring to the DSS module's internal dividers?


- Paul

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Mon, 2011-04-11 at 12:20 -0600, Paul Walmsley wrote:
> Hi
> 
> On Mon, 11 Apr 2011, Tomi Valkeinen wrote:

> > However, I think there is one difference between the clock used just to
> > enable the DSS registers, and the one used to output pixels: we need to
> > be able to adjust the rate of the clock. Thus we need to have a common
> > (omap2/3/4) clock name for it to be able to clk_get() it.
> >
> > Should that clock name be just the "main" clock provided automatically,
> > or something else?
> 
> Are you referring here to the system DPLL and its output dividers, or are 
> you referring to the DSS module's internal dividers?

The system DPLL and its divider. If we are using the dss_dss_clk as
fclk, we need to adjust it depending on the required pixel clock and use
cases (e.g. some scaling factors may need higher fclk).

 Tomi

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

Hi Tomi,

On 4/8/2011 7:51 AM, Valkeinen, Tomi wrote:
> Hi Paul,
> 
> On Thu, 2011-04-07 at 13:27 -0600, Paul Walmsley wrote:
>> Hello Tomi, Benoît,
>>
>> On Mon, 4 Apr 2011, Tomi Valkeinen wrote:
>>
>>> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
>>>
>>>> Based on the E-mail thread so far, I'd say that changing the clock aliases
>>>> is the way to go for right now.  The clock aliases are not hardware data;
>>>> they just control how the clock hardware is mapped to the drivers.
>>>
>>> I'd very much prefer this option. Below is a patch for this.
>>>
>>> If Benoit doesn't complain too much about this, I'd like to get this
>>> merged as soon as possible, as OMAP4 DSS is currently crashing in the
>>> mainline kernel.
>>
>> I will queue your clock aliases patch and attempt to merge it upstream for
>> the -rc series.  I am not happy to be disagreeing with Benoît here, but
>> this is the rationale that I am using.  (Benoît, Tomi, please feel free to
>> correct if there is something blatantly false below.)
>>
>> 1. The integration of the DSS module is an unusual case.  The
>>     MODULEMODE bits for the DSS bits do not control the DSS "main"
>>     functional clock (the clock that is needed to access device
>>     registers); instead, they only control the DSS interface clock.
>>     (This is because the DSS can use a functional clock that is not
>>     provided by the OMAP core.)  So calling the DSS MODULEMODE struct
>>     clk "dss_fck" is not really correct, since it is really controlling
>>     the interface clock.
> 
> If we don't apply the patch, I think the clock aliases are still broken.
> Let's forget the ick/fck thing for a second, and just think the clock
> from PRCM which is used as the source clock for pixel clock. That is
> currently called "fck" on OMAP2/3, but "dss_dss_clk" on OMAP4. This
> cannot be right, can it? From DSS's point of view that is the same
> clock, it should clearly have the same alias on all platforms. I don't
> care what the name is as long as it's consistent on all platforms.
> 
> And I have to say I still don't quite get it what is the problem with
> "fck" name. Or is that meant to be a clock which enables the registers
> on the module, and not the clock used for the pixel clock? If so, I'm
> fine with having "fck" to be what it is currently, but then we need a
> new name for the clock used for pixel clock, which is consistent on all
> platforms.

Both can be used for the system clock (sys_clk -> DSI DPLL or dss_dss_clk).

In fact after multiple discussions with DSS and PRCM folks, I realized that OMAP3 and 4 are using exactly the same clock scheme :-(

The confusion in my mind came from change in naming convention in both the PRCM and the DSS at the same time between OMAP3 and 4.

OMAP3          -> OMAP4
dss1_alwon_fck -> dss_dss_clk (from dpll per output in both cases)
dss2_alwon_fck -> dss_sys_clk
dss_ick        -> dss_fck (-> modulemode) that name really needs to be changed to avoid further confusion.


So in fact the OMAP4 aliased should be: 
CLK("omapdss_dss",	"fck",		&dss_dss_clk,	CK_443X),
CLK("omapdss_dss",	"sys_clk",	&dss_sys_clk,	CK_443X),
CLK("omapdss_dss",	"ick",		&dss_fck,	CK_443X),


That will map perfectly what we have on OMAP3:
CLK("omapdss_dss",	"fck",		&dss1_alwon_fck_3430es2, ...),
CLK("omapdss_dss",	"sys_clk",	&dss2_alwon_fck, ...),
CLK("omapdss_dss",	"ick",		&dss_ick_3430es2, ...),


>> 2. This patch does not create a precedent for hacking around general
>>     driver clocking problems in the clock or clock data.  This is only
>>     needed because the MODULEMODE bits don't control the module
>>     functional clock in this case.  As I understand it, the only other
>>     device that this problem could occur with is McBSP, due to
>>     mcbsp_clks.
>>
>> 3. The existing DSS2 driver clock design apparently works fine when
>>     this change is implemented[1], so no driver changes will be needed.
>>
>> 4. The proposed DSS driver patch to work around this uses a nasty hack
>>     that really should be NACK'ed[2].
>>
>> All this said, we may not be able to merge this change upstream, due to
>> the recent unhappiness about the clock data changes.  If Linus rejects it,
>> you'll need a "Plan B."
>>
>> ...
>>
>> Also, I hope you and the other DSS hackers can finish the PM runtime
>> conversion of the DSS driver soon.  Ideally before any new DSS
>> features are added.  We really need to be able to separate the OMAP
>> integration details from the drivers, and right now, hwmod and PM
>> runtime are the best way we have to do that.
> 
> I also started to look at the PM runtime, but it doesn't fix the issue
> with the inconsistent clock name I described above.

No indeed.

> I also have some questions regarding PM runtime, perhaps I'll just put
> them here as they are slightly related:
> 
> - Should every DSS module handle their clocks independently, i.e. should
> VENC get its own clocks and DSI should get its own? If so, we need a
> bunch of new clock aliases so the devices can get their clocks.

For dedicated clock like tv_clk, it probably makes sense, because the other DSS drivers do not have to care about that clock.

> - Should every DSS module have their own PM runtime handling? (actually
> related to the question above)
> 
> - If the modules are handled separately, how should the dependencies be
> handled? For example, dss_core's reset will reset all the other modules
> also, and most of the submodules need functions from dss_core and
> dss_dispc. So should, say, dss_dsi then call functions in core and dispc
> to "get" them, i.e. increase their pm runtime use count?

Are you sure about that? 
The dss_core does not have any reset in the sysonfig (only a status), but the dispc does have one and the dsi as well.
That being said, the dss modules have some issue with the softreset at boot time, so I'm not sure what these bits are really doing.

Regards,
Benoit

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

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Fri, 2011-04-08 at 17:36 +0200, Cousson, Benoit wrote:
> Hi Tomi,
> 
> On 4/8/2011 7:51 AM, Valkeinen, Tomi wrote:

> > - If the modules are handled separately, how should the dependencies be
> > handled? For example, dss_core's reset will reset all the other modules
> > also, and most of the submodules need functions from dss_core and
> > dss_dispc. So should, say, dss_dsi then call functions in core and dispc
> > to "get" them, i.e. increase their pm runtime use count?
> 
> Are you sure about that? 
> The dss_core does not have any reset in the sysonfig (only a status), but the dispc does have one and the dsi as well.

Ah, I might be speaking only of OMAP2/3. I remember somebody saying that
the DSS reset bit on OMAP4 is marked as reserved. But for OMAP3 (and I
think for OMAP2 also) the DSS_SYSCONFIG reset bit will reset also the
rest of the DSS.

 Tomi

Re: OMAP4 DSS clock setup

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6574 bytes --]

Hi Tomi,

On Fri, 8 Apr 2011, Tomi Valkeinen wrote:

> > Also, I hope you and the other DSS hackers can finish the PM runtime
> > conversion of the DSS driver soon.  Ideally before any new DSS
> > features are added.  We really need to be able to separate the OMAP
> > integration details from the drivers, and right now, hwmod and PM
> > runtime are the best way we have to do that.
> 
> I also started to look at the PM runtime, but it doesn't fix the issue
> with the inconsistent clock name I described above.

After the hwmod/PM runtime conversion, I don't think any of the clock 
aliases currently in clock*_data.c should be used by the DSS driver (or by 
anything else on the system, for that matter).  That's because the 
omap_device code should set up the "main" alias for the DSS main 
functional clock[*], as well as the aliases in the optional clock data in 
the OMAP hwmod data files:

static struct omap_hwmod_opt_clk dss_opt_clks[] = {
	{ .role = "sys_clk", .clk = "dss_sys_clk" },
	{ .role = "tv_clk", .clk = "dss_tv_clk" },
	{ .role = "dss_clk", .clk = "dss_dss_clk" },
	{ .role = "video_clk", .clk = "dss_48mhz_clk" },
};

It might be that some of these role names aren't quite accurate and need 
to be changed.  Those are intended to be meaningful to the driver, so 
comments there are definitely welcome.

[*]. The "main" alias should be defined by the omap_device code 
automatically, similarly to how _add_optional_clock_clkdev() does it.  It 
does not do so currently.  This needs to be fixed in the omap_device.c 
code.

> I also have some questions regarding PM runtime, perhaps I'll just put
> them here as they are slightly related:
> 
> - Should every DSS module handle their clocks independently, i.e. should
> VENC get its own clocks and DSI should get its own? If so, we need a
> bunch of new clock aliases so the devices can get their clocks.

If all that driver code needs to do is to enable its main functional clock 
when it is active and disable that clock when the driver is inactive, 
then, no, the drivers shouldn't need their own clock aliases.  Same if the 
driver only needs to get the rate or set the rate on that main functional 
clock, since that alias should be set up automatically.

But if the driver for that submodule needs to control PRCM-provided 
optional clocks, then it will need to have struct omap_hwmod_opt_clk 
entries defined in the hwmod data.

> - Should every DSS module have their own PM runtime handling? (actually
> related to the question above)

Yes, I think so.  From the integration perspective, we are trying to get 
to the point where each omap_device maps to only one hwmod.

> - If the modules are handled separately, how should the dependencies be 
> handled? For example, dss_core's reset will reset all the other modules 
> also, and most of the submodules need functions from dss_core and 
> dss_dispc. So should, say, dss_dsi then call functions in core and dispc 
> to "get" them, i.e. increase their pm runtime use count?

Probably not.

Here is how I would suggest structuring the code.  This is only a naïve 
suggestion; you and your team almost certainly know better than I.

I'd suggest that you separate low-level register access into .c files 
that are targeted specifically for the IP block.  So in the DSS case, 
you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c.  Each one should 
be a separate platform_device and would export symbols.  Hopefully, there 
should be no cross-dependencies between these low-level files.

Then you'd have "higher-level" code that might need to read/write from 
multiple DSS submodules to complete some higher-level operation.  That 
would be at least one separate driver -- say, "dss2" or something -- with 
dependencies on the low-level drivers.  So, for example, when that 
higher-level driver is modprobe'd, Linux would also load the drivers for 
the underlying hardware blocks that it uses.

> - There seems to be some child/parent relationships in PM runtime.
> Should dss_core be the parent for the rest of the DSS modules? This
> would at least solve the reset issue easily, I guess.

Yes, I think that's more accurate, anyway.  Something isn't right with the 
DSS hwmod data.  According to the OMAP4 Public TRM Rev O Table 10-3 "DSS 
Integration," there's a Sonics LX bus lurking in there.  All of the DSS 
submodules should have slave sockets with that Sonics LX bus as their 
master.  And the hwmod associated with the SLX should have an address 
range that covers all of the DSS submodules.  I assume that the logical 
hwmod to associate the SLX with is "dss_core", as you write.

Also, I notice the "CAUTION" boxes in Section 10.1.3 "DSS Register 
Manual", 10.2.7 "Display Controller Register Manual", etc. etc., that say 
that the DSS and submodules should be accessed through the L3 address 
space.  But all of the DSS hwmod register targets are listed as the L4_PER 
variants.  So the hwmod data also doesn't appear to be correct there.  The 
correct approach would be to have both address spaces listed in struct 
omap_hwmod_addr_space arrays, but to mark only the struct 
omap_hwmod_ocp_if entry associated with the L3 bus as OCP_USER_MPU | 
OCP_USER_SDMA[*].

[*]. On the OMAP core code, looks like we'll also want to modify 
omap_hwmod_build_resources() to mark resources with IORESOURCE_DISABLED if 
they are not marked with any OCP_USER_* flags, and patch the functions in 
drivers/base/platform.c to skip any resources with IORESOURCE_DISABLED 
set.

> - How does saving/restoring the registers for OFF mode go with PM
> runtime? Should the registers be saved in runtime_suspend(), and
> restored in runtime_resume()?

If you don't use shadow registers, then I think so, yes, although Kevin 
might know better than I.

> Can/should omap_pm_get_dev_context_loss_count() still be used to 
> optimize the restoring?

Yes.

...

I regret that this process is relatively complicated :-(  As you and/or 
your team works on this, changes to the hwmod/omap_device/clock core will 
almost certainly be needed.  Please don't hesitate to let us know what's 
not working for you, or to try out patches to the core infrastructure to 
fix what you need.  If I don't respond, please just keep pinging.  I 
wasn't able to fully analyze the DSS module when we originally wrote the 
hwmod code, so surely we'll need to fix some bugs or make some changes for 
things to make sense.

best regards,


- Paul

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
> Hi Tomi,
> 
> On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> 
> > > Also, I hope you and the other DSS hackers can finish the PM runtime
> > > conversion of the DSS driver soon.  Ideally before any new DSS
> > > features are added.  We really need to be able to separate the OMAP
> > > integration details from the drivers, and right now, hwmod and PM
> > > runtime are the best way we have to do that.
> > 
> > I also started to look at the PM runtime, but it doesn't fix the issue
> > with the inconsistent clock name I described above.
> 
> After the hwmod/PM runtime conversion, I don't think any of the clock 
> aliases currently in clock*_data.c should be used by the DSS driver (or by 
> anything else on the system, for that matter).  That's because the 
> omap_device code should set up the "main" alias for the DSS main 

When should "main" clock be used by the driver? Or never, and pm_runtime
handles that?

> functional clock[*], as well as the aliases in the optional clock data in 
> the OMAP hwmod data files:
> 
> static struct omap_hwmod_opt_clk dss_opt_clks[] = {
> 	{ .role = "sys_clk", .clk = "dss_sys_clk" },
> 	{ .role = "tv_clk", .clk = "dss_tv_clk" },
> 	{ .role = "dss_clk", .clk = "dss_dss_clk" },
> 	{ .role = "video_clk", .clk = "dss_48mhz_clk" },
> };
> 
> It might be that some of these role names aren't quite accurate and need 
> to be changed.  Those are intended to be meaningful to the driver, so 
> comments there are definitely welcome.

How about OMAP3? It also has an optional functional clock, but that
doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
Should it be there?

It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
hwmods. Does that mean it can never be turned off if DSS is in use?

> [*]. The "main" alias should be defined by the omap_device code 
> automatically, similarly to how _add_optional_clock_clkdev() does it.  It 
> does not do so currently.  This needs to be fixed in the omap_device.c 
> code.
> 
> > I also have some questions regarding PM runtime, perhaps I'll just put
> > them here as they are slightly related:
> > 
> > - Should every DSS module handle their clocks independently, i.e. should
> > VENC get its own clocks and DSI should get its own? If so, we need a
> > bunch of new clock aliases so the devices can get their clocks.
> 
> If all that driver code needs to do is to enable its main functional clock 
> when it is active and disable that clock when the driver is inactive, 
> then, no, the drivers shouldn't need their own clock aliases.  Same if the 
> driver only needs to get the rate or set the rate on that main functional 
> clock, since that alias should be set up automatically.
> 
> But if the driver for that submodule needs to control PRCM-provided 
> optional clocks, then it will need to have struct omap_hwmod_opt_clk 
> entries defined in the hwmod data.

Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
believe.

> > - Should every DSS module have their own PM runtime handling? (actually
> > related to the question above)
> 
> Yes, I think so.  From the integration perspective, we are trying to get 
> to the point where each omap_device maps to only one hwmod.
> 
> > - If the modules are handled separately, how should the dependencies be 
> > handled? For example, dss_core's reset will reset all the other modules 
> > also, and most of the submodules need functions from dss_core and 
> > dss_dispc. So should, say, dss_dsi then call functions in core and dispc 
> > to "get" them, i.e. increase their pm runtime use count?
> 
> Probably not.
> 
> Here is how I would suggest structuring the code.  This is only a naïve 
> suggestion; you and your team almost certainly know better than I.
> 
> I'd suggest that you separate low-level register access into .c files 
> that are targeted specifically for the IP block.  So in the DSS case, 
> you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c.  Each one should 
> be a separate platform_device and would export symbols.  Hopefully, there 
> should be no cross-dependencies between these low-level files.
> 
> Then you'd have "higher-level" code that might need to read/write from 
> multiple DSS submodules to complete some higher-level operation.  That 
> would be at least one separate driver -- say, "dss2" or something -- with 
> dependencies on the low-level drivers.  So, for example, when that 
> higher-level driver is modprobe'd, Linux would also load the drivers for 
> the underlying hardware blocks that it uses.

But this still leaves my question open: If this "dss2" needs to use,
say, dispc.c and dsi.c, those low level drivers need to be enabled. So I
guess we would have something like dispc_get() or dispc_enable(), which
dss2 would call first. Those functions would then use pm_runtime to
enable the HW module. And the higher level driver would keep the low
level devices enabled while its doing something.

I have been thinking something like this also earlier. It would separate
things cleanly. One thing I don't like about it is the big amount of low
level DSS internal functions that need to be exported.

> > - There seems to be some child/parent relationships in PM runtime.
> > Should dss_core be the parent for the rest of the DSS modules? This
> > would at least solve the reset issue easily, I guess.
> 
> Yes, I think that's more accurate, anyway.  Something isn't right with the 

How are the child/parent relationships done for omap_devices? Does it
come somehow from the hwmod data? 

> DSS hwmod data.  According to the OMAP4 Public TRM Rev O Table 10-3 "DSS 
> Integration," there's a Sonics LX bus lurking in there.  All of the DSS 
> submodules should have slave sockets with that Sonics LX bus as their 
> master.  And the hwmod associated with the SLX should have an address 
> range that covers all of the DSS submodules.  I assume that the logical 
> hwmod to associate the SLX with is "dss_core", as you write.
> 
> Also, I notice the "CAUTION" boxes in Section 10.1.3 "DSS Register 
> Manual", 10.2.7 "Display Controller Register Manual", etc. etc., that say 
> that the DSS and submodules should be accessed through the L3 address 
> space.  But all of the DSS hwmod register targets are listed as the L4_PER 
> variants.  So the hwmod data also doesn't appear to be correct there.  The 
> correct approach would be to have both address spaces listed in struct 
> omap_hwmod_addr_space arrays, but to mark only the struct 
> omap_hwmod_ocp_if entry associated with the L3 bus as OCP_USER_MPU | 
> OCP_USER_SDMA[*].

As far as I'm concerned, L4 interface can be removed totally. TRM says
"The access through the L4_PER interconnect is provided for back
software compatibility.".

 Tomi


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

Re: OMAP4 DSS clock setup

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7191 bytes --]

Hi

On Mon, 11 Apr 2011, Tomi Valkeinen wrote:

> On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
> > On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> > 
> > > > Also, I hope you and the other DSS hackers can finish the PM runtime
> > > > conversion of the DSS driver soon.  Ideally before any new DSS
> > > > features are added.  We really need to be able to separate the OMAP
> > > > integration details from the drivers, and right now, hwmod and PM
> > > > runtime are the best way we have to do that.
> > > 
> > > I also started to look at the PM runtime, but it doesn't fix the issue
> > > with the inconsistent clock name I described above.
> > 
> > After the hwmod/PM runtime conversion, I don't think any of the clock 
> > aliases currently in clock*_data.c should be used by the DSS driver (or by 
> > anything else on the system, for that matter).  That's because the 
> > omap_device code should set up the "main" alias for the DSS main 
> 
> When should "main" clock be used by the driver? Or never, and pm_runtime
> handles that?

If the DSS needs to change the parent or the clock rate of the main clock, 
then it will need to clk_get(dev, "main") and use the clock API.  My only 
point here was that the "main" alias should be automatically added by the 
omap_device code, not via the clock aliases in clock*_data.c.

> How about OMAP3? It also has an optional functional clock, but that
> doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
> Should it be there?

Probably so.

> It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
> hwmods. Does that mean it can never be turned off if DSS is in use?

If the DSS driver were using PM runtime, then yes, that would be true.

> Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
> believe.

In terms of the VENC, I'd agree with that -- based on OMAP4 Public TRM Rev 
O Figure 10-177 "Video Encoder Overview," it looks like VENC uses 
DSS_TV_FCLK.

In terms of the HDMI IP block, based on OMAP4 Public TRM Rev O Figure 
10-159 "HDMI Overview," it looks like sys_clk should be one of the 
optional clocks for HDMI?  Hard to tell from that diagram.

> > > - Should every DSS module have their own PM runtime handling? (actually
> > > related to the question above)
> > 
> > Yes, I think so.  From the integration perspective, we are trying to get 
> > to the point where each omap_device maps to only one hwmod.
> > 
> > > - If the modules are handled separately, how should the dependencies be 
> > > handled? For example, dss_core's reset will reset all the other modules 
> > > also, and most of the submodules need functions from dss_core and 
> > > dss_dispc. So should, say, dss_dsi then call functions in core and dispc 
> > > to "get" them, i.e. increase their pm runtime use count?
> > 
> > Probably not.
> > 
> > Here is how I would suggest structuring the code.  This is only a naïve 
> > suggestion; you and your team almost certainly know better than I.
> > 
> > I'd suggest that you separate low-level register access into .c files 
> > that are targeted specifically for the IP block.  So in the DSS case, 
> > you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c.  Each one should 
> > be a separate platform_device and would export symbols.  Hopefully, there 
> > should be no cross-dependencies between these low-level files.
> > 
> > Then you'd have "higher-level" code that might need to read/write from 
> > multiple DSS submodules to complete some higher-level operation.  That 
> > would be at least one separate driver -- say, "dss2" or something -- with 
> > dependencies on the low-level drivers.  So, for example, when that 
> > higher-level driver is modprobe'd, Linux would also load the drivers for 
> > the underlying hardware blocks that it uses.
> 
> But this still leaves my question open: If this "dss2" needs to use,
> say, dispc.c and dsi.c, those low level drivers need to be enabled. So I
> guess we would have something like dispc_get() or dispc_enable(), which
> dss2 would call first. Those functions would then use pm_runtime to
> enable the HW module. And the higher level driver would keep the low
> level devices enabled while its doing something.

That makes sense to me.

> I have been thinking something like this also earlier. It would separate
> things cleanly. One thing I don't like about it is the big amount of low
> level DSS internal functions that need to be exported.

Yeah, that's one of the downsides of that approach.

> > > - There seems to be some child/parent relationships in PM runtime.
> > > Should dss_core be the parent for the rest of the DSS modules? This
> > > would at least solve the reset issue easily, I guess.
> > 
> > Yes, I think that's more accurate, anyway.  Something isn't right with the 
> 
> How are the child/parent relationships done for omap_devices? Does it
> come somehow from the hwmod data? 

Right now, there's no explicit support in omap_device for parent/child 
relationships.  Probably the right place for that is in the omap_hwmod 
layer, since omap_device code just maps the hwmod data into the Linux 
device/driver model.  There is an interconnect hierarchy that's encoded in 
the omap_hwmod data, but currently there's no explicit handling for a case 
where a parent hwmod needs to be enabled for a child device to be 
accessed.  So far we haven't encountered any use-cases that require this, 
but I've suspected that this needs to be added to the hwmod code, and DSS 
may be the case that justifies it.

> > DSS hwmod data.  According to the OMAP4 Public TRM Rev O Table 10-3 "DSS 
> > Integration," there's a Sonics LX bus lurking in there.  All of the DSS 
> > submodules should have slave sockets with that Sonics LX bus as their 
> > master.  And the hwmod associated with the SLX should have an address 
> > range that covers all of the DSS submodules.  I assume that the logical 
> > hwmod to associate the SLX with is "dss_core", as you write.
> > 
> > Also, I notice the "CAUTION" boxes in Section 10.1.3 "DSS Register 
> > Manual", 10.2.7 "Display Controller Register Manual", etc. etc., that say 
> > that the DSS and submodules should be accessed through the L3 address 
> > space.  But all of the DSS hwmod register targets are listed as the L4_PER 
> > variants.  So the hwmod data also doesn't appear to be correct there.  The 
> > correct approach would be to have both address spaces listed in struct 
> > omap_hwmod_addr_space arrays, but to mark only the struct 
> > omap_hwmod_ocp_if entry associated with the L3 bus as OCP_USER_MPU | 
> > OCP_USER_SDMA[*].
> 
> As far as I'm concerned, L4 interface can be removed totally. TRM says
> "The access through the L4_PER interconnect is provided for back
> software compatibility.".

Okay, good to know.  We may leave them in there - the goal of the hwmod 
data is to describe the hardware independently of the Linux drivers.  But 
either way, the other set of addresses shouldn't appear to the DSS driver, 
so it's really a core code issue.


regards,

- Paul

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

On 4/11/2011 6:05 PM, Paul Walmsley wrote:
> Hi
>
> On Mon, 11 Apr 2011, Tomi Valkeinen wrote:
>
>> On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
>>> On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
>>>
>>>>> Also, I hope you and the other DSS hackers can finish the PM runtime
>>>>> conversion of the DSS driver soon.  Ideally before any new DSS
>>>>> features are added.  We really need to be able to separate the OMAP
>>>>> integration details from the drivers, and right now, hwmod and PM
>>>>> runtime are the best way we have to do that.
>>>>
>>>> I also started to look at the PM runtime, but it doesn't fix the issue
>>>> with the inconsistent clock name I described above.
>>>
>>> After the hwmod/PM runtime conversion, I don't think any of the clock
>>> aliases currently in clock*_data.c should be used by the DSS driver (or by
>>> anything else on the system, for that matter).  That's because the
>>> omap_device code should set up the "main" alias for the DSS main
>>
>> When should "main" clock be used by the driver? Or never, and pm_runtime
>> handles that?
>
> If the DSS needs to change the parent or the clock rate of the main clock,
> then it will need to clk_get(dev, "main") and use the clock API.  My only
> point here was that the "main" alias should be automatically added by the
> omap_device code, not via the clock aliases in clock*_data.c.
>
>> How about OMAP3? It also has an optional functional clock, but that
>> doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
>> Should it be there?
>
> Probably so.
>
>> It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
>> hwmods. Does that mean it can never be turned off if DSS is in use?
>
> If the DSS driver were using PM runtime, then yes, that would be true.
>
>> Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
>> believe.
>
> In terms of the VENC, I'd agree with that -- based on OMAP4 Public TRM Rev
> O Figure 10-177 "Video Encoder Overview," it looks like VENC uses
> DSS_TV_FCLK.
>
> In terms of the HDMI IP block, based on OMAP4 Public TRM Rev O Figure
> 10-159 "HDMI Overview," it looks like sys_clk should be one of the
> optional clocks for HDMI?  Hard to tell from that diagram.
>
>>>> - Should every DSS module have their own PM runtime handling? (actually
>>>> related to the question above)
>>>
>>> Yes, I think so.  From the integration perspective, we are trying to get
>>> to the point where each omap_device maps to only one hwmod.
>>>
>>>> - If the modules are handled separately, how should the dependencies be
>>>> handled? For example, dss_core's reset will reset all the other modules
>>>> also, and most of the submodules need functions from dss_core and
>>>> dss_dispc. So should, say, dss_dsi then call functions in core and dispc
>>>> to "get" them, i.e. increase their pm runtime use count?
>>>
>>> Probably not.
>>>
>>> Here is how I would suggest structuring the code.  This is only a naïve
>>> suggestion; you and your team almost certainly know better than I.
>>>
>>> I'd suggest that you separate low-level register access into .c files
>>> that are targeted specifically for the IP block.  So in the DSS case,
>>> you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c.  Each one should
>>> be a separate platform_device and would export symbols.  Hopefully, there
>>> should be no cross-dependencies between these low-level files.
>>>
>>> Then you'd have "higher-level" code that might need to read/write from
>>> multiple DSS submodules to complete some higher-level operation.  That
>>> would be at least one separate driver -- say, "dss2" or something -- with
>>> dependencies on the low-level drivers.  So, for example, when that
>>> higher-level driver is modprobe'd, Linux would also load the drivers for
>>> the underlying hardware blocks that it uses.
>>
>> But this still leaves my question open: If this "dss2" needs to use,
>> say, dispc.c and dsi.c, those low level drivers need to be enabled. So I
>> guess we would have something like dispc_get() or dispc_enable(), which
>> dss2 would call first. Those functions would then use pm_runtime to
>> enable the HW module. And the higher level driver would keep the low
>> level devices enabled while its doing something.
>
> That makes sense to me.
>
>> I have been thinking something like this also earlier. It would separate
>> things cleanly. One thing I don't like about it is the big amount of low
>> level DSS internal functions that need to be exported.
>
> Yeah, that's one of the downsides of that approach.
>
>>>> - There seems to be some child/parent relationships in PM runtime.
>>>> Should dss_core be the parent for the rest of the DSS modules? This
>>>> would at least solve the reset issue easily, I guess.
>>>
>>> Yes, I think that's more accurate, anyway.  Something isn't right with the
>>
>> How are the child/parent relationships done for omap_devices? Does it
>> come somehow from the hwmod data?
>
> Right now, there's no explicit support in omap_device for parent/child
> relationships.  Probably the right place for that is in the omap_hwmod
> layer, since omap_device code just maps the hwmod data into the Linux
> device/driver model.  There is an interconnect hierarchy that's encoded in
> the omap_hwmod data, but currently there's no explicit handling for a case
> where a parent hwmod needs to be enabled for a child device to be
> accessed.  So far we haven't encountered any use-cases that require this,
> but I've suspected that this needs to be added to the hwmod code, and DSS
> may be the case that justifies it.
>
>>> DSS hwmod data.  According to the OMAP4 Public TRM Rev O Table 10-3 "DSS
>>> Integration," there's a Sonics LX bus lurking in there.  All of the DSS
>>> submodules should have slave sockets with that Sonics LX bus as their
>>> master.  And the hwmod associated with the SLX should have an address
>>> range that covers all of the DSS submodules.  I assume that the logical
>>> hwmod to associate the SLX with is "dss_core", as you write.
>>>
>>> Also, I notice the "CAUTION" boxes in Section 10.1.3 "DSS Register
>>> Manual", 10.2.7 "Display Controller Register Manual", etc. etc., that say
>>> that the DSS and submodules should be accessed through the L3 address
>>> space.  But all of the DSS hwmod register targets are listed as the L4_PER
>>> variants.  So the hwmod data also doesn't appear to be correct there.

What version are you looking at? Both address spaces are already there 
today.

static struct omap_hwmod_addr_space omap44xx_dss_dma_addrs[] = {
	{
		.pa_start	= 0x58000000,
		.pa_end		= 0x5800007f,
		.flags		= ADDR_TYPE_RT
	},
};

/* l3_main_2 -> dss */
static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
	.master		= &omap44xx_l3_main_2_hwmod,
	.slave		= &omap44xx_dss_hwmod,
	.clk		= "l3_div_ck",
	.addr		= omap44xx_dss_dma_addrs,
	.addr_cnt	= ARRAY_SIZE(omap44xx_dss_dma_addrs),
	.user		= OCP_USER_SDMA,
};

static struct omap_hwmod_addr_space omap44xx_dss_addrs[] = {
	{
		.pa_start	= 0x48040000,
		.pa_end		= 0x4804007f,
		.flags		= ADDR_TYPE_RT
	},
};

/* l4_per -> dss */
static struct omap_hwmod_ocp_if omap44xx_l4_per__dss = {
	.master		= &omap44xx_l4_per_hwmod,
	.slave		= &omap44xx_dss_hwmod,
	.clk		= "l4_div_ck",
	.addr		= omap44xx_dss_addrs,
	.addr_cnt	= ARRAY_SIZE(omap44xx_dss_addrs),
	.user		= OCP_USER_MPU,
};

/* dss slave ports */
static struct omap_hwmod_ocp_if *omap44xx_dss_slaves[] = {
	&omap44xx_l3_main_2__dss,
	&omap44xx_l4_per__dss,
};

>>> The
>>> correct approach would be to have both address spaces listed in struct
>>> omap_hwmod_addr_space arrays, but to mark only the struct
>>> omap_hwmod_ocp_if entry associated with the L3 bus as OCP_USER_MPU |
>>> OCP_USER_SDMA[*].

Today, hwmod will use only the L4 one just because the OCP_USER_MPU flag 
is used for that one only. We can just add the SDMA flag to L3 and 
remove the L4 mapping, if needed, but it will only change the one use by 
hwmod code. The driver will not impacted by that at all.

>> As far as I'm concerned, L4 interface can be removed totally. TRM says
>> "The access through the L4_PER interconnect is provided for back
>> software compatibility.".
>
> Okay, good to know.  We may leave them in there - the goal of the hwmod
> data is to describe the hardware independently of the Linux drivers.  But
> either way, the other set of addresses shouldn't appear to the DSS driver,
> so it's really a core code issue.

If we do that, we will have to name the address space to allow the 
driver to choose the proper one.
The is doable since we added that support for McBSP in 2.6.39.
I have some patches ready to add name address space for all IPs with 
dual mapping.

But in that case, removing the L4 completely will avoid any further 
change to the DSS driver. For the moment we are lucky just because the 
L3 entry is the first one in the list:-)

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

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

On 4/11/2011 6:05 PM, Paul Walmsley wrote:
> Hi
>
> On Mon, 11 Apr 2011, Tomi Valkeinen wrote:
>
>> On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
>>> On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
>>>
>>>>> Also, I hope you and the other DSS hackers can finish the PM runtime
>>>>> conversion of the DSS driver soon.  Ideally before any new DSS
>>>>> features are added.  We really need to be able to separate the OMAP
>>>>> integration details from the drivers, and right now, hwmod and PM
>>>>> runtime are the best way we have to do that.
>>>>
>>>> I also started to look at the PM runtime, but it doesn't fix the issue
>>>> with the inconsistent clock name I described above.
>>>
>>> After the hwmod/PM runtime conversion, I don't think any of the clock
>>> aliases currently in clock*_data.c should be used by the DSS driver (or by
>>> anything else on the system, for that matter).  That's because the
>>> omap_device code should set up the "main" alias for the DSS main
>>
>> When should "main" clock be used by the driver? Or never, and pm_runtime
>> handles that?
>
> If the DSS needs to change the parent or the clock rate of the main clock,
> then it will need to clk_get(dev, "main") and use the clock API.  My only
> point here was that the "main" alias should be automatically added by the
> omap_device code, not via the clock aliases in clock*_data.c.

That will be doable only when main_clk will not be mapped to modulemode. 
You cannot change the clock rate of the modulemode since it is a fake 
clock :-(

>> How about OMAP3? It also has an optional functional clock, but that
>> doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
>> Should it be there?
>
> Probably so.
>
>> It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
>> hwmods. Does that mean it can never be turned off if DSS is in use?
>
> If the DSS driver were using PM runtime, then yes, that would be true.

And then even if we switch to dss2 as the functional clock, dss1 cannot 
be gated.

>> Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
>> believe.
>
> In terms of the VENC, I'd agree with that -- based on OMAP4 Public TRM Rev
> O Figure 10-177 "Video Encoder Overview," it looks like VENC uses
> DSS_TV_FCLK.

If we do have an hwmod for dss_venc, we might consider the tv_clk as the 
main clock, which is the case anyway. The only tricky part is the 
dependency with the dss modulemode / fclk that have to be enabled first.

> In terms of the HDMI IP block, based on OMAP4 Public TRM Rev O Figure
> 10-159 "HDMI Overview," it looks like sys_clk should be one of the
> optional clocks for HDMI?  Hard to tell from that diagram.
>
>>>> - Should every DSS module have their own PM runtime handling? (actually
>>>> related to the question above)
>>>
>>> Yes, I think so.  From the integration perspective, we are trying to get
>>> to the point where each omap_device maps to only one hwmod.
>>>
>>>> - If the modules are handled separately, how should the dependencies be
>>>> handled? For example, dss_core's reset will reset all the other modules
>>>> also, and most of the submodules need functions from dss_core and
>>>> dss_dispc. So should, say, dss_dsi then call functions in core and dispc
>>>> to "get" them, i.e. increase their pm runtime use count?
>>>
>>> Probably not.
>>>
>>> Here is how I would suggest structuring the code.  This is only a naïve
>>> suggestion; you and your team almost certainly know better than I.
>>>
>>> I'd suggest that you separate low-level register access into .c files
>>> that are targeted specifically for the IP block.  So in the DSS case,
>>> you'd have dss.c, dispc.c, dsi.c, hdmi.c, rfbi.c, venc.c.  Each one should
>>> be a separate platform_device and would export symbols.  Hopefully, there
>>> should be no cross-dependencies between these low-level files.
>>>
>>> Then you'd have "higher-level" code that might need to read/write from
>>> multiple DSS submodules to complete some higher-level operation.  That
>>> would be at least one separate driver -- say, "dss2" or something -- with
>>> dependencies on the low-level drivers.  So, for example, when that
>>> higher-level driver is modprobe'd, Linux would also load the drivers for
>>> the underlying hardware blocks that it uses.
>>
>> But this still leaves my question open: If this "dss2" needs to use,
>> say, dispc.c and dsi.c, those low level drivers need to be enabled. So I
>> guess we would have something like dispc_get() or dispc_enable(), which
>> dss2 would call first. Those functions would then use pm_runtime to
>> enable the HW module. And the higher level driver would keep the low
>> level devices enabled while its doing something.
>
> That makes sense to me.
>
>> I have been thinking something like this also earlier. It would separate
>> things cleanly. One thing I don't like about it is the big amount of low
>> level DSS internal functions that need to be exported.
>
> Yeah, that's one of the downsides of that approach.
>
>>>> - There seems to be some child/parent relationships in PM runtime.
>>>> Should dss_core be the parent for the rest of the DSS modules? This
>>>> would at least solve the reset issue easily, I guess.
>>>
>>> Yes, I think that's more accurate, anyway.  Something isn't right with the
>>
>> How are the child/parent relationships done for omap_devices? Does it
>> come somehow from the hwmod data?
>
> Right now, there's no explicit support in omap_device for parent/child
> relationships.  Probably the right place for that is in the omap_hwmod
> layer, since omap_device code just maps the hwmod data into the Linux
> device/driver model.  There is an interconnect hierarchy that's encoded in
> the omap_hwmod data, but currently there's no explicit handling for a case
> where a parent hwmod needs to be enabled for a child device to be
> accessed.  So far we haven't encountered any use-cases that require this,
> but I've suspected that this needs to be added to the hwmod code, and DSS
> may be the case that justifies it.

Do we really have to add that in hwmod core code? Cannot we let the 
driver stack handle that dependency? Assuming we have a device for the 
low level DSS code and assuming it is doing more than just enabling / 
disabling the module.

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

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Mon, 2011-04-11 at 10:05 -0600, Paul Walmsley wrote:
> Hi
> 
> On Mon, 11 Apr 2011, Tomi Valkeinen wrote:
> 
> > On Fri, 2011-04-08 at 10:28 -0600, Paul Walmsley wrote:
> > > On Fri, 8 Apr 2011, Tomi Valkeinen wrote:
> > > 
> > > > > Also, I hope you and the other DSS hackers can finish the PM runtime
> > > > > conversion of the DSS driver soon.  Ideally before any new DSS
> > > > > features are added.  We really need to be able to separate the OMAP
> > > > > integration details from the drivers, and right now, hwmod and PM
> > > > > runtime are the best way we have to do that.
> > > > 
> > > > I also started to look at the PM runtime, but it doesn't fix the issue
> > > > with the inconsistent clock name I described above.
> > > 
> > > After the hwmod/PM runtime conversion, I don't think any of the clock 
> > > aliases currently in clock*_data.c should be used by the DSS driver (or by 
> > > anything else on the system, for that matter).  That's because the 
> > > omap_device code should set up the "main" alias for the DSS main 
> > 
> > When should "main" clock be used by the driver? Or never, and pm_runtime
> > handles that?
> 
> If the DSS needs to change the parent or the clock rate of the main clock, 
> then it will need to clk_get(dev, "main") and use the clock API.  My only 
> point here was that the "main" alias should be automatically added by the 
> omap_device code, not via the clock aliases in clock*_data.c.

Ok.

> > How about OMAP3? It also has an optional functional clock, but that
> > doesn't seem to be set in dss_opt_clocks in omap_hwmod_3xxx_data.c.
> > Should it be there?
> 
> Probably so.
> 
> > It seems dss1_alwon_fclk is currently the main clock for OMAP3 dss
> > hwmods. Does that mean it can never be turned off if DSS is in use?
> 
> If the DSS driver were using PM runtime, then yes, that would be true.

Ok. Then there's also room for improvement, as the dss1_alwon_fclk is an
optional clock on OMAP3.

> > Ok. Then we need omap_hwmod_opt_clk for at least for VENC and HDMI, I
> > believe.
> 
> In terms of the VENC, I'd agree with that -- based on OMAP4 Public TRM Rev 
> O Figure 10-177 "Video Encoder Overview," it looks like VENC uses 
> DSS_TV_FCLK.

On OMAP3 DSS_96M_FCLK goes to VENC's video DACs. I don't quite
understand how it goes in OMAP4, the clock tree figure shows something
going into VDAC, but it's unclear what. Well, anyway, the point was not
to dig out the clocks now, but just to point out that some extra clocks
are needed =).

> In terms of the HDMI IP block, based on OMAP4 Public TRM Rev O Figure 
> 10-159 "HDMI Overview," it looks like sys_clk should be one of the 
> optional clocks for HDMI?  Hard to tell from that diagram.

Yes, I think sys_clk is the input clock for the HDMI PLL. The output
from the PLL can then be used for DISPC fckl.

I believe HDMI_PHY_48M_FCLK ("dss_48mhz_clk") is also needed.

 Tomi

Re: OMAP4 DSS clock setup

[Cousson, Benoit]

Hi Paul,

On 4/7/2011 9:27 PM, Paul Walmsley wrote:
> Hello Tomi, Benoît,
>
> On Mon, 4 Apr 2011, Tomi Valkeinen wrote:
>
>> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
>>
>>> Based on the E-mail thread so far, I'd say that changing the clock aliases
>>> is the way to go for right now.  The clock aliases are not hardware data;
>>> they just control how the clock hardware is mapped to the drivers.
>>
>> I'd very much prefer this option. Below is a patch for this.
>>
>> If Benoit doesn't complain too much about this, I'd like to get this
>> merged as soon as possible, as OMAP4 DSS is currently crashing in the
>> mainline kernel.
>
> I will queue your clock aliases patch and attempt to merge it upstream for
> the -rc series.  I am not happy to be disagreeing with Benoît here, but
> this is the rationale that I am using.  (Benoît, Tomi, please feel free to
> correct if there is something blatantly false below.)

That's quite good that you were disagreeing with me, because I think 
that you are indeed right:-)

> 1. The integration of the DSS module is an unusual case.  The
>     MODULEMODE bits for the DSS bits do not control the DSS "main"
>     functional clock (the clock that is needed to access device
>     registers); instead, they only control the DSS interface clock.
>     (This is because the DSS can use a functional clock that is not
>     provided by the OMAP core.)  So calling the DSS MODULEMODE struct
>     clk "dss_fck" is not really correct, since it is really controlling
>     the interface clock.

I've just got the confirmation from a PRCM designer that this is indeed 
what is happening.

In the case of the DSS the optional functional clocks are provided by 
the PRCM and thus managed by the PRCM. The only difference is that since 
two different functional clocks are available, the PRCM cannot chose 
automatically the proper one.
Hence the term optional fck, meaning that the SW has to explicitly 
enable them. The PRCM will not do it when modulemode will be enabled.

So in that case enabling the modulemode will effectively enable the 
module for the PRCM point of view and just request the iclk if not 
already available.

The only point that we need to take care of is the sequence.
The PRCM spec clearly specify that one of the optional clock must be 
active before the modulemode is enabled.
That does not seems to be the case in the current DSS code.

> 2. This patch does not create a precedent for hacking around general
>     driver clocking problems in the clock or clock data.  This is only
>     needed because the MODULEMODE bits don't control the module
>     functional clock in this case.  As I understand it, the only other
>     device that this problem could occur with is McBSP, due to
>     mcbsp_clks.

In that case this is slightly different because even the PRCM does not 
control these external clocks. Whereas in the case of the dss_fck, if 
the DPLL is not locked when you request it, it will be enabled 
automatically. Assuming that auto mode are enabled.

> 3. The existing DSS2 driver clock design apparently works fine when
>     this change is implemented[1], so no driver changes will be needed.

Yeah, but my point was that driver changes will be needed anyway, hence 
my preference to hack the driver instead of hacking the clock data.

> 4. The proposed DSS driver patch to work around this uses a nasty hack
>     that really should be NACK'ed[2].
>
> All this said, we may not be able to merge this change upstream, due to
> the recent unhappiness about the clock data changes.  If Linus rejects it,
> you'll need a "Plan B."

That's was my #2 concern as well.

See you soon at ELC.

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

Re: OMAP4 DSS clock setup

[Paul Walmsley]

Hi Tomi,

On Mon, 4 Apr 2011, Tomi Valkeinen wrote:

> On Fri, 2011-04-01 at 20:12 -0600, Paul Walmsley wrote:
> 
> > Of course, at some point soon, those clock aliases are going to go away.  
> > But hopefully you all will have converted the driver over to PM runtime at 
> > that point.
> > 
> > Once that happens, there's another problem: omap_hwmod_enable() is defined 
> > that the hardware registers should be accessible by the MPU after it 
> > completes.  So by that definition, the hwmod code should be 
> > enabling/disabling that dss_clk clock too when it enables/idles/shuts down 
> > the hwmod.  Probably we'd need to mark that struct omap_hwmod_opt_clk with 
> > some flag.  Then we'd need some kind of way for the DSS to tell the hwmod 
> > code whether it is or isn't reliant on the PRCM-provided functional clock 
> > for its internal functional clock.  Maybe something like 
> > omap_hwmod_{release,require}_system_fclk()?
> 
> Hmm, right. I guess no other HW module has clock setups like this?

I think McBSP can have an optional main functional clock.  So something 
like that should be usable for that case too. (In the McBSP case, the 
optional clock isn't controlled by the PRCM, it's controlled by the SCM, 
but that doesn't really matter to the hwmod code.)

> Currently DSS can use clk_enable/disable() for the system fclk when its
> using DSI PLL for the fclk. So omap_hwmod_{release,require}_system_fclk
> sounds like a simple solution to this.

OK.  The only problem with those functions (actually 
omap_device_{release,require}_system_fclk()) is that they will need to be 
called through platform_data function pointers for now.  Maybe it is 
possible to handle something like this simply with the clock framework 
also.

> Not directly related, but something I've been wondering about is how to
> abstract the DSI/HDMI PLLs in DSS. What do you think, would it be
> possible/worth it to create struct clks for the clocks coming out of
> those PLLs? These would, of course, be DSS internal clks. I'm not very
> familiar with the clock framework, so I don't really have any idea what
> this would require and what would be the pros and cons.

Yes, I think it would be good to try to implement the entire DSS clock 
tree in the clock framework.  One change to the clock code that we know 
we'll need is to put a hwmod pointer in the struct clk which tells the 
clock code that the hwmod needs to be enabled in order to access the 
clock's registers.  Right now, the clock code assumes that all of the 
clock registers are accessible, all of the time.


- Paul

Re: OMAP4 DSS clock setup

[Tomi Valkeinen]

On Fri, 2011-04-08 at 10:50 -0600, Paul Walmsley wrote:
> Hi Tomi,
> 
> On Mon, 4 Apr 2011, Tomi Valkeinen wrote:

> > Not directly related, but something I've been wondering about is how to
> > abstract the DSI/HDMI PLLs in DSS. What do you think, would it be
> > possible/worth it to create struct clks for the clocks coming out of
> > those PLLs? These would, of course, be DSS internal clks. I'm not very
> > familiar with the clock framework, so I don't really have any idea what
> > this would require and what would be the pros and cons.
> 
> Yes, I think it would be good to try to implement the entire DSS clock 
> tree in the clock framework.  One change to the clock code that we know 
> we'll need is to put a hwmod pointer in the struct clk which tells the 
> clock code that the hwmod needs to be enabled in order to access the 
> clock's registers.  Right now, the clock code assumes that all of the 
> clock registers are accessible, all of the time.

It's not quite that simple, as DSI PLL also needs the vdds_dsi regulator
to be enabled... And to use DSI PLL, not only do you need to access DSI
PLL registers, you also need to use DSI registers.

Is it possible to have the driver create its own clock structs when it's
loaded, and have the code for that clock inside the driver, or are
clocks something that has to be handled in the core platform code?

 Tomi