[6/7] drm/i915: Implement PHY lane power gating for CHV

Submitted by Ville Syrjälä on April 10, 2015, 3:21 p.m.

Details

Message ID 1428679293-6208-7-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Ville Syrjälä April 10, 2015, 3:21 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Powergate the PHY lanes when they're not needed. For HDMI all four lanes
are needed always, but for DP we can enable only the needed lanes. And
when the port is not used all lanes can be power gated. This could
reduce power consumption a bit when only a subset of the lanes in the
PHY are required.

A bit of extra care is needed to reconstruct the initial state of the
DPIO_PHY_CONTROL register since we can't read it. So instead we read the
actual lane status from the DPLL/PHY_STATUS registers and use that to
determine which lanes ought to be powergated initially.

Also sprinkle a few debug prints around so that we can monitor the
DPIO_PHY_STATUS changes without having to read it and risk corrupting
it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  2 +
 drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 +
 drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
 5 files changed, 82 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 977bad6..34c366a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,10 +1887,12 @@  enum skl_disp_power_wells {
 #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
 #define   DPLL_PORTD_READY_MASK		(0xf)
 #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
+#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
 #define   PHY_LDO_DELAY_0NS			0x0
 #define   PHY_LDO_DELAY_200NS			0x1
 #define   PHY_LDO_DELAY_600NS			0x2
 #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
+#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
 #define   PHY_CH_SU_PSR				0x1
 #define   PHY_CH_DEEP_PSR			0x7
 #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ac38fd8..0b43f99 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2336,6 +2336,8 @@  static void chv_post_disable_dp(struct intel_encoder *encoder)
 
 	intel_dp_link_down(intel_dp);
 
+	chv_powergate_phy_lanes(encoder, 0xf);
+
 	mutex_lock(&dev_priv->dpio_lock);
 
 	/* Propagate soft reset to data lane reset */
@@ -2482,6 +2484,12 @@  static void intel_enable_dp(struct intel_encoder *encoder)
 	if (IS_VALLEYVIEW(dev))
 		vlv_init_panel_power_sequencer(intel_dp);
 
+	if (IS_CHERRYVIEW(dev)) {
+		/* FIXME deal with lane reversal */
+		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
+		chv_powergate_phy_lanes(encoder, lane_mask);
+	}
+
 	intel_dp_enable_port(intel_dp);
 
 	edp_panel_vdd_on(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3ec829a..54bcca8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1238,6 +1238,8 @@  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 
 void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
 
+void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
+
 /* intel_pm.c */
 void intel_init_clock_gating(struct drm_device *dev);
 void intel_suspend_hw(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 24b0aa1..f5842c3 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -916,6 +916,9 @@  static void intel_disable_hdmi(struct intel_encoder *encoder)
 		I915_WRITE(intel_hdmi->hdmi_reg, temp);
 		POSTING_READ(intel_hdmi->hdmi_reg);
 	}
+
+	if (IS_CHERRYVIEW(dev))
+		chv_powergate_phy_lanes(encoder, 0xf);
 }
 
 static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
@@ -1634,6 +1637,8 @@  static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 				   intel_crtc->config->has_hdmi_sink,
 				   adjusted_mode);
 
+	chv_powergate_phy_lanes(encoder, 0x0);
+
 	intel_enable_hdmi(encoder);
 
 	vlv_wait_port_ready(dev_priv, dport, 0x0);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 5cd8a51..d9e00d3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -668,6 +668,9 @@  static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
 
 	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
 	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
+
+	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, dev_priv->chv_phy_control);
 }
 
 static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
@@ -680,10 +683,15 @@  static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 
 	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
 		phy = DPIO_PHY0;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
 		assert_pll_disabled(dev_priv, PIPE_A);
 		assert_pll_disabled(dev_priv, PIPE_B);
 	} else {
 		phy = DPIO_PHY1;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
 		assert_pll_disabled(dev_priv, PIPE_C);
 	}
 
@@ -691,6 +699,25 @@  static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
 	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
 
 	vlv_set_power_well(dev_priv, power_well, false);
+
+	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, dev_priv->chv_phy_control);
+}
+
+void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
+	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
+
+	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
+	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
+	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
+
+	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
+		      phy, ch, mask, dev_priv->chv_phy_control);
 }
 
 static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
@@ -1402,19 +1429,53 @@  static void chv_phy_control_init(struct drm_i915_private *dev_priv)
 	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
 	 * workaround never ever read DISPLAY_PHY_CONTROL, and
 	 * instead maintain a shadow copy ourselves. Use the actual
-	 * power well state to reconstruct the expected initial
-	 * value.
+	 * power well state and lane status to reconstruct the
+	 * expected initial value.
 	 */
 	dev_priv->chv_phy_control =
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
+		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
 		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
 		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
 		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
-	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
+
+	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
+		uint32_t status = I915_READ(DPLL(PIPE_A));
+		unsigned int mask;
+
+		mask = status & DPLL_PORTB_READY_MASK;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
+		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
+
 		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
-	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
+	} else {
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
+	}
+
+	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
+		uint32_t status = I915_READ(DPIO_PHY_STATUS);
+		unsigned int mask;
+
+		mask = status & DPLL_PORTD_READY_MASK;
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
+
 		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
+	} else {
+		dev_priv->chv_phy_control |=
+			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
+	}
+
+	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
+		      dev_priv->chv_phy_control);
 }
 
 static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)

Comments

On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> are needed always, but for DP we can enable only the needed lanes. And
> when the port is not used all lanes can be power gated. This could
> reduce power consumption a bit when only a subset of the lanes in the
> PHY are required.
>
> A bit of extra care is needed to reconstruct the initial state of the
> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> actual lane status from the DPLL/PHY_STATUS registers and use that to
> determine which lanes ought to be powergated initially.
>
> Also sprinkle a few debug prints around so that we can monitor the
> DPIO_PHY_STATUS changes without having to read it and risk corrupting
> it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h         |  2 +
>   drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>   drivers/gpu/drm/i915/intel_drv.h        |  2 +
>   drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>   5 files changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 977bad6..34c366a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>   #define   DPLL_PORTD_READY_MASK		(0xf)
>   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>   #define   PHY_LDO_DELAY_0NS			0x0
>   #define   PHY_LDO_DELAY_200NS			0x1
>   #define   PHY_LDO_DELAY_600NS			0x2
>   #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>   #define   PHY_CH_SU_PSR				0x1
>   #define   PHY_CH_DEEP_PSR			0x7
>   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ac38fd8..0b43f99 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>   
>   	intel_dp_link_down(intel_dp);
>   
> +	chv_powergate_phy_lanes(encoder, 0xf);
> +
>   	mutex_lock(&dev_priv->dpio_lock);
>   
>   	/* Propagate soft reset to data lane reset */
> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>   	if (IS_VALLEYVIEW(dev))
>   		vlv_init_panel_power_sequencer(intel_dp);
>   
> +	if (IS_CHERRYVIEW(dev)) {
> +		/* FIXME deal with lane reversal */
> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> +		chv_powergate_phy_lanes(encoder, lane_mask);
> +	}
> +
>   	intel_dp_enable_port(intel_dp);
>   
>   	edp_panel_vdd_on(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3ec829a..54bcca8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>   
>   void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>   
> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> +
>   /* intel_pm.c */
>   void intel_init_clock_gating(struct drm_device *dev);
>   void intel_suspend_hw(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 24b0aa1..f5842c3 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>   		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>   		POSTING_READ(intel_hdmi->hdmi_reg);
>   	}
> +
> +	if (IS_CHERRYVIEW(dev))
> +		chv_powergate_phy_lanes(encoder, 0xf);
>   }
>   
>   static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>   				   intel_crtc->config->has_hdmi_sink,
>   				   adjusted_mode);
>   
> +	chv_powergate_phy_lanes(encoder, 0x0);
> +
>   	intel_enable_hdmi(encoder);
>   
>   	vlv_wait_port_ready(dev_priv, dport, 0x0);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5cd8a51..d9e00d3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>   
>   	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> +
> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, dev_priv->chv_phy_control);
>   }
>   
>   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>   
>   	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>   		phy = DPIO_PHY0;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>   		assert_pll_disabled(dev_priv, PIPE_A);
>   		assert_pll_disabled(dev_priv, PIPE_B);
>   	} else {
>   		phy = DPIO_PHY1;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>   		assert_pll_disabled(dev_priv, PIPE_C);
>   	}
>   
> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>   
>   	vlv_set_power_well(dev_priv, power_well, false);
> +
> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, dev_priv->chv_phy_control);
> +}
> +
> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> +
> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> +
> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> +		      phy, ch, mask, dev_priv->chv_phy_control);
>   }
>   

Hi Ville,

I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
and this reigster to be programted beforeDPIO_PHY_CONTROL ?

Thanks
Deepak

