Re: [PATCH] drm/i915/cnl: Fix RMW on ddi vswing sequence.

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Sex, 2017-06-16 às 13:15 -0700, Rodrigo Vivi escreveu:
> Paulo noticed that we were missing few bits clear
> before writing values back to the register on
> these RMW MMIO operations.
> 
> Fixes: cf54ca8bc567 ("drm/i915/cnl: Implement voltage swing
> sequence.")
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 9 +++++++++
>  drivers/gpu/drm/i915/intel_ddi.c | 7 +++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index bd535f1..d4edc79 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1764,8 +1764,11 @@ enum skl_disp_power_wells {
>  						    _CNL_PORT_TX_DW2
> _LN0_AE, \
>  						    _CNL_PORT_TX_DW2
> _LN0_F)
>  #define   SWING_SEL_UPPER(x)		((x >> 3) << 15)
> +#define   SWING_SEL_UPPER_MASK		(1 << 15)
>  #define   SWING_SEL_LOWER(x)		((x & 0x7) << 11)
> +#define   SWING_SEL_LOWER_MASK		(0x7 << 11)
>  #define   RCOMP_SCALAR(x)		((x) << 0)
> +#define   RCOMP_SCALAR_MASK		(0xFF << 0)
>  
>  #define _CNL_PORT_TX_DW4_GRP_AE		0x162350
>  #define _CNL_PORT_TX_DW4_GRP_B		0x1623D0
> @@ -1795,8 +1798,11 @@ enum skl_disp_power_wells {
>  						    _CNL_PORT_TX_DW4
> _LN0_F)
>  #define   LOADGEN_SELECT		(1 << 31)
>  #define   POST_CURSOR_1(x)		((x) << 12)
> +#define   POST_CURSOR_1_MASK		(0x3F << 12)
>  #define   POST_CURSOR_2(x)		((x) << 6)
> +#define   POST_CURSOR_2_MASK		(0x3F << 6)
>  #define   CURSOR_COEFF(x)		((x) << 0)
> +#define   POST_CURSOR_COEFF_MASK	(0x3F << 6)

s/POST_CURSOR_COEFF_MASK/CURSOR_COEFF_MASK/, there's no "post" in the
name of this field.

With that:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

As a side note, I really think all these CNL definitions lack the name
of the registers as prefix. I worry about future namespace collision.
But that's a discussion for another patch.


>  
>  #define _CNL_PORT_TX_DW5_GRP_AE		0x162354
>  #define _CNL_PORT_TX_DW5_GRP_B		0x1623D4
> @@ -1825,7 +1831,9 @@ enum skl_disp_power_wells {
>  #define   TX_TRAINING_EN		(1 << 31)
>  #define   TAP3_DISABLE			(1 << 29)
>  #define   SCALING_MODE_SEL(x)		((x) << 18)
> +#define   SCALING_MODE_SEL_MASK		(0x7 << 18)
>  #define   RTERM_SELECT(x)		((x) << 3)
> +#define   RTERM_SELECT_MASK		(0x7 << 3)
>  
>  #define _CNL_PORT_TX_DW7_GRP_AE		0x16235C
>  #define _CNL_PORT_TX_DW7_GRP_B		0x1623DC
> @@ -1852,6 +1860,7 @@ enum skl_disp_power_wells {
>  						    _CNL_PORT_TX_DW7
> _LN0_AE, \
>  						    _CNL_PORT_TX_DW7
> _LN0_F)
>  #define   N_SCALAR(x)			((x) << 24)
> +#define   N_SCALAR_MASK			(0x7F << 24)
>  
>  /* The spec defines this only for BXT PHY0, but lets assume that
> this
>   * would exist for PHY1 too if it had a second channel.
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index db80938..e66947a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1813,11 +1813,14 @@ static void cnl_ddi_vswing_program(struct
> drm_i915_private *dev_priv,
>  
>  	/* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */
>  	val = I915_READ(CNL_PORT_TX_DW5_LN0(port));
> +	val &= ~(SCALING_MODE_SEL_MASK);
>  	val |= SCALING_MODE_SEL(2);
>  	I915_WRITE(CNL_PORT_TX_DW5_GRP(port), val);
>  
>  	/* Program PORT_TX_DW2 */
>  	val = I915_READ(CNL_PORT_TX_DW2_LN0(port));
> +	val &= ~(SWING_SEL_LOWER_MASK | SWING_SEL_UPPER_MASK |
> +		 RCOMP_SCALAR_MASK);
>  	val |=
> SWING_SEL_UPPER(ddi_translations[level].dw2_swing_sel);
>  	val |=
> SWING_SEL_LOWER(ddi_translations[level].dw2_swing_sel);
>  	/* Rcomp scalar is fixed as 0x98 for every table entry */
> @@ -1828,6 +1831,8 @@ static void cnl_ddi_vswing_program(struct
> drm_i915_private *dev_priv,
>  	/* We cannot write to GRP. It would overrite individual
> loadgen */
>  	for (ln = 0; ln < 4; ln++) {
>  		val = I915_READ(CNL_PORT_TX_DW4_LN(port, ln));
> +		val &= ~(POST_CURSOR_1_MASK | POST_CURSOR_2_MASK |
> +			 POST_CURSOR_COEFF_MASK);
>  		val |=
> POST_CURSOR_1(ddi_translations[level].dw4_post_cursor_1);
>  		val |=
> POST_CURSOR_2(ddi_translations[level].dw4_post_cursor_2);
>  		val |=
> CURSOR_COEFF(ddi_translations[level].dw4_cursor_coeff);
> @@ -1837,12 +1842,14 @@ static void cnl_ddi_vswing_program(struct
> drm_i915_private *dev_priv,
>          /* Program PORT_TX_DW5 */
>  	/* All DW5 values are fixed for every table entry */
>  	val = I915_READ(CNL_PORT_TX_DW5_LN0(port));
> +	val &= ~(RTERM_SELECT_MASK);
>  	val |= RTERM_SELECT(6);
>  	val |= TAP3_DISABLE;
>  	I915_WRITE(CNL_PORT_TX_DW5_GRP(port), val);
>  
>          /* Program PORT_TX_DW7 */
>  	val = I915_READ(CNL_PORT_TX_DW7_LN0(port));
> +	val &= ~N_SCALAR_MASK;
>  	val |= N_SCALAR(ddi_translations[level].dw7_n_scalar);
>  	I915_WRITE(CNL_PORT_TX_DW7_GRP(port), val);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx