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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 2/22/2026 8:28 AM, Simon Horman wrote:
> 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?

Hi Simon,

Yea, looks like we need to do some adjustments here. Also, the AI review 
I just ran on this is reporting another issue that we'll need to look 
into. I'm going to drop this one from the series to not hold the others 
up on this.

Thanks,
Tony

>> +		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);
>> +	}
>> +}
> 
> [ ... ]