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