Re: [PATCH v2 1/1] net/e1000: use device timestamp for igc read_clock() operation

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Dec 11, 2025 at 03:24:42AM +0000, Song, Yoong Siang wrote:
> On Thursday, December 11, 2025 1:13 AM, Richardson, Bruce <bruce.richardson@intel.com> wrote:
> >On Sat, Nov 08, 2025 at 04:06:13PM +0800, Song Yoong Siang wrote:
> >> Change eth_igc_read_clock() to read from hardware timestamp registers
> >> (E1000_SYSTIML/E1000_SYSTIMH) instead of using system clock_gettime().
> >>
> >> This ensures that the clock reading is consistent with the hardware's
> >> internal time base used for Qbv cycle and launch time scheduling,
> >> providing better accuracy for Time-Sensitive Networking applications.
> >>
> >> Fixes: 9630f7c71ecd ("net/igc: enable launch time offloading")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: David Zage <david.zage@intel.com>
> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@intel.com>
> >> ---
> >> v1: https://patches.dpdk.org/project/dpdk/patch/20251107031507.3890366-1-
> >yoong.siang.song@intel.com/
> >>
> >> changelog:
> >> v1 -> v2
> >> - reuse the existing eth_igc_timesync_read_time() (Soumyadeep).
> >> ---
> >>  drivers/net/intel/e1000/igc_ethdev.c | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/intel/e1000/igc_ethdev.c
> >b/drivers/net/intel/e1000/igc_ethdev.c
> >> index b9c91d2446..d4edc82668 100644
> >> --- a/drivers/net/intel/e1000/igc_ethdev.c
> >> +++ b/drivers/net/intel/e1000/igc_ethdev.c
> >> @@ -2813,6 +2813,12 @@ eth_igc_timesync_read_time(struct rte_eth_dev *dev,
> >struct timespec *ts)
> >>  {
> >>  	struct e1000_hw *hw = IGC_DEV_PRIVATE_HW(dev);
> >>
> >> +	/*
> >> +	 * Reading the SYSTIML register latches the upper 32 bits to the SYSTIMH
> >> +	 * shadow register for coherent access. As long as we read SYSTIML first
> >> +	 * followed by SYSTIMH, we avoid race conditions where the time rolls
> >> +	 * over between the two register reads.
> >> +	 */
> >
> >Not sure this is true. If the nsec value == 999,999,999 on read, then the
> >rollover occurs, the resulting timestamp will be almost 1 second out,
> >right?
> 
> Thanks for your comment.
> 
> According to Section 7.5.1.3.1 of the Intel(r) Ethernet Controller I225/I226
> Software User Manual (Revision 1.4.5), the hardware provides atomic access
> to the timestamp through a latching mechanism: When SYSTIML is read, the
> hardware automatically latches the upper 32 bits into a SYSTIMH shadow
> register. This ensures that subsequent reads of SYSTIMH return the value
> that was present at the time SYSTIML was read, providing coherent access to
> the full timestamp. Therefore, the SYSTIML-first, SYSTIMH-second read
> sequence is guaranteed by hardware to be atomic and eliminates the rollover
> race condition you mentioned.
>

Great, thanks for clarifying!

Acked-by: Bruce Richardson <bruce.richardson@intel.com>