Re: [PATCH] drm/omap: dsi: Fix PM for display blank with paired dss_pll calls

[Tony Lindgren <tony@atomide.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel