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

["Song, Yoong Siang" <yoong.siang.song@intel.com>]

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.

>
>Instead, you should probably do one of:
>* read nsec, then sec, then nsec again and verify that nsec2 > nsec1
>* read sec, nsec, then sec, and verify that sec2 == sec1
>
>in both casees retrying the reads if the condition fails.
>
>>  	ts->tv_nsec = E1000_READ_REG(hw, E1000_SYSTIML);
>>  	ts->tv_sec = E1000_READ_REG(hw, E1000_SYSTIMH);
>>
>> @@ -2972,10 +2978,10 @@ eth_igc_timesync_disable(struct rte_eth_dev *dev)
>>  static int
>>  eth_igc_read_clock(__rte_unused struct rte_eth_dev *dev, uint64_t *clock)
>>  {
>> -	struct timespec system_time;
>> +	struct timespec ts;
>>
>> -	clock_gettime(CLOCK_REALTIME, &system_time);
>> -	*clock = system_time.tv_sec * NSEC_PER_SEC + system_time.tv_nsec;
>> +	eth_igc_timesync_read_time(dev, &ts);
>> +	*clock = rte_timespec_to_ns(&ts);
>>
>>  	return 0;
>>  }
>> --
>> 2.48.1
>>