Problem with DSS clocks & accessing registers

Problem with DSS clocks & accessing registers

[Tomi Valkeinen]

Hi Kevin, Paul,

We currently have a small problem with OMAP4 DSS. When we enable the DSS
clocks, it seems that the DSS registers are not always accessible right
after the clock enable.

I understood that on OMAP4 the clock framework doesn't guarantee that
the registers are accessible after enabling clocks, and pm_runtime will
handle this. Is this correct?

I have made a small hack fix for this. I added udelay(10) in the DSS
code which enables the clocks and this seem to remove the problem. The
delay is only called when DSS thinks the clocks have been off, so in
practice the delay shouldn't be visible. Is this fix acceptable for now,
until we get pm_runtime support to DSS? 

 Tomi

Re: Problem with DSS clocks & accessing registers

[Cousson, Benoit]

+ Rajendra

Hi Tomi,

On 3/7/2011 9:22 AM, Valkeinen, Tomi wrote:
> Hi Kevin, Paul,
>
> We currently have a small problem with OMAP4 DSS. When we enable the DSS
> clocks, it seems that the DSS registers are not always accessible right
> after the clock enable.

What clocks are you talking about? As you know, the DSS has a bunch of 
functional clocks available depending of the use case.

> I understood that on OMAP4 the clock framework doesn't guarantee that
> the registers are accessible after enabling clocks, and pm_runtime will
> handle this. Is this correct?

The point is that there is no status at clock level but only one status 
at module level. That's why this check does not have anything to do at 
clock level.
So hwmod with handle that status (OMAP4430_CM_DSS_DSS_CLKCTRL) during 
the hwmod_enable that is indirectly called by pm_runtime, through 
omap_device API.

> I have made a small hack fix for this. I added udelay(10) in the DSS
> code which enables the clocks and this seem to remove the problem. The
> delay is only called when DSS thinks the clocks have been off, so in
> practice the delay shouldn't be visible. Is this fix acceptable for now,
> until we get pm_runtime support to DSS?

Cannot you use omap_device for the moment? Relying on a udelay is not a 
very safe method, even temporally.
Or you might hack the main clock node to check for the module status. I 
think it was done like that at the beginning of OMAP4.
It was removed, but I do not remember if it was done at fmwk level or at 
clock nodes level.

Rajendra should know the full story.

Regards,
Benoit

RE: Problem with DSS clocks & accessing registers

[Santosh Shilimkar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Cousson, Benoit
> Sent: Monday, March 07, 2011 7:21 PM
> To: Valkeinen, Tomi; Nayak, Rajendra
> Cc: Paul Walmsley; Hilman, Kevin; linux-omap; Taneja, Archit;
> Semwal, Sumit
> Subject: Re: Problem with DSS clocks & accessing registers
>
> + Rajendra
>
> Hi Tomi,
>
> On 3/7/2011 9:22 AM, Valkeinen, Tomi wrote:
> > Hi Kevin, Paul,
> >
> > We currently have a small problem with OMAP4 DSS. When we enable
> the DSS
> > clocks, it seems that the DSS registers are not always accessible
> right
> > after the clock enable.
>
> What clocks are you talking about? As you know, the DSS has a bunch
> of functional clocks available depending of the use case.
>
> > I understood that on OMAP4 the clock framework doesn't guarantee
> that
> > the registers are accessible after enabling clocks, and pm_runtime
> will handle this. Is this correct?
>

This seems to the question about a so called optional clock (DSS_CLK)
which really not an optional clock :)

This clock needs to be enabled and disabled along with interface and
functional clock for the registers access to work.

If you look internals of the clock routing in DSS, Figure 10-4
this clock is not really optional. You can call it optional because
there can be a option to use PLL clock instead of this one. But the
fact is either DSS_CLK or the PLL clock has to be running for the
IP accesses to work.

Sumit,
Please add more points about our discussion on this one.

Regards,
Santosh

Re: Problem with DSS clocks & accessing registers

[Cousson, Benoit]

