[PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

From: Seb Laveze <sebastien.laveze@nxp.com>

Add an IOCTL to perform per-timestamp conversion, as an extension of the
ptp virtual framework introduced in commit 5d43f951b1ac ("ptp: add ptp
virtual clock driver framework").

The original implementation allows binding a socket to a given virtual
clock to perform the timestamps conversions. Commit 5d43f951b1ac ("ptp:
add ptp virtual clock driver framework").

This binding works well if the application requires all timestamps in the
same domain but is not convenient when multiple domains need to be
supported using a single socket.

Typically, IEEE 802.1AS-2020 can be implemented using a single socket,
the CMLDS layer using raw PHC timestamps and the domain specific
timestamps converted in the appropriate gPTP domain using this IOCTL.

Signed-off-by: Seb Laveze <sebastien.laveze@nxp.com>
---
 drivers/ptp/ptp_chardev.c      | 24 ++++++++++++++++++++++++
 include/uapi/linux/ptp_clock.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index af3bc65c4595..28c13098fcba 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -118,10 +118,13 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 	struct ptp_clock_request req;
 	struct ptp_clock_caps caps;
 	struct ptp_clock_time *pct;
+	struct ptp_vclock *vclock;
 	unsigned int i, pin_index;
 	struct ptp_pin_desc pd;
 	struct timespec64 ts;
+	unsigned long flags;
 	int enable, err = 0;
+	s64 ns;
 
 	switch (cmd) {
 
@@ -418,6 +421,27 @@ long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg)
 		mutex_unlock(&ptp->pincfg_mux);
 		break;
 
+	case PTP_VCLOCK_CONV_TS:
+		if (!ptp->is_virtual_clock)
+			return -EINVAL;
+
+		vclock = info_to_vclock(ptp->info);
+
+		if (get_timespec64(&ts, (void __user *)arg))
+			return -EFAULT;
+
+		ns = timespec64_to_ns(&ts);
+
+		spin_lock_irqsave(&vclock->lock, flags);
+		ns = timecounter_cyc2time(&vclock->tc, ns);
+		spin_unlock_irqrestore(&vclock->lock, flags);
+
+		ts = ns_to_timespec64(ns);
+
+		if (put_timespec64(&ts, (void __user *)arg))
+			return -EFAULT;
+		break;
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
index 1d108d597f66..13147d454aa8 100644
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -223,6 +223,7 @@ struct ptp_pin_desc {
 	_IOWR(PTP_CLK_MAGIC, 17, struct ptp_sys_offset_precise)
 #define PTP_SYS_OFFSET_EXTENDED2 \
 	_IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
+#define PTP_VCLOCK_CONV_TS  _IOWR(PTP_CLK_MAGIC, 19, struct __kernel_timespec)
 
 struct ptp_extts_event {
 	struct ptp_clock_time t; /* Time event occured. */
-- 
2.25.1

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Mon, Sep 27, 2021 at 11:32:50AM +0200, Sebastien Laveze wrote:
> Add an IOCTL to perform per-timestamp conversion, as an extension of the
> ptp virtual framework introduced in commit 5d43f951b1ac ("ptp: add ptp
> virtual clock driver framework").

I'm not wild about having yet another ioctl for functionality that
already exists.

> This binding works well if the application requires all timestamps in the
> same domain but is not convenient when multiple domains need to be
> supported using a single socket.

Opening multiple sockets is not rocket science.
 
> Typically, IEEE 802.1AS-2020 can be implemented using a single socket,
> the CMLDS layer using raw PHC timestamps and the domain specific
> timestamps converted in the appropriate gPTP domain using this IOCTL.

You say "typically", but how many applications actually do this?  I
can't think of any at all.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Mon, 2021-09-27 at 07:59 -0700, Richard Cochran wrote:
> I'm not wild about having yet another ioctl for functionality that
> already exists.

I was expecting some pushback :)

> > This binding works well if the application requires all timestamps in the
> > same domain but is not convenient when multiple domains need to be
> > supported using a single socket.
> 
> Opening multiple sockets is not rocket science.

I agree but you end-up handling or filtering the same traffic for each
socket. Not rocket science, but not "ideal".

> > Typically, IEEE 802.1AS-2020 can be implemented using a single socket,
> > the CMLDS layer using raw PHC timestamps and the domain specific
> > timestamps converted in the appropriate gPTP domain using this IOCTL.
> 
> You say "typically", but how many applications actually do this?  I
> can't think of any at all.

The "typically" was more a reference to this possible implementation of
AS-2020 using a common CMLDS layer and several domains using a single
socket.

So, without this IOCTL the design would be 1 socket for CMLDS layer
and 1 socket for each domain plus some specific filtering for each
socket to avoid processing the unwanted traffic.

With this IOCTL, the design would be 1 socket and 1 conversion for the
sync messages in the appropriate domain.

This also brings a finer granularity for per-domain timestamps which
may be useful for other applications.

> 
> Thanks,
> Richard

Thanks,
Seb

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Randy Dunlap]

On 9/27/21 2:32 AM, Sebastien Laveze wrote:
> From: Seb Laveze <sebastien.laveze@nxp.com>
> 
> Add an IOCTL to perform per-timestamp conversion, as an extension of the
> ptp virtual framework introduced in commit 5d43f951b1ac ("ptp: add ptp
> virtual clock driver framework").
> 
> The original implementation allows binding a socket to a given virtual
> clock to perform the timestamps conversions. Commit 5d43f951b1ac ("ptp:
> add ptp virtual clock driver framework").
> 
> This binding works well if the application requires all timestamps in the
> same domain but is not convenient when multiple domains need to be
> supported using a single socket.
> 
> Typically, IEEE 802.1AS-2020 can be implemented using a single socket,
> the CMLDS layer using raw PHC timestamps and the domain specific
> timestamps converted in the appropriate gPTP domain using this IOCTL.
> 
> Signed-off-by: Seb Laveze <sebastien.laveze@nxp.com>
> ---
>   drivers/ptp/ptp_chardev.c      | 24 ++++++++++++++++++++++++
>   include/uapi/linux/ptp_clock.h |  1 +
>   2 files changed, 25 insertions(+)
> 

> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 1d108d597f66..13147d454aa8 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -223,6 +223,7 @@ struct ptp_pin_desc {
>   	_IOWR(PTP_CLK_MAGIC, 17, struct ptp_sys_offset_precise)
>   #define PTP_SYS_OFFSET_EXTENDED2 \
>   	_IOWR(PTP_CLK_MAGIC, 18, struct ptp_sys_offset_extended)
> +#define PTP_VCLOCK_CONV_TS  _IOWR(PTP_CLK_MAGIC, 19, struct __kernel_timespec)
>   
>   struct ptp_extts_event {
>   	struct ptp_clock_time t; /* Time event occured. */
> 

Hi,
Would someone please update Documentation/userspace-api/ioctl/ioctl-number.rst
to include
#define PTP_CLK_MAGIC '=', the name of this header file, and optional
comment or contact info, please.

thanks.
-- 
~Randy

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Mon, Sep 27, 2021 at 06:00:08PM +0200, Sebastien Laveze wrote:
> The "typically" was more a reference to this possible implementation of
> AS-2020 using a common CMLDS layer and several domains using a single
> socket.
> 
> So, without this IOCTL the design would be 1 socket for CMLDS layer
> and 1 socket for each domain plus some specific filtering for each
> socket to avoid processing the unwanted traffic.
> 
> With this IOCTL, the design would be 1 socket and 1 conversion for the
> sync messages in the appropriate domain.

The ioctl solution is gross.  A program with eight vclocks should call
recvmsg and parse the CMSG, then go and call ioctl seven times?  Yuck.

What you really want is the socket to return more than one time stamp.
So why not do that instead?

Right now, the SO_TIMESTAMPING has an array of

   struct timespec ts[3] = 
   [0] SOFTWARE
   [1] LEGACY (unused)
   [2] HARDWARE

You can extend that to have

   [0] SOFTWARE
   [1] LEGACY (unused)
   [2] HARDWARE (vclock 0)
   [3] HARDWARE (vclock 1)
   [4] HARDWARE (vclock 2)
   ...
   [N] HARDWARE (vclock N-2)

You could store the selected vclocks in a bit mask associated with the socket.

Hm?

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Mon, 2021-09-27 at 13:23 -0700, Richard Cochran wrote:
> What you really want is the socket to return more than one time stamp.
> So why not do that instead?
> 
> Right now, the SO_TIMESTAMPING has an array of
> 
>    struct timespec ts[3] = 
>    [0] SOFTWARE
>    [1] LEGACY (unused)
>    [2] HARDWARE
> 
> You can extend that to have
> 
>    [0] SOFTWARE
>    [1] LEGACY (unused)
>    [2] HARDWARE (vclock 0)
>    [3] HARDWARE (vclock 1)
>    [4] HARDWARE (vclock 2)
>    ...
>    [N] HARDWARE (vclock N-2)
> 
> You could store the selected vclocks in a bit mask associated with the socket.
> 
> Hm?

Yes that would do it. Only drawback is that ALL rx and tx timestamps
are converted to the N domains instead of a few as needed.

Before going in this direction, is this a change that you really see as
worthwile for virtual clocks and timetamping support ?

What approach do you have in mind for multi-domain support with the
common CMLDS layer ?

Thanks,
Seb

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Tue, Sep 28, 2021 at 01:50:23PM +0200, Sebastien Laveze wrote:
> Yes that would do it. Only drawback is that ALL rx and tx timestamps
> are converted to the N domains instead of a few as needed.

No, the kernel would provide only those that are selected by the
program via the socket option API.

> Before going in this direction, is this a change that you really see as
> worthwile for virtual clocks and timetamping support ?

Yes, indeed.

> What approach do you have in mind for multi-domain support with the
> common CMLDS layer ?

Okay, so I briefly reviewed IEEE Std 802.1AS-2020 Clause 11.2.17.

I think what it boils down to is this:  (please correct me if I'm wrong)

When running PTP over multiple domains on one port, the P2P delay
needs only to be measured in one domain.  It would be wasteful to
measure the peer delay in multiple parallel domains.

linuxptp already provides PORT_DATA_SET.peerMeanPathDelay via management.
The only things missing AFAICT are:

1. neighborRateRatio - This can be provided via a new custom
   management message.

2. Inhibiting the delay mechanism as a per-port option.


All you need to implement CMLDS correction is:

1. nRR from one port, read by other ports via management.

2. periodic clock_gettime() on both ports to figure the rate
   difference between the two ports.

Having said that, because the NR differences will be tiny, and the p2p
message delay so short (like 100 nanoseconds or less), it is
practically worthless to apply the correction.  I personally wouldn't
bother, unless you can prove by measurement that the correction makes
a difference in the synchronization quality.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Tue, 2021-09-28 at 06:31 -0700, Richard Cochran wrote:
> On Tue, Sep 28, 2021 at 01:50:23PM +0200, Sebastien Laveze wrote:
> > Yes that would do it. Only drawback is that ALL rx and tx timestamps
> > are converted to the N domains instead of a few as needed.
> 
> No, the kernel would provide only those that are selected by the
> program via the socket option API.

But _all_ timestamps (rx and tx) are converted when a domain is
selected.

If we consider gPTP,
-using the ioctl, you only need to convert the sync receive timestamps.
PDelay (rx, tx, fup), sync (tx and fup) and signalling don't need to be
converted. So that's for a default sync period of 125 ms, 8 ioctl /
second / domain.
-doing the conversion in the kernel will necessarly be done for every
timestamp handled by the socket. In addition, the vclock device lookup
is not free as well and done for _each_ conversion.

In the end, if the new ioctl is not accepted, I think opening multiple
sockets with bpf filtering and vclock bind is the most appropriate
solution for us. (more details below)

> > 
> Okay, so I briefly reviewed IEEE Std 802.1AS-2020 Clause 11.2.17.
> 
> I think what it boils down to is this:  (please correct me if I'm wrong)
> 
> When running PTP over multiple domains on one port, the P2P delay
> needs only to be measured in one domain.  It would be wasteful to
> measure the peer delay in multiple parallel domains.

From a high-level view, I understand that you would have N
instance/process of linuxptp to support N domains ? CMLDS performed by
one of them and then some signalling to the other instances ?

For our own stack (we work on a multi-OS 802.1AS-2020 implementation)
we took the approach of handling everything in a single process with a
single socket per port (our implementation also supported Bridge mode
with multiple ports).

What we miss currently in the kernel for a better multi-domain usage
and would like to find a solution:
-allow PHC adjustment with virtual clocks. Otherwise scheduled traffic
cannot be used... (I've read your comments on this topic, we are
experimenting things on MCUs and we need to assess on measurements)
-timer support for virtual clocks (nanosleep likely, as yous suggested
IIRC).

Thanks,
Seb

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Wed, Sep 29, 2021 at 05:00:56PM +0200, Sebastien Laveze wrote:
> On Tue, 2021-09-28 at 06:31 -0700, Richard Cochran wrote:
> > On Tue, Sep 28, 2021 at 01:50:23PM +0200, Sebastien Laveze wrote:
> > > Yes that would do it. Only drawback is that ALL rx and tx timestamps
> > > are converted to the N domains instead of a few as needed.
> > 
> > No, the kernel would provide only those that are selected by the
> > program via the socket option API.
> 
> But _all_ timestamps (rx and tx) are converted when a domain is
> selected.

So what?  It is only a mult/shift.  Cheaper than syscall by far.

> If we consider gPTP,
> -using the ioctl, you only need to convert the sync receive timestamps.
> PDelay (rx, tx, fup), sync (tx and fup) and signalling don't need to be
> converted. So that's for a default sync period of 125 ms, 8 ioctl /
> second / domain.

Well, today that is true, for your very specific use case.  But we
don't invent kernel interfaces for one-off projects.

> -doing the conversion in the kernel will necessarly be done for every
> timestamp handled by the socket. In addition, the vclock device lookup
> is not free as well and done for _each_ conversion.

Sounds like something that can be optimized in the kernel implementation.

> From a high-level view, I understand that you would have N
> instance/process of linuxptp to support N domains ?

Yes.

> CMLDS performed by
> one of them and then some signalling to the other instances ?

Yes, something like that.  One process measures peer delay, and the
others read the result via management messages (could also be pushed
via ptp4l's management notification method).
 
> What we miss currently in the kernel for a better multi-domain usage
> and would like to find a solution:
> -allow PHC adjustment with virtual clocks. Otherwise scheduled traffic
> cannot be used... (I've read your comments on this topic, we are
> experimenting things on MCUs and we need to assess on measurements)

Yeah, so you cannot have it both ways, I'm afraid.  Either you adjust
the HW clock or not.  If you don't, it becomes impractical to program
the event registers for output signals.  (Time stamps on input signals
are not an issue, though)

> -timer support for virtual clocks (nanosleep likely, as yous suggested
> IIRC).

Right, and this is (probably) difficult to sell on lkml.  Look at the
hrtimer implementation to see what I mean.

I could imagine adding one additional hrtimer base under user space
control that isn't clock_monotonic or _realtime or _tai, but not N new
bases.

I think the best option for user space wanting timers in multiple
domains is to periodically do 

   gettime(monotonic); gettime(vclock); gettime(monotonic);

figure the conversion, and schedule using clock_monotonic.


HTH,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Thu, 2021-09-30 at 07:35 -0700, Richard Cochran wrote:
> > What we miss currently in the kernel for a better multi-domain usage
> > and would like to find a solution:
> > -allow PHC adjustment with virtual clocks. Otherwise scheduled traffic
> > cannot be used... (I've read your comments on this topic, we are
> > experimenting things on MCUs and we need to assess on measurements)
> 
> Yeah, so you cannot have it both ways, I'm afraid.  Either you adjust
> the HW clock or not.  If you don't, it becomes impractical to program
> the event registers for output signals.  (Time stamps on input signals
> are not an issue, though)

Yes, this is especially true for periodic events completely handled in
hardware like scheduled traffic.

However, not being able to support multiple domains + scheduled traffic
is a true limitation for TSN use cases.

I'm aware of your opinon on this topic:
https://lore.kernel.org/netdev/20210510231832.GA28585@hoboy.vegasvil.org/

However, a few points that _might_ change your mind:
* regular and random small adjustments don't cost that much since the
error you create for the children clocks is only the time for the PHC
to adjust. Since this time is quite small (~10 us ? ), a few ppm on
this short time is negligible.

For example:
Let's take a worst case, the PHC is adjusted every 10 ms, vclock every
1 sec with 1 ppm error.
vclock error is 1 us, now if you add the 100 PHC adjustments each of
them with an error of 1 ppm over 10 us. That gives 0.01 ns * 100 = 1
ns.
This is negligible vs the 1 us error.
Of course, in general, the vclock would be updated more frequently but
in this case even less impacted by PHC adjustments.

* large frequency adjustments are more problematic. I've checked that
some drivers allow up to 10^6 ppm... 
This could lead to non-negligible error. However, since it's already
accepted that using vclock is at the cost of loosing adjustments on the
PHC, it could be accepted that it's still adjustable but with some
restrictions. (1000 ppm max ?)

* offset adjustments do not introduce any error if performed in
software. On other systems we support, handling the offset in software
helped to improve stability as the hardware time becomes monotonic.
There is no added value in setting the offset in the PHC.

> I think the best option for user space wanting timers in multiple
> domains is to periodically do 
> 
>    gettime(monotonic); gettime(vclock); gettime(monotonic);
> 
> figure the conversion, and schedule using clock_monotonic.

Yes, having a good ratio/offset measurement should lead to decent
performance.

Thanks,
Sebastien

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Thu, Oct 07, 2021 at 03:31:43PM +0200, Sebastien Laveze wrote:
> For example:
> Let's take a worst case, the PHC is adjusted every 10 ms, vclock every
> 1 sec with 1 ppm error.

You can't justify adjusting the HW clock and the vclocks
asynchronously with a single, isolated example.

If two independent processes are both adjusting a clock concurrently,
asynchronously, and at different control rates, the result will be
chaos.

It does not matter that it *might* work for some random setup of
yours.

The kernel has to function correctly in general, not just in some
special circumstances.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Thu, 2021-10-07 at 13:19 -0700, Richard Cochran wrote:
> On Thu, Oct 07, 2021 at 03:31:43PM +0200, Sebastien Laveze wrote:
> > For example:
> > Let's take a worst case, the PHC is adjusted every 10 ms, vclock every
> > 1 sec with 1 ppm error.
> 
> You can't justify adjusting the HW clock and the vclocks
> asynchronously with a single, isolated example.

This was a single worst case example, many asynchronous PHC adjustments
impacting vclocks.

> If two independent processes are both adjusting a clock concurrently,
> asynchronously, and at different control rates, the result will be
> chaos.

This is especially what we want to prove feasible and we think it's
posssible with the following conditions:
-limited frequency adjustments
-offset adjustment in software

> It does not matter that it *might* work for some random setup of
> yours.
> 
> The kernel has to function correctly in general, not just in some
> special circumstances.

Of course, so what tests and measurements can we bring on the table to
convince you that it doesn't lead to chaos ?

Thanks,
Sebastien

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Fri, Oct 08, 2021 at 09:13:58AM +0200, Sebastien Laveze wrote:
> On Thu, 2021-10-07 at 13:19 -0700, Richard Cochran wrote:

> > If two independent processes are both adjusting a clock concurrently,
> > asynchronously, and at different control rates, the result will be
> > chaos.
> 
> This is especially what we want to prove feasible and we think it's
> posssible with the following conditions:
> -limited frequency adjustments
> -offset adjustment in software

Sorry, the kernel still must function correctly, even when user space
does crazy stuff.

> > It does not matter that it *might* work for some random setup of
> > yours.
> > 
> > The kernel has to function correctly in general, not just in some
> > special circumstances.
> 
> Of course, so what tests and measurements can we bring on the table to
> convince you that it doesn't lead to chaos ?

Show that it always works, even with worst case crazy adjustments.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Sat, Oct 09, 2021 at 11:24:14AM -0700, Richard Cochran wrote:
> Show that it always works, even with worst case crazy adjustments.

For example:

   * large frequency adjustments are more problematic. I've checked that
     some drivers allow up to 10^6 ppm...
     This could lead to non-negligible error.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Fri, Oct 08, 2021 at 09:13:58AM +0200, Sebastien Laveze wrote:
> Of course, so what tests and measurements can we bring on the table to
> convince you that it doesn't lead to chaos ?

So, to restate what I've said before, you cannot adjust both the
physical clock and the virtual clock at the same time.

Here is a simple example that has no solution, AFAICT.

- Imagine one physical and one virtual clock based on it.

- A user space program using the virtual clock synchronizes to within
  100 nanoseconds of its upstream PTP Domain.  So far, so good.

- Now a second program using the physical clock starts up, and
  proceeds to measure, then correct the gross phase offset to its
  upstream PTP Domain.

- The driver must now add, as your proposal entails, the reverse
  correction into the virtual clock's timecounter/cyclecounter.

- However, this particular physical clock uses a RMW pattern to
  program the offset correction.

- Boom.  Now the duration of the RMW becomes an offset error in the
  virtual clock.  The magnitude may be microseconds or even
  milliseconds for devices behind slow MDIO buses, for example.

End of story.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Mon, Oct 11, 2021 at 05:58:15AM -0700, Richard Cochran wrote:
> On Fri, Oct 08, 2021 at 09:13:58AM +0200, Sebastien Laveze wrote:
> > Of course, so what tests and measurements can we bring on the table to
> > convince you that it doesn't lead to chaos ?

> - However, this particular physical clock uses a RMW pattern to
>   program the offset correction.
> 
> - Boom.  Now the duration of the RMW becomes an offset error in the
>   virtual clock.  The magnitude may be microseconds or even
>   milliseconds for devices behind slow MDIO buses, for example.

Come to think of it, even just calling clock_settime() on the physical
clock will cause trouble for the virtual clocks, and that on all
drivers.

The code would have to call gettime and figure the difference to the
new settime value, then apply the difference to the virtual clocks.  I
expect that that would cause a phase error in the microseconds for
PCIe devices.

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Mon, 2021-10-11 at 05:58 -0700, Richard Cochran wrote:
> Here is a simple example that has no solution, AFAICT.
> 
> - Imagine one physical and one virtual clock based on it.
> 
> - A user space program using the virtual clock synchronizes to within
>   100 nanoseconds of its upstream PTP Domain.  So far, so good.
> 
> - Now a second program using the physical clock starts up, and
>   proceeds to measure, then correct the gross phase offset to its
>   upstream PTP Domain.
> 
> - The driver must now add, as your proposal entails, the reverse
>   correction into the virtual clock's timecounter/cyclecounter.
> - However, this particular physical clock uses a RMW pattern to
>   program the offset correction.
> 
> - Boom.  Now the duration of the RMW becomes an offset error in the
>   virtual clock.  The magnitude may be microseconds or even
>   milliseconds for devices behind slow MDIO buses, for example.
> 

My proposal includes handling PHC offset entirely in software. There is
no way (and we agree on this :)) to change the PHC offset without
impacting children virtual clocks.

Done in software, an offset adjustment has no impact at all on virtual
clocks (since it can always be done atomically, not RMW).

So with, no hardware clock phase adjustment and limited frequency
adjustments, we believe it can be made fully transparent to virtual
clocks. And that would improve the current limitation of no adjustment
all, and would unblock the support of features like Qbv for devices
with a single clock.

Thanks,
Sebastien

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Wed, Oct 13, 2021 at 11:56:00AM +0200, Sebastien Laveze wrote:
> My proposal includes handling PHC offset entirely in software. There is
> no way (and we agree on this :)) to change the PHC offset without
> impacting children virtual clocks.

That means no control over the phase of the output signals.  Super.
 
> Done in software, an offset adjustment has no impact at all on virtual
> clocks (since it can always be done atomically, not RMW).
> 
> So with, no hardware clock phase adjustment and limited frequency
> adjustments, 

But you can't make the end users respect that.  In many cases in the
wild, the GM offset changes suddenly for various reasons, and then the
clients slew at max adjustment.  So there is no expectation of
"limited frequency adjustments."

Thanks,
Richard


> we believe it can be made fully transparent to virtual
> clocks. And that would improve the current limitation of no adjustment
> all, and would unblock the support of features like Qbv for devices
> with a single clock.
> 
> Thanks,
> Sebastien
> 

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Wed, 2021-10-13 at 06:10 -0700, Richard Cochran wrote:
> That means no control over the phase of the output signals.  Super.

You have. There's just a small conversion to go from and to the low-
level hardware counter. (Which also needs to be done for rx/tx
timestamps btw) 
When programming an output signal you use this offset to have the right
phase.

> But you can't make the end users respect that.  In many cases in the
> wild, the GM offset changes suddenly for various reasons, and then the
> clients slew at max adjustment.  So there is no expectation of
> "limited frequency adjustments."

Even if very high, there are already some limitations which are driver
specific.
We wouldn't be changing the current "regular" API but only adding some
limitations in the specific case of virtual clocks usage (added by the
user btw). This is less limitating than preventing any adjustment at
all.

Thanks,
Sebastien

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Richard Cochran]

On Wed, Oct 13, 2021 at 03:28:12PM +0200, Sebastien Laveze wrote:
> On Wed, 2021-10-13 at 06:10 -0700, Richard Cochran wrote:
> > That means no control over the phase of the output signals.  Super.
> 
> You have. There's just a small conversion to go from and to the low-
> level hardware counter. (Which also needs to be done for rx/tx
> timestamps btw) 
> When programming an output signal you use this offset to have the right
> phase.

I have an i210 with 2 periodic output channels.  How am I supposed to
generate signals from two different virtual PTP clocks?

Thanks,
Richard

Re: [PATCH net-next] ptp: add vclock timestamp conversion IOCTL

[Sebastien Laveze]

On Wed, 2021-10-13 at 10:54 -0700, Richard Cochran wrote:
> On Wed, Oct 13, 2021 at 03:28:12PM +0200, Sebastien Laveze wrote:
> > On Wed, 2021-10-13 at 06:10 -0700, Richard Cochran wrote:
> > > That means no control over the phase of the output signals.  Super.
> > 
> > You have. There's just a small conversion to go from and to the low-
> > level hardware counter. (Which also needs to be done for rx/tx
> > timestamps btw) 
> > When programming an output signal you use this offset to have the right
> > phase.
> 
> I have an i210 with 2 periodic output channels.  How am I supposed to
> generate signals from two different virtual PTP clocks?

Is it something currently supported to generate output signals from
virtual clocks ? (I would say no)

It seems to me that any periodic signal handled in hardware (scheduled
traffic for instance or periodic PPS) the hardware PHC frequency needs
to be adjusted.

For the offset, adjusting the PHC counter is not mandatory but requires
to re-configure any active hardware periodic logic. And here I'm not
saying it comes free. (especially if it would have to be supported by
all devices with PHC)

In this regard, we think that the capability to allow PHC adjustements
with virtual clocks may be on a per driver basis:
-driver exposes if it can make atomic offset adjustment or not
-if yes, allow PHC freq adjustments with a limited range. This range
can be known by userspace using PTP_CLOCK_GETCAPS. This limitation
doesn't have to be drastic but just here to prevent 10^6 ppm.

A limited adjustment remains an improvement vs no adjustment at all.

Thanks,
Sebastien