Re: [PATCH 2/2] OMAPDSS: Ensure OPP100 when DSS is operational

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On Wed, 2012-04-18 at 10:03 -0700, Kevin Hilman wrote:
> [ Tomi, sorry for the delay.  I thought I had sent this a while back,
>   but found it in my drafts folder. ]
> 
> +Mike for clock comments
> 
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> 
> > On Tue, 2012-03-13 at 11:37 -0700, Kevin Hilman wrote:
> >> Hi Tomi,
> >> 
> >> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> >> 
> >> > Hi Kevin, Paul,
> >> >
> >> > I know you're busy, but I'd appreciate a comment/ack on these two small
> >> > patches, so I could get them in to next merge window. Otherwise using
> >> > any other OPP than OPP100 will most likely break the DSS.
> >> >
> >> > This looks quite straightforward fix for me, but I'm not sure if there
> >> > could be any side effects.
> >> 
> >> How does it affect OMAP3?  OPP50/OPP100 names are specific to OMAP4.
> >
> > They are? At least 3630 TRM speaks of them in the DSS chapter.
> >
> 
> The OPP names might exist, but the freq/voltage values are different
> between 3430, 3630 and 4430.
> 
> The point is that it's hard to make SoC independent code that depends on
> a particular OPP because OPPs are defined as SoC specific.

Hmm, true. But the OPP name is all we have (in TRM). And, if I
understand correctly, this restriction is a "feature" of the DSS
hardware, thus belongs to the DSS driver.

Is there such a concept as "full power", meaning OMAP's OPP100? Because
I think the table in the TRM could be defined in other words also:
OPP100 would mean "full power" or "normal" or similar, and OPP50 would
mean "any power saving mode which is not full power".

> >> Also, Can you help us understand the exact nature of the constraint?
> >
> > The TRM lists maximum clock rates for the DSS clocks. You should be able
> > to find them by searching "OPP100" in the DSS chapter. In my TRMs there
> > are:
> >
> > OMAP3630: Table 7-19. Display Subsystem Clocks
> > OMAP4430: Table 10-4. DSS Clock Frequencies
> >
> >> It sounds to me like it acutally is a throughput constraint on CORE.  If
> >> so, wouldn't it be clearer to set a throughput constraint that is
> >> calculated based on the pixel clock and resulting bitrate that would
> >> have the same effect?
> >
> > I don't see that these limits would have anything to do with CORE. I'm
> > guessing that the DSS HW just can't function properly with high clocks
> > and low voltage.
> 
> OK, this gets back to something we've talked about a few times that is
> needed in the clock framework.  Basically, we need a way for clocks to
> prevent changes so these kinds of dependencies can be tracked.  We've
> talked about new APIs like clk_allow_changes(), clk_block_changes() or
> something like that.
> 
> Basically, something that allows clocks to know that their will not be
> changed under them. 

I don't think that is the issue.

The clocks do not change to "bad" values accidentally. The DSS driver is
always in control of the clocks. So when the DSS driver sets a clock to
a frequency that requires OPP100, the driver knows this, and similarly,
when the DSS clocks are all below the required limits and we could use
OPP50, the DSS driver knows it.

Also, the DSS clocks that come from DSI and HDMI PLLs are DSS internal
clocks, they do not come from PRCM. So a clock framework change is not
enough.

So, as far as I understand, what is needed is a way for the DSS driver
to inform the power management layer that from this point forward the
DSS HW needs "full power". DSS driver would then use this method when it
knows OPP100 is required and when it knows OPP100 is no longer required.

> > Making a constraint for the throughput is another matter, which should
> > be also fixed at some point. So in the future I hope we'll have PM
> > constraints coming from two sources: 1) a calculation based on the
> > memory throughput needs 
> 
> This can be done today.  That is the purpose of the tput constraint.
> 
> > 2) the minimum clock rates.
> 
> Right, today we don't have a way do to this, and clock framework support
> will be needed.  I'll let Paul & Mike comment on that aspect, and
> hopefully we'll have something in common clock that will be able to
> handle this eventually.

Even if the clock framework would support this, we still have the DSS
internal clocks that have to be handled.

And taking this into a more generic direction, I don't think this is
strictly clock related. I'm not a HW guy so I could as well be totally
wrong here, but I imagine that we could have a feature in DSS HW that
only works with with OPP100. Something that, for whatever reason,
doesn't work with lower voltage levels.

Obviously we don't have to care much about imaginary problems, but a way
for the drivers to constraint the PM OPP level would solve those also.

> > But both of those are non-trivial to code, so this patch aims to keep
> > DSS working until those are implemented. Also, in practice, it's quite
> > rare that the DSS clocks would all be below the limits in the tables.
> > That could only happen with a fixed, known configuration with rather
> > small displays.
> 
> So, in summary, I have no objection $SUBJECT patch which implements the
> constraint using the only available method we have today.

I take that was an ack for these patches? =)

 Tomi


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