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

[Souza, Jose]

On Tue, 2019-12-03 at 13:55 -0800, Matt Roper wrote:
> On Tue, Dec 03, 2019 at 01:29:02PM -0800, José Roberto de Souza
> 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
> > 
> > v3:
> > Only calling icl_phy_set_clock_gating() on
> > intel_ddi_pre_enable_hdmi
> > for GEN11
> > 
> > BSpec issue: 20885
> > BSpec: 49292
> > 
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Reviewed-by: Matt Roper <matthew.d.roper at intel.com> (v1)
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> > 
> > Hi Matt, there was 2 small changes since v1, could check and give
> > another review?
> 
> Yep, the additional WARN_ON() and GEN==11 test look good.
> 
> However I just noticed the wording on bspec page 21735 (the gen11
> equivalent page) has also been updated and it sounds like we may not
> need to do this clockgating there anymore either?  That page now says
> "PHY clock gating should be configured by the typeC micocontroller so
> that display software does not need to modify it."  So maybe we
> should
> just be removing this function entirely now?

Nice catch, I will test this and send the completely removed version.

Thanks

> 
> 
> Matt
> 
> > Thanks
> > 
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 57 +++++++-------------
> > ----
> >  1 file changed, 17 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index ebcc7302706b..62a4a8bb2457 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 */
> > +	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);
> > @@ -3701,7 +3677,8 @@ static void intel_ddi_pre_enable_hdmi(struct
> > intel_encoder *encoder,
> >  	else
> >  		intel_prepare_hdmi_ddi_buffers(encoder, level);
> >  
> > -	icl_phy_set_clock_gating(dig_port, true);
> > +	if (INTEL_GEN(dev_priv) == 11)
> > +		icl_phy_set_clock_gating(dig_port, true);
> >  
> >  	if (IS_GEN9_BC(dev_priv))
> >  		skl_ddi_set_iboost(encoder, level, INTEL_OUTPUT_HDMI);
> > -- 
> > 2.24.0
> >