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 <[email protected]>
---
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 <[email protected]>
Kind regards,
Paul
