Re: [RFC PATCH net-next 3/6] ptp: Add free running time support

[Richard Cochran <richardcochran@gmail.com>]

On Sun, Mar 06, 2022 at 09:56:55AM +0100, Gerhard Engleder wrote:
> ptp vclocks require a clock with free running time for the timecounter.
> Currently only a physical clock forced to free running is supported.
> If vclocks are used, then the physical clock cannot be synchronized
> anymore. The synchronized time is not available in hardware in this
> case. As a result, timed transmission with ETF/TAPRIO hardware support
> is not possible anymore.
> 
> If hardware would support a free running time additionally to the
> physical clock, then the physical clock does not need to be forced to
> free running. Thus, the physical clocks can still be synchronized while
> vclocks are in use.
> 
> The physical clock could be used to synchronize the time domain of the
> TSN network and trigger ETF/TAPRIO. In parallel vclocks can be used to
> synchronize other time domains.
> 
> Allow read and cross time stamp of additional free running time for
> physical clocks. Let vclocks use free running time if available.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/ptp/ptp_vclock.c         | 20 +++++++++++++++-----
>  include/linux/ptp_clock_kernel.h | 27 +++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
> index cb179a3ea508..3715d75ee8bd 100644
> --- a/drivers/ptp/ptp_vclock.c
> +++ b/drivers/ptp/ptp_vclock.c
> @@ -68,7 +68,10 @@ static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
>  	int err;
>  	u64 ns;
>  
> -	err = pptp->info->gettimex64(pptp->info, &pts, sts);
> +	if (pptp->info->getfreeruntimex64)
> +		err = pptp->info->getfreeruntimex64(pptp->info, &pts, sts);
> +	else
> +		err = pptp->info->gettimex64(pptp->info, &pts, sts);

Why all this extra if/then/else here and at registration?
Just provide new ptp_vclock_ helpers and drop the run time conditionals.

>  	if (err)
>  		return err;
>  
> @@ -104,7 +107,10 @@ static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
>  	int err;
>  	u64 ns;
>  
> -	err = pptp->info->getcrosststamp(pptp->info, xtstamp);
> +	if (pptp->info->getfreeruncrosststamp)
> +		err = pptp->info->getfreeruncrosststamp(pptp->info, xtstamp);
> +	else
> +		err = pptp->info->getcrosststamp(pptp->info, xtstamp);

same here

>  	if (err)
>  		return err;
>  
> @@ -143,7 +149,9 @@ static u64 ptp_vclock_read(const struct cyclecounter *cc)
>  	struct ptp_clock *ptp = vclock->pclock;
>  	struct timespec64 ts = {};
>  
> -	if (ptp->info->gettimex64)
> +	if (ptp->info->getfreeruntimex64)
> +		ptp->info->getfreeruntimex64(ptp->info, &ts, NULL);
> +	else if (ptp->info->gettimex64)
>  		ptp->info->gettimex64(ptp->info, &ts, NULL);
>  	else
>  		ptp->info->gettime64(ptp->info, &ts);
> @@ -168,11 +176,13 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
>  
>  	vclock->pclock = pclock;
>  	vclock->info = ptp_vclock_info;
> -	if (pclock->info->gettimex64)
> +	if (pclock->info->getfreeruntimex64 || pclock->info->gettimex64)
>  		vclock->info.gettimex64 = ptp_vclock_gettimex;
>  	else
>  		vclock->info.gettime64 = ptp_vclock_gettime;
> -	if (pclock->info->getcrosststamp)
> +	if ((pclock->info->getfreeruntimex64 &&
> +	     pclock->info->getfreeruncrosststamp) ||
> +	    pclock->info->getcrosststamp)
>  		vclock->info.getcrosststamp = ptp_vclock_getcrosststamp;
>  	vclock->cc = ptp_vclock_cc;
>  
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 554454cb8693..b291517fc7c8 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -108,6 +108,28 @@ struct ptp_system_timestamp {
>   * @settime64:  Set the current time on the hardware clock.
>   *              parameter ts: Time value to set.
>   *
> + * @getfreeruntimex64:  Reads the current free running time from the hardware
> + *                      clock and optionally also the system clock. This
> + *                      operation requires hardware support for an additional
> + *                      free running time including support for hardware time
> + *                      stamps based on that free running time.
> + *                      The free running time must be completely independet from
> + *                      the actual time of the PTP clock. It must be monotonic
> + *                      and its frequency must be constant.
> + *                      parameter ts: Holds the PHC free running timestamp.
> + *                      parameter sts: If not NULL, it holds a pair of
> + *                      timestamps from the system clock. The first reading is
> + *                      made right before reading the lowest bits of the PHC
> + *                      free running timestamp and the second reading
> + *                      immediately follows that.
> + *
> + * @getfreeruncrosststamp:  Reads the current time from the free running
> + *                          hardware clock and system clock simultaneously.
> + *                          parameter cts: Contains timestamp (device,system)
> + *                          pair, where device time is the free running time
> + *                          also used for @getfreeruntimex64 and system time is
> + *                          realtime and monotonic.
> + *
>   * @enable:   Request driver to enable or disable an ancillary feature.
>   *            parameter request: Desired resource to enable or disable.
>   *            parameter on: Caller passes one to enable or zero to disable.
> @@ -155,6 +177,11 @@ struct ptp_clock_info {
>  	int (*getcrosststamp)(struct ptp_clock_info *ptp,
>  			      struct system_device_crosststamp *cts);
>  	int (*settime64)(struct ptp_clock_info *p, const struct timespec64 *ts);
> +	int (*getfreeruntimex64)(struct ptp_clock_info *ptp,
> +				 struct timespec64 *ts,
> +				 struct ptp_system_timestamp *sts);
> +	int (*getfreeruncrosststamp)(struct ptp_clock_info *ptp,
> +				     struct system_device_crosststamp *cts);

Wow, that is really hard to read.  Suggest freerun_gettimex64 and
freerun_crosststamp instead.

>  	int (*enable)(struct ptp_clock_info *ptp,
>  		      struct ptp_clock_request *request, int on);
>  	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
> -- 
> 2.20.1
> 

Thanks,
Richard