Re: [PATCH v4 30/49] drm/i915/bxt: add display initialize/uninitialize sequence (CDCLK)

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 15, 2015 at 04:42:56PM +0300, Imre Deak wrote:
> From: Vandana Kannan <vandana.kannan@intel.com>
> 
> Add CDCLK specific display clock initialization sequence as per BSpec.
> 
> Note that the CDCLK initialization/uninitialization are done at their
> current place only for simplicity, in a future patch - when more of the
> runtime PM features will be enabled - these will be moved to power
> well#1 and modeset encoder enabling/disabling hooks respectively. This
> also means that atm dynamic power gating power well #1 is effectively
> disabled.
> 
> The call to uninitialize CDCLK during system/runtime suspend will be
> added later in this patchset.
> 
> v1: Added function definitions in header files
> v2: Imre's review comments addressed
> - Moved CDCLK related definitions to i915_reg.h
> - Removed defintions for CDCLK frequency
> - Split uninit_cdclk() by adding a phy_uninit function
> - Calculate freq and decimal based on input frequency
> - Program SSA precharge based on input frequency
> - Use wait_for 1ms instead 200us udelay for DE PLL locking
> - Removed initial value for divider, freq, decimal, ratio.
> - Replaced polling loops with wait_for
> - Parameterized latency optim setting
> - Fix the parts where DE PLL has to be disabled.
> - Call CDCLK selection from mode set
> 
> v3: (imre)
> - add note about the plan to move the cdclk/phy init to a better place
> - take rps.hw_lock around pcode access
> - move DE PLL register macros here from another patch since they are
>   used here first
> - add BXT_ prefix to CDCLK flags
> - add missing masking when programming CDCLK_FREQ_DECIMAL
> 
> v4: (ville)
> - split the CDCLK/PHY parts into two patches, update commit message
>   accordingly
> - s/DISPLAY_PCU_CONTROL/HSW_PCODE_DE_WRITE_FREQ_REQ/
> - simplify BXT_DE_PLL_RATIO macros
> - fix BXT_DE_PLL_RATIO_MASK
> - s/bxt_select_cdclk_freq/broxton_set_cdclk_freq/
> - move cdclk init/uninit/set code from intel_ddi.c to intel_display.c
> - remove redundant code comments for broxton_set_cdclk_freq()
> - sanitize fixed point<->integer frequency value conversion
> - use DRM_ERROR instead of WARN
> - do RMW when programming BXT_DE_PLL_CTL for safety
> - add note about PLL lock timeout being exactly 200us
> - make PCU error messages more descriptive
> - instead of using 0 freq to mean PLL off/bypass freq use 19200
>   for clarity, as the latter one is the actual rate
> - simplify pcode programming, removing duplicated
>   sandybridge_pcode_write() call
> - sanitize code flow, remove unnecessary scratch vars in
>   broxton_set_cdclk() (imre)
> - Remove bound check for maxmimum freq to match current code.
>   This check will be added later at a more proper platform
>   independent place once atomic support lands.
> - add note to remove freq guard band which isn't needed on BXT
> - add note to reduce freq to minimum if no pipe is enabled
> - combine broxton_modeset_global_pipes() with
>   valleyview_modeset_global_pipes()
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> (v2)
> Signed-off-by: Imre Deak <imre.deak@intel.com>

That's quite a changelog I caused. Sorry :)

But I like how it's looking now. A few minor FIXMEs in there, but I
agree that those can be dealt with later.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  20 ++++
>  drivers/gpu/drm/i915/intel_ddi.c     |   2 +
>  drivers/gpu/drm/i915/intel_display.c | 226 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  4 files changed, 248 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4b53b20..c79bf8d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5452,6 +5452,9 @@ enum skl_disp_power_wells {
>  #define  DISP_FBC_WM_DIS		(1<<15)
>  #define DISP_ARB_CTL2	0x45004
>  #define  DISP_DATA_PARTITION_5_6	(1<<6)
> +#define DBUF_CTL	0x45008
> +#define  DBUF_POWER_REQUEST		(1<<31)
> +#define  DBUF_POWER_STATE		(1<<30)
>  #define GEN7_MSG_CTL	0x45010
>  #define  WAIT_FOR_PCH_RESET_ACK		(1<<1)
>  #define  WAIT_FOR_PCH_FLR_ACK		(1<<0)
> @@ -6403,6 +6406,7 @@ enum skl_disp_power_wells {
>  #define   GEN6_PCODE_WRITE_D_COMP		0x11
>  #define   GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
>  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> +#define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
>  #define GEN6_PCODE_DATA				0x138128
> @@ -6874,6 +6878,13 @@ enum skl_disp_power_wells {
>  #define  CDCLK_FREQ_675_617		(3<<26)
>  #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
>  
> +#define  BXT_CDCLK_CD2X_DIV_SEL_MASK	(3<<22)
> +#define  BXT_CDCLK_CD2X_DIV_SEL_1	(0<<22)
> +#define  BXT_CDCLK_CD2X_DIV_SEL_1_5	(1<<22)
> +#define  BXT_CDCLK_CD2X_DIV_SEL_2	(2<<22)
> +#define  BXT_CDCLK_CD2X_DIV_SEL_4	(3<<22)
> +#define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1<<16)
> +
>  /* LCPLL_CTL */
>  #define LCPLL1_CTL		0x46010
>  #define LCPLL2_CTL		0x46014
> @@ -6938,6 +6949,15 @@ enum skl_disp_power_wells {
>  #define GET_CFG_CR1_REG(id) (DPLL1_CFGCR1 + (id - SKL_DPLL1) * 8)
>  #define GET_CFG_CR2_REG(id) (DPLL1_CFGCR2 + (id - SKL_DPLL1) * 8)
>  
> +/* BXT display engine PLL */
> +#define BXT_DE_PLL_CTL			0x6d000
> +#define   BXT_DE_PLL_RATIO(x)		(x)	/* {60,65,100} * 19.2MHz */
> +#define   BXT_DE_PLL_RATIO_MASK		0xff
> +
> +#define BXT_DE_PLL_ENABLE		0x46070
> +#define   BXT_DE_PLL_PLL_ENABLE		(1 << 31)
> +#define   BXT_DE_PLL_LOCK		(1 << 30)
> +
>  /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
>   * since on HSW we can't write to it using I915_WRITE. */
>  #define D_COMP_HSW			(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5b50484..25d697b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1880,6 +1880,8 @@ void intel_ddi_pll_init(struct drm_device *dev)
>  	if (IS_SKYLAKE(dev)) {
>  		if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>  			DRM_ERROR("LCPLL1 is disabled\n");
> +	} else if (IS_BROXTON(dev)) {
> +		broxton_init_cdclk(dev);
>  	} else {
>  		/*
>  		 * The LCPLL register should be turned on by the BIOS. For now
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bb5f2a5..5ee5d8c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5196,6 +5196,181 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>  	intel_display_set_init_power(dev_priv, false);
>  }
>  
> +void broxton_set_cdclk(struct drm_device *dev, int frequency)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t divider;
> +	uint32_t ratio;
> +	uint32_t current_freq;
> +	int ret;
> +
> +	/* frequency = 19.2MHz * ratio / 2 / div{1,1.5,2,4} */
> +	switch (frequency) {
> +	case 144000:
> +		divider = BXT_CDCLK_CD2X_DIV_SEL_4;
> +		ratio = BXT_DE_PLL_RATIO(60);
> +		break;
> +	case 288000:
> +		divider = BXT_CDCLK_CD2X_DIV_SEL_2;
> +		ratio = BXT_DE_PLL_RATIO(60);
> +		break;
> +	case 384000:
> +		divider = BXT_CDCLK_CD2X_DIV_SEL_1_5;
> +		ratio = BXT_DE_PLL_RATIO(60);
> +		break;
> +	case 576000:
> +		divider = BXT_CDCLK_CD2X_DIV_SEL_1;
> +		ratio = BXT_DE_PLL_RATIO(60);
> +		break;
> +	case 624000:
> +		divider = BXT_CDCLK_CD2X_DIV_SEL_1;
> +		ratio = BXT_DE_PLL_RATIO(65);
> +		break;
> +	case 19200:
> +		/*
> +		 * Bypass frequency with DE PLL disabled. Init ratio, divider
> +		 * to suppress GCC warning.
> +		 */
> +		ratio = 0;
> +		divider = 0;
> +		break;
> +	default:
> +		DRM_ERROR("unsupported CDCLK freq %d", frequency);
> +
> +		return;
> +	}
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	/* Inform power controller of upcoming frequency change */
> +	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> +				      0x80000000);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret) {
> +		DRM_ERROR("PCode CDCLK freq change notify failed (err %d, freq %d)\n",
> +			  ret, frequency);
> +		return;
> +	}
> +
> +	current_freq = I915_READ(CDCLK_CTL) & CDCLK_FREQ_DECIMAL_MASK;
> +	/* convert from .1 fixpoint MHz with -1MHz offset to kHz */
> +	current_freq = current_freq * 500 + 1000;
> +
> +	/*
> +	 * DE PLL has to be disabled when
> +	 * - setting to 19.2MHz (bypass, PLL isn't used)
> +	 * - before setting to 624MHz (PLL needs toggling)
> +	 * - before setting to any frequency from 624MHz (PLL needs toggling)
> +	 */
> +	if (frequency == 19200 || frequency == 624000 ||
> +	    current_freq == 624000) {
> +		I915_WRITE(BXT_DE_PLL_ENABLE, ~BXT_DE_PLL_PLL_ENABLE);
> +		/* Timeout 200us */
> +		if (wait_for(!(I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK),
> +			     1))
> +			DRM_ERROR("timout waiting for DE PLL unlock\n");
> +	}
> +
> +	if (frequency != 19200) {
> +		uint32_t val;
> +
> +		val = I915_READ(BXT_DE_PLL_CTL);
> +		val &= ~BXT_DE_PLL_RATIO_MASK;
> +		val |= ratio;
> +		I915_WRITE(BXT_DE_PLL_CTL, val);
> +
> +		I915_WRITE(BXT_DE_PLL_ENABLE, BXT_DE_PLL_PLL_ENABLE);
> +		/* Timeout 200us */
> +		if (wait_for(I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK, 1))
> +			DRM_ERROR("timeout waiting for DE PLL lock\n");
> +
> +		val = I915_READ(CDCLK_CTL);
> +		val &= ~BXT_CDCLK_CD2X_DIV_SEL_MASK;
> +		val |= divider;
> +		/*
> +		 * Disable SSA Precharge when CD clock frequency < 500 MHz,
> +		 * enable otherwise.
> +		 */
> +		val &= ~BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> +		if (frequency >= 500000)
> +			val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> +
> +		val &= ~CDCLK_FREQ_DECIMAL_MASK;
> +		/* convert from kHz to .1 fixpoint MHz with -1MHz offset */
> +		val |= (frequency - 1000) / 500;
> +		I915_WRITE(CDCLK_CTL, val);
> +	}
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	ret = sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> +				      DIV_ROUND_UP(frequency, 25000));
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret) {
> +		DRM_ERROR("PCode CDCLK freq set failed, (err %d, freq %d)\n",
> +			  ret, frequency);
> +		return;
> +	}
> +
> +	dev_priv->cdclk_freq = frequency;
> +}
> +
> +void broxton_init_cdclk(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t val;
> +
> +	/*
> +	 * NDE_RSTWRN_OPT RST PCH Handshake En must always be 0b on BXT
> +	 * or else the reset will hang because there is no PCH to respond.
> +	 * Move the handshake programming to initialization sequence.
> +	 * Previously was left up to BIOS.
> +	 */
> +	val = I915_READ(HSW_NDE_RSTWRN_OPT);
> +	val &= ~RESET_PCH_HANDSHAKE_ENABLE;
> +	I915_WRITE(HSW_NDE_RSTWRN_OPT, val);
> +
> +	/* Enable PG1 for cdclk */
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
> +	/* check if cd clock is enabled */
> +	if (I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_PLL_ENABLE) {
> +		DRM_DEBUG_KMS("Display already initialized\n");
> +		return;
> +	}
> +
> +	/*
> +	 * FIXME:
> +	 * - The initial CDCLK needs to be read from VBT.
> +	 *   Need to make this change after VBT has changes for BXT.
> +	 * - check if setting the max (or any) cdclk freq is really necessary
> +	 *   here, it belongs to modeset time
> +	 */
> +	broxton_set_cdclk(dev, 624000);
> +
> +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> +	udelay(10);
> +
> +	if (!(I915_READ(DBUF_CTL) & DBUF_POWER_STATE))
> +		DRM_ERROR("DBuf power enable timeout!\n");
> +}
> +
> +void broxton_uninit_cdclk(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) & ~DBUF_POWER_REQUEST);
> +	udelay(10);
> +
> +	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> +		DRM_ERROR("DBuf power disable timeout!\n");
> +
> +	/* Set minimum (bypass) frequency, in effect turning off the DE PLL */
> +	broxton_set_cdclk(dev, 19200);
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> +}
> +
>  /* returns HPLL frequency in kHz */
>  static int valleyview_get_vco(struct drm_i915_private *dev_priv)
>  {
> @@ -5363,6 +5538,26 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
>  		return 200000;
>  }
>  
> +static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> +			      int max_pixclk)
> +{
> +	/*
> +	 * FIXME:
> +	 * - remove the guardband, it's not needed on BXT
> +	 * - set 19.2MHz bypass frequency if there are no active pipes
> +	 */
> +	if (max_pixclk > 576000*9/10)
> +		return 624000;
> +	else if (max_pixclk > 384000*9/10)
> +		return 576000;
> +	else if (max_pixclk > 288000*9/10)
> +		return 384000;
> +	else if (max_pixclk > 144000*9/10)
> +		return 288000;
> +	else
> +		return 144000;
> +}
> +
>  /* compute the max pixel clock for new configuration */
>  static int intel_mode_max_pixclk(struct drm_atomic_state *state)
>  {
> @@ -5392,12 +5587,17 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state,
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_crtc *intel_crtc;
>  	int max_pixclk = intel_mode_max_pixclk(state);
> +	int cdclk;
>  
>  	if (max_pixclk < 0)
>  		return max_pixclk;
>  
> -	if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
> -	    dev_priv->cdclk_freq)
> +	if (IS_VALLEYVIEW(dev_priv))
> +		cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
> +	else
> +		cdclk = broxton_calc_cdclk(dev_priv, max_pixclk);
> +
> +	if (cdclk == dev_priv->cdclk_freq)
>  		return 0;
>  
>  	/* disable/enable all currently active pipes while we change cdclk */
> @@ -8827,6 +9027,23 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>  	intel_prepare_ddi(dev);
>  }
>  
> +static void broxton_modeset_global_resources(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int max_pixclk = intel_mode_max_pixclk(state);
> +	int req_cdclk;
> +
> +	/* see the comment in valleyview_modeset_global_resources */
> +	if (WARN_ON(max_pixclk < 0))
> +		return;
> +
> +	req_cdclk = broxton_calc_cdclk(dev_priv, max_pixclk);
> +
> +	if (req_cdclk != dev_priv->cdclk_freq)
> +		broxton_set_cdclk(dev, req_cdclk);
> +}
> +
>  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  				      struct intel_crtc_state *crtc_state)
>  {
> @@ -11983,7 +12200,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (IS_VALLEYVIEW(dev)) {
> +	if (IS_VALLEYVIEW(dev) || IS_BROXTON(dev)) {
>  		ret = valleyview_modeset_global_pipes(state, &prepare_pipes);
>  		if (ret)
>  			goto done;
> @@ -14005,6 +14222,9 @@ static void intel_init_display(struct drm_device *dev)
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.modeset_global_resources =
>  			valleyview_modeset_global_resources;
> +	} else if (IS_BROXTON(dev)) {
> +		dev_priv->display.modeset_global_resources =
> +			broxton_modeset_global_resources;
>  	}
>  
>  	switch (INTEL_INFO(dev)->gen) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6a2ee0c..5ba88eb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1112,6 +1112,9 @@ void intel_prepare_reset(struct drm_device *dev);
>  void intel_finish_reset(struct drm_device *dev);
>  void hsw_enable_pc8(struct drm_i915_private *dev_priv);
>  void hsw_disable_pc8(struct drm_i915_private *dev_priv);
> +void broxton_init_cdclk(struct drm_device *dev);
> +void broxton_uninit_cdclk(struct drm_device *dev);
> +void broxton_set_cdclk(struct drm_device *dev, int frequency);
>  void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n);
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx