Re: [PATCH 2/4] drm/i915/gt: Replace TIMESTAMP_OVERRIDE readout

["Vivekanandan, Balasubramani" <balasubramani.vivekanandan@intel.com>]

On 20.02.2025 16:38, Matt Roper wrote:
> The whole GT CS clock initialization area is poorly documented in the
> specs and a lot of this code seems to have been inherited from the
> Windows driver team long ago.  There's nothing in the specs that
> specifically explains using the display reference frequency, as taken
> from TIMESTAMP_OVERRIDE register, to determine the GT command streamer
> clock.  But if the goal is just to get the display reference clock, we
> already have existing display code that takes care of that in a more
> straightforward manner (i.e., by either reading the strap register or
> using a per-platform constant).  Let's drop the usage of
> TIMESTAMP_OVERRIDE (which is a bit questionable to begin with since this
> is a display debug register) and replace it with a call to our existing
> display function.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Looks good to me.

Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Regards,
Bala

> ---
>  .../gpu/drm/i915/gt/intel_gt_clock_utils.c    | 31 ++++++-------------
>  1 file changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_clock_utils.c b/drivers/gpu/drm/i915/gt/intel_gt_clock_utils.c
> index 6e63505fe478..adc21c3322da 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_clock_utils.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_clock_utils.c
> @@ -11,23 +11,6 @@
>  #include "intel_gt_regs.h"
>  #include "soc/intel_dram.h"
>  
> -static u32 read_reference_ts_freq(struct intel_uncore *uncore)
> -{
> -	u32 ts_override = intel_uncore_read(uncore, GEN9_TIMESTAMP_OVERRIDE);
> -	u32 base_freq, frac_freq;
> -
> -	base_freq = ((ts_override & GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK) >>
> -		     GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_SHIFT) + 1;
> -	base_freq *= 1000000;
> -
> -	frac_freq = ((ts_override &
> -		      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK) >>
> -		     GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_SHIFT);
> -	frac_freq = 1000000 / (frac_freq + 1);
> -
> -	return base_freq + frac_freq;
> -}
> -
>  static u32 gen11_get_crystal_clock_freq(struct intel_uncore *uncore,
>  					u32 rpm_config_reg)
>  {
> @@ -64,12 +47,14 @@ static u32 gen11_read_clock_frequency(struct intel_uncore *uncore)
>  	 * We do not, and we assume nobody else does.
>  	 *
>  	 * First figure out the reference frequency. There are 2 ways
> -	 * we can compute the frequency, either through the
> -	 * TIMESTAMP_OVERRIDE register or through RPM_CONFIG. CTC_MODE
> -	 * tells us which one we should use.
> +	 * we can compute the frequency, either from the display reference
> +	 * clock or through RPM_CONFIG. CTC_MODE tells us which one we should
> +	 * use.
>  	 */
>  	if ((ctc_reg & CTC_SOURCE_PARAMETER_MASK) == CTC_SOURCE_DIVIDE_LOGIC) {
> -		freq = read_reference_ts_freq(uncore);
> +		struct intel_display *display = &uncore->i915->display;
> +
> +		freq = intel_display_get_refclk(display) * 1000;
>  	} else {
>  		u32 c0 = intel_uncore_read(uncore, RPM_CONFIG0);
>  
> @@ -93,7 +78,9 @@ static u32 gen9_read_clock_frequency(struct intel_uncore *uncore)
>  	u32 freq = 0;
>  
>  	if ((ctc_reg & CTC_SOURCE_PARAMETER_MASK) == CTC_SOURCE_DIVIDE_LOGIC) {
> -		freq = read_reference_ts_freq(uncore);
> +		struct intel_display *display = &uncore->i915->display;
> +
> +		freq = intel_display_get_refclk(display) * 1000;
>  	} else {
>  		freq = IS_GEN9_LP(uncore->i915) ? 19200000 : 24000000;
>  
> -- 
> 2.48.1
>