[PATCH net-next v2 0/5] ptp: Support hardware clocks with additional free running cycle counter

[PATCH net-next v2 0/5] ptp: Support hardware clocks with additional free running cycle counter

[Gerhard Engleder]

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 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 TAPRIO. In parallel vclocks can be used to
synchronize other time domains.

One year ago I thought for two time domains within a TSN network also
two physical clocks are required. This would lead to new kernel
interfaces for asking for the second clock, ... . But actually for a
time triggered system like TSN there can be only one time domain that
controls the system itself. All other time domains belong to other
layers, but not to the time triggered system itself. So other time
domains can be based on a free running counter if similar mechanisms
like 2 step synchroisation are used.

Synchronisation was tested with two time domains between two directly
connected hosts. Each host run two ptp4l instances, the first used the
physical clock and the second used the virtual clock. I used my FPGA
based network controller as network device. ptp4l was used in
combination with the virtual clock support patches from Miroslav
Lichvar.

v2:
- rename ptp_clock cycles to has_cycles (Richard Cochran)
- call it free running cycle counter (Richard Cochran)
- update struct skb_shared_hwtstamps kdoc (Richard Cochran)
- optimize timestamp address/cookie processing path (Richard Cochran,
  Vinicius Costa Gomes)

v1:
- complete rework based on suggestions (Richard Cochran)

Gerhard Engleder (5):
  ptp: Add cycles support for virtual clocks
  ptp: Request cycles for TX timestamp
  ptp: Pass hwtstamp to ptp_convert_timestamp()
  ptp: Support late timestamp determination
  tsnep: Add free running cycle counter support

 drivers/net/ethernet/engleder/tsnep_hw.h   |  9 +++--
 drivers/net/ethernet/engleder/tsnep_main.c | 31 +++++++++++++---
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 28 +++++++++++++++
 drivers/ptp/ptp_clock.c                    | 31 +++++++++++++---
 drivers/ptp/ptp_private.h                  | 10 ++++++
 drivers/ptp/ptp_sysfs.c                    | 10 +++---
 drivers/ptp/ptp_vclock.c                   | 18 ++++------
 include/linux/netdevice.h                  | 21 +++++++++++
 include/linux/ptp_clock_kernel.h           | 38 +++++++++++++++++---
 include/linux/skbuff.h                     | 14 ++++++--
 net/core/skbuff.c                          |  2 ++
 net/socket.c                               | 41 +++++++++++++++-------
 12 files changed, 208 insertions(+), 45 deletions(-)

-- 
2.20.1

[PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks

[Gerhard Engleder]

ptp vclocks require a free running time for their 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 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 TAPRIO. In parallel vclocks can be used to
synchronize other time domains.

Introduce support for a free running cycle counter called cycles to
physical clocks. Rework ptp vclocks to use this free running cycle
counter. Default implementation is based on time of physical clock.
Thus, behavior of ptp vclocks based on physical clocks without free
running cycle counter is identical to previous behavior.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_clock.c          | 31 +++++++++++++++++++++++++++----
 drivers/ptp/ptp_private.h        | 10 ++++++++++
 drivers/ptp/ptp_sysfs.c          | 10 ++++++----
 drivers/ptp/ptp_vclock.c         | 13 +++++--------
 include/linux/ptp_clock_kernel.h | 31 +++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b6f2cfd15dd2..11b8190807c3 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -77,8 +77,8 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp
 {
 	struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -103,8 +103,8 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 	struct ptp_clock_info *ops;
 	int err = -EOPNOTSUPP;
 
-	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
+	if (ptp_clock_freerun(ptp)) {
+		pr_err("ptp: physical clock is free running\n");
 		return -EBUSY;
 	}
 
@@ -178,6 +178,14 @@ static void ptp_clock_release(struct device *dev)
 	kfree(ptp);
 }
 
+static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts)
+{
+	if (info->getcyclesx64)
+		return info->getcyclesx64(info, ts, NULL);
+	else
+		return info->gettime64(info, ts);
+}
+
 static void ptp_aux_kworker(struct kthread_work *work)
 {
 	struct ptp_clock *ptp = container_of(work, struct ptp_clock,
@@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->n_vclocks_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
+	if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
+		/* Free running cycle counter not supported, use time. */
+		ptp->info->getcycles64 = ptp_getcycles64;
+
+		if (ptp->info->gettimex64)
+			ptp->info->getcyclesx64 = ptp->info->gettimex64;
+
+		if (ptp->info->getcrosststamp)
+			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
+	} else {
+		ptp->has_cycles = true;
+		if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
+			ptp->info->getcycles64 = ptp_getcycles64;
+	}
+
 	if (ptp->info->do_aux_work) {
 		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
 		ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index dba6be477067..ab47c10b3874 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -52,6 +52,7 @@ struct ptp_clock {
 	int *vclock_index;
 	struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */
 	bool is_virtual_clock;
+	bool has_cycles;
 };
 
 #define info_to_vclock(d) container_of((d), struct ptp_vclock, info)
@@ -96,6 +97,15 @@ static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 	return in_use;
 }
 
+/* Check if ptp clock shall be free running */
+static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
+{
+	if (ptp->has_cycles)
+		return false;
+
+	return ptp_vclock_in_use(ptp);
+}
+
 extern struct class *ptp_class;
 
 /*
diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 9233bfedeb17..414a70d32571 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
 			*(ptp->vclock_index + ptp->n_vclocks - i) = -1;
 	}
 
-	if (num == 0)
-		dev_info(dev, "only physical clock in use now\n");
-	else
-		dev_info(dev, "guarantee physical clock free running\n");
+	if (!ptp->has_cycles) {
+		if (num == 0)
+			dev_info(dev, "only physical clock in use now\n");
+		else
+			dev_info(dev, "guarantee physical clock free running\n");
+	}
 
 	ptp->n_vclocks = num;
 	mutex_unlock(&ptp->n_vclocks_mux);
diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index cb179a3ea508..3a095eab9cc5 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -68,7 +68,7 @@ static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->gettimex64(pptp->info, &pts, sts);
+	err = pptp->info->getcyclesx64(pptp->info, &pts, sts);
 	if (err)
 		return err;
 
@@ -104,7 +104,7 @@ static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
 	int err;
 	u64 ns;
 
-	err = pptp->info->getcrosststamp(pptp->info, xtstamp);
+	err = pptp->info->getcrosscycles(pptp->info, xtstamp);
 	if (err)
 		return err;
 
@@ -143,10 +143,7 @@ static u64 ptp_vclock_read(const struct cyclecounter *cc)
 	struct ptp_clock *ptp = vclock->pclock;
 	struct timespec64 ts = {};
 
-	if (ptp->info->gettimex64)
-		ptp->info->gettimex64(ptp->info, &ts, NULL);
-	else
-		ptp->info->gettime64(ptp->info, &ts);
+	ptp->info->getcycles64(ptp->info, &ts);
 
 	return timespec64_to_ns(&ts);
 }
@@ -168,11 +165,11 @@ 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->getcyclesx64)
 		vclock->info.gettimex64 = ptp_vclock_gettimex;
 	else
 		vclock->info.gettime64 = ptp_vclock_gettime;
-	if (pclock->info->getcrosststamp)
+	if (pclock->info->getcrosscycles)
 		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..3ea7110a9d70 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -108,6 +108,32 @@ struct ptp_system_timestamp {
  * @settime64:  Set the current time on the hardware clock.
  *              parameter ts: Time value to set.
  *
+ * @getcycles64:  Reads the current free running cycle counter from the hardware
+ *                clock.
+ *                If @getcycles64 and @getcyclesx64 are not supported, then
+ *                @gettime64 or @gettimex64 will be used as default
+ *                implementation.
+ *                parameter ts: Holds the result.
+ *
+ * @getcyclesx64:  Reads the current free running cycle counter from the
+ *                 hardware clock and optionally also the system clock.
+ *                 If @getcycles64 and @getcyclesx64 are not supported, then
+ *                 @gettimex64 will be used as default implementation if
+ *                 available.
+ *                 parameter ts: Holds the PHC 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 timestamp and the second
+ *                 reading immediately follows that.
+ *
+ * @getcrosscycles:  Reads the current free running cycle counter from the
+ *                   hardware clock and system clock simultaneously.
+ *                   If @getcycles64 and @getcyclesx64 are not supported, then
+ *                   @getcrosststamp will be used as default implementation if
+ *                   available.
+ *                   parameter cts: Contains timestamp (device,system) pair,
+ *                   where 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 +181,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 (*getcycles64)(struct ptp_clock_info *ptp, struct timespec64 *ts);
+	int (*getcyclesx64)(struct ptp_clock_info *ptp, struct timespec64 *ts,
+			    struct ptp_system_timestamp *sts);
+	int (*getcrosscycles)(struct ptp_clock_info *ptp,
+			      struct system_device_crosststamp *cts);
 	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

Re: [PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks

[Richard Cochran]

On Sun, Apr 03, 2022 at 07:55:40PM +0200, Gerhard Engleder wrote:

> @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
>  	mutex_init(&ptp->n_vclocks_mux);
>  	init_waitqueue_head(&ptp->tsev_wq);
>  
> +	if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {

Please swap blocks, using non-negated logical test:

	if (ptp->info->getcycles64 || ptp->info->getcyclesx64)

> +		/* Free running cycle counter not supported, use time. */
> +		ptp->info->getcycles64 = ptp_getcycles64;
> +
> +		if (ptp->info->gettimex64)
> +			ptp->info->getcyclesx64 = ptp->info->gettimex64;
> +
> +		if (ptp->info->getcrosststamp)
> +			ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> +	} else {
> +		ptp->has_cycles = true;
> +		if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> +			ptp->info->getcycles64 = ptp_getcycles64;
> +	}
> +
>  	if (ptp->info->do_aux_work) {
>  		kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
>  		ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);


> @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
>  			*(ptp->vclock_index + ptp->n_vclocks - i) = -1;
>  	}
>  
> -	if (num == 0)
> -		dev_info(dev, "only physical clock in use now\n");
> -	else
> -		dev_info(dev, "guarantee physical clock free running\n");
> +	if (!ptp->has_cycles) {

Not sure what this test means ...

> +		if (num == 0)
> +			dev_info(dev, "only physical clock in use now\n");

Shouldn't this one print even if has_cycles == false?

> +		else
> +			dev_info(dev, "guarantee physical clock free running\n");
> +	}
>  
>  	ptp->n_vclocks = num;
>  	mutex_unlock(&ptp->n_vclocks_mux);

Thanks,
Richard

Re: [PATCH net-next v2 1/5] ptp: Add cycles support for virtual clocks

[Gerhard Engleder]

> > @@ -225,6 +233,21 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
> >       mutex_init(&ptp->n_vclocks_mux);
> >       init_waitqueue_head(&ptp->tsev_wq);
> >
> > +     if (!ptp->info->getcycles64 && !ptp->info->getcyclesx64) {
>
> Please swap blocks, using non-negated logical test:
>
>         if (ptp->info->getcycles64 || ptp->info->getcyclesx64)

I will change it as suggested.

> > +             /* Free running cycle counter not supported, use time. */
> > +             ptp->info->getcycles64 = ptp_getcycles64;
> > +
> > +             if (ptp->info->gettimex64)
> > +                     ptp->info->getcyclesx64 = ptp->info->gettimex64;
> > +
> > +             if (ptp->info->getcrosststamp)
> > +                     ptp->info->getcrosscycles = ptp->info->getcrosststamp;
> > +     } else {
> > +             ptp->has_cycles = true;
> > +             if (!ptp->info->getcycles64 && ptp->info->getcyclesx64)
> > +                     ptp->info->getcycles64 = ptp_getcycles64;
> > +     }
> > +
> >       if (ptp->info->do_aux_work) {
> >               kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker);
> >               ptp->kworker = kthread_create_worker(0, "ptp%d", ptp->index);
>
>
> > @@ -231,10 +231,12 @@ static ssize_t n_vclocks_store(struct device *dev,
> >                       *(ptp->vclock_index + ptp->n_vclocks - i) = -1;
> >       }
> >
> > -     if (num == 0)
> > -             dev_info(dev, "only physical clock in use now\n");
> > -     else
> > -             dev_info(dev, "guarantee physical clock free running\n");
> > +     if (!ptp->has_cycles) {
>
> Not sure what this test means ...

I thought these dev_info() are useless if the free running cycle
counter is supported,
because the behavior of the physical clock does not change in this case.

> > +             if (num == 0)
> > +                     dev_info(dev, "only physical clock in use now\n");
>
> Shouldn't this one print even if has_cycles == false?

It will print if has_cycles == false.

In my opinion this dev_info() tells the user that the physical clock can be used
again. So it does not carry any interesting information if has_cycles == true.

> > +             else
> > +                     dev_info(dev, "guarantee physical clock free running\n");
> > +     }
> >
> >       ptp->n_vclocks = num;
> >       mutex_unlock(&ptp->n_vclocks_mux);

