Re: [net,13/13] e1000e: correct TIMINCA on ADP/TGP systems with wrong XTAL frequency

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
e1000e: correct TIMINCA on ADP/TGP systems with wrong XTAL frequency

On some Tiger Lake (TGP) and Alder Lake (ADP) platforms, the hardware
XTAL clock is incorrectly interpreted as 24 MHz instead of the actual
38.4 MHz. The commit measures PHC vs system time over 1 ms and if the
delta exceeds 100 microseconds, reprograms TIMINCA for the 38.4 MHz
profile and reinitializes the timecounter.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 814698807f3d..ae35ac479026 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c

[ ... ]

> @@ -3905,6 +3905,80 @@ static void e1000_flush_desc_rings(struct e1000_adapter *adapter)
>  		e1000_flush_rx_ring(adapter);
>  }
>
> +/**
> + * e1000e_xtal_tgp_workaround - Adjust XTAL clock based on PHC and system
> + * clock delta.
> + * @adapter: Pointer to the private adapter structure
> + *
> + * Measures the time difference between the PHC (Precision Hardware Clock)
> + * and the system clock over a 1 millisecond interval. If the delta
> + * exceeds 100 microseconds, reconfigure the XTAL clock to 38.4 MHz.
> + */
> +static void e1000e_xtal_tgp_workaround(struct e1000_adapter *adapter)
> +{
> +	s64 phc_delta, sys_delta, sys_start_ns, sys_end_ns, delta_ns;
> +	struct ptp_system_timestamp sys_start = {}, sys_end = {};
> +	struct ptp_clock_info *info = &adapter->ptp_clock_info;
> +	struct timespec64 phc_start, phc_end;
> +	struct e1000_hw *hw = &adapter->hw;
> +	struct netlink_ext_ack extack = {};
> +	unsigned long flags;
> +	u32 timinca;
> +	s32 ret_val;
> +
> +	/* Capture start */
> +	if (info->gettimex64(info, &phc_start, &sys_start)) {
> +		e_dbg("PHC gettimex(start) failed\n");
> +		return;
> +	}
> +
> +	/* Small interval to measure increment */
> +	usleep_range(1000, 1100);
> +
> +	/* Capture end */
> +	if (info->gettimex64(info, &phc_end, &sys_end)) {
> +		e_dbg("PHC gettimex(end) failed\n");
> +		return;
> +	}
> +
> +	/* Compute deltas */
> +	phc_delta = timespec64_to_ns(&phc_end) -
> +		    timespec64_to_ns(&phc_start);
> +
> +	sys_start_ns = (timespec64_to_ns(&sys_start.pre_ts) +
> +			timespec64_to_ns(&sys_start.post_ts)) >> 1;
> +
> +	sys_end_ns = (timespec64_to_ns(&sys_end.pre_ts) +
> +		      timespec64_to_ns(&sys_end.post_ts)) >> 1;
> +
> +	sys_delta = sys_end_ns - sys_start_ns;
> +
> +	delta_ns = phc_delta - sys_delta;
> +	if (delta_ns > 100000) {
> +		e_dbg("Corrected PHC frequency: TIMINCA set for 38.4 MHz\n");
> +		/* Program TIMINCA for 38.4 MHz */
> +		timinca = (INCPERIOD_38400KHZ <<
> +			   E1000_TIMINCA_INCPERIOD_SHIFT) |
> +			  (((INCVALUE_38400KHZ <<
> +			     adapter->cc.shift) &
> +			   E1000_TIMINCA_INCVALUE_MASK));

Is adapter->cc.shift correct here? When e1000e_xtal_tgp_workaround runs,
the earlier call to e1000e_get_base_timinca (via e1000e_phc_adjfine in
e1000e_systim_reset) has already set adapter->cc.shift based on the
incorrectly reported 24 MHz XTAL frequency.

On affected hardware where SYSCFI reports 24 MHz:

e1000e_systim_reset()
 -> e1000e_phc_adjfine(0)
    -> e1000e_get_base_timinca() sets:
       adapter->cc.shift = INCVALUE_SHIFT_24MHZ (14)
 -> e1000e_xtal_tgp_workaround()

So when computing TIMINCA for 38.4 MHz, the code uses shift=14:

  INCVALUE_38400KHZ << 14 = 26 << 14 = 425984

But the 38.4 MHz profile needs shift=19:

  INCVALUE_38400KHZ << 19 = 26 << 19 = 13631488

This produces timinca = 17203200 instead of the correct 30408704.

Additionally, the subsequent timecounter_init call uses adapter->cc which
still has shift=14. This computes tc->mask = (1ULL << 14) - 1 = 16383
instead of the correct (1ULL << 19) - 1 = 524287.

Should the function update adapter->cc.shift = INCVALUE_SHIFT_38400KHZ
before computing timinca and calling timecounter_init, matching the
pattern in e1000e_get_base_timinca for the 38.4 MHz profile?

> +		ew32(TIMINCA, timinca);
> +
> +		/* reset the systim ns time counter */
> +		spin_lock_irqsave(&adapter->systim_lock, flags);
> +		timecounter_init(&adapter->tc, &adapter->cc,
> +				 ktime_to_ns(ktime_get_real()));
> +		spin_unlock_irqrestore(&adapter->systim_lock, flags);
> +
> +		/* restore the previous hwtstamp configuration settings */
> +		ret_val = e1000e_config_hwtstamp(adapter,
> +						 &adapter->hwtstamp_config,
> +						 &extack);
> +		if (ret_val && extack._msg)
> +			e_err("%s\n", extack._msg);
> +	}
> +}

[ ... ]