Re: [PATCH 45/62] drm/i915/bdw: add support for BDW DP voltage swings and pre-emphasis

[Ben Widawsky <ben@bwidawsk.net>]

On Sat, Nov 02, 2013 at 09:07:43PM -0700, Ben Widawsky wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> They're not the same as the Haswell ones.
> 
> Reviewed-by: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 +++++++++
>  drivers/gpu/drm/i915/intel_dp.c | 55 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4131223..6f834b3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5176,6 +5176,7 @@
>  #define DDI_BUF_CTL_B				0x64100
>  #define DDI_BUF_CTL(port) _PORT(port, DDI_BUF_CTL_A, DDI_BUF_CTL_B)
>  #define  DDI_BUF_CTL_ENABLE			(1<<31)
> +/* Haswell */
>  #define  DDI_BUF_EMP_400MV_0DB_HSW		(0<<24)   /* Sel0 */
>  #define  DDI_BUF_EMP_400MV_3_5DB_HSW		(1<<24)   /* Sel1 */
>  #define  DDI_BUF_EMP_400MV_6DB_HSW		(2<<24)   /* Sel2 */
> @@ -5185,6 +5186,16 @@
>  #define  DDI_BUF_EMP_600MV_6DB_HSW		(6<<24)   /* Sel6 */
>  #define  DDI_BUF_EMP_800MV_0DB_HSW		(7<<24)   /* Sel7 */
>  #define  DDI_BUF_EMP_800MV_3_5DB_HSW		(8<<24)   /* Sel8 */
> +/* Broadwell */
> +#define  DDI_BUF_EMP_400MV_0DB_BDW		(0<<24)   /* Sel0 */
> +#define  DDI_BUF_EMP_400MV_3_5DB_BDW		(1<<24)   /* Sel1 */
> +#define  DDI_BUF_EMP_400MV_6DB_BDW		(2<<24)   /* Sel2 */
> +#define  DDI_BUF_EMP_600MV_0DB_BDW		(3<<24)   /* Sel3 */
> +#define  DDI_BUF_EMP_600MV_3_5DB_BDW		(4<<24)   /* Sel4 */
> +#define  DDI_BUF_EMP_600MV_6DB_BDW		(5<<24)   /* Sel5 */

Maybe I am misreading this, isn't this:
600mV, 4.5dB?

> +#define  DDI_BUF_EMP_800MV_0DB_BDW		(6<<24)   /* Sel6 */

850, .5

> +#define  DDI_BUF_EMP_800MV_3_5DB_BDW		(7<<24)   /* Sel7 */

750, 2.5

> +#define  DDI_BUF_EMP_1200MV_0DB_BDW		(8<<24)   /* Sel8 */

1000, 0

Sorry if I just misunderstood how you're getting these values.

>  #define  DDI_BUF_EMP_MASK			(0xf<<24)
>  #define  DDI_BUF_PORT_REVERSAL			(1<<16)
>  #define  DDI_BUF_IS_IDLE			(1<<7)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b3cc333..7725f81 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1950,7 +1950,7 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  
> -	if (IS_VALLEYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev) || IS_BROADWELL(dev))
>  		return DP_TRAIN_VOLTAGE_SWING_1200;
>  	else if (IS_GEN7(dev) && port == PORT_A)
>  		return DP_TRAIN_VOLTAGE_SWING_800;
> @@ -1966,7 +1966,18 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  
> -	if (HAS_DDI(dev)) {
> +	if (IS_BROADWELL(dev)) {
> +		switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> +		case DP_TRAIN_VOLTAGE_SWING_400:
> +		case DP_TRAIN_VOLTAGE_SWING_600:
> +			return DP_TRAIN_PRE_EMPHASIS_6;
> +		case DP_TRAIN_VOLTAGE_SWING_800:
> +			return DP_TRAIN_PRE_EMPHASIS_3_5;
> +		case DP_TRAIN_VOLTAGE_SWING_1200:
> +		default:
> +			return DP_TRAIN_PRE_EMPHASIS_0;
> +		}
> +	} else if (IS_HASWELL(dev)) {
>  		switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
>  		case DP_TRAIN_VOLTAGE_SWING_400:
>  			return DP_TRAIN_PRE_EMPHASIS_9_5;
> @@ -2278,6 +2289,41 @@ intel_hsw_signal_levels(uint8_t train_set)
>  	}
>  }
>  
> +static uint32_t
> +intel_bdw_signal_levels(uint8_t train_set)
> +{
> +	int signal_levels = train_set & (DP_TRAIN_VOLTAGE_SWING_MASK |
> +					 DP_TRAIN_PRE_EMPHASIS_MASK);
> +	switch (signal_levels) {
> +	case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_0:
> +		return DDI_BUF_EMP_400MV_0DB_BDW;	/* Sel0 */
> +	case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_3_5:
> +		return DDI_BUF_EMP_400MV_3_5DB_BDW;	/* Sel1 */
> +	case DP_TRAIN_VOLTAGE_SWING_400 | DP_TRAIN_PRE_EMPHASIS_6:
> +		return DDI_BUF_EMP_400MV_6DB_BDW;	/* Sel2 */
> +
> +	case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_0:
> +		return DDI_BUF_EMP_600MV_0DB_BDW;	/* Sel3 */
> +	case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_3_5:
> +		return DDI_BUF_EMP_600MV_3_5DB_BDW;	/* Sel4 */
> +	case DP_TRAIN_VOLTAGE_SWING_600 | DP_TRAIN_PRE_EMPHASIS_6:
> +		return DDI_BUF_EMP_600MV_6DB_BDW;	/* Sel5 */
> +
> +	case DP_TRAIN_VOLTAGE_SWING_800 | DP_TRAIN_PRE_EMPHASIS_0:
> +		return DDI_BUF_EMP_800MV_0DB_BDW;	/* Sel6 */
> +	case DP_TRAIN_VOLTAGE_SWING_800 | DP_TRAIN_PRE_EMPHASIS_3_5:
> +		return DDI_BUF_EMP_800MV_3_5DB_BDW;	/* Sel7 */
> +
> +	case DP_TRAIN_VOLTAGE_SWING_1200 | DP_TRAIN_PRE_EMPHASIS_0:
> +		return DDI_BUF_EMP_1200MV_0DB_BDW;	/* Sel8 */
> +
> +	default:
> +		DRM_DEBUG_KMS("Unsupported voltage swing/pre-emphasis level:"
> +			      "0x%x\n", signal_levels);
> +		return DDI_BUF_EMP_400MV_0DB_BDW;	/* Sel0 */
> +	}
> +}
> +
>  /* Properly updates "DP" with the correct signal levels. */
>  static void
>  intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
> @@ -2288,7 +2334,10 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>  	uint32_t signal_levels, mask;
>  	uint8_t train_set = intel_dp->train_set[0];
>  
> -	if (HAS_DDI(dev)) {
> +	if (IS_BROADWELL(dev)) {
> +		signal_levels = intel_bdw_signal_levels(train_set);
> +		mask = DDI_BUF_EMP_MASK;
> +	} else if (IS_HASWELL(dev)) {
>  		signal_levels = intel_hsw_signal_levels(train_set);
>  		mask = DDI_BUF_EMP_MASK;
>  	} else if (IS_VALLEYVIEW(dev)) {

Forgive my ignorance, but I really have no idea how to review this
patch. Daniel, I think we have to settle for Art's r-b on this one, or
finds someone who understands why this programming is the way it is. (Or
wait)

-- 
Ben Widawsky, Intel Open Source Technology Center