Re: [PATCH 21/71] drm/i915/chv: Add update and enable pll for Cherryview

Imre Deak
2014-04-29T20:21:01Z

On Wed, 2014-04-09 at 13:28 +0300, ville.syrjala@linux.intel.com wrote:
> From: Chon Ming Lee <chon.ming.lee@intel.com>
>
> Added programming PLL for CHV based on "Application note for 1273 CHV
> Display phy".
>
> v2: -Break the common lane reset into another patch.
> -Break the clock calculation into another patch.
>
> -The changes are based on Ville review.
> -Rework based on DPIO register define naming convention change.
> -Break the dpio write into few lines to improve readability.
> -Correct the udelay during chv_enable_pll.
> -clean up some magic numbers with some new define.
> -program the afc recal bit which was missed.
>
> v3: Based on Ville review
> - minor correction of the bit defination
> - add deassert/propagate data lane reset
>
> v4: Corrected the udelay between dclkp enable and pll enable.
> Minor comment and better way to clear the TX lane reset.
>
> v5: Squash in fixup from Rafael Barbalho.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>

A couple of nitpicks below, fixing any/all of those is optional. This
patch is
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
> drivers/gpu/drm/i915/i915_reg.h | 70 ++++++++++++++++++
> drivers/gpu/drm/i915/intel_display.c | 133 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 201 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8fcf4ea..75f31f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -668,6 +668,12 @@ enum punit_power_well {
> #define _VLV_PCS_DW9_CH1 0x8424
> #define VLV_PCS_DW9(ch) _PORT(ch, _VLV_PCS_DW9_CH0, _VLV_PCS_DW9_CH1)
>
> +#define _CHV_PCS_DW10_CH0 0x8228
> +#define _CHV_PCS_DW10_CH1 0x8428

Note that these are atm unused (the code uses instead the single
addressing versions), although these group addressing versions could be
used too.

> +#define DPIO_PCS_SWING_CALC_TX0_TX2 (1<<30)
> +#define DPIO_PCS_SWING_CALC_TX1_TX3 (1<<31)
> +#define CHV_PCS_DW10(ch) _PORT(ch, _CHV_PCS_DW10_CH0, _CHV_PCS_DW10_CH1)
> +
> #define _VLV_PCS_DW11_CH0 0x822c
> #define _VLV_PCS_DW11_CH1 0x842c
> #define VLV_PCS_DW11(ch) _PORT(ch, _VLV_PCS_DW11_CH0, _VLV_PCS_DW11_CH1)
> @@ -686,14 +692,21 @@ enum punit_power_well {
>
> #define _VLV_TX_DW2_CH0 0x8288
> #define _VLV_TX_DW2_CH1 0x8488
> +#define DPIO_UNIQ_TRANS_SCALE_SHIFT 8
> +#define DPIO_SWING_MARGIN_SHIFT 16
> +#define DPIO_SWING_MARGIN_MASK (0xff << DPIO_SWING_MARGIN_SHIFT)
> #define VLV_TX_DW2(ch) _PORT(ch, _VLV_TX_DW2_CH0, _VLV_TX_DW2_CH1)
>
> #define _VLV_TX_DW3_CH0 0x828c
> #define _VLV_TX_DW3_CH1 0x848c
> +/* The following bit for CHV phy */
> +#define DPIO_TX_UNIQ_TRANS_SCALE_EN (1<<27)
> #define VLV_TX_DW3(ch) _PORT(ch, _VLV_TX_DW3_CH0, _VLV_TX_DW3_CH1)
>
> #define _VLV_TX_DW4_CH0 0x8290
> #define _VLV_TX_DW4_CH1 0x8490
> +#define DPIO_SWING_DEEMPH9P5_SHIFT 24
> +#define DPIO_SWING_DEEMPH9P5_MASK (0xff << DPIO_SWING_DEEMPH9P5_SHIFT)
> #define VLV_TX_DW4(ch) _PORT(ch, _VLV_TX_DW4_CH0, _VLV_TX_DW4_CH1)
>
> #define _VLV_TX3_DW4_CH0 0x690
> @@ -713,6 +726,62 @@ enum punit_power_well {
> #define _VLV_TX_DW14_CH1 0x84b8
> #define VLV_TX_DW14(ch) _PORT(ch, _VLV_TX_DW14_CH0, _VLV_TX_DW14_CH1)
>
> +/* CHV dpPhy registers */
> +#define _CHV_PLL_DW0_CH0 0x8000
> +#define _CHV_PLL_DW0_CH1 0x8180
> +#define CHV_PLL_DW0(ch) _PIPE(ch, _CHV_PLL_DW0_CH0, _CHV_PLL_DW0_CH1)
> +
> +#define _CHV_PLL_DW1_CH0 0x8004
> +#define _CHV_PLL_DW1_CH1 0x8184
> +#define DPIO_CHV_M1_DIV_BY_2 (0 << 0)
> +#define DPIO_CHV_N_DIV_SHIFT (8)

Here and below for simple constants, no need for the ().

> +#define CHV_PLL_DW1(ch) _PIPE(ch, _CHV_PLL_DW1_CH0, _CHV_PLL_DW1_CH1)
> +
> +#define _CHV_PLL_DW2_CH0 0x8008
> +#define _CHV_PLL_DW2_CH1 0x8188
> +#define CHV_PLL_DW2(ch) _PIPE(ch, _CHV_PLL_DW2_CH0, _CHV_PLL_DW2_CH1)
> +
> +#define _CHV_PLL_DW3_CH0 0x800c
> +#define _CHV_PLL_DW3_CH1 0x818c
> +#define DPIO_CHV_FEEDFWD_GAIN 2
> +#define DPIO_CHV_FIRST_MOD (0 << 8)
> +#define DPIO_CHV_SECOND_MOD (1 << 8)
> +#define DPIO_CHV_FRAC_DIV_EN (1 << 16)
> +#define CHV_PLL_DW3(ch) _PIPE(ch, _CHV_PLL_DW3_CH0, _CHV_PLL_DW3_CH1)
> +
> +#define _CHV_PLL_DW6_CH0 0x8018
> +#define _CHV_PLL_DW6_CH1 0x8198
> +#define DPIO_CHV_PROP_COEFF (5 << 0)
> +#define DPIO_CHV_GAIN_CTRL (2 << 16)

These macros combine the field shift and field value, for consistency
I'd define here only the field shift and use the field value inlined in
the code.

> +#define DPIO_CHV_INT_COEFF_SHIFT 8
> +#define CHV_PLL_DW6(ch) _PIPE(ch, _CHV_PLL_DW6_CH0, _CHV_PLL_DW6_CH1)
> +
> +#define _CHV_CMN_DW13_CH0 0x8134
> +#define _CHV_CMN_DW0_CH1 0x8080
> +#define DPIO_CHV_S1_DIV_SELECT (21)
> +#define DPIO_CHV_P1_SHIFT (13) /* 3 bits */
> +#define DPIO_CHV_P2_SHIFT (8) /* 5 bits */
> +#define DPIO_CHV_K_DIV_SHIFT (4)
> +#define DPIO_PLL_LOCK 0x1

(1 << 0) for consistency.

> +#define DPIO_PLL_FREQLOCK 0x2

(1 << 1)

> +#define CHV_CMN_DW13(ch) _PIPE(ch, _CHV_CMN_DW13_CH0, _CHV_CMN_DW0_CH1)
> +
> +#define _CHV_CMN_DW14_CH0 0x8138
> +#define _CHV_CMN_DW1_CH1 0x8084
> +#define DPIO_AFC_RECAL (1 << 14)
> +#define DPIO_DCLKP_EN (1 << 13)
> +#define CHV_CMN_DW14(ch) _PIPE(ch, _CHV_CMN_DW14_CH0, _CHV_CMN_DW1_CH1)
> +
> +#define CHV_CMN_DW30 0x8178
> +#define DPIO_LRC_BYPASS (1 << 3)
> +
> +#define _TXLANE(ch, lane, offset) ((ch ? 0x2400 : 0) + \
> + (lane) * 0x200 + (offset))

This works too, but according to the PHY spec the base addresses would
be more precisely written as
((ch ? 0x2480 : 0x80) + (lane) * 0x200 + (offset))

... and then

> +
> +#define CHV_TX_DW11(ch, lane) _TXLANE(ch, lane, 0xac)

_TXLANE(ch, lane, 0x2c)

> +#define DPIO_FRC_LATENCY_SHFIT (8)
> +#define CHV_TX_DW14(ch, lane) _TXLANE(ch, lane, 0xb8)

_TXLANE(ch, lane, 0x38)

--Imre

> +#define DPIO_UPAR_SHIFT (30)

> /*
> * Fence registers
> */
> @@ -1383,6 +1452,7 @@ enum punit_power_well {
> #define DPLL_LOCK_VLV (1<<15)
> #define DPLL_INTEGRATED_CRI_CLK_VLV (1<<14)
> #define DPLL_INTEGRATED_CLOCK_VLV (1<<13)
> +#define DPLL_SSC_REF_CLOCK_CHV (1<<13)
> #define DPLL_PORTC_READY_MASK (0xf << 4)
> #define DPLL_PORTB_READY_MASK (0xf)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d73fec5..36d6e212 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1549,6 +1549,49 @@ static void vlv_enable_pll(struct intel_crtc *crtc)
> udelay(150); /* wait for warmup */
> }
>
> +static void chv_enable_pll(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int pipe = crtc->pipe;
> + enum dpio_channel port = vlv_pipe_to_channel(pipe);
> + int dpll = DPLL(crtc->pipe);
> + u32 tmp;
> +
> + assert_pipe_disabled(dev_priv, crtc->pipe);
> +
> + BUG_ON(!IS_CHERRYVIEW(dev_priv->dev));
> +
> + mutex_lock(&dev_priv->dpio_lock);
> +
> + /* Enable back the 10bit clock to display controller */
> + tmp = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port));
> + tmp |= DPIO_DCLKP_EN;
> + vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW14(port), tmp);
> +
> + /*
> + * Need to wait > 100ns between dclkp clock enable bit and PLL enable.
> + */
> + udelay(1);
> +
> + /* Enable PLL */
> + tmp = I915_READ(dpll);
> + tmp |= DPLL_VCO_ENABLE;
> + I915_WRITE(dpll, tmp);
> +
> + /* Check PLL is locked */
> + if (wait_for(((I915_READ(dpll) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
> + DRM_ERROR("PLL %d failed to lock\n", pipe);
> +
> + /* Deassert soft data lane reset*/
> + tmp = vlv_dpio_read(dev_priv, pipe, VLV_PCS_DW0(port));
> + tmp |= (DPIO_PCS_TX_LANE2_RESET | DPIO_PCS_TX_LANE1_RESET);
> + vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW0(port), tmp);
> +
> +
> + mutex_unlock(&dev_priv->dpio_lock);
> +}
> +
> static void i9xx_enable_pll(struct intel_crtc *crtc)
> {
> struct drm_device *dev = crtc->base.dev;
> @@ -4503,8 +4546,12 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
> is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI);
>
> - if (!is_dsi)
> - vlv_enable_pll(intel_crtc);
> + if (!is_dsi) {
> + if (IS_CHERRYVIEW(dev))
> + chv_enable_pll(intel_crtc);
> + else
> + vlv_enable_pll(intel_crtc);
> + }
>
> for_each_encoder_on_crtc(dev, crtc, encoder)
> if (encoder->pre_enable)
> @@ -5385,6 +5432,86 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> mutex_unlock(&dev_priv->dpio_lock);
> }
>
> +static void chv_update_pll(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int pipe = crtc->pipe;
> + int dpll_reg = DPLL(crtc->pipe);
> + enum dpio_channel port = vlv_pipe_to_channel(pipe);
> + u32 val, loopfilter, intcoeff;
> + u32 bestn, bestm1, bestm2, bestp1, bestp2, bestm2_frac;
> + int refclk;
> +
> + mutex_lock(&dev_priv->dpio_lock);
> +
> + bestn = crtc->config.dpll.n;
> + bestm2_frac = crtc->config.dpll.m2 & 0x3fffff;
> + bestm1 = crtc->config.dpll.m1;
> + bestm2 = crtc->config.dpll.m2 >> 22;
> + bestp1 = crtc->config.dpll.p1;
> + bestp2 = crtc->config.dpll.p2;
> +
> + /*
> + * Enable Refclk and SSC
> + */
> + val = I915_READ(dpll_reg);
> + val |= (DPLL_SSC_REF_CLOCK_CHV | DPLL_REFA_CLK_ENABLE_VLV);
> + I915_WRITE(dpll_reg, val);
> +
> + /* Propagate soft reset to data lane reset */
> + val = vlv_dpio_read(dev_priv, pipe, VLV_PCS_DW0(port));
> + val &= ~(DPIO_PCS_TX_LANE2_RESET | DPIO_PCS_TX_LANE1_RESET);
> + vlv_dpio_write(dev_priv, pipe, VLV_PCS_DW0(port), val);
> +
> + /* Disable 10bit clock to display controller */
> + val = vlv_dpio_read(dev_priv, pipe, CHV_CMN_DW14(port));
> + val &= ~DPIO_DCLKP_EN;
> + vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW14(port), val);
> +
> + /* p1 and p2 divider */
> + vlv_dpio_write(dev_priv, pipe, CHV_CMN_DW13(port),
> + 5 << DPIO_CHV_S1_DIV_SELECT |
> + bestp1 << DPIO_CHV_P1_SHIFT |
> + bestp2 << DPIO_CHV_P2_SHIFT |
> + 1 << DPIO_CHV_K_DIV_SHIFT);
> +
> + /* Feedback post-divider - m2 */
> + vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW0(port), bestm2);
> +
> + /* Feedback refclk divider - n and m1 */
> + vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW1(port),
> + 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);
> +
> + /* M2 fraction division enable */
> + vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW3(port),
> + DPIO_CHV_FRAC_DIV_EN |
> + DPIO_CHV_FEEDFWD_GAIN);
> +
> + /* Loop filter */
> + refclk = i9xx_get_refclk(&crtc->base, 0);
> + loopfilter = DPIO_CHV_PROP_COEFF | DPIO_CHV_GAIN_CTRL;
> + if (refclk == 100000)
> + intcoeff = 11;
> + else if (refclk == 38400)
> + intcoeff = 10;
> + else
> + intcoeff = 9;
> + loopfilter |= (intcoeff << DPIO_CHV_INT_COEFF_SHIFT);
> + vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW6(port), loopfilter);
> +
> + /* 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);
> +
> + mutex_unlock(&dev_priv->dpio_lock);
> +}
> +
> static void i9xx_update_pll(struct intel_crtc *crtc,
> intel_clock_t *reduced_clock,
> int num_connectors)
> @@ -5768,6 +5895,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> i8xx_update_pll(intel_crtc,
> has_reduced_clock ? &reduced_clock : NULL,
> num_connectors);
> + } else if (IS_CHERRYVIEW(dev)) {
> + chv_update_pll(intel_crtc);
> } else if (IS_VALLEYVIEW(dev)) {
> vlv_update_pll(intel_crtc);
> } else {