Re: [PATCH v4 2/3] net/ice: add frequency adjustment support for PTP

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

On Thu, Oct 10, 2024 at 09:32:20AM +0000, Mingjin Ye wrote:
> 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>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---

Hi Simei, Mingjin,

some review comments inline below.

thanks,
/Bruce

>  doc/guides/nics/ice.rst      |  15 +++
>  drivers/net/ice/ice_ethdev.c | 177 ++++++++++++++++++++++++++---------
>  drivers/net/ice/ice_ethdev.h |   2 +
>  drivers/net/ice/ice_rxtx.c   |   4 +-
>  4 files changed, 153 insertions(+), 45 deletions(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..51fab743ef 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -328,6 +328,21 @@ Forward Error Correction (FEC)
>  
>  Supports get/set FEC mode and get FEC capability.
>  
> +Time Synchronisation
> +~~~~~~~~~~~~~~~~~~~~
> +
> +The system operator can run a PTP (Precision Time Protocol) client application
> +to synchronise the time on the network card (and optionally the time on the
> +system) to the PTP master.
> +
> +ICE PMD supports PTP client applications that use the DPDK IEEE1588 API to
> +communicate with the PTP master clock. Note that the PTP client application
> +needs to run on the PF and vector mode needs to be disabled.
> +
> +.. code-block:: console
> +
> +    examples/dpdk-ptpclient -c f -n 3 -a 0000:ec:00.1 -- -T 1 -p 0x1 -c 1
> +

Couple of questions about this documentation:

* After running the example given, does the time on the network card remain
  synchronised when the user runs their own app? If so, this text is ok,
  but, if not,  I think we better clarify that the PTP client functionality
  can be integrated into their own app.
* I think we need more detail on "vector mode needs to be disabled". Does
  this happen automatically when we go to use PTP, or does the app or user
  need to deal with it? For example, does the dpdk-ptpclient app disable
  vector mode itself? If not, we should include in the command the
  "force-max-simd-bitwidth=64" parameter to disable vector mode.

>  Generic Flow Support
>  ~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 7b1bd163a2..99d39849c4 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -10,6 +10,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> +#include <math.h>
>  
>  #include <rte_tailq.h>
>  #include <rte_os_shim.h>
> @@ -176,6 +177,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,
> @@ -307,6 +309,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,
> @@ -2332,6 +2335,34 @@ ice_get_supported_rxdid(struct ice_hw *hw)
>  	return supported_rxdid;
>  }
>  
> +static void ice_ptp_init_info(struct rte_eth_dev *dev)
> +{
> +	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);
> +
> +	switch (hw->phy_model) {
> +	case ICE_PHY_ETH56G:
> +		ad->ptp_tx_block = hw->pf_id;
> +		ad->ptp_tx_index = 0;
> +		break;
> +	case ICE_PHY_E810:
> +	/* fallthrough */

I don't believe this fallthrough is necessary. It should only be needed
when we have fallthrough after some code. Just having multiple case labels
is fine without annotation.

> +	case ICE_PHY_E830:
> +		ad->ptp_tx_block = hw->port_info->lport;
> +		ad->ptp_tx_index = 0;
> +		break;
> +	case ICE_PHY_E822:
> +		ad->ptp_tx_block = hw->pf_id / ICE_PORTS_PER_QUAD;
> +		ad->ptp_tx_index = (hw->pf_id % ICE_PORTS_PER_QUAD) *
> +				ICE_PORTS_PER_PHY_E822 * ICE_QUADS_PER_PHY_E822;
> +		break;
> +	default:
> +		PMD_DRV_LOG(WARNING, "Unsupported PHY model");
> +		break;
> +	}
> +}
> +
>  static int
>  ice_dev_init(struct rte_eth_dev *dev)
>  {
> @@ -2499,6 +2530,9 @@ ice_dev_init(struct rte_eth_dev *dev)
>  	/* Initialize PHY model */
>  	ice_ptp_init_phy_model(hw);
>  
> +	/* Initialize PTP info */
> +	ice_ptp_init_info(dev);
> +
>  	if (hw->phy_model == ICE_PHY_E822) {
>  		ret = ice_start_phy_timer_e822(hw, hw->pf_id);
>  		if (ret)
> @@ -6466,23 +6500,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;
> -

I see lots of removals of ad->systime_tc from the code in the diff  Are
there any references to that left in the code? If not, please remove the
variable from the adapter structure. Same with the other values below.

> -	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;
> @@ -6497,14 +6514,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;
>  
<snip>