Re: [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> For now there are no callers, but these functions are going to be
> needed for the code that allows Package C8+. Other future features may
> also require this code.
> 

The thing that's missing from the patches is any sort of assertions
about things being on before the disable sequence. Is this something we
don't need to address?

> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++
>  drivers/gpu/drm/i915/intel_display.c | 95 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  3 files changed, 105 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be6164f..8e5a5ec 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4930,7 +4930,14 @@
>  #define  LCPLL_CLK_FREQ_450		(0<<26)
>  #define  LCPLL_CD_CLOCK_DISABLE		(1<<25)
>  #define  LCPLL_CD2X_CLOCK_DISABLE	(1<<23)
> +#define  LCPLL_POWER_DOWN_ALLOW		(1<<22)
>  #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
> +#define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)

Hmm... the doc I am looking at says

> +
> +#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> +#define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
> +#define  D_COMP_COMP_FORCE		(1<<8)
> +#define  D_COMP_COMP_DISABLE		(1<<0)
>  
>  /* Pipe WM_LINETIME - watermark line time */
>  #define PIPE_WM_LINETIME_A		0x45270
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 059c9a8..ffb08bf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	return true;
>  }
>  
> +/*
> + * This function implements pieces of two sequences from BSpec:
> + * - Sequence for display software to disable LCPLL
> + * - Sequence for display software to allow package C8+
> + * The steps implemented here are just the steps that actually touch the LCPLL
> + * register. Callers should take care of disabling all the display engine
> + * functions, doing the mode unset, fixing interrupts, etc.
> + */
> +void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> +		       bool switch_to_fclk, bool allow_power_down)
> +{
> +	uint32_t val;
> +
> +	val = I915_READ(LCPLL_CTL);
> +
> +	if (switch_to_fclk) {
> +		val |= LCPLL_CD_SOURCE_FCLK;
> +		I915_WRITE(LCPLL_CTL, val);
> +		POSTING_READ(LCPLL_CTL);
> +
> +		udelay(1);
> +
> +		val = I915_READ(LCPLL_CTL);
> +		if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
> +			DRM_ERROR("Switching to FCLK failed\n");

wait_for_us(..., 1)?

> +	}
> +
> +	val |= LCPLL_PLL_DISABLE;
> +	I915_WRITE(LCPLL_CTL, val);
> +	POSTING_READ(LCPLL_CTL);
> +
> +	if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
> +		DRM_ERROR("LCPLL still locked\n");
> +
> +	val = I915_READ(D_COMP);
> +	val |= D_COMP_COMP_DISABLE;
> +	I915_WRITE(D_COMP, val);
> +	POSTING_READ(D_COMP);
> +
> +	udelay(2);

ndelay(100)?

> +
> +	val = I915_READ(D_COMP);
> +	if (val & D_COMP_RCOMP_IN_PROGRESS)
> +		DRM_ERROR("D_COMP RCOMP still in progress\n");

wait_for(..., 1)?

> +
> +	if (allow_power_down) {
> +		val = I915_READ(LCPLL_CTL);
> +		val |= LCPLL_POWER_DOWN_ALLOW;
> +		I915_WRITE(LCPLL_CTL, val);
> +		POSTING_READ(LCPLL_CTL);
> +	}
> +}
> +
> +/*
> + * Fully restores LCPLL, disallowing power down and switching back to LCPLL
> + * source.
> + */
> +void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> +{
> +	uint32_t val;
> +
> +	val = I915_READ(LCPLL_CTL);
> +

I think we could potentially exit early here if the PLL is already
locked, and we're on CDclk. And indeed, I've already seen this case
occur, but I'm not sure I will ever see that case again.

> +	if (val & LCPLL_POWER_DOWN_ALLOW) {
> +		val &= ~LCPLL_POWER_DOWN_ALLOW;
> +		I915_WRITE(LCPLL_CTL, val);
> +	}
> +
> +	val = I915_READ(D_COMP);
> +	val |= D_COMP_COMP_FORCE;
> +	val &= ~D_COMP_COMP_DISABLE;
> +	I915_WRITE(D_COMP, val);
> +

I think you need a posting read here. I am not sure we're allowed to
read LCPLL_CTL until we know the write has landed.


> +	val = I915_READ(LCPLL_CTL);
> +	val &= ~LCPLL_PLL_DISABLE;
> +	I915_WRITE(LCPLL_CTL, val);
> +	POSTING_READ(LCPLL_CTL);
        ^ unnecessary POSTING_READ - but meh
> +
> +	if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
> +		DRM_ERROR("LCPLL not locked yet\n");
> +
> +	if (val & LCPLL_CD_SOURCE_FCLK) {
> +		val = I915_READ(LCPLL_CTL);
> +		val &= ~LCPLL_CD_SOURCE_FCLK;
> +		I915_WRITE(LCPLL_CTL, val);
> +		POSTING_READ(LCPLL_CTL);
> +
> +		udelay(1);
> +
> +		val = I915_READ(LCPLL_CTL);
> +		if (val & LCPLL_CD_SOURCE_FCLK_DONE)
> +			DRM_ERROR("Switching back to LCPLL failed\n");
> +	}
> +}
> +
>  static void haswell_modeset_global_resources(struct drm_device *dev)
>  {
>  	bool enable = false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5dfc1a0..15989d1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -832,5 +832,8 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>  extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
>  						 enum transcoder pch_transcoder,
>  						 bool enable);
> +extern void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> +			      bool switch_to_fclk, bool allow_power_down);
> +extern void hsw_restore_lcpll(struct drm_i915_private *dev_priv);
>  
>  #endif /* __INTEL_DRV_H__ */

I'm a bit torn as to whether or not it makes sense to extract the pure
LCPLL disable from hsw_disable_lcpll. Did you think about this, could
you explain the reason you decided against it? (I'm a bit partial since
that was the way I had written it).

Does it every make sense to switch to fclk and not allow_power_down?

-- 
Ben Widawsky, Intel Open Source Technology Center