[dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588 support functions

Mcnamara, John john.mcnamara at intel.com
Tue Nov 10 12:36:18 CET 2015



> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 10, 2015 11:04 AM
> To: Mrzyglod, DanielX T
> Cc: dev at 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


More information about the dev mailing list