[PATCHv2] drm/omap: dsi: Fix PM for display blank with paired dss_pll calls
diff mbox series

Message ID 20190207154516.57549-1-tony@atomide.com
State New
Headers show
Series
  • [PATCHv2] drm/omap: dsi: Fix PM for display blank with paired dss_pll calls
Related show

Commit Message

Tony Lindgren Feb. 7, 2019, 3:45 p.m. UTC
Currently dsi_display_init_dsi() calls dss_pll_enable() but it is not
paired with dss_pll_disable() in dsi_display_uninit_dsi(). This leaves
the DSS clocks enabled when the display is blanked wasting about extra
5mW of power while idle.

The clock that is left on by not calling dss_pll_disable() is
DSS_CLKCTRL bit 10 OPTFCLKEN_SYS_CLK that is the source clock for
DSI PLL.

We can fix this issue by by making the current dsi_pll_uninit() into
dsi_pll_disable(). This way we can just call dss_pll_disable() from
dsi_display_uninit_dsi() and the code becomes a bit easier to follow.

However, we need to also consider that DSI PLL can be muxed for DVI too
as pointed out by Tomi Valkeinen <tomi.valkeinen@ti.com>. In the DVI
case, we want to unconditionally disable the clocks. To get around this
issue, we separate out the DSI lane handling from dsi_pll_enable() and
dsi_pll_disable() as suggested by Tomi in an earlier experimental patch.

So we must only toggle the DSI regulator based on the vdds_dsi_enabled
flag from dsi_display_init_dsi() and dsi_display_uninit_dsi().

We need to make these two changes together to avoid breaking things
for DVI when fixing the DSI clock handling. And this all causes a
slight renumbering of the error path for dsi_display_init_dsi().

Suggested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes since v1:
- Updated with Tomi's suggested changes to not break DVI with
  regulator_enable() made conditional for dsi_display_init_dsi()

- Updated comments to better describe what all goes wrong

---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 60 ++++++++++++++++---------------
 1 file changed, 31 insertions(+), 29 deletions(-)

Comments

Tomi Valkeinen Feb. 8, 2019, 9:11 a.m. UTC | #1
On 07/02/2019 17:45, Tony Lindgren wrote:
> Currently dsi_display_init_dsi() calls dss_pll_enable() but it is not
> paired with dss_pll_disable() in dsi_display_uninit_dsi(). This leaves
> the DSS clocks enabled when the display is blanked wasting about extra
> 5mW of power while idle.
> 
> The clock that is left on by not calling dss_pll_disable() is
> DSS_CLKCTRL bit 10 OPTFCLKEN_SYS_CLK that is the source clock for
> DSI PLL.
> 
> We can fix this issue by by making the current dsi_pll_uninit() into
> dsi_pll_disable(). This way we can just call dss_pll_disable() from
> dsi_display_uninit_dsi() and the code becomes a bit easier to follow.
> 
> However, we need to also consider that DSI PLL can be muxed for DVI too
> as pointed out by Tomi Valkeinen <tomi.valkeinen@ti.com>. In the DVI
> case, we want to unconditionally disable the clocks. To get around this
> issue, we separate out the DSI lane handling from dsi_pll_enable() and
> dsi_pll_disable() as suggested by Tomi in an earlier experimental patch.
> 
> So we must only toggle the DSI regulator based on the vdds_dsi_enabled
> flag from dsi_display_init_dsi() and dsi_display_uninit_dsi().
> 
> We need to make these two changes together to avoid breaking things
> for DVI when fixing the DSI clock handling. And this all causes a
> slight renumbering of the error path for dsi_display_init_dsi().
> 
> Suggested-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> 
> Changes since v1:
> - Updated with Tomi's suggested changes to not break DVI with
>   regulator_enable() made conditional for dsi_display_init_dsi()
> 
> - Updated comments to better describe what all goes wrong
> 
> ---
>  drivers/gpu/drm/omapdrm/dss/dsi.c | 60 ++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 29 deletions(-)

Looks fine to me and works on panda. I'll queue this to the next merge
window (I presume no rush to get this into the current -rcs, it's a bit
late).

 Tomi
Tony Lindgren Feb. 8, 2019, 3:23 p.m. UTC | #2
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190208 09:11]:
> Looks fine to me and works on panda. I'll queue this to the next merge
> window (I presume no rush to get this into the current -rcs, it's a bit
> late).

