[PATCH 1/2] OMAPDSS: add set_min_bus_tput pointer to omapdss's platform data

[PATCH 1/2] OMAPDSS: add set_min_bus_tput pointer to omapdss's platform data

[Tomi Valkeinen]

omapdss driver needs to use the omap_pm_set_min_bus_tput(), so add a new
entry for that in omapdss's platform data, and set it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/display.c |    6 ++++++
 include/video/omapdss.h       |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 3677b1f..a675cab 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -179,6 +179,11 @@ static void omap_dsi_disable_pads(int dsi_id, unsigned lane_mask)
 		omap4_dsi_mux_pads(dsi_id, 0);
 }
 
+static int omap_dss_set_min_bus_tput(struct device *dev, unsigned long tput)
+{
+	return omap_pm_set_min_bus_tput(dev, OCP_INITIATOR_AGENT, tput);
+}
+
 int __init omap_display_init(struct omap_dss_board_info *board_data)
 {
 	int r = 0;
@@ -209,6 +214,7 @@ int __init omap_display_init(struct omap_dss_board_info *board_data)
 	pdata.board_data = board_data;
 	pdata.board_data->get_context_loss_count =
 		omap_pm_get_dev_context_loss_count;
+	pdata.board_data->set_min_bus_tput = omap_dss_set_min_bus_tput;
 
 	for (i = 0; i < oh_count; i++) {
 		oh = omap_hwmod_lookup(curr_dss_hwmod[i].oh_name);
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 483f67c..7aecadb 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -309,6 +309,7 @@ struct omap_dss_board_info {
 	struct omap_dss_device *default_device;
 	int (*dsi_enable_pads)(int dsi_id, unsigned lane_mask);
 	void (*dsi_disable_pads)(int dsi_id, unsigned lane_mask);
+	int (*set_min_bus_tput)(struct device *dev, unsigned long r);
 };
 
 /* Init with the board info */
-- 
1.7.4.1

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

[Tomi Valkeinen]

Most of the DSS clocks have restrictions on their frequency based on the
OPP in use. For example, maximum frequency for a clock may be 180MHz in
OPP100, but 90MHz in OPP50. This means that when a high enough pixel
clock or function clock is required, we need to use OPP100.

However, there's currently no way in the PM framework to make that kind
of request. The closest we get is to ask for very high bus throughput
from the PM framework, which should effectively force OPP100.

This patch is a simple version for handling the problem. Instead of
asking for OPP100 only when needed, this patch asks for OPP100 whenever
DSS is active. This obviously is not an optimal solution for cases with
small displays where OPP50 would work just fine. However, a proper
solution is a complex one, and this patch is a short term solution for
the problem.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 drivers/video/omap2/dss/core.c |   10 ++++++++++
 drivers/video/omap2/dss/dss.c  |   13 +++++++++++++
 drivers/video/omap2/dss/dss.h  |    1 +
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 8613f86..999bcbc 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -87,6 +87,16 @@ struct regulator *dss_get_vdds_sdi(void)
 	return reg;
 }
 
+int dss_set_min_bus_tput(struct device *dev, unsigned long tput)
+{
+	struct omap_dss_board_info *pdata = core.pdev->dev.platform_data;
+
+	if (pdata->set_min_bus_tput)
+		return pdata->set_min_bus_tput(dev, tput);
+	else
+		return 0;
+}
+
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
 static int dss_debug_show(struct seq_file *s, void *unused)
 {
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 4a6b5ee..4a3eb22 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -826,11 +826,24 @@ static int omap_dsshw_remove(struct platform_device *pdev)
 static int dss_runtime_suspend(struct device *dev)
 {
 	dss_save_context();
+	dss_set_min_bus_tput(dev, 0);
 	return 0;
 }
 
 static int dss_runtime_resume(struct device *dev)
 {
+	int r;
+	/*
+	 * Set an arbitrarily high tput request to ensure OPP100.
+	 * What we should really do is to make a request to stay in OPP100,
+	 * without any tput requirements, but that is not currently possible
+	 * via the PM layer.
+	 */
+
+	r = dss_set_min_bus_tput(dev, 1000000000);
+	if (r)
+		return r;
+
 	dss_restore_context();
 	return 0;
 }
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index d4b3dff..42f8f62 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -162,6 +162,7 @@ struct platform_device;
 struct bus_type *dss_get_bus(void);
 struct regulator *dss_get_vdds_dsi(void);
 struct regulator *dss_get_vdds_sdi(void);
+int dss_set_min_bus_tput(struct device *dev, unsigned long tput);
 
 /* apply */
 void dss_apply_init(void);
-- 
1.7.4.1

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

[Tomi Valkeinen]

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

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.

 Tomi

On Thu, 2012-03-08 at 13:13 +0200, Tomi Valkeinen wrote:
> Most of the DSS clocks have restrictions on their frequency based on the
> OPP in use. For example, maximum frequency for a clock may be 180MHz in
> OPP100, but 90MHz in OPP50. This means that when a high enough pixel
> clock or function clock is required, we need to use OPP100.
> 
> However, there's currently no way in the PM framework to make that kind
> of request. The closest we get is to ask for very high bus throughput
> from the PM framework, which should effectively force OPP100.
> 
> This patch is a simple version for handling the problem. Instead of
> asking for OPP100 only when needed, this patch asks for OPP100 whenever
> DSS is active. This obviously is not an optimal solution for cases with
> small displays where OPP50 would work just fine. However, a proper
> solution is a complex one, and this patch is a short term solution for
> the problem.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
>  drivers/video/omap2/dss/core.c |   10 ++++++++++
>  drivers/video/omap2/dss/dss.c  |   13 +++++++++++++
>  drivers/video/omap2/dss/dss.h  |    1 +
>  3 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
> index 8613f86..999bcbc 100644
> --- a/drivers/video/omap2/dss/core.c
> +++ b/drivers/video/omap2/dss/core.c
> @@ -87,6 +87,16 @@ struct regulator *dss_get_vdds_sdi(void)
>  	return reg;
>  }
>  
> +int dss_set_min_bus_tput(struct device *dev, unsigned long tput)
> +{
> +	struct omap_dss_board_info *pdata = core.pdev->dev.platform_data;
> +
> +	if (pdata->set_min_bus_tput)
> +		return pdata->set_min_bus_tput(dev, tput);
> +	else
> +		return 0;
> +}
> +
>  #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_OMAP2_DSS_DEBUG_SUPPORT)
>  static int dss_debug_show(struct seq_file *s, void *unused)
>  {
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 4a6b5ee..4a3eb22 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -826,11 +826,24 @@ static int omap_dsshw_remove(struct platform_device *pdev)
>  static int dss_runtime_suspend(struct device *dev)
>  {
>  	dss_save_context();
> +	dss_set_min_bus_tput(dev, 0);
>  	return 0;
>  }
>  
>  static int dss_runtime_resume(struct device *dev)
>  {
> +	int r;
> +	/*
> +	 * Set an arbitrarily high tput request to ensure OPP100.
> +	 * What we should really do is to make a request to stay in OPP100,
> +	 * without any tput requirements, but that is not currently possible
> +	 * via the PM layer.
> +	 */
> +
> +	r = dss_set_min_bus_tput(dev, 1000000000);
> +	if (r)
> +		return r;
> +
>  	dss_restore_context();
>  	return 0;
>  }
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index d4b3dff..42f8f62 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -162,6 +162,7 @@ struct platform_device;
>  struct bus_type *dss_get_bus(void);
>  struct regulator *dss_get_vdds_dsi(void);
>  struct regulator *dss_get_vdds_sdi(void);
> +int dss_set_min_bus_tput(struct device *dev, unsigned long tput);
>  
>  /* apply */
>  void dss_apply_init(void);


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

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

[Tomi Valkeinen]

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.

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

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 2) the minimum clock rates.

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.

 Tomi

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

[Tomi Valkeinen]

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

Ping.

 Tomi

On Wed, 2012-03-14 at 08:38 +0200, Tomi Valkeinen wrote:
> 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.
> 
> > 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.
> 
> 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 2) the minimum clock rates.
> 
> 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.
> 
>  Tomi
> 
> 


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

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

[Kevin Hilman]

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

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

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

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

Kevin

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

[Turquette, Mike]

On Wed, Apr 18, 2012 at 10:03 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>> On Tue, 2012-03-13 at 11:37 -0700, Kevin Hilman wrote:
>>> 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.

Are the DSS clocks changing frequency behind your back?  Or are the
clock rates staying the same while the voltage is dropped?

For the former issue Kevin is right that we need better clock
semantics.  For the latter issue hopefully the future dvfs
architecture will do the right thing (at least it does on paper).

Regards,
Mike
--
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: [PATCH 2/2] OMAPDSS: Ensure OPP100 when DSS is operational

[Tomi Valkeinen]

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

On Wed, 2012-04-18 at 10:26 -0700, Turquette, Mike wrote:
> On Wed, Apr 18, 2012 at 10:03 AM, Kevin Hilman <khilman@ti.com> wrote:
> > Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
> >> On Tue, 2012-03-13 at 11:37 -0700, Kevin Hilman wrote:
> >>> 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.
> 
> Are the DSS clocks changing frequency behind your back?  Or are the
> clock rates staying the same while the voltage is dropped?

The latter. The clocks do not change behind my back. It's the DSS driver
making the clock rate changes.

> For the former issue Kevin is right that we need better clock
> semantics.  For the latter issue hopefully the future dvfs
> architecture will do the right thing (at least it does on paper).

Note that many of the DSS clocks are generated inside DSS HW, and
managed by the DSS driver, and thus the clock framework doesn't know
anything about them. Will the future dvfs offer some way for the drivers
to limit the OPP?

 Tomi


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

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

[Turquette, Mike]

On Wed, Apr 18, 2012 at 10:06 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2012-04-18 at 10:26 -0700, Turquette, Mike wrote:
>> On Wed, Apr 18, 2012 at 10:03 AM, Kevin Hilman <khilman@ti.com> wrote:
>> > Tomi Valkeinen <tomi.valkeinen@ti.com> writes:
>> >> On Tue, 2012-03-13 at 11:37 -0700, Kevin Hilman wrote:
>> >>> 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.
>>
>> Are the DSS clocks changing frequency behind your back?  Or are the
>> clock rates staying the same while the voltage is dropped?
>
> The latter. The clocks do not change behind my back. It's the DSS driver
> making the clock rate changes.

Right.  The DVFS RFC that I'm working on right now should at least
take care of the voltage scaling down when it shouldn't.

>> For the former issue Kevin is right that we need better clock
>> semantics.  For the latter issue hopefully the future dvfs
>> architecture will do the right thing (at least it does on paper).
>
> Note that many of the DSS clocks are generated inside DSS HW, and
> managed by the DSS driver, and thus the clock framework doesn't know
> anything about them. Will the future dvfs offer some way for the drivers
> to limit the OPP?

It must!

In fact the common clk framework today (merged in 3.4-rc1) allows you
to model your DSS clocks in a common way that jives with the rest of
the SoC/PMIC/whatever.

Regards,
Mike
--
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: [PATCH 2/2] OMAPDSS: Ensure OPP100 when DSS is operational

[Tomi Valkeinen]

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

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

[Kevin Hilman]

Tomi Valkeinen <tomi.valkeinen@ti.com> writes:

> On Wed, 2012-04-18 at 10:03 -0700, Kevin Hilman wrote:

[...]

>> 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? =)

Yes.  A reluctant one, but since I have nothing better to offer you:

Acked-by: Kevin Hilman <khilman@ti.com>