[RFC 0/4] add frequency adjustment support for PTP

[RFC 0/4] add frequency adjustment support for PTP

[Simei Su]

[RFC 1/4] ethdev: add frequency adjustment API.
[RFC 2/4] net/ice: add frequency adjustment support for PTP.
[RFC 3/4] examples/ptpclient: refine application.
[RFC 4/4] examples/ptpclient: add frequency adjustment support.

Simei Su (4):
  ethdev: add frequency adjustment API
  net/ice: add frequency adjustment support for PTP
  examples/ptpclient: refine application
  examples/ptpclient: add frequency adjustment support

 drivers/net/ice/ice_ethdev.c     | 111 +++++++++++++-------
 examples/ptpclient/ptpclient.c   | 222 +++++++++++++++++++++++++++++++++------
 lib/ethdev/ethdev_driver.h       |   5 +
 lib/ethdev/ethdev_trace.h        |   9 ++
 lib/ethdev/ethdev_trace_points.c |   3 +
 lib/ethdev/rte_ethdev.c          |  18 ++++
 lib/ethdev/rte_ethdev.h          |  19 ++++
 7 files changed, 317 insertions(+), 70 deletions(-)

-- 
2.9.5

[RFC 1/4] ethdev: add frequency adjustment API

[Simei Su]

This patch adds freq adjustment API for PTP high accuracy.

Signed-off-by: Simei Su <simei.su@intel.com>
---
 lib/ethdev/ethdev_driver.h       |  5 +++++
 lib/ethdev/ethdev_trace.h        |  9 +++++++++
 lib/ethdev/ethdev_trace_points.c |  3 +++
 lib/ethdev/rte_ethdev.c          | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.h          | 19 +++++++++++++++++++
 5 files changed, 54 insertions(+)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 2c9d615..b1451d2 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -633,6 +633,9 @@ typedef int (*eth_timesync_read_tx_timestamp_t)(struct rte_eth_dev *dev,
 /** @internal Function used to adjust the device clock. */
 typedef int (*eth_timesync_adjust_time)(struct rte_eth_dev *dev, int64_t);
 
+/** @internal Function used to adjust the clock frequency. */
+typedef int (*eth_timesync_adjust_freq)(struct rte_eth_dev *dev, int64_t);
+
 /** @internal Function used to get time from the device clock. */
 typedef int (*eth_timesync_read_time)(struct rte_eth_dev *dev,
 				      struct timespec *timestamp);
@@ -1344,6 +1347,8 @@ struct eth_dev_ops {
 	eth_timesync_read_tx_timestamp_t timesync_read_tx_timestamp;
 	/** Adjust the device clock */
 	eth_timesync_adjust_time   timesync_adjust_time;
+	/** Adjust the clock frequency */
+	eth_timesync_adjust_freq   timesync_adjust_freq;
 	/** Get the device clock time */
 	eth_timesync_read_time     timesync_read_time;
 	/** Set the device clock time */
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 3dc7d02..d92554b 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -2196,6 +2196,15 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(ret);
 )
 
+/* Called in loop in examples/ptpclient */
+RTE_TRACE_POINT_FP(
+	rte_eth_trace_timesync_adjust_freq,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, int64_t ppm, int ret),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_i64(ppm);
+	rte_trace_point_emit_int(ret);
+)
+
 /* Called in loop in app/test-flow-perf */
 RTE_TRACE_POINT_FP(
 	rte_flow_trace_create,
diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
index 61010ca..c01b5d3 100644
--- a/lib/ethdev/ethdev_trace_points.c
+++ b/lib/ethdev/ethdev_trace_points.c
@@ -406,6 +406,9 @@ RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_tx_timestamp,
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_time,
 	lib.ethdev.timesync_adjust_time)
 
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_freq,
+	lib.ethdev.timesync_adjust_freq)
+
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_time,
 	lib.ethdev.timesync_read_time)
 
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 4d03255..f5934bb 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6017,6 +6017,24 @@ rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
 }
 
 int
+rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm)
+{
+	struct rte_eth_dev *dev;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (*dev->dev_ops->timesync_adjust_freq == NULL)
+		return -ENOTSUP;
+	ret = eth_err(port_id, (*dev->dev_ops->timesync_adjust_freq)(dev, ppm));
+
+	rte_eth_trace_timesync_adjust_freq(port_id, ppm, ret);
+
+	return ret;
+}
+
+int
 rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 99fe9e2..9737461 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -5102,6 +5102,25 @@ int rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
 
 /**
+ * Adjust the clock increment rate on an Ethernet device.
+ *
+ * This is usually used in conjunction with other Ethdev timesync functions to
+ * synchronize the device time using the IEEE1588/802.1AS protocol.
+ *
+ * @param port_id
+ *  The port identifier of the Ethernet device.
+ * @param ppm
+ *  Parts per million with 16-bit fractional field
+ *
+ * @return
+ *   - 0: Success.
+ *   - -ENODEV: The port ID is invalid.
+ *   - -EIO: if device is removed.
+ *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+int rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm);
+
+/**
  * Read the time from the timesync clock on an Ethernet device.
  *
  * This is usually used in conjunction with other Ethdev timesync functions to
-- 
2.9.5

[RFC 2/4] net/ice: add frequency adjustment support for PTP

[Simei Su]

Add ice support for new ethdev API to adjust frequency for IEEE1588
PTP. Also, this patch reworks code for converting software update
to hardware update.

Signed-off-by: Simei Su <simei.su@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 111 ++++++++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 9a88cf9..fa4d840 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -158,6 +158,7 @@ static int ice_timesync_read_rx_timestamp(struct rte_eth_dev *dev,
 static int ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 					  struct timespec *timestamp);
 static int ice_timesync_adjust_time(struct rte_eth_dev *dev, int64_t delta);
+static int ice_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm);
 static int ice_timesync_read_time(struct rte_eth_dev *dev,
 				  struct timespec *timestamp);
 static int ice_timesync_write_time(struct rte_eth_dev *dev,
@@ -274,6 +275,7 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.timesync_read_rx_timestamp   = ice_timesync_read_rx_timestamp,
 	.timesync_read_tx_timestamp   = ice_timesync_read_tx_timestamp,
 	.timesync_adjust_time         = ice_timesync_adjust_time,
+	.timesync_adjust_freq         = ice_timesync_adjust_freq,
 	.timesync_read_time           = ice_timesync_read_time,
 	.timesync_write_time          = ice_timesync_write_time,
 	.timesync_disable             = ice_timesync_disable,
@@ -5840,23 +5842,6 @@ ice_timesync_enable(struct rte_eth_dev *dev)
 		}
 	}
 
-	/* Initialize cycle counters for system time/RX/TX timestamp */
-	memset(&ad->systime_tc, 0, sizeof(struct rte_timecounter));
-	memset(&ad->rx_tstamp_tc, 0, sizeof(struct rte_timecounter));
-	memset(&ad->tx_tstamp_tc, 0, sizeof(struct rte_timecounter));
-
-	ad->systime_tc.cc_mask = ICE_CYCLECOUNTER_MASK;
-	ad->systime_tc.cc_shift = 0;
-	ad->systime_tc.nsec_mask = 0;
-
-	ad->rx_tstamp_tc.cc_mask = ICE_CYCLECOUNTER_MASK;
-	ad->rx_tstamp_tc.cc_shift = 0;
-	ad->rx_tstamp_tc.nsec_mask = 0;
-
-	ad->tx_tstamp_tc.cc_mask = ICE_CYCLECOUNTER_MASK;
-	ad->tx_tstamp_tc.cc_shift = 0;
-	ad->tx_tstamp_tc.nsec_mask = 0;
-
 	ad->ptp_ena = 1;
 
 	return 0;
@@ -5871,14 +5856,13 @@ ice_timesync_read_rx_timestamp(struct rte_eth_dev *dev,
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	struct ice_rx_queue *rxq;
 	uint32_t ts_high;
-	uint64_t ts_ns, ns;
+	uint64_t ts_ns;
 
 	rxq = dev->data->rx_queues[flags];
 
 	ts_high = rxq->time_high;
 	ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1, ts_high);
-	ns = rte_timecounter_update(&ad->rx_tstamp_tc, ts_ns);
-	*timestamp = rte_ns_to_timespec(ns);
+	*timestamp = rte_ns_to_timespec(ts_ns);
 
 	return 0;
 }
@@ -5891,7 +5875,7 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	struct ice_adapter *ad =
 			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	uint8_t lport;
-	uint64_t ts_ns, ns, tstamp;
+	uint64_t ts_ns, tstamp;
 	const uint64_t mask = 0xFFFFFFFF;
 	int ret;
 
@@ -5904,8 +5888,7 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 	}
 
 	ts_ns = ice_tstamp_convert_32b_64b(hw, ad, 1, (tstamp >> 8) & mask);
-	ns = rte_timecounter_update(&ad->tx_tstamp_tc, ts_ns);
-	*timestamp = rte_ns_to_timespec(ns);
+	*timestamp = rte_ns_to_timespec(ts_ns);
 
 	return 0;
 }
@@ -5913,12 +5896,66 @@ ice_timesync_read_tx_timestamp(struct rte_eth_dev *dev,
 static int
 ice_timesync_adjust_time(struct rte_eth_dev *dev, int64_t delta)
 {
-	struct ice_adapter *ad =
-			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint8_t tmr_idx = hw->func_caps.ts_func_info.tmr_index_assoc;
+	uint32_t lo, lo2, hi;
+	uint64_t time, ns;
+	int ret;
+
+	if (delta > INT32_MAX || delta < INT32_MIN) {
+		lo = ICE_READ_REG(hw, GLTSYN_TIME_L(tmr_idx));
+		hi = ICE_READ_REG(hw, GLTSYN_TIME_H(tmr_idx));
+		lo2 = ICE_READ_REG(hw, GLTSYN_TIME_L(tmr_idx));
+
+		if (lo2 < lo) {
+			lo = ICE_READ_REG(hw, GLTSYN_TIME_L(tmr_idx));
+			hi = ICE_READ_REG(hw, GLTSYN_TIME_H(tmr_idx));
+		}
+
+		time = ((uint64_t)hi << 32) | lo;
+		ns = time + delta;
+
+		return ice_ptp_init_time(hw, ns);
+	}
+
+	ret = ice_ptp_adj_clock(hw, delta, true);
+	if (ret)
+		return -1;
+
+	return 0;
+}
 
-	ad->systime_tc.nsec += delta;
-	ad->rx_tstamp_tc.nsec += delta;
-	ad->tx_tstamp_tc.nsec += delta;
+#define DEFAULT_INCVAL_E810 0x13b13b13bULL
+static int
+ice_timesync_adjust_freq(struct rte_eth_dev *dev, int64_t ppm)
+{
+	uint64_t freq, divisor = 1000000ULL;
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	int64_t incval = DEFAULT_INCVAL_E810, diff;
+	int neg_adj = 0;
+	int ret;
+
+	if (ppm < 0) {
+		neg_adj = 1;
+		ppm = -ppm;
+	}
+
+	while ((uint64_t)ppm > UINT64_MAX / incval) {
+		ppm >>= 2;
+		divisor >>= 2;
+	}
+
+	freq = (incval * (uint64_t)ppm) >> 16;
+	diff = freq / divisor;
+
+	if (neg_adj)
+		incval -= diff;
+	else
+		incval += diff;
+
+	ret = ice_ptp_write_incval_locked(hw, incval);
+	if (ret)
+		return -1;
 
 	return 0;
 }
@@ -5926,15 +5963,14 @@ ice_timesync_adjust_time(struct rte_eth_dev *dev, int64_t delta)
 static int
 ice_timesync_write_time(struct rte_eth_dev *dev, const struct timespec *ts)
 {
-	struct ice_adapter *ad =
-			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint64_t ns;
+	int ret;
 
 	ns = rte_timespec_to_ns(ts);
-
-	ad->systime_tc.nsec = ns;
-	ad->rx_tstamp_tc.nsec = ns;
-	ad->tx_tstamp_tc.nsec = ns;
+	ret = ice_ptp_init_time(hw, ns);
+	if (ret)
+		return -1;
 
 	return 0;
 }
@@ -5943,11 +5979,9 @@ static int
 ice_timesync_read_time(struct rte_eth_dev *dev, struct timespec *ts)
 {
 	struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct ice_adapter *ad =
-			ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	uint8_t tmr_idx = hw->func_caps.ts_func_info.tmr_index_assoc;
 	uint32_t hi, lo, lo2;
-	uint64_t time, ns;
+	uint64_t time;
 
 	lo = ICE_READ_REG(hw, GLTSYN_TIME_L(tmr_idx));
 	hi = ICE_READ_REG(hw, GLTSYN_TIME_H(tmr_idx));
@@ -5959,8 +5993,7 @@ ice_timesync_read_time(struct rte_eth_dev *dev, struct timespec *ts)
 	}
 
 	time = ((uint64_t)hi << 32) | lo;
-	ns = rte_timecounter_update(&ad->systime_tc, time);
-	*ts = rte_ns_to_timespec(ns);
+	*ts = rte_ns_to_timespec(time);
 
 	return 0;
 }
