[RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[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 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.

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
to 2 step synchronisation 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 patch set from Miroslav
Lichvar.

Please give me some feedback. For me it seems like a straight forward
extension for ptp vclocks, but I'm new to this topic.

Gerhard Engleder (6):
  bpf: Access hwtstamp field of hwtstamps directly
  ptp: Initialize skb_shared_hwtstamps
  ptp: Add free running time support
  ptp: Support time stamps based on free running time
  ptp: Allow vclocks without free running physical clock
  tsnep: Add free running time support

 .../net/ethernet/cavium/liquidio/lio_main.c   |  1 +
 .../ethernet/cavium/liquidio/lio_vf_main.c    |  1 +
 .../net/ethernet/chelsio/cxgb4/cxgb4_ptp.c    |  1 +
 drivers/net/ethernet/engleder/tsnep_hw.h      |  9 ++++--
 drivers/net/ethernet/engleder/tsnep_main.c    |  6 ++++
 drivers/net/ethernet/engleder/tsnep_ptp.c     | 28 +++++++++++++++++++
 .../hisilicon/hns3/hns3pf/hclge_ptp.c         |  1 +
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    |  1 +
 drivers/net/ethernet/sfc/tx_common.c          |  1 +
 drivers/ptp/ptp_clock.c                       |  8 +++---
 drivers/ptp/ptp_ines.c                        |  1 +
 drivers/ptp/ptp_private.h                     |  9 ++++++
 drivers/ptp/ptp_sysfs.c                       | 11 ++++----
 drivers/ptp/ptp_vclock.c                      | 25 +++++++++++++----
 include/linux/ptp_clock_kernel.h              | 27 ++++++++++++++++++
 include/linux/skbuff.h                        |  3 ++
 net/core/filter.c                             |  3 +-
 17 files changed, 117 insertions(+), 19 deletions(-)

-- 
2.20.1

[RFC PATCH net-next 1/6] bpf: Access hwtstamp field of hwtstamps directly

[Gerhard Engleder]

skb_shared_hwtstamps contains only the field hwtstamp. That property is
hard-coded and checked during build in BPF. bpf_target_off() gets the
whole structure as argument (hwtstamps) instead of the actually accessed
field hwtstamp.

Access hwtstamp field directly and allow future extensions of
skb_shared_hwtstamps.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 net/core/filter.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 88767f7da150..09e202b60060 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -9364,13 +9364,12 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
 		break;
 	case offsetof(struct __sk_buff, hwtstamp):
 		BUILD_BUG_ON(sizeof_field(struct skb_shared_hwtstamps, hwtstamp) != 8);
-		BUILD_BUG_ON(offsetof(struct skb_shared_hwtstamps, hwtstamp) != 0);
 
 		insn = bpf_convert_shinfo_access(si, insn);
 		*insn++ = BPF_LDX_MEM(BPF_DW,
 				      si->dst_reg, si->dst_reg,
 				      bpf_target_off(struct skb_shared_info,
-						     hwtstamps, 8,
+						     hwtstamps.hwtstamp, 8,
 						     target_size));
 		break;
 	}
-- 
2.20.1

[RFC PATCH net-next 2/6] ptp: Initialize skb_shared_hwtstamps

[Gerhard Engleder]

There are only a few locations, which do not initialize the whole
skb_shared_hwtstamps structure and write the only field of the
structure. This is ok as long as skb_shared_hwtstamps is not extended
with additional fields.

Always initialize the whole skb_shared_hwtstamps structure to prepare
for future extensions of this structure.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/cavium/liquidio/lio_main.c        | 1 +
 drivers/net/ethernet/cavium/liquidio/lio_vf_main.c     | 1 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c         | 1 +
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c | 1 +
 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c     | 1 +
 drivers/net/ethernet/sfc/tx_common.c                   | 1 +
 drivers/ptp/ptp_ines.c                                 | 1 +
 7 files changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index ba28aa444e5a..8c7868007bdf 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -2212,6 +2212,7 @@ static void handle_timestamp(struct octeon_device *oct,
 		netif_info(lio, tx_done, lio->netdev,
 			   "Got resulting SKBTX_HW_TSTAMP skb=%p ns=%016llu\n",
 			   skb, (unsigned long long)ns);
+		memset(&ts, 0, sizeof(ts));
 		ts.hwtstamp = ns_to_ktime(ns + lio->ptp_adjust);
 		skb_tstamp_tx(skb, &ts);
 	}
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
index 568f211d91cc..ebac2d46a3bf 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_main.c
@@ -1340,6 +1340,7 @@ static void handle_timestamp(struct octeon_device *oct, u32 status, void *buf)
 		netif_info(lio, tx_done, lio->netdev,
 			   "Got resulting SKBTX_HW_TSTAMP skb=%p ns=%016llu\n",
 			   skb, (unsigned long long)ns);
+		memset(&ts, 0, sizeof(ts));
 		ts.hwtstamp = ns_to_ktime(ns + lio->ptp_adjust);
 		skb_tstamp_tx(skb, &ts);
 	}
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c
index 5bf117d2179f..67241dbe575c 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c
@@ -109,6 +109,7 @@ void cxgb4_ptp_read_hwstamp(struct adapter *adapter, struct port_info *pi)
 	tx_ts |= (u64)t4_read_reg(adapter,
 				  T5_PORT_REG(pi->port_id,
 					      MAC_PORT_TX_TS_VAL_HI)) << 32;
+	memset(skb_ts, 0, sizeof(*skb_ts));
 	skb_ts->hwtstamp = ns_to_ktime(tx_ts);
 	skb_tstamp_tx(adapter->ptp_tx_skb, skb_ts);
 	dev_kfree_skb_any(adapter->ptp_tx_skb);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
index a40b1583f114..4e378d856529 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
@@ -103,6 +103,7 @@ void hclge_ptp_clean_tx_hwts(struct hclge_dev *hdev)
 		hdev->ptp->tx_cleaned++;
 
 		ns += (((u64)hi) << 32 | lo) * NSEC_PER_SEC;
+		memset(&hwts, 0, sizeof(hwts));
 		hwts.hwtstamp = ns_to_ktime(ns);
 		skb_tstamp_tx(skb, &hwts);
 		dev_kfree_skb_any(skb);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 35422e64d89f..887a09887f03 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -445,6 +445,7 @@ static void mlxsw_sp1_packet_timestamp(struct mlxsw_sp *mlxsw_sp,
 	nsec = timecounter_cyc2time(&mlxsw_sp->clock->tc, timestamp);
 	spin_unlock_bh(&mlxsw_sp->clock->lock);
 
+	memset(&hwtstamps, 0, sizeof(hwtstamps));
 	hwtstamps.hwtstamp = ns_to_ktime(nsec);
 	mlxsw_sp1_ptp_packet_finish(mlxsw_sp, skb,
 				    key.local_port, key.ingress, &hwtstamps);
diff --git a/drivers/net/ethernet/sfc/tx_common.c b/drivers/net/ethernet/sfc/tx_common.c
index d530cde2b864..f7b6228a6be0 100644
--- a/drivers/net/ethernet/sfc/tx_common.c
+++ b/drivers/net/ethernet/sfc/tx_common.c
@@ -170,6 +170,7 @@ void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
 		     tx_queue->completed_timestamp_minor)) {
 			struct skb_shared_hwtstamps hwtstamp;
 
+			memset(&hwtstamp, 0, sizeof(hwtstamp));
 			hwtstamp.hwtstamp =
 				efx_ptp_nic_to_kernel_time(tx_queue);
 			skb_tstamp_tx(skb, &hwtstamp);
diff --git a/drivers/ptp/ptp_ines.c b/drivers/ptp/ptp_ines.c
index 61f47fb9d997..6413e44267cc 100644
--- a/drivers/ptp/ptp_ines.c
+++ b/drivers/ptp/ptp_ines.c
@@ -659,6 +659,7 @@ static void ines_txtstamp_work(struct work_struct *work)
 		kfree_skb(skb);
 		return;
 	}
+	memset(&ssh, 0, sizeof(ssh));
 	ssh.hwtstamp = ns_to_ktime(ns);
 	skb_complete_tx_timestamp(skb, &ssh);
 }
-- 
2.20.1

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

[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 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);
 	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);
 	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);
 	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: [RFC PATCH net-next 3/6] ptp: Add free running time support

[Richard Cochran]

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

[RFC PATCH net-next 4/6] ptp: Support time stamps based on free running time

[Gerhard Engleder]

Physical clocks are used for hardware time stamping. Also ptp vclocks
support hardware time stamps. If a physical clock additionally supports
a free running time and this time is used as base for ptp vclocks, then
also hardware time stamps based on that free running time are required.

Add hardware time stamp of additional free running time to
skb_shared_hwtstamps and use it if physical clock supports an additional
free running time.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_vclock.c | 5 ++++-
 include/linux/skbuff.h   | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 3715d75ee8bd..84f798a11bca 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -268,7 +268,10 @@ ktime_t ptp_convert_timestamp(const struct skb_shared_hwtstamps *hwtstamps,
 
 	vclock = info_to_vclock(ptp->info);
 
-	ns = ktime_to_ns(hwtstamps->hwtstamp);
+	if (vclock->pclock->info->getfreeruntimex64)
+		ns = ktime_to_ns(hwtstamps->hwfreeruntstamp);
+	else
+		ns = ktime_to_ns(hwtstamps->hwtstamp);
 
 	spin_lock_irqsave(&vclock->lock, flags);
 	ns = timecounter_cyc2time(&vclock->tc, ns);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2be263184d1e..2ec8d944a557 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -521,6 +521,8 @@ 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
+ * @hwfreeruntstamp:	hardware time stamp based on free running time
+ *			transformed into duration since arbitrary point in time
  *
  * Software time stamps generated by ktime_get_real() are stored in
  * skb->tstamp.
@@ -533,6 +535,7 @@ static inline bool skb_frag_must_loop(struct page *p)
  */
 struct skb_shared_hwtstamps {
 	ktime_t	hwtstamp;
+	ktime_t	hwfreeruntstamp;
 };
 
 /* Definitions for tx_flags in struct skb_shared_info */
-- 
2.20.1

Re: [RFC PATCH net-next 4/6] ptp: Support time stamps based on free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 09:56:56AM +0100, Gerhard Engleder wrote:

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2be263184d1e..2ec8d944a557 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -521,6 +521,8 @@ 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
> + * @hwfreeruntstamp:	hardware time stamp based on free running time
> + *			transformed into duration since arbitrary point in time
>   *
>   * Software time stamps generated by ktime_get_real() are stored in
>   * skb->tstamp.
> @@ -533,6 +535,7 @@ static inline bool skb_frag_must_loop(struct page *p)
>   */
>  struct skb_shared_hwtstamps {
>  	ktime_t	hwtstamp;
> +	ktime_t	hwfreeruntstamp;

You are adding eight bytes per frame for what is arguably an extreme
niche case.  I personally wouldn't mind, but expect push back from the
rest of the world!

Maybe this should hide behind a Kconfig option with default off.

@davem, @kuba, what do you think?

>  };
>  
>  /* Definitions for tx_flags in struct skb_shared_info */
> -- 
> 2.20.1
> 

Thanks,
Richard

[RFC PATCH net-next 5/6] ptp: Allow vclocks without free running physical clock

[Gerhard Engleder]

If a physical clock supports an additional free running time, then there
is no need to force the physical clock itself to free running.

Guarantee free running physical clock only if additional free running
time is not supported.

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/ptp/ptp_clock.c   |  8 ++++----
 drivers/ptp/ptp_private.h |  9 +++++++++
 drivers/ptp/ptp_sysfs.c   | 11 ++++++-----
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b6f2cfd15dd2..b9944053e2b8 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_vclock_free_run(ptp)) {
+		pr_err("ptp: virtual clock requires free run\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_vclock_free_run(ptp)) {
+		pr_err("ptp: virtual clock requires free run\n");
 		return -EBUSY;
 	}
 
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index dba6be477067..595eb741d391 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -96,6 +96,15 @@ static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
 	return in_use;
 }
 