>   static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>   	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>   	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>   	 * instead maintain a shadow copy ourselves. Use the actual
> -	 * power well state to reconstruct the expected initial
> -	 * value.
> +	 * power well state and lane status to reconstruct the
> +	 * expected initial value.
>   	 */
>   	dev_priv->chv_phy_control =
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> +
> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> +		uint32_t status = I915_READ(DPLL(PIPE_A));
> +		unsigned int mask;
> +
> +		mask = status & DPLL_PORTB_READY_MASK;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> +
>   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> +	} else {
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> +	}
> +
> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> +		unsigned int mask;
> +
> +		mask = status & DPLL_PORTD_READY_MASK;
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> +
>   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> +	} else {
> +		dev_priv->chv_phy_control |=
> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> +	}
> +
> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> +		      dev_priv->chv_phy_control);
>   }
>   
>   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
> 
> 
> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> > are needed always, but for DP we can enable only the needed lanes. And
> > when the port is not used all lanes can be power gated. This could
> > reduce power consumption a bit when only a subset of the lanes in the
> > PHY are required.
> >
> > A bit of extra care is needed to reconstruct the initial state of the
> > DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> > actual lane status from the DPLL/PHY_STATUS registers and use that to
> > determine which lanes ought to be powergated initially.
> >
> > Also sprinkle a few debug prints around so that we can monitor the
> > DPIO_PHY_STATUS changes without having to read it and risk corrupting
> > it.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reg.h         |  2 +
> >   drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
> >   drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >   drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
> >   drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
> >   5 files changed, 82 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 977bad6..34c366a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
> >   #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >   #define   DPLL_PORTD_READY_MASK		(0xf)
> >   #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> > +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
> >   #define   PHY_LDO_DELAY_0NS			0x0
> >   #define   PHY_LDO_DELAY_200NS			0x1
> >   #define   PHY_LDO_DELAY_600NS			0x2
> >   #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> > +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
> >   #define   PHY_CH_SU_PSR				0x1
> >   #define   PHY_CH_DEEP_PSR			0x7
> >   #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index ac38fd8..0b43f99 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
> >   
> >   	intel_dp_link_down(intel_dp);
> >   
> > +	chv_powergate_phy_lanes(encoder, 0xf);
> > +
> >   	mutex_lock(&dev_priv->dpio_lock);
> >   
> >   	/* Propagate soft reset to data lane reset */
> > @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >   	if (IS_VALLEYVIEW(dev))
> >   		vlv_init_panel_power_sequencer(intel_dp);
> >   
> > +	if (IS_CHERRYVIEW(dev)) {
> > +		/* FIXME deal with lane reversal */
> > +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> > +		chv_powergate_phy_lanes(encoder, lane_mask);
> > +	}
> > +
> >   	intel_dp_enable_port(intel_dp);
> >   
> >   	edp_panel_vdd_on(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 3ec829a..54bcca8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >   
> >   void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
> >   
> > +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> > +
> >   /* intel_pm.c */
> >   void intel_init_clock_gating(struct drm_device *dev);
> >   void intel_suspend_hw(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 24b0aa1..f5842c3 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >   		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> >   		POSTING_READ(intel_hdmi->hdmi_reg);
> >   	}
> > +
> > +	if (IS_CHERRYVIEW(dev))
> > +		chv_powergate_phy_lanes(encoder, 0xf);
> >   }
> >   
> >   static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> > @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
> >   				   intel_crtc->config->has_hdmi_sink,
> >   				   adjusted_mode);
> >   
> > +	chv_powergate_phy_lanes(encoder, 0x0);
> > +
> >   	intel_enable_hdmi(encoder);
> >   
> >   	vlv_wait_port_ready(dev_priv, dport, 0x0);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 5cd8a51..d9e00d3 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >   
> >   	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> >   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> > +
> > +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, dev_priv->chv_phy_control);
> >   }
> >   
> >   static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> > @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >   
> >   	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> >   		phy = DPIO_PHY0;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
> >   		assert_pll_disabled(dev_priv, PIPE_A);
> >   		assert_pll_disabled(dev_priv, PIPE_B);
> >   	} else {
> >   		phy = DPIO_PHY1;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
> >   		assert_pll_disabled(dev_priv, PIPE_C);
> >   	}
> >   
> > @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >   	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >   
> >   	vlv_set_power_well(dev_priv, power_well, false);
> > +
> > +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, dev_priv->chv_phy_control);
> > +}
> > +
> > +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> > +{
> > +	struct drm_device *dev = encoder->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> > +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> > +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> > +
> > +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> > +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> > +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> > +
> > +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> > +		      phy, ch, mask, dev_priv->chv_phy_control);
> >   }
> >   
> 
> Hi Ville,
> 
> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
> and this reigster to be programted beforeDPIO_PHY_CONTROL ?

Doh. Nice catch. Looks like the dynamic power down bits default to off.
I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
I'll cook up a patch to flip them and run a few tests to see if stuff
still works.