OK good to hear panda works too, my panda is in my rack..
This one has been broken for a long time and nobody noticed,
so next is just fine with me.

Regards,

Tony

Patch
diff mbox series

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -1342,12 +1342,9 @@  static int dsi_pll_enable(struct dss_pll *pll)
 	 */
 	dsi_enable_scp_clk(dsi);
 
-	if (!dsi->vdds_dsi_enabled) {
-		r = regulator_enable(dsi->vdds_dsi_reg);
-		if (r)
-			goto err0;
-		dsi->vdds_dsi_enabled = true;
-	}
+	r = regulator_enable(dsi->vdds_dsi_reg);
+	if (r)
+		goto err0;
 
 	/* XXX PLL does not come out of reset without this... */
 	dispc_pck_free_enable(dsi->dss->dispc, 1);
@@ -1372,36 +1369,25 @@  static int dsi_pll_enable(struct dss_pll *pll)
 
 	return 0;
 err1:
-	if (dsi->vdds_dsi_enabled) {
-		regulator_disable(dsi->vdds_dsi_reg);
-		dsi->vdds_dsi_enabled = false;
-	}
+	regulator_disable(dsi->vdds_dsi_reg);
 err0:
 	dsi_disable_scp_clk(dsi);
 	dsi_runtime_put(dsi);
 	return r;
 }
 
-static void dsi_pll_uninit(struct dsi_data *dsi, bool disconnect_lanes)
+static void dsi_pll_disable(struct dss_pll *pll)
 {
+	struct dsi_data *dsi = container_of(pll, struct dsi_data, pll);
+
 	dsi_pll_power(dsi, DSI_PLL_POWER_OFF);
-	if (disconnect_lanes) {
-		WARN_ON(!dsi->vdds_dsi_enabled);
-		regulator_disable(dsi->vdds_dsi_reg);
-		dsi->vdds_dsi_enabled = false;
-	}
+
+	regulator_disable(dsi->vdds_dsi_reg);
 
 	dsi_disable_scp_clk(dsi);
 	dsi_runtime_put(dsi);
 
-	DSSDBG("PLL uninit done\n");
-}
-
-static void dsi_pll_disable(struct dss_pll *pll)
-{
-	struct dsi_data *dsi = container_of(pll, struct dsi_data, pll);
-
-	dsi_pll_uninit(dsi, true);
+	DSSDBG("PLL disable done\n");
 }
 
 static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
@@ -4096,11 +4082,11 @@  static int dsi_display_init_dsi(struct dsi_data *dsi)
 
 	r = dss_pll_enable(&dsi->pll);
 	if (r)
-		goto err0;
+		return r;
 
 	r = dsi_configure_dsi_clocks(dsi);
 	if (r)
-		goto err1;
+		goto err0;
 
 	dss_select_dsi_clk_source(dsi->dss, dsi->module_id,
 				  dsi->module_id == 0 ?
@@ -4108,6 +4094,14 @@  static int dsi_display_init_dsi(struct dsi_data *dsi)
 
 	DSSDBG("PLL OK\n");
 
+	if (!dsi->vdds_dsi_enabled) {
+		r = regulator_enable(dsi->vdds_dsi_reg);
+		if (r)
+			goto err1;
+
+		dsi->vdds_dsi_enabled = true;
+	}
+
 	r = dsi_cio_init(dsi);
 	if (r)
 		goto err2;
@@ -4136,10 +4130,13 @@  static int dsi_display_init_dsi(struct dsi_data *dsi)
 err3:
 	dsi_cio_uninit(dsi);
 err2:
-	dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
+	regulator_disable(dsi->vdds_dsi_reg);
+	dsi->vdds_dsi_enabled = false;
 err1:
-	dss_pll_disable(&dsi->pll);
+	dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
 err0:
+	dss_pll_disable(&dsi->pll);
+
 	return r;
 }
 
@@ -4158,7 +4155,12 @@  static void dsi_display_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
 
 	dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
 	dsi_cio_uninit(dsi);
-	dsi_pll_uninit(dsi, disconnect_lanes);
+	dss_pll_disable(&dsi->pll);
+
+	if (disconnect_lanes) {
+		regulator_disable(dsi->vdds_dsi_reg);
+		dsi->vdds_dsi_enabled = false;
+	}
 }
 
 static int dsi_display_enable(struct omap_dss_device *dssdev)