Re: OMAP4 DSS clock setup

[Archit Taneja <archit@ti.com>]

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