Thank you!

Gerhard

[PATCH net-next v2 2/5] ptp: Request cycles for TX timestamp

[Gerhard Engleder]

The free running cycle counter of physical clocks called cycles shall be
used for hardware timestamps to enable synchronisation.

Introduce new flag SKBTX_HW_TSTAMP_USE_CYCLES, which signals driver to
provide a TX timestamp based on cycles if cycles are supported.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 include/linux/skbuff.h |  3 +++
 net/core/skbuff.c      |  2 ++
 net/socket.c           | 11 ++++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3a30cae8b0a5..aeb3ed4d6cf8 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -578,6 +578,9 @@ enum {
 	/* device driver is going to provide hardware time stamp */
 	SKBTX_IN_PROGRESS = 1 << 2,
 
+	/* generate hardware time stamp based on cycles if supported */
+	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
+
 	/* generate wifi status information (where possible) */
 	SKBTX_WIFI_STATUS = 1 << 4,
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 10bde7c6db44..c0f8f1341c3f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4847,6 +4847,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
 		skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
 					     SKBTX_ANY_TSTAMP;
 		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
+	} else {
+		skb_shinfo(skb)->tx_flags &= ~SKBTX_HW_TSTAMP_USE_CYCLES;
 	}
 
 	if (hwtstamps)
diff --git a/net/socket.c b/net/socket.c
index 6887840682bb..03911a3d8b33 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -683,9 +683,18 @@ void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags)
 {
 	u8 flags = *tx_flags;
 
-	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE)
+	if (tsflags & SOF_TIMESTAMPING_TX_HARDWARE) {
 		flags |= SKBTX_HW_TSTAMP;
 
+		/* PTP hardware clocks can provide a free running cycle counter
+		 * as a time base for virtual clocks. Tell driver to use the
+		 * free running cycle counter for timestamp if socket is bound
+		 * to virtual clock.
+		 */
+		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
+			flags |= SKBTX_HW_TSTAMP_USE_CYCLES;
+	}
+
 	if (tsflags & SOF_TIMESTAMPING_TX_SOFTWARE)
 		flags |= SKBTX_SW_TSTAMP;
 
-- 
2.20.1

Re: [PATCH net-next v2 2/5] ptp: Request cycles for TX timestamp

[Richard Cochran]

On Sun, Apr 03, 2022 at 07:55:41PM +0200, Gerhard Engleder wrote:
> The free running cycle counter of physical clocks called cycles shall be
> used for hardware timestamps to enable synchronisation.
> 
> Introduce new flag SKBTX_HW_TSTAMP_USE_CYCLES, which signals driver to
> provide a TX timestamp based on cycles if cycles are supported.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  include/linux/skbuff.h |  3 +++
>  net/core/skbuff.c      |  2 ++
>  net/socket.c           | 11 ++++++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 3a30cae8b0a5..aeb3ed4d6cf8 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -578,6 +578,9 @@ enum {
>  	/* device driver is going to provide hardware time stamp */
>  	SKBTX_IN_PROGRESS = 1 << 2,
>  
> +	/* generate hardware time stamp based on cycles if supported */
> +	SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
> +
>  	/* generate wifi status information (where possible) */
>  	SKBTX_WIFI_STATUS = 1 << 4,
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 10bde7c6db44..c0f8f1341c3f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4847,6 +4847,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>  		skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
>  					     SKBTX_ANY_TSTAMP;
>  		skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
> +	} else {
> +		skb_shinfo(skb)->tx_flags &= ~SKBTX_HW_TSTAMP_USE_CYCLES;

Why is this needed?

>  	}
>  
>  	if (hwtstamps)

Thanks,
Richard

Re: [PATCH net-next v2 2/5] ptp: Request cycles for TX timestamp

[Gerhard Engleder]

