[Intel-gfx] [PATCH] drm/i915: Fix DDI PHY init if it was already on

[Imre Deak]

On Mon, Oct 02, 2017 at 01:09:57PM -0700, Rodrigo Vivi wrote:
> On Mon, Oct 02, 2017 at 01:53:07PM +0000, Imre Deak wrote:
> > The common lane power down flag of a DPIO PHY has a funky semantic:
> > after the initial enabling of the PHY (so from a disabled state) this
> > flag will be clear. It will be set only after the PHY will be used for
> > the first time (for instance due to enabling the corresponding pipe) and
> > then become unused (due to disabling the pipe). During the initial PHY
> > enablement we don't know which of the above phases we are in, so move
> > the check for the flag where this is known, the HW readout code. This is
> > where the rest of lane power down status checks are done anyway.
> 
> This makes sense. I just wonder why at first place we were doing that
> extra check to see if that was disabled.
> I couldn't find anything on spec to justify the previous use of it and
> new one is consistent with bit 9 that is very similar, so:

Originally it wasn't clear when - or if at all - the flag gets set to
indicate that the common lane is powered down, this isn't explained in
Bspec or anywhere else either. It's just after testing things now more
that it became clear how it behaves as I wrote above. The original check
was based on the fact that after the initial enabling the flag is clear
indicating the common lane is powered, although it isn't used at that
moment (which is rather illogical). This check just didn't take into
account the cases where the PHY is already on during driver loading and
the corresponding pipe went through an enable->disable cycle (like
during driver reloading).

> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

Thanks.

> > 
> > This fixes at least a problem on GLK where after module reloading, the
> > common lane power down flag of PHY1 is set, but the PHY is actually
> > powered-on and properly set up. The GRC readout code for other PHYs will
> > hence think that PHY1 is not powered initially and disable it after the
> > GRC readout. This will cause the AUX power well related to PHY1 to get
> > disabled in a stuck state, timing out when we try to enable it later.
> > 
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Fixes: e93da0a0137b ("drm/i915/bxt: Sanitiy check the PHY lane power down status")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102777
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c      |  3 ++-
> >  drivers/gpu/drm/i915/intel_dpio_phy.c | 20 --------------------
> >  2 files changed, 2 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 93cbbcbbc193..65f4b6786791 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1713,7 +1713,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
> >  out:
> >  	if (ret && IS_GEN9_LP(dev_priv)) {
> >  		tmp = I915_READ(BXT_PHY_CTL(port));
> > -		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> > +		if ((tmp & (BXT_PHY_CMNLANE_POWERDOWN_ACK |
> > +			    BXT_PHY_LANE_POWERDOWN_ACK |
> >  			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED)
> >  			DRM_ERROR("Port %c enabled but PHY powered down? "
> >  				  "(PHY_CTL %08x)\n", port_name(port), tmp);
> > diff --git a/drivers/gpu/drm/i915/intel_dpio_phy.c b/drivers/gpu/drm/i915/intel_dpio_phy.c
> > index 09b670929786..de38d014ed39 100644
> > --- a/drivers/gpu/drm/i915/intel_dpio_phy.c
> > +++ b/drivers/gpu/drm/i915/intel_dpio_phy.c
> > @@ -208,12 +208,6 @@ static const struct bxt_ddi_phy_info glk_ddi_phy_info[] = {
> >  	},
> >  };
> >  
> > -static u32 bxt_phy_port_mask(const struct bxt_ddi_phy_info *phy_info)
> > -{
> > -	return (phy_info->dual_channel * BIT(phy_info->channel[DPIO_CH1].port)) |
> > -		BIT(phy_info->channel[DPIO_CH0].port);
> > -}
> > -
> >  static const struct bxt_ddi_phy_info *
> >  bxt_get_phy_list(struct drm_i915_private *dev_priv, int *count)
> >  {
> > @@ -313,7 +307,6 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> >  			    enum dpio_phy phy)
> >  {
> >  	const struct bxt_ddi_phy_info *phy_info;
> > -	enum port port;
> >  
> >  	phy_info = bxt_get_phy_info(dev_priv, phy);
> >  
> > @@ -335,19 +328,6 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
> >  		return false;
> >  	}
> >  
> > -	for_each_port_masked(port, bxt_phy_port_mask(phy_info)) {
> > -		u32 tmp = I915_READ(BXT_PHY_CTL(port));
> > -
> > -		if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) {
> > -			DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane "
> > -					 "for port %c powered down "
> > -					 "(PHY_CTL %08x)\n",
> > -					 phy, port_name(port), tmp);
> > -
> > -			return false;
> > -		}
> > -	}
> > -
> >  	return true;
> >  }
> >  
> > -- 
> > 2.13.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx