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

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

Commit Message

Tony Lindgren Jan. 31, 2019, 3:32 a.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.

Let's fix this by setting a dsi->disconnect_lanes flag and 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.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Tomi Valkeinen Feb. 4, 2019, 9:57 a.m. UTC | #1
Hi Tony,

On 31/01/2019 05:32, 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

But it is paired with dsi_pll_uninit().

> the DSS clocks enabled when the display is blanked wasting about extra
> 5mW of power while idle.

Which clocks? I think all the clocks are disabled. But if
disconnect_lanes == false, the regulator is left enabled.

If some clocks are left enabled, then that's a bug, but this didn't seem
to be the case after a brief review of the code.

> Let's fix this by setting a dsi->disconnect_lanes flag and 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.

It's been a long time since I worked on these DSI features, but:
- If the regulator is disabled, the DSI lanes are in undefined state
- If the DSI lanes are in undefined state, the panel often sees that as
errors (or, in theory, might even read it as some real event) in the DSI
lanes
- If the panel driver can handle lanes in undefined state, it can set
disconnect_lanes to true
- ULPS needs the lanes to be in defined state (high/low, I can't recall)

I can't remember the details, but I think there was a reason why the
panel-dsi-cm.c doesn't disconnect the lanes... I also faintly remember
that in Nokia we had some hacky code to change the pinmux to keep the
pins in defined states even if the regulator got disabled. But that was
never upstreamed.

 Tomi
Tony Lindgren Feb. 4, 2019, 3:42 p.m. UTC | #2
Hi,

* Tomi Valkeinen <tomi.valkeinen@ti.com> [190204 09:57]:
> On 31/01/2019 05:32, 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
> 
> But it is paired with dsi_pll_uninit().

But we need to also call dss_pll_disable(). Now we're only calling
dsi_pll_disable() and skipping dss_pll_disable().

> > the DSS clocks enabled when the display is blanked wasting about extra
> > 5mW of power while idle.
> 
> Which clocks? I think all the clocks are disabled. But if
> disconnect_lanes == false, the regulator is left enabled.

Without this patch I'm seeing DSS_CLKCTRL bit 10 OPTFCLKEN_SYS_CLK
left enabled after blanking, also known as sys_clk in the dts.

> If some clocks are left enabled, then that's a bug, but this didn't seem
> to be the case after a brief review of the code.

Yeah the code there currently is a bit confusing to follow.
With the missing call to dss_pll_disable(), we never call
clk_disable_unprepare(pll->clkin).

> > Let's fix this by setting a dsi->disconnect_lanes flag and 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.
> 
> It's been a long time since I worked on these DSI features, but:
> - If the regulator is disabled, the DSI lanes are in undefined state
> - If the DSI lanes are in undefined state, the panel often sees that as
> errors (or, in theory, might even read it as some real event) in the DSI
> lanes
> - If the panel driver can handle lanes in undefined state, it can set
> disconnect_lanes to true
> - ULPS needs the lanes to be in defined state (high/low, I can't recall)

Hmm OK. So after this patch we now have the following at dss and
dsi levels:

- dsi_pll_disable() calls regulator_disable(dsi->vdds_dsi_reg) if
  dsi->disconnect_lanes is set

- dss_pll_disable() calls regulator_disable(pll->regulator)
  unconditionally

At least I'm not seeing any issues with dss_pll_disable()
call regulator_disable(pll->regulator) even without having
dsi->disconnect_lanes set.

Sebastian do you think this might be an issue on n950 for a
blank/unblank cycle?

N950 does have vdda_video-supply = <&vdac> configured which
should be the pll->regulator I think.

My guess is that the dsi lanes are fine with just the
dsi->vdds_dsi_reg and do not need the pll->regulator :)

This guess is based on the fact that the DSS components
should be mostly independent modules on the DSS internal
interconnect.

> I can't remember the details, but I think there was a reason why the
> panel-dsi-cm.c doesn't disconnect the lanes... I also faintly remember
> that in Nokia we had some hacky code to change the pinmux to keep the
> pins in defined states even if the regulator got disabled. But that was
> never upstreamed.

OK good to know. That can be done with pinctrl named states now.

Regards,

Tony
Tomi Valkeinen Feb. 5, 2019, 11:06 a.m. UTC | #3
On 04/02/2019 17:42, Tony Lindgren wrote:
> Hi,
> 
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [190204 09:57]:
>> On 31/01/2019 05:32, 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
>>
>> But it is paired with dsi_pll_uninit().
> 
> But we need to also call dss_pll_disable(). Now we're only calling
> dsi_pll_disable() and skipping dss_pll_disable().

Ok, I see now.

>>> the DSS clocks enabled when the display is blanked wasting about extra
>>> 5mW of power while idle.
>>
>> Which clocks? I think all the clocks are disabled. But if
>> disconnect_lanes == false, the regulator is left enabled.
> 
> Without this patch I'm seeing DSS_CLKCTRL bit 10 OPTFCLKEN_SYS_CLK
> left enabled after blanking, also known as sys_clk in the dts.

Ok. That's the source clock for DSI PLL.

>> If some clocks are left enabled, then that's a bug, but this didn't seem
>> to be the case after a brief review of the code.
> 
> Yeah the code there currently is a bit confusing to follow.

Yep... So there's the DSI internal code which needs to deal with ulps
and disconnect_lanes, and then the external interface to the DSI PLL (so
that DPI can use DSI PLL) without ulps/disconnect.

I think your patch breaks this latter one, as disconnect_lanes is zero
in that case and would leave the regulator enabled. This would probably
be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
output, if I recall right.

And storing the 'disconnect_lanes' is a bit ugly, but I don't see right
away how to avoid it...

Maybe the field in dsi_data should be something like
"keep_lanes_powered", and default value false. In
dsi_display_uninit_dsi(), we could set

keep_lanes_powered = !disconnect_lanes;

and after dss_pll_disable() call, set keep_lanes_powered back to false
to keep it in the default state.

 Tomi
Tony Lindgren Feb. 5, 2019, 5:58 p.m. UTC | #4
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190205 11:07]:
> Yep... So there's the DSI internal code which needs to deal with ulps
> and disconnect_lanes, and then the external interface to the DSI PLL (so
> that DPI can use DSI PLL) without ulps/disconnect.
> 
> I think your patch breaks this latter one, as disconnect_lanes is zero
> in that case and would leave the regulator enabled. This would probably
> be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
> output, if I recall right.

Sorry I don't quite follow what happens there with dvi
calling into dsi.. Care to describe a bit more?

> And storing the 'disconnect_lanes' is a bit ugly, but I don't see right
> away how to avoid it...
> 
> Maybe the field in dsi_data should be something like
> "keep_lanes_powered", and default value false. In
> dsi_display_uninit_dsi(), we could set
> 
> keep_lanes_powered = !disconnect_lanes;
> 
> and after dss_pll_disable() call, set keep_lanes_powered back to false
> to keep it in the default state.

If we need more control over the lanes, maybe we should just
add a helper like this so that only panels that need it can
call it:

static int dsi_set_lane_power(struct omap_dss_device *dssdev,
			      bool enable)
{
        struct dsi_data *dsi = to_dsi_data(dssdev);

        dsi->keep_lanes_powered = enable;

        return 0;
}

Regards,

Tony
Tomi Valkeinen Feb. 6, 2019, 9:13 a.m. UTC | #5
On 05/02/2019 19:58, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [190205 11:07]:
>> Yep... So there's the DSI internal code which needs to deal with ulps
>> and disconnect_lanes, and then the external interface to the DSI PLL (so
>> that DPI can use DSI PLL) without ulps/disconnect.
>>
>> I think your patch breaks this latter one, as disconnect_lanes is zero
>> in that case and would leave the regulator enabled. This would probably
>> be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
>> output, if I recall right.
> 
> Sorry I don't quite follow what happens there with dvi
> calling into dsi.. Care to describe a bit more?

We have the DSI PLL, which is "owned" by the DSI driver. Its main purpose is to clock the DSI. However, the output clock from the DSI PLL can also be muxed to go to the DPI (parallel video output, used for DVI on Panda). So for this use, the DPI driver enables, disables and configures the DSI PLL.

When using DSI PLL from DPI, we don't care about this "disconnect_lanes", we always want to fully disable everything. But when using DSI PLL from DSI, we sometimes want to keep the lanes enabled using disconnect_lanes. So the functions these two users use are a bit different.

That said, and looking at the code and trying to remember what all this is about... I think the disconnect_lanes is misplaced, and not related to the DSI PLL. The DSI code has degraded during the years quite a bit...