On 3/7/2011 3:05 PM, Shilimkar, Santosh wrote:
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Cousson, Benoit
>>
>> + Rajendra
>>
>> Hi Tomi,
>>
>> On 3/7/2011 9:22 AM, Valkeinen, Tomi wrote:
>>> Hi Kevin, Paul,
>>>
>>> We currently have a small problem with OMAP4 DSS. When we enable
>> the DSS
>>> clocks, it seems that the DSS registers are not always accessible
>> right
>>> after the clock enable.
>>
>> What clocks are you talking about? As you know, the DSS has a bunch
>> of functional clocks available depending of the use case.
>>
>>> I understood that on OMAP4 the clock framework doesn't guarantee
>> that
>>> the registers are accessible after enabling clocks, and pm_runtime
>> will handle this. Is this correct?
>>
>
> This seems to the question about a so called optional clock (DSS_CLK)
> which really not an optional clock :)
> This clock needs to be enabled and disabled along with interface and
> functional clock for the registers access to work.

To be more accurate, this clock is one of the functional clock available 
for the DSS (the other being the SYS_CLK).

You can use dss_clk or sys_clk (through dsi pll) to generate the 
functional clock of the DSS. The issue, as I said is that the 
availability status of the DSS is bound to the DSS module mode, not to 
any of this DSS clock.

Benoit

Re: Problem with DSS clocks & accessing registers

[Tomi Valkeinen]

On Mon, 2011-03-07 at 07:51 -0600, Cousson, Benoit wrote:
> + Rajendra
> 
> Hi Tomi,
> 
> On 3/7/2011 9:22 AM, Valkeinen, Tomi wrote:
> > Hi Kevin, Paul,
> >
> > We currently have a small problem with OMAP4 DSS. When we enable the DSS
> > clocks, it seems that the DSS registers are not always accessible right
> > after the clock enable.
> 
> What clocks are you talking about? As you know, the DSS has a bunch of 
> functional clocks available depending of the use case.

In this case the clocks for DISPC, iclk and fclk. On OMAP4 fclk is
DSS_FCLK and iclk is not used, I believe. The problem is probably also
in all the other DSS modules.

> > I understood that on OMAP4 the clock framework doesn't guarantee that
> > the registers are accessible after enabling clocks, and pm_runtime will
> > handle this. Is this correct?
> 
> The point is that there is no status at clock level but only one status 
> at module level. That's why this check does not have anything to do at 
> clock level.
> So hwmod with handle that status (OMAP4430_CM_DSS_DSS_CLKCTRL) during 
> the hwmod_enable that is indirectly called by pm_runtime, through 
> omap_device API.

Ah. Ok. How does CM_DSS_DSS_CLKCTRL show that the DSS module is ready?
IDLEST is 0?

> > I have made a small hack fix for this. I added udelay(10) in the DSS
> > code which enables the clocks and this seem to remove the problem. The
> > delay is only called when DSS thinks the clocks have been off, so in
> > practice the delay shouldn't be visible. Is this fix acceptable for now,
> > until we get pm_runtime support to DSS?
> 
> Cannot you use omap_device for the moment? Relying on a udelay is not a 
> very safe method, even temporally.

Yes, udelay is not very good option even as a hack, except if we can
wait some amount of time which we know will always be enough. Can we?

I have to say I'm not very familiar with omap_device, as I haven't been
really working with them (nor hwmods). Sumit or Archit can perhaps fill
in, but my understanding is that work on that area is going on, but it's
not ready yet. So I'm looking for some temporary quick solution for this
so that DSS driver doesn't randomly crash on OMAP4.

 Tomi

Re: Problem with DSS clocks & accessing registers

[Cousson, Benoit]

