[Intel-wired-lan] [PATCH iwl-next v1 1/1] e1000e: correct TIMINCA on ADP/TGP systems with wrong XTAL frequency

[Lifshits, Vitaly]

On 1/5/2026 7:33 PM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for your patch.
> 
> Am 05.01.26 um 10:57 schrieb Vitaly Lifshits:
>> On some Tiger Lake (TGP) and Alder Lake (ADP) platforms, the hardware
>> XTAL clock is incorrectly configured to 24 MHz instead of the expected
>> 38.4 MHz. This causes the PHC to run significantly faster than system
>> time, breaking PTP synchronization.
> 
> Is that fused into hardware or a firmware issue? Has an errata been 
> published?

It is a BIOS configuration issue that a wrong clock value is set.
There is no errata for it.

> 
>> To mitigate this at runtime, measure PHC vs system time over ~1 ms using
>> cross-timestamps. If the PHC increment differs from system time beyond
>> the expected tolerance (currently >100 uSecs), reprogram TIMINCA for the
>> 38.4 MHz profile and reinitialize the timecounter.
> 
> Why not unconditionally configure it for 38.4 MHz?
> 

Because some of the systems have the 24 MHz clock while others have the 
38.4 MHz. It is impossible to determine the clock by a device ID. 
Anyway, I'll rephrase it in the commit message to make it clearer.

>> Tested on an affected system using phc_ctl:
>> Without fix:
>> sudo phc_ctl enp0s31f6 set 0.0 wait 10 get
>> clock time: 16.000541250 (expected ~10s)
>>
>> With fix:
>> sudo phc_ctl enp0s31f6 set 0.0 wait 10 get
>> clock time: 9.984407212 (expected ~10s)
> 
> Please document at least one affected hardware.

I don't have this data as we can't know for sure which BIOS the 
different vendors use and which of them are configured incorrectly.

> 
> Also, please add the new log message.
> 
>      +        e_dbg("Corrected PHC frequency: TIMINCA set for 38.4 MHz\n");
> 
> I’d make it an info though, and log that it’s a hardware (firmware?) issue.

I prefer leaving it as a debug print because otherwise we will get 
complains from end-users about spamming the dmesg log as this print will 
be visible during every boot.

> 
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits at intel.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/netdev.c | 79 ++++++++++++++++++++++
>>   1 file changed, 79 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ 
>> ethernet/intel/e1000e/netdev.c
>> index 116f3c92b5bc..4ab6897577e5 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -3904,6 +3904,82 @@ 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.
>> + *
>> + * 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.
>> + *
>> + * @adapter: Pointer to the private adapter structure
>> + **/
>> +static void e1000e_xtal_tgp_workaround(struct e1000_adapter *adapter)
>> +{
>> +    s64 phc_delta, sys_delta, sys_start_ns, sys_end_ns, delta;
>> +    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;
> 
> What does the variable name mean?

It is the register's name.

> 
>> +    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 = phc_delta - sys_delta;
>> +    if (delta > 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) |
> 
> Why wrap the line?

Because it passes the 80 characters per line limit.

> 
>> +              (((INCVALUE_38400KHZ <<
>> +                 adapter->cc.shift) &
>> +               E1000_TIMINCA_INCVALUE_MASK));
>> +        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) {
>> +        if (extack._msg)
>> +            e_err("%s\n", extack._msg);
> 
> Is a user able to do anything with this log dump?

I used the same code we used in the original function for consistency.

> 
>> +    }
>> +}
>> +
>>   /**
>>    * e1000e_systim_reset - reset the timesync registers after a 
>> hardware reset
>>    * @adapter: board private structure
>> @@ -3955,6 +4031,9 @@ static void e1000e_systim_reset(struct 
>> e1000_adapter *adapter)
>>           if (extack._msg)
>>               e_err("%s\n", extack._msg);
>>       }
>> +
>> +    if (hw->mac.type == e1000_pch_adp || hw->mac.type == e1000_pch_tgp)
>> +        return e1000e_xtal_tgp_workaround(adapter);
>>   }
>>   /**
> 
> With the changes above addressed, feel free to add
> 
> Reviewed-by: Paul Menzel <pmenzel at molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul