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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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:

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

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