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