[Intel-gfx] [PATCH] drm/i915: More DPIO magic for CHV HDMI & DP
Purushothaman, Vijay A
vijay.a.purushothaman at linux.intel.com
Thu Feb 12 05:19:21 PST 2015
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 at linux.intel.com>
>> Tested-by: Hong Liu <hong.liu at 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
More information about the Intel-gfx
mailing list