-- 
2.9.5

[RFC 3/4] examples/ptpclient: refine application

[Simei Su]

This patch reworks code to split delay request message parsing
from follow up message parsing.

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 48 ++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index cdf2da6..74a1bf5 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -382,21 +382,11 @@ parse_sync(struct ptpv2_data_slave_ordinary *ptp_data, uint16_t rx_tstamp_idx)
 static void
 parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 {
-	struct rte_ether_hdr *eth_hdr;
-	struct rte_ether_addr eth_addr;
 	struct ptp_header *ptp_hdr;
-	struct clock_id *client_clkid;
 	struct ptp_message *ptp_msg;
-	struct delay_req_msg *req_msg;
-	struct rte_mbuf *created_pkt;
 	struct tstamp *origin_tstamp;
-	struct rte_ether_addr eth_multicast = ether_multicast;
-	size_t pkt_size;
-	int wait_us;
 	struct rte_mbuf *m = ptp_data->m;
-	int ret;
 
-	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
 	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
 			+ sizeof(struct rte_ether_hdr));
 	if (memcmp(&ptp_data->master_clock_id,
@@ -413,6 +403,26 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 	ptp_data->tstamp1.tv_sec =
 		((uint64_t)ntohl(origin_tstamp->sec_lsb)) |
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
+}
+
+static void
+send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	struct rte_ether_hdr *eth_hdr;
+	struct rte_ether_addr eth_addr;
+	struct ptp_header *ptp_hdr;
+	struct clock_id *client_clkid;
+	struct delay_req_msg *req_msg;
+	struct rte_mbuf *created_pkt;
+	struct rte_ether_addr eth_multicast = ether_multicast;
+	size_t pkt_size;
+	int wait_us;
+	struct rte_mbuf *m = ptp_data->m;
+	int ret;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
+	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
+			+ sizeof(struct rte_ether_hdr));
 
 	if (ptp_data->seqID_FOLLOWUP == ptp_data->seqID_SYNC) {
 		ret = rte_eth_macaddr_get(ptp_data->portid, &eth_addr);
@@ -550,12 +560,6 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
-			/* Evaluate the delta for adjustment. */
-			ptp_data->delta = delta_eval(ptp_data);
-
-			rte_eth_timesync_adjust_time(ptp_data->portid,
-						     ptp_data->delta);
-
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -568,6 +572,16 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 	}
 }
 
+static void
+ptp_adjust_time(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	/* Evaluate the delta for adjustment. */
+	ptp_data->delta = delta_eval(ptp_data);
+
+	rte_eth_timesync_adjust_time(ptp_data->portid,
+				     ptp_data->delta);
+}
+
 /* This function processes PTP packets, implementing slave PTP IEEE1588 L2
  * functionality.
  */
@@ -594,9 +608,11 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
+			ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
-- 
2.9.5

[RFC 4/4] examples/ptpclient: add frequency adjustment support

[Simei Su]

This patch adds PI servo algorithm to support frequency
adjustment API for IEEE1588 PTP.

For example, the command for starting ptpclient with PI algorithm is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1
--controller=pi

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 178 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 161 insertions(+), 17 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index 74a1bf5..3d074af 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -43,6 +43,28 @@
 #define KERNEL_TIME_ADJUST_LIMIT  20000
 #define PTP_PROTOCOL             0x88F7
 
+#define KP 0.7
+#define KI 0.3
+
+enum servo_state {
+	SERVO_UNLOCKED,
+	SERVO_JUMP,
+	SERVO_LOCKED,
+};
+
+struct pi_servo {
+	double offset[2];
+	double local[2];
+	double drift;
+	int count;
+};
+
+enum controller_mode {
+	MODE_NONE,
+	MODE_PI,
+	MAX_ALL
+} mode;
+
 struct rte_mempool *mbuf_pool;
 uint32_t ptp_enabled_port_mask;
 uint8_t ptp_enabled_port_nb;
@@ -132,6 +154,9 @@ struct ptpv2_data_slave_ordinary {
 	uint8_t ptpset;
 	uint8_t kernel_time_set;
 	uint16_t current_ptp_port;
+	int64_t master_offset;
+	int64_t path_delay;
+	struct pi_servo *servo;
 };
 
 static struct ptpv2_data_slave_ordinary ptp_data;
@@ -290,36 +315,44 @@ print_clock_info(struct ptpv2_data_slave_ordinary *ptp_data)
 			ptp_data->tstamp3.tv_sec,
 			(ptp_data->tstamp3.tv_nsec));
 
-	printf("\nT4 - Master Clock.  %lds %ldns ",
+	printf("\nT4 - Master Clock.  %lds %ldns\n",
 			ptp_data->tstamp4.tv_sec,
 			(ptp_data->tstamp4.tv_nsec));
 
-	printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
+	if (mode == MODE_NONE) {
+		printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
 			ptp_data->delta);
 
-	clock_gettime(CLOCK_REALTIME, &sys_time);
-	rte_eth_timesync_read_time(ptp_data->current_ptp_port, &net_time);
+		clock_gettime(CLOCK_REALTIME, &sys_time);
+		rte_eth_timesync_read_time(ptp_data->current_ptp_port,
+					   &net_time);
 
-	time_t ts = net_time.tv_sec;
+		time_t ts = net_time.tv_sec;
 
-	printf("\n\nComparison between Linux kernel Time and PTP:");
+		printf("\n\nComparison between Linux kernel Time and PTP:");
 
-	printf("\nCurrent PTP Time: %.24s %.9ld ns",
+		printf("\nCurrent PTP Time: %.24s %.9ld ns",
 			ctime(&ts), net_time.tv_nsec);
 
-	nsec = (int64_t)timespec64_to_ns(&net_time) -
+		nsec = (int64_t)timespec64_to_ns(&net_time) -
 			(int64_t)timespec64_to_ns(&sys_time);
-	ptp_data->new_adj = ns_to_timeval(nsec);
+		ptp_data->new_adj = ns_to_timeval(nsec);
+
+		gettimeofday(&ptp_data->new_adj, NULL);
 
-	gettimeofday(&ptp_data->new_adj, NULL);
+		time_t tp = ptp_data->new_adj.tv_sec;
 
-	time_t tp = ptp_data->new_adj.tv_sec;
+		printf("\nCurrent SYS Time: %.24s %.6ld ns",
+			ctime(&tp), ptp_data->new_adj.tv_usec);
 
-	printf("\nCurrent SYS Time: %.24s %.6ld ns",
-				ctime(&tp), ptp_data->new_adj.tv_usec);
+		printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
+			nsec);
+	}
 
-	printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
-				nsec);
+	if (mode == MODE_PI) {
+		printf("path delay: %"PRId64"ns\n", ptp_data->path_delay);
+		printf("master offset: %"PRId64"ns\n", ptp_data->master_offset);
+	}
 
 	printf("[Ctrl+C to quit]\n");
 
@@ -405,6 +438,76 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
 }
 
+static double
+pi_sample(struct pi_servo *s, double offset, double local_ts,
+	  enum servo_state *state)
+{
+	double ppb = 0.0;
+
+	switch (s->count) {
+	case 0:
+		s->offset[0] = offset;
+		s->local[0] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 1;
+		break;
+	case 1:
+		s->offset[1] = offset;
+		s->local[1] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 2;
+		break;
+	case 2:
+		s->drift += (s->offset[1] - s->offset[0]) /
+			(s->local[1] - s->local[0]);
+		*state = SERVO_UNLOCKED;
+		s->count = 3;
+		break;
+	case 3:
+		*state = SERVO_JUMP;
+		s->count = 4;
+		break;
+	case 4:
+		s->drift += KI * offset;
+		ppb = KP * offset + s->drift;
+		*state = SERVO_LOCKED;
+		break;
+	}
+
+	return ppb;
+}
+
+static void
+ptp_adjust_freq(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns;
+	double adj_freq;
+	enum servo_state state = SERVO_UNLOCKED;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	ptp_data->master_offset = t2_ns - t1_ns - ptp_data->path_delay;
+	if (!ptp_data->path_delay)
+		return;
+
+	adj_freq = pi_sample(ptp_data->servo, ptp_data->master_offset,
+			     t2_ns, &state);
+	switch (state) {
+	case SERVO_UNLOCKED:
+		break;
+	case SERVO_JUMP:
+		rte_eth_timesync_adjust_time(ptp_data->portid,
+					     -ptp_data->master_offset);
+		t1_ns = 0;
+		t2_ns = 0;
+		break;
+	case SERVO_LOCKED:
+		rte_eth_timesync_adjust_freq(ptp_data->portid,
+					     -(long)(adj_freq * 65.536));
+		break;
+	}
+}
+
 static void
 send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
 {
@@ -536,6 +639,21 @@ update_kernel_time(void)
 
 }
 
+static void
+clock_path_delay(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns, t3_ns, t4_ns;
+	int64_t pd;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	t3_ns = timespec64_to_ns(&ptp_data->tstamp3);
+	t4_ns = timespec64_to_ns(&ptp_data->tstamp4);
+
+	pd = (t2_ns - t3_ns) + (t4_ns - t1_ns);
+	ptp_data->path_delay = pd / 2;
+}
+
 /*
  * Parse the DELAY_RESP message.
  */
@@ -560,6 +678,9 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
+			if (mode == MODE_PI)
+				clock_path_delay(ptp_data);
+
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -608,11 +729,14 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			if (mode == MODE_PI)
+				ptp_adjust_freq(&ptp_data);
 			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
-			ptp_adjust_time(&ptp_data);
+			if (mode == MODE_NONE)
+				ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
@@ -709,7 +833,10 @@ ptp_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	static struct option lgopts[] = { {NULL, 0, 0, 0} };
+	static struct option lgopts[] = {
+		{"controller", 1, 0, 0},
+		{NULL, 0, 0, 0}
+	};
 
 	argvopt = argv;
 
@@ -737,6 +864,11 @@ ptp_parse_args(int argc, char **argv)
 
 			ptp_data.kernel_time_set = ret;
 			break;
+		case 0:
+			if (!strcmp(lgopts[option_index].name, "controller"))
+				if (!strcmp(optarg, "pi"))
+					mode = MODE_PI;
+			break;
 
 		default:
 			print_usage(prgname);
@@ -780,6 +912,15 @@ main(int argc, char *argv[])
 		rte_exit(EXIT_FAILURE, "Error with PTP initialization\n");
 	/* >8 End of parsing specific arguments. */
 
+	if (mode == MODE_PI) {
+		ptp_data.servo = malloc(sizeof(*(ptp_data.servo)));
+		if (!ptp_data.servo)
+			rte_exit(EXIT_FAILURE, "no memory for servo\n");
+
+		ptp_data.servo->drift = 0;
+		ptp_data.servo->count = 0;
+	}
+
 	/* Check that there is an even number of ports to send/receive on. */
 	nb_ports = rte_eth_dev_count_avail();
 
@@ -819,6 +960,9 @@ main(int argc, char *argv[])
 	/* Call lcore_main on the main core only. */
 	lcore_main();
 
+	if (mode == MODE_PI)
+		free(ptp_data.servo);
+
 	/* clean up the EAL */
 	rte_eal_cleanup();
 
-- 
2.9.5

[RFC v2 0/3] add frequency adjustment support for PTP timesync

[Simei Su]

This patchset cover below parts:
(1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq" that
   enables frequency adjustment during PTP timesync. This new API aligns with
   the kernel PTP which already supports frequency adjustment. It brings DPDK
   closer in alignment with the kernel's best practice.

(2)Refine the ptpclient application by applying a PI algorithm that leverages
   the new API to further improve timesync accuracy. These changes doesn't break
   original solution and creates a more accurate solution for DPDK-based high
   accuracy PTP. We have provided significant improvements for timesync accuracy
   on e810 and we believe these improvements will also benefit other devices.

The original command for starting ptpclient is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1

The command with PI algorithm is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1 -- controller=pi

[RFC v2 1/3] ethdev: add frequency adjustment API.
[RFC v2 2/3] examples/ptpclient: refine application.
[RFC v2 3/3] examples/ptpclient: add frequency adjustment support.

v2:
* Remove the ice PMD part from the RFC.
* Add description in cover letter.
* Refine commit log in patch.

Simei Su (3):
  ethdev: add frequency adjustment API
  examples/ptpclient: refine application
  examples/ptpclient: add frequency adjustment support

 examples/ptpclient/ptpclient.c   | 222 +++++++++++++++++++++++++++++++++------
 lib/ethdev/ethdev_driver.h       |   5 +
 lib/ethdev/ethdev_trace.h        |   9 ++
 lib/ethdev/ethdev_trace_points.c |   3 +
 lib/ethdev/rte_ethdev.c          |  18 ++++
 lib/ethdev/rte_ethdev.h          |  19 ++++
 6 files changed, 245 insertions(+), 31 deletions(-)

-- 
2.9.5

[RFC v2 1/3] ethdev: add frequency adjustment API

[Simei Su]

This patch introduces a new timesync API "rte_eth_timesync_adjust_freq"
which enables frequency adjustment during PTP timesync.

Signed-off-by: Simei Su <simei.su@intel.com>
---
 lib/ethdev/ethdev_driver.h       |  5 +++++
 lib/ethdev/ethdev_trace.h        |  9 +++++++++
 lib/ethdev/ethdev_trace_points.c |  3 +++
 lib/ethdev/rte_ethdev.c          | 18 ++++++++++++++++++
 lib/ethdev/rte_ethdev.h          | 19 +++++++++++++++++++
 5 files changed, 54 insertions(+)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 2c9d615..b1451d2 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -633,6 +633,9 @@ typedef int (*eth_timesync_read_tx_timestamp_t)(struct rte_eth_dev *dev,
 /** @internal Function used to adjust the device clock. */
 typedef int (*eth_timesync_adjust_time)(struct rte_eth_dev *dev, int64_t);
 
+/** @internal Function used to adjust the clock frequency. */
+typedef int (*eth_timesync_adjust_freq)(struct rte_eth_dev *dev, int64_t);
+
 /** @internal Function used to get time from the device clock. */
 typedef int (*eth_timesync_read_time)(struct rte_eth_dev *dev,
 				      struct timespec *timestamp);
@@ -1344,6 +1347,8 @@ struct eth_dev_ops {
 	eth_timesync_read_tx_timestamp_t timesync_read_tx_timestamp;
 	/** Adjust the device clock */
 	eth_timesync_adjust_time   timesync_adjust_time;
+	/** Adjust the clock frequency */
+	eth_timesync_adjust_freq   timesync_adjust_freq;
 	/** Get the device clock time */
 	eth_timesync_read_time     timesync_read_time;
 	/** Set the device clock time */
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 3dc7d02..d92554b 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -2196,6 +2196,15 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(ret);
 )
 
+/* Called in loop in examples/ptpclient */
+RTE_TRACE_POINT_FP(
+	rte_eth_trace_timesync_adjust_freq,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, int64_t ppm, int ret),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_i64(ppm);
+	rte_trace_point_emit_int(ret);
+)
+
 /* Called in loop in app/test-flow-perf */
 RTE_TRACE_POINT_FP(
 	rte_flow_trace_create,
diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
index 61010ca..c01b5d3 100644
--- a/lib/ethdev/ethdev_trace_points.c
+++ b/lib/ethdev/ethdev_trace_points.c
@@ -406,6 +406,9 @@ RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_tx_timestamp,
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_time,
 	lib.ethdev.timesync_adjust_time)
 
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_freq,
+	lib.ethdev.timesync_adjust_freq)
+
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_time,
 	lib.ethdev.timesync_read_time)
 
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 4d03255..f5934bb 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6017,6 +6017,24 @@ rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
 }
 
 int
+rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm)
+{
+	struct rte_eth_dev *dev;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (*dev->dev_ops->timesync_adjust_freq == NULL)
+		return -ENOTSUP;
+	ret = eth_err(port_id, (*dev->dev_ops->timesync_adjust_freq)(dev, ppm));
+
+	rte_eth_trace_timesync_adjust_freq(port_id, ppm, ret);
+
+	return ret;
+}
+
+int
 rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 99fe9e2..9737461 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -5102,6 +5102,25 @@ int rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
 
 /**
+ * Adjust the clock increment rate on an Ethernet device.
+ *
+ * This is usually used in conjunction with other Ethdev timesync functions to
+ * synchronize the device time using the IEEE1588/802.1AS protocol.
+ *
+ * @param port_id
+ *  The port identifier of the Ethernet device.
+ * @param ppm
+ *  Parts per million with 16-bit fractional field
+ *
+ * @return
+ *   - 0: Success.
+ *   - -ENODEV: The port ID is invalid.
+ *   - -EIO: if device is removed.
+ *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+int rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm);
+
+/**
  * Read the time from the timesync clock on an Ethernet device.
  *
  * This is usually used in conjunction with other Ethdev timesync functions to
-- 
2.9.5

Re: [RFC v2 1/3] ethdev: add frequency adjustment API

[Thomas Monjalon]

Hello,

03/04/2023 11:22, Simei Su:
> This patch introduces a new timesync API "rte_eth_timesync_adjust_freq"
> which enables frequency adjustment during PTP timesync.

You should explain how it compares with existing functions
like rte_eth_timesync_adjust_time().

[...]
>  /**
> + * Adjust the clock increment rate on an Ethernet device.
> + *
> + * This is usually used in conjunction with other Ethdev timesync functions to
> + * synchronize the device time using the IEEE1588/802.1AS protocol.

All timesync functions have this sentence,
but it does not help to understand how to combine them.

> + *
> + * @param port_id
> + *  The port identifier of the Ethernet device.
> + * @param ppm
> + *  Parts per million with 16-bit fractional field

Sorry I don't understand what this parameter is about.
Probably because I'm not an expert of IEEE1588/802.1AS protocol.
Please make it possible to understand for basic users.

> + *
> + * @return
> + *   - 0: Success.
> + *   - -ENODEV: The port ID is invalid.
> + *   - -EIO: if device is removed.
> + *   - -ENOTSUP: The function is not supported by the Ethernet driver.
> + */
> +int rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm);

In general, I think there is an effort to provide in order to make
the timesync API better understandable.
Example of something to explain: how Rx and Tx timestamps of a port
are differents? Is it different of rte_eth_timesync_read_time()?

The function rte_eth_read_clock() was provided with a better explanation.

Please make improvements in API documentation as part of this API,
I don't want to get more functions without improving explanation
of older functions.
Thank you

RE: [RFC v2 1/3] ethdev: add frequency adjustment API

[Su, Simei]

Hi Thomas,

Thanks a lot for your review. I have already sent RFC v3 patch based on your comments.
https://patchwork.dpdk.org/project/dpdk/cover/20230522132332.102030-1-simei.su@intel.com/

Thanks,
Simei

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, May 15, 2023 10:18 PM
> To: Su, Simei <simei.su@intel.com>
> Cc: ferruh.yigit@amd.com; andrew.rybchenko@oktetlabs.ru; Rybalchenko,
> Kirill <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>;
> david.marchand@redhat.com
> Subject: Re: [RFC v2 1/3] ethdev: add frequency adjustment API
> 
> Hello,
> 
> 03/04/2023 11:22, Simei Su:
> > This patch introduces a new timesync API "rte_eth_timesync_adjust_freq"
> > which enables frequency adjustment during PTP timesync.
> 
> You should explain how it compares with existing functions like
> rte_eth_timesync_adjust_time().
> 
> [...]
> >  /**
> > + * Adjust the clock increment rate on an Ethernet device.
> > + *
> > + * This is usually used in conjunction with other Ethdev timesync
> > + functions to
> > + * synchronize the device time using the IEEE1588/802.1AS protocol.
> 
> All timesync functions have this sentence, but it does not help to understand
> how to combine them.
> 
> > + *
> > + * @param port_id
> > + *  The port identifier of the Ethernet device.
> > + * @param ppm
> > + *  Parts per million with 16-bit fractional field
> 
> Sorry I don't understand what this parameter is about.
> Probably because I'm not an expert of IEEE1588/802.1AS protocol.
> Please make it possible to understand for basic users.
> 
> > + *
> > + * @return
> > + *   - 0: Success.
> > + *   - -ENODEV: The port ID is invalid.
> > + *   - -EIO: if device is removed.
> > + *   - -ENOTSUP: The function is not supported by the Ethernet driver.
> > + */
> > +int rte_eth_timesync_adjust_freq(uint16_t port_id, int64_t ppm);
> 
> In general, I think there is an effort to provide in order to make the timesync
> API better understandable.
> Example of something to explain: how Rx and Tx timestamps of a port are
> differents? Is it different of rte_eth_timesync_read_time()?
> 
> The function rte_eth_read_clock() was provided with a better explanation.
> 
> Please make improvements in API documentation as part of this API, I don't
> want to get more functions without improving explanation of older functions.
> Thank you
> 

