[PATCH 1/5] drm/i915/bxt: mask off the DPLL state checker bits we don't program

[PATCH 1/5] drm/i915/bxt: mask off the DPLL state checker bits we don't program

[Imre Deak]

For the purpose of state checking we only care about the DPLL HW flags
that we actually program, so mask off the ones that we don't.

This fixes one set of DPLL state check failures.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9ae297a..bdc5677 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2479,13 +2479,32 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 		return false;
 
 	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
+	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
+
 	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
+	hw_state->pll0 &= PORT_PLL_M2_MASK;
+
 	hw_state->pll1 = I915_READ(BXT_PORT_PLL(port, 1));
+	hw_state->pll1 &= PORT_PLL_N_MASK;
+
 	hw_state->pll2 = I915_READ(BXT_PORT_PLL(port, 2));
+	hw_state->pll2 &= PORT_PLL_M2_FRAC_MASK;
+
 	hw_state->pll3 = I915_READ(BXT_PORT_PLL(port, 3));
+	hw_state->pll3 &= PORT_PLL_M2_FRAC_ENABLE;
+
 	hw_state->pll6 = I915_READ(BXT_PORT_PLL(port, 6));
+	hw_state->pll6 &= PORT_PLL_PROP_COEFF_MASK |
+			  PORT_PLL_INT_COEFF_MASK |
+			  PORT_PLL_GAIN_CTL_MASK;
+
 	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
+	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
+
 	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
+	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
+			   PORT_PLL_DCO_AMP_MASK;
+
 	/*
 	 * While we write to the group register to program all lanes at once we
 	 * can read only lane registers. We configure all lanes the same way, so
@@ -2496,6 +2515,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 		DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n",
 				 hw_state->pcsdw12,
 				 I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
+	hw_state->pcsdw12 &= LANE_STAGGER_MASK | LANESTAGGER_STRAP_OVRD;
 
 	return true;
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

[Imre Deak]

Although we have a fixed setting for the PLL9 and EBB4 registers, it
still makes sense to check them together with the rest of PLL registers.

While at it also remove a redundant comment about 10 bit clock enabling.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
 drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
 drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++++++++---
 drivers/gpu/drm/i915/intel_display.c |  6 ++++--
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 491ef0c..bf235ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,7 +366,8 @@ struct intel_dpll_hw_state {
 	uint32_t cfgcr1, cfgcr2;
 
 	/* bxt */
-	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pll10, pcsdw12;
+	uint32_t ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10,
+		 pcsdw12;
 };
 
 struct intel_shared_dpll_config {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4bbc85a..bba0691 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1207,7 +1207,8 @@ enum skl_disp_power_wells {
 /* PORT_PLL_8_A */
 #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
 /* PORT_PLL_9_A */
-#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
+#define  PORT_PLL_LOCK_THRESHOLD_SHIFT	1
+#define  PORT_PLL_LOCK_THRESHOLD_MASK	(0x7 << PORT_PLL_LOCK_THRESHOLD_SHIFT)
 /* PORT_PLL_10_A */
 #define  PORT_PLL_DCO_AMP_OVR_EN_H	(1<<27)
 #define  PORT_PLL_DCO_AMP_MASK		0x3c00
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index bdc5677..ca970ba 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1476,11 +1476,15 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 
 	crtc_state->dpll_hw_state.pll8 = targ_cnt;
 
+	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
+
 	if (dcoampovr_en_h)
 		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN_H;
 
 	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(dco_amp);
 
+	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
+
 	crtc_state->dpll_hw_state.pcsdw12 =
 		LANESTAGGER_STRAP_OVRD | lanestagger;
 
@@ -2414,7 +2418,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 
 	temp = I915_READ(BXT_PORT_PLL(port, 9));
 	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
-	temp |= (5 << 1);
+	temp |= pll->config.hw_state.pll9;
 	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
 
 	temp = I915_READ(BXT_PORT_PLL(port, 10));
@@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
 	temp |= PORT_PLL_RECALIBRATE;
 	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
-	/* Enable 10 bit clock */
-	temp |= PORT_PLL_10BIT_CLK_ENABLE;
+	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
+	temp |= pll->config.hw_state.ebb4;
 	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
 
 	/* Enable PLL */
@@ -2481,6 +2485,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
 	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
 
+	hw_state->ebb4 = I915_READ(BXT_PORT_PLL_EBB_4(port));
+	hw_state->ebb4 &= PORT_PLL_10BIT_CLK_ENABLE;
+
 	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
 	hw_state->pll0 &= PORT_PLL_M2_MASK;
 
@@ -2501,6 +2508,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
 	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
 
+	hw_state->pll9 = I915_READ(BXT_PORT_PLL(port, 9));
+	hw_state->pll9 &= PORT_PLL_LOCK_THRESHOLD_MASK;
+
 	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
 	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
 			   PORT_PLL_DCO_AMP_MASK;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9149410..6f79680 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11905,17 +11905,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
 
 	if (IS_BROXTON(dev)) {
-		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, "
+		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
 			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
-			      "pll6: 0x%x, pll8: 0x%x, pcsdw12: 0x%x\n",
+			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
 			      pipe_config->ddi_pll_sel,
 			      pipe_config->dpll_hw_state.ebb0,
+			      pipe_config->dpll_hw_state.ebb4,
 			      pipe_config->dpll_hw_state.pll0,
 			      pipe_config->dpll_hw_state.pll1,
 			      pipe_config->dpll_hw_state.pll2,
 			      pipe_config->dpll_hw_state.pll3,
 			      pipe_config->dpll_hw_state.pll6,
 			      pipe_config->dpll_hw_state.pll8,
+			      pipe_config->dpll_hw_state.pll9,
 			      pipe_config->dpll_hw_state.pcsdw12);
 	} else if (IS_SKYLAKE(dev)) {
 		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

[Jindal, Sonika]

On 6/18/2015 7:55 PM, Imre Deak wrote:
> Although we have a fixed setting for the PLL9 and EBB4 registers, it
> still makes sense to check them together with the rest of PLL registers.
>
> While at it also remove a redundant comment about 10 bit clock enabling.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
>   drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
>   drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++++++++---
>   drivers/gpu/drm/i915/intel_display.c |  6 ++++--
>   4 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 491ef0c..bf235ff 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -366,7 +366,8 @@ struct intel_dpll_hw_state {
>   	uint32_t cfgcr1, cfgcr2;
>
>   	/* bxt */
> -	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pll10, pcsdw12;
> +	uint32_t ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10,
> +		 pcsdw12;
>   };
>
>   struct intel_shared_dpll_config {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4bbc85a..bba0691 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1207,7 +1207,8 @@ enum skl_disp_power_wells {
>   /* PORT_PLL_8_A */
>   #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
>   /* PORT_PLL_9_A */
> -#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
> +#define  PORT_PLL_LOCK_THRESHOLD_SHIFT	1
> +#define  PORT_PLL_LOCK_THRESHOLD_MASK	(0x7 << PORT_PLL_LOCK_THRESHOLD_SHIFT)
>   /* PORT_PLL_10_A */
>   #define  PORT_PLL_DCO_AMP_OVR_EN_H	(1<<27)
>   #define  PORT_PLL_DCO_AMP_MASK		0x3c00
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index bdc5677..ca970ba 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1476,11 +1476,15 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>
>   	crtc_state->dpll_hw_state.pll8 = targ_cnt;
>
> +	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
> +
>   	if (dcoampovr_en_h)
>   		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN_H;
>
>   	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(dco_amp);
>
> +	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
You can add " | PORT_PLL_RECALIBRATE" and check for this one as well in 
bxt_ddi_pll_get_hw_state.

> +
>   	crtc_state->dpll_hw_state.pcsdw12 =
>   		LANESTAGGER_STRAP_OVRD | lanestagger;
>
> @@ -2414,7 +2418,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>
>   	temp = I915_READ(BXT_PORT_PLL(port, 9));
>   	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
> -	temp |= (5 << 1);
> +	temp |= pll->config.hw_state.pll9;
>   	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
>
>   	temp = I915_READ(BXT_PORT_PLL(port, 10));
> @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>   	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
>   	temp |= PORT_PLL_RECALIBRATE;
>   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> -	/* Enable 10 bit clock */
I think it is OK to keep this comment here because all the steps are 
mentioned in comments, even "disable 10 bit clock".

> -	temp |= PORT_PLL_10BIT_CLK_ENABLE;
> +	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
> +	temp |= pll->config.hw_state.ebb4;
>   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
>
>   	/* Enable PLL */
> @@ -2481,6 +2485,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>   	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
>   	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
>
> +	hw_state->ebb4 = I915_READ(BXT_PORT_PLL_EBB_4(port));
> +	hw_state->ebb4 &= PORT_PLL_10BIT_CLK_ENABLE;
> +
>   	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
>   	hw_state->pll0 &= PORT_PLL_M2_MASK;
>
> @@ -2501,6 +2508,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>   	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
>   	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
>
> +	hw_state->pll9 = I915_READ(BXT_PORT_PLL(port, 9));
> +	hw_state->pll9 &= PORT_PLL_LOCK_THRESHOLD_MASK;
> +
>   	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
>   	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
>   			   PORT_PLL_DCO_AMP_MASK;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9149410..6f79680 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11905,17 +11905,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>   	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
>
>   	if (IS_BROXTON(dev)) {
> -		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, "
> +		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
>   			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> -			      "pll6: 0x%x, pll8: 0x%x, pcsdw12: 0x%x\n",
> +			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
>   			      pipe_config->ddi_pll_sel,
>   			      pipe_config->dpll_hw_state.ebb0,
> +			      pipe_config->dpll_hw_state.ebb4,
>   			      pipe_config->dpll_hw_state.pll0,
>   			      pipe_config->dpll_hw_state.pll1,
>   			      pipe_config->dpll_hw_state.pll2,
>   			      pipe_config->dpll_hw_state.pll3,
>   			      pipe_config->dpll_hw_state.pll6,
>   			      pipe_config->dpll_hw_state.pll8,
> +			      pipe_config->dpll_hw_state.pll9,
>   			      pipe_config->dpll_hw_state.pcsdw12);
>   	} else if (IS_SKYLAKE(dev)) {
>   		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

[Imre Deak]

On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote:
> 
> On 6/18/2015 7:55 PM, Imre Deak wrote:
> > Although we have a fixed setting for the PLL9 and EBB4 registers, it
> > still makes sense to check them together with the rest of PLL registers.
> >
> > While at it also remove a redundant comment about 10 bit clock enabling.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
> >   drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
> >   drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++++++++---
> >   drivers/gpu/drm/i915/intel_display.c |  6 ++++--
> >   4 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 491ef0c..bf235ff 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -366,7 +366,8 @@ struct intel_dpll_hw_state {
> >   	uint32_t cfgcr1, cfgcr2;
> >
> >   	/* bxt */
> > -	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pll10, pcsdw12;
> > +	uint32_t ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10,
> > +		 pcsdw12;
> >   };
> >
> >   struct intel_shared_dpll_config {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 4bbc85a..bba0691 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1207,7 +1207,8 @@ enum skl_disp_power_wells {
> >   /* PORT_PLL_8_A */
> >   #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
> >   /* PORT_PLL_9_A */
> > -#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
> > +#define  PORT_PLL_LOCK_THRESHOLD_SHIFT	1
> > +#define  PORT_PLL_LOCK_THRESHOLD_MASK	(0x7 << PORT_PLL_LOCK_THRESHOLD_SHIFT)
> >   /* PORT_PLL_10_A */
> >   #define  PORT_PLL_DCO_AMP_OVR_EN_H	(1<<27)
> >   #define  PORT_PLL_DCO_AMP_MASK		0x3c00
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index bdc5677..ca970ba 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1476,11 +1476,15 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
> >
> >   	crtc_state->dpll_hw_state.pll8 = targ_cnt;
> >
> > +	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
> > +
> >   	if (dcoampovr_en_h)
> >   		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN_H;
> >
> >   	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(dco_amp);
> >
> > +	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
> You can add " | PORT_PLL_RECALIBRATE" and check for this one as well in 
> bxt_ddi_pll_get_hw_state.

I'm not sure. This bit is cleared after the PLL gets enabled, so we'd
have a mismatch when reading out the HW state. I'd regard this as a
control bit that's not part of the programmed state.

> 
> > +
> >   	crtc_state->dpll_hw_state.pcsdw12 =
> >   		LANESTAGGER_STRAP_OVRD | lanestagger;
> >
> > @@ -2414,7 +2418,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >
> >   	temp = I915_READ(BXT_PORT_PLL(port, 9));
> >   	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
> > -	temp |= (5 << 1);
> > +	temp |= pll->config.hw_state.pll9;
> >   	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
> >
> >   	temp = I915_READ(BXT_PORT_PLL(port, 10));
> > @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> >   	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
> >   	temp |= PORT_PLL_RECALIBRATE;
> >   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> > -	/* Enable 10 bit clock */
> I think it is OK to keep this comment here because all the steps are 
> mentioned in comments, even "disable 10 bit clock".

Yea, but some of those comments just say what is really obvious from the
code right afterwards.

> 
> > -	temp |= PORT_PLL_10BIT_CLK_ENABLE;
> > +	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
> > +	temp |= pll->config.hw_state.ebb4;
> >   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> >
> >   	/* Enable PLL */
> > @@ -2481,6 +2485,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >   	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
> >   	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
> >
> > +	hw_state->ebb4 = I915_READ(BXT_PORT_PLL_EBB_4(port));
> > +	hw_state->ebb4 &= PORT_PLL_10BIT_CLK_ENABLE;
> > +
> >   	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
> >   	hw_state->pll0 &= PORT_PLL_M2_MASK;
> >
> > @@ -2501,6 +2508,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >   	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
> >   	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
> >
> > +	hw_state->pll9 = I915_READ(BXT_PORT_PLL(port, 9));
> > +	hw_state->pll9 &= PORT_PLL_LOCK_THRESHOLD_MASK;
> > +
> >   	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
> >   	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
> >   			   PORT_PLL_DCO_AMP_MASK;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9149410..6f79680 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11905,17 +11905,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
> >   	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
> >
> >   	if (IS_BROXTON(dev)) {
> > -		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, "
> > +		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> >   			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> > -			      "pll6: 0x%x, pll8: 0x%x, pcsdw12: 0x%x\n",
> > +			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
> >   			      pipe_config->ddi_pll_sel,
> >   			      pipe_config->dpll_hw_state.ebb0,
> > +			      pipe_config->dpll_hw_state.ebb4,
> >   			      pipe_config->dpll_hw_state.pll0,
> >   			      pipe_config->dpll_hw_state.pll1,
> >   			      pipe_config->dpll_hw_state.pll2,
> >   			      pipe_config->dpll_hw_state.pll3,
> >   			      pipe_config->dpll_hw_state.pll6,
> >   			      pipe_config->dpll_hw_state.pll8,
> > +			      pipe_config->dpll_hw_state.pll9,
> >   			      pipe_config->dpll_hw_state.pcsdw12);
> >   	} else if (IS_SKYLAKE(dev)) {
> >   		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
> >


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

[Daniel Vetter]

On Wed, Jun 24, 2015 at 01:19:10PM +0300, Imre Deak wrote:
> On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote:
> > On 6/18/2015 7:55 PM, Imre Deak wrote:
> > > @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > >   	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
> > >   	temp |= PORT_PLL_RECALIBRATE;
> > >   	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
> > > -	/* Enable 10 bit clock */
> > I think it is OK to keep this comment here because all the steps are 
> > mentioned in comments, even "disable 10 bit clock".
> 
> Yea, but some of those comments just say what is really obvious from the
> code right afterwards.

Concur with Imre here, comments shouldn't ever explain what the code does,
but why. If you need to explain what the code does in comments, then it
needs to be refactored (helper function with clear name extracted, better
naming of variables/defines, ...).

Imo almost all the comments in this code should be remove because they're
obvious.
-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

Re: [PATCH 2/5] drm/i915/bxt: add missing DDI PLL registers to the state checking

[Jindal, Sonika]

Looks good to me:
Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>

On 6/24/2015 3:49 PM, Imre Deak wrote:
> On ke, 2015-06-24 at 15:37 +0530, Jindal, Sonika wrote:
>>
>> On 6/18/2015 7:55 PM, Imre Deak wrote:
>>> Although we have a fixed setting for the PLL9 and EBB4 registers, it
>>> still makes sense to check them together with the rest of PLL registers.
>>>
>>> While at it also remove a redundant comment about 10 bit clock enabling.
>>>
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_drv.h      |  3 ++-
>>>    drivers/gpu/drm/i915/i915_reg.h      |  3 ++-
>>>    drivers/gpu/drm/i915/intel_ddi.c     | 16 +++++++++++++---
>>>    drivers/gpu/drm/i915/intel_display.c |  6 ++++--
>>>    4 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 491ef0c..bf235ff 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -366,7 +366,8 @@ struct intel_dpll_hw_state {
>>>    	uint32_t cfgcr1, cfgcr2;
>>>
>>>    	/* bxt */
>>> -	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pll10, pcsdw12;
>>> +	uint32_t ebb0, ebb4, pll0, pll1, pll2, pll3, pll6, pll8, pll9, pll10,
>>> +		 pcsdw12;
>>>    };
>>>
>>>    struct intel_shared_dpll_config {
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 4bbc85a..bba0691 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -1207,7 +1207,8 @@ enum skl_disp_power_wells {
>>>    /* PORT_PLL_8_A */
>>>    #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
>>>    /* PORT_PLL_9_A */
>>> -#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
>>> +#define  PORT_PLL_LOCK_THRESHOLD_SHIFT	1
>>> +#define  PORT_PLL_LOCK_THRESHOLD_MASK	(0x7 << PORT_PLL_LOCK_THRESHOLD_SHIFT)
>>>    /* PORT_PLL_10_A */
>>>    #define  PORT_PLL_DCO_AMP_OVR_EN_H	(1<<27)
>>>    #define  PORT_PLL_DCO_AMP_MASK		0x3c00
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index bdc5677..ca970ba 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1476,11 +1476,15 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>>>
>>>    	crtc_state->dpll_hw_state.pll8 = targ_cnt;
>>>
>>> +	crtc_state->dpll_hw_state.pll9 = 5 << PORT_PLL_LOCK_THRESHOLD_SHIFT;
>>> +
>>>    	if (dcoampovr_en_h)
>>>    		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN_H;
>>>
>>>    	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(dco_amp);
>>>
>>> +	crtc_state->dpll_hw_state.ebb4 = PORT_PLL_10BIT_CLK_ENABLE;
>> You can add " | PORT_PLL_RECALIBRATE" and check for this one as well in
>> bxt_ddi_pll_get_hw_state.
>
> I'm not sure. This bit is cleared after the PLL gets enabled, so we'd
> have a mismatch when reading out the HW state. I'd regard this as a
> control bit that's not part of the programmed state.
>
Hmm correct, checked the description now..
>>
>>> +
>>>    	crtc_state->dpll_hw_state.pcsdw12 =
>>>    		LANESTAGGER_STRAP_OVRD | lanestagger;
>>>
>>> @@ -2414,7 +2418,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>>
>>>    	temp = I915_READ(BXT_PORT_PLL(port, 9));
>>>    	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
>>> -	temp |= (5 << 1);
>>> +	temp |= pll->config.hw_state.pll9;
>>>    	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
>>>
>>>    	temp = I915_READ(BXT_PORT_PLL(port, 10));
>>> @@ -2427,8 +2431,8 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>>>    	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
>>>    	temp |= PORT_PLL_RECALIBRATE;
>>>    	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
>>> -	/* Enable 10 bit clock */
>> I think it is OK to keep this comment here because all the steps are
>> mentioned in comments, even "disable 10 bit clock".
>
> Yea, but some of those comments just say what is really obvious from the
> code right afterwards.
>
Yes :). Maybe clean up the other comments in the next attempt.

>>
>>> -	temp |= PORT_PLL_10BIT_CLK_ENABLE;
>>> +	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
>>> +	temp |= pll->config.hw_state.ebb4;
>>>    	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
>>>
>>>    	/* Enable PLL */
>>> @@ -2481,6 +2485,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>>    	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
>>>    	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
>>>
>>> +	hw_state->ebb4 = I915_READ(BXT_PORT_PLL_EBB_4(port));
>>> +	hw_state->ebb4 &= PORT_PLL_10BIT_CLK_ENABLE;
>>> +
>>>    	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
>>>    	hw_state->pll0 &= PORT_PLL_M2_MASK;
>>>
>>> @@ -2501,6 +2508,9 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>>    	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
>>>    	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
>>>
>>> +	hw_state->pll9 = I915_READ(BXT_PORT_PLL(port, 9));
>>> +	hw_state->pll9 &= PORT_PLL_LOCK_THRESHOLD_MASK;
>>> +
>>>    	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
>>>    	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
>>>    			   PORT_PLL_DCO_AMP_MASK;
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 9149410..6f79680 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -11905,17 +11905,19 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>>    	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
>>>
>>>    	if (IS_BROXTON(dev)) {
>>> -		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, "
>>> +		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
>>>    			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
>>> -			      "pll6: 0x%x, pll8: 0x%x, pcsdw12: 0x%x\n",
>>> +			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
>>>    			      pipe_config->ddi_pll_sel,
>>>    			      pipe_config->dpll_hw_state.ebb0,
>>> +			      pipe_config->dpll_hw_state.ebb4,
>>>    			      pipe_config->dpll_hw_state.pll0,
>>>    			      pipe_config->dpll_hw_state.pll1,
>>>    			      pipe_config->dpll_hw_state.pll2,
>>>    			      pipe_config->dpll_hw_state.pll3,
>>>    			      pipe_config->dpll_hw_state.pll6,
>>>    			      pipe_config->dpll_hw_state.pll8,
>>> +			      pipe_config->dpll_hw_state.pll9,
>>>    			      pipe_config->dpll_hw_state.pcsdw12);
>>>    	} else if (IS_SKYLAKE(dev)) {
>>>    		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
>>>
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915/bxt: add PLL10 to the PLL state dumper

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6f79680..0e5c613 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11907,7 +11907,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	if (IS_BROXTON(dev)) {
 		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
 			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
-			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
+			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
 			      pipe_config->ddi_pll_sel,
 			      pipe_config->dpll_hw_state.ebb0,
 			      pipe_config->dpll_hw_state.ebb4,
@@ -11918,6 +11918,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 			      pipe_config->dpll_hw_state.pll6,
 			      pipe_config->dpll_hw_state.pll8,
 			      pipe_config->dpll_hw_state.pll9,
+			      pipe_config->dpll_hw_state.pll10,
 			      pipe_config->dpll_hw_state.pcsdw12);
 	} else if (IS_SKYLAKE(dev)) {
 		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915/bxt: add PLL10 to the PLL state dumper

[Jindal, Sonika]

Looks good to me:
Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>

On 6/18/2015 7:55 PM, Imre Deak wrote:
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6f79680..0e5c613 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11907,7 +11907,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>   	if (IS_BROXTON(dev)) {
>   		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
>   			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> -			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pcsdw12: 0x%x\n",
> +			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
>   			      pipe_config->ddi_pll_sel,
>   			      pipe_config->dpll_hw_state.ebb0,
>   			      pipe_config->dpll_hw_state.ebb4,
> @@ -11918,6 +11918,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>   			      pipe_config->dpll_hw_state.pll6,
>   			      pipe_config->dpll_hw_state.pll8,
>   			      pipe_config->dpll_hw_state.pll9,
> +			      pipe_config->dpll_hw_state.pll10,
>   			      pipe_config->dpll_hw_state.pcsdw12);
>   	} else if (IS_SKYLAKE(dev)) {
>   		DRM_DEBUG_KMS("ddi_pll_sel: %u; dpll_hw_state: "
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock

[Imre Deak]

This functionality will be needed by the next patch adding HW readout
support for DDI ports on BXT, so factor it out.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0e5c613..6cf2a15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7993,6 +7993,14 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
 			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
 }
 
+int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock)
+{
+	chv_clock(refclk, pll_clock);
+
+	/* clock.dot is the fast clock */
+	return pll_clock->dot / 5;
+}
+
 static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 			       struct intel_crtc_state *pipe_config)
 {
@@ -8017,10 +8025,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
 	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
 
-	vlv_clock(refclk, &clock);
-
-	/* clock.dot is the fast clock */
-	pipe_config->port_clock = clock.dot / 5;
+	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
 }
 
 static void
@@ -8116,10 +8121,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
 	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
 	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
 
-	chv_clock(refclk, &clock);
-
-	/* clock.dot is the fast clock */
-	pipe_config->port_clock = clock.dot / 5;
+	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
 }
 
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcafefc..95e14bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1139,6 +1139,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
 				int dotclock);
 bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
 			intel_clock_t *best_clock);
+int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock);
+
 bool intel_crtc_active(struct drm_crtc *crtc);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock

[Ville Syrjälä]

On Thu, Jun 18, 2015 at 05:25:56PM +0300, Imre Deak wrote:
> This functionality will be needed by the next patch adding HW readout
> support for DDI ports on BXT, so factor it out.
> 
> No functional change.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0e5c613..6cf2a15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7993,6 +7993,14 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
>  			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
>  }
>  
> +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock)

It doesn't work for VLV, so the name is misleading.

> +{
> +	chv_clock(refclk, pll_clock);
> +
> +	/* clock.dot is the fast clock */
> +	return pll_clock->dot / 5;
> +}

Maybe just make chv_clock() return the divided down clock directtly
instead of adding this extra wrapper.

Though in that case I'd prefer if you change all the vlv_clock(),
i9xx_clock()... functions to do the same, just to keep the code
as similar as possible between platforms.

> +
>  static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  			       struct intel_crtc_state *pipe_config)
>  {
> @@ -8017,10 +8025,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
>  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
>  
> -	vlv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
>  }
>  
>  static void
> @@ -8116,10 +8121,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
>  	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
>  	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
>  
> -	chv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
>  }
>  
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefc..95e14bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1139,6 +1139,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
>  				int dotclock);
>  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
>  			intel_clock_t *best_clock);
> +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock);
> +
>  bool intel_crtc_active(struct drm_crtc *crtc);
>  void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock

[Imre Deak]

On ma, 2015-06-22 at 16:33 +0300, Ville Syrjälä wrote:
> On Thu, Jun 18, 2015 at 05:25:56PM +0300, Imre Deak wrote:
> > This functionality will be needed by the next patch adding HW readout
> > support for DDI ports on BXT, so factor it out.
> > 
> > No functional change.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  2 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0e5c613..6cf2a15 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7993,6 +7993,14 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> >  			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
> >  }
> >  
> > +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock)
> 
> It doesn't work for VLV, so the name is misleading.

Oops, completely missed that. It would also break on VLV after this
change, since I do use it also for VLV.

> > +{
> > +	chv_clock(refclk, pll_clock);
> > +
> > +	/* clock.dot is the fast clock */
> > +	return pll_clock->dot / 5;
> > +}
> 
> Maybe just make chv_clock() return the divided down clock directtly
> instead of adding this extra wrapper.
> 
> Though in that case I'd prefer if you change all the vlv_clock(),
> i9xx_clock()... functions to do the same, just to keep the code
> as similar as possible between platforms.

Ok, will look into this.

> > +
> >  static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  			       struct intel_crtc_state *pipe_config)
> >  {
> > @@ -8017,10 +8025,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> >  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> >  
> > -	vlv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
> >  }
> >  
> >  static void
> > @@ -8116,10 +8121,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
> >  	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
> >  	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
> >  
> > -	chv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
> >  }
> >  
> >  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bcafefc..95e14bb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1139,6 +1139,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
> >  				int dotclock);
> >  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> >  			intel_clock_t *best_clock);
> > +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock);
> > +
> >  bool intel_crtc_active(struct drm_crtc *crtc);
> >  void hsw_enable_ips(struct intel_crtc *crtc);
> >  void hsw_disable_ips(struct intel_crtc *crtc);
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/5] drm/i915/vlv: move the vlv PLL helper next to its platform counterparts

[Imre Deak]

Move the helper next to the PLL helpers of the other platforms for
clarity.

No functional change.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b030ce4..fb7fd5f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -418,16 +418,6 @@ static const intel_limit_t intel_limits_bxt = {
 	.p2 = { .p2_slow = 1, .p2_fast = 20 },
 };
 
-static void vlv_clock(int refclk, intel_clock_t *clock)
-{
-	clock->m = clock->m1 * clock->m2;
-	clock->p = clock->p1 * clock->p2;
-	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return;
-	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
-	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
-}
-
 static bool
 needs_modeset(struct drm_crtc_state *state)
 {
@@ -589,6 +579,16 @@ static void i9xx_clock(int refclk, intel_clock_t *clock)
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
 }
 
+static void vlv_clock(int refclk, intel_clock_t *clock)
+{
+	clock->m = clock->m1 * clock->m2;
+	clock->p = clock->p1 * clock->p2;
+	if (WARN_ON(clock->n == 0 || clock->p == 0))
+		return;
+	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
+	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
+}
+
 static void chv_clock(int refclk, intel_clock_t *clock)
 {
 	clock->m = clock->m1 * clock->m2;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/5] drm/i915/vlv: move the vlv PLL helper next to its platform counterparts

[Jindal, Sonika]

Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>

On 6/23/2015 2:05 AM, Imre Deak wrote:
> Move the helper next to the PLL helpers of the other platforms for
> clarity.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b030ce4..fb7fd5f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -418,16 +418,6 @@ static const intel_limit_t intel_limits_bxt = {
>   	.p2 = { .p2_slow = 1, .p2_fast = 20 },
>   };
>
> -static void vlv_clock(int refclk, intel_clock_t *clock)
> -{
> -	clock->m = clock->m1 * clock->m2;
> -	clock->p = clock->p1 * clock->p2;
> -	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return;
> -	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> -	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> -}
> -
>   static bool
>   needs_modeset(struct drm_crtc_state *state)
>   {
> @@ -589,6 +579,16 @@ static void i9xx_clock(int refclk, intel_clock_t *clock)
>   	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
>   }
>
> +static void vlv_clock(int refclk, intel_clock_t *clock)
> +{
> +	clock->m = clock->m1 * clock->m2;
> +	clock->p = clock->p1 * clock->p2;
> +	if (WARN_ON(clock->n == 0 || clock->p == 0))
> +		return;
> +	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> +	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> +}
> +
>   static void chv_clock(int refclk, intel_clock_t *clock)
>   {
>   	clock->m = clock->m1 * clock->m2;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4.1/5] drm/i915: calculate the port clock rate along with other PLL params

[Imre Deak]

Depending on the platform the port clock fed to the pipe can be the PLL's
post-divided fast clock rate or a /5 divided version of it. To make this
more obvious across the platforms calculate this port clock along with
the rest of the PLL parameters.

This is also needed by the next patch where we can reuse the CHV helper
for the BXT PLL HW readout code; so export the corresponding helper.

While at it also add a more descriptive name to the helpers and a
comment explaining what's being calculated.

No functional change.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 61 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 2 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb7fd5f..a11ce7fa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -553,15 +553,25 @@ intel_limit(struct intel_crtc_state *crtc_state, int refclk)
 	return limit;
 }
 
+/*
+ * Platform specific helpers to calculate the port PLL loopback- (clock.m),
+ * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
+ * (clock.dot) clock rates. This fast dot clock is fed to the port's IO logic.
+ * The helpers' return value is the rate of the clock that is fed to the
+ * display engine's pipe which can be the above fast dot clock rate or a
+ * divided-down version of it.
+ */
 /* m1 is reserved as 0 in Pineview, n is a ring counter */
-static void pineview_clock(int refclk, intel_clock_t *clock)
+static int pnv_calc_dpll_params(int refclk, intel_clock_t *clock)
 {
 	clock->m = clock->m2 + 2;
 	clock->p = clock->p1 * clock->p2;
 	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return;
+		return 0;
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
+
+	return clock->dot;
 }
 
 static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
@@ -569,35 +579,41 @@ static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
 	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
 }
 
-static void i9xx_clock(int refclk, intel_clock_t *clock)
+static int i9xx_calc_dpll_params(int refclk, intel_clock_t *clock)
 {
 	clock->m = i9xx_dpll_compute_m(clock);
 	clock->p = clock->p1 * clock->p2;
 	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
-		return;
+		return 0;
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
+
+	return clock->dot;
 }
 
-static void vlv_clock(int refclk, intel_clock_t *clock)
+static int vlv_calc_dpll_params(int refclk, intel_clock_t *clock)
 {
 	clock->m = clock->m1 * clock->m2;
 	clock->p = clock->p1 * clock->p2;
 	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return;
+		return 0;
 	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
+
+	return clock->dot / 5;
 }
 
-static void chv_clock(int refclk, intel_clock_t *clock)
+int chv_calc_dpll_params(int refclk, intel_clock_t *clock)
 {
 	clock->m = clock->m1 * clock->m2;
 	clock->p = clock->p1 * clock->p2;
 	if (WARN_ON(clock->n == 0 || clock->p == 0))
-		return;
+		return 0;
 	clock->vco = DIV_ROUND_CLOSEST_ULL((uint64_t)refclk * clock->m,
 			clock->n << 22);
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
+
+	return clock->dot / 5;
 }
 
 #define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while (0)
@@ -692,7 +708,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit,
 					clock.p1 <= limit->p1.max; clock.p1++) {
 					int this_err;
 
-					i9xx_clock(refclk, &clock);
+					i9xx_calc_dpll_params(refclk, &clock);
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
@@ -737,7 +753,7 @@ pnv_find_best_dpll(const intel_limit_t *limit,
 					clock.p1 <= limit->p1.max; clock.p1++) {
 					int this_err;
 
-					pineview_clock(refclk, &clock);
+					pnv_calc_dpll_params(refclk, &clock);
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
@@ -787,7 +803,7 @@ g4x_find_best_dpll(const intel_limit_t *limit,
 				     clock.p1 >= limit->p1.min; clock.p1--) {
 					int this_err;
 
-					i9xx_clock(refclk, &clock);
+					i9xx_calc_dpll_params(refclk, &clock);
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
@@ -877,7 +893,7 @@ vlv_find_best_dpll(const intel_limit_t *limit,
 					clock.m2 = DIV_ROUND_CLOSEST(target * clock.p * clock.n,
 								     refclk * clock.m1);
 
-					vlv_clock(refclk, &clock);
+					vlv_calc_dpll_params(refclk, &clock);
 
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
@@ -940,7 +956,7 @@ chv_find_best_dpll(const intel_limit_t *limit,
 
 			clock.m2 = m2;
 
-			chv_clock(refclk, &clock);
+			chv_calc_dpll_params(refclk, &clock);
 
 			if (!intel_PLL_is_valid(dev, limit, &clock))
 				continue;
@@ -7926,10 +7942,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
 	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
 
-	vlv_clock(refclk, &clock);
-
-	/* clock.dot is the fast clock */
-	pipe_config->port_clock = clock.dot / 5;
+	pipe_config->port_clock = vlv_calc_dpll_params(refclk, &clock);
 }
 
 static void
@@ -8025,10 +8038,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
 	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
 	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
 
-	chv_clock(refclk, &clock);
-
-	/* clock.dot is the fast clock */
-	pipe_config->port_clock = clock.dot / 5;
+	pipe_config->port_clock = chv_calc_dpll_params(refclk, &clock);
 }
 
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
@@ -10481,6 +10491,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 	u32 dpll = pipe_config->dpll_hw_state.dpll;
 	u32 fp;
 	intel_clock_t clock;
+	int port_clock;
 	int refclk = i9xx_pll_refclk(dev, pipe_config);
 
 	if ((dpll & DISPLAY_RATE_SELECT_FPA1) == 0)
@@ -10521,9 +10532,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 		}
 
 		if (IS_PINEVIEW(dev))
-			pineview_clock(refclk, &clock);
+			port_clock = pnv_calc_dpll_params(refclk, &clock);
 		else
-			i9xx_clock(refclk, &clock);
+			port_clock = i9xx_calc_dpll_params(refclk, &clock);
 	} else {
 		u32 lvds = IS_I830(dev) ? 0 : I915_READ(LVDS);
 		bool is_lvds = (pipe == 1) && (lvds & LVDS_PORT_EN);
@@ -10549,7 +10560,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 				clock.p2 = 2;
 		}
 
-		i9xx_clock(refclk, &clock);
+		port_clock = i9xx_calc_dpll_params(refclk, &clock);
 	}
 
 	/*
@@ -10557,7 +10568,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 	 * port_clock to compute adjusted_mode.crtc_clock in the
 	 * encoder's get_config() function.
 	 */
-	pipe_config->port_clock = clock.dot;
+	pipe_config->port_clock = port_clock;
 }
 
 int intel_dotclock_calculate(int link_freq,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e2174fd..7a5b3bc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1138,6 +1138,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
 				int dotclock);
 bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
 			intel_clock_t *best_clock);
+int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
+
 bool intel_crtc_active(struct drm_crtc *crtc);
 void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4.1/5] drm/i915: calculate the port clock rate along with other PLL params

[Ville Syrjälä]

On Mon, Jun 22, 2015 at 11:35:51PM +0300, Imre Deak wrote:
> Depending on the platform the port clock fed to the pipe can be the PLL's
> post-divided fast clock rate or a /5 divided version of it. To make this
> more obvious across the platforms calculate this port clock along with
> the rest of the PLL parameters.
> 
> This is also needed by the next patch where we can reuse the CHV helper
> for the BXT PLL HW readout code; so export the corresponding helper.
> 
> While at it also add a more descriptive name to the helpers and a
> comment explaining what's being calculated.
> 
> No functional change.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

I'm not sure I entirely like the new name, but the old name wasn't
good either, and I can't think of anything better.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 61 +++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fb7fd5f..a11ce7fa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -553,15 +553,25 @@ intel_limit(struct intel_crtc_state *crtc_state, int refclk)
>  	return limit;
>  }
>  
> +/*
> + * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> + * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> + * (clock.dot) clock rates. This fast dot clock is fed to the port's IO logic.
> + * The helpers' return value is the rate of the clock that is fed to the
> + * display engine's pipe which can be the above fast dot clock rate or a
> + * divided-down version of it.
> + */
>  /* m1 is reserved as 0 in Pineview, n is a ring counter */
> -static void pineview_clock(int refclk, intel_clock_t *clock)
> +static int pnv_calc_dpll_params(int refclk, intel_clock_t *clock)
>  {
>  	clock->m = clock->m2 + 2;
>  	clock->p = clock->p1 * clock->p2;
>  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return;
> +		return 0;
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> +
> +	return clock->dot;
>  }
>  
>  static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> @@ -569,35 +579,41 @@ static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
>  	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
>  }
>  
> -static void i9xx_clock(int refclk, intel_clock_t *clock)
> +static int i9xx_calc_dpll_params(int refclk, intel_clock_t *clock)
>  {
>  	clock->m = i9xx_dpll_compute_m(clock);
>  	clock->p = clock->p1 * clock->p2;
>  	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
> -		return;
> +		return 0;
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> +
> +	return clock->dot;
>  }
>  
> -static void vlv_clock(int refclk, intel_clock_t *clock)
> +static int vlv_calc_dpll_params(int refclk, intel_clock_t *clock)
>  {
>  	clock->m = clock->m1 * clock->m2;
>  	clock->p = clock->p1 * clock->p2;
>  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return;
> +		return 0;
>  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> +
> +	return clock->dot / 5;
>  }
>  
> -static void chv_clock(int refclk, intel_clock_t *clock)
> +int chv_calc_dpll_params(int refclk, intel_clock_t *clock)
>  {
>  	clock->m = clock->m1 * clock->m2;
>  	clock->p = clock->p1 * clock->p2;
>  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> -		return;
> +		return 0;
>  	clock->vco = DIV_ROUND_CLOSEST_ULL((uint64_t)refclk * clock->m,
>  			clock->n << 22);
>  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> +
> +	return clock->dot / 5;
>  }
>  
>  #define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while (0)
> @@ -692,7 +708,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit,
>  					clock.p1 <= limit->p1.max; clock.p1++) {
>  					int this_err;
>  
> -					i9xx_clock(refclk, &clock);
> +					i9xx_calc_dpll_params(refclk, &clock);
>  					if (!intel_PLL_is_valid(dev, limit,
>  								&clock))
>  						continue;
> @@ -737,7 +753,7 @@ pnv_find_best_dpll(const intel_limit_t *limit,
>  					clock.p1 <= limit->p1.max; clock.p1++) {
>  					int this_err;
>  
> -					pineview_clock(refclk, &clock);
> +					pnv_calc_dpll_params(refclk, &clock);
>  					if (!intel_PLL_is_valid(dev, limit,
>  								&clock))
>  						continue;
> @@ -787,7 +803,7 @@ g4x_find_best_dpll(const intel_limit_t *limit,
>  				     clock.p1 >= limit->p1.min; clock.p1--) {
>  					int this_err;
>  
> -					i9xx_clock(refclk, &clock);
> +					i9xx_calc_dpll_params(refclk, &clock);
>  					if (!intel_PLL_is_valid(dev, limit,
>  								&clock))
>  						continue;
> @@ -877,7 +893,7 @@ vlv_find_best_dpll(const intel_limit_t *limit,
>  					clock.m2 = DIV_ROUND_CLOSEST(target * clock.p * clock.n,
>  								     refclk * clock.m1);
>  
> -					vlv_clock(refclk, &clock);
> +					vlv_calc_dpll_params(refclk, &clock);
>  
>  					if (!intel_PLL_is_valid(dev, limit,
>  								&clock))
> @@ -940,7 +956,7 @@ chv_find_best_dpll(const intel_limit_t *limit,
>  
>  			clock.m2 = m2;
>  
> -			chv_clock(refclk, &clock);
> +			chv_calc_dpll_params(refclk, &clock);
>  
>  			if (!intel_PLL_is_valid(dev, limit, &clock))
>  				continue;
> @@ -7926,10 +7942,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
>  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
>  
> -	vlv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = vlv_calc_dpll_params(refclk, &clock);
>  }
>  
>  static void
> @@ -8025,10 +8038,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
>  	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
>  	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
>  
> -	chv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = chv_calc_dpll_params(refclk, &clock);
>  }
>  
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> @@ -10481,6 +10491,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  	u32 dpll = pipe_config->dpll_hw_state.dpll;
>  	u32 fp;
>  	intel_clock_t clock;
> +	int port_clock;
>  	int refclk = i9xx_pll_refclk(dev, pipe_config);
>  
>  	if ((dpll & DISPLAY_RATE_SELECT_FPA1) == 0)
> @@ -10521,9 +10532,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  		}
>  
>  		if (IS_PINEVIEW(dev))
> -			pineview_clock(refclk, &clock);
> +			port_clock = pnv_calc_dpll_params(refclk, &clock);
>  		else
> -			i9xx_clock(refclk, &clock);
> +			port_clock = i9xx_calc_dpll_params(refclk, &clock);
>  	} else {
>  		u32 lvds = IS_I830(dev) ? 0 : I915_READ(LVDS);
>  		bool is_lvds = (pipe == 1) && (lvds & LVDS_PORT_EN);
> @@ -10549,7 +10560,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  				clock.p2 = 2;
>  		}
>  
> -		i9xx_clock(refclk, &clock);
> +		port_clock = i9xx_calc_dpll_params(refclk, &clock);
>  	}
>  
>  	/*
> @@ -10557,7 +10568,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  	 * port_clock to compute adjusted_mode.crtc_clock in the
>  	 * encoder's get_config() function.
>  	 */
> -	pipe_config->port_clock = clock.dot;
> +	pipe_config->port_clock = port_clock;
>  }
>  
>  int intel_dotclock_calculate(int link_freq,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e2174fd..7a5b3bc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1138,6 +1138,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
>  				int dotclock);
>  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
>  			intel_clock_t *best_clock);
> +int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
> +
>  bool intel_crtc_active(struct drm_crtc *crtc);
>  void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
> -- 
> 2.1.4

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

Re: [PATCH v2 4.1/5] drm/i915: calculate the port clock rate along with other PLL params

[Daniel Vetter]

On Wed, Jun 24, 2015 at 03:53:23PM +0300, Ville Syrjälä wrote:
> On Mon, Jun 22, 2015 at 11:35:51PM +0300, Imre Deak wrote:
> > Depending on the platform the port clock fed to the pipe can be the PLL's
> > post-divided fast clock rate or a /5 divided version of it. To make this
> > more obvious across the platforms calculate this port clock along with
> > the rest of the PLL parameters.
> > 
> > This is also needed by the next patch where we can reuse the CHV helper
> > for the BXT PLL HW readout code; so export the corresponding helper.
> > 
> > While at it also add a more descriptive name to the helpers and a
> > comment explaining what's being calculated.
> > 
> > No functional change.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> I'm not sure I entirely like the new name, but the old name wasn't
> good either, and I can't think of anything better.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

All merged to dinq, thanks.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 61 +++++++++++++++++++++---------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >  2 files changed, 38 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fb7fd5f..a11ce7fa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -553,15 +553,25 @@ intel_limit(struct intel_crtc_state *crtc_state, int refclk)
> >  	return limit;
> >  }
> >  
> > +/*
> > + * Platform specific helpers to calculate the port PLL loopback- (clock.m),
> > + * and post-divider (clock.p) values, pre- (clock.vco) and post-divided fast
> > + * (clock.dot) clock rates. This fast dot clock is fed to the port's IO logic.
> > + * The helpers' return value is the rate of the clock that is fed to the
> > + * display engine's pipe which can be the above fast dot clock rate or a
> > + * divided-down version of it.
> > + */
> >  /* m1 is reserved as 0 in Pineview, n is a ring counter */
> > -static void pineview_clock(int refclk, intel_clock_t *clock)
> > +static int pnv_calc_dpll_params(int refclk, intel_clock_t *clock)
> >  {
> >  	clock->m = clock->m2 + 2;
> >  	clock->p = clock->p1 * clock->p2;
> >  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return;
> > +		return 0;
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > +
> > +	return clock->dot;
> >  }
> >  
> >  static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> > @@ -569,35 +579,41 @@ static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> >  	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> >  }
> >  
> > -static void i9xx_clock(int refclk, intel_clock_t *clock)
> > +static int i9xx_calc_dpll_params(int refclk, intel_clock_t *clock)
> >  {
> >  	clock->m = i9xx_dpll_compute_m(clock);
> >  	clock->p = clock->p1 * clock->p2;
> >  	if (WARN_ON(clock->n + 2 == 0 || clock->p == 0))
> > -		return;
> > +		return 0;
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > +
> > +	return clock->dot;
> >  }
> >  
> > -static void vlv_clock(int refclk, intel_clock_t *clock)
> > +static int vlv_calc_dpll_params(int refclk, intel_clock_t *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> >  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return;
> > +		return 0;
> >  	clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > +
> > +	return clock->dot / 5;
> >  }
> >  
> > -static void chv_clock(int refclk, intel_clock_t *clock)
> > +int chv_calc_dpll_params(int refclk, intel_clock_t *clock)
> >  {
> >  	clock->m = clock->m1 * clock->m2;
> >  	clock->p = clock->p1 * clock->p2;
> >  	if (WARN_ON(clock->n == 0 || clock->p == 0))
> > -		return;
> > +		return 0;
> >  	clock->vco = DIV_ROUND_CLOSEST_ULL((uint64_t)refclk * clock->m,
> >  			clock->n << 22);
> >  	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
> > +
> > +	return clock->dot / 5;
> >  }
> >  
> >  #define INTELPllInvalid(s)   do { /* DRM_DEBUG(s); */ return false; } while (0)
> > @@ -692,7 +708,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit,
> >  					clock.p1 <= limit->p1.max; clock.p1++) {
> >  					int this_err;
> >  
> > -					i9xx_clock(refclk, &clock);
> > +					i9xx_calc_dpll_params(refclk, &clock);
> >  					if (!intel_PLL_is_valid(dev, limit,
> >  								&clock))
> >  						continue;
> > @@ -737,7 +753,7 @@ pnv_find_best_dpll(const intel_limit_t *limit,
> >  					clock.p1 <= limit->p1.max; clock.p1++) {
> >  					int this_err;
> >  
> > -					pineview_clock(refclk, &clock);
> > +					pnv_calc_dpll_params(refclk, &clock);
> >  					if (!intel_PLL_is_valid(dev, limit,
> >  								&clock))
> >  						continue;
> > @@ -787,7 +803,7 @@ g4x_find_best_dpll(const intel_limit_t *limit,
> >  				     clock.p1 >= limit->p1.min; clock.p1--) {
> >  					int this_err;
> >  
> > -					i9xx_clock(refclk, &clock);
> > +					i9xx_calc_dpll_params(refclk, &clock);
> >  					if (!intel_PLL_is_valid(dev, limit,
> >  								&clock))
> >  						continue;
> > @@ -877,7 +893,7 @@ vlv_find_best_dpll(const intel_limit_t *limit,
> >  					clock.m2 = DIV_ROUND_CLOSEST(target * clock.p * clock.n,
> >  								     refclk * clock.m1);
> >  
> > -					vlv_clock(refclk, &clock);
> > +					vlv_calc_dpll_params(refclk, &clock);
> >  
> >  					if (!intel_PLL_is_valid(dev, limit,
> >  								&clock))
> > @@ -940,7 +956,7 @@ chv_find_best_dpll(const intel_limit_t *limit,
> >  
> >  			clock.m2 = m2;
> >  
> > -			chv_clock(refclk, &clock);
> > +			chv_calc_dpll_params(refclk, &clock);
> >  
> >  			if (!intel_PLL_is_valid(dev, limit, &clock))
> >  				continue;
> > @@ -7926,10 +7942,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> >  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> >  
> > -	vlv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = vlv_calc_dpll_params(refclk, &clock);
> >  }
> >  
> >  static void
> > @@ -8025,10 +8038,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
> >  	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
> >  	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
> >  
> > -	chv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = chv_calc_dpll_params(refclk, &clock);
> >  }
> >  
> >  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > @@ -10481,6 +10491,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> >  	u32 dpll = pipe_config->dpll_hw_state.dpll;
> >  	u32 fp;
> >  	intel_clock_t clock;
> > +	int port_clock;
> >  	int refclk = i9xx_pll_refclk(dev, pipe_config);
> >  
> >  	if ((dpll & DISPLAY_RATE_SELECT_FPA1) == 0)
> > @@ -10521,9 +10532,9 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> >  		}
> >  
> >  		if (IS_PINEVIEW(dev))
> > -			pineview_clock(refclk, &clock);
> > +			port_clock = pnv_calc_dpll_params(refclk, &clock);
> >  		else
> > -			i9xx_clock(refclk, &clock);
> > +			port_clock = i9xx_calc_dpll_params(refclk, &clock);
> >  	} else {
> >  		u32 lvds = IS_I830(dev) ? 0 : I915_READ(LVDS);
> >  		bool is_lvds = (pipe == 1) && (lvds & LVDS_PORT_EN);
> > @@ -10549,7 +10560,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> >  				clock.p2 = 2;
> >  		}
> >  
> > -		i9xx_clock(refclk, &clock);
> > +		port_clock = i9xx_calc_dpll_params(refclk, &clock);
> >  	}
> >  
> >  	/*
> > @@ -10557,7 +10568,7 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> >  	 * port_clock to compute adjusted_mode.crtc_clock in the
> >  	 * encoder's get_config() function.
> >  	 */
> > -	pipe_config->port_clock = clock.dot;
> > +	pipe_config->port_clock = port_clock;
> >  }
> >  
> >  int intel_dotclock_calculate(int link_freq,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e2174fd..7a5b3bc 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1138,6 +1138,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
> >  				int dotclock);
> >  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> >  			intel_clock_t *best_clock);
> > +int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
> > +
> >  bool intel_crtc_active(struct drm_crtc *crtc);
> >  void hsw_enable_ips(struct intel_crtc *crtc);
> >  void hsw_disable_ips(struct intel_crtc *crtc);
> > -- 
> > 2.1.4
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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

Re: [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock

[Jindal, Sonika]

On 6/18/2015 7:55 PM, Imre Deak wrote:
> This functionality will be needed by the next patch adding HW readout
> support for DDI ports on BXT, so factor it out.
>
> No functional change.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++--------
>   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>   2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0e5c613..6cf2a15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7993,6 +7993,14 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
>   			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
>   }
>
> +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock)
> +{
> +	chv_clock(refclk, pll_clock);
Is vlv_clock function same as chv_clock ?
I see one clock->n << 22 in chv_clock which is not there in vlv_clock.

> +
> +	/* clock.dot is the fast clock */
> +	return pll_clock->dot / 5;
> +}
> +
>   static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>   			       struct intel_crtc_state *pipe_config)
>   {
> @@ -8017,10 +8025,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>   	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
>   	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
>
> -	vlv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
>   }
>
>   static void
> @@ -8116,10 +8121,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
>   	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
>   	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
>
> -	chv_clock(refclk, &clock);
> -
> -	/* clock.dot is the fast clock */
> -	pipe_config->port_clock = clock.dot / 5;
> +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
>   }
>
>   static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefc..95e14bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1139,6 +1139,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
>   				int dotclock);
>   bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
>   			intel_clock_t *best_clock);
> +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock);
> +
>   bool intel_crtc_active(struct drm_crtc *crtc);
>   void hsw_enable_ips(struct intel_crtc *crtc);
>   void hsw_disable_ips(struct intel_crtc *crtc);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/vlv: factor out vlv_calc_port_clock

[Imre Deak]

On ke, 2015-06-24 at 15:46 +0530, Jindal, Sonika wrote:
> 
> On 6/18/2015 7:55 PM, Imre Deak wrote:
> > This functionality will be needed by the next patch adding HW readout
> > support for DDI ports on BXT, so factor it out.
> >
> > No functional change.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++--------
> >   drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> >   2 files changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0e5c613..6cf2a15 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7993,6 +7993,14 @@ static void i9xx_get_pfit_config(struct intel_crtc *crtc,
> >   			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
> >   }
> >
> > +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock)
> > +{
> > +	chv_clock(refclk, pll_clock);
> Is vlv_clock function same as chv_clock ?
> I see one clock->n << 22 in chv_clock which is not there in vlv_clock.

No, I botched this up. But Ville noticed it already and I've sent v2
where it should be fixed.

> 
> > +
> > +	/* clock.dot is the fast clock */
> > +	return pll_clock->dot / 5;
> > +}
> > +
> >   static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >   			       struct intel_crtc_state *pipe_config)
> >   {
> > @@ -8017,10 +8025,7 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >   	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> >   	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> >
> > -	vlv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
> >   }
> >
> >   static void
> > @@ -8116,10 +8121,7 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc,
> >   	clock.p1 = (cmn_dw13 >> DPIO_CHV_P1_DIV_SHIFT) & 0x7;
> >   	clock.p2 = (cmn_dw13 >> DPIO_CHV_P2_DIV_SHIFT) & 0x1f;
> >
> > -	chv_clock(refclk, &clock);
> > -
> > -	/* clock.dot is the fast clock */
> > -	pipe_config->port_clock = clock.dot / 5;
> > +	pipe_config->port_clock = vlv_calc_port_clock(refclk, &clock);
> >   }
> >
> >   static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bcafefc..95e14bb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1139,6 +1139,8 @@ ironlake_check_encoder_dotclock(const struct intel_crtc_state *pipe_config,
> >   				int dotclock);
> >   bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int target_clock,
> >   			intel_clock_t *best_clock);
> > +int vlv_calc_port_clock(int refclk, intel_clock_t *pll_clock);
> > +
> >   bool intel_crtc_active(struct drm_crtc *crtc);
> >   void hsw_enable_ips(struct intel_crtc *crtc);
> >   void hsw_disable_ips(struct intel_crtc *crtc);
> >


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915/bxt: add DDI port HW readout support

[Imre Deak]

Add support for reading out the HW state for DDI ports. Since the actual
programming is very similar to the CHV/VLV DPIO PLL programming we can
reuse much of the logic from there.

This fixes the state checker failures I saw on my BXT with HDMI output.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
 drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bba0691..fcf6ad5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
 #define _PORT_PLL_EBB_0_A		0x162034
 #define _PORT_PLL_EBB_0_B		0x6C034
 #define _PORT_PLL_EBB_0_C		0x6C340
-#define   PORT_PLL_P1_MASK		(0x07 << 13)
-#define   PORT_PLL_P1(x)		((x)  << 13)
-#define   PORT_PLL_P2_MASK		(0x1f << 8)
-#define   PORT_PLL_P2(x)		((x)  << 8)
+#define   PORT_PLL_P1_SHIFT		13
+#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P2_SHIFT		8
+#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
+#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
 #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
 						_PORT_PLL_EBB_0_B,	\
 						_PORT_PLL_EBB_0_C)
@@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
 /* PORT_PLL_0_A */
 #define   PORT_PLL_M2_MASK		0xFF
 /* PORT_PLL_1_A */
-#define   PORT_PLL_N_MASK		(0x0F << 8)
-#define   PORT_PLL_N(x)			((x) << 8)
+#define   PORT_PLL_N_SHIFT		8
+#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
+#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
 /* PORT_PLL_2_A */
 #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
 /* PORT_PLL_3_A */
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca970ba..6859068 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
 				enum intel_dpll_id dpll)
 {
-	/* FIXME formula not available in bspec */
-	return 0;
+	struct intel_shared_dpll *pll;
+	struct intel_dpll_hw_state *state;
+	intel_clock_t clock;
+
+	/* For DDI ports we always use a shared PLL. */
+	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
+		return 0;
+
+	pll = &dev_priv->shared_dplls[dpll];
+	state = &pll->config.hw_state;
+
+	clock.m1 = 2;
+	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
+	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
+		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
+	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
+	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
+	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
+
+	return vlv_calc_port_clock(100000, &clock);
 }
 
 static void bxt_ddi_clock_get(struct intel_encoder *encoder,
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/bxt: add DDI port HW readout support

[Ville Syrjälä]

On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> Add support for reading out the HW state for DDI ports. Since the actual
> programming is very similar to the CHV/VLV DPIO PLL programming we can
> reuse much of the logic from there.
> 
> This fixes the state checker failures I saw on my BXT with HDMI output.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
>  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bba0691..fcf6ad5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
>  #define _PORT_PLL_EBB_0_A		0x162034
>  #define _PORT_PLL_EBB_0_B		0x6C034
>  #define _PORT_PLL_EBB_0_C		0x6C340
> -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> -#define   PORT_PLL_P1(x)		((x)  << 13)
> -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> -#define   PORT_PLL_P2(x)		((x)  << 8)
> +#define   PORT_PLL_P1_SHIFT		13
> +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> +#define   PORT_PLL_P2_SHIFT		8
> +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
>  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
>  						_PORT_PLL_EBB_0_B,	\
>  						_PORT_PLL_EBB_0_C)
> @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
>  /* PORT_PLL_0_A */
>  #define   PORT_PLL_M2_MASK		0xFF
>  /* PORT_PLL_1_A */
> -#define   PORT_PLL_N_MASK		(0x0F << 8)
> -#define   PORT_PLL_N(x)			((x) << 8)
> +#define   PORT_PLL_N_SHIFT		8
> +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
>  /* PORT_PLL_2_A */
>  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
>  /* PORT_PLL_3_A */
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca970ba..6859068 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
>  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
>  				enum intel_dpll_id dpll)
>  {
> -	/* FIXME formula not available in bspec */
> -	return 0;
> +	struct intel_shared_dpll *pll;
> +	struct intel_dpll_hw_state *state;
> +	intel_clock_t clock;
> +
> +	/* For DDI ports we always use a shared PLL. */
> +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> +		return 0;
> +
> +	pll = &dev_priv->shared_dplls[dpll];
> +	state = &pll->config.hw_state;
> +
> +	clock.m1 = 2;

For chv I opted to read out the m1 divider from PLL_DW1 too even though
2 is the only supported value. Doing so might catch some cases where the
register gets misprogrammed.

But that's a minor issue, so even without this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;

I just realized we're missing the fracnen check from chv_crtc_clock_get().
Care to send a patch to add the check there as well?

> +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
> +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> +
> +	return vlv_calc_port_clock(100000, &clock);
>  }
>  
>  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/5] drm/i915/bxt: add DDI port HW readout support

[Imre Deak]

On ma, 2015-06-22 at 16:44 +0300, Ville Syrjälä wrote:
> On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> > Add support for reading out the HW state for DDI ports. Since the actual
> > programming is very similar to the CHV/VLV DPIO PLL programming we can
> > reuse much of the logic from there.
> > 
> > This fixes the state checker failures I saw on my BXT with HDMI output.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
> >  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
> >  2 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index bba0691..fcf6ad5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
> >  #define _PORT_PLL_EBB_0_A		0x162034
> >  #define _PORT_PLL_EBB_0_B		0x6C034
> >  #define _PORT_PLL_EBB_0_C		0x6C340
> > -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> > -#define   PORT_PLL_P1(x)		((x)  << 13)
> > -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> > -#define   PORT_PLL_P2(x)		((x)  << 8)
> > +#define   PORT_PLL_P1_SHIFT		13
> > +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> > +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> > +#define   PORT_PLL_P2_SHIFT		8
> > +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> > +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
> >  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
> >  						_PORT_PLL_EBB_0_B,	\
> >  						_PORT_PLL_EBB_0_C)
> > @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
> >  /* PORT_PLL_0_A */
> >  #define   PORT_PLL_M2_MASK		0xFF
> >  /* PORT_PLL_1_A */
> > -#define   PORT_PLL_N_MASK		(0x0F << 8)
> > -#define   PORT_PLL_N(x)			((x) << 8)
> > +#define   PORT_PLL_N_SHIFT		8
> > +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> > +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
> >  /* PORT_PLL_2_A */
> >  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
> >  /* PORT_PLL_3_A */
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ca970ba..6859068 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> >  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
> >  				enum intel_dpll_id dpll)
> >  {
> > -	/* FIXME formula not available in bspec */
> > -	return 0;
> > +	struct intel_shared_dpll *pll;
> > +	struct intel_dpll_hw_state *state;
> > +	intel_clock_t clock;
> > +
> > +	/* For DDI ports we always use a shared PLL. */
> > +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> > +		return 0;
> > +
> > +	pll = &dev_priv->shared_dplls[dpll];
> > +	state = &pll->config.hw_state;
> > +
> > +	clock.m1 = 2;
> 
> For chv I opted to read out the m1 divider from PLL_DW1 too even though
> 2 is the only supported value. Doing so might catch some cases where the
> register gets misprogrammed.

I haven't found the BXT counterpart for this at least when looking
through BSpec. PORT_PLL_1[2:0] is "i_fbpredivratio" and it reads 0 for
me.

> But that's a minor issue, so even without this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> > +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> > +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
> 
> I just realized we're missing the fracnen check from chv_crtc_clock_get().
> Care to send a patch to add the check there as well?

Ok.

> > +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;

Btw, now that I looked through BSpec and the code again, this looks like
also be tied to 1, so we'd need to check for this in
bxt_ddi_pll_select(). chv_find_best_dpll() does the right thing already.

> > +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> > +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> > +
> > +	return vlv_calc_port_clock(100000, &clock);
> >  }
> >  
> >  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/bxt: add DDI port HW readout support

[Ville Syrjälä]

On Mon, Jun 22, 2015 at 05:22:33PM +0300, Imre Deak wrote:
> On ma, 2015-06-22 at 16:44 +0300, Ville Syrjälä wrote:
> > On Thu, Jun 18, 2015 at 05:25:57PM +0300, Imre Deak wrote:
> > > Add support for reading out the HW state for DDI ports. Since the actual
> > > programming is very similar to the CHV/VLV DPIO PLL programming we can
> > > reuse much of the logic from there.
> > > 
> > > This fixes the state checker failures I saw on my BXT with HDMI output.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
> > >  drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
> > >  2 files changed, 29 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index bba0691..fcf6ad5 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
> > >  #define _PORT_PLL_EBB_0_A		0x162034
> > >  #define _PORT_PLL_EBB_0_B		0x6C034
> > >  #define _PORT_PLL_EBB_0_C		0x6C340
> > > -#define   PORT_PLL_P1_MASK		(0x07 << 13)
> > > -#define   PORT_PLL_P1(x)		((x)  << 13)
> > > -#define   PORT_PLL_P2_MASK		(0x1f << 8)
> > > -#define   PORT_PLL_P2(x)		((x)  << 8)
> > > +#define   PORT_PLL_P1_SHIFT		13
> > > +#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
> > > +#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
> > > +#define   PORT_PLL_P2_SHIFT		8
> > > +#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
> > > +#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
> > >  #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
> > >  						_PORT_PLL_EBB_0_B,	\
> > >  						_PORT_PLL_EBB_0_C)
> > > @@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
> > >  /* PORT_PLL_0_A */
> > >  #define   PORT_PLL_M2_MASK		0xFF
> > >  /* PORT_PLL_1_A */
> > > -#define   PORT_PLL_N_MASK		(0x0F << 8)
> > > -#define   PORT_PLL_N(x)			((x) << 8)
> > > +#define   PORT_PLL_N_SHIFT		8
> > > +#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
> > > +#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
> > >  /* PORT_PLL_2_A */
> > >  #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
> > >  /* PORT_PLL_3_A */
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index ca970ba..6859068 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
> > >  static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
> > >  				enum intel_dpll_id dpll)
> > >  {
> > > -	/* FIXME formula not available in bspec */
> > > -	return 0;
> > > +	struct intel_shared_dpll *pll;
> > > +	struct intel_dpll_hw_state *state;
> > > +	intel_clock_t clock;
> > > +
> > > +	/* For DDI ports we always use a shared PLL. */
> > > +	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
> > > +		return 0;
> > > +
> > > +	pll = &dev_priv->shared_dplls[dpll];
> > > +	state = &pll->config.hw_state;
> > > +
> > > +	clock.m1 = 2;
> > 
> > For chv I opted to read out the m1 divider from PLL_DW1 too even though
> > 2 is the only supported value. Doing so might catch some cases where the
> > register gets misprogrammed.
> 
> I haven't found the BXT counterpart for this at least when looking
> through BSpec. PORT_PLL_1[2:0] is "i_fbpredivratio" and it reads 0 for
> me.

That's the one.

> 
> > But that's a minor issue, so even without this is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > +	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> > > +	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
> > > +		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
> > 
> > I just realized we're missing the fracnen check from chv_crtc_clock_get().
> > Care to send a patch to add the check there as well?
> 
> Ok.
> 
> > > +	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
> 
> Btw, now that I looked through BSpec and the code again, this looks like
> also be tied to 1, so we'd need to check for this in
> bxt_ddi_pll_select(). chv_find_best_dpll() does the right thing already.

You mean WARN if n != 1? I suppose it can't hurt.

> 
> > > +	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
> > > +	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
> > > +
> > > +	return vlv_calc_port_clock(100000, &clock);
> > >  }
> > >  
> > >  static void bxt_ddi_clock_get(struct intel_encoder *encoder,
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 

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

[PATCH v2 5/5] drm/i915/bxt: add DDI port HW readout support

[Imre Deak]

Add support for reading out the HW state for DDI ports. Since the actual
programming is very similar to the CHV/VLV DPIO PLL programming we can
reuse much of the logic from there.

This fixes the state checker failures I saw on my BXT with HDMI output.

v2:
- rebased on v2 of patch 4/5

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 15 +++++++++------
 drivers/gpu/drm/i915/intel_ddi.c | 22 ++++++++++++++++++++--
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 77f1c7e..1b35423 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1169,10 +1169,12 @@ enum skl_disp_power_wells {
 #define _PORT_PLL_EBB_0_A		0x162034
 #define _PORT_PLL_EBB_0_B		0x6C034
 #define _PORT_PLL_EBB_0_C		0x6C340
-#define   PORT_PLL_P1_MASK		(0x07 << 13)
-#define   PORT_PLL_P1(x)		((x)  << 13)
-#define   PORT_PLL_P2_MASK		(0x1f << 8)
-#define   PORT_PLL_P2(x)		((x)  << 8)
+#define   PORT_PLL_P1_SHIFT		13
+#define   PORT_PLL_P1_MASK		(0x07 << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P1(x)		((x)  << PORT_PLL_P1_SHIFT)
+#define   PORT_PLL_P2_SHIFT		8
+#define   PORT_PLL_P2_MASK		(0x1f << PORT_PLL_P2_SHIFT)
+#define   PORT_PLL_P2(x)		((x)  << PORT_PLL_P2_SHIFT)
 #define BXT_PORT_PLL_EBB_0(port)	_PORT3(port, _PORT_PLL_EBB_0_A, \
 						_PORT_PLL_EBB_0_B,	\
 						_PORT_PLL_EBB_0_C)
@@ -1192,8 +1194,9 @@ enum skl_disp_power_wells {
 /* PORT_PLL_0_A */
 #define   PORT_PLL_M2_MASK		0xFF
 /* PORT_PLL_1_A */
-#define   PORT_PLL_N_MASK		(0x0F << 8)
-#define   PORT_PLL_N(x)			((x) << 8)
+#define   PORT_PLL_N_SHIFT		8
+#define   PORT_PLL_N_MASK		(0x0F << PORT_PLL_N_SHIFT)
+#define   PORT_PLL_N(x)			((x) << PORT_PLL_N_SHIFT)
 /* PORT_PLL_2_A */
 #define   PORT_PLL_M2_FRAC_MASK		0x3FFFFF
 /* PORT_PLL_3_A */
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca970ba..31eb289 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -971,8 +971,26 @@ static void hsw_ddi_clock_get(struct intel_encoder *encoder,
 static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
 				enum intel_dpll_id dpll)
 {
-	/* FIXME formula not available in bspec */
-	return 0;
+	struct intel_shared_dpll *pll;
+	struct intel_dpll_hw_state *state;
+	intel_clock_t clock;
+
+	/* For DDI ports we always use a shared PLL. */
+	if (WARN_ON(dpll == DPLL_ID_PRIVATE))
+		return 0;
+
+	pll = &dev_priv->shared_dplls[dpll];
+	state = &pll->config.hw_state;
+
+	clock.m1 = 2;
+	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
+	if (state->pll3 & PORT_PLL_M2_FRAC_ENABLE)
+		clock.m2 |= state->pll2 & PORT_PLL_M2_FRAC_MASK;
+	clock.n = (state->pll1 & PORT_PLL_N_MASK) >> PORT_PLL_N_SHIFT;
+	clock.p1 = (state->ebb0 & PORT_PLL_P1_MASK) >> PORT_PLL_P1_SHIFT;
+	clock.p2 = (state->ebb0 & PORT_PLL_P2_MASK) >> PORT_PLL_P2_SHIFT;
+
+	return chv_calc_dpll_params(100000, &clock);
 }
 
 static void bxt_ddi_clock_get(struct intel_encoder *encoder,
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915/bxt: mask off the DPLL state checker bits we don't program

[Jindal, Sonika]

Looks good to me.
Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>

On 6/18/2015 7:55 PM, Imre Deak wrote:
> For the purpose of state checking we only care about the DPLL HW flags
> that we actually program, so mask off the ones that we don't.
>
> This fixes one set of DPLL state check failures.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_ddi.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9ae297a..bdc5677 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2479,13 +2479,32 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>   		return false;
>
>   	hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port));
> +	hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK;
> +
>   	hw_state->pll0 = I915_READ(BXT_PORT_PLL(port, 0));
> +	hw_state->pll0 &= PORT_PLL_M2_MASK;
> +
>   	hw_state->pll1 = I915_READ(BXT_PORT_PLL(port, 1));
> +	hw_state->pll1 &= PORT_PLL_N_MASK;
> +
>   	hw_state->pll2 = I915_READ(BXT_PORT_PLL(port, 2));
> +	hw_state->pll2 &= PORT_PLL_M2_FRAC_MASK;
> +
>   	hw_state->pll3 = I915_READ(BXT_PORT_PLL(port, 3));
> +	hw_state->pll3 &= PORT_PLL_M2_FRAC_ENABLE;
> +
>   	hw_state->pll6 = I915_READ(BXT_PORT_PLL(port, 6));
> +	hw_state->pll6 &= PORT_PLL_PROP_COEFF_MASK |
> +			  PORT_PLL_INT_COEFF_MASK |
> +			  PORT_PLL_GAIN_CTL_MASK;
> +
>   	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
> +	hw_state->pll8 &= PORT_PLL_TARGET_CNT_MASK;
> +
>   	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
> +	hw_state->pll10 &= PORT_PLL_DCO_AMP_OVR_EN_H |
> +			   PORT_PLL_DCO_AMP_MASK;
> +
>   	/*
>   	 * While we write to the group register to program all lanes at once we
>   	 * can read only lane registers. We configure all lanes the same way, so
> @@ -2496,6 +2515,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>   		DRM_DEBUG_DRIVER("lane stagger config different for lane 01 (%08x) and 23 (%08x)\n",
>   				 hw_state->pcsdw12,
>   				 I915_READ(BXT_PORT_PCS_DW12_LN23(port)));
> +	hw_state->pcsdw12 &= LANE_STAGGER_MASK | LANESTAGGER_STRAP_OVRD;
>
>   	return true;
>   }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx