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

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

[Simon Horman]

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

[ ... ]

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

[Tony Nguyen]

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

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

[Jakub Kicinski]

On Tue, 24 Feb 2026 14:59:36 -0800 Tony Nguyen wrote:
> 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.

I'd sometimes apply series partially for y'all but FWIW the idpf
"defensive programming instead of proper rollback" patches really
don't make me want to interact with this series more than I have to.
You don't have to rework them. Just expect some delays, I guess.

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

[Tony Nguyen]

On 2/24/2026 4:11 PM, Jakub Kicinski wrote:
> On Tue, 24 Feb 2026 14:59:36 -0800 Tony Nguyen wrote:
>> 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.
> 
> I'd sometimes apply series partially for y'all but FWIW the idpf
> "defensive programming instead of proper rollback" patches really
> don't make me want to interact with this series more than I have to.
> You don't have to rework them. Just expect some delays, I guess.

Hi Jakub,

Yea, when we reviewed them, we tried to come up with a solution to 
suggest that wouldn't need the added checks but couldn't come up with 
anything that wouldn't require large amounts of changes/refactoring. 
I'll send out a new version without this last patch 😐

Thanks,
Tony