[RFC v2 2/3] examples/ptpclient: refine application

[Simei Su]

This patch reworks code to split delay request message parsing
from follow up message parsing which doesn't break original logic.

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 48 ++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index cdf2da6..74a1bf5 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -382,21 +382,11 @@ parse_sync(struct ptpv2_data_slave_ordinary *ptp_data, uint16_t rx_tstamp_idx)
 static void
 parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 {
-	struct rte_ether_hdr *eth_hdr;
-	struct rte_ether_addr eth_addr;
 	struct ptp_header *ptp_hdr;
-	struct clock_id *client_clkid;
 	struct ptp_message *ptp_msg;
-	struct delay_req_msg *req_msg;
-	struct rte_mbuf *created_pkt;
 	struct tstamp *origin_tstamp;
-	struct rte_ether_addr eth_multicast = ether_multicast;
-	size_t pkt_size;
-	int wait_us;
 	struct rte_mbuf *m = ptp_data->m;
-	int ret;
 
-	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
 	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
 			+ sizeof(struct rte_ether_hdr));
 	if (memcmp(&ptp_data->master_clock_id,
@@ -413,6 +403,26 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 	ptp_data->tstamp1.tv_sec =
 		((uint64_t)ntohl(origin_tstamp->sec_lsb)) |
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
+}
+
+static void
+send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	struct rte_ether_hdr *eth_hdr;
+	struct rte_ether_addr eth_addr;
+	struct ptp_header *ptp_hdr;
+	struct clock_id *client_clkid;
+	struct delay_req_msg *req_msg;
+	struct rte_mbuf *created_pkt;
+	struct rte_ether_addr eth_multicast = ether_multicast;
+	size_t pkt_size;
+	int wait_us;
+	struct rte_mbuf *m = ptp_data->m;
+	int ret;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
+	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
+			+ sizeof(struct rte_ether_hdr));
 
 	if (ptp_data->seqID_FOLLOWUP == ptp_data->seqID_SYNC) {
 		ret = rte_eth_macaddr_get(ptp_data->portid, &eth_addr);
@@ -550,12 +560,6 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
-			/* Evaluate the delta for adjustment. */
-			ptp_data->delta = delta_eval(ptp_data);
-
-			rte_eth_timesync_adjust_time(ptp_data->portid,
-						     ptp_data->delta);
-
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -568,6 +572,16 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 	}
 }
 
+static void
+ptp_adjust_time(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	/* Evaluate the delta for adjustment. */
+	ptp_data->delta = delta_eval(ptp_data);
+
+	rte_eth_timesync_adjust_time(ptp_data->portid,
+				     ptp_data->delta);
+}
+
 /* This function processes PTP packets, implementing slave PTP IEEE1588 L2
  * functionality.
  */
@@ -594,9 +608,11 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
+			ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
-- 
2.9.5

[RFC v2 3/3] examples/ptpclient: add frequency adjustment support

[Simei Su]

This patch applys PI servo algorithm to leverage frequency adjustment
API to improve PTP timesync accuracy.

The command for starting ptpclient with PI algorithm is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1
--controller=pi

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 178 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 161 insertions(+), 17 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index 74a1bf5..3d074af 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -43,6 +43,28 @@
 #define KERNEL_TIME_ADJUST_LIMIT  20000
 #define PTP_PROTOCOL             0x88F7
 
+#define KP 0.7
+#define KI 0.3
+
+enum servo_state {
+	SERVO_UNLOCKED,
+	SERVO_JUMP,
+	SERVO_LOCKED,
+};
+
+struct pi_servo {
+	double offset[2];
+	double local[2];
+	double drift;
+	int count;
+};
+
+enum controller_mode {
+	MODE_NONE,
+	MODE_PI,
+	MAX_ALL
+} mode;
+
 struct rte_mempool *mbuf_pool;
 uint32_t ptp_enabled_port_mask;
 uint8_t ptp_enabled_port_nb;
@@ -132,6 +154,9 @@ struct ptpv2_data_slave_ordinary {
 	uint8_t ptpset;
 	uint8_t kernel_time_set;
 	uint16_t current_ptp_port;
+	int64_t master_offset;
+	int64_t path_delay;
+	struct pi_servo *servo;
 };
 
 static struct ptpv2_data_slave_ordinary ptp_data;
@@ -290,36 +315,44 @@ print_clock_info(struct ptpv2_data_slave_ordinary *ptp_data)
 			ptp_data->tstamp3.tv_sec,
 			(ptp_data->tstamp3.tv_nsec));
 
-	printf("\nT4 - Master Clock.  %lds %ldns ",
+	printf("\nT4 - Master Clock.  %lds %ldns\n",
 			ptp_data->tstamp4.tv_sec,
 			(ptp_data->tstamp4.tv_nsec));
 
-	printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
+	if (mode == MODE_NONE) {
+		printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
 			ptp_data->delta);
 
-	clock_gettime(CLOCK_REALTIME, &sys_time);
-	rte_eth_timesync_read_time(ptp_data->current_ptp_port, &net_time);
+		clock_gettime(CLOCK_REALTIME, &sys_time);
+		rte_eth_timesync_read_time(ptp_data->current_ptp_port,
+					   &net_time);
 
-	time_t ts = net_time.tv_sec;
+		time_t ts = net_time.tv_sec;
 
-	printf("\n\nComparison between Linux kernel Time and PTP:");
+		printf("\n\nComparison between Linux kernel Time and PTP:");
 
-	printf("\nCurrent PTP Time: %.24s %.9ld ns",
+		printf("\nCurrent PTP Time: %.24s %.9ld ns",
 			ctime(&ts), net_time.tv_nsec);
 
-	nsec = (int64_t)timespec64_to_ns(&net_time) -
+		nsec = (int64_t)timespec64_to_ns(&net_time) -
 			(int64_t)timespec64_to_ns(&sys_time);
-	ptp_data->new_adj = ns_to_timeval(nsec);
+		ptp_data->new_adj = ns_to_timeval(nsec);
+
+		gettimeofday(&ptp_data->new_adj, NULL);
 
-	gettimeofday(&ptp_data->new_adj, NULL);
+		time_t tp = ptp_data->new_adj.tv_sec;
 
-	time_t tp = ptp_data->new_adj.tv_sec;
+		printf("\nCurrent SYS Time: %.24s %.6ld ns",
+			ctime(&tp), ptp_data->new_adj.tv_usec);
 
-	printf("\nCurrent SYS Time: %.24s %.6ld ns",
-				ctime(&tp), ptp_data->new_adj.tv_usec);
+		printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
+			nsec);
+	}
 
-	printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
-				nsec);
+	if (mode == MODE_PI) {
+		printf("path delay: %"PRId64"ns\n", ptp_data->path_delay);
+		printf("master offset: %"PRId64"ns\n", ptp_data->master_offset);
+	}
 
 	printf("[Ctrl+C to quit]\n");
 
@@ -405,6 +438,76 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
 }
 
+static double
+pi_sample(struct pi_servo *s, double offset, double local_ts,
+	  enum servo_state *state)
+{
+	double ppb = 0.0;
+
+	switch (s->count) {
+	case 0:
+		s->offset[0] = offset;
+		s->local[0] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 1;
+		break;
+	case 1:
+		s->offset[1] = offset;
+		s->local[1] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 2;
+		break;
+	case 2:
+		s->drift += (s->offset[1] - s->offset[0]) /
+			(s->local[1] - s->local[0]);
+		*state = SERVO_UNLOCKED;
+		s->count = 3;
+		break;
+	case 3:
+		*state = SERVO_JUMP;
+		s->count = 4;
+		break;
+	case 4:
+		s->drift += KI * offset;
+		ppb = KP * offset + s->drift;
+		*state = SERVO_LOCKED;
+		break;
+	}
+
+	return ppb;
+}
+
+static void
+ptp_adjust_freq(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns;
+	double adj_freq;
+	enum servo_state state = SERVO_UNLOCKED;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	ptp_data->master_offset = t2_ns - t1_ns - ptp_data->path_delay;
+	if (!ptp_data->path_delay)
+		return;
+
+	adj_freq = pi_sample(ptp_data->servo, ptp_data->master_offset,
+			     t2_ns, &state);
+	switch (state) {
+	case SERVO_UNLOCKED:
+		break;
+	case SERVO_JUMP:
+		rte_eth_timesync_adjust_time(ptp_data->portid,
+					     -ptp_data->master_offset);
+		t1_ns = 0;
+		t2_ns = 0;
+		break;
+	case SERVO_LOCKED:
+		rte_eth_timesync_adjust_freq(ptp_data->portid,
+					     -(long)(adj_freq * 65.536));
+		break;
+	}
+}
+
 static void
 send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
 {
@@ -536,6 +639,21 @@ update_kernel_time(void)
 
 }
 
+static void
+clock_path_delay(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns, t3_ns, t4_ns;
+	int64_t pd;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	t3_ns = timespec64_to_ns(&ptp_data->tstamp3);
+	t4_ns = timespec64_to_ns(&ptp_data->tstamp4);
+
+	pd = (t2_ns - t3_ns) + (t4_ns - t1_ns);
+	ptp_data->path_delay = pd / 2;
+}
+
 /*
  * Parse the DELAY_RESP message.
  */
@@ -560,6 +678,9 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
+			if (mode == MODE_PI)
+				clock_path_delay(ptp_data);
+
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -608,11 +729,14 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			if (mode == MODE_PI)
+				ptp_adjust_freq(&ptp_data);
 			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
-			ptp_adjust_time(&ptp_data);
+			if (mode == MODE_NONE)
+				ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
@@ -709,7 +833,10 @@ ptp_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	static struct option lgopts[] = { {NULL, 0, 0, 0} };
+	static struct option lgopts[] = {
+		{"controller", 1, 0, 0},
+		{NULL, 0, 0, 0}
+	};
 
 	argvopt = argv;
 
@@ -737,6 +864,11 @@ ptp_parse_args(int argc, char **argv)
 
 			ptp_data.kernel_time_set = ret;
 			break;
+		case 0:
+			if (!strcmp(lgopts[option_index].name, "controller"))
+				if (!strcmp(optarg, "pi"))
+					mode = MODE_PI;
+			break;
 
 		default:
 			print_usage(prgname);
@@ -780,6 +912,15 @@ main(int argc, char *argv[])
 		rte_exit(EXIT_FAILURE, "Error with PTP initialization\n");
 	/* >8 End of parsing specific arguments. */
 
+	if (mode == MODE_PI) {
+		ptp_data.servo = malloc(sizeof(*(ptp_data.servo)));
+		if (!ptp_data.servo)
+			rte_exit(EXIT_FAILURE, "no memory for servo\n");
+
+		ptp_data.servo->drift = 0;
+		ptp_data.servo->count = 0;
+	}
+
 	/* Check that there is an even number of ports to send/receive on. */
 	nb_ports = rte_eth_dev_count_avail();
 
@@ -819,6 +960,9 @@ main(int argc, char *argv[])
 	/* Call lcore_main on the main core only. */
 	lcore_main();
 
+	if (mode == MODE_PI)
+		free(ptp_data.servo);
+
 	/* clean up the EAL */
 	rte_eal_cleanup();
 
-- 
2.9.5

[RFC v3 0/3] add frequency adjustment support for PTP timesync

[Simei Su]

This patchset cover below parts:
(1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq" that
   enables frequency adjustment during PTP timesync. This new API aligns with
   the kernel PTP which already supports frequency adjustment. It brings DPDK
   closer in alignment with the kernel's best practice.

(2)Refine the ptpclient application by applying a PI algorithm that leverages
   the new API to further improve timesync accuracy. These changes doesn't break
   original solution and creates a more accurate solution for DPDK-based high
   accuracy PTP. We have provided significant improvements for timesync accuracy
   on e810 and we believe these improvements will also benefit other devices.

The original command for starting ptpclient is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1

The command with PI algorithm is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1 -- controller=pi

[RFC v3 1/3] ethdev: add frequency adjustment API.
[RFC v3 2/3] examples/ptpclient: refine application.
[RFC v3 3/3] examples/ptpclient: add frequency adjustment support.

v3:
* Refine commit log in patch.
* Add more description for the new API.

v2:
* Remove the ice PMD part from the RFC.
* Add description in cover letter.
* Refine commit log in patch.

Simei Su (3):
  ethdev: add frequency adjustment API
  examples/ptpclient: refine application
  examples/ptpclient: add frequency adjustment support

 examples/ptpclient/ptpclient.c   | 222 +++++++++++++++++++++++++++++++++------
 lib/ethdev/ethdev_driver.h       |   5 +
 lib/ethdev/ethdev_trace.h        |   9 ++
 lib/ethdev/ethdev_trace_points.c |   3 +
 lib/ethdev/rte_ethdev.c          |  19 ++++
 lib/ethdev/rte_ethdev.h          |  38 +++++++
 6 files changed, 265 insertions(+), 31 deletions(-)

-- 
2.9.5

[RFC v3 1/3] ethdev: add frequency adjustment API

[Simei Su]

This patch introduces a new timesync API "rte_eth_timesync_adjust_fine"
which enables finer adjustment of the PHC clock. During PTP timesync,
"rte_eth_timesync_adjust_time" focuses on phase adjustment while
"rte_eth_timesync_adjust_fine" focuses on frequency adjustment.

This new function gets the scaled_ppm (desired frequency offset from
nominal frequency in parts per million, but with a 16 bit binary
fractional field).

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 lib/ethdev/ethdev_driver.h       |  5 +++++
 lib/ethdev/ethdev_trace.h        |  9 +++++++++
 lib/ethdev/ethdev_trace_points.c |  3 +++
 lib/ethdev/rte_ethdev.c          | 19 +++++++++++++++++++
 lib/ethdev/rte_ethdev.h          | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 74 insertions(+)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 2c9d615..b6fce64 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -633,6 +633,9 @@ typedef int (*eth_timesync_read_tx_timestamp_t)(struct rte_eth_dev *dev,
 /** @internal Function used to adjust the device clock. */
 typedef int (*eth_timesync_adjust_time)(struct rte_eth_dev *dev, int64_t);
 
+/** @internal Function used to adjust the clock increment rate. */
+typedef int (*eth_timesync_adjust_fine)(struct rte_eth_dev *dev, int64_t);
+
 /** @internal Function used to get time from the device clock. */
 typedef int (*eth_timesync_read_time)(struct rte_eth_dev *dev,
 				      struct timespec *timestamp);
@@ -1344,6 +1347,8 @@ struct eth_dev_ops {
 	eth_timesync_read_tx_timestamp_t timesync_read_tx_timestamp;
 	/** Adjust the device clock */
 	eth_timesync_adjust_time   timesync_adjust_time;
+	/** Adjust the clock increment rate */
+	eth_timesync_adjust_fine   timesync_adjust_fine;
 	/** Get the device clock time */
 	eth_timesync_read_time     timesync_read_time;
 	/** Set the device clock time */
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 3dc7d02..1b10aab 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -2196,6 +2196,15 @@ RTE_TRACE_POINT_FP(
 	rte_trace_point_emit_int(ret);
 )
 
+/* Called in loop in examples/ptpclient */
+RTE_TRACE_POINT_FP(
+	rte_eth_trace_timesync_adjust_fine,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, int64_t scaled_ppm, int ret),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_i64(scaled_ppm);
+	rte_trace_point_emit_int(ret);
+)
+
 /* Called in loop in app/test-flow-perf */
 RTE_TRACE_POINT_FP(
 	rte_flow_trace_create,
diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
index 61010ca..b7ac391 100644
--- a/lib/ethdev/ethdev_trace_points.c
+++ b/lib/ethdev/ethdev_trace_points.c
@@ -406,6 +406,9 @@ RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_tx_timestamp,
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_time,
 	lib.ethdev.timesync_adjust_time)
 
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_adjust_fine,
+	lib.ethdev.timesync_adjust_fine)
+
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_timesync_read_time,
 	lib.ethdev.timesync_read_time)
 
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 4d03255..17be7c8 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -6017,6 +6017,25 @@ rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
 }
 
 int
+rte_eth_timesync_adjust_fine(uint16_t port_id, int64_t scaled_ppm)
+{
+	struct rte_eth_dev *dev;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (*dev->dev_ops->timesync_adjust_fine == NULL)
+		return -ENOTSUP;
+	ret = eth_err(port_id, (*dev->dev_ops->timesync_adjust_fine)(dev,
+								scaled_ppm));
+
+	rte_eth_trace_timesync_adjust_fine(port_id, scaled_ppm, ret);
+
+	return ret;
+}
+
+int
 rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 99fe9e2..a743d83 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -5102,6 +5102,44 @@ int rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
 
 /**
+ * Adjust the frequency of the PHC cycle counter by the indicated amount
+ * from the base frequency.
+ *
+ * This function is used to do hardware timestamp adjustment in fine
+ * granularity. It can be used in conjunction with rte_eth_timesync_adjust_time
+ * to do more precise time control.
+ *
+ * E.g, below is a simple usage:
+ * if master offset > master offset threshold
+ *	do rte_eth_timesync_adjust_time;
+ * else
+ *	do rte_eth_timesync_adjust_fine;
+ *
+ * The user can apply a control algorithm to leverage these two APIs, one
+ * example is in dpdk-ptpclient.
+ *
+ * This API is implemented with the below basic logic:
+ *   - Determine a base frequency value
+ *   - Multiply this by the abs() of the requested adjustment, then divide by
+ *     the appropriate divisor (65536 billion).
+ *   - Add or subtract this difference from the base frequency to calculate a
+ *     new adjustment.
+ *
+ * @param port_id
+ *  The port identifier of the Ethernet device.
+ * @param scaled_ppm
+ *  Desired frequency change in scaled parts per million. Scaled parts per
+ *  million is ppm with a 16-bit binary fractional field.
+ *
+ * @return
+ *   - 0: Success.
+ *   - -ENODEV: The port ID is invalid.
+ *   - -EIO: if device is removed.
+ *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+int rte_eth_timesync_adjust_fine(uint16_t port_id, int64_t scaled_ppm);
+
+/**
  * Read the time from the timesync clock on an Ethernet device.
  *
  * This is usually used in conjunction with other Ethdev timesync functions to
-- 
2.9.5

[RFC v3 2/3] examples/ptpclient: refine application

[Simei Su]

This patch reworks code to split delay request message parsing
from follow up message parsing which doesn't break original logic.

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 48 ++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index cdf2da6..74a1bf5 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -382,21 +382,11 @@ parse_sync(struct ptpv2_data_slave_ordinary *ptp_data, uint16_t rx_tstamp_idx)
 static void
 parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 {
-	struct rte_ether_hdr *eth_hdr;
-	struct rte_ether_addr eth_addr;
 	struct ptp_header *ptp_hdr;
-	struct clock_id *client_clkid;
 	struct ptp_message *ptp_msg;
-	struct delay_req_msg *req_msg;
-	struct rte_mbuf *created_pkt;
 	struct tstamp *origin_tstamp;
-	struct rte_ether_addr eth_multicast = ether_multicast;
-	size_t pkt_size;
-	int wait_us;
 	struct rte_mbuf *m = ptp_data->m;
-	int ret;
 
-	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
 	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
 			+ sizeof(struct rte_ether_hdr));
 	if (memcmp(&ptp_data->master_clock_id,
@@ -413,6 +403,26 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 	ptp_data->tstamp1.tv_sec =
 		((uint64_t)ntohl(origin_tstamp->sec_lsb)) |
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
+}
+
+static void
+send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	struct rte_ether_hdr *eth_hdr;
+	struct rte_ether_addr eth_addr;
+	struct ptp_header *ptp_hdr;
+	struct clock_id *client_clkid;
+	struct delay_req_msg *req_msg;
+	struct rte_mbuf *created_pkt;
+	struct rte_ether_addr eth_multicast = ether_multicast;
+	size_t pkt_size;
+	int wait_us;
+	struct rte_mbuf *m = ptp_data->m;
+	int ret;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
+	ptp_hdr = (struct ptp_header *)(rte_pktmbuf_mtod(m, char *)
+			+ sizeof(struct rte_ether_hdr));
 
 	if (ptp_data->seqID_FOLLOWUP == ptp_data->seqID_SYNC) {
 		ret = rte_eth_macaddr_get(ptp_data->portid, &eth_addr);
@@ -550,12 +560,6 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
-			/* Evaluate the delta for adjustment. */
-			ptp_data->delta = delta_eval(ptp_data);
-
-			rte_eth_timesync_adjust_time(ptp_data->portid,
-						     ptp_data->delta);
-
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -568,6 +572,16 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 	}
 }
 
+static void
+ptp_adjust_time(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	/* Evaluate the delta for adjustment. */
+	ptp_data->delta = delta_eval(ptp_data);
+
+	rte_eth_timesync_adjust_time(ptp_data->portid,
+				     ptp_data->delta);
+}
+
 /* This function processes PTP packets, implementing slave PTP IEEE1588 L2
  * functionality.
  */
@@ -594,9 +608,11 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
+			ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
-- 
2.9.5

Re: [RFC v3 2/3] examples/ptpclient: refine application

[Ferruh Yigit]

On 5/22/2023 2:23 PM, Simei Su wrote:
> This patch reworks code to split delay request message parsing
> from follow up message parsing which doesn't break original logic.
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
>

