Re: [PATCH] drm/i915: More DPIO magic for CHV HDMI & DP

["Ville Syrjälä" <ville.syrjala@intel.com>]

On Fri, Jan 30, 2015 at 12:01:53AM +0530, Vijay Purushothaman wrote:
> This patch implements latest changes in Gain, lock threshold and integer
> co-efficient values using sideband r/w. Without these changes there will
> be signal integrity issues for both HDMI and DP.
> 
> Change-Id: I7b7151b5ab3a52c4c912cf10602c69a7d1a70222
> Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman@linux.intel.com>
> Tested-by: Hong Liu <hong.liu@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   31 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   67 ++++++++++++++++++++++++----------
>  2 files changed, 79 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 137c5e0..2b3f065 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1049,6 +1049,37 @@ enum punit_power_well {
>  #define   DPIO_CHV_PROP_COEFF_SHIFT	0
>  #define CHV_PLL_DW6(ch) _PIPE(ch, _CHV_PLL_DW6_CH0, _CHV_PLL_DW6_CH1)
>  
> +#define _CHV_PLL_DW7_CH0		0x801c
> +#define _CHV_PLL_DW7_CH1		0x803c
> +#define CHV_PLL_DW7(ch) _PIPE(ch, _CHV_PLL_DW7_CH0, _CHV_PLL_DW7_CH1)

unused

> +
> +#define _CHV_PLL_DW8_CH0		0x8020
> +#define _CHV_PLL_DW8_CH1		0x81A0
> +#define CHV_PLL_DW8(ch) _PIPE(ch, _CHV_PLL_DW8_CH0, _CHV_PLL_DW8_CH1)
> +
> +#define _CHV_PLL_DW9_CH0		0x8024
> +#define _CHV_PLL_DW9_CH1		0x81A4
> +#define  DPIO_CHV_INT_LOCK_THRESHOLD_SHIFT		1 /* 3 bits */
> +#define  DPIO_CHV_INT_LOCK_THRESHOLD_SEL_COARSE	1 /* 1: coarse & 0 : fine  */
> +#define CHV_PLL_DW9(ch) _PIPE(ch, _CHV_PLL_DW9_CH0, _CHV_PLL_DW9_CH1)
> +
> +#define _CHV_PLL_DW10_CH0		0x8040
> +#define _CHV_PLL_DW10_CH1		0x8060
> +#define CHV_PLL_DW10(ch) _PIPE(ch, _CHV_PLL_DW10_CH0, _CHV_PLL_DW10_CH1)
> +
> +#define _CHV_PLL_DW11_BCAST		0xC044
> +#define _CHV_PLL_DW11_CH0		0x8044
> +#define _CHV_PLL_DW11_CH1		0x8064
> +#define CHV_PLL_DW11(ch) _PIPE(ch, _CHV_PLL_DW11_CH0, _CHV_PLL_DW11_CH1)
> +
> +#define _CHV_PLL_DW12_CH0		0x8048
> +#define _CHV_PLL_DW12_CH1		0x8068
> +#define CHV_PLL_DW12(ch) _PIPE(ch, _CHV_PLL_DW12_CH0, _CHV_PLL_DW12_CH1)
> +
> +#define _CHV_PLL_DW13_CH0		0x804C
> +#define _CHV_PLL_DW13_CH1		0x806C
> +#define CHV_PLL_DW13(ch) _PIPE(ch, _CHV_PLL_DW13_CH0, _CHV_PLL_DW13_CH1)

DW10-DW13 are unused as well

> +
>  #define _CHV_CMN_DW5_CH0               0x8114
>  #define   CHV_BUFRIGHTENA1_DISABLE	(0 << 20)
>  #define   CHV_BUFRIGHTENA1_NORMAL	(1 << 20)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c362d11e..fb27faf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6576,9 +6576,9 @@ static void chv_update_pll(struct intel_crtc *crtc)
>  	int pipe = crtc->pipe;
>  	int dpll_reg = DPLL(crtc->pipe);
>  	enum dpio_channel port = vlv_pipe_to_channel(pipe);
> -	u32 loopfilter, intcoeff;
> +	u32 loopfilter, tribuf_calcntr;
>  	u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
> -	int refclk;
> +	int vco;
>  
>  	crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
>  		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> @@ -6595,6 +6595,7 @@ static void chv_update_pll(struct intel_crtc *crtc)
>  	bestm2 = crtc->config.dpll.m2 >> 22;
>  	bestp1 = crtc->config.dpll.p1;
>  	bestp2 = crtc->config.dpll.p2;
> +	vco = crtc->config.dpll.vco;
>  
>  	/*
>  	 * Enable Refclk and SSC
> @@ -6619,31 +6620,59 @@ static void chv_update_pll(struct intel_crtc *crtc)
>  			DPIO_CHV_M1_DIV_BY_2 |
>  			1 << DPIO_CHV_N_DIV_SHIFT);
>  
> -	/* M2 fraction division */
> -	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);
> +	if (bestm2_frac) {
> +		/* M2 fraction division */
> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW2(port), bestm2_frac);
> +
> +		/* M2 fraction division enable */
> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
> +				vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port)) &

s/&/|/

As a general style issue I don't like hiding the vlv_dpio_read() inside
the vlv_dpio_write(). So the patter has been:

val = vlv_dpio_read();
change val
vlv_dpip_write(val);

Eventually I'm planning to get rid of the RMW stuff. But I've not done
that yet since I was worried some of the unchanged reset values would
still change as the hardware evolves. I'm hoping stuff has been more or
less nailed down by now so we could probably attempt this.

> +				DPIO_CHV_FRAC_DIV_EN);
> +
> +		/* Program digital lock detect threshold */
> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW9(port),
> +				vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW9(port)) |
> +				(0x5 << DPIO_CHV_INT_LOCK_THRESHOLD_SHIFT));
> +	} else {
> +		/* M2 fraction division disable */
> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
> +				vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW3(port)) &
> +				~(DPIO_CHV_FRAC_DIV_EN));
>  
> -	/* M2 fraction division enable */
> -	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
> -		       DPIO_CHV_FRAC_DIV_EN |
> -		       (2 << DPIO_CHV_FEEDFWD_GAIN_SHIFT));
> +		/* Program digital lock detect threshold */
> +		vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW9(port),
> +				vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW9(port)) |
> +				(0x5 << DPIO_CHV_INT_LOCK_THRESHOLD_SHIFT) |
> +				DPIO_CHV_INT_LOCK_THRESHOLD_SEL_COARSE);
> +	}
>  
>  	/* Loop filter */
> -	refclk = i9xx_get_refclk(&crtc->base, 0);
> -	loopfilter = 5 << DPIO_CHV_PROP_COEFF_SHIFT |
> -		2 << DPIO_CHV_GAIN_CTRL_SHIFT;
> -	if (refclk == 100000)
> -		intcoeff = 11;
> -	else if (refclk == 38400)
> -		intcoeff = 10;
> +	if (vco == 540000)
> +		loopfilter = 0x10803;
> +	else if (vco <= 620000)
> +		loopfilter = 0x30B05;
> +	else if (vco <= 648000)

Those vco limits look way too low. The vco freq should be somewhere
in the 4-6 GHz range. Are these off by a factor of 10, or were you
thinking of some other clock here?

> +		loopfilter = 0x30904;
> +	else
> +		loopfilter = 0x10803;

I'd like to have names for all the magic bits, especially as the spec
situation is what it is, so decoding magic numbers is a bit painful.

> +	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW6(port),
> +			((vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW6(port)) &
> +			0xFF000000) | loopfilter));

The top bits seem to be all reserved and default to 0. So the mask and
RMW can surely go?

> +
> +	if (vco <= 620000)
> +		tribuf_calcntr = 0x9;
> +	else if (vco <= 648000)
> +		tribuf_calcntr = 0x8;
>  	else
> -		intcoeff = 9;
> -	loopfilter |= intcoeff << DPIO_CHV_INT_COEFF_SHIFT;
> -	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW6(port), loopfilter);
> +		tribuf_calcntr = 0;
> +	vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW8(port),
> +			((vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW8(port)) &
> +			0xFFFFFF00) | tribuf_calcntr));
>  
>  	/* AFC Recal */
>  	vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW14(port),
>  			vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port)) |
> -			DPIO_AFC_RECAL);
> +			DPIO_AFC_RECAL | DPIO_DCLKP_EN);

Why this? We enable it in chv_enable_pll() already after the PLL itself
has been enabled. Is that too late for some reason?


Generally I think the patch should be split into several parts:
- frac vs. int divider
- int_lock_threshold
- loop filter stuff
- dclkp enable, if really needed, and the commit message should say why
  it's needed.


On a slightly related topic, yesterday I managed to narrow down some kind
of problem with data lane DCC calibration. It manifests as link training
failing when driving DP port B with pipe A after the cmnlane power well
has been turned off, or if port B has been driven with pipe B previously.
If I retry the modeset a second time the link training will succeed. I
also managed to make it work by forcing a DCC calibration in
chv_pre_enable_dp(). What's slightly odd is that with pipe A the forced
calibration succeeds, but with pipe B it doesn't and yet pipe B always
works and the calibration status seems to indicates success after the
DP port register has been enabled. I'm still looking into this, but I
was just wondering if you've seen anything similar?

>  
>  	mutex_unlock(&dev_priv->dpio_lock);
>  }
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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