Re: [PATCH 6/6] drm/i915/bxt: Sanitiy check the PHY lane power down status

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote:
> We can check the power state of the PHY data and common lanes as
> reported by the PHY. Do this in case we need to debug problems where the
> PHY gets stuck in an unexpected state.
> 
> Note that I only check these when the lanes are expected to be powered
> on purpose, since it's not clear at what point the PHY power/clock gates
> things.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  9 +++++++++
>  drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 81fc498..e904818 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells {
>  #define BXT_P_CR_GT_DISP_PWRON		_MMIO(0x138090)
>  #define   GT_DISPLAY_POWER_ON(phy)	(1 << (phy))
>  
> +#define _BXT_PHY_CTL_DDI_A		0x64C00
> +#define _BXT_PHY_CTL_DDI_B		0x64C10
> +#define _BXT_PHY_CTL_DDI_C		0x64C20
> +#define   BXT_PHY_CMNLANE_POWERDOWN_ACK	(1 << 10)
> +#define   BXT_PHY_LANE_POWERDOWN_ACK	(1 << 9)
> +#define   BXT_PHY_LANE_ENABLED		(1 << 8)
> +#define BXT_PHY_CTL(port)		_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
> +							 _BXT_PHY_CTL_DDI_B)
> +
>  #define _PHY_CTL_FAMILY_EDP		0x64C80
>  #define _PHY_CTL_FAMILY_DDI		0x64C90
>  #define   COMMON_RESET_DIS		(1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index acf0a7a..afa28a1 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  	DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port));
>  
>  out:
> +	if (ret && IS_BROXTON(dev_priv)) {
> +		tmp = I915_READ(BXT_PHY_CTL(port));
> +		if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK |
> +			    BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) {
> +			DRM_DEBUG_KMS("Port %c enabled but PHY powered down? "
> +				      "(PHY_CTL %08x)\n", port_name(port), tmp);
> +			ret = false;

Hmm. I'm not sure I'd change the return value here because then we'd
fail to follow the proper shutdown sequence, perhaps even messing
up the next modeset?

So maybe just ERROR/WARN in these cases?

> +		}
> +	}
> +
>  	intel_display_power_put(dev_priv, power_domain);
>  
>  	return ret;
> @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  			    enum dpio_phy phy)
>  {
> +	enum port port;
> +
>  	if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy)))
>  		return false;
>  
> @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv,
>  		return false;
>  	}
>  
> +	for_each_port_masked(port,
> +			     phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) :
> +					        BIT(PORT_A)) {
> +		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.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx