| Message ID | 20210628184611.3024919-1-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Jun 28, 2021 at 11:46:11AM -0700, Jonathan Lemon wrote: > This event differs from CLOCK_EXTTS in two ways: > > 1) The caller provides the sec/nsec fields directly, instead of > needing to convert them from the timestamp field. And that is useful? how? > 2) A 32 bit data field is attached to the event, which is returned > to userspace, which allows returning timestamped data information. > This may be used for things like returning the phase difference > between two time sources. What two time sources? What problem are you trying to solve? As it stands, without any kind of rational at all, this patch gets a NAK. Thanks, Richard
On Mon, Jun 28, 2021 at 04:30:56PM -0700, Richard Cochran wrote: > On Mon, Jun 28, 2021 at 11:46:11AM -0700, Jonathan Lemon wrote: > > This event differs from CLOCK_EXTTS in two ways: > > > > 1) The caller provides the sec/nsec fields directly, instead of > > needing to convert them from the timestamp field. > > And that is useful? how? The devices I have provide the sec/nsec values directly from the FPGA/hardware. (IIRC, there is another in-tree driver that does the same thing). Right now, these values must be coerced into a timestap, and then re-converted back to a sec/nsec value, which seems a bit pointless. > > 2) A 32 bit data field is attached to the event, which is returned > > to userspace, which allows returning timestamped data information. > > This may be used for things like returning the phase difference > > between two time sources. > > What two time sources? > > What problem are you trying to solve? > > As it stands, without any kind of rational at all, this patch gets a NAK. I have two use cases: 1) The external PPS source from the FPGA returns a counter corresponding to the pulse events which is used to insure a pulse isn't lost. This is a marginal case, since the counter can be reverse engineered from the timestamp. 2) The OpenCompute timecard has an on-board rubidium atomic clock, and also a GPS receiver. The atomic clock needs to be steered slightly to keep thse in phase. The PPS from the clock and GPS are measured and the phase difference/error is reported every second. This difference is then used to steer the clock, so it can reliably take over in case of GPS loss. So, I need a PPS (with timestamp), along with the phase difference (data). This fits in nicely with the extts event model. I really don't want to have to re-invent another ptp_chardev device that does the same thing - nor recreate a extended PTP.
On Mon, Jun 28, 2021 at 05:19:28PM -0700, Jonathan Lemon wrote: > This fits in nicely with the extts event model. I really > don't want to have to re-invent another ptp_chardev device > that does the same thing - nor recreate a extended PTP. If you have two clocks, then you should expose two PHC devices. If you want to compare two PHC in hardware, then we can have a new syscall for that, like clock_compare(clock_t a, clock_t b); Thanks, Richard
On Tue, Jun 29, 2021 at 05:09:33PM -0700, Richard Cochran wrote: > On Mon, Jun 28, 2021 at 05:19:28PM -0700, Jonathan Lemon wrote: > > This fits in nicely with the extts event model. I really > > don't want to have to re-invent another ptp_chardev device > > that does the same thing - nor recreate a extended PTP. > > If you have two clocks, then you should expose two PHC devices. > > If you want to compare two PHC in hardware, then we can have a new > syscall for that, like clock_compare(clock_t a, clock_t b); This isn't what I'm doing - there is only one clock. 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(). What's missing is the ability to report the phase difference to user space so the adjustment can be performed. Signal reporting to user space is already in place by doing a read(/dev/ptp), which returns: struct ptp_extts_event { struct ptp_clock_time t; /* Time event occured. */ unsigned int index; /* Which channel produced the event. */ unsigned int flags; /* Reserved for future use. */ unsigned int rsv[2]; /* Reserved for future use. */ } This is exactly what I want, with the additional feature of returning the phase difference in a one of the rsv[] words, and perhaps also using the flags word to indicate usage of the reserved field. 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: static irqreturn_t ptp_ocp_phase_irq(int irq, void *priv) { struct ptp_ocp_ext_src *ext = priv; struct ocp_phase_reg __iomem *reg = ext->mem; struct ptp_clock_event ev; u32 phase_error; phase_error = ioread32(®->phase_error); ev.type = PTP_CLOCK_EXTTSUSR; ev.index = ext->info->index; ev.data = phase_error; pps_get_ts(&ev.pps_times); ptp_clock_event(ext->bp->ptp, &ev); iowrite32(0, ®->intr); return IRQ_HANDLED; } and static irqreturn_t ptp_ocp_ts_irq(int irq, void *priv) { struct ptp_ocp_ext_src *ext = priv; struct ts_reg __iomem *reg = ext->mem; struct ptp_clock_event ev; ev.type = PTP_CLOCK_EXTTSUSR; ev.index = ext->info->index; ev.pps_times.ts_real.tv_sec = ioread32(®->time_sec); ev.pps_times.ts_real.tv_nsec = ioread32(®->time_ns); ev.data = ioread32(®->ts_count); ptp_clock_event(ext->bp->ptp, &ev); iowrite32(1, ®->intr); /* write 1 to ack */ return IRQ_HANDLED; } I'm not seeing why this is controversial.
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. I guess from what you have explained so far that the: - GPS produces a pulse on the full second. - That pulse is time stamped in the PHC. - The HW calculates the difference between the full second and the captured time. - User space steers the PHC based on the difference. If this is so, why not simply use the time stamp itself? Or if the extra number is a correction to the time stamp, why not apply the correction to the time stamp? Thanks, Richard
> -----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
On Wed, Jun 30, 2021 at 03:55:03PM +0000, Machnikowski, Maciej wrote: > > > > -----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(). > > > > 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? This is what is being done right now - just in hardware for higher precision. I asked the HW guys to check whether doing this in SW instead will provide the same precision - I should hear back by tomorrow.
On Wed, Jun 30, 2021 at 07:42:57AM -0700, Richard Cochran wrote: > 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. The current 'struct ptp_extts_event' returns 32 bytes to userspace for every event. Of these, 16 bytes (50%) are unused, as the structure only returns a timestamp + index, without any event information. It seems logical that these unused bytes (which are event specific) could be used to convey more information about the event itself. > 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. I question whether the definition of "all-purpose" really applies here. All it tells me is that "an event happened on this channel at this time". If the user doesn't care about additional data, it can just be ignored, right? In the meantime, let's see what the HW guys say about doing the comparision in SW. Other vendors have PPS input to their MAC, so the disciplining is done in HW, bypassing adjphase() completely.
On Tue, Jun 29, 2021 at 08:50:31PM -0700, Jonathan Lemon wrote: > Since these events are channel specific, I don't see why > this is problematic. The problem is that the semantics of ptp_clock_event::data are not defined... > The code blocks in question from my > upcoming patch (dependent on this) is: > > static irqreturn_t > ptp_ocp_phase_irq(int irq, void *priv) > { > struct ptp_ocp_ext_src *ext = priv; > struct ocp_phase_reg __iomem *reg = ext->mem; > struct ptp_clock_event ev; > u32 phase_error; > > phase_error = ioread32(®->phase_error); > > ev.type = PTP_CLOCK_EXTTSUSR; > ev.index = ext->info->index; > ev.data = phase_error; > pps_get_ts(&ev.pps_times); Here the time stamp is the system time, and the .data field is the mysterious "phase_error", but ... > ptp_clock_event(ext->bp->ptp, &ev); > > iowrite32(0, ®->intr); > > return IRQ_HANDLED; > } > > and > > static irqreturn_t > ptp_ocp_ts_irq(int irq, void *priv) > { > struct ptp_ocp_ext_src *ext = priv; > struct ts_reg __iomem *reg = ext->mem; > struct ptp_clock_event ev; > > ev.type = PTP_CLOCK_EXTTSUSR; > ev.index = ext->info->index; > ev.pps_times.ts_real.tv_sec = ioread32(®->time_sec); > ev.pps_times.ts_real.tv_nsec = ioread32(®->time_ns); > ev.data = ioread32(®->ts_count); here the time stamp comes from the PHC device, apparently, and the .data field is a counter. > ptp_clock_event(ext->bp->ptp, &ev); > > iowrite32(1, ®->intr); /* write 1 to ack */ > > return IRQ_HANDLED; > } > > > I'm not seeing why this is controversial. Simply put, it makes no sense to provide a new PTP_CLOCK_EXTTSUSR that has multiple, random meanings. There has got to be a better way. Thanks, Richard
On Thu, Jul 01, 2021 at 07:59:35AM -0700, Richard Cochran wrote: > On Tue, Jun 29, 2021 at 08:50:31PM -0700, Jonathan Lemon wrote: > > > Since these events are channel specific, I don't see why > > this is problematic. > > The problem is that the semantics of ptp_clock_event::data are not > defined... > > > The code blocks in question from my > > upcoming patch (dependent on this) is: > > > > static irqreturn_t > > ptp_ocp_phase_irq(int irq, void *priv) > > { > > struct ptp_ocp_ext_src *ext = priv; > > struct ocp_phase_reg __iomem *reg = ext->mem; > > struct ptp_clock_event ev; > > u32 phase_error; > > > > phase_error = ioread32(®->phase_error); > > > > ev.type = PTP_CLOCK_EXTTSUSR; > > ev.index = ext->info->index; > > ev.data = phase_error; > > pps_get_ts(&ev.pps_times); > > Here the time stamp is the system time, and the .data field is the > mysterious "phase_error", but ... Yes, the time here is not really relevant. > > ptp_clock_event(ext->bp->ptp, &ev); > > > > iowrite32(0, ®->intr); > > > > return IRQ_HANDLED; > > } > > > > and > > > > static irqreturn_t > > ptp_ocp_ts_irq(int irq, void *priv) > > { > > struct ptp_ocp_ext_src *ext = priv; > > struct ts_reg __iomem *reg = ext->mem; > > struct ptp_clock_event ev; > > > > ev.type = PTP_CLOCK_EXTTSUSR; > > ev.index = ext->info->index; > > ev.pps_times.ts_real.tv_sec = ioread32(®->time_sec); > > ev.pps_times.ts_real.tv_nsec = ioread32(®->time_ns); > > ev.data = ioread32(®->ts_count); > > here the time stamp comes from the PHC device, apparently, and the > .data field is a counter. > > > ptp_clock_event(ext->bp->ptp, &ev); > > > > iowrite32(1, ®->intr); /* write 1 to ack */ > > > > return IRQ_HANDLED; > > } > > > > > > I'm not seeing why this is controversial. > > Simply put, it makes no sense to provide a new PTP_CLOCK_EXTTSUSR that > has multiple, random meanings. There has got to be a better way. I agree with this. I just talked to the HW folks, and they're willing to change things so 2 PPS events are returned, which eliminates the need for an internal comparator. The returned time would come from a latched value from a HW register (same as the ptp_ocp_ts_irq version above). I'm still stuck with how to provide the sec/nsec from the hardware directly to the application, though, since the current code does: static void enqueue_external_timestamp(struct timestamp_event_queue *queue, struct ptp_clock_event *src) { struct ptp_extts_event *dst; unsigned long flags; s64 seconds; u32 remainder; seconds = div_u64_rem(src->timestamp, 1000000000, &remainder); It seems like there should be a way to use pps_times here instead of needing to convert back and forth.
On Thu, Jul 01, 2021 at 09:15:55AM -0700, Jonathan Lemon wrote: > static void enqueue_external_timestamp(struct timestamp_event_queue *queue, > struct ptp_clock_event *src) > { > struct ptp_extts_event *dst; > unsigned long flags; > s64 seconds; > u32 remainder; > > seconds = div_u64_rem(src->timestamp, 1000000000, &remainder); > > > It seems like there should be a way to use pps_times here instead > of needing to convert back and forth. You could re-factor that to have two callers, with the part that enqueues in a shared helper function. The only reason the API has a 64 bit word instead of a timespec is that many, but not all drivers use timecounter_cyc2time() or similar to calculate the time stamp. But the ptp_clock_event is really meant to be polymorphic, with pps_times only set for traditional NTP PPS events (activated by the PTP_ENABLE_PPS ioctl). * struct ptp_clock_event - decribes a PTP hardware clock event * * @type: One of the ptp_clock_events enumeration values. * @index: Identifies the source of the event. * @timestamp: When the event occurred (%PTP_CLOCK_EXTTS only). * @pps_times: When the event occurred (%PTP_CLOCK_PPSUSR only). The PTP_CLOCK_EXTTS is different. It is meant for generic time stamping of external signals, activated by the PTP_EXTTS_REQUEST ioctl. I'm not sure which type is better suited to your HW. Thanks, Richard
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 841d8900504d..c176fa82df85 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -63,6 +63,28 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue, spin_unlock_irqrestore(&queue->lock, flags); } +static void enqueue_external_usr_timestamp(struct timestamp_event_queue *queue, + struct ptp_clock_event *src) +{ + struct ptp_extts_event *dst; + unsigned long flags; + + spin_lock_irqsave(&queue->lock, flags); + + dst = &queue->buf[queue->tail]; + dst->index = src->index; + dst->t.sec = src->pps_times.ts_real.tv_sec; + dst->t.nsec = src->pps_times.ts_real.tv_nsec; + dst->rsv[0] = src->data; + + if (!queue_free(queue)) + queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS; + + queue->tail = (queue->tail + 1) % PTP_MAX_TIMESTAMPS; + + spin_unlock_irqrestore(&queue->lock, flags); +} + /* posix clock implementation */ static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp) @@ -311,6 +333,11 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) wake_up_interruptible(&ptp->tsev_wq); break; + case PTP_CLOCK_EXTTSUSR: + enqueue_external_usr_timestamp(&ptp->tsevq, event); + wake_up_interruptible(&ptp->tsev_wq); + break; + case PTP_CLOCK_PPS: pps_get_ts(&evt); pps_event(ptp->pps_source, &evt, PTP_PPS_EVENT, NULL); diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index aba237c0b3a2..ef1aa788350e 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -166,6 +166,7 @@ enum ptp_clock_events { PTP_CLOCK_EXTTS, PTP_CLOCK_PPS, PTP_CLOCK_PPSUSR, + PTP_CLOCK_EXTTSUSR, }; /** @@ -175,6 +176,7 @@ enum ptp_clock_events { * @index: Identifies the source of the event. * @timestamp: When the event occurred (%PTP_CLOCK_EXTTS only). * @pps_times: When the event occurred (%PTP_CLOCK_PPSUSR only). + * @data: Extra data for event (%PTP_CLOCK_EXTTSUSR only). */ struct ptp_clock_event { @@ -184,6 +186,7 @@ struct ptp_clock_event { u64 timestamp; struct pps_event_time pps_times; }; + unsigned int data; }; /**
This event differs from CLOCK_EXTTS in two ways: 1) The caller provides the sec/nsec fields directly, instead of needing to convert them from the timestamp field. 2) A 32 bit data field is attached to the event, which is returned to userspace, which allows returning timestamped data information. This may be used for things like returning the phase difference between two time sources. For discussion: The data field is returned as rsv[0], which is part of the current UAPI. Arguably, this should be renamed, and possibly a flag value set in the 'struct ptp_extts_event' indicating field validity. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> --- drivers/ptp/ptp_clock.c | 27 +++++++++++++++++++++++++++ include/linux/ptp_clock_kernel.h | 3 +++ 2 files changed, 30 insertions(+)