Re: [PATCH] drm/i915/display/tgl: Do not program clockgating

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

On Tue, 26 Nov 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.
>
> BSpec issue: 20885
> BSpec: 49292
>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: 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 | 55 +++++++-----------------
>  1 file changed, 16 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..7488dcbb637f 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3167,6 +3167,11 @@ icl_phy_set_clock_gating(struct intel_digital_port *dig_port, bool enable)
>  	u32 val, bits;
>  	int ln;
>  
> +	/*
> +	 * Should not be called for GEN12+, see "PHY Clockgating programming"
> +	 * note
> +	 */
> +

IMHO this kind of comments do us no good.

If you want to be paranoid,

	if (WARN_ON(INTEL_GEN(dev_priv) >= 12))
        	return;

which has the benefit of being self-documenting code. But I'm not
convinced of even that.

BR,
Jani.

>  	if (tc_port == PORT_TC_NONE)
>  		return;
>  
> @@ -3175,39 +3180,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 +3500,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 +3551,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