Re: [PATCH 15/15] drm/i915: Add CHV PHY LDO power sanity checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 27, 2015 at 10:09:37AM +0530, Deepak wrote:
> 
> 
> On 07/09/2015 02:16 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> >From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> >At various points when changing the DPIO lane/phy power states,
> >construct an expected value of the DISPLAY_PHY_STATUS register
> >and compare it with the real thing.
> >
> >To construct the expected value we look at our shadow PHY_CONTROL
> >register value (which should match what we've just written to the
> >hardware), and we also need to look at the actual state of the cmn
> >power wells as a disabled power well causes the relevant LDO status
> >to be reported as 'on' in DISPLAY_PHY_STATUS.
> >
> >When initially powering up the PHY it performs various internal
> >calibrations for which it fully powers up. That means that if we check
> >for the expetected power state immediately upon releasing cmnreset we
> >would get the occasional false positive. But we can of course
> >poll until the expected value appears. It shouldn't be too long so
> >this shouldn't make modesets substantially longer.
> >
> >One extra complication is introduced when we cross the streams, ie.
> >drive port B with pipe B. In this case we trick CL2 (where the DPLL lives)
> >into life by temporaily powering up the lanes in the second channel,
> >and once the pipe is up and runnign we release the lane power override.
> >At that point the power state of CL2 has somehow gotten entangled with
> >the power state of the first channel. That means that constructing the
> >expected DISPLAY_PHY_STATUS value is a bit tricky since based on the
> >lane power states in the second channel, CL2 should also be powered
> >down. But we can use the DPLL enable bit to determine when CL2 should
> >be alive even if the lanes are powered down. However the power state
> >of CL2 isn't actually tied in with the DPLL state, but to the state
> >of the lanes in first channel, so we have to avoid checking the
> >expected state between shutting down the DPLL and powering down
> >the lanes in the first channel. So no calling assert_chv_phy_status()
> >before the DISPLAY_PHY_CONTROL write in chv_phy_powergate_lanes(),
> >but after the write is a safe time to check.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Remaining 3 patches pulled into dinq, thanks.
-Daniel

> >---
> >  drivers/gpu/drm/i915/i915_reg.h         |   2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 126 +++++++++++++++++++++++++++-----
> >  2 files changed, 111 insertions(+), 17 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >index 586a0f7..429105e 100644
> >--- a/drivers/gpu/drm/i915/i915_reg.h
> >+++ b/drivers/gpu/drm/i915/i915_reg.h
> >@@ -2206,6 +2206,8 @@ enum skl_disp_power_wells {
> >  #define   PHY_COM_LANE_RESET_DEASSERT(phy)	(1 << (phy))
> >  #define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
> >  #define   PHY_POWERGOOD(phy)	(((phy) == DPIO_PHY0) ? (1<<31) : (1<<30))
> >+#define   PHY_STATUS_CMN_LDO(phy, ch)                   (1 << (6-(6*(phy)+3*(ch))))
> >+#define   PHY_STATUS_SPLINE_LDO(phy, ch, spline)        (1 << (8-(6*(phy)+3*(ch)+(spline))))
> >  /*
> >   * The i830 generation, in LVDS mode, defines P1 as the bit number set within
> >diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >index a1d9676..e6f1b4ee 100644
> >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >@@ -942,6 +942,107 @@ static void vlv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >  	vlv_set_power_well(dev_priv, power_well, false);
> >  }
> >+#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
> >+
> >+static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,
> >+						 int power_well_id)
> >+{
> >+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >+	struct i915_power_well *power_well;
> >+	int i;
> >+
> >+	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> >+		if (power_well->data == power_well_id)
> >+			return power_well;
> >+	}
> >+
> >+	return NULL;
> >+}
> >+
> >+#define BITS_SET(val, bits) (((val) & (bits)) == (bits))
> >+
> >+static void assert_chv_phy_status(struct drm_i915_private *dev_priv)
> >+{
> >+	struct i915_power_well *cmn_bc =
> >+		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_BC);
> >+	struct i915_power_well *cmn_d =
> >+		lookup_power_well(dev_priv, PUNIT_POWER_WELL_DPIO_CMN_D);
> >+	u32 phy_control = dev_priv->chv_phy_control;
> >+	u32 phy_status = 0;
> >+	u32 tmp;
> >+
> >+	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> >+		phy_status |= PHY_POWERGOOD(DPIO_PHY0);
> >+
> >+		/* this assumes override is only used to enable lanes */
> >+		if ((phy_control & PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0)) == 0)
> >+			phy_control |= PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0);
> >+
> >+		if ((phy_control & PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1)) == 0)
> >+			phy_control |= PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> >+
> >+		/* CL1 is on whenever anything is on in either channel */
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> >+			     PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1)))
> >+			phy_status |= PHY_STATUS_CMN_LDO(DPIO_PHY0, DPIO_CH0);
> >+
> >+		/*
> >+		 * The DPLLB check accounts for the pipe B + port A usage
> >+		 * with CL2 powered up but all the lanes in the second channel
> >+		 * powered down.
> >+		 */
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1)) &&
> >+		    (I915_READ(DPLL(PIPE_B)) & DPLL_VCO_ENABLE) == 0)
> >+			phy_status |= PHY_STATUS_CMN_LDO(DPIO_PHY0, DPIO_CH1);
> >+
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0x3, DPIO_PHY0, DPIO_CH0)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH0, 0);
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xc, DPIO_PHY0, DPIO_CH0)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH0, 1);
> >+
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0x3, DPIO_PHY0, DPIO_CH1)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH1, 0);
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xc, DPIO_PHY0, DPIO_CH1)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY0, DPIO_CH1, 1);
> >+	}
> >+
> >+	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> >+		phy_status |= PHY_POWERGOOD(DPIO_PHY1);
> >+
> >+		/* this assumes override is only used to enable lanes */
> >+		if ((phy_control & PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0)) == 0)
> >+			phy_control |= PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> >+
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0)))
> >+			phy_status |= PHY_STATUS_CMN_LDO(DPIO_PHY1, DPIO_CH0);
> >+
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0x3, DPIO_PHY1, DPIO_CH0)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY1, DPIO_CH0, 0);
> >+		if (BITS_SET(phy_control,
> >+			     PHY_CH_POWER_DOWN_OVRD(0xc, DPIO_PHY1, DPIO_CH0)))
> >+			phy_status |= PHY_STATUS_SPLINE_LDO(DPIO_PHY1, DPIO_CH0, 1);
> >+	}
> >+
> >+	/*
> >+	 * 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))
> >+		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);
> >+}
> >+
> >+#undef BITS_SET
> >+
> >  static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >  					   struct i915_power_well *power_well)
> >  {
> >@@ -998,6 +1099,8 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >  	DRM_DEBUG_KMS("Enabled DPIO PHY%d (PHY_CONTROL=0x%08x)\n",
> >  		      phy, dev_priv->chv_phy_control);
> >+
> >+	assert_chv_phy_status(dev_priv);
> >  }
> >  static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >@@ -1024,6 +1127,8 @@ 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);
> >+
> >+	assert_chv_phy_status(dev_priv);
> >  }
> >  static void assert_chv_phy_powergate(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >@@ -1101,6 +1206,8 @@ bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >  	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d (DPIO_PHY_CONTROL=0x%08x)\n",
> >  		      phy, ch, dev_priv->chv_phy_control);
> >+	assert_chv_phy_status(dev_priv);
> >+
> >  out:
> >  	mutex_unlock(&power_domains->lock);
> >@@ -1130,6 +1237,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >  	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (PHY_CONTROL=0x%08x)\n",
> >  		      phy, ch, mask, dev_priv->chv_phy_control);
> >+	assert_chv_phy_status(dev_priv);
> >+
> Patch looks fine with assertion at right places
> Reviewed-by: Deepak S <deepak.s@xxxxxxxxxxxxxxx>
> >  	assert_chv_phy_powergate(dev_priv, phy, ch, override, mask);
> >  	mutex_unlock(&power_domains->lock);
> >@@ -1302,8 +1411,6 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	intel_runtime_pm_put(dev_priv);
> >  }
> >-#define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
> >-
> >  #define HSW_ALWAYS_ON_POWER_DOMAINS (			\
> >  	BIT(POWER_DOMAIN_PIPE_A) |			\
> >  	BIT(POWER_DOMAIN_TRANSCODER_EDP) |		\
> >@@ -1565,21 +1672,6 @@ static struct i915_power_well chv_power_wells[] = {
> >  	},
> >  };
> >-static struct i915_power_well *lookup_power_well(struct drm_i915_private *dev_priv,
> >-						 int power_well_id)
> >-{
> >-	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> >-	struct i915_power_well *power_well;
> >-	int i;
> >-
> >-	for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) {
> >-		if (power_well->data == power_well_id)
> >-			return power_well;
> >-	}
> >-
> >-	return NULL;
> >-}
> >-
> >  bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> >  				    int power_well_id)
> >  {
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux