[PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Mike Chan]

Signed-off-by: Mike Chan <mike@android.com>
---
 drivers/video/omap2/dss/dispc.c |    6 ++++-
 drivers/video/omap2/dss/dsi.c   |   50 +++++++++++++++++++++++++++++++++++---
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index d061d75..4216466 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -484,13 +484,17 @@ static inline void enable_clocks(bool enable)
 bool dispc_go_busy(enum omap_channel channel)
 {
 	int bit;
+	bool ret;
 
 	if (channel == OMAP_DSS_CHANNEL_LCD)
 		bit = 5; /* GOLCD */
 	else
 		bit = 6; /* GODIGIT */
 
-	return REG_GET(DISPC_CONTROL, bit, bit) == 1;
+	enable_clocks(1);
+	ret = REG_GET(DISPC_CONTROL, bit, bit) == 1;
+	enable_clocks(0);
+	return ret;
 }
 
 void dispc_go(enum omap_channel channel)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 5e3d653..e9d8a5f 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -1186,6 +1186,9 @@ int dsi_pll_init(bool enable_hsclk, bool enable_hsdiv)
 	if (r)
 		goto err1;
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	DSSDBG("PLL init done\n");
 
 	return 0;
@@ -1199,11 +1202,9 @@ err0:
 
 void dsi_pll_uninit(void)
 {
-	enable_clocks(0);
-	dsi_enable_pll_clock(0);
-
 	dsi.pll_locked = 0;
 	dsi_pll_power(DSI_PLL_POWER_OFF);
+
 	regulator_disable(dsi.vdds_dsi_reg);
 	DSSDBG("PLL uninit done\n");
 }
@@ -1848,6 +1849,9 @@ static int dsi_vc_send_bta_sync(int channel)
 
 	INIT_COMPLETION(dsi.bta_completion);
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	dsi_vc_enable_bta_irq(channel);
 
 	r = dsi_vc_send_bta(channel);
@@ -1870,6 +1874,9 @@ static int dsi_vc_send_bta_sync(int channel)
 err:
 	dsi_vc_disable_bta_irq(channel);
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	return r;
 }
 
@@ -1921,6 +1928,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
 		return -EINVAL;
 	}
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	dsi_vc_write_long_header(channel, data_type, len, ecc);
 
 	/*dsi_vc_print_status(0); */
@@ -1964,6 +1974,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
 		dsi_vc_write_long_payload(channel, b1, b2, b3, 0);
 	}
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	return r;
 }
 
@@ -1979,6 +1992,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
 				channel,
 				data_type, data & 0xff, (data >> 8) & 0xff);
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	if (FLD_GET(dsi_read_reg(DSI_VC_CTRL(channel)), 16, 16)) {
 		DSSERR("ERROR FIFO FULL, aborting transfer\n");
 		return -EINVAL;
@@ -1990,6 +2006,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
 
 	dsi_write_reg(DSI_VC_SHORT_PACKET_HEADER(channel), r);
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	return 0;
 }
 
@@ -2775,13 +2794,15 @@ static int dsi_update_thread(void *data)
 			break;
 
 		dsi_bus_lock();
-
 		if (dsi.update_mode == OMAP_DSS_UPDATE_DISABLED ||
 				kthread_should_stop()) {
 			dsi_bus_unlock();
 			break;
 		}
 
+		enable_clocks(1);
+		dsi_enable_pll_clock(1);
+
 		dsi_perf_mark_setup();
 
 		if (dsi.update_region.dirty) {
@@ -2872,6 +2893,9 @@ static int dsi_update_thread(void *data)
 
 		complete_all(&dsi.update_completion);
 
+		enable_clocks(0);
+		dsi_enable_pll_clock(0);
+
 		dsi_bus_unlock();
 
 		/* XXX We need to give others chance to get the bus lock. Is
@@ -3068,6 +3092,9 @@ static int dsi_display_enable(struct omap_dss_device *dssdev)
 	if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
 		dsi_start_auto_update(dssdev);
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	dsi_bus_unlock();
 	mutex_unlock(&dsi.lock);
 
@@ -3101,6 +3128,9 @@ static void dsi_display_disable(struct omap_dss_device *dssdev)
 	dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
 	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	dsi_display_uninit_dispc(dssdev);
 
 	dsi_display_uninit_dsi(dssdev);
@@ -3128,6 +3158,9 @@ static int dsi_display_suspend(struct omap_dss_device *dssdev)
 	dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
 	dssdev->state = OMAP_DSS_DISPLAY_SUSPENDED;
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	dsi_display_uninit_dispc(dssdev);
 
 	dsi_display_uninit_dsi(dssdev);
@@ -3181,6 +3214,9 @@ static int dsi_display_resume(struct omap_dss_device *dssdev)
 	if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
 		dsi_start_auto_update(dssdev);
 
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	dsi_bus_unlock();
 	mutex_unlock(&dsi.lock);
 
@@ -3325,6 +3361,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
 
 	dsi_bus_lock();
 
+	enable_clocks(1);
+	dsi_enable_pll_clock(1);
+
 	dsi.te_enabled = enable;
 
 	if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
@@ -3332,6 +3371,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
 
 	dsi_set_te(dssdev, enable);
 end:
+	enable_clocks(0);
+	dsi_enable_pll_clock(0);
+
 	dsi_bus_unlock();
 
 	return 0;
-- 
1.5.4.5

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Mike Chan]

On Thu, Sep 17, 2009 at 4:36 PM, Mike Chan <mike@android.com> wrote:
> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  drivers/video/omap2/dss/dispc.c |    6 ++++-
>  drivers/video/omap2/dss/dsi.c   |   50 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index d061d75..4216466 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -484,13 +484,17 @@ static inline void enable_clocks(bool enable)
>  bool dispc_go_busy(enum omap_channel channel)
>  {
>        int bit;
> +       bool ret;
>
>        if (channel == OMAP_DSS_CHANNEL_LCD)
>                bit = 5; /* GOLCD */
>        else
>                bit = 6; /* GODIGIT */
>
> -       return REG_GET(DISPC_CONTROL, bit, bit) == 1;
> +       enable_clocks(1);
> +       ret = REG_GET(DISPC_CONTROL, bit, bit) == 1;
> +       enable_clocks(0);
> +       return ret;
>  }
>
>  void dispc_go(enum omap_channel channel)
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 5e3d653..e9d8a5f 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1186,6 +1186,9 @@ int dsi_pll_init(bool enable_hsclk, bool enable_hsdiv)
>        if (r)
>                goto err1;
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        DSSDBG("PLL init done\n");
>
>        return 0;
> @@ -1199,11 +1202,9 @@ err0:
>
>  void dsi_pll_uninit(void)
>  {
> -       enable_clocks(0);
> -       dsi_enable_pll_clock(0);
> -
>        dsi.pll_locked = 0;
>        dsi_pll_power(DSI_PLL_POWER_OFF);
> +
>        regulator_disable(dsi.vdds_dsi_reg);
>        DSSDBG("PLL uninit done\n");
>  }
> @@ -1848,6 +1849,9 @@ static int dsi_vc_send_bta_sync(int channel)
>
>        INIT_COMPLETION(dsi.bta_completion);
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        dsi_vc_enable_bta_irq(channel);
>
>        r = dsi_vc_send_bta(channel);
> @@ -1870,6 +1874,9 @@ static int dsi_vc_send_bta_sync(int channel)
>  err:
>        dsi_vc_disable_bta_irq(channel);
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        return r;
>  }
>
> @@ -1921,6 +1928,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>                return -EINVAL;
>        }
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        dsi_vc_write_long_header(channel, data_type, len, ecc);
>
>        /*dsi_vc_print_status(0); */
> @@ -1964,6 +1974,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>                dsi_vc_write_long_payload(channel, b1, b2, b3, 0);
>        }
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        return r;
>  }
>
> @@ -1979,6 +1992,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>                                channel,
>                                data_type, data & 0xff, (data >> 8) & 0xff);
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        if (FLD_GET(dsi_read_reg(DSI_VC_CTRL(channel)), 16, 16)) {
>                DSSERR("ERROR FIFO FULL, aborting transfer\n");
>                return -EINVAL;
> @@ -1990,6 +2006,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>
>        dsi_write_reg(DSI_VC_SHORT_PACKET_HEADER(channel), r);
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        return 0;
>  }
>
> @@ -2775,13 +2794,15 @@ static int dsi_update_thread(void *data)
>                        break;
>
>                dsi_bus_lock();
> -
>                if (dsi.update_mode == OMAP_DSS_UPDATE_DISABLED ||
>                                kthread_should_stop()) {
>                        dsi_bus_unlock();
>                        break;
>                }
>
> +               enable_clocks(1);
> +               dsi_enable_pll_clock(1);
> +
>                dsi_perf_mark_setup();
>
>                if (dsi.update_region.dirty) {
> @@ -2872,6 +2893,9 @@ static int dsi_update_thread(void *data)
>
>                complete_all(&dsi.update_completion);
>
> +               enable_clocks(0);
> +               dsi_enable_pll_clock(0);
> +
>                dsi_bus_unlock();
>
>                /* XXX We need to give others chance to get the bus lock. Is
> @@ -3068,6 +3092,9 @@ static int dsi_display_enable(struct omap_dss_device *dssdev)
>        if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>                dsi_start_auto_update(dssdev);
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        dsi_bus_unlock();
>        mutex_unlock(&dsi.lock);
>
> @@ -3101,6 +3128,9 @@ static void dsi_display_disable(struct omap_dss_device *dssdev)
>        dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>        dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        dsi_display_uninit_dispc(dssdev);
>
>        dsi_display_uninit_dsi(dssdev);
> @@ -3128,6 +3158,9 @@ static int dsi_display_suspend(struct omap_dss_device *dssdev)
>        dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>        dssdev->state = OMAP_DSS_DISPLAY_SUSPENDED;
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        dsi_display_uninit_dispc(dssdev);
>
>        dsi_display_uninit_dsi(dssdev);
> @@ -3181,6 +3214,9 @@ static int dsi_display_resume(struct omap_dss_device *dssdev)
>        if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>                dsi_start_auto_update(dssdev);
>
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        dsi_bus_unlock();
>        mutex_unlock(&dsi.lock);
>
> @@ -3325,6 +3361,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>
>        dsi_bus_lock();
>
> +       enable_clocks(1);
> +       dsi_enable_pll_clock(1);
> +
>        dsi.te_enabled = enable;
>
>        if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> @@ -3332,6 +3371,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>
>        dsi_set_te(dssdev, enable);
>  end:
> +       enable_clocks(0);
> +       dsi_enable_pll_clock(0);
> +
>        dsi_bus_unlock();
>
>        return 0;
> --
> 1.5.4.5
>
>

Tomi this is based off of our android-omap-2.6.29 branch.

-- 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] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

Hi,

If you disable the clocks to allow RET, you also allow OFF mode. And
resuming from OFF mode hasn't been implemented for DSI, if I recall
right. And when I was testing it, it didn't seem to be trivial with the
DSI PLL.

That's why currently you need to disable the display to turn off the
clocks, and the whole DSI block is re-configured when enabling the
display again.

 Tomi


On Fri, 2009-09-18 at 01:36 +0200, ext Mike Chan wrote:
> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  drivers/video/omap2/dss/dispc.c |    6 ++++-
>  drivers/video/omap2/dss/dsi.c   |   50 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index d061d75..4216466 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -484,13 +484,17 @@ static inline void enable_clocks(bool enable)
>  bool dispc_go_busy(enum omap_channel channel)
>  {
>  	int bit;
> +	bool ret;
>  
>  	if (channel == OMAP_DSS_CHANNEL_LCD)
>  		bit = 5; /* GOLCD */
>  	else
>  		bit = 6; /* GODIGIT */
>  
> -	return REG_GET(DISPC_CONTROL, bit, bit) == 1;
> +	enable_clocks(1);
> +	ret = REG_GET(DISPC_CONTROL, bit, bit) == 1;
> +	enable_clocks(0);
> +	return ret;
>  }
>  
>  void dispc_go(enum omap_channel channel)
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 5e3d653..e9d8a5f 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1186,6 +1186,9 @@ int dsi_pll_init(bool enable_hsclk, bool enable_hsdiv)
>  	if (r)
>  		goto err1;
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	DSSDBG("PLL init done\n");
>  
>  	return 0;
> @@ -1199,11 +1202,9 @@ err0:
>  
>  void dsi_pll_uninit(void)
>  {
> -	enable_clocks(0);
> -	dsi_enable_pll_clock(0);
> -
>  	dsi.pll_locked = 0;
>  	dsi_pll_power(DSI_PLL_POWER_OFF);
> +
>  	regulator_disable(dsi.vdds_dsi_reg);
>  	DSSDBG("PLL uninit done\n");
>  }
> @@ -1848,6 +1849,9 @@ static int dsi_vc_send_bta_sync(int channel)
>  
>  	INIT_COMPLETION(dsi.bta_completion);
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	dsi_vc_enable_bta_irq(channel);
>  
>  	r = dsi_vc_send_bta(channel);
> @@ -1870,6 +1874,9 @@ static int dsi_vc_send_bta_sync(int channel)
>  err:
>  	dsi_vc_disable_bta_irq(channel);
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	return r;
>  }
>  
> @@ -1921,6 +1928,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>  		return -EINVAL;
>  	}
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	dsi_vc_write_long_header(channel, data_type, len, ecc);
>  
>  	/*dsi_vc_print_status(0); */
> @@ -1964,6 +1974,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>  		dsi_vc_write_long_payload(channel, b1, b2, b3, 0);
>  	}
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	return r;
>  }
>  
> @@ -1979,6 +1992,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>  				channel,
>  				data_type, data & 0xff, (data >> 8) & 0xff);
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	if (FLD_GET(dsi_read_reg(DSI_VC_CTRL(channel)), 16, 16)) {
>  		DSSERR("ERROR FIFO FULL, aborting transfer\n");
>  		return -EINVAL;
> @@ -1990,6 +2006,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>  
>  	dsi_write_reg(DSI_VC_SHORT_PACKET_HEADER(channel), r);
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	return 0;
>  }
>  
> @@ -2775,13 +2794,15 @@ static int dsi_update_thread(void *data)
>  			break;
>  
>  		dsi_bus_lock();
> -
>  		if (dsi.update_mode == OMAP_DSS_UPDATE_DISABLED ||
>  				kthread_should_stop()) {
>  			dsi_bus_unlock();
>  			break;
>  		}
>  
> +		enable_clocks(1);
> +		dsi_enable_pll_clock(1);
> +
>  		dsi_perf_mark_setup();
>  
>  		if (dsi.update_region.dirty) {
> @@ -2872,6 +2893,9 @@ static int dsi_update_thread(void *data)
>  
>  		complete_all(&dsi.update_completion);
>  
> +		enable_clocks(0);
> +		dsi_enable_pll_clock(0);
> +
>  		dsi_bus_unlock();
>  
>  		/* XXX We need to give others chance to get the bus lock. Is
> @@ -3068,6 +3092,9 @@ static int dsi_display_enable(struct omap_dss_device *dssdev)
>  	if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>  		dsi_start_auto_update(dssdev);
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	dsi_bus_unlock();
>  	mutex_unlock(&dsi.lock);
>  
> @@ -3101,6 +3128,9 @@ static void dsi_display_disable(struct omap_dss_device *dssdev)
>  	dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>  	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	dsi_display_uninit_dispc(dssdev);
>  
>  	dsi_display_uninit_dsi(dssdev);
> @@ -3128,6 +3158,9 @@ static int dsi_display_suspend(struct omap_dss_device *dssdev)
>  	dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>  	dssdev->state = OMAP_DSS_DISPLAY_SUSPENDED;
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	dsi_display_uninit_dispc(dssdev);
>  
>  	dsi_display_uninit_dsi(dssdev);
> @@ -3181,6 +3214,9 @@ static int dsi_display_resume(struct omap_dss_device *dssdev)
>  	if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>  		dsi_start_auto_update(dssdev);
>  
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	dsi_bus_unlock();
>  	mutex_unlock(&dsi.lock);
>  
> @@ -3325,6 +3361,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>  
>  	dsi_bus_lock();
>  
> +	enable_clocks(1);
> +	dsi_enable_pll_clock(1);
> +
>  	dsi.te_enabled = enable;
>  
>  	if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> @@ -3332,6 +3371,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>  
>  	dsi_set_te(dssdev, enable);
>  end:
> +	enable_clocks(0);
> +	dsi_enable_pll_clock(0);
> +
>  	dsi_bus_unlock();
>  
>  	return 0;

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Mike Chan]

On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
<tomi.valkeinen@nokia.com> wrote:
> Hi,
>
> If you disable the clocks to allow RET, you also allow OFF mode. And
> resuming from OFF mode hasn't been implemented for DSI, if I recall
> right. And when I was testing it, it didn't seem to be trivial with the
> DSI PLL.
>

You can limiting the pwrdm next state to RET when being called from cpuidle.

> That's why currently you need to disable the display to turn off the
> clocks, and the whole DSI block is re-configured when enabling the
> display again.
>

Display enable/disable should only be called when you turn on / off
the screen, I'm not sure how this applies to getting the dss_pwrdm to
enter RET on idle while scren is on.

We haven't tested OFF, but having all power domains enter RET while
screen is on and device idle (no rendering) can give significant (~2x)
battery life improvements, depending on your hardware of course.


-- Mike

>  Tomi
>
>
> On Fri, 2009-09-18 at 01:36 +0200, ext Mike Chan wrote:
>> Signed-off-by: Mike Chan <mike@android.com>
>> ---
>>  drivers/video/omap2/dss/dispc.c |    6 ++++-
>>  drivers/video/omap2/dss/dsi.c   |   50 +++++++++++++++++++++++++++++++++++---
>>  2 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index d061d75..4216466 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -484,13 +484,17 @@ static inline void enable_clocks(bool enable)
>>  bool dispc_go_busy(enum omap_channel channel)
>>  {
>>       int bit;
>> +     bool ret;
>>
>>       if (channel == OMAP_DSS_CHANNEL_LCD)
>>               bit = 5; /* GOLCD */
>>       else
>>               bit = 6; /* GODIGIT */
>>
>> -     return REG_GET(DISPC_CONTROL, bit, bit) == 1;
>> +     enable_clocks(1);
>> +     ret = REG_GET(DISPC_CONTROL, bit, bit) == 1;
>> +     enable_clocks(0);
>> +     return ret;
>>  }
>>
>>  void dispc_go(enum omap_channel channel)
>> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
>> index 5e3d653..e9d8a5f 100644
>> --- a/drivers/video/omap2/dss/dsi.c
>> +++ b/drivers/video/omap2/dss/dsi.c
>> @@ -1186,6 +1186,9 @@ int dsi_pll_init(bool enable_hsclk, bool enable_hsdiv)
>>       if (r)
>>               goto err1;
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       DSSDBG("PLL init done\n");
>>
>>       return 0;
>> @@ -1199,11 +1202,9 @@ err0:
>>
>>  void dsi_pll_uninit(void)
>>  {
>> -     enable_clocks(0);
>> -     dsi_enable_pll_clock(0);
>> -
>>       dsi.pll_locked = 0;
>>       dsi_pll_power(DSI_PLL_POWER_OFF);
>> +
>>       regulator_disable(dsi.vdds_dsi_reg);
>>       DSSDBG("PLL uninit done\n");
>>  }
>> @@ -1848,6 +1849,9 @@ static int dsi_vc_send_bta_sync(int channel)
>>
>>       INIT_COMPLETION(dsi.bta_completion);
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       dsi_vc_enable_bta_irq(channel);
>>
>>       r = dsi_vc_send_bta(channel);
>> @@ -1870,6 +1874,9 @@ static int dsi_vc_send_bta_sync(int channel)
>>  err:
>>       dsi_vc_disable_bta_irq(channel);
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       return r;
>>  }
>>
>> @@ -1921,6 +1928,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>>               return -EINVAL;
>>       }
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       dsi_vc_write_long_header(channel, data_type, len, ecc);
>>
>>       /*dsi_vc_print_status(0); */
>> @@ -1964,6 +1974,9 @@ static int dsi_vc_send_long(int channel, u8 data_type, u8 *data, u16 len,
>>               dsi_vc_write_long_payload(channel, b1, b2, b3, 0);
>>       }
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       return r;
>>  }
>>
>> @@ -1979,6 +1992,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>>                               channel,
>>                               data_type, data & 0xff, (data >> 8) & 0xff);
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       if (FLD_GET(dsi_read_reg(DSI_VC_CTRL(channel)), 16, 16)) {
>>               DSSERR("ERROR FIFO FULL, aborting transfer\n");
>>               return -EINVAL;
>> @@ -1990,6 +2006,9 @@ static int dsi_vc_send_short(int channel, u8 data_type, u16 data, u8 ecc)
>>
>>       dsi_write_reg(DSI_VC_SHORT_PACKET_HEADER(channel), r);
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       return 0;
>>  }
>>
>> @@ -2775,13 +2794,15 @@ static int dsi_update_thread(void *data)
>>                       break;
>>
>>               dsi_bus_lock();
>> -
>>               if (dsi.update_mode == OMAP_DSS_UPDATE_DISABLED ||
>>                               kthread_should_stop()) {
>>                       dsi_bus_unlock();
>>                       break;
>>               }
>>
>> +             enable_clocks(1);
>> +             dsi_enable_pll_clock(1);
>> +
>>               dsi_perf_mark_setup();
>>
>>               if (dsi.update_region.dirty) {
>> @@ -2872,6 +2893,9 @@ static int dsi_update_thread(void *data)
>>
>>               complete_all(&dsi.update_completion);
>>
>> +             enable_clocks(0);
>> +             dsi_enable_pll_clock(0);
>> +
>>               dsi_bus_unlock();
>>
>>               /* XXX We need to give others chance to get the bus lock. Is
>> @@ -3068,6 +3092,9 @@ static int dsi_display_enable(struct omap_dss_device *dssdev)
>>       if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>>               dsi_start_auto_update(dssdev);
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       dsi_bus_unlock();
>>       mutex_unlock(&dsi.lock);
>>
>> @@ -3101,6 +3128,9 @@ static void dsi_display_disable(struct omap_dss_device *dssdev)
>>       dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>>       dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       dsi_display_uninit_dispc(dssdev);
>>
>>       dsi_display_uninit_dsi(dssdev);
>> @@ -3128,6 +3158,9 @@ static int dsi_display_suspend(struct omap_dss_device *dssdev)
>>       dsi.update_mode = OMAP_DSS_UPDATE_DISABLED;
>>       dssdev->state = OMAP_DSS_DISPLAY_SUSPENDED;
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       dsi_display_uninit_dispc(dssdev);
>>
>>       dsi_display_uninit_dsi(dssdev);
>> @@ -3181,6 +3214,9 @@ static int dsi_display_resume(struct omap_dss_device *dssdev)
>>       if (dsi.update_mode == OMAP_DSS_UPDATE_AUTO)
>>               dsi_start_auto_update(dssdev);
>>
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       dsi_bus_unlock();
>>       mutex_unlock(&dsi.lock);
>>
>> @@ -3325,6 +3361,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>>
>>       dsi_bus_lock();
>>
>> +     enable_clocks(1);
>> +     dsi_enable_pll_clock(1);
>> +
>>       dsi.te_enabled = enable;
>>
>>       if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
>> @@ -3332,6 +3371,9 @@ static int dsi_display_enable_te(struct omap_dss_device *dssdev, bool enable)
>>
>>       dsi_set_te(dssdev, enable);
>>  end:
>> +     enable_clocks(0);
>> +     dsi_enable_pll_clock(0);
>> +
>>       dsi_bus_unlock();
>>
>>       return 0;
>
>
--
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] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

Hi,

On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
> <tomi.valkeinen@nokia.com> wrote:
> > Hi,
> >
> > If you disable the clocks to allow RET, you also allow OFF mode. And
> > resuming from OFF mode hasn't been implemented for DSI, if I recall
> > right. And when I was testing it, it didn't seem to be trivial with the
> > DSI PLL.
> >
> 
> You can limiting the pwrdm next state to RET when being called from cpuidle.

No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
can either keep the dsi code as it is now, or explicitely disable OFF
mode and then apply your patch. But your patch alone won't work.

In the long run I think we anyway need to somehow dynamically manage the
power state. I haven't measured it but I believe resuming from OFF will
have a bit of a penalty, as (I think) DSI PLL etc. will have to
reinitialized. But it would still be good to allow RET whenever
possible, and OFF only after some period of inactivity.

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

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

> Hi,
>
> On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
>> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
>> <tomi.valkeinen@nokia.com> wrote:
>> > Hi,
>> >
>> > If you disable the clocks to allow RET, you also allow OFF mode. And
>> > resuming from OFF mode hasn't been implemented for DSI, if I recall
>> > right. And when I was testing it, it didn't seem to be trivial with the
>> > DSI PLL.
>> >
>> 
>> You can limiting the pwrdm next state to RET when being called from cpuidle.
>
> No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
> can either keep the dsi code as it is now, or explicitely disable OFF
> mode and then apply your patch. But your patch alone won't work.

Or could add a hack to this patch so that 'enable_off_mode' doesn't
affect DSS_MOD until DSS has off-mode support.

> In the long run I think we anyway need to somehow dynamically manage the
> power state. I haven't measured it but I believe resuming from OFF will
> have a bit of a penalty, as (I think) DSI PLL etc. will have to
> reinitialized. But it would still be good to allow RET whenever
> possible, and OFF only after some period of inactivity.

This is the purpose of latency constraints.  These can be used when
the latency of going OFF will cause a problem.

Kevin

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

On Tue, 2009-09-22 at 16:54 +0200, ext Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
> 
> > Hi,
> >
> > On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
> >> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
> >> <tomi.valkeinen@nokia.com> wrote:
> >> > Hi,
> >> >
> >> > If you disable the clocks to allow RET, you also allow OFF mode. And
> >> > resuming from OFF mode hasn't been implemented for DSI, if I recall
> >> > right. And when I was testing it, it didn't seem to be trivial with the
> >> > DSI PLL.
> >> >
> >> 
> >> You can limiting the pwrdm next state to RET when being called from cpuidle.
> >
> > No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
> > can either keep the dsi code as it is now, or explicitely disable OFF
> > mode and then apply your patch. But your patch alone won't work.
> 
> Or could add a hack to this patch so that 'enable_off_mode' doesn't
> affect DSS_MOD until DSS has off-mode support.

Is this something that the driver can do with the current PM API? The
thing is, most of the DSS has off-mode support, only DSI and RFBI is
missing the support. So optimally we would allow full PM normally, but
when DSI display is in use, only allow RET.

> 
> > In the long run I think we anyway need to somehow dynamically manage the
> > power state. I haven't measured it but I believe resuming from OFF will
> > have a bit of a penalty, as (I think) DSI PLL etc. will have to
> > reinitialized. But it would still be good to allow RET whenever
> > possible, and OFF only after some period of inactivity.
> 
> This is the purpose of latency constraints.  These can be used when
> the latency of going OFF will cause a problem.

I think this is a different problem. My understanding of the PM latency
functionality is that they control how quickly MPU responds to
interrupts, or how quickly the HW module is awake after clk_enable().

The problem here is that there's bunch of DSI initialization that needs
to be done after the DSS module has woken up. The DSI PLL needs to be
configured and locked, complex IO has to be configured etc. I don't know
how long those take (should make some measurements at some point), but I
imagine they are not anywhere near instant. And the latency from those
things is what I'm concerned about. So it would be beneficial if the DSS
driver could define easily what DSS PM level is allowed.

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

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

> On Tue, 2009-09-22 at 16:54 +0200, ext Kevin Hilman wrote:
>> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
>> 
>> > Hi,
>> >
>> > On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
>> >> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
>> >> <tomi.valkeinen@nokia.com> wrote:
>> >> > Hi,
>> >> >
>> >> > If you disable the clocks to allow RET, you also allow OFF mode. And
>> >> > resuming from OFF mode hasn't been implemented for DSI, if I recall
>> >> > right. And when I was testing it, it didn't seem to be trivial with the
>> >> > DSI PLL.
>> >> >
>> >> 
>> >> You can limiting the pwrdm next state to RET when being called from cpuidle.
>> >
>> > No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
>> > can either keep the dsi code as it is now, or explicitely disable OFF
>> > mode and then apply your patch. But your patch alone won't work.
>> 
>> Or could add a hack to this patch so that 'enable_off_mode' doesn't
>> affect DSS_MOD until DSS has off-mode support.
>
> Is this something that the driver can do with the current PM API? The
> thing is, most of the DSS has off-mode support, only DSI and RFBI is
> missing the support. So optimally we would allow full PM normally, but
> when DSI display is in use, only allow RET.

There's not an official API for this, but for testing, you could just
set the next state for the DSS_MOD to be off.  This would be overridden
whenever someone changes the enable_off_mode flag.

In general, we don't want drivers restricting states at that level.
We'd rather the drivers use the OMAP PM layer to define/declare its
constraints.

>> 
>> > In the long run I think we anyway need to somehow dynamically manage the
>> > power state. I haven't measured it but I believe resuming from OFF will
>> > have a bit of a penalty, as (I think) DSI PLL etc. will have to
>> > reinitialized. But it would still be good to allow RET whenever
>> > possible, and OFF only after some period of inactivity.
>> 
>> This is the purpose of latency constraints.  These can be used when
>> the latency of going OFF will cause a problem.
>
> I think this is a different problem. My understanding of the PM latency
> functionality is that they control how quickly MPU responds to
> interrupts, or how quickly the HW module is awake after clk_enable().

That's only part of the story.

> The problem here is that there's bunch of DSI initialization that needs
> to be done after the DSS module has woken up. The DSI PLL needs to be
> configured and locked, complex IO has to be configured etc. I don't know
> how long those take (should make some measurements at some point), but I
> imagine they are not anywhere near instant. And the latency from those
> things is what I'm concerned about. So it would be beneficial if the DSS
> driver could define easily what DSS PM level is allowed.

Actually, the OMAP PM layer is meant to handle device wakeup latencies
as well.  See Documentation/arm/OMAP/omap_pm, and the
set_max_dev_wakeup_lat() hook in particular.

For example, if you know that DSS wakeup from OFF takes X usecs and
the driver has set a max device wakeup latency < X, then DSS will
never go to OFF.  The same can be done to prevent RET for even faster
wakeup.

Kevin

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

On Wed, 2009-09-23 at 17:44 +0200, ext Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
> 
> > On Tue, 2009-09-22 at 16:54 +0200, ext Kevin Hilman wrote:
> >> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
> >> 
> >> > Hi,
> >> >
> >> > On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
> >> >> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
> >> >> <tomi.valkeinen@nokia.com> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > If you disable the clocks to allow RET, you also allow OFF mode. And
> >> >> > resuming from OFF mode hasn't been implemented for DSI, if I recall
> >> >> > right. And when I was testing it, it didn't seem to be trivial with the
> >> >> > DSI PLL.
> >> >> >
> >> >> 
> >> >> You can limiting the pwrdm next state to RET when being called from cpuidle.
> >> >
> >> > No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
> >> > can either keep the dsi code as it is now, or explicitely disable OFF
> >> > mode and then apply your patch. But your patch alone won't work.
> >> 
> >> Or could add a hack to this patch so that 'enable_off_mode' doesn't
> >> affect DSS_MOD until DSS has off-mode support.
> >
> > Is this something that the driver can do with the current PM API? The
> > thing is, most of the DSS has off-mode support, only DSI and RFBI is
> > missing the support. So optimally we would allow full PM normally, but
> > when DSI display is in use, only allow RET.
> 
> There's not an official API for this, but for testing, you could just
> set the next state for the DSS_MOD to be off.  This would be overridden
> whenever someone changes the enable_off_mode flag.
> 
> In general, we don't want drivers restricting states at that level.
> We'd rather the drivers use the OMAP PM layer to define/declare its
> constraints.

If I want to disable OFF mode for DSS with constraints, doesn't it mean
that I need to dig out a magic number, and use it as the max wakeup
latency, and that latency will then limit DSS from going to OFF mode?

> 
> >> 
> >> > In the long run I think we anyway need to somehow dynamically manage the
> >> > power state. I haven't measured it but I believe resuming from OFF will
> >> > have a bit of a penalty, as (I think) DSI PLL etc. will have to
> >> > reinitialized. But it would still be good to allow RET whenever
> >> > possible, and OFF only after some period of inactivity.
> >> 
> >> This is the purpose of latency constraints.  These can be used when
> >> the latency of going OFF will cause a problem.
> >
> > I think this is a different problem. My understanding of the PM latency
> > functionality is that they control how quickly MPU responds to
> > interrupts, or how quickly the HW module is awake after clk_enable().
> 
> That's only part of the story.
> 
> > The problem here is that there's bunch of DSI initialization that needs
> > to be done after the DSS module has woken up. The DSI PLL needs to be
> > configured and locked, complex IO has to be configured etc. I don't know
> > how long those take (should make some measurements at some point), but I
> > imagine they are not anywhere near instant. And the latency from those
> > things is what I'm concerned about. So it would be beneficial if the DSS
> > driver could define easily what DSS PM level is allowed.
> 
> Actually, the OMAP PM layer is meant to handle device wakeup latencies
> as well.  See Documentation/arm/OMAP/omap_pm, and the
> set_max_dev_wakeup_lat() hook in particular.
> 
> For example, if you know that DSS wakeup from OFF takes X usecs and
> the driver has set a max device wakeup latency < X, then DSS will
> never go to OFF.  The same can be done to prevent RET for even faster
> wakeup.

The doc says "set_max_dev_wakeup_lat() function to constrain the device
wakeup latency (from clk_enable() to accessibility)". I understood
"accessibility" meaning that the HW registers can be accessed, and also
as registers should be accessible after clk_enable(), it would mean that
clk_enable will block at most the defined wakeup latency.

If it is not like that, and the driver initialization is included, how
does the PM layer know how long it takes for the DSS driver to
reconfigure the DSS hardware from OFF mode?

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

Tomi Valkeinen wrote:
> On Wed, 2009-09-23 at 17:44 +0200, ext Kevin Hilman wrote:
>> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
>>
>>> On Tue, 2009-09-22 at 16:54 +0200, ext Kevin Hilman wrote:
>>>> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
>>>>
>>>>> Hi,
>>>>>
>>>>> On Fri, 2009-09-18 at 19:33 +0200, ext Mike Chan wrote:
>>>>>> On Fri, Sep 18, 2009 at 1:27 AM, Tomi Valkeinen
>>>>>> <tomi.valkeinen@nokia.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> If you disable the clocks to allow RET, you also allow OFF mode. And
>>>>>>> resuming from OFF mode hasn't been implemented for DSI, if I recall
>>>>>>> right. And when I was testing it, it didn't seem to be trivial with the
>>>>>>> DSI PLL.
>>>>>>>
>>>>>> You can limiting the pwrdm next state to RET when being called from cpuidle.
>>>>> No, you _must_ limit it to RET. Otherwise the DSI will break down. So we
>>>>> can either keep the dsi code as it is now, or explicitely disable OFF
>>>>> mode and then apply your patch. But your patch alone won't work.
>>>> Or could add a hack to this patch so that 'enable_off_mode' doesn't
>>>> affect DSS_MOD until DSS has off-mode support.
>>> Is this something that the driver can do with the current PM API? The
>>> thing is, most of the DSS has off-mode support, only DSI and RFBI is
>>> missing the support. So optimally we would allow full PM normally, but
>>> when DSI display is in use, only allow RET.
>> There's not an official API for this, but for testing, you could just
>> set the next state for the DSS_MOD to be off.  This would be overridden
>> whenever someone changes the enable_off_mode flag.
>>
>> In general, we don't want drivers restricting states at that level.
>> We'd rather the drivers use the OMAP PM layer to define/declare its
>> constraints.
> 
> If I want to disable OFF mode for DSS with constraints, doesn't it mean
> that I need to dig out a magic number, and use it as the max wakeup
> latency, and that latency will then limit DSS from going to OFF mode?

Yes, the various wakeup times for RET and OFF should be measured so that 
smart decisions can be made.

>>>>> In the long run I think we anyway need to somehow dynamically manage the
>>>>> power state. I haven't measured it but I believe resuming from OFF will
>>>>> have a bit of a penalty, as (I think) DSI PLL etc. will have to
>>>>> reinitialized. But it would still be good to allow RET whenever
>>>>> possible, and OFF only after some period of inactivity.
>>>> This is the purpose of latency constraints.  These can be used when
>>>> the latency of going OFF will cause a problem.
>>> I think this is a different problem. My understanding of the PM latency
>>> functionality is that they control how quickly MPU responds to
>>> interrupts, or how quickly the HW module is awake after clk_enable().
>> That's only part of the story.
>>
>>> The problem here is that there's bunch of DSI initialization that needs
>>> to be done after the DSS module has woken up. The DSI PLL needs to be
>>> configured and locked, complex IO has to be configured etc. I don't know
>>> how long those take (should make some measurements at some point), but I
>>> imagine they are not anywhere near instant. And the latency from those
>>> things is what I'm concerned about. So it would be beneficial if the DSS
>>> driver could define easily what DSS PM level is allowed.
>> Actually, the OMAP PM layer is meant to handle device wakeup latencies
>> as well.  See Documentation/arm/OMAP/omap_pm, and the
>> set_max_dev_wakeup_lat() hook in particular.
>>
>> For example, if you know that DSS wakeup from OFF takes X usecs and
>> the driver has set a max device wakeup latency < X, then DSS will
>> never go to OFF.  The same can be done to prevent RET for even faster
>> wakeup.
> 
> The doc says "set_max_dev_wakeup_lat() function to constrain the device
> wakeup latency (from clk_enable() to accessibility)". I understood
> "accessibility" meaning that the HW registers can be accessed, and also
> as registers should be accessible after clk_enable(), it would mean that
> clk_enable will block at most the defined wakeup latency.

Not exactly, clk_enable() doesn't actually block.  Rather, the max 
latencies are intended to be used when considering which C-state to 
enter.  If a C-state has a wakeup latency longer than any of the latency 
constraints set (MPU or device) then that C-state will not be entered.

In this way you effectively prevent OFF mode (deeper C-state) by setting 
a max latency which is shorter than the wakeup latency of OFF mode.

Right now you could do it simply by setting a max MPU wakeup latency, 
but we really need to measure the various device wakeup latencies so we 
can use them as well.

> If it is not like that, and the driver initialization is included, how
> does the PM layer know how long it takes for the DSS driver to
> reconfigure the DSS hardware from OFF mode?

Currently it doesn't, but if you were measure it, we can use those 
numbers in the decision making process.

Kevin

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

On Thu, 2009-09-24 at 17:52 +0200, ext Kevin Hilman wrote:
> Tomi Valkeinen wrote:

> > If it is not like that, and the driver initialization is included, how
> > does the PM layer know how long it takes for the DSS driver to
> > reconfigure the DSS hardware from OFF mode?
> 
> Currently it doesn't, but if you were measure it, we can use those 
> numbers in the decision making process.

Ok, now I see. However, I'm not sure if that will work. The problem is
that the wakeup latency depends on many things. When using DPI/RFBI the
wakeup is very fast. With SDI it's probably a bit slower and with DSI
even slower.

And at least with DSI PLL, the wakeup time depends on the frequencies
used (according to TRM), and in some cases it can be optimized, in some
cases not. So I don't think there's one single value that fits all.

Also, I still think it would be better if the driver was also able to
prevent OFF mode explicitely. Defining the max-wakeup-lat with a magic
number sounds a bit prone to breaking up.

But perhaps, as you said, when drivers work properly they don't have to
care about OFF mode as such, but only about the wakeup latency, and thus
the max-wakeup-lat is enough. I'm just not quite sure about that, as OFF
mode may have side effects as the module is totally powered off, while
with RET the side effects should be minimal.

I don't have any concrete example about the side effects, but one
particular thing I'm thinking about is DSI PLL. If DSS is in RET, I
believe DSI PLL works normally. But if the DSS is reset via OFF mode, I
believe DSI PLL is also reset. But I'm not sure if DSI PLL is ever
needed while DSS would be off, so this may be theoretical =).

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

On Fri, Sep 25, 2009 at 2:19 AM, Tomi Valkeinen
<tomi.valkeinen@nokia.com> wrote:
> On Thu, 2009-09-24 at 17:52 +0200, ext Kevin Hilman wrote:
>> Tomi Valkeinen wrote:
>
>> > If it is not like that, and the driver initialization is included, how
>> > does the PM layer know how long it takes for the DSS driver to
>> > reconfigure the DSS hardware from OFF mode?
>>
>> Currently it doesn't, but if you were measure it, we can use those
>> numbers in the decision making process.
>
> Ok, now I see. However, I'm not sure if that will work. The problem is
> that the wakeup latency depends on many things. When using DPI/RFBI the
> wakeup is very fast. With SDI it's probably a bit slower and with DSI
> even slower.

The varying latencies are not an issues.  When in the different modes,
just register a different latency requirement.

> And at least with DSI PLL, the wakeup time depends on the frequencies
> used (according to TRM), and in some cases it can be optimized, in some
> cases not. So I don't think there's one single value that fits all.

A single value isn't necessary.

> Also, I still think it would be better if the driver was also able to
> prevent OFF mode explicitely. Defining the max-wakeup-lat with a magic
> number sounds a bit prone to breaking up.

I disagree.  What is important is that the driver communicates *why*
it needs to prevent OFF mode (can't handle the latency etc.)  and
decision making up to the PM core.  The drivers should not embed
policy in them.

> But perhaps, as you said, when drivers work properly they don't have to
> care about OFF mode as such, but only about the wakeup latency, and thus
> the max-wakeup-lat is enough. I'm just not quite sure about that, as OFF
> mode may have side effects as the module is totally powered off, while
> with RET the side effects should be minimal.
>
> I don't have any concrete example about the side effects, but one
> particular thing I'm thinking about is DSI PLL. If DSS is in RET, I
> believe DSI PLL works normally. But if the DSS is reset via OFF mode, I
> believe DSI PLL is also reset. But I'm not sure if DSI PLL is ever
> needed while DSS would be off, so this may be theoretical =).
>
>  Tomi

This problem is not unique to DSS, and the other drivers are handling this.

Therefore, I'm inclined to merge this patch to that it's clear that
DSS support for OFF mode is broken and needs to be fixed.

As a *temporary*, debug solution, I might accept a hack where DSS2
disables OFF mode explicitly, but this will not go upstream.

Kevin
--
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] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

On Wed, 2009-09-30 at 20:31 +0200, ext Kevin Hilman wrote:
> On Fri, Sep 25, 2009 at 2:19 AM, Tomi Valkeinen
> <tomi.valkeinen@nokia.com> wrote:
> > On Thu, 2009-09-24 at 17:52 +0200, ext Kevin Hilman wrote:
> >> Tomi Valkeinen wrote:
> >
> >> > If it is not like that, and the driver initialization is included, how
> >> > does the PM layer know how long it takes for the DSS driver to
> >> > reconfigure the DSS hardware from OFF mode?
> >>
> >> Currently it doesn't, but if you were measure it, we can use those
> >> numbers in the decision making process.
> >
> > Ok, now I see. However, I'm not sure if that will work. The problem is
> > that the wakeup latency depends on many things. When using DPI/RFBI the
> > wakeup is very fast. With SDI it's probably a bit slower and with DSI
> > even slower.
> 
> The varying latencies are not an issues.  When in the different modes,
> just register a different latency requirement.

How does that help? The problem is not that there are different max
latency requirements in different modes, but that the actual wake-up
latency varies.

I found some latency values in resource34xx.h. If they are what I
presume they are, they define that waking DSS from OFF takes 70us. Let's
presume it's correct for DPI, so reconfiguring DSS for DPI use takes
~70us.

But reconfiguring DSS for DSI use may take, say, 5000us. So if I set max
latency req to 100us, OFF mode will be "enabled", and it will work fine
for DPI. But for DSI we will get lantencies around 5000us.

So is there an API to change that value in resource34xx.h dynamically,
depending on what DSS block is in use? Or am I still missing something
here? =)

> 
> > And at least with DSI PLL, the wakeup time depends on the frequencies
> > used (according to TRM), and in some cases it can be optimized, in some
> > cases not. So I don't think there's one single value that fits all.
> 
> A single value isn't necessary.
> 
> > Also, I still think it would be better if the driver was also able to
> > prevent OFF mode explicitely. Defining the max-wakeup-lat with a magic
> > number sounds a bit prone to breaking up.
> 
> I disagree.  What is important is that the driver communicates *why*
> it needs to prevent OFF mode (can't handle the latency etc.)  and
> decision making up to the PM core.  The drivers should not embed
> policy in them.
> 
> > But perhaps, as you said, when drivers work properly they don't have to
> > care about OFF mode as such, but only about the wakeup latency, and thus
> > the max-wakeup-lat is enough. I'm just not quite sure about that, as OFF
> > mode may have side effects as the module is totally powered off, while
> > with RET the side effects should be minimal.
> >
> > I don't have any concrete example about the side effects, but one
> > particular thing I'm thinking about is DSI PLL. If DSS is in RET, I
> > believe DSI PLL works normally. But if the DSS is reset via OFF mode, I
> > believe DSI PLL is also reset. But I'm not sure if DSI PLL is ever
> > needed while DSS would be off, so this may be theoretical =).
> >
> >  Tomi
> 
> This problem is not unique to DSS, and the other drivers are handling this.

So, how can that DSI PLL example be done? An example use case for the
above could be a DSI peripheral that requires continuous DSI clock
(generated by DSI PLL). OFF mode would kill that clock, while RET would
not. Here we are not interested in latencies, but only in that the DSS
block is not powered off.

Well, I have to say that this example may be a bit far fetched, I'm not
100% sure how the HW works. It may be that it's enough to keep the DSI
power on to keep the DSI clock going. Or not. But the point was that
perhaps there are situations where OFF mode has side effects, while RET
doesn't. And in these cases the driver wants to disable OFF mode, but
doesn't care about the latencies.

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

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

> On Wed, 2009-09-30 at 20:31 +0200, ext Kevin Hilman wrote:
>> On Fri, Sep 25, 2009 at 2:19 AM, Tomi Valkeinen
>> <tomi.valkeinen@nokia.com> wrote:
>> > On Thu, 2009-09-24 at 17:52 +0200, ext Kevin Hilman wrote:
>> >> Tomi Valkeinen wrote:
>> >
>> >> > If it is not like that, and the driver initialization is included, how
>> >> > does the PM layer know how long it takes for the DSS driver to
>> >> > reconfigure the DSS hardware from OFF mode?
>> >>
>> >> Currently it doesn't, but if you were measure it, we can use those
>> >> numbers in the decision making process.
>> >
>> > Ok, now I see. However, I'm not sure if that will work. The problem is
>> > that the wakeup latency depends on many things. When using DPI/RFBI the
>> > wakeup is very fast. With SDI it's probably a bit slower and with DSI
>> > even slower.
>> 
>> The varying latencies are not an issues.  When in the different modes,
>> just register a different latency requirement.
>
> How does that help? The problem is not that there are different max
> latency requirements in different modes, but that the actual wake-up
> latency varies.
>
> I found some latency values in resource34xx.h. If they are what I
> presume they are, they define that waking DSS from OFF takes 70us. Let's
> presume it's correct for DPI, so reconfiguring DSS for DPI use takes
> ~70us.
>
> But reconfiguring DSS for DSI use may take, say, 5000us. So if I set max
> latency req to 100us, OFF mode will be "enabled", and it will work fine
> for DPI. But for DSI we will get lantencies around 5000us.
>
> So is there an API to change that value in resource34xx.h dynamically,
> depending on what DSS block is in use? Or am I still missing something
> here? =)

You're right, we currently do not have a way to dynamically update
this table and we should for completeness.

>> 
>> > And at least with DSI PLL, the wakeup time depends on the frequencies
>> > used (according to TRM), and in some cases it can be optimized, in some
>> > cases not. So I don't think there's one single value that fits all.
>> 
>> A single value isn't necessary.
>> 
>> > Also, I still think it would be better if the driver was also able to
>> > prevent OFF mode explicitely. Defining the max-wakeup-lat with a magic
>> > number sounds a bit prone to breaking up.
>> 
>> I disagree.  What is important is that the driver communicates *why*
>> it needs to prevent OFF mode (can't handle the latency etc.)  and
>> decision making up to the PM core.  The drivers should not embed
>> policy in them.
>> 
>> > But perhaps, as you said, when drivers work properly they don't have to
>> > care about OFF mode as such, but only about the wakeup latency, and thus
>> > the max-wakeup-lat is enough. I'm just not quite sure about that, as OFF
>> > mode may have side effects as the module is totally powered off, while
>> > with RET the side effects should be minimal.
>> >
>> > I don't have any concrete example about the side effects, but one
>> > particular thing I'm thinking about is DSI PLL. If DSS is in RET, I
>> > believe DSI PLL works normally. But if the DSS is reset via OFF mode, I
>> > believe DSI PLL is also reset. But I'm not sure if DSI PLL is ever
>> > needed while DSS would be off, so this may be theoretical =).
>> >
>> >  Tomi
>> 
>> This problem is not unique to DSS, and the other drivers are handling this.
>
> So, how can that DSI PLL example be done? An example use case for the
> above could be a DSI peripheral that requires continuous DSI clock
> (generated by DSI PLL). OFF mode would kill that clock, while RET would
> not. Here we are not interested in latencies, but only in that the DSS
> block is not powered off.

Excuse my ignorance of the DSS/DSI/etc., but if a DSI periperal is use
that requires a continual DSI clock then shouldn't the driver always
keep the DSI clock enabled (iow, it should never call clk_disable()).
If a clock is left enabled, even if OFF-mode is targetted for that
powerdomain, it will not reach OFF because the clockdomain is active.

> Well, I have to say that this example may be a bit far fetched, I'm
> not 100% sure how the HW works. It may be that it's enough to keep
> the DSI power on to keep the DSI clock going. Or not. But the point
> was that perhaps there are situations where OFF mode has side
> effects, while RET doesn't. And in these cases the driver wants to
> disable OFF mode, but doesn't care about the latencies.

I think those specific cases should be explored rather than just
trying to disable OFF mode at a high level.  As I said before, for
good PM, we really want to have a good understanding of all the
reasons for preventing off-mode, with well documented constraints.

In the special cases or special modes that might have OFF-mode side
effects where latency is not the cocern, then those should be handled
by the driver by leaving the (sub)modules active/on or by leaving
clocks enabled for those modes.

So, getting back to the problem of how to prevent DSS OFF because of
latency, the right API to use is

   omap_pm_set_max_dev_wakeup_lat()

The latency passed in is compared to the device-specific latencies in
the table you found and that device's powerdomain state is set
accordingly.

BUT, this currently doesn't work. :(

The problem is that while the omap_device infrastructure is now in
mainline, this API will not work until an omap_device is implemented
for the DSS module.

After I finish pushing a new PM branch based on 2.6.32, I can have a
look at the current DSS driver and make a proposal/patch for how to
add an omap_device.  

In the DSS git, the master branch seems to be based at 2.6.31-rc5.  Do
you have an updated version against 2.6.32-rc1 or omap/master?

Kevin

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Tomi Valkeinen]

On Thu, 2009-10-01 at 18:19 +0200, ext Kevin Hilman wrote:
> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:

> > So is there an API to change that value in resource34xx.h dynamically,
> > depending on what DSS block is in use? Or am I still missing something
> > here? =)
> 
> You're right, we currently do not have a way to dynamically update
> this table and we should for completeness.

Ok. This was what I was after in the first place, but didn't know how to
form the question properly =).

> Excuse my ignorance of the DSS/DSI/etc., but if a DSI periperal is use
> that requires a continual DSI clock then shouldn't the driver always
> keep the DSI clock enabled (iow, it should never call clk_disable()).
> If a clock is left enabled, even if OFF-mode is targetted for that
> powerdomain, it will not reach OFF because the clockdomain is active.

Well, this is quite theoretical, and I agree that let's just forget
about this and worry it if such a case ever emerges. But here's what I
was thinking:

OMAP's DSI block can be clocked by OMAP's normal clocks (DPLL4, if I
recall right), which are handled by clk_enable/disable. But the clock
signal that goes over DSI bus to the peripheral is generated by the DSI
PLL, which is a DSS internal PLL not handled by clk_enable/disable. DSI
PLL usually gets its reference clock from sysclock.

For example, we have these displays that have their own framebuffer, and
they independently update the actual panel from their internal
framebuffer, while OMAP can be totally sleeping. Currently these
displays generate their own clock for this internal updating.

There's an option in the OMAP DSI block to set the DSI clock output to
always on (versus DSI clock output enabled automatically when there's
data to transfer over DSI bus). And the reasoning for this option is
that some peripherals may require continuous clock to operate. So we
could have a display that uses DSI clock for staying alive, doing some
internal work like refreshing the panel.

> In the DSS git, the master branch seems to be based at 2.6.31-rc5.  Do
> you have an updated version against 2.6.32-rc1 or omap/master?

Which tree are you referring to? My gitorious tree contains the latest
version that I've been pushing to mainstream.

git://gitorious.org/linux-omap-dss2/linux.git (master branch)

It's based on the mainstream tree, but I think it should apply to l-o
easily. I can also make l-o based branch if the conflicts are bad.

 Tomi

Re: [PATCH] video: omap2: dss: RET on idle, enable/disable dss clocks only when needed.

[Kevin Hilman]

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

> On Thu, 2009-10-01 at 18:19 +0200, ext Kevin Hilman wrote:
>> Tomi Valkeinen <tomi.valkeinen@nokia.com> writes:
>
>> > So is there an API to change that value in resource34xx.h dynamically,
>> > depending on what DSS block is in use? Or am I still missing something
>> > here? =)
>> 
>> You're right, we currently do not have a way to dynamically update
>> this table and we should for completeness.
>
> Ok. This was what I was after in the first place, but didn't know how to
> form the question properly =).
>
>> Excuse my ignorance of the DSS/DSI/etc., but if a DSI periperal is use
>> that requires a continual DSI clock then shouldn't the driver always
>> keep the DSI clock enabled (iow, it should never call clk_disable()).
>> If a clock is left enabled, even if OFF-mode is targetted for that
>> powerdomain, it will not reach OFF because the clockdomain is active.
>
> Well, this is quite theoretical, and I agree that let's just forget
> about this and worry it if such a case ever emerges. But here's what I
> was thinking:
>
> OMAP's DSI block can be clocked by OMAP's normal clocks (DPLL4, if I
> recall right), which are handled by clk_enable/disable. But the clock
> signal that goes over DSI bus to the peripheral is generated by the DSI
> PLL, which is a DSS internal PLL not handled by clk_enable/disable. DSI
> PLL usually gets its reference clock from sysclock.
>
> For example, we have these displays that have their own framebuffer, and
> they independently update the actual panel from their internal
> framebuffer, while OMAP can be totally sleeping. Currently these
> displays generate their own clock for this internal updating.
>
> There's an option in the OMAP DSI block to set the DSI clock output to
> always on (versus DSI clock output enabled automatically when there's
> data to transfer over DSI bus). And the reasoning for this option is
> that some peripherals may require continuous clock to operate. So we
> could have a display that uses DSI clock for staying alive, doing some
> internal work like refreshing the panel.

OK, I understand the need to keep these clocks going in some cases.

Based on how some of the other HW modules work, I would assume that if
DSI is active (generating clocks), it would never allow and idle
transition so it wouldn't hit off mode.  I guess we'd need to dig into
the DSS/DSI docs to see if that's true.

But the basic model for going into RET or OFF is that the PRCM uses an
idle protocol to ask each module if it can idle.  Here's a simplified
summary: First, PRCM sends and idle request.  If the module is idle,
it responsds with an idle ack and the PRCM is then free to disable its
clocks etc.  But, if the module doesn't ack, clocks will not be cut and
the module will stay active and prevent full-chip RET/OFF.

So, following this model, I assume that the DSS would never idle ack if
one of its sub-modules, in this case DSI, is active.  That would need
to be verified in the docs.

>> In the DSS git, the master branch seems to be based at 2.6.31-rc5.  Do
>> you have an updated version against 2.6.32-rc1 or omap/master?
>
> Which tree are you referring to? My gitorious tree contains the latest
> version that I've been pushing to mainstream.
>
> git://gitorious.org/linux-omap-dss2/linux.git (master branch)
>
> It's based on the mainstream tree, but I think it should apply to l-o
> easily. I can also make l-o based branch if the conflicts are bad.

Hmm, the git tree I was looking at was from bat.org.  I'll update to
this one and have a look.

Thanks,

Kevin