+/* Check if free run for virtual clock is required */
+static inline bool ptp_vclock_free_run(struct ptp_clock *ptp)
+{
+	if (ptp->info->getfreeruntimex64)
+		return false;
+	else
+		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..485dcfd4a8c3 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -230,11 +230,12 @@ static ssize_t n_vclocks_store(struct device *dev,
 		for (i = 1; i <= ptp->n_vclocks - num; i++)
 			*(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->info->getfreeruntimex64) {
+		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);
-- 
2.20.1

[RFC PATCH net-next 6/6] tsnep: Add free running time 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 time stamps are supported. As the name implies, this counter
cannot be set and its frequency cannot be adjusted.

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

Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
---
 drivers/net/ethernet/engleder/tsnep_hw.h   |  9 +++++--
 drivers/net/ethernet/engleder/tsnep_main.c |  6 +++++
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 28 ++++++++++++++++++++++
 3 files changed, 41 insertions(+), 2 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..4aa3d04da2e4 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -472,9 +472,12 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 			struct skb_shared_hwtstamps hwtstamps;
 			u64 timestamp =
 				__le64_to_cpu(entry->desc_wb->timestamp);
+			u64 counter =
+				__le64_to_cpu(entry->desc_wb->counter);
 
 			memset(&hwtstamps, 0, sizeof(hwtstamps));
 			hwtstamps.hwtstamp = ns_to_ktime(timestamp);
+			hwtstamps.hwfreeruntstamp = ns_to_ktime(counter);
 
 			skb_tstamp_tx(entry->skb, &hwtstamps);
 		}
@@ -706,9 +709,12 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
 					(struct tsnep_rx_inline *)skb->data;
 				u64 timestamp =
 					__le64_to_cpu(rx_inline->timestamp);
+				u64 counter =
+					__le64_to_cpu(rx_inline->counter);
 
 				memset(hwtstamps, 0, sizeof(*hwtstamps));
 				hwtstamps->hwtstamp = ns_to_ktime(timestamp);
+				hwtstamps->hwfreeruntstamp = ns_to_ktime(counter);
 			}
 			skb_pull(skb, TSNEP_RX_INLINE_METADATA_SIZE);
 			skb->protocol = eth_type_trans(skb,
diff --git a/drivers/net/ethernet/engleder/tsnep_ptp.c b/drivers/net/ethernet/engleder/tsnep_ptp.c
index eaad453d487e..5f0b807fd86c 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_getfreeruntimex64(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.getfreeruntimex64 = tsnep_ptp_getfreeruntimex64;
 
 	spin_lock_init(&adapter->ptp_lock);
 
-- 
2.20.1

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 09:56:52AM +0100, Gerhard Engleder wrote:

> 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.

So... the HW must provide frame time stamps using the one clock and
ancillary operations using the other, right?

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 08:49:41AM -0800, Richard Cochran wrote:
> On Sun, Mar 06, 2022 at 09:56:52AM +0100, Gerhard Engleder wrote:
> 
> > 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.
> 
> So... the HW must provide frame time stamps using the one clock and
> ancillary operations using the other, right?

Looking at your tsnep driver, the requirement is that the HW provide
each frame with TWO time stamps, one from each clock.
 
Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 09:56:52AM +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.

NAK.

I don't see why you don't simply provide two PHC instances from this
one device.

AFAICT, this series is not needed at all, as the existing API covers
this use case already.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

On Sun, Mar 6, 2022 at 6:05 PM Richard Cochran <richardcochran@gmail.com> 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.
>
> NAK.
>
> I don't see why you don't simply provide two PHC instances from this
> one device.

Because with vclocks the user space interface is already available. Also In
my opinion it is a good fit. The second PHC would be based on the free running
hardware counter. So it would not provide any additional functionality compared
to the vclocks based on it.

Are two PHC instances supported? At least for ethtool there is only a single
phc_index field.

> AFAICT, this series is not needed at all, as the existing API covers
> this use case already.

I assume that you mean for ETF the transmission time can be converted,
similar like for time stamps. So for ETF you are right. It was too quick to
mention ETF along with TAPRIO.

My use case is TAPRIO with hardware support. For TAPRIO the hardware
has to act based on the synchronized time within the TSN network. No
transmission times, which could be converted, are used. The hardware
is in charge to transmit frames from a certain queue only during defined
intervals. These intervals are based on the synchronized time. So the
hardware must be synchronized somehow. This is my solution to keep
the hardware synchronized while vclocks are in use.

How can I cover my use case with the existing API? I had no idea so far.

Thanks!

Gerhard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 07:38:55PM +0100, Gerhard Engleder wrote:
> How can I cover my use case with the existing API? I had no idea so far.

Okay, so 2 PHCs doesn't help, but still all you need is:

1. a different method to convert time stamps to vclock time base

2. a different method for vclocks' gettime

So let me suggest a much smaller change to the phc/vclock api... stay tuned

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Sun, Mar 06, 2022 at 01:50:32PM -0800, Richard Cochran wrote:
> On Sun, Mar 06, 2022 at 07:38:55PM +0100, Gerhard Engleder wrote:
> > How can I cover my use case with the existing API? I had no idea so far.
> 
> Okay, so 2 PHCs doesn't help, but still all you need is:
> 
> 1. a different method to convert time stamps to vclock time base
> 
> 2. a different method for vclocks' gettime
> 
> So let me suggest a much smaller change to the phc/vclock api... stay tuned

For #1:

On the receive path, the stack calls ptp_convert_timestamp() if the
socket has the SOF_TIMESTAMPING_RAW_HARDWARE option.  In that method,
you need only get the raw cycle count if supported by the pclock.

So instead of:

	vclock = info_to_vclock(ptp->info);

	ns = ktime_to_ns(hwtstamps->hwtstamp);

	spin_lock_irqsave(&vclock->lock, flags);
	ns = timecounter_cyc2time(&vclock->tc, ns);
	spin_unlock_irqrestore(&vclock->lock, flags);

something like this:

	vclock = info_to_vclock(ptp->info);

	cycles = pclock->ktime_to_cycles(hwtstamps->hwtstamp);

	spin_lock_irqsave(&vclock->lock, flags);
	ns = timecounter_cyc2time(&vclock->tc, cycles);
	spin_unlock_irqrestore(&vclock->lock, flags);

This new class method, ktime_to_cycles, can simply do ktime_to_ns() by
default for all of the existing drivers, but your special driver can
look up the hwtstamp in a cache of {hwtstamp, cycles} pairs.

(No need to bloat skbuff by another eight bytes!)

For #2:

Similarly, add a new class method, say, pclock.get_cycles that does

	if (ptp->info->gettimex64)
		ptp->info->gettimex64(ptp->info, &ts, NULL);
	else
		ptp->info->gettime64(ptp->info, &ts);

by default, but in your driver will read the special counter.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

> > > How can I cover my use case with the existing API? I had no idea so far.
> >
> > Okay, so 2 PHCs doesn't help, but still all you need is:
> >
> > 1. a different method to convert time stamps to vclock time base
> >
> > 2. a different method for vclocks' gettime
> >
> > So let me suggest a much smaller change to the phc/vclock api... stay tuned
>
> For #1:
>
> On the receive path, the stack calls ptp_convert_timestamp() if the
> socket has the SOF_TIMESTAMPING_RAW_HARDWARE option.  In that method,
> you need only get the raw cycle count if supported by the pclock.
>
> So instead of:
>
>         vclock = info_to_vclock(ptp->info);
>
>         ns = ktime_to_ns(hwtstamps->hwtstamp);
>
>         spin_lock_irqsave(&vclock->lock, flags);
>         ns = timecounter_cyc2time(&vclock->tc, ns);
>         spin_unlock_irqrestore(&vclock->lock, flags);
>
> something like this:
>
>         vclock = info_to_vclock(ptp->info);
>
>         cycles = pclock->ktime_to_cycles(hwtstamps->hwtstamp);
>
>         spin_lock_irqsave(&vclock->lock, flags);
>         ns = timecounter_cyc2time(&vclock->tc, cycles);
>         spin_unlock_irqrestore(&vclock->lock, flags);
>
> This new class method, ktime_to_cycles, can simply do ktime_to_ns() by
> default for all of the existing drivers, but your special driver can
> look up the hwtstamp in a cache of {hwtstamp, cycles} pairs.

ktime_to_cycles uses hwtstamp as key for the cache lookup. As long as
the PHC is monotonic, the key is unique. If the time of the PHC is set, then
the cache would be invalidated. I'm afraid that setting the PHC could lead to
wrong or missing timestamps. Setting the PHC in hardware, timestamp
generation in hardware, and cache invalidation in software would need to
be synchronized somehow.

Practically the PHC should be set only once. It would be also ok to use
vclocks for PTP after PHC has been set. So it should work. But it would
not be the generic PHC/vclock user space interface anymore.

> (No need to bloat skbuff by another eight bytes!)

I understand that bloating skbuff shall be prevented. Actually I need
to find a way
to generate the correct timestamp within ptp_convert_timestamp and without
bloating skbuff. The cache is one possibility. What do you think about the
following idea?

For TX it is known which timestamp is required. So I would have to find a way
to detect which timestamp shall be filled into hwtstamp.

For RX both timestamps are already available within skbuff, because they are
stored in front of the Ethernet header by the hardware. So I have to find a way
to detect the RX case and copy the right timestamp to hwtstamp.

This would prevent the cache and does not bloat skbuff.

> For #2:
>
> Similarly, add a new class method, say, pclock.get_cycles that does
>
>         if (ptp->info->gettimex64)
>                 ptp->info->gettimex64(ptp->info, &ts, NULL);
>         else
>                 ptp->info->gettime64(ptp->info, &ts);
>
> by default, but in your driver will read the special counter.

Looks better than my getfreeruntimex64.

Thanks for your suggestion!

Gerhard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 06:54:19PM +0100, Gerhard Engleder wrote:

> ktime_to_cycles uses hwtstamp as key for the cache lookup. As long as
> the PHC is monotonic, the key is unique. If the time of the PHC is set, then
> the cache would be invalidated. I'm afraid that setting the PHC could lead to
> wrong or missing timestamps.

"Wrong" timestamps happen with normal PHCs anyhow.  Not a big deal.
Your driver can drop the cache and force racing vclock frames to omit
the Rx timestamp.  User space must deal with this in any case.

> For RX both timestamps are already available within skbuff, because they are
> stored in front of the Ethernet header by the hardware. So I have to find a way
> to detect the RX case and copy the right timestamp to hwtstamp.

This works for your driver, but not for the generic vclock.  Maybe you
could let the ptp_convert_timestamp() method pass the skb back into your
driver.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 06:54:19PM +0100, Gerhard Engleder wrote:

> ktime_to_cycles uses hwtstamp as key for the cache lookup. As long as
> the PHC is monotonic, the key is unique. If the time of the PHC is set, then
> the cache would be invalidated. I'm afraid that setting the PHC could lead to
> wrong or missing timestamps. Setting the PHC in hardware, timestamp
> generation in hardware, and cache invalidation in software would need to
> be synchronized somehow.

You can avoid errors even with a time jump:

Make a variant (union) of skb_shared_hwtstamps to allow driver to
provide an address or cookie instead of ktime_t.  Set a flag in the
skbuff to signal this.

Let the Rx path check the flag and fetch the time stamp by callback
with the address/cookie.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

> > ktime_to_cycles uses hwtstamp as key for the cache lookup. As long as
> > the PHC is monotonic, the key is unique. If the time of the PHC is set, then
> > the cache would be invalidated. I'm afraid that setting the PHC could lead to
> > wrong or missing timestamps. Setting the PHC in hardware, timestamp
> > generation in hardware, and cache invalidation in software would need to
> > be synchronized somehow.
>
> You can avoid errors even with a time jump:
>
> Make a variant (union) of skb_shared_hwtstamps to allow driver to
> provide an address or cookie instead of ktime_t.  Set a flag in the
> skbuff to signal this.
>
> Let the Rx path check the flag and fetch the time stamp by callback
> with the address/cookie.
>
> > For TX it is known which timestamp is required. So I would have to find a way
> > to detect which timestamp shall be filled into hwtstamp.
>
> How about tx_flags in struct skb_shared_info ?

I will try to make an implementation!

Shall I use tx_flags in struct skb_shared_info for both, TX and RX? Or should
I use flags in struct skb_shared_info for address/cookie signalisation?

3 of 8 flags are unused in tx_flags. It may be possible to use the same flag for
TX and RX. Is it worth it trying to save flags?

For TX it signals "fill cycle based timestamp" and could be called
SKBTX_HW_CSTAMP.

For RX it signals "hwtstamp is an address/cookie" and could be called
SKBFL_TSTAMP_COOKIE.

Thank you!
Gerhard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Tue, Mar 08, 2022 at 08:49:03PM +0100, Gerhard Engleder wrote:

> 3 of 8 flags are unused in tx_flags. It may be possible to use the same flag for
> TX and RX. Is it worth it trying to save flags?

Yes, every bit is precious.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 06:54:19PM +0100, Gerhard Engleder wrote:

> For TX it is known which timestamp is required. So I would have to find a way
> to detect which timestamp shall be filled into hwtstamp.

How about tx_flags in struct skb_shared_info ?

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Michael Walle]

Hi,

> 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.
> 
> 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
> to 2 step synchronisation 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 patch set from Miroslav
> Lichvar.
> 
> Please give me some feedback. For me it seems like a straight forward
> extension for ptp vclocks, but I'm new to this topic.

From what I understand, you have a second PHC which is just a second
PHC you cannot control; i.e. it is equivalent to a PHC in free running
mode. This PHC will take the timestamps for the PTP frames. You can
create multiple vclocks and you can use ptp4l to synchronize these.

The first (controlable) PHC is used to do the Qbv scheduling, thus
needs a synchronized time.

How do you synchronize the vclock with this PHC? And how precise
is it? I know that some cards can do cross timestamping in hardware
to aid the synchronization (but I think that is not supported right
now in linux).

Richard Cochran wrote:
> You are adding eight bytes per frame for what is arguably an extreme
> niche case.

I don't think it is a niche case, btw. I was always wondering why
NXP introduced the vclock thingy. And apparently it is for
802.1AS-rev, but one use case for that is 802.1Qbv and there you'll
need a (synchronized) hardware clock to control the gates. So while
we can have multiple time domain support with the vclock, we cannot
use Qbv with them. That was something I have always wondered about.
Or.. maybe I'm missing something here.

-michael

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 01:07:51PM +0100, Michael Walle wrote:
> Richard Cochran wrote:
> > You are adding eight bytes per frame for what is arguably an extreme
> > niche case.
> 
> I don't think it is a niche case, btw. I was always wondering why
> NXP introduced the vclock thingy. And apparently it is for
> 802.1AS-rev, but one use case for that is 802.1Qbv and there you'll
> need a (synchronized) hardware clock to control the gates. So while
> we can have multiple time domain support with the vclock, we cannot
> use Qbv with them. That was something I have always wondered about.
> Or.. maybe I'm missing something here.

Niche is relative.

Believe it or not, the overwhelmingly great majority of people using
Linux have no interest in TSN.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Miroslav Lichvar]

On Mon, Mar 07, 2022 at 06:05:31AM -0800, Richard Cochran wrote:
> On Mon, Mar 07, 2022 at 01:07:51PM +0100, Michael Walle wrote:
> > Richard Cochran wrote:
> > > You are adding eight bytes per frame for what is arguably an extreme
> > > niche case.
> > 
> > I don't think it is a niche case, btw. I was always wondering why
> > NXP introduced the vclock thingy. And apparently it is for
...
> 
> Niche is relative.
> 
> Believe it or not, the overwhelmingly great majority of people using
> Linux have no interest in TSN.

Is this not the same issue as what was recently discussed about some
devices being able to provide two (e.g. PHY+MAC) or even more
timestamps at the same time?

There is a need to have multiple PHCs per device and for that to work
the drivers need to be able to save multiple timestamps per packet.

In this series it is an additional freerunning clock. That seems too
specific. I think we need a more general approach that will support
two and more physical PHCs per device. Virtual clocks are not involved
here.

-- 
Miroslav Lichvar

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 03:23:58PM +0100, Miroslav Lichvar wrote:

> There is a need to have multiple PHCs per device and for that to work
> the drivers need to be able to save multiple timestamps per packet.

That is a big job that entails a new user API.

For the present, this device can be supported with only minimal
changes in the PHC class layer.  See my other reply.

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Richard Cochran]

On Mon, Mar 07, 2022 at 06:37:48AM -0800, Richard Cochran wrote:
> On Mon, Mar 07, 2022 at 03:23:58PM +0100, Miroslav Lichvar wrote:
> 
> > There is a need to have multiple PHCs per device and for that to work
> > the drivers need to be able to save multiple timestamps per packet.
> 
> That is a big job that entails a new user API.

On second thought, maybe the address/cookie idea will at least open up
multiple driver timestamps in a practical way...

Thanks,
Richard

Re: [RFC PATCH net-next 0/6] ptp: Support hardware clocks with additional free running time

[Gerhard Engleder]

> From what I understand, you have a second PHC which is just a second
> PHC you cannot control; i.e. it is equivalent to a PHC in free running
> mode. This PHC will take the timestamps for the PTP frames. You can
> create multiple vclocks and you can use ptp4l to synchronize these.
>
> The first (controlable) PHC is used to do the Qbv scheduling, thus
> needs a synchronized time.
>
> How do you synchronize the vclock with this PHC? And how precise
> is it? I know that some cards can do cross timestamping in hardware
> to aid the synchronization (but I think that is not supported right
> now in linux).

There is no need to synchronize the first (controlable) PHC with the vclock.
A ptp4l instance is running and synchronizing the time for Qbv/TAPRIO.
vclocks are used for other time domains, which do not affect Qbv/TAPRIO.

> > You are adding eight bytes per frame for what is arguably an extreme
> > niche case.
>
> I don't think it is a niche case, btw. I was always wondering why
> NXP introduced the vclock thingy. And apparently it is for
> 802.1AS-rev, but one use case for that is 802.1Qbv and there you'll
> need a (synchronized) hardware clock to control the gates. So while
> we can have multiple time domain support with the vclock, we cannot
> use Qbv with them. That was something I have always wondered about.

I agree that most people using Linux have no interest in TSN. For the few
people who are interested in TSN, I assume using two time domains in
combination with Qbv/TAPRIO is a common goal. Is there anyone else who
wants to use two time domains in combination with Qbv/TAPRIO?

Gerhard