[dpdk-dev,v5,1/7] ethdev: add additional ieee1588 support functions
Commit Message
Add additional functions to support the existing IEEE1588
functionality.
* rte_eth_timesync_settime(), function to set the device clock time.
* rte_eth_timesync_gettime, function to get the device clock time.
* rte_eth_timesync_adjust, function to adjust the device clock time.
Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
doc/guides/rel_notes/release_2_2.rst | 3 ++
lib/librte_ether/rte_ethdev.c | 36 +++++++++++++++++++
lib/librte_ether/rte_ethdev.h | 65 +++++++++++++++++++++++++++++++++-
lib/librte_ether/rte_ether_version.map | 3 ++
4 files changed, 106 insertions(+), 1 deletion(-)
Comments
Hi,
Sorry for not having followed closer this series.
It was submitted at the last minute and got too few comments.
I'll try to fix it now to be sure it will be one of the first series
ready for the 2.3 cycle.
2015-11-05 15:06, Daniel Mrzyglod:
> --- a/doc/guides/rel_notes/release_2_2.rst
> +++ b/doc/guides/rel_notes/release_2_2.rst
> @@ -222,6 +222,9 @@ API Changes
>
> * The devargs union field virtual is renamed to virt for C++ compatibility.
>
> +* Add new functions in ethdev to support IEEE1588: rte_eth_timesync_time_adjust()
> + rte_eth_timesync_time_get(), rte_eth_timesync_time_set()
No need to add an entry in API changes for new functions.
> +/**
> + * Read the time from the timesync clock on an Ethernet device.
> + *
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @param time
> + * Pointer to the timespec struct.
> + *
> + * @return
> + * - 0: Success.
> + */
> +extern int rte_eth_timesync_time_get(uint8_t port_id,
> + struct timespec *time);
How is it different from rte_eth_timesync_read_rx_timestamp() and
rte_eth_timesync_read_tx_timestamp()?
Why repeating the word time? Why not rte_eth_timesync_get()?
Not related to this patch, but in rte_eth_timesync_read_rx_timestamp(),
the flags parameter breaks the API layer separation with drivers:
* @param flags
* Device specific flags. Used to pass the RX timesync register index to
* i40e. Unused in igb/ixgbe, pass 0 instead.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 10, 2015 11:04 AM
> To: Mrzyglod, DanielX T
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> support functions
>
> Hi,
>
> Sorry for not having followed closer this series.
> It was submitted at the last minute and got too few comments.
Hi,
The V2 version was submitted on the last day of the merge window but the API was available in the V1. Comments could have been made then.
> I'll try to fix it now to be sure it will be one of the first series ready
> for the 2.3 cycle.
These comments are minor and could be fixed now.
>
> 2015-11-05 15:06, Daniel Mrzyglod:
> > --- a/doc/guides/rel_notes/release_2_2.rst
> > +++ b/doc/guides/rel_notes/release_2_2.rst
> > @@ -222,6 +222,9 @@ API Changes
> >
> > * The devargs union field virtual is renamed to virt for C++
> compatibility.
> >
> > +* Add new functions in ethdev to support IEEE1588:
> > +rte_eth_timesync_time_adjust()
> > + rte_eth_timesync_time_get(), rte_eth_timesync_time_set()
>
> No need to add an entry in API changes for new functions.
OK. We can remove it now or I can remove it in the final release note edit.
>
> > +/**
> > + * Read the time from the timesync clock on an Ethernet device.
> > + *
> > + * @param port_id
> > + * The port identifier of the Ethernet device.
> > + * @param time
> > + * Pointer to the timespec struct.
> > + *
> > + * @return
> > + * - 0: Success.
> > + */
> > +extern int rte_eth_timesync_time_get(uint8_t port_id,
> > + struct timespec *time);
>
> How is it different from rte_eth_timesync_read_rx_timestamp() and
> rte_eth_timesync_read_tx_timestamp()?
>
> Why repeating the word time? Why not rte_eth_timesync_get()?
In the context of PTP there is a difference between the time (os or NIC) and the timestamp (either in the mbuf, a register or as part of the payload).
> Not related to this patch, but in rte_eth_timesync_read_rx_timestamp(),
> the flags parameter breaks the API layer separation with drivers:
> * @param flags
> * Device specific flags. Used to pass the RX timesync register index to
> * i40e. Unused in igb/ixgbe, pass 0 instead.
Yes. Unfortunately i40e has 4 RX registers and this information needs to be passed down to the function is some way. I'd imagine that some other nics might need similar config. I asked for feedback on this in the 2.1 cycle from other nic maintainers but didn't get any. The kernel uses a struct hwtstamp_config that I was initially going to use but didn't.
John
2015-11-10 11:36, Mcnamara, John:
> From: Thomas Monjalon
> > I'll try to fix it now to be sure it will be one of the first series ready
> > for the 2.3 cycle.
>
> These comments are minor and could be fixed now.
After having a closer look in the drivers change, it seems to be restricted to
the PTP functions of the Intel drivers.
So you can ask to the Intel validation team if they are OK to add it in RC2.
I think it would be a wrong idea because we need to stop moving the ethdev and
drivers code, and focus on other DPDK areas for the RC2.
> > > +extern int rte_eth_timesync_time_get(uint8_t port_id,
> > > + struct timespec *time);
> >
> > How is it different from rte_eth_timesync_read_rx_timestamp() and
> > rte_eth_timesync_read_tx_timestamp()?
> >
> > Why repeating the word time? Why not rte_eth_timesync_get()?
>
> In the context of PTP there is a difference between the time (os or NIC) and the timestamp (either in the mbuf, a register or as part of the payload).
Do you think we can make it clear in the definition of these functions?
More wording comments:
- rte_eth_timesync_time_get
- rte_eth_timesync_read_rx_timestamp
Why is it "get" in a case and "read" in another?
Why the verb is at the end in the first and before the complement in the latter?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, November 10, 2015 11:58 AM
> To: Mcnamara, John
> Cc: Mrzyglod, DanielX T; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> support functions
>
> 2015-11-10 11:36, Mcnamara, John:
> > From: Thomas Monjalon
> > > I'll try to fix it now to be sure it will be one of the first series
> > > ready for the 2.3 cycle.
> >
> > These comments are minor and could be fixed now.
>
> After having a closer look in the drivers change, it seems to be
> restricted to the PTP functions of the Intel drivers.
> So you can ask to the Intel validation team if they are OK to add it in
> RC2.
> I think it would be a wrong idea because we need to stop moving the ethdev
> and drivers code, and focus on other DPDK areas for the RC2.
Hi Thomas,
Ok. I'll ask the validation team to evaluate the effect of the patches.
>
> > > > +extern int rte_eth_timesync_time_get(uint8_t port_id,
> > > > + struct timespec *time);
> > >
> > > How is it different from rte_eth_timesync_read_rx_timestamp() and
> > > rte_eth_timesync_read_tx_timestamp()?
> > >
> > > Why repeating the word time? Why not rte_eth_timesync_get()?
> >
> > In the context of PTP there is a difference between the time (os or NIC)
> and the timestamp (either in the mbuf, a register or as part of the
> payload).
>
> Do you think we can make it clear in the definition of these functions?
Yes. I think that could be made clearer. I'll fix that and the others.
>
> More wording comments:
> - rte_eth_timesync_time_get
> - rte_eth_timesync_read_rx_timestamp
> Why is it "get" in a case and "read" in another?
> Why the verb is at the end in the first and before the complement in the
> latter?
My preference would be for verb_noun but I think noun_verb was used for consistency with the rest of the Ethdev API (although that isn't quite consistent either). And yes, read() would be more consistent with the timesync API while get() is more consistent with the rest of the Ethdev API. I think it would be best, in this case, to try maintain self-consistency and use rte_eth_timesync_read_time().
John.
--
2015-11-10 14:12, Mcnamara, John:
> My preference would be for verb_noun but I think noun_verb was used for consistency with the rest of the Ethdev API (although that isn't quite consistent either). And yes, read() would be more consistent with the timesync API while get() is more consistent with the rest of the Ethdev API. I think it would be best, in this case, to try maintain self-consistency and use rte_eth_timesync_read_time().
Yes, self consistency seems a good option.
Hi Thomas& John,
Some update from validation team.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mcnamara, John
> Sent: Tuesday, November 10, 2015 10:12 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> support functions
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, November 10, 2015 11:58 AM
> > To: Mcnamara, John
> > Cc: Mrzyglod, DanielX T; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> > support functions
> >
> > 2015-11-10 11:36, Mcnamara, John:
> > > From: Thomas Monjalon
> > > > I'll try to fix it now to be sure it will be one of the first series
> > > > ready for the 2.3 cycle.
> > >
> > > These comments are minor and could be fixed now.
> >
> > After having a closer look in the drivers change, it seems to be
> > restricted to the PTP functions of the Intel drivers.
> > So you can ask to the Intel validation team if they are OK to add it in
> > RC2.
> > I think it would be a wrong idea because we need to stop moving the
> ethdev
> > and drivers code, and focus on other DPDK areas for the RC2.
>
> Hi Thomas,
>
> Ok. I'll ask the validation team to evaluate the effect of the patches.
>
>
For our validation team's view, in this patch set implemented some APIs which used to support Precision Time Protocol.
The sample based those APIs can work as real ptp client. We have verified it work fine with linux ptp server.
Hi Thomas,
Marvin has already tested the patches with a recent HEAD release of DPDK and are ready to test them as part of RC2. As you say the changes are localized to the Intel drivers and from our review they look like low risk of breaking anything else.
Thanks
Waterman
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liu, Yong
Sent: Tuesday, November 10, 2015 11:18 PM
To: Mcnamara, John; Thomas Monjalon
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588 support functions
Hi Thomas& John,
Some update from validation team.
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mcnamara, John
> Sent: Tuesday, November 10, 2015 10:12 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> support functions
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, November 10, 2015 11:58 AM
> > To: Mcnamara, John
> > Cc: Mrzyglod, DanielX T; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional
> > ieee1588 support functions
> >
> > 2015-11-10 11:36, Mcnamara, John:
> > > From: Thomas Monjalon
> > > > I'll try to fix it now to be sure it will be one of the first
> > > > series ready for the 2.3 cycle.
> > >
> > > These comments are minor and could be fixed now.
> >
> > After having a closer look in the drivers change, it seems to be
> > restricted to the PTP functions of the Intel drivers.
> > So you can ask to the Intel validation team if they are OK to add it
> > in RC2.
> > I think it would be a wrong idea because we need to stop moving the
> ethdev
> > and drivers code, and focus on other DPDK areas for the RC2.
>
> Hi Thomas,
>
> Ok. I'll ask the validation team to evaluate the effect of the patches.
>
>
For our validation team's view, in this patch set implemented some APIs which used to support Precision Time Protocol.
The sample based those APIs can work as real ptp client. We have verified it work fine with linux ptp server.
@@ -222,6 +222,9 @@ API Changes
* The devargs union field virtual is renamed to virt for C++ compatibility.
+* Add new functions in ethdev to support IEEE1588: rte_eth_timesync_time_adjust()
+ rte_eth_timesync_time_get(), rte_eth_timesync_time_set()
+
ABI Changes
-----------
@@ -3193,6 +3193,42 @@ rte_eth_timesync_read_tx_timestamp(uint8_t port_id, struct timespec *timestamp)
}
int
+rte_eth_timesync_time_adjust(uint8_t port_id, int64_t delta)
+{
+ struct rte_eth_dev *dev;
+
+ VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_time_adjust, -ENOTSUP);
+ return (*dev->dev_ops->timesync_time_adjust)(dev, delta);
+}
+
+int
+rte_eth_timesync_time_get(uint8_t port_id, struct timespec *timestamp)
+{
+ struct rte_eth_dev *dev;
+
+ VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_time_get, -ENOTSUP);
+ return (*dev->dev_ops->timesync_time_get)(dev, timestamp);
+}
+
+int
+rte_eth_timesync_time_set(uint8_t port_id, struct timespec *timestamp)
+{
+ struct rte_eth_dev *dev;
+
+ VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ FUNC_PTR_OR_ERR_RET(*dev->dev_ops->timesync_time_set, -ENOTSUP);
+ return (*dev->dev_ops->timesync_time_set)(dev, timestamp);
+}
+
+int
rte_eth_dev_get_reg_length(uint8_t port_id)
{
struct rte_eth_dev *dev;
@@ -1206,6 +1206,17 @@ typedef int (*eth_timesync_read_tx_timestamp_t)(struct rte_eth_dev *dev,
struct timespec *timestamp);
/**< @internal Function used to read a TX IEEE1588/802.1AS timestamp. */
+typedef int (*eth_timesync_time_adjust)(struct rte_eth_dev *dev, int64_t);
+/**< @internal Function used to adjust device clock */
+
+typedef int (*eth_timesync_time_get)(struct rte_eth_dev *dev,
+ struct timespec *timestamp);
+/**< @internal Function used to get time from device clock. */
+
+typedef int (*eth_timesync_time_set)(struct rte_eth_dev *dev,
+ struct timespec *timestamp);
+/**< @internal Function used to get time from device clock */
+
typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
/**< @internal Retrieve device register count */
@@ -1400,6 +1411,12 @@ struct eth_dev_ops {
/** Get DCB information */
eth_get_dcb_info get_dcb_info;
+ /** Adjust the device clock */
+ eth_timesync_time_adjust timesync_time_adjust;
+ /** Get the device clock timespec */
+ eth_timesync_time_get timesync_time_get;
+ /** Set the device clock timespec */
+ eth_timesync_time_set timesync_time_set;
};
/**
@@ -3755,6 +3772,53 @@ extern int rte_eth_timesync_read_tx_timestamp(uint8_t port_id,
struct timespec *timestamp);
/**
+ * Adjust the timesync clock on an Ethernet device..
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param delta
+ * The adjustment in nanoseconds
+ *
+ * @return
+ * - 0: Success.
+ * - -ENODEV: The port ID is invalid.
+ * - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+extern int rte_eth_timesync_time_adjust(uint8_t port_id, int64_t delta);
+
+/**
+ * Read the time from the timesync clock on an Ethernet device.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param time
+ * Pointer to the timespec struct.
+ *
+ * @return
+ * - 0: Success.
+ */
+extern int rte_eth_timesync_time_get(uint8_t port_id,
+ struct timespec *time);
+
+
+/**
+ * Set the time of the timesync clock on an Ethernet device.
+ *
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @param time
+ * Pointer to the timespec struct.
+ *
+ * @return
+ * - 0: Success.
+ * - -EINVAL: No timestamp is available.
+ * - -ENODEV: The port ID is invalid.
+ * - -ENOTSUP: The function is not supported by the Ethernet driver.
+ */
+extern int rte_eth_timesync_time_set(uint8_t port_id,
+ struct timespec *time);
+
+/**
* Copy pci device info to the Ethernet device data.
*
* @param eth_dev
@@ -3767,7 +3831,6 @@ extern int rte_eth_timesync_read_tx_timestamp(uint8_t port_id,
*/
extern void rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev, struct rte_pci_device *pci_dev);
-
#ifdef __cplusplus
}
#endif
@@ -135,5 +135,8 @@ DPDK_2.2 {
rte_eth_dev_get_dcb_info;
rte_eth_rx_queue_info_get;
rte_eth_tx_queue_info_get;
+ rte_eth_timesync_time_adjust;
+ rte_eth_timesync_time_get;
+ rte_eth_timesync_time_set;
} DPDK_2.1;