[PATCH v2 2/3] drm/xe: Don't try to derive GT clock freq from display register on Xe2

[Lucas De Marchi]

On Fri, Sep 13, 2024 at 09:29:12AM GMT, Matt Roper wrote:
>Driver initialization has two approaches for calculating the GT clock --
>it either derives it from the crystal clock frequency as read from a GT
>register, or it derives it from the display's TIMESTAMP_OVERRIDE
>register.  The driver is informed of which approach needs to be used by
>a bit setting in the CTC_MODE register.
>
>Use of a display register to initialize the GT frequency seems a bit
>suspicious and doesn't seem to really be documented clearly in the
>bspec.  However there is a note on the CTC_MODE register indicating that
>the setting that corresponds to use of the display register (which the
>bspec simply describes as "Broadwell Divide logic") is no longer
>supported on Xe2 and beyond.  Furthermore, the TIMESTAMP_OVERRIDE
>register itself was removed in display version 20.  Rework the GT clock
>init function to only consider CTC_MODE (and then possibly use display's
>TIMESTAMP_OVERRIDE) on pre-Xe2 platforms.
>
>Bspec: 62395
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt_clock.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt_clock.c b/drivers/gpu/drm/xe/xe_gt_clock.c
>index 886c071c10f5..93940dc2c146 100644
>--- a/drivers/gpu/drm/xe/xe_gt_clock.c
>+++ b/drivers/gpu/drm/xe/xe_gt_clock.c
>@@ -43,29 +43,29 @@ static u32 get_crystal_clock_freq(u32 rpm_config_reg)
> int xe_gt_clock_init(struct xe_gt *gt)
> {
> 	struct xe_device *xe = gt_to_xe(gt);
>-	u32 ctc_reg = xe_mmio_read32(&gt->mmio, CTC_MODE);
>-	u32 freq = 0;
>+	u32 c0, freq = 0;
>
> 	/* Assuming gen11+ so assert this assumption is correct */
> 	xe_gt_assert(gt, GRAPHICS_VER(xe) >= 11);
>
>-	if (ctc_reg & CTC_SOURCE_DIVIDE_LOGIC) {
>+	if (GRAPHICS_VER(gt_to_xe(gt)) < 20 &&
>+	    xe_mmio_read32(&gt->mmio, CTC_MODE) & CTC_SOURCE_DIVIDE_LOGIC) {
> 		xe_gt_assert(gt, xe->info.probe_display);
>+		gt->info.reference_clock = xe_display_read_ref_ts_freq(xe);

I'd have left the assigment to gt->info.reference_clock in just one
place, at the end of this function... and use the convention "latest
platforms first). But that's a nit.

Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>


thanks
Lucas De Marchi

>
>-		freq = xe_display_read_ref_ts_freq(xe);
>-	} else {
>-		u32 c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>-
>-		freq = get_crystal_clock_freq(c0);
>-
>-		/*
>-		 * Now figure out how the command stream's timestamp
>-		 * register increments from this frequency (it might
>-		 * increment only every few clock cycle).
>-		 */
>-		freq >>= 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, c0);
>+		return 0;
> 	}
>
>+	c0 = xe_mmio_read32(&gt->mmio, RPM_CONFIG0);
>+	freq = get_crystal_clock_freq(c0);
>+
>+	/*
>+	 * Now figure out how the command stream's timestamp
>+	 * register increments from this frequency (it might
>+	 * increment only every few clock cycle).
>+	 */
>+	freq >>= 3 - REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, c0);
>+
> 	gt->info.reference_clock = freq;
> 	return 0;
> }
>-- 
>2.45.2
>