All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
@ 2021-06-28 18:46 Jonathan Lemon
  2021-06-28 23:30 ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Lemon @ 2021-06-28 18:46 UTC (permalink / raw)
  To: netdev, richardcochran; +Cc: kernel-team

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(+)

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;
 };
 
 /**
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-28 18:46 [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event Jonathan Lemon
@ 2021-06-28 23:30 ` Richard Cochran
  2021-06-29  0:19   ` Jonathan Lemon
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2021-06-28 23:30 UTC (permalink / raw)
  To: Jonathan Lemon; +Cc: netdev, kernel-team

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-28 23:30 ` Richard Cochran
@ 2021-06-29  0:19   ` Jonathan Lemon
  2021-06-30  0:09     ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Lemon @ 2021-06-29  0:19 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, kernel-team

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.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-29  0:19   ` Jonathan Lemon
@ 2021-06-30  0:09     ` Richard Cochran
  2021-06-30  3:50       ` Jonathan Lemon
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2021-06-30  0:09 UTC (permalink / raw)
  To: Jonathan Lemon; +Cc: netdev, kernel-team

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30  0:09     ` Richard Cochran
@ 2021-06-30  3:50       ` Jonathan Lemon
  2021-06-30 14:42         ` Richard Cochran
  2021-07-01 14:59         ` Richard Cochran
  0 siblings, 2 replies; 12+ messages in thread
From: Jonathan Lemon @ 2021-06-30  3:50 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, kernel-team

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(&reg->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, &reg->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(&reg->time_sec);
            ev.pps_times.ts_real.tv_nsec = ioread32(&reg->time_ns);
            ev.data = ioread32(&reg->ts_count);

            ptp_clock_event(ext->bp->ptp, &ev);

            iowrite32(1, &reg->intr);       /* write 1 to ack */

            return IRQ_HANDLED;
    }


I'm not seeing why this is controversial.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30  3:50       ` Jonathan Lemon
@ 2021-06-30 14:42         ` Richard Cochran
  2021-06-30 15:55           ` Machnikowski, Maciej
  2021-06-30 22:57           ` Jonathan Lemon
  2021-07-01 14:59         ` Richard Cochran
  1 sibling, 2 replies; 12+ messages in thread
From: Richard Cochran @ 2021-06-30 14:42 UTC (permalink / raw)
  To: Jonathan Lemon; +Cc: netdev, kernel-team

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30 14:42         ` Richard Cochran
@ 2021-06-30 15:55           ` Machnikowski, Maciej
  2021-06-30 22:16             ` Jonathan Lemon
  2021-06-30 22:57           ` Jonathan Lemon
  1 sibling, 1 reply; 12+ messages in thread
From: Machnikowski, Maciej @ 2021-06-30 15:55 UTC (permalink / raw)
  To: Richard Cochran, Jonathan Lemon; +Cc: netdev, kernel-team



> -----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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30 15:55           ` Machnikowski, Maciej
@ 2021-06-30 22:16             ` Jonathan Lemon
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Lemon @ 2021-06-30 22:16 UTC (permalink / raw)
  To: Machnikowski, Maciej; +Cc: Richard Cochran, netdev, kernel-team

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.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30 14:42         ` Richard Cochran
  2021-06-30 15:55           ` Machnikowski, Maciej
@ 2021-06-30 22:57           ` Jonathan Lemon
  1 sibling, 0 replies; 12+ messages in thread
From: Jonathan Lemon @ 2021-06-30 22:57 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, kernel-team

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.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-06-30  3:50       ` Jonathan Lemon
  2021-06-30 14:42         ` Richard Cochran
@ 2021-07-01 14:59         ` Richard Cochran
  2021-07-01 16:15           ` Jonathan Lemon
  1 sibling, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2021-07-01 14:59 UTC (permalink / raw)
  To: Jonathan Lemon; +Cc: netdev, kernel-team

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(&reg->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, &reg->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(&reg->time_sec);
>             ev.pps_times.ts_real.tv_nsec = ioread32(&reg->time_ns);
>             ev.data = ioread32(&reg->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, &reg->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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-07-01 14:59         ` Richard Cochran
@ 2021-07-01 16:15           ` Jonathan Lemon
  2021-07-01 17:07             ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Lemon @ 2021-07-01 16:15 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, kernel-team

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(&reg->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, &reg->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(&reg->time_sec);
> >             ev.pps_times.ts_real.tv_nsec = ioread32(&reg->time_ns);
> >             ev.data = ioread32(&reg->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, &reg->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.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event
  2021-07-01 16:15           ` Jonathan Lemon
@ 2021-07-01 17:07             ` Richard Cochran
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Cochran @ 2021-07-01 17:07 UTC (permalink / raw)
  To: Jonathan Lemon; +Cc: netdev, kernel-team

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-07-01 17:07 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28 18:46 [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event Jonathan Lemon
2021-06-28 23:30 ` Richard Cochran
2021-06-29  0:19   ` Jonathan Lemon
2021-06-30  0:09     ` Richard Cochran
2021-06-30  3:50       ` Jonathan Lemon
2021-06-30 14:42         ` Richard Cochran
2021-06-30 15:55           ` Machnikowski, Maciej
2021-06-30 22:16             ` Jonathan Lemon
2021-06-30 22:57           ` Jonathan Lemon
2021-07-01 14:59         ` Richard Cochran
2021-07-01 16:15           ` Jonathan Lemon
2021-07-01 17:07             ` Richard Cochran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.