Re: [PATCH 3/8] drm/i915: Combine bxt_set_cdclk and cnl_set_cdclk

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

On Fri, Sep 06, 2019 at 05:21:38PM -0700, Matt Roper wrote:
> We'd previously combined ICL/TGL logic into the cnl_set_cdclk function,
> but BXT is pretty similar as well.  Roll the cnl/icl/tgl logic back into
> the bxt function; the only things we really need to handle separately
> are punit notification and calling different functions to enable/disable
> the cdclk PLL.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Not sure if all the ifs are getting out of hand or not. But I guess
it's still legible.

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

> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 267 +++++++++------------
>  1 file changed, 119 insertions(+), 148 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 8ac31f8775f0..6b5b1328a3fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1449,6 +1449,39 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
>  	dev_priv->cdclk.hw.vco = vco;
>  }
>  
> +static void cnl_cdclk_pll_disable(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	val = I915_READ(BXT_DE_PLL_ENABLE);
> +	val &= ~BXT_DE_PLL_PLL_ENABLE;
> +	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> +
> +	/* Timeout 200us */
> +	if (wait_for((I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK) == 0, 1))
> +		DRM_ERROR("timeout waiting for CDCLK PLL unlock\n");
> +
> +	dev_priv->cdclk.hw.vco = 0;
> +}
> +
> +static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
> +{
> +	int ratio = DIV_ROUND_CLOSEST(vco, dev_priv->cdclk.hw.ref);
> +	u32 val;
> +
> +	val = CNL_CDCLK_PLL_RATIO(ratio);
> +	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> +
> +	val |= BXT_DE_PLL_PLL_ENABLE;
> +	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> +
> +	/* Timeout 200us */
> +	if (wait_for((I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK) != 0, 1))
> +		DRM_ERROR("timeout waiting for CDCLK PLL lock\n");
> +
> +	dev_priv->cdclk.hw.vco = vco;
> +}
> +
>  static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  			  const struct intel_cdclk_state *cdclk_state,
>  			  enum pipe pipe)
> @@ -1458,6 +1491,27 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  	u32 val, divider;
>  	int ret;
>  
> +	/* Inform power controller of upcoming frequency change. */
> +	if (INTEL_GEN(dev_priv) >= 10)
> +		ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> +					SKL_CDCLK_PREPARE_FOR_CHANGE,
> +					SKL_CDCLK_READY_FOR_CHANGE,
> +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> +	else
> +		/*
> +		 * BSpec requires us to wait up to 150usec, but that leads to
> +		 * timeouts; the 2ms used here is based on experiment.
> +		 */
> +		ret = sandybridge_pcode_write_timeout(dev_priv,
> +						      HSW_PCODE_DE_WRITE_FREQ_REQ,
> +						      0x80000000, 150, 2);
> +
> +	if (ret) {
> +		DRM_ERROR("Failed to inform PCU about cdclk change (err %d, freq %d)\n",
> +			  ret, cdclk);
> +		return;
> +	}
> +
>  	/* cdclk = vco / 2 / div{1,1.5,2,4} */
>  	switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
>  	default:
> @@ -1468,63 +1522,82 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  		divider = BXT_CDCLK_CD2X_DIV_SEL_1;
>  		break;
>  	case 3:
> -		WARN(IS_GEMINILAKE(dev_priv), "Unsupported divider\n");
> +		WARN(IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10,
> +		     "Unsupported divider\n");
>  		divider = BXT_CDCLK_CD2X_DIV_SEL_1_5;
>  		break;
>  	case 4:
>  		divider = BXT_CDCLK_CD2X_DIV_SEL_2;
>  		break;
>  	case 8:
> +		WARN(INTEL_GEN(dev_priv) >= 10, "Unsupported divider\n");
>  		divider = BXT_CDCLK_CD2X_DIV_SEL_4;
>  		break;
>  	}
>  
> -	/*
> -	 * Inform power controller of upcoming frequency change. BSpec
> -	 * requires us to wait up to 150usec, but that leads to timeouts;
> -	 * the 2ms used here is based on experiment.
> -	 */
> -	ret = sandybridge_pcode_write_timeout(dev_priv,
> -					      HSW_PCODE_DE_WRITE_FREQ_REQ,
> -					      0x80000000, 150, 2);
> -	if (ret) {
> -		DRM_ERROR("PCode CDCLK freq change notify failed (err %d, freq %d)\n",
> -			  ret, cdclk);
> -		return;
> -	}
> +	if (INTEL_GEN(dev_priv) >= 10) {
> +		if (dev_priv->cdclk.hw.vco != 0 &&
> +		    dev_priv->cdclk.hw.vco != vco)
> +			cnl_cdclk_pll_disable(dev_priv);
>  
> -	if (dev_priv->cdclk.hw.vco != 0 &&
> -	    dev_priv->cdclk.hw.vco != vco)
> -		bxt_de_pll_disable(dev_priv);
> +		if (dev_priv->cdclk.hw.vco != vco)
> +			cnl_cdclk_pll_enable(dev_priv, vco);
>  
> -	if (dev_priv->cdclk.hw.vco != vco)
> -		bxt_de_pll_enable(dev_priv, vco);
> +	} else {
> +		if (dev_priv->cdclk.hw.vco != 0 &&
> +		    dev_priv->cdclk.hw.vco != vco)
> +			bxt_de_pll_disable(dev_priv);
> +
> +		if (dev_priv->cdclk.hw.vco != vco)
> +			bxt_de_pll_enable(dev_priv, vco);
> +	}
>  
>  	val = divider | skl_cdclk_decimal(cdclk);
> -	if (pipe == INVALID_PIPE)
> -		val |= BXT_CDCLK_CD2X_PIPE_NONE;
> -	else
> -		val |= BXT_CDCLK_CD2X_PIPE(pipe);
> +
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		if (pipe == INVALID_PIPE)
> +			val |= TGL_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= TGL_CDCLK_CD2X_PIPE(pipe);
> +	} else if (INTEL_GEN(dev_priv) >= 11) {
> +		if (pipe == INVALID_PIPE)
> +			val |= ICL_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= ICL_CDCLK_CD2X_PIPE(pipe);
> +	} else {
> +		if (pipe == INVALID_PIPE)
> +			val |= BXT_CDCLK_CD2X_PIPE_NONE;
> +		else
> +			val |= BXT_CDCLK_CD2X_PIPE(pipe);
> +	}
> +
>  	/*
>  	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
>  	 * enable otherwise.
>  	 */
> -	if (cdclk >= 500000)
> +	if (IS_GEN9_LP(dev_priv) && cdclk >= 500000)
>  		val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
>  	I915_WRITE(CDCLK_CTL, val);
>  
>  	if (pipe != INVALID_PIPE)
>  		intel_wait_for_vblank(dev_priv, pipe);
>  
> -	/*
> -	 * The timeout isn't specified, the 2ms used here is based on
> -	 * experiment.
> -	 * FIXME: Waiting for the request completion could be delayed until
> -	 * the next PCODE request based on BSpec.
> -	 */
> -	ret = sandybridge_pcode_write_timeout(dev_priv,
> -					      HSW_PCODE_DE_WRITE_FREQ_REQ,
> -					      cdclk_state->voltage_level, 150, 2);
> +	if (INTEL_GEN(dev_priv) >= 10) {
> +		ret = sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> +					      cdclk_state->voltage_level);
> +	} else {
> +		/*
> +		 * The timeout isn't specified, the 2ms used here is based on
> +		 * experiment.
> +		 * FIXME: Waiting for the request completion could be delayed
> +		 * until the next PCODE request based on BSpec.
> +		 */
> +		ret = sandybridge_pcode_write_timeout(dev_priv,
> +						      HSW_PCODE_DE_WRITE_FREQ_REQ,
> +						      cdclk_state->voltage_level,
> +						      150, 2);
> +	}
> +
>  	if (ret) {
>  		DRM_ERROR("PCode CDCLK freq set failed, (err %d, freq %d)\n",
>  			  ret, cdclk);
> @@ -1532,6 +1605,13 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  	}
>  
>  	intel_update_cdclk(dev_priv);
> +
> +	if (INTEL_GEN(dev_priv) >= 10)
> +		/*
> +		 * Can't read out the voltage level :(
> +		 * Let's just assume everything is as expected.
> +		 */
> +		dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level;
>  }
>  
>  static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
> @@ -1617,115 +1697,6 @@ static void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
> -static void cnl_cdclk_pll_disable(struct drm_i915_private *dev_priv)
> -{
> -	u32 val;
> -
> -	val = I915_READ(BXT_DE_PLL_ENABLE);
> -	val &= ~BXT_DE_PLL_PLL_ENABLE;
> -	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> -
> -	/* Timeout 200us */
> -	if (wait_for((I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK) == 0, 1))
> -		DRM_ERROR("timeout waiting for CDCLK PLL unlock\n");
> -
> -	dev_priv->cdclk.hw.vco = 0;
> -}
> -
> -static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco)
> -{
> -	int ratio = DIV_ROUND_CLOSEST(vco, dev_priv->cdclk.hw.ref);
> -	u32 val;
> -
> -	val = CNL_CDCLK_PLL_RATIO(ratio);
> -	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> -
> -	val |= BXT_DE_PLL_PLL_ENABLE;
> -	I915_WRITE(BXT_DE_PLL_ENABLE, val);
> -
> -	/* Timeout 200us */
> -	if (wait_for((I915_READ(BXT_DE_PLL_ENABLE) & BXT_DE_PLL_LOCK) != 0, 1))
> -		DRM_ERROR("timeout waiting for CDCLK PLL lock\n");
> -
> -	dev_priv->cdclk.hw.vco = vco;
> -}
> -
> -static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> -			  const struct intel_cdclk_state *cdclk_state,
> -			  enum pipe pipe)
> -{
> -	int cdclk = cdclk_state->cdclk;
> -	int vco = cdclk_state->vco;
> -	u32 val, divider;
> -	int ret;
> -
> -	ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> -				SKL_CDCLK_PREPARE_FOR_CHANGE,
> -				SKL_CDCLK_READY_FOR_CHANGE,
> -				SKL_CDCLK_READY_FOR_CHANGE, 3);
> -	if (ret) {
> -		DRM_ERROR("Failed to inform PCU about cdclk change (%d)\n",
> -			  ret);
> -		return;
> -	}
> -
> -	/* cdclk = vco / 2 / div{1,2} */
> -	switch (DIV_ROUND_CLOSEST(vco, cdclk)) {
> -	default:
> -		WARN_ON(cdclk != dev_priv->cdclk.hw.bypass);
> -		WARN_ON(vco != 0);
> -		/* fall through */
> -	case 2:
> -		divider = BXT_CDCLK_CD2X_DIV_SEL_1;
> -		break;
> -	case 4:
> -		divider = BXT_CDCLK_CD2X_DIV_SEL_2;
> -		break;
> -	}
> -
> -	if (dev_priv->cdclk.hw.vco != 0 &&
> -	    dev_priv->cdclk.hw.vco != vco)
> -		cnl_cdclk_pll_disable(dev_priv);
> -
> -	if (dev_priv->cdclk.hw.vco != vco)
> -		cnl_cdclk_pll_enable(dev_priv, vco);
> -
> -	val = divider | skl_cdclk_decimal(cdclk);
> -
> -	if (INTEL_GEN(dev_priv) >= 12) {
> -		if (pipe == INVALID_PIPE)
> -			val |= TGL_CDCLK_CD2X_PIPE_NONE;
> -		else
> -			val |= TGL_CDCLK_CD2X_PIPE(pipe);
> -	} else if (INTEL_GEN(dev_priv) >= 11) {
> -		if (pipe == INVALID_PIPE)
> -			val |= ICL_CDCLK_CD2X_PIPE_NONE;
> -		else
> -			val |= ICL_CDCLK_CD2X_PIPE(pipe);
> -	} else {
> -		if (pipe == INVALID_PIPE)
> -			val |= BXT_CDCLK_CD2X_PIPE_NONE;
> -		else
> -			val |= BXT_CDCLK_CD2X_PIPE(pipe);
> -	}
> -	I915_WRITE(CDCLK_CTL, val);
> -
> -	if (pipe != INVALID_PIPE)
> -		intel_wait_for_vblank(dev_priv, pipe);
> -
> -	/* inform PCU of the change */
> -	sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> -				cdclk_state->voltage_level);
> -
> -	intel_update_cdclk(dev_priv);
> -
> -	/*
> -	 * Can't read out the voltage level :(
> -	 * Let's just assume everything is as expected.
> -	 */
> -	dev_priv->cdclk.hw.voltage_level = cdclk_state->voltage_level;
> -}
> -
>  static void cnl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	u32 cdctl, expected;
> @@ -1806,7 +1777,7 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv)
>  		sanitized_state.voltage_level =
>  			icl_calc_voltage_level(sanitized_state.cdclk);
>  
> -	cnl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
> +	bxt_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
>  }
>  
>  static void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
> @@ -1822,7 +1793,7 @@ static void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  		cdclk_state.voltage_level =
>  			icl_calc_voltage_level(cdclk_state.cdclk);
>  
> -	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
>  static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
> @@ -1841,7 +1812,7 @@ static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
>  	cdclk_state.vco = calc_cdclk_pll_vco(dev_priv, cdclk_state.cdclk);
>  	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
>  
> -	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
>  static void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> @@ -1852,7 +1823,7 @@ static void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
>  	cdclk_state.vco = 0;
>  	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
>  
> -	cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> +	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
>  /**
> @@ -2655,12 +2626,12 @@ void intel_update_rawclk(struct drm_i915_private *dev_priv)
>  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
>  {
>  	if (INTEL_GEN(dev_priv) >= 11) {
> -		dev_priv->display.set_cdclk = cnl_set_cdclk;
> +		dev_priv->display.set_cdclk = bxt_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk;
>  		dev_priv->cdclk.table = icl_cdclk_table;
>  		dev_priv->cdclk.table_size = ARRAY_SIZE(icl_cdclk_table);
>  	} else if (IS_CANNONLAKE(dev_priv)) {
> -		dev_priv->display.set_cdclk = cnl_set_cdclk;
> +		dev_priv->display.set_cdclk = bxt_set_cdclk;
>  		dev_priv->display.modeset_calc_cdclk = cnl_modeset_calc_cdclk;
>  		dev_priv->cdclk.table = cnl_cdclk_table;
>  		dev_priv->cdclk.table_size = ARRAY_SIZE(cnl_cdclk_table);
> -- 
> 2.20.1

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