Re: [PATCH] drm/i915/bxt: Port PLL programming BUN

[Imre Deak <imre.deak@intel.com>]

On ma, 2015-05-04 at 20:50 +0530, Vandana Kannan wrote:
> BUN 1: prop_coeff, int_coeff, tdctargetcnt programming updated and tied to
> VCO frequencies. Program i_lockthresh in PORT_PLL_9.
> 
> VCO calculated based on the formula:
> Desired Output = Port bit rate in MHz (DisplayPort HBR2 is 5400 MHz)
> Fast Clock = Desired Output / 2
> VCO = Fast Clock * P1 * P2
> 
> Prop_coeff, int_coeff, and tdctargetcnt modified according to above
> calculation.
> 
> BUN 2: Port PLLs require additional programming at certain frequencies -
> DCO amplitude in PORT_PLL_10
> 
> Review comments from Siva which were addressed in the initial version of the
> patch.
> 	- Change PORT_PLL_LOCK_THRESHOLD to PORT_PLL_LOCK_THRESHOLD_MASK
> 	- Calculate for HDMI
> 	- Correct values for vco = 5.4
> 	- return in case of invalid vco range
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>  drivers/gpu/drm/i915/i915_reg.h  |  6 ++++
>  drivers/gpu/drm/i915/intel_ddi.c | 72 +++++++++++++++++++++++++++++++---------
>  3 files changed, 63 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f3c77ca..cea54ee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -309,7 +309,7 @@ struct intel_dpll_hw_state {
>  	uint32_t cfgcr1, cfgcr2;
>  
>  	/* bxt */
> -	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, pcsdw12;
> +	uint32_t ebb0, pll0, pll1, pll2, pll3, pll6, pll8, 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 36805b6..86fdc3e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1185,6 +1185,12 @@ enum skl_disp_power_wells {
>  #define   PORT_PLL_GAIN_CTL(x)		((x)  << 16)
>  /* PORT_PLL_8_A */
>  #define   PORT_PLL_TARGET_CNT_MASK	0x3FF
> +/* PORT_PLL_9_A */
> +#define  PORT_PLL_LOCK_THRESHOLD_MASK	0xe
> +/* PORT_PLL_10_A */
> +#define  PORT_PLL_DCO_AMP_OVR_EN	(1<<27)

This is actually OVR_EN_H, so I'd rename it accordingly, or simply use
OVR_DIS.

> +#define  PORT_PLL_DCO_AMP_MASK		0x3c00
> +#define  PORT_PLL_DCO_AMP(x)		(x<<10)
>  #define _PORT_PLL_BASE(port)		_PORT3(port, _PORT_PLL_0_A,	\
>  						_PORT_PLL_0_B,		\
>  						_PORT_PLL_0_C)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 455d44b..3014c37 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1319,18 +1319,20 @@ struct bxt_clk_div {
>  	uint32_t int_coef;
>  	uint32_t gain_ctl;
>  	uint32_t targ_cnt;
> +	uint32_t dcoampovr_en;

As above _en_h, or _dis.

> +	uint32_t dco_amp;
>  	uint32_t lanestagger;
>  };
>  
>  /* pre-calculated values for DP linkrates */
>  static struct bxt_clk_div bxt_dp_clk_val[7] = {
> -	/* 162 */ {4, 2, 32, 1677722, 1, 1, 5, 11, 2, 9, 0xd},
> -	/* 270 */ {4, 1, 27,       0, 0, 1, 3,  8, 1, 9, 0xd},
> -	/* 540 */ {2, 1, 27,       0, 0, 1, 3,  8, 1, 9, 0x18},
> -	/* 216 */ {3, 2, 32, 1677722, 1, 1, 5, 11, 2, 9, 0xd},
> -	/* 243 */ {4, 1, 24, 1258291, 1, 1, 5, 11, 2, 9, 0xd},
> -	/* 324 */ {4, 1, 32, 1677722, 1, 1, 5, 11, 2, 9, 0xd},
> -	/* 432 */ {3, 1, 32, 1677722, 1, 1, 5, 11, 2, 9, 0x18}
> +	/* 162 */ {4, 2, 32, 1677722, 1, 1, 4, 9, 3, 8, 0, 15, 0xd},
> +	/* 270 */ {4, 1, 27,       0, 0, 1, 3,  8, 1, 9, 0, 15, 0xd},
> +	/* 540 */ {2, 1, 27,       0, 0, 1, 3,  8, 1, 9, 0, 15, 0x18},
> +	/* 216 */ {3, 2, 32, 1677722, 1, 1, 4, 9, 3, 8, 0, 15, 0xd},
> +	/* 243 */ {4, 1, 24, 1258291, 1, 1, 5, 11, 3, 9, 1, 15, 0xd},
> +	/* 324 */ {4, 1, 32, 1677722, 1, 1, 4, 9, 3, 8, 0, 15, 0xd},
> +	/* 432 */ {3, 1, 32, 1677722, 1, 1, 4, 9, 3, 8, 0, 15, 0x18}
>  };
>  
>  static bool
> @@ -1364,11 +1366,34 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		clk_div.m2_frac = best_clock.m2 & ((1 << 22) - 1);
>  		clk_div.m2_frac_en = clk_div.m2_frac != 0;
>  
> -		/* FIXME: set coef, gain, targcnt based on freq band */
> -		clk_div.prop_coef = 5;
> -		clk_div.int_coef = 11;
> -		clk_div.gain_ctl = 2;
> -		clk_div.targ_cnt = 9;
> +		clk_div.dco_amp = 15;
> +		clk_div.dcoampovr_en = 0;
> +		if (best_clock.vco >= 6200000 && best_clock.vco <= 6480000) {
> +			clk_div.prop_coef = 4;
> +			clk_div.int_coef = 9;
> +			clk_div.gain_ctl = 3;
> +			clk_div.targ_cnt = 8;
> +		} else if ((best_clock.vco > 5400000 &&
> +				best_clock.vco < 6200000) ||
> +				(best_clock.vco >= 4800000 &&
> +				best_clock.vco < 5400000)) {
> +			clk_div.prop_coef = 5;
> +			clk_div.int_coef = 11;
> +			clk_div.gain_ctl = 3;
> +			clk_div.targ_cnt = 9;
> +			if (best_clock.vco >= 4800000 &&
> +				best_clock.vco < 5400000)
> +				clk_div.dcoampovr_en = 1;
> +		} else if (best_clock.vco == 5400000) {
> +			clk_div.prop_coef = 3;
> +			clk_div.int_coef = 8;
> +			clk_div.gain_ctl = 1;
> +			clk_div.targ_cnt = 9;
> +		} else {
> +			DRM_ERROR("Invalid VCO\n");
> +			return false;
> +		}
> +

This gets messy as we duplicate the code to calculate the PHY params for
DP and HDMI and we do them in different ways. The lanestagger value for
324MHz symbol rate in bxt_dp_clk_val looks also incorrect, should be
0x18 instead of 0xd.

Things would become much clearer if you'd use the above logic for both
HDMI and DP:

int vco;
if (intel_encoder->type == INTEL_OUTPUT_HDMI) {
	...
	vco = best_clock.vco;
} else {
	...
	vco = clock * 10 / 2 * clk_div.p1 * clk_div.p2;
}

Afterwards you can use the logic you added in this patch for both HDMI
and DP.

Similarly, lanestagger should be calculated in the same way for both
HDMI and DP (changing it in a separate patch).

>  		if (clock > 270000)
>  			clk_div.lanestagger = 0x18;
>  		else if (clock > 135000)
> @@ -1417,6 +1442,11 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  
>  	crtc_state->dpll_hw_state.pll8 = clk_div.targ_cnt;
>  
> +	if (clk_div.dcoampovr_en)
> +		crtc_state->dpll_hw_state.pll10 = PORT_PLL_DCO_AMP_OVR_EN;
> +
> +	crtc_state->dpll_hw_state.pll10 |= PORT_PLL_DCO_AMP(clk_div.dco_amp);
> +
>  	crtc_state->dpll_hw_state.pcsdw12 =
>  		LANESTAGGER_STRAP_OVRD | clk_div.lanestagger;
>  
> @@ -2348,10 +2378,19 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	temp |= pll->config.hw_state.pll8;
>  	I915_WRITE(BXT_PORT_PLL(port, 8), temp);
>  
> -	/*
> -	 * FIXME: program PORT_PLL_9/i_lockthresh according to the latest
> -	 * specification update.
> -	 */
> +	/* Spec update: Write 5 to lock threshold */

This comment just says what the code does, so no need to spell it out
separately.

> +	temp = I915_READ(BXT_PORT_PLL(port, 9));
> +	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
> +	temp |= (5 << 1);
> +	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
> +
> +	/* Spec update: Write DCO amplitude override value to i_dcoamp */
> +	/* Spec update: Write DCO amplitude override enable to i_dcoampovrden_h */

No need for the above comments.

> +	temp = I915_READ(BXT_PORT_PLL(port, 10));
> +	temp &= ~PORT_PLL_DCO_AMP_OVR_EN;
> +	temp &= ~PORT_PLL_DCO_AMP_MASK;
> +	temp |= pll->config.hw_state.pll10;
> +	I915_WRITE(BXT_PORT_PLL(port, 10), temp);
>  
>  	/* Recalibrate with new settings */
>  	temp = I915_READ(BXT_PORT_PLL_EBB_4(port));
> @@ -2415,6 +2454,7 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	hw_state->pll3 = I915_READ(BXT_PORT_PLL(port, 3));
>  	hw_state->pll6 = I915_READ(BXT_PORT_PLL(port, 6));
>  	hw_state->pll8 = I915_READ(BXT_PORT_PLL(port, 8));
> +	hw_state->pll10 = I915_READ(BXT_PORT_PLL(port, 10));
>  	/*
>  	 * 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

We should also read out pll10 in bxt_ddi_pll_get_hw_state() for
consistency.

--Imre

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