Re: [Intel-gfx] [PATCH CI] drm/i915/display/tgl: Do not program clockgating

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 03 Dec 2019, "Souza, Jose" <jose.souza@intel.com> wrote:
> On Tue, 2019-12-03 at 10:20 +0200, Jani Nikula wrote:
>> On Mon, 02 Dec 2019, José Roberto de Souza <jose.souza@intel.com>
>> wrote:
>> > Talked with HW team and this is a left over, driver should not
>> > program clockgating, dekel firmware will be reponsible for any
>> > clockgating programing.
>> > 
>> > v2:
>> > Added WARN_ON
>> > 
>> > BSpec issue: 20885
>> > BSpec: 49292
>> > 
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Cc: Matt Roper <matthew.d.roper@intel.com>
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_ddi.c | 54 +++++++-------------
>> > ----
>> >  1 file changed, 15 insertions(+), 39 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
>> > b/drivers/gpu/drm/i915/display/intel_ddi.c
>> > index a976606d21c7..66052a9f1474 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> > @@ -3167,6 +3167,10 @@ icl_phy_set_clock_gating(struct
>> > intel_digital_port *dig_port, bool enable)
>> >  	u32 val, bits;
>> >  	int ln;
>> >  
>> > +	/* See "PHY Clockgating programming" note */
>> 
>> Where?
>
> BSpec: 49292, PHY Clockgating programming: Display driver should not
> program the following bits.

In a code comment, people would look for the note in the source. That's
certainly what *I* did. And it's pretty rude to have a reference to
internal documentation in a public source tree.

If you think we need to have the comment, please paraphrase the
spec. For example, just "Per Bspec we shouldn't do this on gen12+" or
something is better than an obscure reference.

BR,
Jani.


>
>> 
>> > +	if (WARN_ON(INTEL_GEN(dev_priv) >= 12))
>> > +		return;
>> > +
>> >  	if (tc_port == PORT_TC_NONE)
>> >  		return;
>> >  
>> > @@ -3175,39 +3179,26 @@ icl_phy_set_clock_gating(struct
>> > intel_digital_port *dig_port, bool enable)
>> >  	       MG_DP_MODE_CFG_GAONPWR_GATING;
>> >  
>> >  	for (ln = 0; ln < 2; ln++) {
>> > -		if (INTEL_GEN(dev_priv) >= 12) {
>> > -			I915_WRITE(HIP_INDEX_REG(tc_port),
>> > HIP_INDEX_VAL(tc_port, ln));
>> > -			val = I915_READ(DKL_DP_MODE(tc_port));
>> > -		} else {
>> > -			val = I915_READ(MG_DP_MODE(ln, tc_port));
>> > -		}
>> > +		val = I915_READ(MG_DP_MODE(ln, tc_port));
>> >  
>> >  		if (enable)
>> >  			val |= bits;
>> >  		else
>> >  			val &= ~bits;
>> >  
>> > -		if (INTEL_GEN(dev_priv) >= 12)
>> > -			I915_WRITE(DKL_DP_MODE(tc_port), val);
>> > -		else
>> > -			I915_WRITE(MG_DP_MODE(ln, tc_port), val);
>> > +		I915_WRITE(MG_DP_MODE(ln, tc_port), val);
>> >  	}
>> >  
>> > -	if (INTEL_GEN(dev_priv) == 11) {
>> > -		bits = MG_MISC_SUS0_CFG_TR2PWR_GATING |
>> > -		       MG_MISC_SUS0_CFG_CL2PWR_GATING |
>> > -		       MG_MISC_SUS0_CFG_GAONPWR_GATING |
>> > -		       MG_MISC_SUS0_CFG_TRPWR_GATING |
>> > -		       MG_MISC_SUS0_CFG_CL1PWR_GATING |
>> > -		       MG_MISC_SUS0_CFG_DGPWR_GATING;
>> > +	bits = MG_MISC_SUS0_CFG_TR2PWR_GATING |
>> > MG_MISC_SUS0_CFG_CL2PWR_GATING |
>> > +	       MG_MISC_SUS0_CFG_GAONPWR_GATING |
>> > MG_MISC_SUS0_CFG_TRPWR_GATING |
>> > +	       MG_MISC_SUS0_CFG_CL1PWR_GATING |
>> > MG_MISC_SUS0_CFG_DGPWR_GATING;
>> >  
>> > -		val = I915_READ(MG_MISC_SUS0(tc_port));
>> > -		if (enable)
>> > -			val |= (bits |
>> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3));
>> > -		else
>> > -			val &= ~(bits |
>> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK);
>> > -		I915_WRITE(MG_MISC_SUS0(tc_port), val);
>> > -	}
>> > +	val = I915_READ(MG_MISC_SUS0(tc_port));
>> > +	if (enable)
>> > +		val |= (bits | MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE(3));
>> > +	else
>> > +		val &= ~(bits |
>> > MG_MISC_SUS0_SUSCLK_DYNCLKGATE_MODE_MASK);
>> > +	I915_WRITE(MG_MISC_SUS0(tc_port), val);
>> >  }
>> >  
>> >  static void
>> > @@ -3508,12 +3499,6 @@ static void tgl_ddi_pre_enable_dp(struct
>> > intel_encoder *encoder,
>> >  	 * down this function.
>> >  	 */
>> >  
>> > -	/*
>> > -	 * 7.d Type C with DP alternate or fixed/legacy/static
>> > connection -
>> > -	 * Disable PHY clock gating per Type-C DDI Buffer page
>> > -	 */
>> > -	icl_phy_set_clock_gating(dig_port, false);
>> > -
>> >  	/* 7.e Configure voltage swing and related IO settings */
>> >  	tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level,
>> >  				encoder->type);
>> > @@ -3565,15 +3550,6 @@ static void tgl_ddi_pre_enable_dp(struct
>> > intel_encoder *encoder,
>> >  	if (!is_trans_port_sync_mode(crtc_state))
>> >  		intel_dp_stop_link_train(intel_dp);
>> >  
>> > -	/*
>> > -	 * TODO: enable clock gating
>> > -	 *
>> > -	 * It is not written in DP enabling sequence but "PHY
>> > Clockgating
>> > -	 * programming" states that clock gating should be enabled
>> > after the
>> > -	 * link training but doing so causes all the following
>> > trainings to fail
>> > -	 * so not enabling it for now.
>> > -	 */
>> > -
>> >  	/* 7.l Configure and enable FEC if needed */
>> >  	intel_ddi_enable_fec(encoder, crtc_state);
>> >  	intel_dsc_enable(encoder, crtc_state);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx