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

[Paul Menzel]

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?

> 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?

> 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.

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.

> 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?

> +	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?

> +			  (((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?

> +	}
> +}
> +
>   /**
>    * 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