[PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[Min Li]

From: Min Li <min.li.xe@renesas.com>

This change is for the PHC devices that can measure the
phase offset between PHC signal and the external signal, such
as GNSS. With this change, ts2phc can use the existing extts
interface to retrieve measurement offset so that the alignment
between PHC and the external signal can be achieved.

Signed-off-by: Min Li <min.li.xe@renesas.com>
---
 drivers/ptp/ptp_clock.c          | 12 +++++++++---
 include/linux/ptp_clock_kernel.h |  2 ++
 include/uapi/linux/ptp_clock.h   |  9 +++++++--
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 3134568af622..c87954c8642f 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->o = src->offset;
+	}
 
 	if (!queue_free(queue))
 		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
@@ -416,6 +421,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 1ef4e0f9bd2a..7f2d1e1cc185 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,
 };
@@ -218,6 +219,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 da700999cad4..61e0473cdf53 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 occured. */
+		__s64 o; /* measured offset */
+	};
 	unsigned int index;      /* Which channel produced the event. */
 	unsigned int flags;      /* Reserved for future use. */
 	unsigned int rsv[2];     /* Reserved for future use. */
-- 
2.39.2

Re: [PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[Jacob Keller]

On 11/13/2023 1:50 PM, Min Li wrote:
> From: Min Li <min.li.xe@renesas.com>
> 
> This change is for the PHC devices that can measure the
> phase offset between PHC signal and the external signal, such
> as GNSS. With this change, ts2phc can use the existing extts
> interface to retrieve measurement offset so that the alignment
> between PHC and the external signal can be achieved.
> 
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---

Ok, so this adds a new event type "extoff" which is for eternal
timestamp offset, and it allows reporting the offset instead of the raw
time.

Userspace can request this feature via the new PTP_EXT_OFFSET flag, and
drivers will report it as a new event type.

You mention GNSS, but is there an example or a link to a driver change
you could provide to show its use?

I think the concept seems reasonable and the code seems correct.

Thanks,
Jake

Re: [PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[Richard Cochran]

On Mon, Nov 13, 2023 at 04:50:25PM -0500, Min Li wrote:
> From: Min Li <min.li.xe@renesas.com>
> 
> This change is for the PHC devices that can measure the
> phase offset between PHC signal and the external signal, such
> as GNSS. With this change, ts2phc can use the existing extts
> interface to retrieve measurement offset so that the alignment
> between PHC and the external signal can be achieved.
> 
> Signed-off-by: Min Li <min.li.xe@renesas.com>
> ---
>  drivers/ptp/ptp_clock.c          | 12 +++++++++---
>  include/linux/ptp_clock_kernel.h |  2 ++
>  include/uapi/linux/ptp_clock.h   |  9 +++++++--
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index 3134568af622..c87954c8642f 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)

Space after 'if' keyword please.

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

ditto.

> +		dst->t.sec = seconds;
> +		dst->t.nsec = remainder;
> +	} else {
> +		dst->o = src->offset;
> +	}
>  
>  	if (!queue_free(queue))
>  		queue->head = (queue->head + 1) % PTP_MAX_TIMESTAMPS;
> @@ -416,6 +421,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 1ef4e0f9bd2a..7f2d1e1cc185 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,
>  };
> @@ -218,6 +219,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 da700999cad4..61e0473cdf53 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 occured. */
> +		__s64 o; /* measured offset */

How about calling it offset_ns so that the unit is clear?

> +	};
>  	unsigned int index;      /* Which channel produced the event. */
>  	unsigned int flags;      /* Reserved for future use. */
>  	unsigned int rsv[2];     /* Reserved for future use. */
> -- 
> 2.39.2
> 

Re: [PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[Richard Cochran]

On Mon, Nov 13, 2023 at 03:00:15PM -0800, Jacob Keller wrote:
> You mention GNSS, but is there an example or a link to a driver change
> you could provide to show its use?

Yes, the new option must wait for a driver that implements it.  Can
you make a patch series where the driver change appears in the second
patch?

Thanks,
Richard

RE: [PATCH net-next RFC 1/1] ptp: use extts interface for the measured external offset

[Min Li]

> 
> On Mon, Nov 13, 2023 at 03:00:15PM -0800, Jacob Keller wrote:
> > You mention GNSS, but is there an example or a link to a driver change
> > you could provide to show its use?
> 
> Yes, the new option must wait for a driver that implements it.  Can you make
> a patch series where the driver change appears in the second patch?

Hi Richard/Jacob

Thanks for the review. Unfortunately, I don't have the patch based on the 
Existing PHC driver.

But I have a brand new PHC driver for our new timing HW that implements 
the feature. The measurement is done by the HW using the 
Time to Digital Converter (TDC) that performs the function of a stopwatch and 
measures the elapsed time between a START pulse and STOP pulses. So I have
GNSS 1pps input to the HW and TDC will perform continuous measurement
between the input 1pps and the 1pps of the onboard PHC.

I also have the ts2phc patch ready along with the PHC change. I already 
tested them on our HW and proved that ts2phc can align the 2 signal using 
the existing PI filter.

Please let me know how you wanna proceed. Do you want me to attach the
new PHC with this patch?

Min