* [PATCH iwl-net 0/2] ice: E825 timer adjustment fixes
@ 2026-10-24 1:24 Jacob Keller
2026-10-24 1:24 ` [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter Jacob Keller
2026-10-24 1:24 ` [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830 Jacob Keller
0 siblings, 2 replies; 5+ messages in thread
From: Jacob Keller @ 2026-10-24 1:24 UTC (permalink / raw)
To: Arkadiusz Kubalewski, Sergey Temerkhanov, Przemek Kitszel,
Intel Wired LAN, Przemyslaw Korba, Grzegorz Nitka,
Maciej Machnikowski
Cc: Jacob Keller
Recently we've had reports of bad timer adjustment on E825. This series
fixes the issues by replacing direct timer adjustments with a struct
timecounter to maintain the phase in software instead.
To motivate this, the following is an analysis of how the E825 PHY
timestamps function.
The ice driver maintains PTP clock time in the MAC with a 96-bit
register. Timestamps are captured in the PHY which only stores 64
bits, only including the lower 32-bits of nanoseconds. The driver
performs clock adjustments using a hardware timer command mechanism,
where the clock adjustment is programmed into shadow registers in
both the MAC and PHY, and then a timer command is initiated to
synchronously update the timers.
Since the PHY only maintains the lower 32 bits of nanoseconds,
adjustments are also limited such that the maximum atomic adjustment
is ~2.1 seconds (2^31-1 nanoseconds).
For large offsets, the driver uses a non-atomic flow where the
current time is read, an adjustment is applied in software, and then
written back. This necessarily loses some elapsed time, which
applications must deal with. For particular servo configurations,
where the parameters are tuned for a very slow servo, this error can
take a long time to finish correction.
On E825, the adjustment in the PHY does not always work reliably.
The reason for this is subtle, and is a quirk of how the timestamps
are actually generated. The timestamps for Tx and Rx packets come
from the PCS timer rather than directly from the PHY timer. The PCS
timer is derived from internal PHY signaling.
To keep the PCS timer and PHY timer in sync, every few cycles
(testing seems to indicate every 8 cycles) the PHY timer is copied
to the PCS timer. On other cycles, the PCS timer is incremented by a
value determined from the PCS clock rate.
Crucially, the copy of the PHY timer is skipped if the current value
of the PHY timer is lower than the PCS timer (*and* on this cycle
the regular PCS clock increment is also skipped).
During normal operation, the PCS timer will tick from its own source
for 7 ticks then copy the PCS timer. If the PCS timer is slightly
ahead, then PHY timer update is skipped, and the PCS timer skips an
update every 8 clock cycles until it is back in alignment with the
PHY timer.
Unfortunately, this hardware logic did not take into account manual
adjustments to the PHY. If the PHY timer is adjusted backwards, (or
if a forward adjustment causes the PHY timer to overflow), the PHY
to PCS sync will not occur again until the PCS timer wraps back
around to zero. In the worst case, an adjustment could land the PHY
timer to just below the PCS timer just after it wraps around. PHY to
PCS syncs would stop for ~3.6 seconds.
During this time, the Tx and Rx timestamps will read from the PCS
timer. These timestamps will be fed into the timestamp extension
algorithm which will produce incoherent results, and data will be
fed into the PTP application servo. During the invalid window,
faulty timestamps will cause the servo to steer incorrectly, likely
away from the true target time. After the PCS timer synchronizes
again, the timestamps will return to normal. However, the servo may
have steered the clock incorrectly. In default ptp4l configurations,
direct clock adjustment is only performed once during startup. This
forces the application to use frequency slewing to correct any
remaining offset. With slow servo parameters, this can take a *very*
long time. In some setups, we had reports of taking several minutes
to nearly an hour before stabilizing, and in some configurations the
servo was unable to ever recover.
Note that adjustments work properly on the MAC timer, and indeed on
the first-layer PHY timer value that the driver can read over the
sideband queue. Only the PCS timer (which is inaccessible to the
driver) fails to adjust. This made tracking the precise origin of
the failure difficult.
Testing shows that pretty much all "negative" adjustments and about half
of the positive adjustments can trigger this failure. It is also
difficult to see without using extremely slow servo parameters. With a
servo tuned for fast reaction, the incorrect timestamps will cause some
difficulty in the first adjustment, but the servo will quickly correct
and recover within a few seconds at most.
I used the following P/I servo parameters in my testing:
pi_proportional_const 0.60
pi_integral_const 0.001
pi_proportional_scale 0.0
pi_proportional_exponent -0.3
pi_proportional_norm_max 0.7
pi_integral_scale 0.0
pi_integral_exponent 0.4
pi_integral_norm_max 0.3
In particular, the pi_integral_const will force the servo to be very
slow at reacting.
Even trying to compare the MAC timer with the Tx and Rx timestamps is
not trivial. There is no mechanism to synchronously read the MAC timer
along with a timestamp. Additionally, timestamp extension makes this even
harder to track. The MAC timer is immediately adjusted and the cached PHC
time is updated. Normally timestamps from before the adjustment should
reasonably convert, however if the adjustment is large enough the timestamp
can flip to being detected as "in the past" and in the worst cases the
resulting extended timestamps will have a second sudden jump of -4.2
seconds before the PCS timer adjustment is applied and things settle. All
of this while being fed into a servo trying to steer the clock can make it
difficult to understand what is going on.
With that background out of the way, here's a summary of the patch content:
First, the timestamp extension logic is refactored to store the cached
PHC time value inside the ice_adapter structure under a new ice_phc
structure which will be used to store timecounter data. This change
reduces the number of threads that need to perform timestamp extension
and makes introduction of timecounter simpler as it gives a natural
place for accessing the structure and simplifies modifications to the Rx
hotpath in particular.
Next, the driver is modified to allow use of a timecounter for
representing the time instead of directly adjusting the phase in
hardware. Frequency adjustments continue to be handled in hardware. For
now, only the E825 and E830 devices are switched. The legacy
implementation remains for supporting older installations of E810 and
E822 platform devices. We plan to complete migration in the future, but
felt that immediate conversion would slow down ability to validate that
all older installations will behave correctly. Since the E825 device
timer adjustment logic is completely useless, this can speed up ability
to adopt the fix. Although E830 does not appear to have the same timer
command issue, it was decided to convert both since they are the most
recent devices and undergoing the most testing and active development.
Although I would normally rate conversion to timecounter as a "feature", I
have submitted this series as a fix due to the severity of the failures
encountered on E825.We would prefer to be able to avoid delays and help get
this to critical customers soon.
This series fixes a separate issue from [1] and does not *directly* depend
on it. The fixes in that series are necessary but not sufficient to
completely eliminate PHY timer desync issues on E825.
There are some structural conflicts with the ctrl_pf RCU changes at [2]
which I have tried to integrate, so this won't be a straightforward
cherry-pick to trees without that series.
One final note, the conversion to timecounter supersedes the changes
from [3], and that patch has been removed from more recent versions of
that series. The reset partially helped, but was only applied to the
non-atomic settime interface. In particular, the settime interface was
vulnerable to the same PCS issue, but the PHY soft reset *does* clear
both the PHY and PCS timers back to 0. Performing a reset after the
adjustment will force both timers back to zero and then perform a
forward adjustment that won't trigger the failure condition. However,
attempting to add this reset to *every* timer adjustment comes with
unnecessary delay since the whole process takes a long time especially
with the PHY soft reset. It was ultimately deemed as not a proper fix
suitable for deployment.
Link: [1] https://lore.kernel.org/intel-wired-lan/20260925222824.270640-1-grzegorz.nitka@intel.com/
Link: [2] https://lore.kernel.org/intel-wired-lan/20261013091235.1423979-1-sergey.temerkhanov@intel.com/
Link: [3] https://lore.kernel.org/netdev/20261008215614.1987250-16-anthony.l.nguyen@intel.com/
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Jacob Keller (2):
ice: store cached PHC time in ice_adapter
ice: use struct timecounter for E825 and E830
drivers/net/ethernet/intel/ice/ice_adapter.h | 42 +++
drivers/net/ethernet/intel/ice/ice_ptp.h | 12 +-
drivers/net/ethernet/intel/ice/ice_txrx.h | 4 +-
drivers/net/ethernet/intel/ice/ice_adapter.c | 10 +
drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
drivers/net/ethernet/intel/ice/ice_lib.c | 2 +-
drivers/net/ethernet/intel/ice/ice_ptp.c | 381 +++++++++++++++++----------
7 files changed, 311 insertions(+), 142 deletions(-)
---
base-commit: bfd1885cfb920d10c2739c9f152615ec26e3b045
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git dev-queue
change-id: 20261022-jk-e825-e830-timecounter-06c15de02e76
Best regards,
--
Jacob Keller <jacob.e.keller@intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter
2026-10-24 1:24 [PATCH iwl-net 0/2] ice: E825 timer adjustment fixes Jacob Keller
@ 2026-10-24 1:24 ` Jacob Keller
2026-10-24 1:46 ` sashiko-bot
2026-10-24 1:24 ` [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830 Jacob Keller
1 sibling, 1 reply; 5+ messages in thread
From: Jacob Keller @ 2026-10-24 1:24 UTC (permalink / raw)
To: Arkadiusz Kubalewski, Sergey Temerkhanov, Przemek Kitszel,
Intel Wired LAN, Przemyslaw Korba, Grzegorz Nitka,
Maciej Machnikowski
Cc: Jacob Keller
The ice driver uses a cached copy of the PTP Hardware Clock time for its
timestamp extension algorithm implemented in ice_ptp_extend_32b_ts(). This
algorithm is based on work originally from struct timecounter, and relies
on a cached copy of the PTP time used to extend 32-bit snapshots of the PTP
time into full 64-bit time values.
The driver currently stores this cache once in the ice_ptp structure for
each PF and once per each Rx ring on each PF. Each PF is then responsible
for calling ice_ptp_update_cached_phctime() at least once every 2 seconds.
This is handled by the per-PF periodic work task that runs on a per-PF
kthread. Each Rx ring must be updated, which requires looping the VSI and
ring structures, and might fail due to the rings being reconfigured.
In order to fix some device-specific bugs, a following change is going to
refactor the driver to support using a timecounter structure to avoid
needing to perform direct time adjustments in hardware on E825 and E830.
Use of a timecounter will require the that the Rx path have a reference to
that structure. To ensure that all PFs on the same device have the same
time domain, the structure must be part of the whole adapter, making
ice_adapter the best place to store it. The initial implementation will
focus only on migrating the E825 and E830 devices, to minimize impact and
potential regression.
Since the Rx path will need to reference a structure anyways, it makes
little sense to maintain both a cached copy of the PHC time in addition to
a pointer to the adapter.
As a first step, refactor to store the existing cached PHC time in
ice_adapter, instead of per-PF in each ice_ptp structure and the Rx rings.
Create a struct ice_phc structure to store the PTP Hardware Clock fields,
and embed it in ice_adapter. Store a pointer to the PHC structure in the Rx
rings, replacing the cached_phctime.
Pass a pointer to the PHC structure to ice_ptp_extend_32b_ts() and obtain
the cached time from that structure.
Modify the ice_ptp_update_cached_phctime(), removing logic to update the
cached time within the Rx rings. Convert it to a void function since it can
no longer fail in a meaningful way. Note that the previous logic could
return -ENODEV if ice_ptp_read_src_clk_reg() couldn't access the PHC.
However, the existing callers do not meaningfully respond to this error.
Change ice_ptp_periodic_work() to check ice_pf_src_tmr_owned() and only
call ice_ptp_update_cached_phctime() for the clock owner.
Separate the PHC reset time from the cached time value in
ice_ptp_rebuild_owner(), rather than relying on the cached_phc_time. Read
the value of the PHC time near the same time that we save ptp->reset_time.
This eliminates the failure point on updating the cached PHC time, and
reduces needlessly duplicated work of having each PF update its own cached
time copy. The Rx path now requires accessing the PHC structure via a
dereference and this may theoretically impact performance due to changes
in caching. However, the timecounter refactor will require doing at least
this much anyways.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_adapter.h | 13 +++
drivers/net/ethernet/intel/ice/ice_ptp.h | 6 +-
drivers/net/ethernet/intel/ice/ice_txrx.h | 4 +-
drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
drivers/net/ethernet/intel/ice/ice_lib.c | 2 +-
drivers/net/ethernet/intel/ice/ice_ptp.c | 145 +++++++++------------------
6 files changed, 64 insertions(+), 108 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index a91f99f4ce30..a56dc5306541 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -34,6 +34,16 @@ struct ice_port_list {
struct srcu_struct srcu;
};
+/**
+ * struct ice_phc - PTP Hardware Clock fields
+ * @cached_time: Cached copy of the PHC time for timestamp extension
+ * @update_jiffies: Jiffies when PHC was last updated
+ */
+struct ice_phc {
+ u64 cached_time;
+ unsigned long update_jiffies;
+};
+
/**
* struct ice_adapter - PCI adapter resources shared across PFs
* @refcount: Reference count. struct ice_pf objects hold the references.
@@ -43,6 +53,7 @@ struct ice_port_list {
* @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
* Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
* @ps_lock: Mutex to serialize PHY port start/stop across adapter.
+ * @phc: PTP Hardware Clock fields
* @ctrl_pf_lock: Protect control PF lifetime for sleepable users
* @ctrl_pf: Control PF of the adapter
* @rebuild_lock: serialize PFR recovery across PFs of the same adapter
@@ -63,6 +74,8 @@ struct ice_adapter {
/* For serializing PHY port start/stop sequences */
struct mutex ps_lock;
+ struct ice_phc phc;
+
struct rw_semaphore ctrl_pf_lock;
struct ice_pf __rcu *ctrl_pf;
struct ice_port_list ports;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 40a0e654c2c6..430441b43150 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -235,8 +235,6 @@ struct ice_ptp_pin_desc {
* @tx_interrupt_mode: the TX interrupt mode for the PTP clock
* @port: data for the PHY port initialization procedure
* @work: delayed work function for periodic tasks
- * @cached_phc_time: a cached copy of the PHC time for timestamp extension
- * @cached_phc_jiffies: jiffies when cached_phc_time was last updated
* @kworker: kwork thread for handling periodic work
* @ext_ts_irq: the external timestamp IRQ in use
* @pin_desc: structure defining pins
@@ -248,6 +246,7 @@ struct ice_ptp_pin_desc {
* @tstamp_config: hardware timestamping configuration
* @tx_refclks: bitmaps table to store the information about TX reference clocks
* @reset_time: kernel time after clock stop on reset
+ * @phc_reset_time: PHC time at clock stop on reset
* @tx_hwtstamp_good: number of completed Tx timestamp requests
* @tx_hwtstamp_skipped: number of Tx time stamp requests skipped
* @tx_hwtstamp_timeouts: number of Tx skbs discarded with no time stamp
@@ -270,8 +269,6 @@ struct ice_ptp {
enum ice_ptp_tx_interrupt tx_interrupt_mode;
struct ice_ptp_port port;
struct kthread_delayed_work work;
- u64 cached_phc_time;
- unsigned long cached_phc_jiffies;
struct kthread_worker *kworker;
u8 ext_ts_irq;
struct ptp_pin_desc pin_desc[ICE_N_PINS_MAX];
@@ -283,6 +280,7 @@ struct ice_ptp {
struct kernel_hwtstamp_config tstamp_config;
unsigned long tx_refclks[ICE_E825_MAX_PHYS][ICE_REF_CLK_MAX];
u64 reset_time;
+ u64 phc_reset_time;
u64 tx_hwtstamp_good;
u32 tx_hwtstamp_skipped;
u32 tx_hwtstamp_timeouts;
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index 5e517f219379..83a34322e0d3 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -221,7 +221,7 @@ enum ice_tx_ring_flags {
};
struct ice_pkt_ctx {
- u64 cached_phctime;
+ struct ice_phc *phc;
__be16 vlan_proto;
};
@@ -321,7 +321,7 @@ struct ice_rx_ring {
union {
struct ice_pkt_ctx pkt_ctx;
struct {
- u64 cached_phctime;
+ struct ice_phc *phc;
__be16 vlan_proto;
};
};
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 90fa11b22820..35dff5e93740 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3338,7 +3338,7 @@ ice_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
/* clone ring and setup updated count */
rx_rings[i] = *vsi->rx_rings[i];
rx_rings[i].count = new_rx_cnt;
- rx_rings[i].cached_phctime = pf->ptp.cached_phc_time;
+ rx_rings[i].phc = &pf->adapter->phc;
rx_rings[i].desc = NULL;
rx_rings[i].xdp_buf = NULL;
rx_rings[i].xdp_rxq = (struct xdp_rxq_info){ };
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 5bd9deca53e7..d4d97e3acc3a 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -1533,7 +1533,7 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
ring->vsi = vsi;
ring->netdev = vsi->netdev;
ring->count = vsi->num_rx_desc;
- ring->cached_phctime = pf->ptp.cached_phc_time;
+ ring->phc = &pf->adapter->phc;
if (ice_is_feature_supported(pf, ICE_F_GCS))
ring->flags |= ICE_RX_FLAGS_RING_GCS;
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index ec24c0928078..4b4a00beadd2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -274,7 +274,7 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
/**
* ice_ptp_extend_32b_ts - Convert a 32b nanoseconds timestamp to 64b
- * @cached_phc_time: recently cached copy of PHC time
+ * @phc: PTP Hardware Clock structure
* @in_tstamp: Ingress/egress 32b nanoseconds timestamp value
*
* Hardware captures timestamps which contain only 32 bits of nominal
@@ -310,13 +310,17 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
* a second, and (b) discarding any Tx timestamp packet if it has waited for
* a timestamp for more than one second.
*/
-static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
+static u64 ice_ptp_extend_32b_ts(struct ice_phc *phc, u32 in_tstamp)
{
u32 delta, phc_time_lo;
- u64 ns;
+ u64 ns, cached_time;
- /* Extract the lower 32 bits of the PHC time */
- phc_time_lo = (u32)cached_phc_time;
+ cached_time = READ_ONCE(phc->cached_time);
+ /* Do not report a timestamp if we don't have a cached PHC time */
+ if (!cached_time)
+ return 0;
+
+ phc_time_lo = (u32)cached_time;
/* Calculate the delta between the lower 32bits of the cached PHC
* time and the in_tstamp value
@@ -331,9 +335,9 @@ static u64 ice_ptp_extend_32b_ts(u64 cached_phc_time, u32 in_tstamp)
if (delta > (U32_MAX / 2)) {
/* reverse the delta calculation here */
delta = (phc_time_lo - in_tstamp);
- ns = cached_phc_time - delta;
+ ns = cached_time - delta;
} else {
- ns = cached_phc_time + delta;
+ ns = cached_time + delta;
}
return ns;
@@ -372,7 +376,7 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
u64 cached_time;
/* Discard the hardware timestamp if the cached PHC time is too old */
- discard_time = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
+ discard_time = pf->adapter->phc.update_jiffies + msecs_to_jiffies(2000);
if (time_is_before_jiffies(discard_time)) {
pf->ptp.tx_hwtstamp_discarded++;
return 0;
@@ -382,13 +386,14 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
* how ice_ptp_update_cached_phctime() reports that the source clock
* has become unreachable.
*/
- cached_time = READ_ONCE(pf->ptp.cached_phc_time);
+ cached_time = READ_ONCE(pf->adapter->phc.cached_time);
if (!cached_time) {
pf->ptp.tx_hwtstamp_discarded++;
return 0;
}
- return ice_ptp_extend_32b_ts(cached_time, (in_tstamp >> 8) & mask);
+ return ice_ptp_extend_32b_ts(&pf->adapter->phc,
+ (in_tstamp >> 8) & mask);
}
/**
@@ -987,35 +992,26 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
}
/**
- * ice_ptp_update_cached_phctime - Update the cached PHC time values
+ * ice_ptp_update_cached_phctime - Update the cached PHC time value
* @pf: Board specific private structure
*
- * This function updates the system time values which are cached in the PF
- * structure and the Rx rings.
+ * Update the PTP hardware clock time values which are cached in the PHC
+ * structure.
*
- * This function must be called periodically to ensure that the cached value
- * is never more than 2 seconds old.
- *
- * Note that the cached copy in the PF PTP structure is always updated, even
- * if we can't update the copy in the Rx rings.
- *
- * Return:
- * * 0 - OK, successfully updated
- * * -EAGAIN - PF was busy, need to reschedule the update
- * * -ENODEV - no control PF owns the source clock register space
- * * -EOPNOTSUPP - PTP support is not compiled in
+ * This function must be called periodically by the clock owner to ensure that
+ * the cached value is never more than 2 seconds old.
*/
-static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
+static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
{
+ struct ice_phc *phc = &pf->adapter->phc;
struct device *dev = ice_pf_to_dev(pf);
unsigned long update_before;
u64 systime;
- int err, i;
+ int err;
- update_before = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
- if (pf->ptp.cached_phc_time &&
- time_is_before_jiffies(update_before)) {
- unsigned long time_taken = jiffies - pf->ptp.cached_phc_jiffies;
+ update_before = phc->update_jiffies + msecs_to_jiffies(2000);
+ if (phc->cached_time && time_is_before_jiffies(update_before)) {
+ unsigned long time_taken = jiffies - phc->update_jiffies;
dev_warn(dev, "%u msecs passed between update to cached PHC time\n",
jiffies_to_msecs(time_taken));
@@ -1032,36 +1028,12 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
* longer tracks the PHC, and both treat a zero cache as
* "no timestamp".
*/
- systime = 0;
- WRITE_ONCE(pf->ptp.cached_phc_time, 0);
+ WRITE_ONCE(phc->cached_time, 0);
} else {
- /* Update the cached PHC time stored in the PF structure */
- WRITE_ONCE(pf->ptp.cached_phc_time, systime);
- WRITE_ONCE(pf->ptp.cached_phc_jiffies, jiffies);
+ /* Update the cached PHC time stored in the PHC structure */
+ WRITE_ONCE(phc->cached_time, systime);
+ WRITE_ONCE(phc->update_jiffies, jiffies);
}
-
- if (test_and_set_bit(ICE_CFG_BUSY, pf->state))
- return -EAGAIN;
-
- ice_for_each_vsi(pf, i) {
- struct ice_vsi *vsi = pf->vsi[i];
- int j;
-
- if (!vsi)
- continue;
-
- if (vsi->type != ICE_VSI_PF)
- continue;
-
- ice_for_each_rxq(vsi, j) {
- if (!vsi->rx_rings[j])
- continue;
- WRITE_ONCE(vsi->rx_rings[j]->cached_phctime, systime);
- }
- }
- clear_bit(ICE_CFG_BUSY, pf->state);
-
- return err;
}
/**
@@ -1070,34 +1042,11 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
*
* This function must be called when the cached PHC time is no longer valid,
* such as after a time adjustment. It marks any currently outstanding Tx
- * timestamps as stale and updates the cached PHC time for both the PF and Rx
- * rings.
- *
- * If updating the PHC time cannot be done immediately, a warning message is
- * logged and the work item is scheduled immediately to minimize the window
- * with a wrong cached timestamp.
+ * timestamps as stale and updates the cached PHC time.
*/
static void ice_ptp_reset_cached_phctime(struct ice_pf *pf)
{
- struct device *dev = ice_pf_to_dev(pf);
- int err;
-
- /* Update the cached PHC time immediately if possible, otherwise
- * schedule the work item to execute soon.
- */
- err = ice_ptp_update_cached_phctime(pf);
- if (err) {
- /* If another thread is updating the Rx rings, we won't
- * properly reset them here. This could lead to reporting of
- * invalid timestamps, but there isn't much we can do.
- */
- dev_warn(dev, "%s: unable to immediately update cached PHC time, err %d\n",
- __func__, err);
-
- /* Queue the work item to update the Rx rings when possible */
- kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work,
- msecs_to_jiffies(10));
- }
+ ice_ptp_update_cached_phctime(pf);
/* Mark any outstanding timestamps as stale, since they might have
* been captured in hardware before the time update. This could lead
@@ -2464,9 +2413,7 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
if (!(rx_desc->wb.time_stamp_low & ICE_PTP_TS_VALID))
return 0;
- cached_time = READ_ONCE(pkt_ctx->cached_phctime);
-
- /* Do not report a timestamp if we don't have a cached PHC time */
+ cached_time = READ_ONCE(pkt_ctx->phc->cached_time);
if (!cached_time)
return 0;
@@ -2477,7 +2424,7 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
* bits itself.
*/
ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
- ts_ns = ice_ptp_extend_32b_ts(cached_time, ts_high);
+ ts_ns = ice_ptp_extend_32b_ts(pkt_ctx->phc, ts_high);
ice_trace(rx_hwtstamp, cached_time, ts_high, ts_ns);
@@ -3078,24 +3025,20 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
{
struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
- int err;
if (pf->ptp.state != ICE_PTP_READY)
return;
ice_ptp_tspll_monitor(pf);
- err = ice_ptp_update_cached_phctime(pf);
+ if (ice_pf_src_tmr_owned(pf))
+ ice_ptp_update_cached_phctime(pf);
ice_ptp_maybe_trigger_tx_interrupt(pf);
- /* Run twice a second, or retry soon if another thread was busy
- * updating the Rx rings. Other errors, such as a missing control PF,
- * do not clear up on their own and must not turn the periodic worker
- * into a busy loop.
- */
+ /* Run twice a second */
kthread_queue_delayed_work(ptp->kworker, &ptp->work,
- msecs_to_jiffies(err == -EAGAIN ? 10 : 500));
+ msecs_to_jiffies(500));
}
/**
@@ -3190,6 +3133,8 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
/* Acquire PHC and system timer to restore after reset */
ptp->reset_time = ktime_get_real_ns();
+ if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
+ ptp->phc_reset_time = 0;
}
/**
@@ -3229,13 +3174,13 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
if (err)
goto err_unlock;
- /* Write the initial Time value to PHY and LAN using the cached PHC
- * time before the reset and time difference between stopping and
- * starting the clock.
+ /* Write the initial Time value to PHY and LAN using the PHC time
+ * before the reset and time difference between stopping and starting
+ * the clock.
*/
- if (ptp->cached_phc_time) {
+ if (ptp->phc_reset_time) {
time_diff = ktime_get_real_ns() - ptp->reset_time;
- ts = ns_to_timespec64(ptp->cached_phc_time + time_diff);
+ ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
} else {
ts = ktime_to_timespec64(ktime_get_real());
}
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830
2026-10-24 1:24 [PATCH iwl-net 0/2] ice: E825 timer adjustment fixes Jacob Keller
2026-10-24 1:24 ` [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter Jacob Keller
@ 2026-10-24 1:24 ` Jacob Keller
2026-10-24 1:55 ` sashiko-bot
1 sibling, 1 reply; 5+ messages in thread
From: Jacob Keller @ 2026-10-24 1:24 UTC (permalink / raw)
To: Arkadiusz Kubalewski, Sergey Temerkhanov, Przemek Kitszel,
Intel Wired LAN, Przemyslaw Korba, Grzegorz Nitka,
Maciej Machnikowski
Cc: Jacob Keller
The ice driver maintains PTP clock time in the MAC with a 96-bit register.
Timestamps are captured in the PHY which only stores 64 bits, only
including the lower 32-bits of nanoseconds. The driver performs clock
adjustments using a hardware timer command mechanism, where the clock
adjustment is programmed into shadow registers in both the MAC and PHY, and
then a timer command is initiated to synchronously update the timers.
Since the PHY only maintains the lower 32 bits of nanoseconds, adjustments
are also limited such that the maximum atomic adjustment is ~2.1 seconds
(2^31-1 nanoseconds).
For large offsets, the driver uses a non-atomic flow where the current time
is read, an adjustment is applied in software, and then written back. This
necessarily loses some elapsed time, which applications must deal with. For
particular servo configurations, where the parameters are tuned for a very
slow servo, this error can take a long time to finish correction.
Worst, on E825 in specific, the adjustment in the PHY does not always work
reliably. The reason for this is subtle, and is a quirk of how the
timestamps are actually generated. The timestamps for Tx and Rx packets
come from the PCS timer rather than directly from the PHY timer. The PCS
timer is derived from internal PHY signaling.
To keep the PCS timer and PHY timer in sync, every few cycles (testing
seems to indicate every 8 cycles) the PHY timer is copied to the PCS timer.
On other cycles, the PCS timer is incremented by a value determined from
the PCS clock rate.
Crucially, the copy of the PHY timer is skipped if the current value of the
PHY timer is lower than the PCS timer (*and* on this cycle the regular PCS
clock increment is also skipped).
During normal operation, the PCS timer will tick from its own source for 7
ticks then copy the PCS timer. If the PCS timer is slightly ahead, then PHY
timer update is skipped, and the PCS timer skips an update every 8 clock
cycles until it is back in alignment with the PHY timer.
Unfortunately, this hardware logic did not take into account manual
adjustments to the PHY. If the PHY timer is adjusted backwards, (or if a
forward adjustment causes the PHY timer to overflow), the PHY to PCS sync
will not occur again until the PCS timer wraps back around to zero. In the
worst case, an adjustment could land the PHY timer to just below the PCS
timer just after it wraps around. PHY to PCS syncs would stop for ~3.6
seconds.
During this time, the Tx and Rx timestamps will read from the PCS timer.
These timestamps will be fed into the timestamp extension algorithm which
will produce incoherent results, and data will be fed into the PTP
application servo. During the invalid window, faulty timestamps will cause
the servo to steer incorrectly, likely away from the true target time.
After the PCS timer synchronizes again, the timestamps will return to
normal. However, the servo may have steered the clock incorrectly. In
default ptp4l configurations, direct clock adjustment is only performed
once during startup. This forces the application to use frequency slewing
to correct any remaining offset. With slow servo parameters, this can take
a *very* long time. In some setups, we had reports of taking several
minutes to nearly an hour before stabilizing, and in some configurations
the servo was unable to ever recover.
Note that adjustments work properly on the MAC timer, and indeed on the
first-layer PHY timer value that the driver can read over the sideband
queue. Only the PCS timer (which is inaccessible to the driver) fails to
adjust. This made tracking the precise origin of the failure difficult.
To handle this, the most robust solution is to simply not adjust the
hardware clock directly. The kernel already has struct timecounter and
struct cyclecounter for representing hardware counters that cannot handle
64-bits of precision. Indeed, the PHY timer already is only 32 bits, and
ice has used its own custom logic in ice_ptp_extend_32b_ts to extend
timestamps without using a timecounter.
This makes switching to a timecounter fairly straight forward in most
cases. However, such a switch covers many product families and devices. To
limit the initial scope, implement the timecounter only for the newer E825
and E830 devices. Older E810 and E822 platform devices will currently
remain using the legacy approach which has been in use for years. We may
opt to convert all devices in the future, but for now limiting the scope
keeps from potentially regressing legacy deployments.
The timecounter and cyclecounter are embdded within the recently introduced
ice_phc structure, and initialized as part of the ice_adapter. A seqlock is
used to allow lockless reading in the hot path. The ice_phc_cyc2time wraps
the timecounter_cyc2time inside a scoped_seqlock_read() to allow converting
cycles from the hardware into the timecounter domain.
In most places, the timecounter is a drop-in replacement for existing
logic, including using timecounter_init instead of ice_ptp_write_init and
timecounter_adjtime instead of ice_ptp_write_adj.
The ice_ptp_update_ached_phctime will conditionally handle whether to
update the cached_time value, or perform a timecounter_read(). To avoid
contention, this is the only place which actually performs the
timecounter_read(). This is sufficient since the clock owner periodic task
runs twice a second, so this will guarantee to read the timecounter at
least once every overflow.
All other accesses to time values use ice_phc_cyc2time when
phc->use_timecounter is enabled, ensuring that the timecounter phase offset
is handled, including external timestamp events, gettime, cross
timestamping, and periodic output signals.
Periodic output signal programming is the only tricky conversion:
The ice_ptp_cfg_perout() function programs the SDP output signals. For
timecounter, the clk time is adjusted using ice_phc_cyc2time to convert to
the timecounter domain. Then, the timecounter phase is subtracted from the
requested start time. This works because the MAC timer still maintains the
full 96 bits, so even if it overflows, we can still reliably program start
times more than 4 seconds in the future.
Although the particular hardware issue is only known to affect E825, this
timecounter conversion is applied to both E825 and E830 family. These two
products are the most recent and consistency between their behavior is
desired. Older products could benefit from the timecounter (as arbitrarily
large offsets may be programmed atomically). However, at this time it was
deemed safest to initially implement timecounter only for the new devices,
leaving legacy implementation alone. After some time is given to prove out
the implementation and ensure there are no gaps, we may opt to convert all
devices and drop the legacy code entirely.
Fixes: 7cab44f1c35f ("ice: Introduce ETH56G PHY model for E825C products")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
drivers/net/ethernet/intel/ice/ice_adapter.h | 29 +++
drivers/net/ethernet/intel/ice/ice_ptp.h | 6 +
drivers/net/ethernet/intel/ice/ice_adapter.c | 10 ++
drivers/net/ethernet/intel/ice/ice_ptp.c | 260 ++++++++++++++++++++++-----
4 files changed, 259 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
index a56dc5306541..38600ee81168 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.h
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
@@ -8,7 +8,9 @@
#include <linux/mutex.h>
#include <linux/rwsem.h>
#include <linux/srcu.h>
+#include <linux/timecounter.h>
#include <linux/spinlock_types.h>
+#include <linux/seqlock_types.h>
#include <linux/refcount_types.h>
#include "ice_type.h"
@@ -36,12 +38,39 @@ struct ice_port_list {
/**
* struct ice_phc - PTP Hardware Clock fields
+ *
+ * Structure used for handling the timestamp extension logic for this device.
+ * Devices will either use the legacy timestamp extension logic or
+ * a timecounter, not both.
+ *
+ * For simplicity, the timecounter fields are initialized as part of creating
+ * the ice_adapter regardless of whether the device will actually use them.
+ * The PTP clock owner is responsible for setting use_timecounter and
+ * initializing the timecounter to the correct start time.
+ *
+ * Legacy timestamp extension fields:
* @cached_time: Cached copy of the PHC time for timestamp extension
+ *
+ * struct timecounter fields:
+ * @tc: timecounter representing the current time
+ * @cc: cyclecounter underlying the timecounter
+ * @tc_lock: seqlock_t used to protect access to the timecounter
+ *
+ * General fields:
* @update_jiffies: Jiffies when PHC was last updated
+ * @use_timecounter: if true, the device uses the timecounter
*/
struct ice_phc {
+ /* Legacy timestamp extension fields */
u64 cached_time;
+
+ /* struct timecounter fields */
+ struct timecounter tc;
+ struct cyclecounter cc;
+ seqlock_t tc_lock;
+
unsigned long update_jiffies;
+ u8 use_timecounter : 1;
};
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
index 430441b43150..6da9762b2a4c 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
@@ -330,6 +330,7 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf);
bool ice_ptp_tx_tstamps_pending(struct ice_pf *pf, bool in_irq);
int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
struct ptp_system_timestamp *sts, u64 *time);
+u64 ice_ptp_read_cycles(struct cyclecounter *cc);
u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
const struct ice_pkt_ctx *pkt_ctx);
@@ -388,6 +389,11 @@ static inline int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
return -EOPNOTSUPP;
}
+static inline u64 ice_ptp_read_cycles(struct cyclecounter *cc)
+{
+ return 0;
+}
+
static inline u64
ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
const struct ice_pkt_ctx *pkt_ctx)
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
index a702b13e4352..02ba6f4e9d46 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
@@ -6,6 +6,7 @@
#include <linux/pci.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
+#include <linux/seqlock.h>
#include <linux/srcu.h>
#include <linux/xarray.h>
#include "ice_adapter.h"
@@ -81,6 +82,15 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
mutex_init(&adapter->ps_lock);
+ adapter->phc.cc.mask = CYCLECOUNTER_MASK(32);
+ adapter->phc.cc.mult = 1;
+ adapter->phc.cc.shift = 0;
+ adapter->phc.cc.read = ice_ptp_read_cycles;
+
+ timecounter_init(&adapter->phc.tc, &adapter->phc.cc, 0);
+
+ seqlock_init(&adapter->phc.tc_lock);
+
return adapter;
}
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 4b4a00beadd2..488db8298059 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -210,6 +210,38 @@ void ice_ptp_restore_timestamp_mode(struct ice_pf *pf)
ice_flush(hw);
}
+u64 ice_ptp_read_cycles(struct cyclecounter *cc)
+{
+ struct ice_adapter *adapter;
+ struct ice_phc *phc;
+ struct ice_pf *pf;
+ struct ice_hw *hw;
+ u8 tmr_idx;
+
+ phc = container_of(cc, struct ice_phc, cc);
+ adapter = container_of(phc, struct ice_adapter, phc);
+
+ scoped_guard(rcu) {
+ pf = rcu_dereference(adapter->ctrl_pf);
+ if (!pf)
+ return 0;
+
+ /* This came from ctrl_pf so it is always the primary HW */
+ hw = &pf->hw;
+ tmr_idx = ice_get_ptp_src_clock_index(hw);
+
+ /* This does not acquire the adapter->ptp_gltsyn_time_lock.
+ * This is safe because (a) devices which use the cyclecounter
+ * do not directly adjust the clock and (b) we only need to
+ * read GLTSYN_TIME_L, so no tearing is possible anyways.
+ */
+ if (hw->mac_type == ICE_MAC_E830)
+ return rd32(hw, E830_GLTSYN_TIME_L(tmr_idx));
+
+ return rd32(hw, GLTSYN_TIME_L(tmr_idx));
+ }
+}
+
/**
* ice_ptp_read_src_clk_reg - Read the source clock register
* @pf: Board private structure
@@ -272,6 +304,17 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
return 0;
}
+static u64 ice_phc_cyc2time(struct ice_phc *phc, u64 cycles)
+{
+ u64 ns;
+
+ scoped_seqlock_read(&phc->tc_lock, ss_lock_irqsave) {
+ ns = timecounter_cyc2time(&phc->tc, cycles);
+ }
+
+ return ns;
+}
+
/**
* ice_ptp_extend_32b_ts - Convert a 32b nanoseconds timestamp to 64b
* @phc: PTP Hardware Clock structure
@@ -282,6 +325,11 @@ int ice_ptp_read_src_clk_reg(struct ice_pf *pf,
* Note that the captured timestamp values may be 40 bits, but the lower
* 8 bits are sub-nanoseconds and generally discarded.
*
+ * For devices which use the timecounter, call ice_phc_cyc2time to convert the
+ * hardware cycles, accounting for the phase offset of the timecounter.
+ *
+ * For other devices not using the timecounter:
+ *
* Extend the 32bit nanosecond timestamp using the following algorithm and
* assumptions:
*
@@ -315,6 +363,9 @@ static u64 ice_ptp_extend_32b_ts(struct ice_phc *phc, u32 in_tstamp)
u32 delta, phc_time_lo;
u64 ns, cached_time;
+ if (phc->use_timecounter)
+ return ice_phc_cyc2time(phc, in_tstamp);
+
cached_time = READ_ONCE(phc->cached_time);
/* Do not report a timestamp if we don't have a cached PHC time */
if (!cached_time)
@@ -387,7 +438,7 @@ static u64 ice_ptp_extend_40b_ts(struct ice_pf *pf, u64 in_tstamp)
* has become unreachable.
*/
cached_time = READ_ONCE(pf->adapter->phc.cached_time);
- if (!cached_time) {
+ if (!pf->adapter->phc.use_timecounter && !cached_time) {
pf->ptp.tx_hwtstamp_discarded++;
return 0;
}
@@ -995,19 +1046,17 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
* ice_ptp_update_cached_phctime - Update the cached PHC time value
* @pf: Board specific private structure
*
- * Update the PTP hardware clock time values which are cached in the PHC
- * structure.
+ * For devices that use a timecounter structure, update the timecounter.
+ * Ohterwise, update the cached PHC time used for timestamp extension.
*
* This function must be called periodically by the clock owner to ensure that
- * the cached value is never more than 2 seconds old.
+ * the timecounter or cached value is never more than 2 seconds old.
*/
static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
{
struct ice_phc *phc = &pf->adapter->phc;
struct device *dev = ice_pf_to_dev(pf);
unsigned long update_before;
- u64 systime;
- int err;
update_before = phc->update_jiffies + msecs_to_jiffies(2000);
if (phc->cached_time && time_is_before_jiffies(update_before)) {
@@ -1018,21 +1067,33 @@ static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
pf->ptp.late_cached_phc_updates++;
}
- /* Read the current PHC time */
- err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
- if (err) {
- /* The source clock is gone, so the cache can no longer be
- * refreshed. Publish zero instead of leaving the last good
- * value behind: neither ice_ptp_get_rx_hwts() nor
- * ice_ptp_extend_40b_ts() may extend against a value that no
- * longer tracks the PHC, and both treat a zero cache as
- * "no timestamp".
- */
- WRITE_ONCE(phc->cached_time, 0);
+ WRITE_ONCE(phc->update_jiffies, jiffies);
+
+ if (phc->use_timecounter) {
+ unsigned long flags;
+
+ write_seqlock_irqsave(&phc->tc_lock, flags);
+ timecounter_read(&phc->tc);
+ write_sequnlock_irqrestore(&phc->tc_lock, flags);
} else {
- /* Update the cached PHC time stored in the PHC structure */
+ u64 systime;
+ int err;
+
+ err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
+ if (err) {
+ /* The source clock is gone, so the cache can no
+ * longer be refreshed. Publish zero instead of
+ * leaving the last good value behind: neither
+ * ice_ptp_get_rx_hwts() nor ice_ptp_extend_40b_ts()
+ * may extend against a value that no longer tracks
+ * the PHC, and both treat a zero cache as "no
+ * timestamp".
+ */
+ systime = 0;
+ }
+
+ /* Update the cached PHC time stored in the PF structure */
WRITE_ONCE(phc->cached_time, systime);
- WRITE_ONCE(phc->update_jiffies, jiffies);
}
}
@@ -1567,6 +1628,7 @@ static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
*/
void ice_ptp_extts_event(struct ice_pf *pf)
{
+ struct ice_phc *phc = &pf->adapter->phc;
struct ptp_clock_event event;
struct ice_hw *hw = &pf->hw;
u8 chan, tmr_idx;
@@ -1591,7 +1653,11 @@ void ice_ptp_extts_event(struct ice_pf *pf)
lo = rd32(hw, GLTSYN_EVNT_L(chan, tmr_idx));
hi = rd32(hw, GLTSYN_EVNT_H(chan, tmr_idx));
- event.timestamp = (u64)hi << 32 | lo;
+
+ if (phc->use_timecounter)
+ event.timestamp = ice_phc_cyc2time(phc, lo);
+ else
+ event.timestamp = (u64)hi << 32 | lo;
/* Add delay compensation */
pin_desc_idx = ice_ptp_find_pin_idx(pf, PTP_PF_EXTTS, chan);
@@ -1793,8 +1859,9 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan,
static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
int on)
{
+ struct ice_phc *phc = &pf->adapter->phc;
+ u64 clk, period, start, phase, cycles;
unsigned int gpio_pin, prop_delay_ns;
- u64 clk, period, start, phase;
struct ice_hw *hw = &pf->hw;
int pin_desc_idx;
int err;
@@ -1840,12 +1907,21 @@ static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
if (err)
return err;
clk += NSEC_PER_MSEC * 500;
+ if (phc->use_timecounter) {
+ cycles = clk;
+ clk = ice_phc_cyc2time(phc, cycles);
+ }
+
if (rq->flags & PTP_PEROUT_PHASE || start <= clk - prop_delay_ns)
start = div64_u64(clk + period - 1, period) * period + phase;
/* Compensate for propagation delay from the generator to the pin. */
start -= prop_delay_ns;
+ /* Compensate for timecounter phase offset, if necessary */
+ if (phc->use_timecounter)
+ start -= (clk - cycles);
+
return ice_ptp_write_perout(hw, rq->index, gpio_pin, start, period);
}
@@ -1973,7 +2049,7 @@ static int ice_ptp_gpio_enable(struct ptp_clock_info *info,
* @sts: Optional parameter for holding a pair of system timestamps from
* the system clock. Will be ignored if NULL is given.
*
- * Read the device clock and return the correct value on ns, after converting it
+ * Read the device time from the PHC and return it as a timespec.
* into a timespec struct.
*/
static int
@@ -1988,6 +2064,9 @@ ice_ptp_gettimex64(struct ptp_clock_info *info, struct timespec64 *ts,
if (err)
return err;
+ if (pf->adapter->phc.use_timecounter)
+ time_ns = ice_phc_cyc2time(&pf->adapter->phc, time_ns);
+
*ts = ns_to_timespec64(time_ns);
return 0;
}
@@ -2046,6 +2125,33 @@ ice_ptp_settime64(struct ptp_clock_info *info, const struct timespec64 *ts)
return 0;
}
+/**
+ * ice_ptp_settime64_tc - Initialize the PHC timecounter to requested time
+ * @info: the driver's PTP info structure
+ * @ts: timespec64 structure that holds the new time value.
+ *
+ * Reset the PHC timecounter to the provided user input value.
+ */
+static int ice_ptp_settime64_tc(struct ptp_clock_info *info,
+ const struct timespec64 *ts)
+{
+ struct ice_pf *pf = ptp_info_to_pf(info);
+ struct ice_phc *phc = &pf->adapter->phc;
+ unsigned long flags;
+ u64 ns;
+
+ ns = timespec64_to_ns(ts);
+
+ write_seqlock_irqsave(&phc->tc_lock, flags);
+ timecounter_init(&phc->tc, &phc->cc, ns);
+ write_sequnlock_irqrestore(&phc->tc_lock, flags);
+
+ /* Reenable periodic outputs */
+ ice_ptp_enable_all_perout(pf);
+
+ return 0;
+}
+
/**
* ice_ptp_adjtime_nonatomic - Do a non-atomic clock adjustment
* @info: the driver's PTP info structure
@@ -2113,6 +2219,27 @@ static int ice_ptp_adjtime(struct ptp_clock_info *info, s64 delta)
return 0;
}
+/**
+ * ice_ptp_adjtime_tc - Adjust the timecounter offset by the indicated delta
+ * @info: the driver's PTP info structure
+ * @delta: Offset in nanoseconds to adjust the time by
+ */
+static int ice_ptp_adjtime_tc(struct ptp_clock_info *info, s64 delta)
+{
+ struct ice_pf *pf = ptp_info_to_pf(info);
+ struct ice_phc *phc = &pf->adapter->phc;
+ unsigned long flags;
+
+ write_seqlock_irqsave(&phc->tc_lock, flags);
+ timecounter_adjtime(&phc->tc, delta);
+ write_sequnlock_irqrestore(&phc->tc_lock, flags);
+
+ /* Reenable periodic outputs */
+ ice_ptp_enable_all_perout(pf);
+
+ return 0;
+}
+
/**
* struct ice_crosststamp_cfg - Device cross timestamp configuration
* @lock_reg: The hardware semaphore lock to use
@@ -2248,6 +2375,11 @@ static int ice_capture_crosststamp(ktime_t *device,
ts_lo = rd32(hw, cfg->dev_time_l[tmr_idx]);
ts_hi = rd32(hw, cfg->dev_time_h[tmr_idx]);
ts = ((u64)ts_hi << 32) | ts_lo;
+
+ /* Account for timecounter phase offset if necessary */
+ if (pf->adapter->phc.use_timecounter)
+ ts = ice_phc_cyc2time(&pf->adapter->phc, ts);
+
*device = ns_to_ktime(ts);
err_timeout:
@@ -2414,15 +2546,9 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
return 0;
cached_time = READ_ONCE(pkt_ctx->phc->cached_time);
- if (!cached_time)
+ if (!pkt_ctx->phc->use_timecounter && !cached_time)
return 0;
- /* Use ice_ptp_extend_32b_ts directly, using the ring-specific cached
- * PHC value, rather than accessing the PF. This also allows us to
- * simply pass the upper 32bits of nanoseconds directly. Calling
- * ice_ptp_extend_40b_ts is unnecessary as it would just discard these
- * bits itself.
- */
ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
ts_ns = ice_ptp_extend_32b_ts(pkt_ctx->phc, ts_high);
@@ -2647,15 +2773,21 @@ static void ice_ptp_set_caps(struct ice_pf *pf)
dev_driver_string(dev), dev_name(dev));
info->owner = THIS_MODULE;
info->max_adj = 100000000;
- info->adjtime = ice_ptp_adjtime;
info->adjfine = ice_ptp_adjfine;
info->gettimex64 = ice_ptp_gettimex64;
- info->settime64 = ice_ptp_settime64;
info->n_per_out = GLTSYN_TGT_H_IDX_MAX;
info->n_ext_ts = GLTSYN_EVNT_H_IDX_MAX;
info->enable = ice_ptp_gpio_enable;
info->verify = ice_verify_pin;
+ if (pf->adapter->phc.use_timecounter) {
+ info->adjtime = ice_ptp_adjtime_tc;
+ info->settime64 = ice_ptp_settime64_tc;
+ } else {
+ info->adjtime = ice_ptp_adjtime;
+ info->settime64 = ice_ptp_settime64;
+ }
+
info->supported_extts_flags = PTP_RISING_EDGE |
PTP_FALLING_EDGE |
PTP_STRICT_FLAGS;
@@ -3103,6 +3235,7 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
struct ice_ptp *ptp = &pf->ptp;
struct ice_hw *hw = &pf->hw;
u8 src_tmr;
+ u64 ns;
if (ptp->state != ICE_PTP_READY)
return;
@@ -3133,8 +3266,11 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
/* Acquire PHC and system timer to restore after reset */
ptp->reset_time = ktime_get_real_ns();
- if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
- ptp->phc_reset_time = 0;
+ if (ice_ptp_read_src_clk_reg(pf, NULL, &ns))
+ ns = 0;
+ else if (pf->adapter->phc.use_timecounter)
+ ns = ice_phc_cyc2time(&pf->adapter->phc, ns);
+ ptp->phc_reset_time = ns;
}
/**
@@ -3146,10 +3282,10 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
*/
static int ice_ptp_rebuild_owner(struct ice_pf *pf)
{
+ struct ice_phc *phc = &pf->adapter->phc;
struct ice_ptp *ptp = &pf->ptp;
struct ice_hw *hw = &pf->hw;
- struct timespec64 ts;
- u64 time_diff;
+ u64 time_diff, ns;
int err;
err = ice_ptp_init_phc(hw);
@@ -3180,13 +3316,24 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
*/
if (ptp->phc_reset_time) {
time_diff = ktime_get_real_ns() - ptp->reset_time;
- ts = ns_to_timespec64(ptp->phc_reset_time + time_diff);
+ ns = ptp->phc_reset_time + time_diff;
} else {
- ts = ktime_to_timespec64(ktime_get_real());
+ ns = ktime_get_real_ns();
+ }
+
+ if (phc->use_timecounter) {
+ unsigned long flags;
+
+ write_seqlock_irqsave(&phc->tc_lock, flags);
+ timecounter_init(&phc->tc, &phc->cc, ns);
+ write_sequnlock_irqrestore(&phc->tc_lock, flags);
+ } else {
+ struct timespec64 ts = ns_to_timespec64(ns);
+
+ err = ice_ptp_write_init(pf, &ts);
+ if (err)
+ goto err_unlock;
}
- err = ice_ptp_write_init(pf, &ts);
- if (err)
- goto err_unlock;
/* Release the global hardware lock */
ice_ptp_unlock(hw);
@@ -3375,10 +3522,21 @@ int ice_ptp_clock_index(struct ice_pf *pf)
*/
static int ice_ptp_init_owner(struct ice_pf *pf)
{
+ struct ice_phc *phc = &pf->adapter->phc;
struct ice_hw *hw = &pf->hw;
- struct timespec64 ts;
int err;
+ switch (hw->mac_type) {
+ case ICE_MAC_GENERIC_3K_E825:
+ case ICE_MAC_E830:
+ /* Use struct timecounter for E825 and E830 devices */
+ dev_dbg(ice_pf_to_dev(pf), "PTP 1588 enabling timecounter mode\n");
+ phc->use_timecounter = 1;
+ break;
+ default:
+ break;
+ }
+
err = ice_ptp_init_phc(hw);
if (err) {
dev_err(ice_pf_to_dev(pf), "Failed to initialize PHC, err %d\n",
@@ -3404,11 +3562,21 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
if (err)
goto err_unlock;
- ts = ktime_to_timespec64(ktime_get_real());
- /* Write the initial Time value to PHY and LAN */
- err = ice_ptp_write_init(pf, &ts);
- if (err)
- goto err_unlock;
+ if (phc->use_timecounter) {
+ unsigned long flags;
+
+ write_seqlock_irqsave(&phc->tc_lock, flags);
+ timecounter_init(&phc->tc, &phc->cc, ktime_get_real_ns());
+ write_sequnlock_irqrestore(&phc->tc_lock, flags);
+ } else {
+ struct timespec64 ts;
+
+ /* Write the initial Time value to PHY and LAN */
+ ktime_get_real_ts64(&ts);
+ err = ice_ptp_write_init(pf, &ts);
+ if (err)
+ goto err_unlock;
+ }
/* Release the global hardware lock */
ice_ptp_unlock(hw);
--
2.56.0.215.g47fdf2208c5b
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter
2026-10-24 1:24 ` [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter Jacob Keller
@ 2026-10-24 1:46 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-24 1:46 UTC (permalink / raw)
To: Jacob Keller, Intel Wired LAN
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ice: stale shared phc.cached_time when clock-owning PF stops or unbinds
--
commit 866055ac7173935e690af3f73268e559a0698a8d
Author: Jacob Keller <jacob.e.keller@intel.com>
ice: store cached PHC time in ice_adapter
This commit moves the cached PHC time from per-PF and per-Rx-ring fields
into a shared struct ice_phc embedded in struct ice_adapter, and updates
only the clock-owning PF to refresh that shared value in its periodic
work.
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index ec24c09280785..4b4a00beadd2c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
[ ... ]
> @@ -987,35 +992,26 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
> }
>
> /**
> - * ice_ptp_update_cached_phctime - Update the cached PHC time values
> + * ice_ptp_update_cached_phctime - Update the cached PHC time value
> * @pf: Board specific private structure
> *
> - * This function updates the system time values which are cached in the PF
> - * structure and the Rx rings.
> - *
> - * This function must be called periodically to ensure that the cached value
> - * is never more than 2 seconds old.
> + * Update the PTP hardware clock time values which are cached in the PHC
> + * structure.
> *
> - * Note that the cached copy in the PF PTP structure is always updated, even
> - * if we can't update the copy in the Rx rings.
> - *
> - * Return:
> - * * 0 - OK, successfully updated
> - * * -EAGAIN - PF was busy, need to reschedule the update
> - * * -ENODEV - no control PF owns the source clock register space
> - * * -EOPNOTSUPP - PTP support is not compiled in
> + * This function must be called periodically by the clock owner to ensure that
> + * the cached value is never more than 2 seconds old.
> */
> -static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
> +static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
> {
> + struct ice_phc *phc = &pf->adapter->phc;
> struct device *dev = ice_pf_to_dev(pf);
> unsigned long update_before;
> u64 systime;
> - int err, i;
> + int err;
>
> - update_before = pf->ptp.cached_phc_jiffies + msecs_to_jiffies(2000);
> - if (pf->ptp.cached_phc_time &&
> - time_is_before_jiffies(update_before)) {
> - unsigned long time_taken = jiffies - pf->ptp.cached_phc_jiffies;
> + update_before = phc->update_jiffies + msecs_to_jiffies(2000);
> + if (phc->cached_time && time_is_before_jiffies(update_before)) {
> + unsigned long time_taken = jiffies - phc->update_jiffies;
>
> dev_warn(dev, "%u msecs passed between update to cached PHC time\n",
> jiffies_to_msecs(time_taken));
> @@ -1032,36 +1028,12 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf)
> * longer tracks the PHC, and both treat a zero cache as
> * "no timestamp".
> */
> - systime = 0;
> - WRITE_ONCE(pf->ptp.cached_phc_time, 0);
> + WRITE_ONCE(phc->cached_time, 0);
> } else {
> - /* Update the cached PHC time stored in the PF structure */
> - WRITE_ONCE(pf->ptp.cached_phc_time, systime);
> - WRITE_ONCE(pf->ptp.cached_phc_jiffies, jiffies);
> + /* Update the cached PHC time stored in the PHC structure */
> + WRITE_ONCE(phc->cached_time, systime);
> + WRITE_ONCE(phc->update_jiffies, jiffies);
> }
[ ... ]
> @@ -3078,24 +3025,20 @@ static void ice_ptp_periodic_work(struct kthread_work *work)
> {
> struct ice_ptp *ptp = container_of(work, struct ice_ptp, work.work);
> struct ice_pf *pf = container_of(ptp, struct ice_pf, ptp);
> - int err;
>
> if (pf->ptp.state != ICE_PTP_READY)
> return;
>
> ice_ptp_tspll_monitor(pf);
>
> - err = ice_ptp_update_cached_phctime(pf);
> + if (ice_pf_src_tmr_owned(pf))
> + ice_ptp_update_cached_phctime(pf);
[Severity: Medium]
What happens to pf->adapter->phc.cached_time on remaining PFs if the
clock-owning PF is unbound, reset, or fails PTP rebuild?
Previously, commit dcc2c9108e65 ("ice: Reject PTP access without a control
PF") handled control PF removal by having each PF run
ice_ptp_update_cached_phctime() from ice_ptp_periodic_work(). When the
control PF went away, ice_ptp_read_src_clk_reg() returned -ENODEV on the
secondary PFs, which cleared cached_phc_time and the Rx ring cached_phctime
copies to 0.
With this change, ice_ptp_periodic_work() in
drivers/net/ethernet/intel/ice/ice_ptp.c only calls
ice_ptp_update_cached_phctime() when ice_pf_src_tmr_owned(pf) is true
(and the only other callers are ice_ptp_settime64() and ice_ptp_adjtime()
via ice_ptp_reset_cached_phctime(), which also only run on the clock
owner). Because ice_pf_src_tmr_owned(pf) guarantees ice_is_primary(&pf->hw)
is true, ice_ptp_read_src_clk_reg() never returns -ENODEV here, making the
error branch in ice_ptp_update_cached_phctime() that zeroes
phc->cached_time unreachable.
Meanwhile, when the clock-owning PF stops its periodic worker in
ice_ptp_prepare_for_reset() or is unbound via
ice_ptp_release() -> ice_ptp_cleanup_adapter():
drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_cleanup_adapter() {
...
if (ice_get_ctrl_pf(pf) == pf) {
rcu_assign_pointer(pf->adapter->ctrl_pf, NULL);
synchronize = true;
}
...
}
nothing zeroes pf->adapter->phc.cached_time.
Because the Rx path in ice_ptp_get_rx_hwts() -> ice_ptp_extend_32b_ts()
only checks whether phc->cached_time is non-zero and does not check
phc->update_jiffies, won't active sibling or secondary PFs continue
extending 32-bit hardware Rx timestamps against the frozen
phc->cached_time indefinitely?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-jk-e825-e830-timecounter-v1-0-e932602f0d16@intel.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830
2026-10-24 1:24 ` [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830 Jacob Keller
@ 2026-10-24 1:55 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-24 1:55 UTC (permalink / raw)
To: Jacob Keller, Intel Wired LAN
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] ice: 500ms offset before ice_phc_cyc2time() in ice_ptp_cfg_perout() wraps
- [High] ice: uncompensated timecounter offset for E830 TxTime in ice_tx_map()
- [Medium] ice: late PHC update check broken in ice_ptp_update_cached_phctime()
- [Medium] ice: missing timecounter conversion in ice_vc_get_phc_time()
- [Medium] ice: missing timecounter staleness and validity handling in ice_phc
- [Medium] ice: ice_ptp_prepare_for_reset() converts zero GLTSYN_TIME via timecounter
--
commit 348b48a49347fa2790d183338857035e4d8f1fa4
Author: Jacob Keller <jacob.e.keller@intel.com>
ice: use struct timecounter for E825 and E830
This commit switches E825 and E830 PTP hardware clock management from direct
hardware timer adjustments to a software struct timecounter and cyclecounter
embedded in struct ice_phc. This avoids PCS timer synchronization stalls on
E825 caused by manual PHY timer adjustments and converts timestamp extension,
clock reads, cross-timestamping, and periodic output configuration to account
for the timecounter phase offset.
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 4b4a00beadd2c..488db8298059a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -210,6 +210,38 @@ void ice_ptp_restore_timestamp_mode(struct ice_pf *pf)
> ice_flush(hw);
> }
>
> +u64 ice_ptp_read_cycles(struct cyclecounter *cc)
> +{
> + struct ice_adapter *adapter;
> + struct ice_phc *phc;
> + struct ice_pf *pf;
> + struct ice_hw *hw;
> + u8 tmr_idx;
> +
> + phc = container_of(cc, struct ice_phc, cc);
> + adapter = container_of(phc, struct ice_adapter, phc);
> +
> + scoped_guard(rcu) {
> + pf = rcu_dereference(adapter->ctrl_pf);
> + if (!pf)
> + return 0;
[ ... ]
> @@ -995,19 +1046,17 @@ static int ice_ptp_init_tx(struct ice_pf *pf, struct ice_ptp_tx *tx, u8 port)
> * ice_ptp_update_cached_phctime - Update the cached PHC time value
> * @pf: Board specific private structure
> *
> - * Update the PTP hardware clock time values which are cached in the PHC
> - * structure.
> + * For devices that use a timecounter structure, update the timecounter.
> + * Ohterwise, update the cached PHC time used for timestamp extension.
> *
> * This function must be called periodically by the clock owner to ensure that
> - * the cached value is never more than 2 seconds old.
> + * the timecounter or cached value is never more than 2 seconds old.
> */
> static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
> {
> struct ice_phc *phc = &pf->adapter->phc;
> struct device *dev = ice_pf_to_dev(pf);
> unsigned long update_before;
> - u64 systime;
> - int err;
>
> update_before = phc->update_jiffies + msecs_to_jiffies(2000);
> if (phc->cached_time && time_is_before_jiffies(update_before)) {
> @@ -1018,21 +1067,33 @@ static void ice_ptp_update_cached_phctime(struct ice_pf *pf)
> pf->ptp.late_cached_phc_updates++;
> }
>
> - /* Read the current PHC time */
> - err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
> - if (err) {
> - /* The source clock is gone, so the cache can no longer be
> - * refreshed. Publish zero instead of leaving the last good
> - * value behind: neither ice_ptp_get_rx_hwts() nor
> - * ice_ptp_extend_40b_ts() may extend against a value that no
> - * longer tracks the PHC, and both treat a zero cache as
> - * "no timestamp".
> - */
> - WRITE_ONCE(phc->cached_time, 0);
> + WRITE_ONCE(phc->update_jiffies, jiffies);
> +
> + if (phc->use_timecounter) {
> + unsigned long flags;
> +
> + write_seqlock_irqsave(&phc->tc_lock, flags);
> + timecounter_read(&phc->tc);
> + write_sequnlock_irqrestore(&phc->tc_lock, flags);
> } else {
> - /* Update the cached PHC time stored in the PHC structure */
> + u64 systime;
> + int err;
> +
> + err = ice_ptp_read_src_clk_reg(pf, NULL, &systime);
> + if (err) {
> + /* The source clock is gone, so the cache can no
> + * longer be refreshed. Publish zero instead of
> + * leaving the last good value behind: neither
> + * ice_ptp_get_rx_hwts() nor ice_ptp_extend_40b_ts()
> + * may extend against a value that no longer tracks
> + * the PHC, and both treat a zero cache as "no
> + * timestamp".
> + */
> + systime = 0;
> + }
> +
> + /* Update the cached PHC time stored in the PF structure */
> WRITE_ONCE(phc->cached_time, systime);
> - WRITE_ONCE(phc->update_jiffies, jiffies);
> }
> }
[Severity: Medium]
In ice_ptp_update_cached_phctime(), the check for late updates still tests
phc->cached_time:
update_before = phc->update_jiffies + msecs_to_jiffies(2000);
if (phc->cached_time && time_is_before_jiffies(update_before)) {
unsigned long time_taken = jiffies - phc->update_jiffies;
dev_warn(dev, "%u msecs passed between update to cached PHC time\n",
jiffies_to_msecs(time_taken));
pf->ptp.late_cached_phc_updates++;
}
When phc->use_timecounter is true on E825 and E830 devices, phc->cached_time
is initialized to 0 in ice_adapter_new() and is never written. Won't this
cause the condition to always evaluate to false in timecounter mode,
preventing late timecounter updates from ever emitting the warning or
incrementing pf->ptp.late_cached_phc_updates?
Also, by moving WRITE_ONCE(phc->update_jiffies, jiffies) above the
if (phc->use_timecounter) branch, phc->update_jiffies is now updated on
legacy devices even when ice_ptp_read_src_clk_reg() fails and systime is set
to 0. Should phc->update_jiffies only be updated when the clock read
succeeds, and should the late-update check also cover timecounter mode?
[ ... ]
> @@ -1793,8 +1859,9 @@ static int ice_ptp_write_perout(struct ice_hw *hw, unsigned int chan,
> static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
> int on)
> {
> + struct ice_phc *phc = &pf->adapter->phc;
> + u64 clk, period, start, phase, cycles;
> unsigned int gpio_pin, prop_delay_ns;
> - u64 clk, period, start, phase;
> struct ice_hw *hw = &pf->hw;
> int pin_desc_idx;
> int err;
> @@ -1840,12 +1907,21 @@ static int ice_ptp_cfg_perout(struct ice_pf *pf, struct ptp_perout_request *rq,
> if (err)
> return err;
> clk += NSEC_PER_MSEC * 500;
> + if (phc->use_timecounter) {
> + cycles = clk;
> + clk = ice_phc_cyc2time(phc, cycles);
> + }
> +
> if (rq->flags & PTP_PEROUT_PHASE || start <= clk - prop_delay_ns)
> start = div64_u64(clk + period - 1, period) * period + phase;
>
> /* Compensate for propagation delay from the generator to the pin. */
> start -= prop_delay_ns;
>
> + /* Compensate for timecounter phase offset, if necessary */
> + if (phc->use_timecounter)
> + start -= (clk - cycles);
> +
> return ice_ptp_write_perout(hw, rq->index, gpio_pin, start, period);
> }
[Severity: High]
In ice_ptp_cfg_perout(), 500 ms (NSEC_PER_MSEC * 500) is added to the raw
hardware clock value before passing cycles to ice_phc_cyc2time():
err = ice_ptp_read_src_clk_reg(pf, NULL, &clk);
if (err)
return err;
clk += NSEC_PER_MSEC * 500;
if (phc->use_timecounter) {
cycles = clk;
clk = ice_phc_cyc2time(phc, cycles);
}
Since phc->cc uses CYCLECOUNTER_MASK(32), timecounter_cyc2time() treats any
cycle value where (cycles - tc->cycle_last) & 0xffffffff exceeds
cc->mask / 2 (~2.147 seconds) as having occurred in the past, subtracting the
delta from tc->nsec instead of adding it.
Because cycles is already 0.5 seconds ahead of the current hardware counter,
doesn't this shrink the forward window before wraparound from ~2.147 seconds
to ~1.647 seconds? If tc->cycle_last was updated between 1.648 and 2.0
seconds ago (within the normal periodic update window, or during
ice_ptp_adjtime_tc() -> ice_ptp_enable_all_perout() where tc->cycle_last is
not refreshed), ice_phc_cyc2time() will wrap backwards by 2^32 ns, making
(clk - cycles) and the resulting GLTSYN_TGT_L/H start time off by 2^32 ns
(and shifting the periodic output phase by 2^32 mod period).
Should ice_phc_cyc2time() be called on the raw clk value before adding
NSEC_PER_MSEC * 500?
[ ... ]
> @@ -2414,15 +2546,9 @@ u64 ice_ptp_get_rx_hwts(const union ice_32b_rx_flex_desc *rx_desc,
> return 0;
>
> cached_time = READ_ONCE(pkt_ctx->phc->cached_time);
> - if (!cached_time)
> + if (!pkt_ctx->phc->use_timecounter && !cached_time)
> return 0;
>
> - /* Use ice_ptp_extend_32b_ts directly, using the ring-specific cached
> - * PHC value, rather than accessing the PF. This also allows us to
> - * simply pass the upper 32bits of nanoseconds directly. Calling
> - * ice_ptp_extend_40b_ts is unnecessary as it would just discard these
> - * bits itself.
> - */
> ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
> ts_ns = ice_ptp_extend_32b_ts(pkt_ctx->phc, ts_high);
[Severity: Medium]
When pkt_ctx->phc->use_timecounter is true, ice_ptp_get_rx_hwts() bypasses
the cached_time check and does not check phc->update_jiffies (unlike
ice_ptp_extend_40b_ts()) or whether the clock owner is active.
Because timecounter_cyc2time() with a 32-bit cyclecounter is only valid
within +/- 2.14 seconds of tc->cycle_last, what happens if the periodic work
is delayed past 2 seconds, or if the clock owner PF on a multi-PF adapter is
unbound in ice_ptp_release(), fails ice_ptp_rebuild(), or fails
ice_ptp_init_owner() after setting phc->use_timecounter = 1? Won't sibling
PFs continue extending Rx timestamps against a frozen or uninitialized
phc->tc instead of returning 0?
Relatedly, in ice_ptp_release(), ice_ptp_cleanup_adapter() clears
adapter->ctrl_pf before cancelling pf->ptp.work and unregistering the PTP
clock, which allows ice_ptp_read_cycles() to observe a NULL ctrl_pf and
return 0 as a valid cycle count into timecounter_read() or
timecounter_init(). Also, ice_ptp_init_owner(), ice_ptp_rebuild_owner(), and
ice_ptp_settime64_tc() call timecounter_init() without refreshing
phc->update_jiffies, which can leave phc->update_jiffies stale across a reset
until the periodic work runs and cause ice_ptp_extend_40b_ts() to discard
valid Tx timestamps right after ice_ptp_restore_timestamp_mode().
[ ... ]
> @@ -3133,8 +3266,11 @@ void ice_ptp_prepare_for_reset(struct ice_pf *pf, enum ice_reset_req reset_type)
>
> /* Acquire PHC and system timer to restore after reset */
> ptp->reset_time = ktime_get_real_ns();
> - if (ice_ptp_read_src_clk_reg(pf, NULL, &ptp->phc_reset_time))
> - ptp->phc_reset_time = 0;
> + if (ice_ptp_read_src_clk_reg(pf, NULL, &ns))
> + ns = 0;
> + else if (pf->adapter->phc.use_timecounter)
> + ns = ice_phc_cyc2time(&pf->adapter->phc, ns);
> + ptp->phc_reset_time = ns;
> }
[Severity: Medium]
When a hardware reset (such as CORER, GLOBR, or EMPR) has already cleared the
GLTSYN_TIME registers to 0 before ice_ptp_prepare_for_reset() runs,
ice_ptp_read_src_clk_reg() on the clock owner PF returns 0 with ns = 0.
Previously, ns = 0 resulted in ptp->phc_reset_time = 0, causing
ice_ptp_rebuild_owner() to fall back to ktime_get_real_ns():
if (ptp->phc_reset_time) {
time_diff = ktime_get_real_ns() - ptp->reset_time;
ns = ptp->phc_reset_time + time_diff;
} else {
ns = ktime_get_real_ns();
}
With phc.use_timecounter enabled, the else-if branch passes ns = 0 to
ice_phc_cyc2time(&pf->adapter->phc, 0), which computes a 32-bit delta between
cycle 0 and the pre-reset tc->cycle_last. Doesn't this turn a zeroed hardware
register into a non-zero ptp->phc_reset_time shifted by up to +/- 2.147
seconds, bypassing the ktime_get_real_ns() fallback in
ice_ptp_rebuild_owner()?
[ ... ]
> @@ -3375,10 +3522,21 @@ int ice_ptp_clock_index(struct ice_pf *pf)
> */
> static int ice_ptp_init_owner(struct ice_pf *pf)
> {
> + struct ice_phc *phc = &pf->adapter->phc;
> struct ice_hw *hw = &pf->hw;
> - struct timespec64 ts;
> int err;
>
> + switch (hw->mac_type) {
> + case ICE_MAC_GENERIC_3K_E825:
> + case ICE_MAC_E830:
> + /* Use struct timecounter for E825 and E830 devices */
> + dev_dbg(ice_pf_to_dev(pf), "PTP 1588 enabling timecounter mode\n");
> + phc->use_timecounter = 1;
> + break;
> + default:
> + break;
> + }
[Severity: High]
Enabling phc->use_timecounter on ICE_MAC_E830 means the hardware
E830_GLTSYN_TIME registers are no longer initialized in ice_ptp_init_owner()
or ice_ptp_rebuild_owner(), nor adjusted in ice_ptp_settime64_tc() and
ice_ptp_adjtime_tc().
However, E830 supports ICE_F_TXTIME hardware offload:
drivers/net/ethernet/intel/ice/ice_lib.c:ice_init_feature_support() {
if (pf->hw.mac_type == ICE_MAC_E830) {
ice_set_feature_support(pf, ICE_F_MBX_LIMIT);
ice_set_feature_support(pf, ICE_F_GCS);
ice_set_feature_support(pf, ICE_F_TXTIME);
}
}
And in ice_tx_map(), the TxTime descriptor timestamp is populated directly
from first->skb->tstamp without compensating for the timecounter phase
offset:
drivers/net/ethernet/intel/ice/ice_txrx.c:ice_tx_map() {
ts = ktime_to_timespec64(first->skb->tstamp);
tstamp = ts.tv_nsec >> ICE_TXTIME_CTX_RESOLUTION_128NS;
ts_desc = ICE_TS_DESC(tstamp_ring, j);
ts_desc->tx_desc_idx_tstamp = ice_build_tstamp_desc(i, tstamp);
}
How does ICE_F_TXTIME work on E830 when the PHC time is maintained in the
software timecounter while the hardware TxTime queue compares descriptor
timestamps against the unadjusted hardware clock?
[Severity: Medium]
Similarly, when an SR-IOV VF requests the PHC time via
VIRTCHNL_OP_1588_PTP_GET_TIME, ice_vc_get_phc_time() in
drivers/net/ethernet/intel/ice/virt/virtchnl.c still reads the raw hardware
register without converting through ice_phc_cyc2time():
drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_phc_time() {
if (ice_ptp_read_src_clk_reg(pf, NULL, &phc_time->time)) {
v_ret = VIRTCHNL_STATUS_ERR_NOT_SUPPORTED;
len = 0;
}
}
Furthermore, in the VF driver, iavf_ptp_extend_32b_timestamp() in
drivers/net/ethernet/intel/iavf/iavf_ptp.c computes:
u32 low = lower_32_bits(cached_phc_time);
u32 delta = in_tstamp - low;
which assumes the lower 32 bits of the PHC time returned by the PF match the
raw 32-bit hardware timestamp domain in the Rx descriptor. Won't enabling
phc->use_timecounter on E825 and E830 break both VF PTP clock reads and VF Rx
hardware timestamp extension?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-jk-e825-e830-timecounter-v1-0-e932602f0d16@intel.com?part=2
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-10-24 1:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-24 1:24 [PATCH iwl-net 0/2] ice: E825 timer adjustment fixes Jacob Keller
2026-10-24 1:24 ` [PATCH iwl-net 1/2] ice: store cached PHC time in ice_adapter Jacob Keller
2026-10-24 1:46 ` sashiko-bot
2026-10-24 1:24 ` [PATCH iwl-net 2/2] ice: use struct timecounter for E825 and E830 Jacob Keller
2026-10-24 1:55 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox