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(). > > 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
