> -----Original Message-----
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Monday, August 10, 2015 1:50 AM
> To: Hall, Christopher S
> Cc: john.stu...@linaro.org; t...@linutronix.de; mi...@redhat.com; Kirsher,
> Jeffrey T; Ronciak, John; h...@zytor.com; x...@kernel.org; linux-
> ker...@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] Added getsynctime64() callback
>
> On Fri, Aug 07, 2015 at 04:01:35PM -0700, Christopher Hall wrote:
> > --- a/drivers/net/ethernet/intel/e1000e/defines.h
> > +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> > @@ -527,6 +527,13 @@
> > #define E1000_RXCW_C 0x20000000 /* Receive config */
> > #define E1000_RXCW_SYNCH 0x40000000 /* Receive config synch
> */
> >
> > +/* HH Time Sync */
> > +#define E1000_TSYNCTXCTL_MAX_ALLOWED_DLY_MASK 0x0000F000 /* max
> delay */
> > +#define E1000_TSYNCTXCTL_SYNC_COMP 0x40000000 /* sync
> complete
> > + */
> > +#define E1000_TSYNCTXCTL_START_SYNC 0x80000000 /*
> initiate sync
> > + */
>
> Split comment looks bad. Trim this leading space instead. ^^^^^^
OK.
>
> > @@ -98,6 +100,91 @@ static int e1000e_phc_adjtime(struct ptp_clock_info
> *ptp, s64 delta)
> > return 0;
> > }
> >
> > +#define HW_WAIT_COUNT (2)
> > +#define HW_RETRY_COUNT (2)
>
> A busy wait, plus a retry, ...
>
> > +#define SYNCTIME_RETRY_COUNT (2)
>
> plus another retry!
>
> Seems a bit heavy handed to me. Is the HW really that flakey?
>
> I would expect that a reasonably long polling loop should be
> sufficient. If not, then the HW ignores certain requests, and that is
> worth a comment.
>
> In any case, I don't understand why you have two nested retry loops.
The retry in get_synctime() is a left over from the previous patch. It's not
necessary,
the current timekeeping code won't fail in a way necessitating a retry. It's
removed.
The inner retry loop is due to huge inaccuracies in udelay(). I've done some
testing
and it appears udelay(2) actually results in about an 8 microsecond delay. On
Skylake the time for completion of the cross timestamp should be about 2
microseconds.
If we eliminate the inner most loop we either spin for too long or possibly
risk not
waiting long enough. Are there any guarantees for udelay()?
As for HW_RETRY_LOOP, I will confirm whether this is necessary. It was in
reference
code I was given, but, I agree, it seems odd.
>
> > +static int e1000e_phc_getsynctime(struct ptp_clock_info *ptp,
> > + struct timespec64 *devts,
> > + struct timespec64 *systs)
> > +{
> > + struct e1000_adapter *adapter = container_of(ptp, struct
> e1000_adapter,
> > + ptp_clock_info);
> > + unsigned long flags;
> > + u32 remainder;
> > + struct correlated_ts art_correlated_ts;
> > + u64 device_time;
> > + int i, ret;
> > +
> > + if (!cpu_has_art)
> > + return -EOPNOTSUPP;
>
> Perform this check before registration, setting .getsynctime64
> accordingly.
The problem here is that ART initialization doesn't happen until we install TSC
as a clocksource. This design is per Thomas' suggestion. That occurs after
the driver is loaded (as a module).
In my somewhat limited testing, it's about 400 ms later. The problem is the
several seconds of TSC frequency refinement. I, in principle, agree, but we
either need to move the ART initialization earlier (probably split it) or defer
PTP clock initialization in the driver.
>
> Thanks,
> Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
