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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/10/2024 10:13 AM, Mingjin Ye wrote:
> This patch adds freq adjustment API for PTP high accuracy.
> 
> Signed-off-by: Simei Su <simei.su@intel.com>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

<...>

> diff --git a/doc/guides/rel_notes/release_24_11.rst b/doc/guides/rel_notes/release_24_11.rst
> index 0ff70d9057..10ac35e5c0 100644
> --- a/doc/guides/rel_notes/release_24_11.rst
> +++ b/doc/guides/rel_notes/release_24_11.rst
> @@ -25,35 +25,11 @@ New Features
>  ------------
>  
>  .. This section should contain new features added in this release.
> -   Sample format:
>  
> -   * **Add a title in the past tense with a full stop.**
> +* **Added Ethernet device clock incremental rate adjustment.**
>  
> -     Add a short 1-2 sentence description in the past tense.
> -     The description should be enough to allow someone scanning
> -     the release notes to understand the new feature.
> -
> -     If the feature adds a lot of sub-features you can use a bullet list
> -     like this:
> -
> -     * Added feature foo to do something.
> -     * Enhanced feature bar to do something else.
> -
> -     Refer to the previous release notes for examples.
> -
> -     Suggested order in release notes items:
> -     * Core libs (EAL, mempool, ring, mbuf, buses)
> -     * Device abstraction libs and PMDs (ordered alphabetically by vendor name)
> -       - ethdev (lib, PMDs)
> -       - cryptodev (lib, PMDs)
> -       - eventdev (lib, PMDs)
> -       - etc
> -     * Other libs
> -     * Apps, Examples, Tools (if significant)
> -
> -     This section is a comment. Do not overwrite or remove it.
> -     Also, make sure to start the actual text at the margin.
> -     =======================================================
> +  Added new function ``rte_eth_timesync_adjust_freq`` to
> +  adjust the clock increment rate for Ethernet devices.
> 

Can you please check above, patch removes unrelated parts of the
document, I assume caused by rebase issues.

>  
>  Removed Items
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 883e59a927..68cadc14a5 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -664,6 +664,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);
> @@ -1378,6 +1381,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 3bec87bfdb..e273d5853e 100644
> --- a/lib/ethdev/ethdev_trace.h
> +++ b/lib/ethdev/ethdev_trace.h
> @@ -2183,6 +2183,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 99e04f5893..a99fec0c1e 100644
> --- a/lib/ethdev/ethdev_trace_points.c
> +++ b/lib/ethdev/ethdev_trace_points.c
> @@ -409,6 +409,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 f1c658f49e..660eab2f1e 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6310,6 +6310,24 @@ rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta)
>  	return ret;
>  }
>  
> +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)
>  {
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 548fada1c7..c3587089b9 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5297,6 +5297,26 @@ 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
> + *
>

Can you please describe the API a little bit more?

I assume frequency can be adjusted up and down, how this reflected via API?
Can you please give some practical examples of how this API intended to use?
I assume after an initial sync, some time later the externally received
time information and device time mismatch and requires adjustment. In
this case:
1. How to decide which one of the API use,
'rte_eth_timesync_adjust_time()' and this new
'rte_eth_timesync_adjust_freq()' ?
2. Lets say device is behind 1us after 1 minute (is these scale of the
measurements makes sense, if not please give better examples), what
should be the 'ppm' parameter to fix this? And similar sample for the
case device time is faster than external case.