On 3/7/2011 4:03 PM, Valkeinen, Tomi wrote:
> On Mon, 2011-03-07 at 07:51 -0600, Cousson, Benoit wrote:
>> + Rajendra
>>
>> Hi Tomi,
>>
>> On 3/7/2011 9:22 AM, Valkeinen, Tomi wrote:
>>> Hi Kevin, Paul,
>>>
>>> We currently have a small problem with OMAP4 DSS. When we enable the DSS
>>> clocks, it seems that the DSS registers are not always accessible right
>>> after the clock enable.
>>
>> What clocks are you talking about? As you know, the DSS has a bunch of
>> functional clocks available depending of the use case.
>
> In this case the clocks for DISPC, iclk and fclk. On OMAP4 fclk is
> DSS_FCLK and iclk is not used, I believe. The problem is probably also
> in all the other DSS modules.
>
>>> I understood that on OMAP4 the clock framework doesn't guarantee that
>>> the registers are accessible after enabling clocks, and pm_runtime will
>>> handle this. Is this correct?
>>
>> The point is that there is no status at clock level but only one status
>> at module level. That's why this check does not have anything to do at
>> clock level.
>> So hwmod with handle that status (OMAP4430_CM_DSS_DSS_CLKCTRL) during
>> the hwmod_enable that is indirectly called by pm_runtime, through
>> omap_device API.
>
> Ah. Ok. How does CM_DSS_DSS_CLKCTRL show that the DSS module is ready?
> IDLEST is 0?

Yep, or 2 if only the interface clock is idle and a separate functional 
clock is running, which is the case of the DSS. You can check the 
function used in hwmod: omap4_cm_wait_module_ready in cminst44xx.c.

- 0x0 func: Module is fully functional, including OCP
- 0x2 idle: Module is in Idle mode (only OCP part). It is functional if 
using separate functional clock

>>> I have made a small hack fix for this. I added udelay(10) in the DSS
>>> code which enables the clocks and this seem to remove the problem. The
>>> delay is only called when DSS thinks the clocks have been off, so in
>>> practice the delay shouldn't be visible. Is this fix acceptable for now,
>>> until we get pm_runtime support to DSS?
>>
>> Cannot you use omap_device for the moment? Relying on a udelay is not a
>> very safe method, even temporally.
>
> Yes, udelay is not very good option even as a hack, except if we can
> wait some amount of time which we know will always be enough. Can we?

You should really avoid that, because we really don't have any good way 
to get a good number.

> I have to say I'm not very familiar with omap_device, as I haven't been
> really working with them (nor hwmods). Sumit or Archit can perhaps fill
> in, but my understanding is that work on that area is going on, but it's
> not ready yet. So I'm looking for some temporary quick solution for this
> so that DSS driver doesn't randomly crash on OMAP4.

Adding the check in the clock framework for DSS clock nodes only might 
be the cleanest way to do that right now.

You can map clock nodes to clkops_omap2_dflt_wait clkops instead of the 
clkops_omap2_dflt currently used. But then you will have to add the 
support for OMAP4, since for the moment the function seems to work only 
for OMAP2 & 3 (omap2_clk_dflt_find_idlest).

It is a little bit more complex than the current WA but should not be 
that hard either to do.
This is a much cleaner WA before having the full hwmod support for the 
DSS for my point of view.

Paul,
Any thoughts?

Regards,
Benoit

Re: Problem with DSS clocks & accessing registers

[Paul Walmsley]

Hi,

On Tue, 8 Mar 2011, Cousson, Benoit wrote:

> Adding the check in the clock framework for DSS clock nodes only might be the
> cleanest way to do that right now.
> 
> You can map clock nodes to clkops_omap2_dflt_wait clkops instead of the
> clkops_omap2_dflt currently used. But then you will have to add the support
> for OMAP4, since for the moment the function seems to work only for OMAP2 & 3
> (omap2_clk_dflt_find_idlest).
> 
> It is a little bit more complex than the current WA but should not be that
> hard either to do.
> This is a much cleaner WA before having the full hwmod support for the DSS for
> my point of view.
> 
> Paul,
> Any thoughts?

Tomi, unless you have the time to try implementing it via the clock 
framework, I'd suggest that the udelay() is probably the best approach to 
get something into 2.6.39.  I don't like the udelay() either, and I'd 
suggest marking it with a #warning or a big fat comment. 

Unfortunately I've run out of time to do any additional work for this 
merge window.


- Paul