Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support

["Cousson, Benoit" <b-cousson@ti.com>]

On 6/6/2011 3:55 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-06-06 at 15:46 +0200, Cousson, Benoit wrote:
>> On 6/6/2011 3:21 PM, Valkeinen, Tomi wrote:
>>> On Mon, 2011-06-06 at 15:15 +0200, Cousson, Benoit wrote:
>>>> On 6/6/2011 3:01 PM, Valkeinen, Tomi wrote:
>>>>> On Mon, 2011-06-06 at 14:56 +0200, Cousson, Benoit wrote:
>>>
>>>>> In this long term solution, if the dss_fclk is the main_clk, how does
>>>>> the framework handle the situation when we want to switch from the
>>>>> standard DSS fclk to the one from DSI PLL?
>>>>
>>>> That part cannot be done by the hwmod fmwk anyway. The goal of the fmwk
>>>> is to ensure that the module is accessible by the driver whatever the
>>>> PRCM clock used.
>>>> Enabling the DSI PLL will require the PRCM clock to be enabled first.
>>>>
>>>> Using the DSI PLL as the fclk is doable, but is it really useful or needed?
>>>
>>> Yes, it's useful and needed. It gives us much finer control to the clock
>>> frequencies, and so allows us to go to higher frequencies and also more
>>> exactly to the required pixel clock.
>>>
>>>> Assuming you need that mode, you will always have to explicitly switch
>>>> from DSI to PRCM clock before trying to disable the DSS.
>>>> This is something you will have to do inside the DSS driver. It should
>>>> be transparent to the hwmod fmwk.
>>>
>>> This sounds ok.
>>>
>>> I think the main question is how do we disable the standard DSS fclk
>>> from PRCM when using DSI PLL? As far as I know, disabling that clock
>>> will allow some areas of OMAP to be shut down even while DSS is working.
>>> So from power management point of view it sounds a needed feature.
>>
>> Yes, at least in theory, but considering that any use case that will
>> require the DSI PLL will use a LCD panel + backlight, or an OLED panel
>> that will consume 50 times more than the 186 MHz clock, I do not think
>> it is really needed.
>> Moreover, that clock is generated by the PER DPLL that will be always
>> enabled in most usecase because it does generate the UART, I2C and most
>> basic peripherals clocks. If we cannot gate the PER DPLL, there is no
>> saving to expect from gating the DSS fclk only.
>> Bottom-line is that there is no practical power saving to expect from
>> that mode.
>>
>>> If the clock is main_clk for the HWMOD, it sounds to me it's always
>>> enabled if the HWMOD is enabled?
>>
>> Yes, but that sounds to me a good trade off to avoid unnecessary
>> complexity in your driver or in the hwmod fmwk.
>
> Ok, if there are no real power savings there, then I agree, it's
> pointless to add that complexity.
>
> So how do we go forward in short term? I'd very much like to remove all
> the "silly" code from the DSS pm_runtime patch series caused by this
> opt_clock handling. Is it possible to get some kind of a temporary
> solution in the hwmod framework which would somehow solve this from DSS
> driver's point of view? A flag that causes hwmod fmwk to enable
> opt-clocks automatically? Or is it possible to have more than one
> mandatory clock?

Before doing that, could you maybe just try something to make OMAP4 
looks a little bit more like OMAP3?

dss_fck -> ick
dss_dss_fck -> main_clk

That should ensure that both modulemode and the PRCM fclk will be 
managed by pm_runtime.

I just did a basic patch for the first module, you should maybe change 
some other entries.

Regards,
Benoit

---
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c 
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 614d680..4dfd18a 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1134,7 +1134,7 @@ static struct omap_hwmod_addr_space 
omap44xx_dss_dma_addrs[] = {
  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",
+       .clk            = "dss_fck",
         .addr           = omap44xx_dss_dma_addrs,
         .addr_cnt       = ARRAY_SIZE(omap44xx_dss_dma_addrs),
         .user           = OCP_USER_SDMA,
@@ -1167,14 +1167,13 @@ static struct omap_hwmod_ocp_if 
*omap44xx_dss_slaves[] = {
  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" },
  };

  static struct omap_hwmod omap44xx_dss_hwmod = {
         .name           = "dss_core",
         .class          = &omap44xx_dss_hwmod_class,
-       .main_clk       = "dss_fck",
+       .main_clk       = "dss_dss_fck",
         .prcm           = {
                 .omap4 = {
                         .clkctrl_reg = OMAP4430_CM_DSS_DSS_CLKCTRL,