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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
e1000e: correct TIMINCA on ADP/TGP systems with wrong XTAL frequency

This commit adds a runtime workaround for some Tiger Lake and Alder Lake
platforms where the XTAL clock is misreported as 24 MHz instead of the
actual 38.4 MHz. After e1000e_systim_reset() it samples PHC vs system
time over ~1 ms via cross-timestamps and, if the delta exceeds 100 us,
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 7ce0cc8ab8f4..db2080541f19 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -3902,6 +3902,81 @@ 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.
> + */

[Low]
The kernel-doc says "Adjust XTAL clock" and the body comment below says
"reconfigure the XTAL clock to 38.4 MHz", but nothing here reprograms a
crystal or PLL.  The function only writes TIMINCA and updates
adapter->cc.shift / adapter->tc to compensate in software for the
misreported XTAL frequency.  Could the comments be reworded to describe
what is actually being changed (TIMINCA / cyclecounter shift) so that
future readers do not look for an XTAL register write that does not
exist?

> +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) {

[Low]
Is a single ~1 ms sample with a one-directional threshold robust enough
to commit to a permanent (until-next-reset) clock-configuration change?

Scheduling jitter, virtualization steal time on a guest, or an SMI/NMI
that straddles either gettimex64 capture can skew sys_delta or phc_delta
in either direction.  There is no sanity bound on sys_delta (for example,
800 us..1.5 ms) and no median/min over multiple samples.

The commit message phrasing "differs from system time beyond the
expected tolerance" reads as a tolerance check, but the code only catches
the positive direction, so a hardware that runs slow than reference
time would not be flagged.  Would a multi-sample median, a sys_delta
sanity range, or an abs() comparison make detection less fragile?

> +		e_dbg("Corrected PHC frequency: TIMINCA set for 38.4 MHz\n");
> +		/* Program TIMINCA for 38.4 MHz */
> +		spin_lock_irqsave(&adapter->systim_lock, flags);
> +		adapter->cc.shift = INCVALUE_SHIFT_38400KHZ;
> +		timinca = (INCPERIOD_38400KHZ <<
> +			   E1000_TIMINCA_INCPERIOD_SHIFT) |
> +			  (((INCVALUE_38400KHZ <<
> +			     adapter->cc.shift) &
> +			   E1000_TIMINCA_INCVALUE_MASK));
> +		ew32(TIMINCA, timinca);

[High]
Is this correction durable against the next ptp4l/phc2sys adjfine call?

After this branch sets adapter->cc.shift = INCVALUE_SHIFT_38400KHZ and
writes the 38.4 MHz TIMINCA, e1000e_phc_adjfine() in
drivers/net/ethernet/intel/e1000e/ptp.c calls e1000e_get_base_timinca()
on every adjfine, which on these parts hits:

    case e1000_pch_cnp:
    case e1000_pch_tgp:
    case e1000_pch_adp:
    case e1000_pch_nvp:
        if (er32(TSYNCRXCTL) & E1000_TSYNCRXCTL_SYSCFI) {
            /* Stable 24MHz frequency */
            incperiod = INCPERIOD_24MHZ;
            incvalue  = INCVALUE_24MHZ;
            shift     = INCVALUE_SHIFT_24MHZ;
            adapter->cc.shift = shift;
        } else {
            ...
        }

On the affected systems the SYSCFI bit is the very thing that is
mis-reported, so e1000e_get_base_timinca() takes the 24 MHz branch
again, overwrites adapter->cc.shift back to INCVALUE_SHIFT_24MHZ, and
e1000e_phc_adjfine() programs a 24 MHz-based TIMINCA back into the
register.  ptp4l/phc2sys issue adjfine continuously, so the correction
appears to last only until the first adjfine after each systim_reset.

Could the detection set a per-adapter "force 38.4 MHz" flag on first
trip, and have e1000e_get_base_timinca() honor that flag so the 38.4 MHz
selection becomes durable across adjfine and any future reset?

[Medium]
Does this path drop the user-space-applied frequency adjustment from
hardware?

e1000e_systim_reset() first calls info->adjfine(info,
adapter->ptp_delta), which goes through e1000e_phc_adjfine() and
programs TIMINCA with the per-ppm-scaled INCVALUE via
adjust_by_scaled_ppm():

    incvalue = timinca & E1000_TIMINCA_INCVALUE_MASK;
    incvalue = adjust_by_scaled_ppm(incvalue, delta);
    timinca &= ~E1000_TIMINCA_INCVALUE_MASK;
    timinca |= incvalue;
    ew32(TIMINCA, timinca);

Immediately after, this hunk unconditionally writes a clean 38.4 MHz
baseline:

    timinca = (INCPERIOD_38400KHZ << E1000_TIMINCA_INCPERIOD_SHIFT) |
              (((INCVALUE_38400KHZ << adapter->cc.shift) &
                E1000_TIMINCA_INCVALUE_MASK));
    ew32(TIMINCA, timinca);

adapter->ptp_delta is left in software but is no longer reflected in
hardware, so any frequency calibration the PTP daemon had previously
servoed onto is discarded until it re-converges.  Should the workaround
re-apply the saved delta (for example via info->adjfine(info,
adapter->ptp_delta), or by feeding the new incvalue through
adjust_by_scaled_ppm()) before re-initializing the timecounter?

> +
> +		/* reset the systim ns time counter */
> +		timecounter_init(&adapter->tc, &adapter->cc,
> +				 ktime_get_real_ns());
> +		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);
> +	}
> +}
> +
>  /**
>   * e1000e_systim_reset - reset the timesync registers after a hardware reset
>   * @adapter: board private structure
> @@ -3953,6 +4028,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)
> +		e1000e_xtal_tgp_workaround(adapter);
>  }

Because this hook runs on every systim_reset (ifup, suspend/resume,
error recovery), and e1000e_get_base_timinca() picks 24 MHz first,
there is also a window between the 24 MHz programming above and the
1 ms sleep + rewrite inside e1000e_xtal_tgp_workaround() during which
TIMINCA is wrong.  Hardware timestamps captured during that window
would use the 24 MHz cyclecounter conversion.  Is that intentional?