> > The free running cycle counter of physical clocks called cycles shall be
> > used for hardware timestamps to enable synchronisation.
> >
> > Introduce new flag SKBTX_HW_TSTAMP_USE_CYCLES, which signals driver to
> > provide a TX timestamp based on cycles if cycles are supported.
> >
> > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> > ---
> >  include/linux/skbuff.h |  3 +++
> >  net/core/skbuff.c      |  2 ++
> >  net/socket.c           | 11 ++++++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 3a30cae8b0a5..aeb3ed4d6cf8 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -578,6 +578,9 @@ enum {
> >       /* device driver is going to provide hardware time stamp */
> >       SKBTX_IN_PROGRESS = 1 << 2,
> >
> > +     /* generate hardware time stamp based on cycles if supported */
> > +     SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3,
> > +
> >       /* generate wifi status information (where possible) */
> >       SKBTX_WIFI_STATUS = 1 << 4,
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 10bde7c6db44..c0f8f1341c3f 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -4847,6 +4847,8 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
> >               skb_shinfo(skb)->tx_flags |= skb_shinfo(orig_skb)->tx_flags &
> >                                            SKBTX_ANY_TSTAMP;
> >               skb_shinfo(skb)->tskey = skb_shinfo(orig_skb)->tskey;
> > +     } else {
> > +             skb_shinfo(skb)->tx_flags &= ~SKBTX_HW_TSTAMP_USE_CYCLES;
>
> Why is this needed?

It prevents that SKBTX_HW_TSTAMP_USE_CYCLES is set due to the call of
skb_clone(),
when the timestamp is delivered back to the socket. It lowers the flag
usage, but it is not
absolutely needed. I could remove that code.

Thank you!

Gerhard

[PATCH net-next v2 3/5] ptp: Pass hwtstamp to ptp_convert_timestamp()

[Gerhard Engleder]

ptp_convert_timestamp() converts only the timestamp hwtstamp, which is
a field of the argument with the type struct skb_shared_hwtstamps *. So
a pointer to the hwtstamp field of this structure is sufficient.

Rework ptp_convert_timestamp() to use an argument of type ktime_t *.
This allows to add additional timestamp manipulation stages before the
call of ptp_convert_timestamp().

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_vclock.c         | 5 ++---
 include/linux/ptp_clock_kernel.h | 7 +++----
 net/socket.c                     | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 3a095eab9cc5..c30bcce2bb43 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -232,8 +232,7 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 }
 EXPORT_SYMBOL(ptp_get_vclocks_index);
 
-ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
-			      int vclock_index)
+ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index)
 {
 	char name[PTP_CLOCK_NAME_LEN] = "";
 	struct ptp_vclock *vclock;
@@ -255,7 +254,7 @@ ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
 
 	vclock = info_to_vclock(ptp->info);
 
-	ns = ktime_to_ns(hwtstamps->hwtstamp);
+	ns = ktime_to_ns(*hwtstamp);
 
 	spin_lock_irqsave(&vclock->lock, flags);
 	ns = timecounter_cyc2time(&vclock->tc, ns);
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 3ea7110a9d70..ed6e15ead466 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -380,17 +380,16 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index);
 /**
  * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
  *
- * @hwtstamps:    skb_shared_hwtstamps structure pointer
+ * @hwtstamp:     timestamp
  * @vclock_index: phc index of ptp vclock.
  *
  * Returns converted timestamp, or 0 on error.
  */
-ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
-			      int vclock_index);
+ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
 #else
 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
 { return 0; }
-static inline ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
+static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
 					    int vclock_index)
 { return 0; }
 
diff --git a/net/socket.c b/net/socket.c
index 03911a3d8b33..4801aeaeb285 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -888,7 +888,7 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
 	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
 		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
-			hwtstamp = ptp_convert_timestamp(shhwtstamps,
+			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
 							 sk->sk_bind_phc);
 		else
 			hwtstamp = shhwtstamps->hwtstamp;
-- 
2.20.1

[PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

If a physical clock supports a free running cycle counter, then
timestamps shall be based on this time too. For TX it is known in
advance before the transmission if a timestamp based on the free running
cycle counter is needed. For RX it is impossible to know which timestamp
is needed before the packet is received and assigned to a socket.

Support late timestamp determination by a network device. Therefore, an
address/cookie is stored within the new netdev_data field of struct
skb_shared_hwtstamps. This address/cookie is provided to a new network
device function called ndo_get_tstamp(), which returns a timestamp based
on the normal/adjustable time or based on the free running cycle
counter. If function is not supported, then timestamp handling is not
changed.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 include/linux/netdevice.h | 21 +++++++++++++++++++++
 include/linux/skbuff.h    | 11 ++++++++---
 net/socket.c              | 30 +++++++++++++++++++-----------
 3 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 59e27a2b7bf0..f6cc4c673082 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1353,6 +1353,12 @@ struct netdev_net_notifier {
  *	The caller must be under RCU read context.
  * int (*ndo_fill_forward_path)(struct net_device_path_ctx *ctx, struct net_device_path *path);
  *     Get the forwarding path to reach the real device from the HW destination address
+ * ktime_t (*ndo_get_tstamp)(struct net_device *dev,
+ *			     const struct skb_shared_hwtstamps *hwtstamps,
+ *			     bool cycles);
+ *	Get hardware timestamp based on normal/adjustable time or free running
+ *	cycle counter. This function is required if physical clock supports a
+ *	free running cycle counter.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1570,6 +1576,9 @@ struct net_device_ops {
 	struct net_device *	(*ndo_get_peer_dev)(struct net_device *dev);
 	int                     (*ndo_fill_forward_path)(struct net_device_path_ctx *ctx,
                                                          struct net_device_path *path);
+	ktime_t			(*ndo_get_tstamp)(struct net_device *dev,
+						  const struct skb_shared_hwtstamps *hwtstamps,
+						  bool cycles);
 };
 
 /**
@@ -4764,6 +4773,18 @@ static inline void netdev_rx_csum_fault(struct net_device *dev,
 void net_enable_timestamp(void);
 void net_disable_timestamp(void);
 
+static inline ktime_t netdev_get_tstamp(struct net_device *dev,
+					const struct skb_shared_hwtstamps *hwtstamps,
+					bool cycles)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+
+	if (ops->ndo_get_tstamp)
+		return ops->ndo_get_tstamp(dev, hwtstamps, cycles);
+
+	return hwtstamps->hwtstamp;
+}
+
 #ifdef CONFIG_PROC_FS
 int __init dev_proc_init(void);
 #else
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index aeb3ed4d6cf8..c428b678e7f1 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -551,8 +551,10 @@ static inline bool skb_frag_must_loop(struct page *p)
 
 /**
  * struct skb_shared_hwtstamps - hardware time stamps
- * @hwtstamp:	hardware time stamp transformed into duration
- *		since arbitrary point in time
+ * @hwtstamp:		hardware time stamp transformed into duration
+ *			since arbitrary point in time
+ * @netdev_data:	address/cookie of network device driver used as
+ *			reference to actual hardware time stamp
  *
  * Software time stamps generated by ktime_get_real() are stored in
  * skb->tstamp.
@@ -564,7 +566,10 @@ static inline bool skb_frag_must_loop(struct page *p)
  * &skb_shared_info. Use skb_hwtstamps() to get a pointer.
  */
 struct skb_shared_hwtstamps {
-	ktime_t	hwtstamp;
+	union {
+		ktime_t	hwtstamp;
+		void *netdev_data;
+	};
 };
 
 /* Definitions for tx_flags in struct skb_shared_info */
diff --git a/net/socket.c b/net/socket.c
index 4801aeaeb285..d64bd3dfcf6a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -805,21 +805,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp)
 	return skb->tstamp && !false_tstamp && skb_is_err_queue(skb);
 }
 
