RE: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event

From: "Machnikowski, Maciej" <maciej.machnikowski@intel.com>
To: Richard Cochran <richardcochran@gmail.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kernel-team@fb.com" <kernel-team@fb.com>
Subject: RE: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
Date: Wed, 30 Jun 2021 15:55:03 +0000	[thread overview]
Message-ID: <PH0PR11MB495167E58F24332D30517809EA019@PH0PR11MB4951.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210630144257.GA30627@hoboy.vegasvil.org>

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: Wednesday, June 30, 2021 4:43 PM
> To: Jonathan Lemon <jonathan.lemon@gmail.com>
> Cc: netdev@vger.kernel.org; kernel-team@fb.com
> Subject: Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
> 
> On Tue, Jun 29, 2021 at 08:50:31PM -0700, Jonathan Lemon wrote:
> > The PHC should be sync'd to the PPS coming from the GPS signal.
> > However, the GPS may be in holdover, so the actual counter comes from
> > an atomic oscillator.  As the oscillator may be ever so slightly out
> > of sync with the GPS (or drifts with temperature), so we need to
> > measure the phase difference between the two and steer the oscillator
> > slightly.
> >
> > The phase comparision between the two signals is done in HW with a
> > phasemeter, for precise comparisons.  The actual phase
> > steering/adjustment is done through adjphase().
> 
> So you don't need the time stamp itself, just the phase offset, right?
> 
> > What's missing is the ability to report the phase difference to user
> > space so the adjustment can be performed.
> 
> So let's create an interface for that reporting.
> 
> > Since these events are channel specific, I don't see why this is
> > problematic.  The code blocks in question from my upcoming patch
> > (dependent on this) is:
> 
> The long standing policy is not to add new features that don't have users.  It
> would certainly help me in review if I could see the entire patch series.  Also,
> I wonder what the user space code looks like.
> 
> > I'm not seeing why this is controversial.
> 
> It is about getting the right interface.  The external time stamp interface is
> generic and all-purpose, and so I question whether your extension makes
> sense.

You can use different channel index in the struct ptp_clock_event to receive 
them from more than one source. Then just calculate the difference between 
the 1PPS from channel 0 and channel 1. Wouldn't that be sufficient?

Regards
Maciek