| Message ID | 20181026162742.631-5-mlichvar@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | More accurate PHC<->system clock synchronization | expand |
> -----Original Message----- > From: Miroslav Lichvar [mailto:mlichvar@redhat.com] > Sent: Friday, October 26, 2018 9:28 AM > To: netdev@vger.kernel.org > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>; > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com> > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c > b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c > index b3e0d8bb5cbd..d31e8d3effc7 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c > @@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp, > struct timespec64 *ts) > return 0; > } > > +/** > + * ixgbe_ptp_gettimex > + * @ptp: the ptp clock structure > + * @sts: structure to hold the system time before reading the PHC, > + * the PHC timestamp, and system time after reading the PHC > + * > + * read the timecounter and return the correct value on ns, > + * after converting it into a struct timespec. > + */ > +static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, > + struct ptp_system_timestamp *sts) > +{ > + struct ixgbe_adapter *adapter = > + container_of(ptp, struct ixgbe_adapter, ptp_caps); > + struct ixgbe_hw *hw = &adapter->hw; > + unsigned long flags; > + struct timespec64 ts; > + u64 ns, stamp; > + > + spin_lock_irqsave(&adapter->tmreg_lock, flags); > + > + switch (adapter->hw.mac.type) { > + case ixgbe_mac_X550: > + case ixgbe_mac_X550EM_x: > + case ixgbe_mac_x550em_a: > + /* Upper 32 bits represent billions of cycles, lower 32 bits > + * represent cycles. However, we use timespec64_to_ns for the > + * correct math even though the units haven't been corrected > + * yet. > + */ > + ptp_read_system_prets(sts); > + IXGBE_READ_REG(hw, IXGBE_SYSTIMR); > + ptp_read_system_postts(sts); > + ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML); > + ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH); > + stamp = timespec64_to_ns(&ts); > + break; > + default: > + ptp_read_system_prets(sts); > + stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML); > + ptp_read_system_postts(sts); > + stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32; > + break; > + } > + > + ns = timecounter_cyc2time(&adapter->hw_tc, stamp); > + > + spin_unlock_irqrestore(&adapter->tmreg_lock, flags); > + > + sts->phc_ts = ns_to_timespec64(ns); > + > + return 0; > +} > + What about replacing gettime64 with: static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts) { struct ptp_system_timestamp sts ixgbe_ptp_gettimex(ptp, &tst); *ts = sts.phc_ts } Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call? Thanks, Jake > /** > * ixgbe_ptp_settime > * @ptp: the ptp clock structure > @@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter > *adapter) > adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599; > adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; > adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; > + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; > adapter->ptp_caps.settime64 = ixgbe_ptp_settime; > adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; > adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540; > @@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter > *adapter) > adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599; > adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; > adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; > + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; > adapter->ptp_caps.settime64 = ixgbe_ptp_settime; > adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; > break; > @@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter > *adapter) > adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550; > adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; > adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; > + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; > adapter->ptp_caps.settime64 = ixgbe_ptp_settime; > adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; > adapter->ptp_setup_sdp = NULL; > -- > 2.17.2
On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote: > > -----Original Message----- > > From: Miroslav Lichvar [mailto:mlichvar@redhat.com] > > Sent: Friday, October 26, 2018 9:28 AM > > To: netdev@vger.kernel.org > > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran <richardcochran@gmail.com>; > > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar <mlichvar@redhat.com> > > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > > > Cc: Richard Cochran <richardcochran@gmail.com> > > Cc: Jacob Keller <jacob.e.keller@intel.com> > > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> > What about replacing gettime64 with: > > static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts) > { > struct ptp_system_timestamp sts > > ixgbe_ptp_gettimex(ptp, &tst); > *ts = sts.phc_ts > } That will work, but it will be slower. With HPET as a clocksource there would be few microseconds of an extra (symmetric) delay and the applications would have to assume a larger maximum error. I think there could be a flag in ptp_system_timestamp, or a parameter of gettimex64(), which would enable/disable reading of the system clock. > Actually, could that even just be provided by the PTP core if gettime64 isn't implemented? This way new drivers only have to implement the new interface, and userspace will just get the old behavior if they use the old call? Good idea. Thanks,
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On > Behalf Of Miroslav Lichvar > Sent: Monday, October 29, 2018 6:31 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richard Cochran > <richardcochran@gmail.com> > Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > On Fri, Oct 26, 2018 at 04:54:57PM +0000, Keller, Jacob E wrote: > > > -----Original Message----- > > > From: Miroslav Lichvar [mailto:mlichvar@redhat.com] > > > Sent: Friday, October 26, 2018 9:28 AM > > > To: netdev@vger.kernel.org > > > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran > <richardcochran@gmail.com>; > > > Keller, Jacob E <jacob.e.keller@intel.com>; Miroslav Lichvar > <mlichvar@redhat.com> > > > Subject: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > > > > > Cc: Richard Cochran <richardcochran@gmail.com> > > > Cc: Jacob Keller <jacob.e.keller@intel.com> > > > Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> > > > What about replacing gettime64 with: > > > > static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, struct timespec64 *ts) > > { > > struct ptp_system_timestamp sts > > > > ixgbe_ptp_gettimex(ptp, &tst); > > *ts = sts.phc_ts > > } > > That will work, but it will be slower. With HPET as a clocksource > there would be few microseconds of an extra (symmetric) delay and the > applications would have to assume a larger maximum error. > Right. My intention for this would be that we'd switch from gettime64 to gettime64x going forward, and provide this as a way to avoid having to duplicate logic in drivers while we're transitioning? Thus, new applications should be using the new call if it's available in the driver. Hmm. > I think there could be a flag in ptp_system_timestamp, or a parameter > of gettimex64(), which would enable/disable reading of the system > clock. > > > Actually, could that even just be provided by the PTP core if gettime64 isn't > implemented? This way new drivers only have to implement the new interface, and > userspace will just get the old behavior if they use the old call? > > Good idea. Ideally we can find a way that minimizes the overhead for the old call. Thanks, Jake > > Thanks, > > -- > Miroslav Lichvar
On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote: > I think there could be a flag in ptp_system_timestamp, or a parameter > of gettimex64(), which would enable/disable reading of the system > clock. I'm not a fan of functions that change their behavior based on flags in their input parameters. Thanks, Richard
On Wed, Oct 31, 2018 at 07:40:03AM -0700, Richard Cochran wrote: > On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote: > > I think there could be a flag in ptp_system_timestamp, or a parameter > > of gettimex64(), which would enable/disable reading of the system > > clock. > > I'm not a fan of functions that change their behavior based on flags > in their input parameters. How about separating the PHC timestamp from the ptp_system_timestamp structure and use NULL to indicate we don't want to read the system clock? A gettimex64(ptp, ts, NULL) call would be equal to gettime64(ptp, ts). struct ptp_system_timestamp { struct timespec64 pre_ts; struct timespec64 post_ts; }; int (*gettimex64)(struct ptp_clock_info *ptp, struct timespec64 *ts, struct ptp_system_timestamp *sts);
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Wednesday, October 31, 2018 7:40 AM > To: Miroslav Lichvar <mlichvar@redhat.com> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; intel-wired- > lan@lists.osuosl.org > Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > On Mon, Oct 29, 2018 at 02:31:09PM +0100, Miroslav Lichvar wrote: > > I think there could be a flag in ptp_system_timestamp, or a parameter > > of gettimex64(), which would enable/disable reading of the system > > clock. > > I'm not a fan of functions that change their behavior based on flags > in their input parameters. > > Thanks, > Richard Neither am I. I do however want to find a solution that avoids having drivers needlessly duplicate almost the same functionality. Thanks, Jake
On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote: > > How about separating the PHC timestamp from the ptp_system_timestamp > structure and use NULL to indicate we don't want to read the system > clock? A gettimex64(ptp, ts, NULL) call would be equal to > gettime64(ptp, ts). Doesn't sound too bad to me. Thanks, Richard
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Wednesday, October 31, 2018 2:17 PM > To: Miroslav Lichvar <mlichvar@redhat.com> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; intel-wired- > lan@lists.osuosl.org > Subject: Re: [RFC PATCH 4/4] ixgbe: add support for extended PHC gettime > > On Wed, Oct 31, 2018 at 03:49:35PM +0100, Miroslav Lichvar wrote: > > > > How about separating the PHC timestamp from the ptp_system_timestamp > > structure and use NULL to indicate we don't want to read the system > > clock? A gettimex64(ptp, ts, NULL) call would be equal to > > gettime64(ptp, ts). > > Doesn't sound too bad to me. > > Thanks, > Richard Yep, this seems fine to me as well. Regards, Jake
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c index b3e0d8bb5cbd..d31e8d3effc7 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c @@ -466,6 +466,60 @@ static int ixgbe_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts) return 0; } +/** + * ixgbe_ptp_gettimex + * @ptp: the ptp clock structure + * @sts: structure to hold the system time before reading the PHC, + * the PHC timestamp, and system time after reading the PHC + * + * read the timecounter and return the correct value on ns, + * after converting it into a struct timespec. + */ +static int ixgbe_ptp_gettimex(struct ptp_clock_info *ptp, + struct ptp_system_timestamp *sts) +{ + struct ixgbe_adapter *adapter = + container_of(ptp, struct ixgbe_adapter, ptp_caps); + struct ixgbe_hw *hw = &adapter->hw; + unsigned long flags; + struct timespec64 ts; + u64 ns, stamp; + + spin_lock_irqsave(&adapter->tmreg_lock, flags); + + switch (adapter->hw.mac.type) { + case ixgbe_mac_X550: + case ixgbe_mac_X550EM_x: + case ixgbe_mac_x550em_a: + /* Upper 32 bits represent billions of cycles, lower 32 bits + * represent cycles. However, we use timespec64_to_ns for the + * correct math even though the units haven't been corrected + * yet. + */ + ptp_read_system_prets(sts); + IXGBE_READ_REG(hw, IXGBE_SYSTIMR); + ptp_read_system_postts(sts); + ts.tv_nsec = IXGBE_READ_REG(hw, IXGBE_SYSTIML); + ts.tv_sec = IXGBE_READ_REG(hw, IXGBE_SYSTIMH); + stamp = timespec64_to_ns(&ts); + break; + default: + ptp_read_system_prets(sts); + stamp = IXGBE_READ_REG(hw, IXGBE_SYSTIML); + ptp_read_system_postts(sts); + stamp |= (u64)IXGBE_READ_REG(hw, IXGBE_SYSTIMH) << 32; + break; + } + + ns = timecounter_cyc2time(&adapter->hw_tc, stamp); + + spin_unlock_irqrestore(&adapter->tmreg_lock, flags); + + sts->phc_ts = ns_to_timespec64(ns); + + return 0; +} + /** * ixgbe_ptp_settime * @ptp: the ptp clock structure @@ -1217,6 +1271,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter) adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599; adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; adapter->ptp_caps.settime64 = ixgbe_ptp_settime; adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; adapter->ptp_setup_sdp = ixgbe_ptp_setup_sdp_x540; @@ -1234,6 +1289,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter) adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_82599; adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; adapter->ptp_caps.settime64 = ixgbe_ptp_settime; adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; break; @@ -1250,6 +1306,7 @@ static long ixgbe_ptp_create_clock(struct ixgbe_adapter *adapter) adapter->ptp_caps.adjfreq = ixgbe_ptp_adjfreq_X550; adapter->ptp_caps.adjtime = ixgbe_ptp_adjtime; adapter->ptp_caps.gettime64 = ixgbe_ptp_gettime; + adapter->ptp_caps.gettimex64 = ixgbe_ptp_gettimex; adapter->ptp_caps.settime64 = ixgbe_ptp_settime; adapter->ptp_caps.enable = ixgbe_ptp_feature_enable; adapter->ptp_setup_sdp = NULL;
Cc: Richard Cochran <richardcochran@gmail.com> Cc: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 57 ++++++++++++++++++++ 1 file changed, 57 insertions(+)