diff mbox series

[04/17] drm/i915/ddi: Set missing TC DP PHY lane stagger delay in DDI_BUF_CTL

Message ID 20250129200221.2508101-5-imre.deak@intel.com (mailing list archive)
State New
Headers show
Series drm/i915/ddi: Fix/simplify port enabling/disabling | expand

Commit Message

Imre Deak Jan. 29, 2025, 8:02 p.m. UTC 
From: Imre Deak <imre.deak@gmail.com>

Add the missing PHY lane stagger delay programming for ICL-ADL
platforms on TypeC DP outputs.

Bspec: 7534, 49533
Signed-off-by: Imre Deak <imre.deak@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h          |  3 +++
 2 files changed, 21 insertions(+)

Comments

Jani Nikula Feb. 5, 2025, 12:22 p.m. UTC | #1 
On Wed, 29 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> From: Imre Deak <imre.deak@gmail.com>
>
> Add the missing PHY lane stagger delay programming for ICL-ADL
> platforms on TypeC DP outputs.
>
> Bspec: 7534, 49533
> Signed-off-by: Imre Deak <imre.deak@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h          |  3 +++
>  2 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 76e8296cb134b..6192c0d3c87a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -328,9 +328,21 @@ static u32 ddi_buf_phy_link_rate(int port_clock)
>  	}
>  }
>  
> +static int dp_phy_lane_stagger_delay(int port_clock)
> +{
> +	/*
> +	 * Return the number of link symbols per 100 ns:
> +	 * port_clock (10 kHz) -> bits    / 100 us
> +	 * / symbol_size       -> symbols / 100 us
> +	 * / 1000              -> symbols / 100 ns
> +	 */
> +	return DIV_ROUND_UP(port_clock, intel_dp_link_symbol_size(port_clock) * 1000);

Okay, this checks out, but it was incredibly difficult (for me) to
follow, even with the comment. The meaning of 100 ns is also not
documented i.e. we want a lane stagger delay of 100 ns or greater, and
we must express this in terms of symbols in the register.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +}
> +
>  static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
>  				      const struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> @@ -356,6 +368,12 @@ static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
>  		if (!intel_tc_port_in_tbt_alt_mode(dig_port))
>  			intel_dp->DP |= DDI_BUF_CTL_TC_PHY_OWNERSHIP;
>  	}
> +
> +	if (IS_DISPLAY_VER(display, 11, 13) && intel_encoder_is_tc(encoder)) {
> +		int delay = dp_phy_lane_stagger_delay(crtc_state->port_clock);
> +
> +		intel_dp->DP |= DDI_BUF_LANE_STAGGER_DELAY(delay);
> +	}
>  }
>  
>  static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04e47d0a8ab92..7fe4e71fc08ec 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3632,6 +3632,9 @@ enum skl_power_gate {
>  #define  DDI_BUF_PORT_DATA_20BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 1)
>  #define  DDI_BUF_PORT_DATA_40BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 2)
>  #define  DDI_BUF_PORT_REVERSAL			(1 << 16)
> +#define  DDI_BUF_LANE_STAGGER_DELAY_MASK	REG_GENMASK(15, 8)
> +#define  DDI_BUF_LANE_STAGGER_DELAY(symbols)	REG_FIELD_PREP(DDI_BUF_LANE_STAGGER_DELAY_MASK, \
> +							       symbols)
>  #define  DDI_BUF_IS_IDLE			(1 << 7)
>  #define  DDI_BUF_CTL_TC_PHY_OWNERSHIP		REG_BIT(6)
>  #define  DDI_A_4_LANES				(1 << 4)
Imre Deak Feb. 5, 2025, 1:20 p.m. UTC | #2 
On Wed, Feb 05, 2025 at 02:22:56PM +0200, Jani Nikula wrote:
> On Wed, 29 Jan 2025, Imre Deak <imre.deak@intel.com> wrote:
> > From: Imre Deak <imre.deak@gmail.com>
> >
> > Add the missing PHY lane stagger delay programming for ICL-ADL
> > platforms on TypeC DP outputs.
> >
> > Bspec: 7534, 49533
> > Signed-off-by: Imre Deak <imre.deak@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 18 ++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h          |  3 +++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 76e8296cb134b..6192c0d3c87a5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -328,9 +328,21 @@ static u32 ddi_buf_phy_link_rate(int port_clock)
> >  	}
> >  }
> >  
> > +static int dp_phy_lane_stagger_delay(int port_clock)
> > +{
> > +	/*
> > +	 * Return the number of link symbols per 100 ns:
> > +	 * port_clock (10 kHz) -> bits    / 100 us
> > +	 * / symbol_size       -> symbols / 100 us
> > +	 * / 1000              -> symbols / 100 ns
> > +	 */
> > +	return DIV_ROUND_UP(port_clock, intel_dp_link_symbol_size(port_clock) * 1000);
> 
> Okay, this checks out, but it was incredibly difficult (for me) to
> follow, even with the comment. The meaning of 100 ns is also not
> documented i.e. we want a lane stagger delay of 100 ns or greater, and
> we must express this in terms of symbols in the register.

I could clarify the above one-line code comment based on the bspec
description as (i.e. replace "Return the number of link symbols per 100
ns" with):

"""
Return the number of symbol clocks delay used to stagger the
assertion/desassertion of the port lane enables. The target delay time
is 100 ns or greater, return the number of symbols specific to
the provided port_clock (aka link clock) corresponding to this delay
time, i.e. so that

   number_of_symbols * duration_of_one_symbol >= 100 ns

The delay must be applied only on TypeC DP outputs, for everything else
the delay must be set to 0.
"""

followed by the above unit conversion hints.

> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > +}
> > +
> >  static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
> >  				      const struct intel_crtc_state *crtc_state)
> >  {
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > @@ -356,6 +368,12 @@ static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
> >  		if (!intel_tc_port_in_tbt_alt_mode(dig_port))
> >  			intel_dp->DP |= DDI_BUF_CTL_TC_PHY_OWNERSHIP;
> >  	}
> > +
> > +	if (IS_DISPLAY_VER(display, 11, 13) && intel_encoder_is_tc(encoder)) {
> > +		int delay = dp_phy_lane_stagger_delay(crtc_state->port_clock);
> > +
> > +		intel_dp->DP |= DDI_BUF_LANE_STAGGER_DELAY(delay);
> > +	}
> >  }
> >  
> >  static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 04e47d0a8ab92..7fe4e71fc08ec 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3632,6 +3632,9 @@ enum skl_power_gate {
> >  #define  DDI_BUF_PORT_DATA_20BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 1)
> >  #define  DDI_BUF_PORT_DATA_40BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 2)
> >  #define  DDI_BUF_PORT_REVERSAL			(1 << 16)
> > +#define  DDI_BUF_LANE_STAGGER_DELAY_MASK	REG_GENMASK(15, 8)
> > +#define  DDI_BUF_LANE_STAGGER_DELAY(symbols)	REG_FIELD_PREP(DDI_BUF_LANE_STAGGER_DELAY_MASK, \
> > +							       symbols)
> >  #define  DDI_BUF_IS_IDLE			(1 << 7)
> >  #define  DDI_BUF_CTL_TC_PHY_OWNERSHIP		REG_BIT(6)
> >  #define  DDI_A_4_LANES				(1 << 4)
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 76e8296cb134b..6192c0d3c87a5 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -328,9 +328,21 @@  static u32 ddi_buf_phy_link_rate(int port_clock)
 	}
 }
 
+static int dp_phy_lane_stagger_delay(int port_clock)
+{
+	/*
+	 * Return the number of link symbols per 100 ns:
+	 * port_clock (10 kHz) -> bits    / 100 us
+	 * / symbol_size       -> symbols / 100 us
+	 * / 1000              -> symbols / 100 ns
+	 */
+	return DIV_ROUND_UP(port_clock, intel_dp_link_symbol_size(port_clock) * 1000);
+}
+
 static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
 				      const struct intel_crtc_state *crtc_state)
 {
+	struct intel_display *display = to_intel_display(encoder);
 	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
@@ -356,6 +368,12 @@  static void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder,
 		if (!intel_tc_port_in_tbt_alt_mode(dig_port))
 			intel_dp->DP |= DDI_BUF_CTL_TC_PHY_OWNERSHIP;
 	}
+
+	if (IS_DISPLAY_VER(display, 11, 13) && intel_encoder_is_tc(encoder)) {
+		int delay = dp_phy_lane_stagger_delay(crtc_state->port_clock);
+
+		intel_dp->DP |= DDI_BUF_LANE_STAGGER_DELAY(delay);
+	}
 }
 
 static int icl_calc_tbt_pll_link(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04e47d0a8ab92..7fe4e71fc08ec 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3632,6 +3632,9 @@  enum skl_power_gate {
 #define  DDI_BUF_PORT_DATA_20BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 1)
 #define  DDI_BUF_PORT_DATA_40BIT		REG_FIELD_PREP(DDI_BUF_PORT_DATA_MASK, 2)
 #define  DDI_BUF_PORT_REVERSAL			(1 << 16)
+#define  DDI_BUF_LANE_STAGGER_DELAY_MASK	REG_GENMASK(15, 8)
+#define  DDI_BUF_LANE_STAGGER_DELAY(symbols)	REG_FIELD_PREP(DDI_BUF_LANE_STAGGER_DELAY_MASK, \
+							       symbols)
 #define  DDI_BUF_IS_IDLE			(1 << 7)
 #define  DDI_BUF_CTL_TC_PHY_OWNERSHIP		REG_BIT(6)
 #define  DDI_A_4_LANES				(1 << 4)