On 04/02/2019 17:42, Tony Lindgren wrote: > Hi, > > * Tomi Valkeinen <tomi.valkeinen@xxxxxx> [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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