-static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb)
+static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb,
+			   int if_index)
 {
 	struct scm_ts_pktinfo ts_pktinfo;
-	struct net_device *orig_dev;
 
 	if (!skb_mac_header_was_set(skb))
 		return;
 
 	memset(&ts_pktinfo, 0, sizeof(ts_pktinfo));
 
-	rcu_read_lock();
-	orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
-	if (orig_dev)
-		ts_pktinfo.if_index = orig_dev->ifindex;
-	rcu_read_unlock();
+	ts_pktinfo.if_index = if_index;
 
 	ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb);
 	put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO,
@@ -839,6 +835,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	int empty = 1, false_tstamp = 0;
 	struct skb_shared_hwtstamps *shhwtstamps =
 		skb_hwtstamps(skb);
+	struct net_device *orig_dev;
+	int if_index;
 	ktime_t hwtstamp;
 
 	/* Race occurred between timestamp enabling and packet
@@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
 	if (shhwtstamps &&
 	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
 	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
+		rcu_read_lock();
+		orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
+		if (orig_dev) {
+			if_index = orig_dev->ifindex;
+			hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps,
+						     sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC);
+		} else {
+			if_index = 0;
+			hwtstamp = shhwtstamps->hwtstamp;
+		}
+		rcu_read_unlock();
+
 		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
-			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
+			hwtstamp = ptp_convert_timestamp(&hwtstamp,
 							 sk->sk_bind_phc);
-		else
-			hwtstamp = shhwtstamps->hwtstamp;
 
 		if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) {
 			empty = 0;
 
 			if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) &&
 			    !skb_is_err_queue(skb))
-				put_ts_pktinfo(msg, skb);
+				put_ts_pktinfo(msg, skb, if_index);
 		}
 	}
 	if (!empty) {
-- 
2.20.1

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Richard Cochran]

On Sun, Apr 03, 2022 at 07:55:43PM +0200, Gerhard Engleder wrote:

> @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>  	if (shhwtstamps &&
>  	    (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>  	    !skb_is_swtx_tstamp(skb, false_tstamp)) {
> +		rcu_read_lock();
> +		orig_dev = dev_get_by_napi_id(skb_napi_id(skb));

__sock_recv_timestamp() is hot path.

No need to call dev_get_by_napi_id() for the vast majority of cases
using plain old MAC time stamping.

Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).

Thanks,
Richard

> +		if (orig_dev) {
> +			if_index = orig_dev->ifindex;
> +			hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps,
> +						     sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC);
> +		} else {
> +			if_index = 0;
> +			hwtstamp = shhwtstamps->hwtstamp;
> +		}
> +		rcu_read_unlock();
> +
>  		if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC)
> -			hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp,
> +			hwtstamp = ptp_convert_timestamp(&hwtstamp,
>  							 sk->sk_bind_phc);
> -		else
> -			hwtstamp = shhwtstamps->hwtstamp;
>  
>  		if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) {
>  			empty = 0;
>  
>  			if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) &&
>  			    !skb_is_err_queue(skb))
> -				put_ts_pktinfo(msg, skb);
> +				put_ts_pktinfo(msg, skb, if_index);
>  		}
>  	}
>  	if (!empty) {
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> >       if (shhwtstamps &&
> >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > +             rcu_read_lock();
> > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
>
> __sock_recv_timestamp() is hot path.
>
> No need to call dev_get_by_napi_id() for the vast majority of cases
> using plain old MAC time stamping.

Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
That's the reason for the removal of a separate flag, which signals the need to
timestamp determination based on address/cookie. I thought there is no need
for that flag, as netdev is already available later in the existing code.

> Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).

This flag tells netdev_get_tstamp() which timestamp is required. If it
is not set, then
netdev_get_tstamp() has to deliver the normal timestamp as always. But
this normal
timestamp is only available via address/cookie. So netdev_get_tstamp() must be
called.

Thank you!

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Richard Cochran]

On Sun, Apr 10, 2022 at 02:54:36PM +0200, Gerhard Engleder wrote:
> > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > >       if (shhwtstamps &&
> > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > > +             rcu_read_lock();
> > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> >
> > __sock_recv_timestamp() is hot path.
> >
> > No need to call dev_get_by_napi_id() for the vast majority of cases
> > using plain old MAC time stamping.
> 
> Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?

No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.

> That's the reason for the removal of a separate flag, which signals the need to
> timestamp determination based on address/cookie. I thought there is no need
> for that flag, as netdev is already available later in the existing code.
> 
> > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
> 
> This flag tells netdev_get_tstamp() which timestamp is required. If it
> is not set, then
> netdev_get_tstamp() has to deliver the normal timestamp as always. But
> this normal
> timestamp is only available via address/cookie. So netdev_get_tstamp() must be
> called.

It should be this:

- normal, non-vclock:	use hwtstamps->hwtstamp directly
- vclock:		use slower path with lookup

I don't see why you can't implement that.


Thanks,
Richard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > > >       if (shhwtstamps &&
> > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > > > +             rcu_read_lock();
> > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > >
> > > __sock_recv_timestamp() is hot path.
> > >
> > > No need to call dev_get_by_napi_id() for the vast majority of cases
> > > using plain old MAC time stamping.
> >
> > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
>
> No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.

You are right, my fault.

> > That's the reason for the removal of a separate flag, which signals the need to
> > timestamp determination based on address/cookie. I thought there is no need
> > for that flag, as netdev is already available later in the existing code.
> >
> > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
> >
> > This flag tells netdev_get_tstamp() which timestamp is required. If it
> > is not set, then
> > netdev_get_tstamp() has to deliver the normal timestamp as always. But
> > this normal
> > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
> > called.
>
> It should be this:
>
> - normal, non-vclock:   use hwtstamps->hwtstamp directly
> - vclock:               use slower path with lookup
>
> I don't see why you can't implement that.

I will try to implement it that way.

Thank you!

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> > > > >       if (shhwtstamps &&
> > > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> > > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> > > > > +             rcu_read_lock();
> > > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> > > >
> > > > __sock_recv_timestamp() is hot path.
> > > >
> > > > No need to call dev_get_by_napi_id() for the vast majority of cases
> > > > using plain old MAC time stamping.
> > >
> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
> >
> > No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.
>
> You are right, my fault.
>
> > > That's the reason for the removal of a separate flag, which signals the need to
> > > timestamp determination based on address/cookie. I thought there is no need
> > > for that flag, as netdev is already available later in the existing code.
> > >
> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
> > >
> > > This flag tells netdev_get_tstamp() which timestamp is required. If it
> > > is not set, then
> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But
> > > this normal
> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
> > > called.
> >
> > It should be this:
> >
> > - normal, non-vclock:   use hwtstamps->hwtstamp directly
> > - vclock:               use slower path with lookup
> >
> > I don't see why you can't implement that.
>
> I will try to implement it that way.

I'm thinking about why there should be a slow path with lookup. If the
address/cookie
points to a defined data structure with two timestamps, then no lookup
for the phc or
netdev is necessary. It should be possible for every driver to
allocate a skbuff with enough
space for this structure in front of the received Ethernet frame. The
structure could be:

struct skb_inline_hwtstamps {
        ktime_t hwtstamp;
        ktime_t hwcstamp;
};

Actually my device and igc are storing the timestamps in front of the
received Ethernet
frame. In my opinion it is obvious to the store metadata of received
Ethernet frames in
front of it, because it eliminates the need for another DMA transfer.
What is your opinion
Vinicius?

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Vinicius Costa Gomes]

Gerhard Engleder <gerhard@engleder-embedded.com> writes:

>> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
>> > > > >       if (shhwtstamps &&
>> > > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
>> > > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
>> > > > > +             rcu_read_lock();
>> > > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
>> > > >
>> > > > __sock_recv_timestamp() is hot path.
>> > > >
>> > > > No need to call dev_get_by_napi_id() for the vast majority of cases
>> > > > using plain old MAC time stamping.
>> > >
>> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
>> >
>> > No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.
>>
>> You are right, my fault.
>>
>> > > That's the reason for the removal of a separate flag, which signals the need to
>> > > timestamp determination based on address/cookie. I thought there is no need
>> > > for that flag, as netdev is already available later in the existing code.
>> > >
>> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
>> > >
>> > > This flag tells netdev_get_tstamp() which timestamp is required. If it
>> > > is not set, then
>> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But
>> > > this normal
>> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
>> > > called.
>> >
>> > It should be this:
>> >
>> > - normal, non-vclock:   use hwtstamps->hwtstamp directly
>> > - vclock:               use slower path with lookup
>> >
>> > I don't see why you can't implement that.
>>
>> I will try to implement it that way.
>
> I'm thinking about why there should be a slow path with lookup. If the
> address/cookie
> points to a defined data structure with two timestamps, then no lookup
> for the phc or
> netdev is necessary. It should be possible for every driver to
> allocate a skbuff with enough
> space for this structure in front of the received Ethernet frame. The
> structure could be:
>
> struct skb_inline_hwtstamps {
>         ktime_t hwtstamp;
>         ktime_t hwcstamp;
> };
>
> Actually my device and igc are storing the timestamps in front of the
> received Ethernet
> frame. In my opinion it is obvious to the store metadata of received
> Ethernet frames in
> front of it, because it eliminates the need for another DMA transfer.
> What is your opinion
> Vinicius?

If I am understanding this right, the idea is providing both "cycles"
(free running cycles measurement) and PHC timestamp for all packets, for
igc, it will work fine for RX (the HW already writes the timestamps for
two timer registers in the host memory), but for TX it's going be
awkward/slow (I would have to read two extra registers), but I think
it's still possible.

But it would be best to avoid the overhead, and only providing the
"extra" (the cycles one) measurement if necessary for TX, so
SKBTX_HW_TSTAMP_USE_CYCLES would still be needed.

So, in short, I am fine with it, as long as I can get away with only
providing the cycles measurement for TX if necessary.

>
> Gerhard

Cheers,
-- 
Vinicius

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> >> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk,
> >> > > > >       if (shhwtstamps &&
> >> > > > >           (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
> >> > > > >           !skb_is_swtx_tstamp(skb, false_tstamp)) {
> >> > > > > +             rcu_read_lock();
> >> > > > > +             orig_dev = dev_get_by_napi_id(skb_napi_id(skb));
> >> > > >
> >> > > > __sock_recv_timestamp() is hot path.
> >> > > >
> >> > > > No need to call dev_get_by_napi_id() for the vast majority of cases
> >> > > > using plain old MAC time stamping.
> >> > >
> >> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()?
> >> >
> >> > No.  Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested.
> >>
> >> You are right, my fault.
> >>
> >> > > That's the reason for the removal of a separate flag, which signals the need to
> >> > > timestamp determination based on address/cookie. I thought there is no need
> >> > > for that flag, as netdev is already available later in the existing code.
> >> > >
> >> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC).
> >> > >
> >> > > This flag tells netdev_get_tstamp() which timestamp is required. If it
> >> > > is not set, then
> >> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But
> >> > > this normal
> >> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be
> >> > > called.
> >> >
> >> > It should be this:
> >> >
> >> > - normal, non-vclock:   use hwtstamps->hwtstamp directly
> >> > - vclock:               use slower path with lookup
> >> >
> >> > I don't see why you can't implement that.
> >>
> >> I will try to implement it that way.
> >
> > I'm thinking about why there should be a slow path with lookup. If the
> > address/cookie
> > points to a defined data structure with two timestamps, then no lookup
> > for the phc or
> > netdev is necessary. It should be possible for every driver to
> > allocate a skbuff with enough
> > space for this structure
On Tue, Apr 12, 2022 at 11:05 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Gerhard Engleder <gerhard@engleder-embedded.com> writes:
> in front of the received Ethernet frame. The
> > structure could be:
> >
> > struct skb_inline_hwtstamps {
> >         ktime_t hwtstamp;
> >         ktime_t hwcstamp;
> > };
> >
> > Actually my device and igc are storing the timestamps in front of the
> > received Ethernet
> > frame. In my opinion it is obvious to the store metadata of received
> > Ethernet frames in
> > front of it, because it eliminates the need for another DMA transfer.
> > What is your opinion
> > Vinicius?
>
> If I am understanding this right, the idea is providing both "cycles"
> (free running cycles measurement) and PHC timestamp for all packets, for
> igc, it will work fine for RX (the HW already writes the timestamps for
> two timer registers in the host memory), but for TX it's going be
> awkward/slow (I would have to read two extra registers), but I think
> it's still possible.
>
> But it would be best to avoid the overhead, and only providing the
> "extra" (the cycles one) measurement if necessary for TX, so
> SKBTX_HW_TSTAMP_USE_CYCLES would still be needed.
>
> So, in short, I am fine with it, as long as I can get away with only
> providing the cycles measurement for TX if necessary.

Of course for TX only cycles measurement shall be provided and
SKBTX_HW_TSTAMP_USE_CYCLES is used for that.

Thanks!

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Richard Cochran]

On Tue, Apr 12, 2022 at 09:24:10PM +0200, Gerhard Engleder wrote:
> I'm thinking about why there should be a slow path with lookup. If the
> address/cookie
> points to a defined data structure with two timestamps, then no lookup
> for the phc or
> netdev is necessary. It should be possible for every driver to
> allocate a skbuff with enough
> space for this structure in front of the received Ethernet frame.

Adding 16 bytes for every allocated skbuff is going to be a tough
sell.  Most people don't want/need this.

Thanks,
Richard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> > I'm thinking about why there should be a slow path with lookup. If the
> > address/cookie
> > points to a defined data structure with two timestamps, then no lookup
> > for the phc or
> > netdev is necessary. It should be possible for every driver to
> > allocate a skbuff with enough
> > space for this structure in front of the received Ethernet frame.
>
> Adding 16 bytes for every allocated skbuff is going to be a tough
> sell.  Most people don't want/need this.

Most people are not affected because they use drivers which do not
support cycles. Those drivers stay the same, no 16 bytes are added.
For TX those 16 bytes are not added, because SKBTX_HW_TSTAMP_USE_CYCLES
is used to fill in the right time stamp.

For igc and tsnep the 16 bytes in front of the RX frame exist anyway.
So this would be a minimal solution with best performance as a first
step. A lookup for netdev/phc can be added in the future if there is
a driver, which needs that.

Is it worth posting an implementation in that direction?

Thanks!

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Richard Cochran]

On Wed, Apr 13, 2022 at 10:51:54PM +0200, Gerhard Engleder wrote:
> For igc and tsnep the 16 bytes in front of the RX frame exist anyway.
> So this would be a minimal solution with best performance as a first
> step. A lookup for netdev/phc can be added in the future if there is
> a driver, which needs that.

It is a design mistake to base new kernel interfaces on hardware
quirks.

> Is it worth posting an implementation in that direction?

Sure, but please make thoughts about how this would work for the
non-igc world.

IIRC one of the nxp switches also has such counters?  You can start
with that.

Thanks,
Richard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Gerhard Engleder]

> > For igc and tsnep the 16 bytes in front of the RX frame exist anyway.
> > So this would be a minimal solution with best performance as a first
> > step. A lookup for netdev/phc can be added in the future if there is
> > a driver, which needs that.
>
> It is a design mistake to base new kernel interfaces on hardware
> quirks.
>
> > Is it worth posting an implementation in that direction?
>
> Sure, but please make thoughts about how this would work for the
> non-igc world.
>
> IIRC one of the nxp switches also has such counters?  You can start
> with that.

I did some measurements (A53, 1.2GHz): netdev lookup and call to
my driver takes ~400ns. ptp_convert_timestamp() takes ~6000ns, I
assume because of class_find_device_by_name() call. So eliminating
the netdev lookup is the wrong optimisation target.

I will try to do it like that:

- normal, driver without cycles support:   use hwtstamps->hwtstamp
directly (not changed)
- driver with cycles support:   netdev lookup for address/cookie
conversion to hwtstamp (newly implemented)
- vclock:   slow path with phc lookup (not changed)

Gerhard

Re: [PATCH net-next v2 4/5] ptp: Support late timestamp determination

[Richard Cochran]

On Sat, Apr 16, 2022 at 12:04:20AM +0200, Gerhard Engleder wrote:
> - vclock:   slow path with phc lookup (not changed)

This one could really use caching.  (I know that isn't your patch, but
maybe you can try to fix it?)

Thanks,
Richard

[PATCH net-next v2 5/5] tsnep: Add free running cycle counter support

[Gerhard Engleder]

The TSN endpoint Ethernet MAC supports a free running counter
additionally to its clock. This free running counter can be read and
hardware timestamps are supported. As the name implies, this counter
cannot be set and its frequency cannot be adjusted.

Add free running cycle counter support based on this free running
counter to physical clock. This also requires hardware time stamps
based on that free running counter.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_hw.h   |  9 +++++--
 drivers/net/ethernet/engleder/tsnep_main.c | 31 ++++++++++++++++++----
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 28 +++++++++++++++++++
 3 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_hw.h b/drivers/net/ethernet/engleder/tsnep_hw.h
index 71cc8577d640..916ceac3ada2 100644
--- a/drivers/net/ethernet/engleder/tsnep_hw.h
+++ b/drivers/net/ethernet/engleder/tsnep_hw.h
@@ -43,6 +43,10 @@
 #define ECM_RESET_CHANNEL 0x00000100
 #define ECM_RESET_TXRX 0x00010000
 
+/* counter */
+#define ECM_COUNTER_LOW 0x0028
+#define ECM_COUNTER_HIGH 0x002C
+
 /* control and status */
 #define ECM_STATUS 0x0080
 #define ECM_LINK_MODE_OFF 0x01000000
@@ -190,7 +194,8 @@ struct tsnep_tx_desc {
 /* tsnep TX descriptor writeback */
 struct tsnep_tx_desc_wb {
 	__le32 properties;
-	__le32 reserved1[3];
+	__le32 reserved1;
+	__le64 counter;
 	__le64 timestamp;
 	__le32 dma_delay;
 	__le32 reserved2;
@@ -221,7 +226,7 @@ struct tsnep_rx_desc_wb {
 
 /* tsnep RX inline meta */
 struct tsnep_rx_inline {
-	__le64 reserved;
+	__le64 counter;
 	__le64 timestamp;
 };
 
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 904f3304727e..c97651903892 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -470,8 +470,15 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 		    (__le32_to_cpu(entry->desc_wb->properties) &
 		     TSNEP_DESC_EXTENDED_WRITEBACK_FLAG)) {
 			struct skb_shared_hwtstamps hwtstamps;
-			u64 timestamp =
-				__le64_to_cpu(entry->desc_wb->timestamp);
+			u64 timestamp;
+
+			if (skb_shinfo(entry->skb)->tx_flags &
+			    SKBTX_HW_TSTAMP_USE_CYCLES)
+				timestamp =
+					__le64_to_cpu(entry->desc_wb->counter);
+			else
+				timestamp =
+					__le64_to_cpu(entry->desc_wb->timestamp);
 
 			memset(&hwtstamps, 0, sizeof(hwtstamps));
 			hwtstamps.hwtstamp = ns_to_ktime(timestamp);
@@ -704,11 +711,9 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 					skb_hwtstamps(skb);
 				struct tsnep_rx_inline *rx_inline =
 					(struct tsnep_rx_inline *)skb->data;
-				u64 timestamp =
-					__le64_to_cpu(rx_inline->timestamp);
 
 				memset(hwtstamps, 0, sizeof(*hwtstamps));
-				hwtstamps->hwtstamp = ns_to_ktime(timestamp);
+				hwtstamps->netdev_data = rx_inline;
 			}
 			skb_pull(skb, TSNEP_RX_INLINE_METADATA_SIZE);
 			skb->protocol = eth_type_trans(skb,
@@ -1010,6 +1015,21 @@ static int tsnep_netdev_set_mac_address(struct net_device *netdev, void *addr)
 	return 0;
 }
 
+static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
+				       const struct skb_shared_hwtstamps *hwtstamps,
+				       bool cycles)
+{
+	struct tsnep_rx_inline *rx_inline = hwtstamps->netdev_data;
+	u64 timestamp;
+
+	if (cycles)
+		timestamp = __le64_to_cpu(rx_inline->counter);
+	else
+		timestamp = __le64_to_cpu(rx_inline->timestamp);
+
+	return ns_to_ktime(timestamp);
+}
+
 static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_open = tsnep_netdev_open,
 	.ndo_stop = tsnep_netdev_close,
@@ -1019,6 +1039,7 @@ static const struct net_device_ops tsnep_netdev_ops = {
 
 	.ndo_get_stats64 = tsnep_netdev_get_stats64,
 	.ndo_set_mac_address = tsnep_netdev_set_mac_address,
+	.ndo_get_tstamp = tsnep_netdev_get_tstamp,
 	.ndo_setup_tc = tsnep_tc_setup,
 };
 
diff --git a/drivers/net/ethernet/engleder/tsnep_ptp.c b/drivers/net/ethernet/engleder/tsnep_ptp.c
index eaad453d487e..54fbf0126815 100644
--- a/drivers/net/ethernet/engleder/tsnep_ptp.c
+++ b/drivers/net/ethernet/engleder/tsnep_ptp.c
@@ -175,6 +175,33 @@ static int tsnep_ptp_settime64(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int tsnep_ptp_getcyclesx64(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  struct ptp_system_timestamp *sts)
+{
+	struct tsnep_adapter *adapter = container_of(ptp, struct tsnep_adapter,
+						     ptp_clock_info);
+	u32 high_before;
+	u32 low;
+	u32 high;
+	u64 counter;
+
+	/* read high dword twice to detect overrun */
+	high = ioread32(adapter->addr + ECM_COUNTER_HIGH);
+	do {
+		ptp_read_system_prets(sts);
+		low = ioread32(adapter->addr + ECM_COUNTER_LOW);
+		ptp_read_system_postts(sts);
+		high_before = high;
+		high = ioread32(adapter->addr + ECM_COUNTER_HIGH);
+	} while (high != high_before);
+	counter = (((u64)high) << 32) | ((u64)low);
+
+	*ts = ns_to_timespec64(counter);
+
+	return 0;
+}
+
 int tsnep_ptp_init(struct tsnep_adapter *adapter)
 {
 	int retval = 0;
@@ -192,6 +219,7 @@ int tsnep_ptp_init(struct tsnep_adapter *adapter)
 	adapter->ptp_clock_info.adjtime = tsnep_ptp_adjtime;
 	adapter->ptp_clock_info.gettimex64 = tsnep_ptp_gettimex64;
 	adapter->ptp_clock_info.settime64 = tsnep_ptp_settime64;
+	adapter->ptp_clock_info.getcyclesx64 = tsnep_ptp_getcyclesx64;
 
 	spin_lock_init(&adapter->ptp_lock);
 
-- 
2.20.1

Re: [PATCH net-next v2 0/5] ptp: Support hardware clocks with additional free running cycle counter

[Richard Cochran]

On Sun, Apr 03, 2022 at 07:55:39PM +0200, 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 TAPRIO hardware support
> is not possible anymore.

I'm travelling this week, but I will review this as soon as I can.

Thanks,
Richard