I think the code should always enable the regulator in pll_enable, and always disable it in pll_disable. The disconnect_lanes should be handled separately from the PLL code, as it's not really related.

Here's a quick edit about what I'm thing about, not tested:

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 00a9c2ab9e6c..7e122d94ad2b 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -352,7 +352,7 @@ struct dsi_data {
 
 	struct dss_pll pll;
 
-	bool vdds_dsi_enabled;
+	bool vdds_dsi_enabled; // XXX marks if the regulator has been left on for lanes
 	struct regulator *vdds_dsi_reg;
 
 	struct {
@@ -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)
@@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
 
 	DSSDBG("PLL OK\n");
 
+	// XXX enable the regulator for the lanes
+	regulator_enable(dsi->vdds_dsi_reg);
+	dsi->vdds_dsi_enabled = true;
+
 	r = dsi_cio_init(dsi);
 	if (r)
 		goto err2;
@@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
 err3:
 	dsi_cio_uninit(dsi);
 err2:
+	// XXX disable the regulator for the lanes
+	regulator_disable(dsi->vdds_dsi_reg);
+	dsi->vdds_dsi_enabled = false;
+
 	dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
 err1:
 	dss_pll_disable(&dsi->pll);
@@ -4158,7 +4152,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)
Tony Lindgren Feb. 6, 2019, 4 p.m. UTC | #6
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190206 09:13]:
> On 05/02/2019 19:58, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [190205 11:07]:
> >> Yep... So there's the DSI internal code which needs to deal with ulps
> >> and disconnect_lanes, and then the external interface to the DSI PLL (so
> >> that DPI can use DSI PLL) without ulps/disconnect.
> >>
> >> I think your patch breaks this latter one, as disconnect_lanes is zero
> >> in that case and would leave the regulator enabled. This would probably
> >> be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
> >> output, if I recall right.
> > 
> > Sorry I don't quite follow what happens there with dvi
> > calling into dsi.. Care to describe a bit more?
> 
> We have the DSI PLL, which is "owned" by the DSI driver. Its main purpose is to clock the DSI. However, the output clock from the DSI PLL can also be muxed to go to the DPI (parallel video output, used for DVI on Panda). So for this use, the DPI driver enables, disables and configures the DSI PLL.
> 
> When using DSI PLL from DPI, we don't care about this "disconnect_lanes", we always want to fully disable everything. But when using DSI PLL from DSI, we sometimes want to keep the lanes enabled using disconnect_lanes. So the functions these two users use are a bit different.
> 
> That said, and looking at the code and trying to remember what all this is about... I think the disconnect_lanes is misplaced, and not related to the DSI PLL. The DSI code has degraded during the years quite a bit...
> 
> I think the code should always enable the regulator in pll_enable, and always disable it in pll_disable. The disconnect_lanes should be handled separately from the PLL code, as it's not really related.
> 
> Here's a quick edit about what I'm thing about, not tested:

OK I'll give it a try. Based on a quick glance, we need to still
check for enabled regulator to avoid unpaired calls.

>  static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
> @@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
>  
>  	DSSDBG("PLL OK\n");
>  
> +	// XXX enable the regulator for the lanes
> +	regulator_enable(dsi->vdds_dsi_reg);
> +	dsi->vdds_dsi_enabled = true;
> +

So the above should only be done if !dsi->vdds_dsi_enabled?

>  	r = dsi_cio_init(dsi);
>  	if (r)
>  		goto err2;
> @@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
>  err3:
>  	dsi_cio_uninit(dsi);
>  err2:
> +	// XXX disable the regulator for the lanes
> +	regulator_disable(dsi->vdds_dsi_reg);
> +	dsi->vdds_dsi_enabled = false;
> +

And here only if dsi->vdds_dsi_enabled?

> @@ -4158,7 +4152,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;
> +	}
>  }

Since they would be paired with the conditional handling
here?

Regards,

Tony
Tomi Valkeinen Feb. 6, 2019, 4:09 p.m. UTC | #7
On 06/02/2019 18:00, Tony Lindgren wrote:

> OK I'll give it a try. Based on a quick glance, we need to still
> check for enabled regulator to avoid unpaired calls.
> 
>>  static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
>> @@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
>>  
>>  	DSSDBG("PLL OK\n");
>>  
>> +	// XXX enable the regulator for the lanes
>> +	regulator_enable(dsi->vdds_dsi_reg);
>> +	dsi->vdds_dsi_enabled = true;
>> +
> 
> So the above should only be done if !dsi->vdds_dsi_enabled?
> 
>>  	r = dsi_cio_init(dsi);
>>  	if (r)
>>  		goto err2;
>> @@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
>>  err3:
>>  	dsi_cio_uninit(dsi);
>>  err2:
>> +	// XXX disable the regulator for the lanes
>> +	regulator_disable(dsi->vdds_dsi_reg);
>> +	dsi->vdds_dsi_enabled = false;
>> +
> 
> And here only if dsi->vdds_dsi_enabled?
> 
>> @@ -4158,7 +4152,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;
>> +	}
>>  }
> 
> Since they would be paired with the conditional handling
> here?

Hmm, yes, I think you're right. And there's already one in dsi_remove(),
which handles the final disable at unload time.

 Tomi
Tony Lindgren Feb. 6, 2019, 4:29 p.m. UTC | #8
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190206 16:09]:
> On 06/02/2019 18:00, Tony Lindgren wrote:
> 
> > OK I'll give it a try. Based on a quick glance, we need to still
> > check for enabled regulator to avoid unpaired calls.
> > 
> >>  static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
> >> @@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
> >>  
> >>  	DSSDBG("PLL OK\n");
> >>  
> >> +	// XXX enable the regulator for the lanes
> >> +	regulator_enable(dsi->vdds_dsi_reg);
> >> +	dsi->vdds_dsi_enabled = true;
> >> +
> > 
> > So the above should only be done if !dsi->vdds_dsi_enabled?

Actually the conditional handling is only needed above. And
the regulator_enable needs error handling added that causes
some renumbering of the exit path.

> >>  	r = dsi_cio_init(dsi);
> >>  	if (r)
> >>  		goto err2;
> >> @@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
> >>  err3:
> >>  	dsi_cio_uninit(dsi);
> >>  err2:
> >> +	// XXX disable the regulator for the lanes
> >> +	regulator_disable(dsi->vdds_dsi_reg);
> >> +	dsi->vdds_dsi_enabled = false;
> >> +
> > 
> > And here only if dsi->vdds_dsi_enabled?

On errors here we should just shut it down.

> >> @@ -4158,7 +4152,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;
> >> +	}
> >>  }
> > 
> > Since they would be paired with the conditional handling
> > here?
> 
> Hmm, yes, I think you're right. And there's already one in dsi_remove(),
> which handles the final disable at unload time.

OK. Looks good to me otherwise.

So I guess we should fix. Do you want me to post it all
as a single patch after some testing?

It seems that we'll be breaking things one way or another
trying to do it in two patches :)

Regards,

Tony
Tomi Valkeinen Feb. 7, 2019, 9:07 a.m. UTC | #9
On 06/02/2019 18:29, Tony Lindgren wrote:

> OK. Looks good to me otherwise.
> 
> So I guess we should fix. Do you want me to post it all
> as a single patch after some testing?

Yes please =)

 Tomi
Tony Lindgren Feb. 7, 2019, 3:46 p.m. UTC | #10
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190207 09:07]:
> On 06/02/2019 18:29, Tony Lindgren wrote:
> 
> > OK. Looks good to me otherwise.
> > 
> > So I guess we should fix. Do you want me to post it all
> > as a single patch after some testing?
> 
> Yes please =)

OK v2 patch sent.

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
@@ -339,6 +339,7 @@  struct dsi_data {
 	int irq;
 
 	bool is_enabled;
+	unsigned int disconnect_lanes:1;
 
 	struct clk *dss_clk;
 	struct regmap *syscon;
@@ -1382,10 +1383,12 @@  static int dsi_pll_enable(struct dss_pll *pll)
 	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) {
+	if (dsi->disconnect_lanes) {
 		WARN_ON(!dsi->vdds_dsi_enabled);
 		regulator_disable(dsi->vdds_dsi_reg);
 		dsi->vdds_dsi_enabled = false;
@@ -1397,13 +1400,6 @@  static void dsi_pll_uninit(struct dsi_data *dsi, bool disconnect_lanes)
 	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);
-}
-
 static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
 {
 	struct dsi_data *dsi = p;
@@ -4158,7 +4154,9 @@  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);
+
+	dsi->disconnect_lanes = disconnect_lanes;
+	dss_pll_disable(&dsi->pll);
 }
 
 static int dsi_display_enable(struct omap_dss_device *dssdev)