> 
> Thanks
> Deepak
> 
> >   static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> > @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >   	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> >   	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> >   	 * instead maintain a shadow copy ourselves. Use the actual
> > -	 * power well state to reconstruct the expected initial
> > -	 * value.
> > +	 * power well state and lane status to reconstruct the
> > +	 * expected initial value.
> >   	 */
> >   	dev_priv->chv_phy_control =
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> > +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
> >   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> >   		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> >   		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> > -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> > +
> > +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> > +		uint32_t status = I915_READ(DPLL(PIPE_A));
> > +		unsigned int mask;
> > +
> > +		mask = status & DPLL_PORTB_READY_MASK;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> > +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> > +
> >   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> > -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> > +	} else {
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> > +	}
> > +
> > +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> > +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> > +		unsigned int mask;
> > +
> > +		mask = status & DPLL_PORTD_READY_MASK;
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> > +
> >   		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> > +	} else {
> > +		dev_priv->chv_phy_control |=
> > +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> > +	}
> > +
> > +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> > +		      dev_priv->chv_phy_control);
> >   }
> >   
> >   static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> 

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
> On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
>>
>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
>>> are needed always, but for DP we can enable only the needed lanes. And
>>> when the port is not used all lanes can be power gated. This could
>>> reduce power consumption a bit when only a subset of the lanes in the
>>> PHY are required.
>>>
>>> A bit of extra care is needed to reconstruct the initial state of the
>>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
>>> actual lane status from the DPLL/PHY_STATUS registers and use that to
>>> determine which lanes ought to be powergated initially.
>>>
>>> Also sprinkle a few debug prints around so that we can monitor the
>>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
>>> it.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_reg.h         |  2 +
>>>    drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>>>    drivers/gpu/drm/i915/intel_drv.h        |  2 +
>>>    drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>>>    5 files changed, 82 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 977bad6..34c366a 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>    #define   DPLL_PORTD_READY_MASK		(0xf)
>>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>>>    #define   PHY_LDO_DELAY_0NS			0x0
>>>    #define   PHY_LDO_DELAY_200NS			0x1
>>>    #define   PHY_LDO_DELAY_600NS			0x2
>>>    #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>>>    #define   PHY_CH_SU_PSR				0x1
>>>    #define   PHY_CH_DEEP_PSR			0x7
>>>    #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index ac38fd8..0b43f99 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>>>    
>>>    	intel_dp_link_down(intel_dp);
>>>    
>>> +	chv_powergate_phy_lanes(encoder, 0xf);
>>> +
>>>    	mutex_lock(&dev_priv->dpio_lock);
>>>    
>>>    	/* Propagate soft reset to data lane reset */
>>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>    	if (IS_VALLEYVIEW(dev))
>>>    		vlv_init_panel_power_sequencer(intel_dp);
>>>    
>>> +	if (IS_CHERRYVIEW(dev)) {
>>> +		/* FIXME deal with lane reversal */
>>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
>>> +		chv_powergate_phy_lanes(encoder, lane_mask);
>>> +	}
>>> +
>>>    	intel_dp_enable_port(intel_dp);
>>>    
>>>    	edp_panel_vdd_on(intel_dp);
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 3ec829a..54bcca8 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>>    
>>>    void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>>>    
>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
>>> +
>>>    /* intel_pm.c */
>>>    void intel_init_clock_gating(struct drm_device *dev);
>>>    void intel_suspend_hw(struct drm_device *dev);
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 24b0aa1..f5842c3 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>    		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>>>    		POSTING_READ(intel_hdmi->hdmi_reg);
>>>    	}
>>> +
>>> +	if (IS_CHERRYVIEW(dev))
>>> +		chv_powergate_phy_lanes(encoder, 0xf);
>>>    }
>>>    
>>>    static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>>>    				   intel_crtc->config->has_hdmi_sink,
>>>    				   adjusted_mode);
>>>    
>>> +	chv_powergate_phy_lanes(encoder, 0x0);
>>> +
>>>    	intel_enable_hdmi(encoder);
>>>    
>>>    	vlv_wait_port_ready(dev_priv, dport, 0x0);
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index 5cd8a51..d9e00d3 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>    
>>>    	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>> +
>>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, dev_priv->chv_phy_control);
>>>    }
>>>    
>>>    static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    
>>>    	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>>>    		phy = DPIO_PHY0;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>>>    		assert_pll_disabled(dev_priv, PIPE_A);
>>>    		assert_pll_disabled(dev_priv, PIPE_B);
>>>    	} else {
>>>    		phy = DPIO_PHY1;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>>>    		assert_pll_disabled(dev_priv, PIPE_C);
>>>    	}
>>>    
>>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>    
>>>    	vlv_set_power_well(dev_priv, power_well, false);
>>> +
>>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, dev_priv->chv_phy_control);
>>> +}
>>> +
>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
>>> +{
>>> +	struct drm_device *dev = encoder->base.dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
>>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
>>> +
>>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
>>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>> +
>>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
>>> +		      phy, ch, mask, dev_priv->chv_phy_control);
>>>    }
>>>    
>> Hi Ville,
>>
>> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
>> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
> Doh. Nice catch. Looks like the dynamic power down bits default to off.
> I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
> I'll cook up a patch to flip them and run a few tests to see if stuff
> still works.

Ok Thanks
One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?

>
>> Thanks
>> Deepak
>>
>>>    static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>    	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>>    	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>>>    	 * instead maintain a shadow copy ourselves. Use the actual
>>> -	 * power well state to reconstruct the expected initial
>>> -	 * value.
>>> +	 * power well state and lane status to reconstruct the
>>> +	 * expected initial value.
>>>    	 */
>>>    	dev_priv->chv_phy_control =
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>> +
>>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
>>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
>>> +		unsigned int mask;
>>> +
>>> +		mask = status & DPLL_PORTB_READY_MASK;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
>>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
>>> +
>>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>> +	} else {
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
>>> +	}
>>> +
>>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
>>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
>>> +		unsigned int mask;
>>> +
>>> +		mask = status & DPLL_PORTD_READY_MASK;
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
>>> +
>>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>> +	} else {
>>> +		dev_priv->chv_phy_control |=
>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
>>> +	}
>>> +
>>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
>>> +		      dev_priv->chv_phy_control);
>>>    }
>>>    
>>>    static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
On Sat, May 09, 2015 at 11:05:27AM +0530, Deepak S wrote:
> 
> 
> On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
> > On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
> >>
> >> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
> >>> are needed always, but for DP we can enable only the needed lanes. And
> >>> when the port is not used all lanes can be power gated. This could
> >>> reduce power consumption a bit when only a subset of the lanes in the
> >>> PHY are required.
> >>>
> >>> A bit of extra care is needed to reconstruct the initial state of the
> >>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
> >>> actual lane status from the DPLL/PHY_STATUS registers and use that to
> >>> determine which lanes ought to be powergated initially.
> >>>
> >>> Also sprinkle a few debug prints around so that we can monitor the
> >>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
> >>> it.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_reg.h         |  2 +
> >>>    drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
> >>>    drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >>>    drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
> >>>    drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
> >>>    5 files changed, 82 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 977bad6..34c366a 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
> >>>    #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
> >>>    #define   DPLL_PORTD_READY_MASK		(0xf)
> >>>    #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
> >>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
> >>>    #define   PHY_LDO_DELAY_0NS			0x0
> >>>    #define   PHY_LDO_DELAY_200NS			0x1
> >>>    #define   PHY_LDO_DELAY_600NS			0x2
> >>>    #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
> >>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
> >>>    #define   PHY_CH_SU_PSR				0x1
> >>>    #define   PHY_CH_DEEP_PSR			0x7
> >>>    #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>> index ac38fd8..0b43f99 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
> >>>    
> >>>    	intel_dp_link_down(intel_dp);
> >>>    
> >>> +	chv_powergate_phy_lanes(encoder, 0xf);
> >>> +
> >>>    	mutex_lock(&dev_priv->dpio_lock);
> >>>    
> >>>    	/* Propagate soft reset to data lane reset */
> >>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >>>    	if (IS_VALLEYVIEW(dev))
> >>>    		vlv_init_panel_power_sequencer(intel_dp);
> >>>    
> >>> +	if (IS_CHERRYVIEW(dev)) {
> >>> +		/* FIXME deal with lane reversal */
> >>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
> >>> +		chv_powergate_phy_lanes(encoder, lane_mask);
> >>> +	}
> >>> +
> >>>    	intel_dp_enable_port(intel_dp);
> >>>    
> >>>    	edp_panel_vdd_on(intel_dp);
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 3ec829a..54bcca8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >>>    
> >>>    void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
> >>>    
> >>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
> >>> +
> >>>    /* intel_pm.c */
> >>>    void intel_init_clock_gating(struct drm_device *dev);
> >>>    void intel_suspend_hw(struct drm_device *dev);
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index 24b0aa1..f5842c3 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
> >>>    		I915_WRITE(intel_hdmi->hdmi_reg, temp);
> >>>    		POSTING_READ(intel_hdmi->hdmi_reg);
> >>>    	}
> >>> +
> >>> +	if (IS_CHERRYVIEW(dev))
> >>> +		chv_powergate_phy_lanes(encoder, 0xf);
> >>>    }
> >>>    
> >>>    static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
> >>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
> >>>    				   intel_crtc->config->has_hdmi_sink,
> >>>    				   adjusted_mode);
> >>>    
> >>> +	chv_powergate_phy_lanes(encoder, 0x0);
> >>> +
> >>>    	intel_enable_hdmi(encoder);
> >>>    
> >>>    	vlv_wait_port_ready(dev_priv, dport, 0x0);
> >>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> index 5cd8a51..d9e00d3 100644
> >>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
> >>>    
> >>>    	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
> >>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>> +
> >>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >>>    static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>>    
> >>>    	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
> >>>    		phy = DPIO_PHY0;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
> >>>    		assert_pll_disabled(dev_priv, PIPE_A);
> >>>    		assert_pll_disabled(dev_priv, PIPE_B);
> >>>    	} else {
> >>>    		phy = DPIO_PHY1;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
> >>>    		assert_pll_disabled(dev_priv, PIPE_C);
> >>>    	}
> >>>    
> >>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
> >>>    	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>>    
> >>>    	vlv_set_power_well(dev_priv, power_well, false);
> >>> +
> >>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, dev_priv->chv_phy_control);
> >>> +}
> >>> +
> >>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
> >>> +{
> >>> +	struct drm_device *dev = encoder->base.dev;
> >>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
> >>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
> >>> +
> >>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
> >>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
> >>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
> >>> +
> >>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
> >>> +		      phy, ch, mask, dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >> Hi Ville,
> >>
> >> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
> >> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
> > Doh. Nice catch. Looks like the dynamic power down bits default to off.
> > I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
> > I'll cook up a patch to flip them and run a few tests to see if stuff
> > still works.
> 
> Ok Thanks
> One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?

I've taken a stab at this, and sadly its making things quite a bit more
complicated. Whenever the lane is power gated we also lose the relevant
registers, so if we power gate the unused lanes before programming all
the PCS/TX registers, we need to change the code so that it doesn't
try to program the registers in the power gated lanes.

Also I'm not entirely sure how LRC etc. works when some/all lanes power
gated. There are some notes that sort of make me think the hw internally
powers up everything regardless of the input signals to guarantee that
things are properly calibrated, but if we lose the registers does that
mean we lose the calibrationresults as well?

And on the other hand there are a bunch of other notes that make me think
they assumed we would only powergate the unused lanes after link training
has finished, which again suggests that we should train the link with max
lanes initially and drop the unused lanes afterwards. But then it's not
clear what happens if we need to increase the number of lanes afterwards
without doing a full PHY reinit.

I also had the machine hard hang if I enabled the CL1 PSR power down bit
in the single channel PHY, whereas setting it in the dual channel PHY is
apparently OK.

So looks like this topic needs a bit more study.

> 
> >
> >> Thanks
> >> Deepak
> >>
> >>>    static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
> >>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
> >>>    	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
> >>>    	 * workaround never ever read DISPLAY_PHY_CONTROL, and
> >>>    	 * instead maintain a shadow copy ourselves. Use the actual
> >>> -	 * power well state to reconstruct the expected initial
> >>> -	 * value.
> >>> +	 * power well state and lane status to reconstruct the
> >>> +	 * expected initial value.
> >>>    	 */
> >>>    	dev_priv->chv_phy_control =
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
> >>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
> >>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
> >>>    		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
> >>>    		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
> >>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
> >>> +
> >>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
> >>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
> >>> +		unsigned int mask;
> >>> +
> >>> +		mask = status & DPLL_PORTB_READY_MASK;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
> >>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
> >>> +
> >>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
> >>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
> >>> +	} else {
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
> >>> +	}
> >>> +
> >>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
> >>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
> >>> +		unsigned int mask;
> >>> +
> >>> +		mask = status & DPLL_PORTD_READY_MASK;
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
> >>> +
> >>>    		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
> >>> +	} else {
> >>> +		dev_priv->chv_phy_control |=
> >>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
> >>> +	}
> >>> +
> >>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
> >>> +		      dev_priv->chv_phy_control);
> >>>    }
> >>>    
> >>>    static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
On Monday 11 May 2015 05:13 PM, Ville Syrjälä wrote:
> On Sat, May 09, 2015 at 11:05:27AM +0530, Deepak S wrote:
>>
>> On Friday 08 May 2015 09:35 PM, Ville Syrjälä wrote:
>>> On Fri, May 08, 2015 at 08:19:12PM +0530, Deepak S wrote:
>>>> On Friday 10 April 2015 08:51 PM, ville.syrjala@linux.intel.com wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> Powergate the PHY lanes when they're not needed. For HDMI all four lanes
>>>>> are needed always, but for DP we can enable only the needed lanes. And
>>>>> when the port is not used all lanes can be power gated. This could
>>>>> reduce power consumption a bit when only a subset of the lanes in the
>>>>> PHY are required.
>>>>>
>>>>> A bit of extra care is needed to reconstruct the initial state of the
>>>>> DPIO_PHY_CONTROL register since we can't read it. So instead we read the
>>>>> actual lane status from the DPLL/PHY_STATUS registers and use that to
>>>>> determine which lanes ought to be powergated initially.
>>>>>
>>>>> Also sprinkle a few debug prints around so that we can monitor the
>>>>> DPIO_PHY_STATUS changes without having to read it and risk corrupting
>>>>> it.
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_reg.h         |  2 +
>>>>>     drivers/gpu/drm/i915/intel_dp.c         |  8 ++++
>>>>>     drivers/gpu/drm/i915/intel_drv.h        |  2 +
>>>>>     drivers/gpu/drm/i915/intel_hdmi.c       |  5 +++
>>>>>     drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++++++++--
>>>>>     5 files changed, 82 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 977bad6..34c366a 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -1887,10 +1887,12 @@ enum skl_disp_power_wells {
>>>>>     #define DPIO_PHY_STATUS			(VLV_DISPLAY_BASE + 0x6240)
>>>>>     #define   DPLL_PORTD_READY_MASK		(0xf)
>>>>>     #define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
>>>>> +#define   PHY_CH_POWER_DOWN_OVRD_EN(phy, ch)	(1 << (2*(phy)+(ch)+27))
>>>>>     #define   PHY_LDO_DELAY_0NS			0x0
>>>>>     #define   PHY_LDO_DELAY_200NS			0x1
>>>>>     #define   PHY_LDO_DELAY_600NS			0x2
>>>>>     #define   PHY_LDO_SEQ_DELAY(delay, phy)		((delay) << (2*(phy)+23))
>>>>> +#define   PHY_CH_POWER_DOWN_OVRD(mask, phy, ch)	((mask) << (8*(phy)+4*(ch)+11))
>>>>>     #define   PHY_CH_SU_PSR				0x1
>>>>>     #define   PHY_CH_DEEP_PSR			0x7
>>>>>     #define   PHY_CH_POWER_MODE(mode, phy, ch)	((mode) << (6*(phy)+3*(ch)+2))
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index ac38fd8..0b43f99 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -2336,6 +2336,8 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>>>>>     
>>>>>     	intel_dp_link_down(intel_dp);
>>>>>     
>>>>> +	chv_powergate_phy_lanes(encoder, 0xf);
>>>>> +
>>>>>     	mutex_lock(&dev_priv->dpio_lock);
>>>>>     
>>>>>     	/* Propagate soft reset to data lane reset */
>>>>> @@ -2482,6 +2484,12 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>>>>>     	if (IS_VALLEYVIEW(dev))
>>>>>     		vlv_init_panel_power_sequencer(intel_dp);
>>>>>     
>>>>> +	if (IS_CHERRYVIEW(dev)) {
>>>>> +		/* FIXME deal with lane reversal */
>>>>> +		lane_mask = 0xf & ~((1 << intel_dp->lane_count) - 1);
>>>>> +		chv_powergate_phy_lanes(encoder, lane_mask);
>>>>> +	}
>>>>> +
>>>>>     	intel_dp_enable_port(intel_dp);
>>>>>     
>>>>>     	edp_panel_vdd_on(intel_dp);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 3ec829a..54bcca8 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -1238,6 +1238,8 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>>>>     
>>>>>     void intel_display_set_init_power(struct drm_i915_private *dev, bool enable);
>>>>>     
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask);
>>>>> +
>>>>>     /* intel_pm.c */
>>>>>     void intel_init_clock_gating(struct drm_device *dev);
>>>>>     void intel_suspend_hw(struct drm_device *dev);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> index 24b0aa1..f5842c3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>> @@ -916,6 +916,9 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>>>     		I915_WRITE(intel_hdmi->hdmi_reg, temp);
>>>>>     		POSTING_READ(intel_hdmi->hdmi_reg);
>>>>>     	}
>>>>> +
>>>>> +	if (IS_CHERRYVIEW(dev))
>>>>> +		chv_powergate_phy_lanes(encoder, 0xf);
>>>>>     }
>>>>>     
>>>>>     static int hdmi_portclock_limit(struct intel_hdmi *hdmi, bool respect_dvi_limit)
>>>>> @@ -1634,6 +1637,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
>>>>>     				   intel_crtc->config->has_hdmi_sink,
>>>>>     				   adjusted_mode);
>>>>>     
>>>>> +	chv_powergate_phy_lanes(encoder, 0x0);
>>>>> +
>>>>>     	intel_enable_hdmi(encoder);
>>>>>     
>>>>>     	vlv_wait_port_ready(dev_priv, dport, 0x0);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> index 5cd8a51..d9e00d3 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> @@ -668,6 +668,9 @@ static void chv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_priv,
>>>>>     
>>>>>     	dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
>>>>>     	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Enabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>>>     static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>> @@ -680,10 +683,15 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>>     
>>>>>     	if (power_well->data == PUNIT_POWER_WELL_DPIO_CMN_BC) {
>>>>>     		phy = DPIO_PHY0;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0) |
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH1);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_A);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_B);
>>>>>     	} else {
>>>>>     		phy = DPIO_PHY1;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, phy, DPIO_CH0);
>>>>>     		assert_pll_disabled(dev_priv, PIPE_C);
>>>>>     	}
>>>>>     
>>>>> @@ -691,6 +699,25 @@ static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
>>>>>     	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>>     
>>>>>     	vlv_set_power_well(dev_priv, power_well, false);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Disabled DPIO PHY%d (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, dev_priv->chv_phy_control);
>>>>> +}
>>>>> +
>>>>> +void chv_powergate_phy_lanes(struct intel_encoder *encoder, unsigned int mask)
>>>>> +{
>>>>> +	struct drm_device *dev = encoder->base.dev;
>>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>>>>> +	enum dpio_phy phy = DPIO_PHY(intel_crtc->pipe);
>>>>> +	enum dpio_channel ch = vlv_dport_to_channel(enc_to_dig_port(&encoder->base));
>>>>> +
>>>>> +	dev_priv->chv_phy_control &= ~PHY_CH_POWER_DOWN_OVRD(0xf, phy, ch);
>>>>> +	dev_priv->chv_phy_control |= PHY_CH_POWER_DOWN_OVRD(mask, phy, ch);
>>>>> +	I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv->chv_phy_control);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Power gating DPIO PHY%d CH%d lanes 0x%x (DPIO_PHY_CONTROL=0x%08x)\n",
>>>>> +		      phy, ch, mask, dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>> Hi Ville,
>>>>
>>>> I think based on the spec, we needto program 0x8170 as 0xxxC3xxxx to affect the DDI0 power down
>>>> and this reigster to be programted beforeDPIO_PHY_CONTROL ?
>>> Doh. Nice catch. Looks like the dynamic power down bits default to off.
>>> I also found the same bit in CL2 DW6, which isn't mentioned by the spec.
>>> I'll cook up a patch to flip them and run a few tests to see if stuff
>>> still works.
>> Ok Thanks
>> One more Q. I think we need to re-program phy lanes power down override /enable values after power gating/ungating PHY0/1?
> I've taken a stab at this, and sadly its making things quite a bit more
> complicated. Whenever the lane is power gated we also lose the relevant
> registers, so if we power gate the unused lanes before programming all
> the PCS/TX registers, we need to change the code so that it doesn't
> try to program the registers in the power gated lanes.
>
> Also I'm not entirely sure how LRC etc. works when some/all lanes power
> gated. There are some notes that sort of make me think the hw internally
> powers up everything regardless of the input signals to guarantee that
> things are properly calibrated, but if we lose the registers does that
> mean we lose the calibrationresults as well?
>
> And on the other hand there are a bunch of other notes that make me think
> they assumed we would only powergate the unused lanes after link training
> has finished, which again suggests that we should train the link with max
> lanes initially and drop the unused lanes afterwards. But then it's not
> clear what happens if we need to increase the number of lanes afterwards
> without doing a full PHY reinit.
>
> I also had the machine hard hang if I enabled the CL1 PSR power down bit
> in the single channel PHY, whereas setting it in the dual channel PHY is
> apparently OK.
>
> So looks like this topic needs a bit more study.

Yes ville. I agree we need some more study. I will also do some
experiments to find the HW behavior in different scenario.

>>>> Thanks
>>>> Deepak
>>>>
>>>>>     static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>>>>> @@ -1402,19 +1429,53 @@ static void chv_phy_control_init(struct drm_i915_private *dev_priv)
>>>>>     	 * DISPLAY_PHY_CONTROL can get corrupted if read. As a
>>>>>     	 * workaround never ever read DISPLAY_PHY_CONTROL, and
>>>>>     	 * instead maintain a shadow copy ourselves. Use the actual
>>>>> -	 * power well state to reconstruct the expected initial
>>>>> -	 * value.
>>>>> +	 * power well state and lane status to reconstruct the
>>>>> +	 * expected initial value.
>>>>>     	 */
>>>>>     	dev_priv->chv_phy_control =
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH0) |
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY0, DPIO_CH1) |
>>>>> +		PHY_CH_POWER_DOWN_OVRD_EN(DPIO_PHY1, DPIO_CH0) |
>>>>>     		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY0) |
>>>>>     		PHY_LDO_SEQ_DELAY(PHY_LDO_DELAY_600NS, DPIO_PHY1) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH0) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY0, DPIO_CH1) |
>>>>>     		PHY_CH_POWER_MODE(PHY_CH_SU_PSR, DPIO_PHY1, DPIO_CH0);
>>>>> -	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc))
>>>>> +
>>>>> +	if (cmn_bc->ops->is_enabled(dev_priv, cmn_bc)) {
>>>>> +		uint32_t status = I915_READ(DPLL(PIPE_A));
>>>>> +		unsigned int mask;
>>>>> +
>>>>> +		mask = status & DPLL_PORTB_READY_MASK;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH0);
>>>>> +		mask = (status & DPLL_PORTC_READY_MASK) >> 4;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY0, DPIO_CH1);
>>>>> +
>>>>>     		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY0);
>>>>> -	if (cmn_d->ops->is_enabled(dev_priv, cmn_d))
>>>>> +	} else {
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH0) |
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY0, DPIO_CH1);
>>>>> +	}
>>>>> +
>>>>> +	if (cmn_d->ops->is_enabled(dev_priv, cmn_d)) {
>>>>> +		uint32_t status = I915_READ(DPIO_PHY_STATUS);
>>>>> +		unsigned int mask;
>>>>> +
>>>>> +		mask = status & DPLL_PORTD_READY_MASK;
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(mask, DPIO_PHY1, DPIO_CH0);
>>>>> +
>>>>>     		dev_priv->chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(DPIO_PHY1);
>>>>> +	} else {
>>>>> +		dev_priv->chv_phy_control |=
>>>>> +			PHY_CH_POWER_DOWN_OVRD(0xf, DPIO_PHY1, DPIO_CH0);
>>>>> +	}
>>>>> +
>>>>> +	DRM_DEBUG_KMS("Initial DPIO_PHY_CONTROL=0x%08x\n",
>>>>> +		      dev_priv->chv_phy_control);
>>>>>     }
>>>>>     
>>>>>     static void vlv_cmnlane_wa(struct drm_i915_private *dev_priv)
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx