diff mbox

[02/16] drm/i915: Make {vlv, chv}_{disable, update}_pll() more similar

Message ID 1458052809-23426-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä March 15, 2016, 2:39 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The VLV and CHV DPLL disable and update are almost identical in
how the DPLL/DPLL_MD registers need to be set up. But the code
looks more different than it really is. Try to bring them into
line.

v2: s/chv_update_pll/chv_compute_dpll/

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++----------------------
 1 file changed, 25 insertions(+), 38 deletions(-)

Comments

Jani Nikula March 30, 2016, 1:31 p.m. UTC | #1 
On Tue, 15 Mar 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The VLV and CHV DPLL disable and update are almost identical in
> how the DPLL/DPLL_MD registers need to be set up. But the code
> looks more different than it really is. Try to bring them into
> line.
>
> v2: s/chv_update_pll/chv_compute_dpll/
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 22930f05457c..414ed5007e60 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1759,16 +1759,13 @@ static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	/* Make sure the pipe isn't still relying on us */
>  	assert_pipe_disabled(dev_priv, pipe);
>  
> -	/*
> -	 * Leave integrated clock source and reference clock enabled for pipe B.
> -	 * The latter is needed for VGA hotplug / manual detection.
> -	 */

So, you change this to keep the reference clock enabled for both
pipes. Deserves a mention in the commit message. AFAICT it's the only
functional change in the patch.

Other than that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> -	val = DPLL_VGA_MODE_DIS;
> -	if (pipe == PIPE_B)
> -		val = DPLL_INTEGRATED_CRI_CLK_VLV | DPLL_REF_CLK_ENABLE_VLV;
> +	val = DPLL_INTEGRATED_REF_CLK_VLV |
> +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> +	if (pipe != PIPE_A)
> +		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
> +
>  	I915_WRITE(DPLL(pipe), val);
>  	POSTING_READ(DPLL(pipe));
> -
>  }
>  
>  static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> @@ -1779,11 +1776,11 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	/* Make sure the pipe isn't still relying on us */
>  	assert_pipe_disabled(dev_priv, pipe);
>  
> -	/* Set PLL en = 0 */
>  	val = DPLL_SSC_REF_CLK_CHV |
>  		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
>  	if (pipe != PIPE_A)
>  		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
> +
>  	I915_WRITE(DPLL(pipe), val);
>  	POSTING_READ(DPLL(pipe));
>  
> @@ -7240,24 +7237,27 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
>  static void vlv_compute_dpll(struct intel_crtc *crtc,
>  			     struct intel_crtc_state *pipe_config)
>  {
> -	u32 dpll, dpll_md;
> +	pipe_config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> +		DPLL_VCO_ENABLE | DPLL_EXT_BUFFER_ENABLE_VLV;
> +	if (crtc->pipe != PIPE_A)
> +		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
> -	/*
> -	 * Enable DPIO clock input. We should never disable the reference
> -	 * clock for pipe B, since VGA hotplug / manual detection depends
> -	 * on it.
> -	 */
> -	dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REF_CLK_ENABLE_VLV |
> -		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_REF_CLK_VLV;
> -	/* We should never disable this, set it here for state tracking */
> -	if (crtc->pipe == PIPE_B)
> -		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> -	dpll |= DPLL_VCO_ENABLE;
> -	pipe_config->dpll_hw_state.dpll = dpll;
> +	pipe_config->dpll_hw_state.dpll_md =
> +		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> +}
> +
> +static void chv_compute_dpll(struct intel_crtc *crtc,
> +			     struct intel_crtc_state *pipe_config)
> +{
> +	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
> +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> +		DPLL_VCO_ENABLE;
> +	if (crtc->pipe != PIPE_A)
> +		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
>  
> -	dpll_md = (pipe_config->pixel_multiplier - 1)
> -		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
> -	pipe_config->dpll_hw_state.dpll_md = dpll_md;
> +	pipe_config->dpll_hw_state.dpll_md =
> +		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
>  }
>  
>  static void vlv_prepare_pll(struct intel_crtc *crtc,
> @@ -7351,19 +7351,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>  	mutex_unlock(&dev_priv->sb_lock);
>  }
>  
> -static void chv_compute_dpll(struct intel_crtc *crtc,
> -			     struct intel_crtc_state *pipe_config)
> -{
> -	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
> -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> -		DPLL_VCO_ENABLE;
> -	if (crtc->pipe != PIPE_A)
> -		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> -
> -	pipe_config->dpll_hw_state.dpll_md =
> -		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> -}
> -
>  static void chv_prepare_pll(struct intel_crtc *crtc,
>  			    const struct intel_crtc_state *pipe_config)
>  {
Ville Syrjälä April 1, 2016, 7:59 p.m. UTC | #2 
On Wed, Mar 30, 2016 at 04:31:04PM +0300, Jani Nikula wrote:
> On Tue, 15 Mar 2016, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The VLV and CHV DPLL disable and update are almost identical in
> > how the DPLL/DPLL_MD registers need to be set up. But the code
> > looks more different than it really is. Try to bring them into
> > line.
> >
> > v2: s/chv_update_pll/chv_compute_dpll/
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++----------------------
> >  1 file changed, 25 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 22930f05457c..414ed5007e60 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1759,16 +1759,13 @@ static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  	/* Make sure the pipe isn't still relying on us */
> >  	assert_pipe_disabled(dev_priv, pipe);
> >  
> > -	/*
> > -	 * Leave integrated clock source and reference clock enabled for pipe B.
> > -	 * The latter is needed for VGA hotplug / manual detection.
> > -	 */
> 
> So, you change this to keep the reference clock enabled for both
> pipes. Deserves a mention in the commit message. AFAICT it's the only
> functional change in the patch.

Added the following note while pushing:

"Note that we now leave the refclock always enabled for both
 DPLLs in the dual channel PHY. But that's perfectly fine since
 it's the same clock, and we anyway already do that when turning
 the disp2d power well on."

> 
> Other than that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> > -	val = DPLL_VGA_MODE_DIS;
> > -	if (pipe == PIPE_B)
> > -		val = DPLL_INTEGRATED_CRI_CLK_VLV | DPLL_REF_CLK_ENABLE_VLV;
> > +	val = DPLL_INTEGRATED_REF_CLK_VLV |
> > +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> > +	if (pipe != PIPE_A)
> > +		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
> > +
> >  	I915_WRITE(DPLL(pipe), val);
> >  	POSTING_READ(DPLL(pipe));
> > -
> >  }
> >  
> >  static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> > @@ -1779,11 +1776,11 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> >  	/* Make sure the pipe isn't still relying on us */
> >  	assert_pipe_disabled(dev_priv, pipe);
> >  
> > -	/* Set PLL en = 0 */
> >  	val = DPLL_SSC_REF_CLK_CHV |
> >  		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
> >  	if (pipe != PIPE_A)
> >  		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
> > +
> >  	I915_WRITE(DPLL(pipe), val);
> >  	POSTING_READ(DPLL(pipe));
> >  
> > @@ -7240,24 +7237,27 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
> >  static void vlv_compute_dpll(struct intel_crtc *crtc,
> >  			     struct intel_crtc_state *pipe_config)
> >  {
> > -	u32 dpll, dpll_md;
> > +	pipe_config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
> > +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> > +		DPLL_VCO_ENABLE | DPLL_EXT_BUFFER_ENABLE_VLV;
> > +	if (crtc->pipe != PIPE_A)
> > +		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> >  
> > -	/*
> > -	 * Enable DPIO clock input. We should never disable the reference
> > -	 * clock for pipe B, since VGA hotplug / manual detection depends
> > -	 * on it.
> > -	 */
> > -	dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REF_CLK_ENABLE_VLV |
> > -		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_REF_CLK_VLV;
> > -	/* We should never disable this, set it here for state tracking */
> > -	if (crtc->pipe == PIPE_B)
> > -		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> > -	dpll |= DPLL_VCO_ENABLE;
> > -	pipe_config->dpll_hw_state.dpll = dpll;
> > +	pipe_config->dpll_hw_state.dpll_md =
> > +		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > +}
> > +
> > +static void chv_compute_dpll(struct intel_crtc *crtc,
> > +			     struct intel_crtc_state *pipe_config)
> > +{
> > +	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
> > +		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> > +		DPLL_VCO_ENABLE;
> > +	if (crtc->pipe != PIPE_A)
> > +		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> >  
> > -	dpll_md = (pipe_config->pixel_multiplier - 1)
> > -		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > -	pipe_config->dpll_hw_state.dpll_md = dpll_md;
> > +	pipe_config->dpll_hw_state.dpll_md =
> > +		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> >  }
> >  
> >  static void vlv_prepare_pll(struct intel_crtc *crtc,
> > @@ -7351,19 +7351,6 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
> >  	mutex_unlock(&dev_priv->sb_lock);
> >  }
> >  
> > -static void chv_compute_dpll(struct intel_crtc *crtc,
> > -			     struct intel_crtc_state *pipe_config)
> > -{
> > -	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
> > -		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> > -		DPLL_VCO_ENABLE;
> > -	if (crtc->pipe != PIPE_A)
> > -		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> > -
> > -	pipe_config->dpll_hw_state.dpll_md =
> > -		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> > -}
> > -
> >  static void chv_prepare_pll(struct intel_crtc *crtc,
> >  			    const struct intel_crtc_state *pipe_config)
> >  {
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 22930f05457c..414ed5007e60 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1759,16 +1759,13 @@  static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	/* Make sure the pipe isn't still relying on us */
 	assert_pipe_disabled(dev_priv, pipe);
 
-	/*
-	 * Leave integrated clock source and reference clock enabled for pipe B.
-	 * The latter is needed for VGA hotplug / manual detection.
-	 */
-	val = DPLL_VGA_MODE_DIS;
-	if (pipe == PIPE_B)
-		val = DPLL_INTEGRATED_CRI_CLK_VLV | DPLL_REF_CLK_ENABLE_VLV;
+	val = DPLL_INTEGRATED_REF_CLK_VLV |
+		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
+	if (pipe != PIPE_A)
+		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
+
 	I915_WRITE(DPLL(pipe), val);
 	POSTING_READ(DPLL(pipe));
-
 }
 
 static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
@@ -1779,11 +1776,11 @@  static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
 	/* Make sure the pipe isn't still relying on us */
 	assert_pipe_disabled(dev_priv, pipe);
 
-	/* Set PLL en = 0 */
 	val = DPLL_SSC_REF_CLK_CHV |
 		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS;
 	if (pipe != PIPE_A)
 		val |= DPLL_INTEGRATED_CRI_CLK_VLV;
+
 	I915_WRITE(DPLL(pipe), val);
 	POSTING_READ(DPLL(pipe));
 
@@ -7240,24 +7237,27 @@  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
 static void vlv_compute_dpll(struct intel_crtc *crtc,
 			     struct intel_crtc_state *pipe_config)
 {
-	u32 dpll, dpll_md;
+	pipe_config->dpll_hw_state.dpll = DPLL_INTEGRATED_REF_CLK_VLV |
+		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
+		DPLL_VCO_ENABLE | DPLL_EXT_BUFFER_ENABLE_VLV;
+	if (crtc->pipe != PIPE_A)
+		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	/*
-	 * Enable DPIO clock input. We should never disable the reference
-	 * clock for pipe B, since VGA hotplug / manual detection depends
-	 * on it.
-	 */
-	dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REF_CLK_ENABLE_VLV |
-		DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_REF_CLK_VLV;
-	/* We should never disable this, set it here for state tracking */
-	if (crtc->pipe == PIPE_B)
-		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
-	dpll |= DPLL_VCO_ENABLE;
-	pipe_config->dpll_hw_state.dpll = dpll;
+	pipe_config->dpll_hw_state.dpll_md =
+		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
+}
+
+static void chv_compute_dpll(struct intel_crtc *crtc,
+			     struct intel_crtc_state *pipe_config)
+{
+	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
+		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
+		DPLL_VCO_ENABLE;
+	if (crtc->pipe != PIPE_A)
+		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	dpll_md = (pipe_config->pixel_multiplier - 1)
-		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-	pipe_config->dpll_hw_state.dpll_md = dpll_md;
+	pipe_config->dpll_hw_state.dpll_md =
+		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
 }
 
 static void vlv_prepare_pll(struct intel_crtc *crtc,
@@ -7351,19 +7351,6 @@  static void vlv_prepare_pll(struct intel_crtc *crtc,
 	mutex_unlock(&dev_priv->sb_lock);
 }
 
-static void chv_compute_dpll(struct intel_crtc *crtc,
-			     struct intel_crtc_state *pipe_config)
-{
-	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLK_CHV |
-		DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
-		DPLL_VCO_ENABLE;
-	if (crtc->pipe != PIPE_A)
-		pipe_config->dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
-
-	pipe_config->dpll_hw_state.dpll_md =
-		(pipe_config->pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
-}
-
 static void chv_prepare_pll(struct intel_crtc *crtc,
 			    const struct intel_crtc_state *pipe_config)
 {