[Intel-wired-lan] [PATCH iwl-next v1 1/1] e1000e: correct TIMINCA on ADP/TGP systems with wrong XTAL frequency
Paul Menzel
pmenzel at molgen.mpg.de
Tue Jan 6 10:29:57 UTC 2026
Dear Vitaly,
Thank you for your reply.
Am 06.01.26 um 10:59 schrieb Lifshits, Vitaly:
> On 1/5/2026 7:33 PM, Paul Menzel wrote:
>> 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.
Interesting. Please add that to the commit message.
>>> 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.
Thank you.
>>> 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.
Please document the system you test this on.
>> 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.
It’s a misconfiguration on the system. Users should know about it, and
otherwise also hardware vendors won’t notice, and can’t fix it.
>>> 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.
Just use `reg` or `reg_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 = 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.
Would the one below work?
timinca =
(INCPERIOD_38400KHZ << E1000_TIMINCA_INCPERIOD_SHIFT) |
>>> + (((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.
Understood.
>>> + }
>>> +}
>>> +
>>> /**
>>> * 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
More information about the Intel-wired-lan
mailing list