On Wed, Jun 05, 2013 at 01:34:23PM +0200, Daniel Vetter wrote:
> It's been splattered over 3 different places all doing random things.
> Now we have (mostly) the same sequence as i8xx/i9xx, but all called
> from the crtc_enable hook (through the pll->enable function):
> - write new dividers
> - enable vco and wait for stable clocks
> - write again for the pixel mutliplier
>
> I've left the seemingly random 200 usec delay in there, just in case.
>
> Also move the encoder->pre_pll_enable hook into the crtc_enable
> function, at the same spot we currently have a hack to enable the lvds
> port. Since that hack is now redundant, kill it.
>
> While doing this patch I've learned the hard way that we can only fire
> up the LVDS port if both the pch dpll _and_ the fdi rc pll are not yet
> enabled. Otherwise things go haywire, at least on cpt.
>
> v2: It is paramount to write the FPx divisors before we enable the
> the vco by writing to the DPLL registers, for otherwise the divisors
> won't get updated. This is in line with the i8xx/i9xx dpll.
>
> v3: To keep the nice abstraction add a ->mode_set callback to set the
> divisors. Also streamline the enabling/disabling code a bit by
> removing some cargo-cult duplication and clearing registers where
> possible in the ->disable hook.
>
> v4: Remove now unused local variable.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
For some reason my brain refuses to work on this patch. I say, let's
test it in the wild instead.
Acked-by: Damien Lespiau <damien.lespiau at intel.com>
--
Damien
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/intel_display.c | 75 +++++++++++++-----------------------
> 2 files changed, 29 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4dc94ed..9fc1ea4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -157,6 +157,8 @@ struct intel_shared_dpll {
> /* should match the index in the dev_priv->shared_dplls array */
> enum intel_dpll_id id;
> struct intel_dpll_hw_state hw_state;
> + void (*mode_set)(struct drm_i915_private *dev_priv,
> + struct intel_shared_dpll *pll);
> void (*enable)(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll);
> void (*disable)(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc1b5f7..334f86a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3064,13 +3064,7 @@ found:
> WARN_ON(pll->on);
> assert_shared_dpll_disabled(dev_priv, pll);
>
> - /* Wait for the clocks to stabilize before rewriting the regs */
> - I915_WRITE(PCH_DPLL(pll->id), dpll & ~DPLL_VCO_ENABLE);
> - POSTING_READ(PCH_DPLL(pll->id));
> - udelay(150);
> -
> - I915_WRITE(PCH_FP0(pll->id), fp);
> - I915_WRITE(PCH_DPLL(pll->id), dpll & ~DPLL_VCO_ENABLE);
> + pll->mode_set(dev_priv, pll);
> }
> pll->refcount++;
>
> @@ -3120,7 +3114,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> struct intel_encoder *encoder;
> int pipe = intel_crtc->pipe;
> int plane = intel_crtc->plane;
> - u32 temp;
>
> WARN_ON(!crtc->enabled);
>
> @@ -3134,12 +3127,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>
> intel_update_watermarks(dev);
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> - temp = I915_READ(PCH_LVDS);
> - if ((temp & LVDS_PORT_EN) == 0)
> - I915_WRITE(PCH_LVDS, temp | LVDS_PORT_EN);
> - }
> -
> + for_each_encoder_on_crtc(dev, crtc, encoder)
> + if (encoder->pre_pll_enable)
> + encoder->pre_pll_enable(encoder);
>
> if (intel_crtc->config.has_pch_encoder) {
> /* Note: FDI PLL enabling _must_ be done before we enable the
> @@ -5682,10 +5672,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> if (intel_crtc->config.has_dp_encoder)
> intel_dp_set_m_n(intel_crtc);
>
> - for_each_encoder_on_crtc(dev, crtc, encoder)
> - if (encoder->pre_pll_enable)
> - encoder->pre_pll_enable(encoder);
> -
> if (is_lvds && has_reduced_clock && i915_powersave)
> intel_crtc->lowfreq_avail = true;
> else
> @@ -5694,23 +5680,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> if (intel_crtc->config.has_pch_encoder) {
> pll = intel_crtc_to_shared_dpll(intel_crtc);
>
> - I915_WRITE(PCH_DPLL(pll->id), dpll);
> -
> - /* Wait for the clocks to stabilize. */
> - POSTING_READ(PCH_DPLL(pll->id));
> - udelay(150);
> -
> - /* The pixel multiplier can only be updated once the
> - * DPLL is enabled and the clocks are stable.
> - *
> - * So write it again.
> - */
> - I915_WRITE(PCH_DPLL(pll->id), dpll);
> -
> - if (has_reduced_clock)
> - I915_WRITE(PCH_FP1(pll->id), fp2);
> - else
> - I915_WRITE(PCH_FP1(pll->id), fp);
> }
>
> intel_set_pipe_timings(intel_crtc);
> @@ -8735,19 +8704,32 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> return val & DPLL_VCO_ENABLE;
> }
>
> +static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> + struct intel_shared_dpll *pll)
> +{
> + I915_WRITE(PCH_FP0(pll->id), pll->hw_state.fp0);
> + I915_WRITE(PCH_FP1(pll->id), pll->hw_state.fp1);
> +}
> +
> static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
> struct intel_shared_dpll *pll)
> {
> - uint32_t reg, val;
> -
> /* PCH refclock must be enabled first */
> assert_pch_refclk_enabled(dev_priv);
>
> - reg = PCH_DPLL(pll->id);
> - val = I915_READ(reg);
> - val |= DPLL_VCO_ENABLE;
> - I915_WRITE(reg, val);
> - POSTING_READ(reg);
> + I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
> +
> + /* Wait for the clocks to stabilize. */
> + POSTING_READ(PCH_DPLL(pll->id));
> + udelay(150);
> +
> + /* The pixel multiplier can only be updated once the
> + * DPLL is enabled and the clocks are stable.
> + *
> + * So write it again.
> + */
> + I915_WRITE(PCH_DPLL(pll->id), pll->hw_state.dpll);
> + POSTING_READ(PCH_DPLL(pll->id));
> udelay(200);
> }
>
> @@ -8756,7 +8738,6 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv,
> {
> struct drm_device *dev = dev_priv->dev;
> struct intel_crtc *crtc;
> - uint32_t reg, val;
>
> /* Make sure no transcoder isn't still depending on us. */
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> @@ -8764,11 +8745,8 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv,
> assert_pch_transcoder_disabled(dev_priv, crtc->pipe);
> }
>
> - reg = PCH_DPLL(pll->id);
> - val = I915_READ(reg);
> - val &= ~DPLL_VCO_ENABLE;
> - I915_WRITE(reg, val);
> - POSTING_READ(reg);
> + I915_WRITE(PCH_DPLL(pll->id), 0);
> + POSTING_READ(PCH_DPLL(pll->id));
> udelay(200);
> }
>
> @@ -8787,6 +8765,7 @@ static void ibx_pch_dpll_init(struct drm_device *dev)
> for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> dev_priv->shared_dplls[i].id = i;
> dev_priv->shared_dplls[i].name = ibx_pch_dpll_names[i];
> + dev_priv->shared_dplls[i].mode_set = ibx_pch_dpll_mode_set;
> dev_priv->shared_dplls[i].enable = ibx_pch_dpll_enable;
> dev_priv->shared_dplls[i].disable = ibx_pch_dpll_disable;
> dev_priv->shared_dplls[i].get_hw_state =
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
