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

["Purushothaman, Vijay A" <vijay.a.purushothaman@linux.intel.com>]

On 1/30/2015 4:39 PM, Ville Syrjälä wrote:
> 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
I will remove these definitions in the next patch series.

>
>> +
>> +#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
I will remove these definitions 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.
I was trying to follow the same convention used in this function. I do 
agree that hiding vlv_dpio_read() inside vlv_dpio_write() is not a good 
practice.
I would prefer to keep the RMW stuff though. In general i am averse to 
touch anything in DPIO side logic since it results in many trial and 
errors before we get the sequence right.

>
>> +				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?
The values used are in KHz range. I will fix this in a proper way in 
next patch series.
>
>> +		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.
Will try my level best to decode the magic bits. This value is used 
as-is from windows code snippet. I was told that the PHY h/w team passed 
these magic numbers in the same format and since it was working in 
windows, my request for detailed documentation is not getting the right 
attention.

>
>> +	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?
Yes.

>
>> +
>> +	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?
Again this is based on trial and error in other OS and the tip from 
windows driver team is to set both DPIO_AFC_RECAL and DPIO_CLKP_EN in a 
single shot. Otherwise they are seeing some stability issues. I guess i 
will have to move this part to chv_enable_pll.
>
>
> 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.
sure. I will address this in my next patch set.
>
> 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?
I have not observed this yet. This could be because  most of our efforts 
are on a different code base - ADF.

Thanks,
Vijay
>
>>   
>>   	mutex_unlock(&dev_priv->dpio_lock);
>>   }
>> -- 
>> 1.7.9.5

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