| Message ID | PH7PR03MB706431C1C25954AD76134FD8A08CA@PH7PR03MB7064.namprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3,1/2] ptp: introduce PTP_CLOCK_EXTOFF event for the measured external offset | expand |
On Thu, Dec 14, 2023 at 11:36:24AM -0500, Min Li wrote: > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index da700999c..66f4dd73a 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -32,6 +32,7 @@ > #define PTP_RISING_EDGE (1<<1) > #define PTP_FALLING_EDGE (1<<2) > #define PTP_STRICT_FLAGS (1<<3) > +#define PTP_EXT_OFFSET (1<<4) This isn't going to work. If user space enables time stamps twice, once with PTP_EXT_OFFSET and once without, it won't be able to differentiate the two when it reads a ptp_extts_event. Thanks, Richard
> On Thu, Dec 14, 2023 at 11:36:24AM -0500, Min Li wrote: > > diff --git a/include/uapi/linux/ptp_clock.h > > b/include/uapi/linux/ptp_clock.h index da700999c..66f4dd73a 100644 > > --- a/include/uapi/linux/ptp_clock.h > > +++ b/include/uapi/linux/ptp_clock.h > > @@ -32,6 +32,7 @@ > > #define PTP_RISING_EDGE (1<<1) > > #define PTP_FALLING_EDGE (1<<2) > > #define PTP_STRICT_FLAGS (1<<3) > > +#define PTP_EXT_OFFSET (1<<4) > > This isn't going to work. > > If user space enables time stamps twice, once with PTP_EXT_OFFSET and > once without, it won't be able to differentiate the two when it reads a > ptp_extts_event. > > Thanks, > Richard Hi Richard Would it be Ok if I use the flags to differentiate extts events from extoff? struct ptp_extts_event { - struct ptp_clock_time t; /* Time event occured. */ + union { + struct ptp_clock_time t; /* Time event occurred. */ + __s64 offset_ns; /* Offset event occurred. */ + }; unsigned int index; /* Which channel produced the event. */ unsigned int flags; /* Reserved for future use. */ unsigned int rsv[2]; /* Reserved for future use. */
On Thu, Dec 14, 2023 at 09:59:32PM +0000, Min Li wrote: > Would it be Ok if I use the flags to differentiate extts events from extoff? That makes sense to me. We can return the relevant ptp_extts_request.flags. Something like: #define PTP_EXTTS_FLAGS_VALID PTP_ENABLE_FEATURE Then you can return ptp_extts_event.flags = PTP_EXTTS_FLAGS_VALID | PTP_EXT_OFFSET; Later on, other drivers can indicate PTP_[RISING|FALLING]_EDGE, if they can tell which one happened. > struct ptp_extts_event { > - struct ptp_clock_time t; /* Time event occured. */ > + union { > + struct ptp_clock_time t; /* Time event occurred. */ > + __s64 offset_ns; /* Offset event occurred. */ BTW, please don't make a union here. Instead just add text to the comment of `struct ptp_clock_time t`. The `struct ptp_clock_time t` can be a postive or a negative value (see comment at the top of the file), and so you can put an offset in there as well. Thanks, Richard
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 15b804ba4..01b55c1e2 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -48,14 +48,19 @@ static void enqueue_external_timestamp(struct timestamp_event_queue *queue, s64 seconds; u32 remainder; - seconds = div_u64_rem(src->timestamp, 1000000000, &remainder); + if (src->type != PTP_CLOCK_EXTOFF) + seconds = div_u64_rem(src->timestamp, 1000000000, &remainder); spin_lock_irqsave(&queue->lock, flags); dst = &queue->buf[queue->tail]; dst->index = src->index; - dst->t.sec = seconds; - dst->t.nsec = remainder; + if (src->type != PTP_CLOCK_EXTOFF) { + dst->t.sec = seconds; + dst->t.nsec = remainder; + } else { + dst->offset_ns = src->offset; + } /* Both WRITE_ONCE() are paired with READ_ONCE() in queue_cnt() */ if (!queue_free(queue)) @@ -417,6 +422,7 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event) break; case PTP_CLOCK_EXTTS: + case PTP_CLOCK_EXTOFF: /* Enqueue timestamp on selected queues */ spin_lock_irqsave(&ptp->tsevqs_lock, flags); list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index 1ef4e0f9b..6e4b8206c 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -200,6 +200,7 @@ struct ptp_clock; enum ptp_clock_events { PTP_CLOCK_ALARM, PTP_CLOCK_EXTTS, + PTP_CLOCK_EXTOFF, PTP_CLOCK_PPS, PTP_CLOCK_PPSUSR, }; @@ -210,6 +211,7 @@ enum ptp_clock_events { * @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). + * @offset: When the event occurred (%PTP_CLOCK_EXTOFF only). * @pps_times: When the event occurred (%PTP_CLOCK_PPSUSR only). */ @@ -218,6 +220,7 @@ struct ptp_clock_event { int index; union { u64 timestamp; + s64 offset; struct pps_event_time pps_times; }; }; diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index da700999c..66f4dd73a 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -32,6 +32,7 @@ #define PTP_RISING_EDGE (1<<1) #define PTP_FALLING_EDGE (1<<2) #define PTP_STRICT_FLAGS (1<<3) +#define PTP_EXT_OFFSET (1<<4) #define PTP_EXTTS_EDGES (PTP_RISING_EDGE | PTP_FALLING_EDGE) /* @@ -40,7 +41,8 @@ #define PTP_EXTTS_VALID_FLAGS (PTP_ENABLE_FEATURE | \ PTP_RISING_EDGE | \ PTP_FALLING_EDGE | \ - PTP_STRICT_FLAGS) + PTP_STRICT_FLAGS | \ + PTP_EXT_OFFSET) /* * flag fields valid for the original PTP_EXTTS_REQUEST ioctl. @@ -228,7 +230,10 @@ struct ptp_pin_desc { #define PTP_MASK_EN_SINGLE _IOW(PTP_CLK_MAGIC, 20, unsigned int) struct ptp_extts_event { - struct ptp_clock_time t; /* Time event occured. */ + union { + struct ptp_clock_time t; /* Time event occurred. */ + __s64 offset_ns; /* Offset event occurred. */ + }; unsigned int index; /* Which channel produced the event. */ unsigned int flags; /* Reserved for future use. */ unsigned int rsv[2]; /* Reserved for future use. */