Re: [PATCH 08/17] drm/i915/icl: Implement voltage swing programming sequence for Combo PHY DDI

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

Em Qua, 2018-04-25 às 11:01 -0700, Rodrigo Vivi escreveu:
> On Tue, Apr 24, 2018 at 05:34:14PM -0700, Paulo Zanoni wrote:
> > Em Qui, 2018-04-05 às 17:20 -0700, Rodrigo Vivi escreveu:
> > > On Thu, Feb 22, 2018 at 12:55:10AM -0300, Paulo Zanoni wrote:
> > > > From: Manasi Navare <manasi.d.navare@intel.com>
> > > > 
> > > > This is an important part of the DDI initalization as well as
> > > > for changing the voltage during DisplayPort link training.
> > > > 
> > > > The Voltage swing seqeuence is similar to Cannonlake.
> > > > However it has different register definitions and hence
> > > > it makes sense to create a separate vswing sequence and
> > > > program functions for ICL to leave room for more changes
> > > > in case the Bspec changes later and deviates from CNL sequence.
> > > > 
> > > > v2:
> > > > Use ~TAP3_DISABLE for enbaling that bit (Jani Nikula)
> > > > 
> > > > v3:
> > > > * Use dw4_scaling column for PORT_TX_DW4 values (Rodrigo)
> > > > 
> > > > v4:
> > > > * Call it combo_vswing, use switch statement (Paulo)
> > > > 
> > > > v5 (from Paulo):
> > > > * Fix a typo.
> > > > * s/rate < 600000/rate <= 600000/.
> > > > * Don't remove blank lines that should be there.
> > > > 
> > > > v6:
> > > > * Rebased by Rodrigo on top of Cannonlake changes
> > > >   where non vswing sequences are not aligned with iboost
> > > >   anymore.
> > > > 
> > > > v7: Another rebase after an upstream rework.
> > > > 
> > > > v8 (from Paulo):
> > > > * Adjust the code to the upstream output type changes.
> > > > * Squash the patch that moved some functions up.
> > > > * Merge both get_combo_buf_trans functions in order to simplify
> > > > the
> > > >   code.
> > > > * Change the changelog format.
> > > > 
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (v5)
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c | 189
> > > > ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 186 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 0a4683991ec2..c38873cb98ca 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -849,6 +849,45 @@ cnl_get_buf_trans_edp(struct
> > > > drm_i915_private
> > > > *dev_priv, int *n_entries)
> > > >  	}
> > > >  }
> > > >  
> > > > +static const struct icl_combo_phy_ddi_buf_trans *
> > > > +icl_get_combo_buf_trans(struct drm_i915_private *dev_priv,
> > > > enum
> > > > port port,
> > > > +			int type, int *n_entries)
> > > > +{
> > > > +	u32 voltage = I915_READ(ICL_PORT_COMP_DW3(port)) &
> > > > VOLTAGE_INFO_MASK;
> > > > +
> > > > +	if (type == INTEL_OUTPUT_EDP && dev_priv-
> > > > > vbt.edp.low_vswing) {
> > > > 
> > > > +		switch (voltage) {
> > > > +		case VOLTAGE_INFO_0_85V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_0_85V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_edp_0_85V;
> > > > +		case VOLTAGE_INFO_0_95V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_0_95V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_edp_0_95V;
> > > > +		case VOLTAGE_INFO_1_05V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_edp_1_05V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_edp_1_05V;
> > > > +		default:
> > > > +			MISSING_CASE(voltage);
> > > > +			return NULL;
> > > > +		}
> > > > +	} else {
> > > 
> > > DP ends up here on HDMI?
> > > This is strange...
> > 
> > DP ends on the "dp_hdmi" part, while edp ends on the "edp" part.
> > What
> > is strange about that?
> 
> Oh... actually spec is strange...
> They have 2 tables, 1 for DP and 1 for HDMI.
> I just read them again and they are the same indeed.
> So, nothing wrong here.
> 
> > 
> > > 
> > > Also, for clarity, why not to split this into separated functions
> > > like all other platforms?
> > 
> > So we don't end up reading PORT_COMP_DW3 multiple times (like we do
> > for
> > CNL), and as a bonus the code looks better IMHO.
> 
> Agree.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

You're reviewing v8 of the patch. This thread already has v9 (as a
reply to v8). Does the r-b also apply to it?

v9 was also resent as https://patchwork.freedesktop.org/patch/213515/

> 
> > 
> > 
> > 
> > > 
> > > > +		switch (voltage) {
> > > > +		case VOLTAGE_INFO_0_85V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hdmi_0_85V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_dp_hdmi_0_85V;
> > > > +		case VOLTAGE_INFO_0_95V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hdmi_0_95V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_dp_hdmi_0_95V;
> > > > +		case VOLTAGE_INFO_1_05V:
> > > > +			*n_entries =
> > > > ARRAY_SIZE(icl_combo_phy_ddi_translations_dp_hdmi_1_05V);
> > > > +			return
> > > > icl_combo_phy_ddi_translations_dp_hdmi_1_05V;
> > > > +		default:
> > > > +			MISSING_CASE(voltage);
> > > > +			return NULL;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > >  static int intel_ddi_hdmi_level(struct drm_i915_private
> > > > *dev_priv,
> > > > enum port port)
> > > >  {
> > > >  	int n_entries, level, default_entry;
> > > > @@ -2178,6 +2217,144 @@ static void
> > > > cnl_ddi_vswing_sequence(struct
> > > > intel_encoder *encoder,
> > > >  	I915_WRITE(CNL_PORT_TX_DW5_GRP(port), val);
> > > >  }
> > > >  
> > > > +static void icl_ddi_combo_vswing_program(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +					 u32 level, enum port
> > > > port, int type)
> > > > +{
> > > > +	const struct icl_combo_phy_ddi_buf_trans
> > > > *ddi_translations
> > > > = NULL;
> > > > +	u32 n_entries, val;
> > > > +	int ln;
> > > > +
> > > > +	ddi_translations = icl_get_combo_buf_trans(dev_priv,
> > > > port,
> > > > type,
> > > > +						   &n_entries)
> > > > ;
> > > > +	if (!ddi_translations)
> > > > +		return;
> > > > +
> > > > +	if (level >= n_entries) {
> > > > +		DRM_DEBUG_KMS("DDI translation not found for
> > > > level
> > > > %d. Using %d instead.", level, n_entries - 1);
> > > > +		level = n_entries - 1;
> > > > +	}
> > > > +
> > > > +	/* Set PORT_TX_DW5 Scaling Mode Sel to 110b. */
> > > > +	val = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> > > > +	val &= ~SCALING_MODE_SEL_MASK;
> > > > +	val |= SCALING_MODE_SEL(0x6);
> > > > +	I915_WRITE(ICL_PORT_TX_DW5_GRP(port), val);
> > > > +
> > > > +	/* Program PORT_TX_DW5 */
> > > > +	val = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> > > > +	/* Set DisableTap2 and DisableTap3 if MIPI DSI
> > > > +	 * Clear DisableTap2 and DisableTap3 for all other
> > > > Ports
> > > > +	 */
> > > > +	if (type == INTEL_OUTPUT_DSI) {
> > > > +		val |= TAP2_DISABLE;
> > > > +		val |= TAP3_DISABLE;
> > > > +	} else {
> > > > +		val &= ~TAP2_DISABLE;
> > > > +		val &= ~TAP3_DISABLE;
> > > > +	}
> > > > +	I915_WRITE(ICL_PORT_TX_DW5_GRP(port), val);
> > > > +
> > > > +	/* Program PORT_TX_DW2 */
> > > > +	val = I915_READ(ICL_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_select);
> > > > +	val |=
> > > > SWING_SEL_LOWER(ddi_translations[level].dw2_swing_select);
> > > > +	/* Program Rcomp scalar for every table entry */
> > > > +	val |=
> > > > RCOMP_SCALAR(ddi_translations[level].dw2_swing_scalar);
> > > > +	I915_WRITE(ICL_PORT_TX_DW2_GRP(port), val);
> > > > +
> > > > +	/* Program PORT_TX_DW4 */
> > > > +	/* We cannot write to GRP. It would overwrite
> > > > individual
> > > > loadgen. */
> > > > +	for (ln = 0; ln <= 3; ln++) {
> > > > +		val = I915_READ(ICL_PORT_TX_DW4_LN(port, ln));
> > > > +		val &= ~(POST_CURSOR_1_MASK |
> > > > POST_CURSOR_2_MASK |
> > > > +			 CURSOR_COEFF_MASK);
> > > > +		val |= ddi_translations[level].dw4_scaling;
> > > > +		I915_WRITE(ICL_PORT_TX_DW4_LN(port, ln), val);
> > > > +	}
> > > > +}
> > > > +
> > > > +static void icl_combo_phy_ddi_vswing_sequence(struct
> > > > intel_encoder
> > > > *encoder, u32 level)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(encoder-
> > > > > base.dev);
> > > > 
> > > > +	enum port port = encoder->port;
> > > > +	int type = encoder->type;
> > > > +	int width = 0;
> > > > +	int rate = 0;
> > > > +	u32 val;
> > > > +	int ln = 0;
> > > > +
> > > > +	if (type == INTEL_OUTPUT_HDMI) {
> > > > +		width = 4;
> > > > +		/* Rate is always < than 6GHz for HDMI */
> > > > +	} else {
> > > > +		struct intel_dp *intel_dp =
> > > > enc_to_intel_dp(&encoder->base);
> > > > +
> > > > +		width = intel_dp->lane_count;
> > > > +		rate = intel_dp->link_rate;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * 1. If port type is eDP or DP,
> > > > +	 * set PORT_PCS_DW1 cmnkeeper_enable to 1b,
> > > > +	 * else clear to 0b.
> > > > +	 */
> > > > +	val = I915_READ(ICL_PORT_PCS_DW1_LN0(port));
> > > > +	if (type == INTEL_OUTPUT_HDMI)
> > > > +		val &= ~COMMON_KEEPER_EN;
> > > > +	else
> > > > +		val |= COMMON_KEEPER_EN;
> > > > +	I915_WRITE(ICL_PORT_PCS_DW1_GRP(port), val);
> > > > +
> > > > +	/* 2. Program loadgen select */
> > > > +	/*
> > > > +	 * Program PORT_TX_DW4_LN depending on Bit rate and
> > > > used
> > > > lanes
> > > > +	 * <= 6 GHz and 4 lanes (LN0=0, LN1=1, LN2=1, LN3=1)
> > > > +	 * <= 6 GHz and 1,2 lanes (LN0=0, LN1=1, LN2=1, LN3=0)
> > > > +	 * > 6 GHz (LN0=0, LN1=0, LN2=0, LN3=0)
> > > > +	 */
> > > > +	for (ln = 0; ln <= 3; ln++) {
> > > > +		val = I915_READ(ICL_PORT_TX_DW4_LN(port, ln));
> > > > +		val &= ~LOADGEN_SELECT;
> > > > +
> > > > +		if ((rate <= 600000 && width == 4 && ln >= 1)
> > > > ||
> > > > +		    (rate <= 600000 && width < 4 && (ln == 1
> > > > || ln
> > > > == 2))) {
> > > > +			val |= LOADGEN_SELECT;
> > > > +		}
> > > > +		I915_WRITE(ICL_PORT_TX_DW4_LN(port, ln), val);
> > > > +	}
> > > > +
> > > > +	/* 3. Set PORT_CL_DW5 SUS Clock Config to 11b */
> > > > +	val = I915_READ(ICL_PORT_CL_DW5(port));
> > > > +	val |= SUS_CLOCK_CONFIG;
> > > > +	I915_WRITE(ICL_PORT_CL_DW5(port), val);
> > > > +
> > > > +	/* 4. Clear training enable to change swing values */
> > > > +	val = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> > > > +	val &= ~TX_TRAINING_EN;
> > > > +	I915_WRITE(ICL_PORT_TX_DW5_GRP(port), val);
> > > > +
> > > > +	/* 5. Program swing and de-emphasis */
> > > > +	icl_ddi_combo_vswing_program(dev_priv, level, port,
> > > > type);
> > > > +
> > > > +	/* 6. Set training enable to trigger update */
> > > > +	val = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> > > > +	val |= TX_TRAINING_EN;
> > > > +	I915_WRITE(ICL_PORT_TX_DW5_GRP(port), val);
> > > > +}
> > > > +
> > > > +static void icl_ddi_vswing_sequence(struct intel_encoder
> > > > *encoder,
> > > > u32 level)
> > > > +{
> > > > +	enum port port = encoder->port;
> > > > +
> > > > +	if (port == PORT_A || port == PORT_B)
> > > > +		icl_combo_phy_ddi_vswing_sequence(encoder,
> > > > level);
> > > > +	else
> > > > +		/* Not Implemented Yet */
> > > > +		WARN_ON(1);
> > > > +}
> > > > +
> > > >  static uint32_t translate_signal_level(int signal_levels)
> > > >  {
> > > >  	int i;
> > > > @@ -2209,7 +2386,9 @@ u32 bxt_signal_levels(struct intel_dp
> > > > *intel_dp)
> > > >  	struct intel_encoder *encoder = &dport->base;
> > > >  	int level = intel_ddi_dp_level(intel_dp);
> > > >  
> > > > -	if (IS_CANNONLAKE(dev_priv))
> > > > +	if (IS_ICELAKE(dev_priv))
> > > > +		icl_ddi_vswing_sequence(encoder, level);
> > > > +	else if (IS_CANNONLAKE(dev_priv))
> > > >  		cnl_ddi_vswing_sequence(encoder, level,
> > > > encoder-
> > > > > type);
> > > > 
> > > >  	else
> > > >  		bxt_ddi_vswing_sequence(encoder, level,
> > > > encoder-
> > > > > type);
> > > > 
> > > > @@ -2389,7 +2568,9 @@ static void
> > > > intel_ddi_pre_enable_dp(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	intel_display_power_get(dev_priv, dig_port-
> > > > > ddi_io_power_domain);
> > > > 
> > > >  
> > > > -	if (IS_CANNONLAKE(dev_priv))
> > > > +	if (IS_ICELAKE(dev_priv))
> > > > +		icl_ddi_vswing_sequence(encoder, level);
> > > > +	else if (IS_CANNONLAKE(dev_priv))
> > > >  		cnl_ddi_vswing_sequence(encoder, level,
> > > > encoder-
> > > > > type);
> > > > 
> > > >  	else if (IS_GEN9_LP(dev_priv))
> > > >  		bxt_ddi_vswing_sequence(encoder, level,
> > > > encoder-
> > > > > type);
> > > > 
> > > > @@ -2420,7 +2601,9 @@ static void
> > > > intel_ddi_pre_enable_hdmi(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >  	intel_display_power_get(dev_priv, dig_port-
> > > > > ddi_io_power_domain);
> > > > 
> > > >  
> > > > -	if (IS_CANNONLAKE(dev_priv))
> > > > +	if (IS_ICELAKE(dev_priv))
> > > > +		icl_ddi_vswing_sequence(encoder, level);
> > > > +	else if (IS_CANNONLAKE(dev_priv))
> > > >  		cnl_ddi_vswing_sequence(encoder, level,
> > > > INTEL_OUTPUT_HDMI);
> > > >  	else if (IS_GEN9_LP(dev_priv))
> > > >  		bxt_ddi_vswing_sequence(encoder, level,
> > > > INTEL_OUTPUT_HDMI);
> > > > -- 
> > > > 2.14.3
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx