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 09:05:58AM +0000, Bruce Richardson wrote:
> 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>
>  
Applied to dpdk-next-net-intel.

Thanks,
/Bruce