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