| Message ID | 20190131033210.21449-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: dsi: Fix PM for display blank with paired dss_pll calls | expand |
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
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
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
* 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
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)
* 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
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
* 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
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
* 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
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)
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(-)