Refactoring looks good to me.

[RFC v3 3/3] examples/ptpclient: add frequency adjustment support

[Simei Su]

This patch applys PI servo algorithm to leverage frequency adjustment
API to improve PTP timesync accuracy.

The command for starting ptpclient with PI algorithm is:
./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1
--controller=pi

Signed-off-by: Simei Su <simei.su@intel.com>
Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
---
 examples/ptpclient/ptpclient.c | 178 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 161 insertions(+), 17 deletions(-)

diff --git a/examples/ptpclient/ptpclient.c b/examples/ptpclient/ptpclient.c
index 74a1bf5..55b63be 100644
--- a/examples/ptpclient/ptpclient.c
+++ b/examples/ptpclient/ptpclient.c
@@ -43,6 +43,28 @@
 #define KERNEL_TIME_ADJUST_LIMIT  20000
 #define PTP_PROTOCOL             0x88F7
 
+#define KP 0.7
+#define KI 0.3
+
+enum servo_state {
+	SERVO_UNLOCKED,
+	SERVO_JUMP,
+	SERVO_LOCKED,
+};
+
+struct pi_servo {
+	double offset[2];
+	double local[2];
+	double drift;
+	int count;
+};
+
+enum controller_mode {
+	MODE_NONE,
+	MODE_PI,
+	MAX_ALL
+} mode;
+
 struct rte_mempool *mbuf_pool;
 uint32_t ptp_enabled_port_mask;
 uint8_t ptp_enabled_port_nb;
@@ -132,6 +154,9 @@ struct ptpv2_data_slave_ordinary {
 	uint8_t ptpset;
 	uint8_t kernel_time_set;
 	uint16_t current_ptp_port;
+	int64_t master_offset;
+	int64_t path_delay;
+	struct pi_servo *servo;
 };
 
 static struct ptpv2_data_slave_ordinary ptp_data;
@@ -290,36 +315,44 @@ print_clock_info(struct ptpv2_data_slave_ordinary *ptp_data)
 			ptp_data->tstamp3.tv_sec,
 			(ptp_data->tstamp3.tv_nsec));
 
-	printf("\nT4 - Master Clock.  %lds %ldns ",
+	printf("\nT4 - Master Clock.  %lds %ldns\n",
 			ptp_data->tstamp4.tv_sec,
 			(ptp_data->tstamp4.tv_nsec));
 
-	printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
+	if (mode == MODE_NONE) {
+		printf("\nDelta between master and slave clocks:%"PRId64"ns\n",
 			ptp_data->delta);
 
-	clock_gettime(CLOCK_REALTIME, &sys_time);
-	rte_eth_timesync_read_time(ptp_data->current_ptp_port, &net_time);
+		clock_gettime(CLOCK_REALTIME, &sys_time);
+		rte_eth_timesync_read_time(ptp_data->current_ptp_port,
+					   &net_time);
 
-	time_t ts = net_time.tv_sec;
+		time_t ts = net_time.tv_sec;
 
-	printf("\n\nComparison between Linux kernel Time and PTP:");
+		printf("\n\nComparison between Linux kernel Time and PTP:");
 
-	printf("\nCurrent PTP Time: %.24s %.9ld ns",
+		printf("\nCurrent PTP Time: %.24s %.9ld ns",
 			ctime(&ts), net_time.tv_nsec);
 
-	nsec = (int64_t)timespec64_to_ns(&net_time) -
+		nsec = (int64_t)timespec64_to_ns(&net_time) -
 			(int64_t)timespec64_to_ns(&sys_time);
-	ptp_data->new_adj = ns_to_timeval(nsec);
+		ptp_data->new_adj = ns_to_timeval(nsec);
+
+		gettimeofday(&ptp_data->new_adj, NULL);
 
-	gettimeofday(&ptp_data->new_adj, NULL);
+		time_t tp = ptp_data->new_adj.tv_sec;
 
-	time_t tp = ptp_data->new_adj.tv_sec;
+		printf("\nCurrent SYS Time: %.24s %.6ld ns",
+			ctime(&tp), ptp_data->new_adj.tv_usec);
 
-	printf("\nCurrent SYS Time: %.24s %.6ld ns",
-				ctime(&tp), ptp_data->new_adj.tv_usec);
+		printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
+			nsec);
+	}
 
-	printf("\nDelta between PTP and Linux Kernel time:%"PRId64"ns\n",
-				nsec);
+	if (mode == MODE_PI) {
+		printf("path delay: %"PRId64"ns\n", ptp_data->path_delay);
+		printf("master offset: %"PRId64"ns\n", ptp_data->master_offset);
+	}
 
 	printf("[Ctrl+C to quit]\n");
 
@@ -405,6 +438,76 @@ parse_fup(struct ptpv2_data_slave_ordinary *ptp_data)
 		(((uint64_t)ntohs(origin_tstamp->sec_msb)) << 32);
 }
 
+static double
+pi_sample(struct pi_servo *s, double offset, double local_ts,
+	  enum servo_state *state)
+{
+	double ppb = 0.0;
+
+	switch (s->count) {
+	case 0:
+		s->offset[0] = offset;
+		s->local[0] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 1;
+		break;
+	case 1:
+		s->offset[1] = offset;
+		s->local[1] = local_ts;
+		*state = SERVO_UNLOCKED;
+		s->count = 2;
+		break;
+	case 2:
+		s->drift += (s->offset[1] - s->offset[0]) /
+			(s->local[1] - s->local[0]);
+		*state = SERVO_UNLOCKED;
+		s->count = 3;
+		break;
+	case 3:
+		*state = SERVO_JUMP;
+		s->count = 4;
+		break;
+	case 4:
+		s->drift += KI * offset;
+		ppb = KP * offset + s->drift;
+		*state = SERVO_LOCKED;
+		break;
+	}
+
+	return ppb;
+}
+
+static void
+ptp_adjust_freq(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns;
+	double adj_freq;
+	enum servo_state state = SERVO_UNLOCKED;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	ptp_data->master_offset = t2_ns - t1_ns - ptp_data->path_delay;
+	if (!ptp_data->path_delay)
+		return;
+
+	adj_freq = pi_sample(ptp_data->servo, ptp_data->master_offset,
+			     t2_ns, &state);
+	switch (state) {
+	case SERVO_UNLOCKED:
+		break;
+	case SERVO_JUMP:
+		rte_eth_timesync_adjust_time(ptp_data->portid,
+					     -ptp_data->master_offset);
+		t1_ns = 0;
+		t2_ns = 0;
+		break;
+	case SERVO_LOCKED:
+		rte_eth_timesync_adjust_fine(ptp_data->portid,
+					     -(long)(adj_freq * 65.536));
+		break;
+	}
+}
+
 static void
 send_delay_request(struct ptpv2_data_slave_ordinary *ptp_data)
 {
@@ -536,6 +639,21 @@ update_kernel_time(void)
 
 }
 
+static void
+clock_path_delay(struct ptpv2_data_slave_ordinary *ptp_data)
+{
+	uint64_t t1_ns, t2_ns, t3_ns, t4_ns;
+	int64_t pd;
+
+	t1_ns = timespec64_to_ns(&ptp_data->tstamp1);
+	t2_ns = timespec64_to_ns(&ptp_data->tstamp2);
+	t3_ns = timespec64_to_ns(&ptp_data->tstamp3);
+	t4_ns = timespec64_to_ns(&ptp_data->tstamp4);
+
+	pd = (t2_ns - t3_ns) + (t4_ns - t1_ns);
+	ptp_data->path_delay = pd / 2;
+}
+
 /*
  * Parse the DELAY_RESP message.
  */
@@ -560,6 +678,9 @@ parse_drsp(struct ptpv2_data_slave_ordinary *ptp_data)
 				((uint64_t)ntohl(rx_tstamp->sec_lsb)) |
 				(((uint64_t)ntohs(rx_tstamp->sec_msb)) << 32);
 
+			if (mode == MODE_PI)
+				clock_path_delay(ptp_data);
+
 			ptp_data->current_ptp_port = ptp_data->portid;
 
 			/* Update kernel time if enabled in app parameters. */
@@ -608,11 +729,14 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
 			break;
 		case FOLLOW_UP:
 			parse_fup(&ptp_data);
+			if (mode == MODE_PI)
+				ptp_adjust_freq(&ptp_data);
 			send_delay_request(&ptp_data);
 			break;
 		case DELAY_RESP:
 			parse_drsp(&ptp_data);
-			ptp_adjust_time(&ptp_data);
+			if (mode == MODE_NONE)
+				ptp_adjust_time(&ptp_data);
 			print_clock_info(&ptp_data);
 			break;
 		default:
@@ -709,7 +833,10 @@ ptp_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	static struct option lgopts[] = { {NULL, 0, 0, 0} };
+	static struct option lgopts[] = {
+		{"controller", 1, 0, 0},
+		{NULL, 0, 0, 0}
+	};
 
 	argvopt = argv;
 
@@ -737,6 +864,11 @@ ptp_parse_args(int argc, char **argv)
 
 			ptp_data.kernel_time_set = ret;
 			break;
+		case 0:
+			if (!strcmp(lgopts[option_index].name, "controller"))
+				if (!strcmp(optarg, "pi"))
+					mode = MODE_PI;
+			break;
 
 		default:
 			print_usage(prgname);
@@ -780,6 +912,15 @@ main(int argc, char *argv[])
 		rte_exit(EXIT_FAILURE, "Error with PTP initialization\n");
 	/* >8 End of parsing specific arguments. */
 
+	if (mode == MODE_PI) {
+		ptp_data.servo = malloc(sizeof(*(ptp_data.servo)));
+		if (!ptp_data.servo)
+			rte_exit(EXIT_FAILURE, "no memory for servo\n");
+
+		ptp_data.servo->drift = 0;
+		ptp_data.servo->count = 0;
+	}
+
 	/* Check that there is an even number of ports to send/receive on. */
 	nb_ports = rte_eth_dev_count_avail();
 
@@ -819,6 +960,9 @@ main(int argc, char *argv[])
 	/* Call lcore_main on the main core only. */
 	lcore_main();
 
+	if (mode == MODE_PI)
+		free(ptp_data.servo);
+
 	/* clean up the EAL */
 	rte_eal_cleanup();
 
-- 
2.9.5

Re: [RFC v3 3/3] examples/ptpclient: add frequency adjustment support

[Ferruh Yigit]

On 5/22/2023 2:23 PM, Simei Su wrote:
> This patch applys PI servo algorithm to leverage frequency adjustment
> API to improve PTP timesync accuracy.
> 
> The command for starting ptpclient with PI algorithm is:
> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1
> --controller=pi
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
> ---
>  examples/ptpclient/ptpclient.c | 178 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 161 insertions(+), 17 deletions(-)
> 

<...>

> +
> +enum controller_mode {
> +	MODE_NONE,
> +	MODE_PI,
> +	MAX_ALL
> +} mode;
> +

Better to have 'mode' variable as 'static', can be good to split enum
and variable declaration.

<...>

> @@ -608,11 +729,14 @@ parse_ptp_frames(uint16_t portid, struct rte_mbuf *m) {
>  			break;
>  		case FOLLOW_UP:
>  			parse_fup(&ptp_data);
> +			if (mode == MODE_PI)
> +				ptp_adjust_freq(&ptp_data);
>  			send_delay_request(&ptp_data);
>  			break;
>  		case DELAY_RESP:
>  			parse_drsp(&ptp_data);
> -			ptp_adjust_time(&ptp_data);
> +			if (mode == MODE_NONE)
> +				ptp_adjust_time(&ptp_data);
>  			print_clock_info(&ptp_data);
>  			break;
>  		default:
>  

Why with FOLLOW_UP PTP message only frequency adjustment done,
and with DELAY_RESP PTP message only time adjustment done?

Is this related to he PTP protocol, or your design decision?

RE: [RFC v3 3/3] examples/ptpclient: add frequency adjustment support

[Su, Simei]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Saturday, June 3, 2023 3:53 AM
> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Subject: Re: [RFC v3 3/3] examples/ptpclient: add frequency adjustment
> support
> 
> On 5/22/2023 2:23 PM, Simei Su wrote:
> > This patch applys PI servo algorithm to leverage frequency adjustment
> > API to improve PTP timesync accuracy.
> >
> > The command for starting ptpclient with PI algorithm is:
> > ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> > 0x1 --controller=pi
> >
> > Signed-off-by: Simei Su <simei.su@intel.com>
> > Signed-off-by: Wenjun Wu <wenjun1.wu@intel.com>
> > ---
> >  examples/ptpclient/ptpclient.c | 178
> > +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 161 insertions(+), 17 deletions(-)
> >
> 
> <...>
> 
> > +
> > +enum controller_mode {
> > +	MODE_NONE,
> > +	MODE_PI,
> > +	MAX_ALL
> > +} mode;
> > +
> 
> Better to have 'mode' variable as 'static', can be good to split enum and
> variable declaration.
> 

OK, got it. Thanks for your reminder.

> <...>
> 
> > @@ -608,11 +729,14 @@ parse_ptp_frames(uint16_t portid, struct
> rte_mbuf *m) {
> >  			break;
> >  		case FOLLOW_UP:
> >  			parse_fup(&ptp_data);
> > +			if (mode == MODE_PI)
> > +				ptp_adjust_freq(&ptp_data);
> >  			send_delay_request(&ptp_data);
> >  			break;
> >  		case DELAY_RESP:
> >  			parse_drsp(&ptp_data);
> > -			ptp_adjust_time(&ptp_data);
> > +			if (mode == MODE_NONE)
> > +				ptp_adjust_time(&ptp_data);
> >  			print_clock_info(&ptp_data);
> >  			break;
> >  		default:
> >
> 
> Why with FOLLOW_UP PTP message only frequency adjustment done, and
> with DELAY_RESP PTP message only time adjustment done?
> 
> Is this related to he PTP protocol, or your design decision?

It's my design. In current design, if it's without PI algorithm, it only involves time adjustment for "DELAY_RESP PTP message" which is original framework.
If it's with PI algorithm, it involves time adjustment or frequency adjustment based on output state in PI algorithm for " FOLLOW_UP PTP message" while
for " DELAY_RESP PTP message", it only needs to print clock info.

Thanks,
Simei

Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync

[Ferruh Yigit]

On 5/22/2023 2:23 PM, Simei Su wrote:
> This patchset cover below parts:
> (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq" that
>    enables frequency adjustment during PTP timesync. This new API aligns with
>    the kernel PTP which already supports frequency adjustment. It brings DPDK
>    closer in alignment with the kernel's best practice.
> 

Hi Simei,

I am not expert on PTP, so please help me understand,

I can see there is a new API to update device frequency by updating
device clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
(Although I have some reservation with the API name, we can visit it later.)

PTP doesn't enforce any specific sync method, right?
Like adjusting by adding/subtracting or adjusting by updating clock
frequency.
As far as I understand updating device clock frequency is HW capability
to adjust time, perhaps in a finer grain, is this correct?

If so, is this HW capability should be exposed to user with a new API?
If this is a generic HW capability, I think it is OK to have single
responsibility API, but if this is a vendor specific feature it should
be behind a generic API.


Or should we hide this capability behind existing
'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
method for synchronization, so this can be transparent the user.
What is the benefit to make user selected explicitly one option or other?



> (2)Refine the ptpclient application by applying a PI algorithm that leverages
>    the new API to further improve timesync accuracy. These changes doesn't break
>    original solution and creates a more accurate solution for DPDK-based high
>    accuracy PTP. We have provided significant improvements for timesync accuracy
>    on e810 and we believe these improvements will also benefit other devices.
> 

What is "PI algorithm"? and what PI stands for?

> The original command for starting ptpclient is:
> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1
> 
> The command with PI algorithm is:
> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p 0x1 -- controller=pi
> 
> [RFC v3 1/3] ethdev: add frequency adjustment API.
> [RFC v3 2/3] examples/ptpclient: refine application.
> [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
> 
> v3:
> * Refine commit log in patch.
> * Add more description for the new API.
> 
> v2:
> * Remove the ice PMD part from the RFC.
> * Add description in cover letter.
> * Refine commit log in patch.
> 
> Simei Su (3):
>   ethdev: add frequency adjustment API
>   examples/ptpclient: refine application
>   examples/ptpclient: add frequency adjustment support
> 
>  examples/ptpclient/ptpclient.c   | 222 +++++++++++++++++++++++++++++++++------
>  lib/ethdev/ethdev_driver.h       |   5 +
>  lib/ethdev/ethdev_trace.h        |   9 ++
>  lib/ethdev/ethdev_trace_points.c |   3 +
>  lib/ethdev/rte_ethdev.c          |  19 ++++
>  lib/ethdev/rte_ethdev.h          |  38 +++++++
>  6 files changed, 265 insertions(+), 31 deletions(-)
> 

RE: [RFC v3 0/3] add frequency adjustment support for PTP timesync

[Su, Simei]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Saturday, June 3, 2023 3:44 AM
> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync
> 
> On 5/22/2023 2:23 PM, Simei Su wrote:
> > This patchset cover below parts:
> > (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq"
> that
> >    enables frequency adjustment during PTP timesync. This new API aligns
> with
> >    the kernel PTP which already supports frequency adjustment. It brings
> DPDK
> >    closer in alignment with the kernel's best practice.
> >
> 
> Hi Simei,
> 
> I am not expert on PTP, so please help me understand,
> 
> I can see there is a new API to update device frequency by updating device
> clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
> (Although I have some reservation with the API name, we can visit it later.)

Yes, this API is to adjust the frequency in a finer granularity, so naming it as 'rte_eth_timesync_adjust_fine()'.

> 
> PTP doesn't enforce any specific sync method, right?
> Like adjusting by adding/subtracting or adjusting by updating clock frequency.
> As far as I understand updating device clock frequency is HW capability to
> adjust time, perhaps in a finer grain, is this correct?

Yes, it's correct.

> 
> If so, is this HW capability should be exposed to user with a new API?
> If this is a generic HW capability, I think it is OK to have single responsibility
> API, but if this is a vendor specific feature it should be behind a generic API.

It's a generic HW capability instead of vendor specific feature.

> 
> 
> Or should we hide this capability behind existing
> 'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
> method for synchronization, so this can be transparent the user.
> What is the benefit to make user selected explicitly one option or other?
> 

In general, 'rte_eth_timesync_adjust_fine()' is used in conjunction with
'rte_eth_timesync_adjust_time()' at different moment to do more precise time control.

> 
> 
> > (2)Refine the ptpclient application by applying a PI algorithm that leverages
> >    the new API to further improve timesync accuracy. These changes
> doesn't break
> >    original solution and creates a more accurate solution for DPDK-based
> high
> >    accuracy PTP. We have provided significant improvements for timesync
> accuracy
> >    on e810 and we believe these improvements will also benefit other
> devices.
> >
> 
> What is "PI algorithm"? and what PI stands for?

Proportional-integral-derivative control is called PID controller. It is widely used in industrial control.
The control deviation is constituted based on the given value and the actual output value.
The deviation produces control quantity based on proportional, integrated and differentiated linear combination, thereby controlling the controlled object.

'P' means proportional controller. It changes the magnitude only.
'I' means integral controller. It lags the output phase.
'PI' means proportional Integral controller. It changes the magnitude as well as laging the output.

Proportional-Integral(PI) controller is used to produce a fractional tick-rate adjustment that coordinates the local clock with 
master clock time. The PI controller corrects both the time and rate of the local clock. The proportional term tracks and 
corrects the direct input, which is the time difference between two clocks. The integral term tracks and corrects steady-state 
error, which is the rate difference between two clocks.

Thanks,
Simei

> 
> > The original command for starting ptpclient is:
> > ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> > 0x1
> >
> > The command with PI algorithm is:
> > ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> > 0x1 -- controller=pi
> >
> > [RFC v3 1/3] ethdev: add frequency adjustment API.
> > [RFC v3 2/3] examples/ptpclient: refine application.
> > [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
> >
> > v3:
> > * Refine commit log in patch.
> > * Add more description for the new API.
> >
> > v2:
> > * Remove the ice PMD part from the RFC.
> > * Add description in cover letter.
> > * Refine commit log in patch.
> >
> > Simei Su (3):
> >   ethdev: add frequency adjustment API
> >   examples/ptpclient: refine application
> >   examples/ptpclient: add frequency adjustment support
> >
> >  examples/ptpclient/ptpclient.c   | 222
> +++++++++++++++++++++++++++++++++------
> >  lib/ethdev/ethdev_driver.h       |   5 +
> >  lib/ethdev/ethdev_trace.h        |   9 ++
> >  lib/ethdev/ethdev_trace_points.c |   3 +
> >  lib/ethdev/rte_ethdev.c          |  19 ++++
> >  lib/ethdev/rte_ethdev.h          |  38 +++++++
> >  6 files changed, 265 insertions(+), 31 deletions(-)
> >

Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync

[Ferruh Yigit]

On 6/7/2023 9:19 AM, Su, Simei wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Saturday, June 3, 2023 3:44 AM
>> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
>> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
>> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
>> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync
>>
>> On 5/22/2023 2:23 PM, Simei Su wrote:
>>> This patchset cover below parts:
>>> (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq"
>> that
>>>    enables frequency adjustment during PTP timesync. This new API aligns
>> with
>>>    the kernel PTP which already supports frequency adjustment. It brings
>> DPDK
>>>    closer in alignment with the kernel's best practice.
>>>
>>
>> Hi Simei,
>>
>> I am not expert on PTP, so please help me understand,
>>
>> I can see there is a new API to update device frequency by updating device
>> clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
>> (Although I have some reservation with the API name, we can visit it later.)
> 
> Yes, this API is to adjust the frequency in a finer granularity, so naming it as 'rte_eth_timesync_adjust_fine()'.
> 

Can existing 'rte_eth_timesync_adjust_time()' API also used to adjust
the device clock frequency?

If so, since 'fine' is subjective, how user can decide to use one or
other, and why we can't use existing API?

>>
>> PTP doesn't enforce any specific sync method, right?
>> Like adjusting by adding/subtracting or adjusting by updating clock frequency.
>> As far as I understand updating device clock frequency is HW capability to
>> adjust time, perhaps in a finer grain, is this correct?
> 
> Yes, it's correct.
> 
>>
>> If so, is this HW capability should be exposed to user with a new API?
>> If this is a generic HW capability, I think it is OK to have single responsibility
>> API, but if this is a vendor specific feature it should be behind a generic API.
> 
> It's a generic HW capability instead of vendor specific feature.
> 
>>
>>
>> Or should we hide this capability behind existing
>> 'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
>> method for synchronization, so this can be transparent the user.
>> What is the benefit to make user selected explicitly one option or other?
>>
> 
> In general, 'rte_eth_timesync_adjust_fine()' is used in conjunction with
> 'rte_eth_timesync_adjust_time()' at different moment to do more precise time control.
> 

If I understand this is not defined by PTP control, so how user can do
distinction between two?

Or same argument again, can user call same existing API and this detail
can be hidden behind implementation? Meaning driver/HW selects that
moment to call one or other.

>>
>>
>>> (2)Refine the ptpclient application by applying a PI algorithm that leverages
>>>    the new API to further improve timesync accuracy. These changes
>> doesn't break
>>>    original solution and creates a more accurate solution for DPDK-based
>> high
>>>    accuracy PTP. We have provided significant improvements for timesync
>> accuracy
>>>    on e810 and we believe these improvements will also benefit other
>> devices.
>>>
>>
>> What is "PI algorithm"? and what PI stands for?
> 
> Proportional-integral-derivative control is called PID controller. It is widely used in industrial control.
> The control deviation is constituted based on the given value and the actual output value.
> The deviation produces control quantity based on proportional, integrated and differentiated linear combination, thereby controlling the controlled object.
> 
> 'P' means proportional controller. It changes the magnitude only.
> 'I' means integral controller. It lags the output phase.
> 'PI' means proportional Integral controller. It changes the magnitude as well as laging the output.
> 
> Proportional-Integral(PI) controller is used to produce a fractional tick-rate adjustment that coordinates the local clock with 
> master clock time. The PI controller corrects both the time and rate of the local clock. The proportional term tracks and 
> corrects the direct input, which is the time difference between two clocks. The integral term tracks and corrects steady-state 
> error, which is the rate difference between two clocks.
> 

Thank you.

> Thanks,
> Simei
> 
>>
>>> The original command for starting ptpclient is:
>>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
>>> 0x1
>>>
>>> The command with PI algorithm is:
>>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
>>> 0x1 -- controller=pi
>>>
>>> [RFC v3 1/3] ethdev: add frequency adjustment API.
>>> [RFC v3 2/3] examples/ptpclient: refine application.
>>> [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
>>>
>>> v3:
>>> * Refine commit log in patch.
>>> * Add more description for the new API.
>>>
>>> v2:
>>> * Remove the ice PMD part from the RFC.
>>> * Add description in cover letter.
>>> * Refine commit log in patch.
>>>
>>> Simei Su (3):
>>>   ethdev: add frequency adjustment API
>>>   examples/ptpclient: refine application
>>>   examples/ptpclient: add frequency adjustment support
>>>
>>>  examples/ptpclient/ptpclient.c   | 222
>> +++++++++++++++++++++++++++++++++------
>>>  lib/ethdev/ethdev_driver.h       |   5 +
>>>  lib/ethdev/ethdev_trace.h        |   9 ++
>>>  lib/ethdev/ethdev_trace_points.c |   3 +
>>>  lib/ethdev/rte_ethdev.c          |  19 ++++
>>>  lib/ethdev/rte_ethdev.h          |  38 +++++++
>>>  6 files changed, 265 insertions(+), 31 deletions(-)
>>>
> 

RE: [RFC v3 0/3] add frequency adjustment support for PTP timesync

[Su, Simei]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, June 8, 2023 2:29 AM
> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP timesync
> 
> On 6/7/2023 9:19 AM, Su, Simei wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Saturday, June 3, 2023 3:44 AM
> >> To: Su, Simei <simei.su@intel.com>; thomas@monjalon.net;
> >> andrew.rybchenko@oktetlabs.ru; Rybalchenko, Kirill
> >> <kirill.rybalchenko@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> >> Cc: dev@dpdk.org; Wu, Wenjun1 <wenjun1.wu@intel.com>
> >> Subject: Re: [RFC v3 0/3] add frequency adjustment support for PTP
> >> timesync
> >>
> >> On 5/22/2023 2:23 PM, Simei Su wrote:
> >>> This patchset cover below parts:
> >>> (1)Introduce a new timesync API called "rte_eth_timesync_adjust_freq"
> >> that
> >>>    enables frequency adjustment during PTP timesync. This new API
> >>> aligns
> >> with
> >>>    the kernel PTP which already supports frequency adjustment. It
> >>> brings
> >> DPDK
> >>>    closer in alignment with the kernel's best practice.
> >>>
> >>
> >> Hi Simei,
> >>
> >> I am not expert on PTP, so please help me understand,
> >>
> >> I can see there is a new API to update device frequency by updating
> >> device clock, 'rte_eth_timesync_adjust_fine()', that part is OK.
> >> (Although I have some reservation with the API name, we can visit it
> >> later.)
> >
> > Yes, this API is to adjust the frequency in a finer granularity, so naming it as
> 'rte_eth_timesync_adjust_fine()'.
> >
> 
> Can existing 'rte_eth_timesync_adjust_time()' API also used to adjust the
> device clock frequency?

' rte_eth_timesync_adjust_time()' API focuses on phase adjustment while
' rte_eth_timesync_adjust_fine()' API focused on frequency adjustment.
They focus on different aspects.

> 
> If so, since 'fine' is subjective, how user can decide to use one or other, and
> why we can't use existing API?

Previously, we only use ' rte_eth_timesync_adjust_time()' API to do time synchronization,
but the accuracy is low. We found the gap maybe missing frequency adjustment
which is already supported in kernel driver and linuxptp application.
So we implement it to bring DPDK closer in alignment with the kernel's best practice and
find the accuracy has a significant improvement.

E.g, below is a simple usage:
if master offset > master offset threshold
   do rte_eth_timesync_adjust_time;
else
   do rte_eth_timesync_adjust_fine;.

> 
> >>
> >> PTP doesn't enforce any specific sync method, right?
> >> Like adjusting by adding/subtracting or adjusting by updating clock
> frequency.
> >> As far as I understand updating device clock frequency is HW
> >> capability to adjust time, perhaps in a finer grain, is this correct?
> >
> > Yes, it's correct.
> >
> >>
> >> If so, is this HW capability should be exposed to user with a new API?
> >> If this is a generic HW capability, I think it is OK to have single
> >> responsibility API, but if this is a vendor specific feature it should be behind
> a generic API.
> >
> > It's a generic HW capability instead of vendor specific feature.
> >
> >>
> >>
> >> Or should we hide this capability behind existing
> >> 'rte_eth_timesync_adjust_time()' API, and HW/driver use best possible
> >> method for synchronization, so this can be transparent the user.
> >> What is the benefit to make user selected explicitly one option or other?
> >>
> >
> > In general, 'rte_eth_timesync_adjust_fine()' is used in conjunction
> > with 'rte_eth_timesync_adjust_time()' at different moment to do more
> precise time control.
> >
> 
> If I understand this is not defined by PTP control, so how user can do
> distinction between two?
> 
> Or same argument again, can user call same existing API and this detail can be
> hidden behind implementation? Meaning driver/HW selects that moment to
> call one or other.

In summary, the new API is introduced because of below consideration:
(1) align with kernel driver and linuxptp application
(2) it's not vendor specific API but generic API
(3) refine current algorithm in ptpclient application to improve time accuracy
(4) the focus is different from ' rte_eth_timesync_adjust_time'

In order not to destroy original framework in ptpclient application and let it can be used by other PMDs which is without frequency adjustment for normal use,
we add --controller=pi in starting command to select using PI algorithm to improve accuracy. Because besides PI algorithm, other linear algorithm can
be used for it.

Thanks,
Simei

> 
> >>
> >>
> >>> (2)Refine the ptpclient application by applying a PI algorithm that
> leverages
> >>>    the new API to further improve timesync accuracy. These changes
> >> doesn't break
> >>>    original solution and creates a more accurate solution for
> >>> DPDK-based
> >> high
> >>>    accuracy PTP. We have provided significant improvements for
> >>> timesync
> >> accuracy
> >>>    on e810 and we believe these improvements will also benefit other
> >> devices.
> >>>
> >>
> >> What is "PI algorithm"? and what PI stands for?
> >
> > Proportional-integral-derivative control is called PID controller. It is widely
> used in industrial control.
> > The control deviation is constituted based on the given value and the actual
> output value.
> > The deviation produces control quantity based on proportional, integrated
> and differentiated linear combination, thereby controlling the controlled
> object.
> >
> > 'P' means proportional controller. It changes the magnitude only.
> > 'I' means integral controller. It lags the output phase.
> > 'PI' means proportional Integral controller. It changes the magnitude as well
> as laging the output.
> >
> > Proportional-Integral(PI) controller is used to produce a fractional
> > tick-rate adjustment that coordinates the local clock with master
> > clock time. The PI controller corrects both the time and rate of the
> > local clock. The proportional term tracks and corrects the direct input,
> which is the time difference between two clocks. The integral term tracks and
> corrects steady-state error, which is the rate difference between two clocks.
> >
> 
> Thank you.
> 
> > Thanks,
> > Simei
> >
> >>
> >>> The original command for starting ptpclient is:
> >>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> >>> 0x1
> >>>
> >>> The command with PI algorithm is:
> >>> ./build/examples/dpdk-ptpclient -a 0000:81:00.0 -c 1 -n 3 -- -T 0 -p
> >>> 0x1 -- controller=pi
> >>>
> >>> [RFC v3 1/3] ethdev: add frequency adjustment API.
> >>> [RFC v3 2/3] examples/ptpclient: refine application.
> >>> [RFC v3 3/3] examples/ptpclient: add frequency adjustment support.
> >>>
> >>> v3:
> >>> * Refine commit log in patch.
> >>> * Add more description for the new API.
> >>>
> >>> v2:
> >>> * Remove the ice PMD part from the RFC.
> >>> * Add description in cover letter.
> >>> * Refine commit log in patch.
> >>>
> >>> Simei Su (3):
> >>>   ethdev: add frequency adjustment API
> >>>   examples/ptpclient: refine application
> >>>   examples/ptpclient: add frequency adjustment support
> >>>
> >>>  examples/ptpclient/ptpclient.c   | 222
> >> +++++++++++++++++++++++++++++++++------
> >>>  lib/ethdev/ethdev_driver.h       |   5 +
> >>>  lib/ethdev/ethdev_trace.h        |   9 ++
> >>>  lib/ethdev/ethdev_trace_points.c |   3 +
> >>>  lib/ethdev/rte_ethdev.c          |  19 ++++
> >>>  lib/ethdev/rte_ethdev.h          |  38 +++++++
> >>>  6 files changed, 265 insertions(+), 31 deletions(-)
> >>>
> >