Re: [PATCH net-next 2/3] ptp: introduce a phase offset in the periodic output request

[Jacob Keller <jacob.e.keller@intel.com>]

On 7/16/2020 2:20 PM, Vladimir Oltean wrote:
> Some PHCs like the ocelot/felix switch cannot emit generic periodic
> output, but just PPS (pulse per second) signals, which:
> - don't start from arbitrary absolute times, but are rather
>   phase-aligned to the beginning of [the closest next] second.
> - have an optional phase offset relative to that beginning of the
>   second.
> 
> For those, it was initially established that they should reject any
> other absolute time for the PTP_PEROUT_REQUEST than 0.000000000 [1].
> 
> But when it actually came to writing an application [2] that makes use
> of this functionality, we realized that we can't really deal generically
> with PHCs that support absolute start time, and with PHCs that don't,
> without an explicit interface. Namely, in an ideal world, PHC drivers
> would ensure that the "perout.start" value written to hardware will
> result in a functional output. This means that if the PTP time has
> become in the past of this PHC's current time, it should be
> automatically fast-forwarded by the driver into a close enough future
> time that is known to work (note: this is necessary only if the hardware
> doesn't do this fast-forward by itself). But we don't really know what
> is the status for PHC drivers in use today, so in the general sense,
> user space would be risking to have a non-functional periodic output if
> it simply asked for a start time of 0.000000000.
> 
> So let's introduce a flag for this type of reduced-functionality
> hardware, named PTP_PEROUT_PHASE. The start time is just "soon", the
> only thing we know for sure about this signal is that its rising edge
> events, Rn, occur at:
> 
> Rn = period.phase + n * perout.period
> 
> The "phase" in the periodic output structure is simply an alias to the
> "start" time, since both cannot logically be specified at the same time.
> Therefore, the binary layout of the structure is not affected.
> 
> [1]: https://patchwork.ozlabs.org/project/netdev/patch/20200320103726.32559-7-yangbo.lu@nxp.com/
> [2]: https://www.mail-archive.com/linuxptp-devel@lists.sourceforge.net/msg04142.html
> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  include/uapi/linux/ptp_clock.h | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 1d2841155f7d..1d108d597f66 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -55,12 +55,14 @@
>   */
>  #define PTP_PEROUT_ONE_SHOT		(1<<0)
>  #define PTP_PEROUT_DUTY_CYCLE		(1<<1)
> +#define PTP_PEROUT_PHASE		(1<<2)
>  
>  /*
>   * flag fields valid for the new PTP_PEROUT_REQUEST2 ioctl.
>   */
>  #define PTP_PEROUT_VALID_FLAGS		(PTP_PEROUT_ONE_SHOT | \
> -					 PTP_PEROUT_DUTY_CYCLE)
> +					 PTP_PEROUT_DUTY_CYCLE | \
> +					 PTP_PEROUT_PHASE)
>  
>  /*
>   * No flags are valid for the original PTP_PEROUT_REQUEST ioctl
> @@ -103,7 +105,20 @@ struct ptp_extts_request {
>  };
>  
>  struct ptp_perout_request {
> -	struct ptp_clock_time start;  /* Absolute start time. */
> +	union {
> +		/*
> +		 * Absolute start time.
> +		 * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
> +		 */
> +		struct ptp_clock_time start;
> +		/*
> +		 * Phase offset. The signal should start toggling at an
> +		 * unspecified integer multiple of the period, plus this value.
> +		 * The start time should be "as soon as possible".
> +		 * Valid only if (flags & PTP_PEROUT_PHASE) is set.
> +		 */
> +		struct ptp_clock_time phase;
> +	};

Ok. Since when using the PHASE mode the start time is "meaningless" we
can re-use it for this purpose without breaking the binary structure.
Makes sense.

>  	struct ptp_clock_time period; /* Desired period, zero means disable. */
>  	unsigned int index;           /* Which channel to configure. */
>  	unsigned int flags;
>