Re: [PATCH 2/2] drm/i915: Skip CHV PHY asserts until PHY has been fully reset

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 05, 2015 at 03:57:45PM +0530, Deepak wrote:
> 
> 
> On Tuesday 08 September 2015 08:35 PM, ville.syrjala@linux.intel.com wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The BIOS can leave the CHV display PHY in some odd state where
> >some of the LDOs/lanes won't power down fully when unused. This
> >will trigger a host of asserts that were added in:
> >30142273a3e83936fd7b45aa5339311a9295ca51 drm/i915: Add CHV PHY LDO power sanity checks
> >6669e39f95b5530ca8cb9137703ceb5e83e5d648 drm/i915: Add some CHV DPIO lane power state asserts
> >
> >To avoid that, skip the asserts until the PHY power well has been
> >disabled at least once. That will fully reset the PHY, and once
> >brought back up, the dynamic power down features will work correctly.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h         |  3 +++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 46 ++++++++++++++++++++++++++++++++-
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index 503dff5..cf1880e 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -1956,6 +1956,9 @@ struct drm_i915_private {
> >  	bool edp_low_vswing;
> >+	/* perform PHY state sanity checks? */
> >+	bool chv_phy_assert[2];
> >+
> >  	/*
> >  	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> >  	 * will be rejected. Instead look for a better place.
> >diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >index 3f682a1..a0a2e6d 100644
> >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >@@ -987,8 +987,29 @@ static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
> >  		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
> >  	u32 phy_control = dev_priv->chv_phy_control;
> >  	u32 phy_status = 0;
> >+	u32 phy_status_mask = 0xffffffff;
> >  	u32 tmp;
> >+	/*
> >+	 * The BIOS can leave the PHY is some weird state
> >+	 * where it doesn't fully power down some parts.
> >+	 * Disable the asserts until the PHY has been fully
> >+	 * reset (ie. the power well has been disabled at
> >+	 * least once).
> >+	 */
> >+	if (!dev_priv->chv_phy_assert[DPIO_PHY0])
> >+		phy_status_mask &= ~(PHY_STATUS_CMN_LDO(DPIO_PHY0, DPIO_CH0) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH0, 0) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH0, 1) |
> >+				     PHY_STATUS_CMN_LDO(DPIO_PHY0, DPIO_CH1) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH1, 0) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH1, 1));
> >+
> >+	if (!dev_priv->chv_phy_assert[DPIO_PHY1])
> >+		phy_status_mask &= ~(PHY_STATUS_CMN_LDO(DPIO_PHY1, DPIO_CH0) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY1, DPIO_CH0, 0) |
> >+				     PHY_STATUS_SPLINE_LDO(DPIO_PHY1, DPIO_CH0, 1));
> >+
> >  	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> >  		phy_status |= PHY_POWERGOOD(DPIO_PHY0);
> >@@ -1049,11 +1070,13 @@ static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
> >  			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY1, DPIO_CH0, 1);
> >  	}
> >+	phy_status &= phy_status_mask;
> >+
> >  	/*
> >  	 * The PHY may be busy with some initial calibration and whatnot,
> >  	 * so the power state can take a while to actually change.
> >  	 */
> >-	if (wait_for((tmp = I915_READ(DISPLAY_PHY_STATUS)) == phy_status, 10))
> >+	if (wait_for((tmp = I915_READ(DISPLAY_PHY_STATUS) & phy_status_mask) == phy_status, 10))
> >  		WARN(phy_status != tmp,
> >  		     "Unexpected PHY_STATUS 0x%08x, expected 0x%08x (PHY_CONTROL=0x%08x)\n",
> >  		     tmp, phy_status, dev_priv->chv_phy_control);
> >@@ -1146,6 +1169,9 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >  	DRM_DEBUG_KMS("Disabled DPIO PHY%d (PHY_CONTROL=0x%08x)\n",
> >  		      phy, dev_priv->chv_phy_control);
> >+	/* PHY is fully reset now, so we can enable the PHY state asserts */
> >+	dev_priv->chv_phy_assert[phy] = true;
> >+
> >  	assert_chv_phy_status(dev_priv);
> >  }
> >@@ -1155,6 +1181,16 @@ static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpi
> >  	enum pipe pipe = phy == DPIO_PHY0 ? PIPE_A : PIPE_C;
> >  	u32 reg, val, expected, actual;
> >+	/*
> >+	 * The BIOS can leave the PHY is some weird state
> >+	 * where it doesn't fully power down some parts.
> >+	 * Disable the asserts until the PHY has been fully
> >+	 * reset (ie. the power well has been disabled at
> >+	 * least once).
> >+	 */
> >+	if (!dev_priv->chv_phy_assert[phy])
> >+		return;
> >+
> >  	if (ch == DPIO_CH0)
> >  		reg = _CHV_CMN_DW0_CH0;
> >  	else
> >@@ -1911,6 +1947,10 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >  			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> >  		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> >+
> >+		dev_priv->chv_phy_assert[DPIO_PHY0] = false;
> >+	} else {
> >+		dev_priv->chv_phy_assert[DPIO_PHY0] = true;
> >  	}
> >  	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> >@@ -1929,6 +1969,10 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >  			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> >  		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> >+
> >+		dev_priv->chv_phy_assert[DPIO_PHY1] = false;
> >+	} else {
> >+		dev_priv->chv_phy_assert[DPIO_PHY1] = true;
> >  	}
> >  	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> 
> Change looks fine.
> Reviewed-by: Deepak S<deepak.s@linux.intel.com>

Both patches applied to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx