[net-next,v6,4/7] net: dsa: hellcreek: Add support for hardware timestamping

From: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>

The switch has the ability to take hardware generated time stamps per port for
PTPv2 event messages in Rx and Tx direction. That is useful for achieving needed
time synchronization precision for TSN devices/switches. So add support for it.

There are two directions:

 * RX

   The switch has a single register per port to capture a timestamp. That
   mechanism is not used due to correlation problems. If the software processing
   is too slow and a PTPv2 event message is received before the previous one has
   been processed, false timestamps will be captured. Therefore, the switch can
   do "inline" timestamping which means it can insert the nanoseconds part of
   the timestamp directly into the PTPv2 event message. The reserved field (4
   bytes) is leveraged for that. This might not be in accordance with (older)
   PTP standards, but is the only way to get reliable results.

 * TX

   In Tx direction there is no correlation problem, because the software and the
   driver has to ensure that only one event message is "on the fly". However,
   the switch provides also a mechanism to check whether a timestamp is
   lost. That can only happen when a timestamp is read and at this point another
   message is timestamped. So, that lost bit is checked just in case to indicate
   to the user that the driver or the software is somewhat buggy.

Signed-off-by: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Acked-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/dsa/hirschmann/Makefile           |   1 +
 drivers/net/dsa/hirschmann/hellcreek.c        |  15 +
 drivers/net/dsa/hirschmann/hellcreek.h        |  25 +
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 479 ++++++++++++++++++
 .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  58 +++
 drivers/net/dsa/hirschmann/hellcreek_ptp.c    |  48 +-
 drivers/net/dsa/hirschmann/hellcreek_ptp.h    |   4 +
 7 files changed, 616 insertions(+), 14 deletions(-)
 create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
 create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h

On Sun, Oct 04, 2020 at 01:29:08PM +0200, Kurt Kanzenbach wrote:
> From: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
> 
> The switch has the ability to take hardware generated time stamps per port for
> PTPv2 event messages in Rx and Tx direction. That is useful for achieving needed
> time synchronization precision for TSN devices/switches. So add support for it.
> 
> There are two directions:
> 
>  * RX
> 
>    The switch has a single register per port to capture a timestamp. That
>    mechanism is not used due to correlation problems. If the software processing
>    is too slow and a PTPv2 event message is received before the previous one has
>    been processed, false timestamps will be captured. Therefore, the switch can
>    do "inline" timestamping which means it can insert the nanoseconds part of
>    the timestamp directly into the PTPv2 event message. The reserved field (4
>    bytes) is leveraged for that. This might not be in accordance with (older)
>    PTP standards, but is the only way to get reliable results.
> 
>  * TX
> 
>    In Tx direction there is no correlation problem, because the software and the
>    driver has to ensure that only one event message is "on the fly". However,
>    the switch provides also a mechanism to check whether a timestamp is
>    lost. That can only happen when a timestamp is read and at this point another
>    message is timestamped. So, that lost bit is checked just in case to indicate
>    to the user that the driver or the software is somewhat buggy.
> 
> Signed-off-by: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  drivers/net/dsa/hirschmann/Makefile           |   1 +
>  drivers/net/dsa/hirschmann/hellcreek.c        |  15 +
>  drivers/net/dsa/hirschmann/hellcreek.h        |  25 +
>  .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 479 ++++++++++++++++++
>  .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  58 +++
>  drivers/net/dsa/hirschmann/hellcreek_ptp.c    |  48 +-
>  drivers/net/dsa/hirschmann/hellcreek_ptp.h    |   4 +
>  7 files changed, 616 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
>  create mode 100644 drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> 
> diff --git a/drivers/net/dsa/hirschmann/Makefile b/drivers/net/dsa/hirschmann/Makefile
> index 39de02a03640..f4075c2998b5 100644
> --- a/drivers/net/dsa/hirschmann/Makefile
> +++ b/drivers/net/dsa/hirschmann/Makefile
> @@ -2,3 +2,4 @@
>  obj-$(CONFIG_NET_DSA_HIRSCHMANN_HELLCREEK)	+= hellcreek_sw.o
>  hellcreek_sw-objs := hellcreek.o
>  hellcreek_sw-objs += hellcreek_ptp.o
> +hellcreek_sw-objs += hellcreek_hwtstamp.o
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
> index 3e1040039a96..e9ec9dcca0d8 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek.c
> @@ -25,6 +25,7 @@
>  
>  #include "hellcreek.h"
>  #include "hellcreek_ptp.h"
> +#include "hellcreek_hwtstamp.h"
>  
>  static const struct hellcreek_counter hellcreek_counter[] = {
>  	{ 0x00, "RxFiltered", },
> @@ -1191,6 +1192,11 @@ static const struct dsa_switch_ops hellcreek_ds_ops = {
>  	.port_bridge_leave   = hellcreek_port_bridge_leave,
>  	.port_stp_state_set  = hellcreek_port_stp_state_set,
>  	.phylink_validate    = hellcreek_phylink_validate,
> +	.port_hwtstamp_set   = hellcreek_port_hwtstamp_set,
> +	.port_hwtstamp_get   = hellcreek_port_hwtstamp_get,
> +	.port_txtstamp	     = hellcreek_port_txtstamp,
> +	.port_rxtstamp	     = hellcreek_port_rxtstamp,
> +	.get_ts_info	     = hellcreek_get_ts_info,
>  };
>  
>  static int hellcreek_probe(struct platform_device *pdev)
> @@ -1302,10 +1308,18 @@ static int hellcreek_probe(struct platform_device *pdev)
>  		goto err_ptp_setup;
>  	}
>  
> +	ret = hellcreek_hwtstamp_setup(hellcreek);
> +	if (ret) {
> +		dev_err(dev, "Failed to setup hardware timestamping!\n");
> +		goto err_tstamp_setup;
> +	}
> +
>  	platform_set_drvdata(pdev, hellcreek);
>  
>  	return 0;
>  
> +err_tstamp_setup:
> +	hellcreek_ptp_free(hellcreek);
>  err_ptp_setup:
>  	dsa_unregister_switch(hellcreek->ds);
>  
> @@ -1316,6 +1330,7 @@ static int hellcreek_remove(struct platform_device *pdev)
>  {
>  	struct hellcreek *hellcreek = platform_get_drvdata(pdev);
>  
> +	hellcreek_hwtstamp_free(hellcreek);
>  	hellcreek_ptp_free(hellcreek);
>  	dsa_unregister_switch(hellcreek->ds);
>  	platform_set_drvdata(pdev, NULL);
> diff --git a/drivers/net/dsa/hirschmann/hellcreek.h b/drivers/net/dsa/hirschmann/hellcreek.h
> index 0c95577b81ad..0b42c15736ce 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek.h
> @@ -213,6 +213,28 @@ struct hellcreek_counter {
>  
>  struct hellcreek;
>  
> +/* State flags for hellcreek_port_hwtstamp::state */
> +enum {
> +	HELLCREEK_HWTSTAMP_ENABLED,
> +	HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
> +};
> +
> +/* A structure to hold hardware timestamping information per port */
> +struct hellcreek_port_hwtstamp {
> +	/* Timestamping state */
> +	unsigned long state;
> +
> +	/* Resources for receive timestamping */
> +	struct sk_buff_head rx_queue; /* For synchronization messages */
> +
> +	/* Resources for transmit timestamping */
> +	unsigned long tx_tstamp_start;
> +	struct sk_buff *tx_skb;
> +
> +	/* Current timestamp configuration */
> +	struct hwtstamp_config tstamp_config;
> +};
> +
>  struct hellcreek_port {
>  	struct hellcreek *hellcreek;
>  	struct list_head vlan_list;
> @@ -221,6 +243,9 @@ struct hellcreek_port {
>  	int vlan_filtering;	/* Is VLAN filtering activated */
>  	u16 ptcfg;		/* ptcfg shadow */
>  	u64 *counter_values;
> +
> +	/* Per-port timestamping resources */
> +	struct hellcreek_port_hwtstamp port_hwtstamp;
>  };
>  
>  struct hellcreek_fdb_entry {
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> new file mode 100644
> index 000000000000..69dd9a2e8bb6
> --- /dev/null
> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> @@ -0,0 +1,479 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * DSA driver for:
> + * Hirschmann Hellcreek TSN switch.
> + *
> + * Copyright (C) 2019,2020 Hochschule Offenburg
> + * Copyright (C) 2019,2020 Linutronix GmbH
> + * Authors: Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
> + *	    Kurt Kanzenbach <kurt@linutronix.de>
> + */
> +
> +#include <linux/ptp_classify.h>
> +
> +#include "hellcreek.h"
> +#include "hellcreek_hwtstamp.h"
> +#include "hellcreek_ptp.h"
> +
> +int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
> +			  struct ethtool_ts_info *info)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +
> +	info->phc_index = hellcreek->ptp_clock ?
> +		ptp_clock_index(hellcreek->ptp_clock) : -1;
> +	info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> +		SOF_TIMESTAMPING_RX_HARDWARE |
> +		SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	/* enabled tx timestamping */
> +	info->tx_types = BIT(HWTSTAMP_TX_ON);
> +
> +	/* L2 & L4 PTPv2 event rx messages are timestamped */
> +	info->rx_filters = BIT(HWTSTAMP_FILTER_PTP_V2_EVENT);
> +
> +	return 0;
> +}
> +
> +/* Enabling/disabling TX and RX HW timestamping for different PTP messages is
> + * not available in the switch. Thus, this function only serves as a check if
> + * the user requested what is actually available or not
> + */

Correct me if I'm wrong, but to the user it makes zero difference
whether the hardware takes timestamps or not. What matters is whether
the skb will be delivered to the stack with a hardware timestamp or not,
so you should definitely accept a hwtstamp_config with TX and RX
timestamping disabled.

> +static int hellcreek_set_hwtstamp_config(struct hellcreek *hellcreek, int port,
> +					 struct hwtstamp_config *config)
> +{
> +	struct hellcreek_port_hwtstamp *ps =
> +		&hellcreek->ports[port].port_hwtstamp;
> +	bool tx_tstamp_enable = false;
> +	bool rx_tstamp_enable = false;
> +
> +	/* Interaction with the timestamp hardware is prevented here.  It is
> +	 * enabled when this config function ends successfully
> +	 */
> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_ENABLED, &ps->state);
> +
> +	/* Reserved for future extensions */
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_ON:
> +		tx_tstamp_enable = true;
> +		break;
> +
> +	/* TX HW timestamping can't be disabled on the switch */
> +	case HWTSTAMP_TX_OFF:
> +		config->tx_type = HWTSTAMP_TX_ON;
> +		break;
> +
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config->rx_filter) {
> +	/* RX HW timestamping can't be disabled on the switch */
> +	case HWTSTAMP_FILTER_NONE:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		break;
> +
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		rx_tstamp_enable = true;
> +		break;
> +
> +	/* RX HW timestamping can't be enabled for all messages on the switch */
> +	case HWTSTAMP_FILTER_ALL:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		break;
> +
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	if (!tx_tstamp_enable)
> +		return -ERANGE;
> +
> +	if (!rx_tstamp_enable)
> +		return -ERANGE;
> +
> +	/* If this point is reached, then the requested hwtstamp config is
> +	 * compatible with the hwtstamp offered by the switch.  Therefore,
> +	 * enable the interaction with the HW timestamping
> +	 */
> +	set_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state);
> +
> +	return 0;
> +}
> +
> +int hellcreek_port_hwtstamp_set(struct dsa_switch *ds, int port,
> +				struct ifreq *ifr)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port_hwtstamp *ps;
> +	struct hwtstamp_config config;
> +	int err;
> +
> +	ps = &hellcreek->ports[port].port_hwtstamp;
> +
> +	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> +		return -EFAULT;
> +
> +	err = hellcreek_set_hwtstamp_config(hellcreek, port, &config);
> +	if (err)
> +		return err;
> +
> +	/* Save the chosen configuration to be returned later */
> +	memcpy(&ps->tstamp_config, &config, sizeof(config));
> +
> +	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
> +		-EFAULT : 0;
> +}
> +
> +int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
> +				struct ifreq *ifr)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port_hwtstamp *ps;
> +	struct hwtstamp_config *config;
> +
> +	ps = &hellcreek->ports[port].port_hwtstamp;
> +	config = &ps->tstamp_config;
> +
> +	return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
> +		-EFAULT : 0;
> +}
> +
> +/* Returns a pointer to the PTP header if the caller should time stamp, or NULL
> + * if the caller should not.
> + */
> +static struct ptp_header *hellcreek_should_tstamp(struct hellcreek *hellcreek,
> +						  int port, struct sk_buff *skb,
> +						  unsigned int type)
> +{
> +	struct hellcreek_port_hwtstamp *ps =
> +		&hellcreek->ports[port].port_hwtstamp;
> +	struct ptp_header *hdr;
> +
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
> +		return NULL;
> +
> +	if (!test_bit(HELLCREEK_HWTSTAMP_ENABLED, &ps->state))
> +		return NULL;
> +
> +	return hdr;
> +}
> +
> +static u64 hellcreek_get_reserved_field(const struct ptp_header *hdr)
> +{
> +	return be32_to_cpu(hdr->reserved2);
> +}
> +
> +static void hellcreek_clear_reserved_field(struct ptp_header *hdr)
> +{
> +	hdr->reserved2 = 0;
> +}
> +
> +static int hellcreek_ptp_hwtstamp_available(struct hellcreek *hellcreek,
> +					    unsigned int ts_reg)
> +{
> +	u16 status;
> +
> +	status = hellcreek_ptp_read(hellcreek, ts_reg);
> +
> +	if (status & PR_TS_STATUS_TS_LOST)
> +		dev_err(hellcreek->dev,
> +			"Tx time stamp lost! This should never happen!\n");
> +
> +	/* If hwtstamp is not available, this means the previous hwtstamp was
> +	 * successfully read, and the one we need is not yet available
> +	 */
> +	return (status & PR_TS_STATUS_TS_AVAIL) ? 1 : 0;
> +}
> +
> +/* Get nanoseconds timestamp from timestamping unit */
> +static u64 hellcreek_ptp_hwtstamp_read(struct hellcreek *hellcreek,
> +				       unsigned int ts_reg)
> +{
> +	u16 nsl, nsh;
> +
> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
> +	nsh = hellcreek_ptp_read(hellcreek, ts_reg);
> +	nsl = hellcreek_ptp_read(hellcreek, ts_reg);
> +
> +	return (u64)nsl | ((u64)nsh << 16);
> +}
> +
> +static int hellcreek_txtstamp_work(struct hellcreek *hellcreek,
> +				   struct hellcreek_port_hwtstamp *ps, int port)
> +{
> +	struct skb_shared_hwtstamps shhwtstamps;
> +	unsigned int status_reg, data_reg;
> +	struct sk_buff *tmp_skb;
> +	int ts_status;
> +	u64 ns = 0;
> +
> +	if (!ps->tx_skb)
> +		return 0;
> +
> +	switch (port) {
> +	case 2:
> +		status_reg = PR_TS_TX_P1_STATUS_C;
> +		data_reg   = PR_TS_TX_P1_DATA_C;
> +		break;
> +	case 3:
> +		status_reg = PR_TS_TX_P2_STATUS_C;
> +		data_reg   = PR_TS_TX_P2_DATA_C;
> +		break;
> +	default:
> +		dev_err(hellcreek->dev, "Wrong port for timestamping!\n");
> +		return 0;
> +	}
> +
> +	ts_status = hellcreek_ptp_hwtstamp_available(hellcreek, status_reg);
> +
> +	/* Not available yet? */
> +	if (ts_status == 0) {
> +		/* Check whether the operation of reading the tx timestamp has
> +		 * exceeded its allowed period
> +		 */
> +		if (time_is_before_jiffies(ps->tx_tstamp_start +
> +					   TX_TSTAMP_TIMEOUT)) {
> +			dev_err(hellcreek->dev,
> +				"Timeout while waiting for Tx timestamp!\n");
> +			goto free_and_clear_skb;
> +		}
> +
> +		/* The timestamp should be available quickly, while getting it
> +		 * in high priority. Restart the work
> +		 */
> +		return 1;
> +	}
> +
> +	mutex_lock(&hellcreek->ptp_lock);
> +	ns  = hellcreek_ptp_hwtstamp_read(hellcreek, data_reg);
> +	ns += hellcreek_ptp_gettime_seconds(hellcreek, ns);
> +	mutex_unlock(&hellcreek->ptp_lock);
> +
> +	/* Now we have the timestamp in nanoseconds, store it in the correct
> +	 * structure in order to send it to the user
> +	 */
> +	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +	shhwtstamps.hwtstamp = ns_to_ktime(ns);
> +
> +	tmp_skb = ps->tx_skb;
> +	ps->tx_skb = NULL;
> +
> +	/* skb_complete_tx_timestamp() frees up the client to make another
> +	 * timestampable transmit.  We have to be ready for it by clearing the
> +	 * ps->tx_skb "flag" beforehand
> +	 */
> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> +
> +	/* Deliver a clone of the original outgoing tx_skb with tx hwtstamp */
> +	skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +
> +	return 0;
> +
> +free_and_clear_skb:
> +	dev_kfree_skb_any(ps->tx_skb);
> +	ps->tx_skb = NULL;
> +	clear_bit_unlock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> +
> +	return 0;
> +}
> +
> +static void hellcreek_get_rxts(struct hellcreek *hellcreek,
> +			       struct hellcreek_port_hwtstamp *ps,
> +			       struct sk_buff *skb, struct sk_buff_head *rxq,
> +			       int port)
> +{
> +	struct skb_shared_hwtstamps *shwt;
> +	struct sk_buff_head received;
> +	unsigned long flags;
> +
> +	/* The latched timestamp belongs to one of the received frames. */
> +	__skb_queue_head_init(&received);
> +
> +	/* Lock & disable interrupts */
> +	spin_lock_irqsave(&rxq->lock, flags);
> +
> +	/* Add the reception queue "rxq" to the "received" queue an reintialize
> +	 * "rxq".  From now on, we deal with "received" not with "rxq"
> +	 */
> +	skb_queue_splice_tail_init(rxq, &received);
> +
> +	spin_unlock_irqrestore(&rxq->lock, flags);
> +
> +	for (; skb; skb = __skb_dequeue(&received)) {
> +		struct ptp_header *hdr;
> +		unsigned int type;
> +		u64 ns;
> +
> +		/* Get nanoseconds from ptp packet */
> +		type = SKB_PTP_TYPE(skb);
> +		hdr  = ptp_parse_header(skb, type);
> +		ns   = hellcreek_get_reserved_field(hdr);
> +		hellcreek_clear_reserved_field(hdr);
> +
> +		/* Add seconds part */
> +		mutex_lock(&hellcreek->ptp_lock);
> +		ns += hellcreek_ptp_gettime_seconds(hellcreek, ns);
> +		mutex_unlock(&hellcreek->ptp_lock);
> +
> +		/* Save time stamp */
> +		shwt = skb_hwtstamps(skb);
> +		memset(shwt, 0, sizeof(*shwt));
> +		shwt->hwtstamp = ns_to_ktime(ns);
> +		netif_rx_ni(skb);
> +	}
> +}
> +
> +static void hellcreek_rxtstamp_work(struct hellcreek *hellcreek,
> +				    struct hellcreek_port_hwtstamp *ps,
> +				    int port)
> +{
> +	struct sk_buff *skb;
> +
> +	skb = skb_dequeue(&ps->rx_queue);
> +	if (skb)
> +		hellcreek_get_rxts(hellcreek, ps, skb, &ps->rx_queue, port);
> +}
> +
> +long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
> +{
> +	struct hellcreek *hellcreek = ptp_to_hellcreek(ptp);
> +	struct dsa_switch *ds = hellcreek->ds;
> +	int i, restart = 0;
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		struct hellcreek_port_hwtstamp *ps;
> +
> +		if (!dsa_is_user_port(ds, i))
> +			continue;
> +
> +		ps = &hellcreek->ports[i].port_hwtstamp;
> +
> +		if (test_bit(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
> +			restart |= hellcreek_txtstamp_work(hellcreek, ps, i);
> +
> +		hellcreek_rxtstamp_work(hellcreek, ps, i);
> +	}
> +
> +	return restart ? 1 : -1;
> +}
> +
> +bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *clone, unsigned int type)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port_hwtstamp *ps;
> +	struct ptp_header *hdr;
> +
> +	ps = &hellcreek->ports[port].port_hwtstamp;
> +
> +	/* Check if the driver is expected to do HW timestamping */
> +	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> +		return false;
> +
> +	/* Make sure the message is a PTP message that needs to be timestamped
> +	 * and the interaction with the HW timestamping is enabled. If not, stop
> +	 * here
> +	 */
> +	hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
> +	if (!hdr)
> +		return false;
> +
> +	if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
> +				  &ps->state))
> +		return false;
> +
> +	ps->tx_skb = clone;
> +
> +	/* store the number of ticks occurred since system start-up till this
> +	 * moment
> +	 */
> +	ps->tx_tstamp_start = jiffies;
> +
> +	ptp_schedule_worker(hellcreek->ptp_clock, 0);
> +
> +	return true;
> +}
> +
> +bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *skb, unsigned int type)
> +{
> +	struct hellcreek *hellcreek = ds->priv;
> +	struct hellcreek_port_hwtstamp *ps;
> +	struct ptp_header *hdr;
> +
> +	ps = &hellcreek->ports[port].port_hwtstamp;
> +
> +	/* This check only fails if the user did not initialize hardware
> +	 * timestamping beforehand.
> +	 */
> +	if (ps->tstamp_config.rx_filter != HWTSTAMP_FILTER_PTP_V2_EVENT)
> +		return false;
> +
> +	/* Make sure the message is a PTP message that needs to be timestamped
> +	 * and the interaction with the HW timestamping is enabled. If not, stop
> +	 * here
> +	 */
> +	hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
> +	if (!hdr)
> +		return false;
> +
> +	SKB_PTP_TYPE(skb) = type;
> +
> +	skb_queue_tail(&ps->rx_queue, skb);
> +
> +	ptp_schedule_worker(hellcreek->ptp_clock, 0);
> +
> +	return true;
> +}
> +
> +static void hellcreek_hwtstamp_port_setup(struct hellcreek *hellcreek, int port)
> +{
> +	struct hellcreek_port_hwtstamp *ps =
> +		&hellcreek->ports[port].port_hwtstamp;
> +
> +	skb_queue_head_init(&ps->rx_queue);
> +}
> +
> +int hellcreek_hwtstamp_setup(struct hellcreek *hellcreek)
> +{
> +	struct dsa_switch *ds = hellcreek->ds;
> +	int i;
> +
> +	/* Initialize timestamping ports. */
> +	for (i = 0; i < ds->num_ports; ++i) {
> +		if (!dsa_is_user_port(ds, i))
> +			continue;
> +
> +		hellcreek_hwtstamp_port_setup(hellcreek, i);
> +	}
> +
> +	/* Select the synchronized clock as the source timekeeper for the
> +	 * timestamps and enable inline timestamping.
> +	 */
> +	hellcreek_ptp_write(hellcreek, PR_SETTINGS_C_TS_SRC_TK_MASK |
> +			    PR_SETTINGS_C_RES3TS,
> +			    PR_SETTINGS_C);
> +
> +	return 0;
> +}
> +
> +void hellcreek_hwtstamp_free(struct hellcreek *hellcreek)
> +{
> +	/* Nothing todo */
> +}
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> new file mode 100644
> index 000000000000..c0745ffa1ebb
> --- /dev/null
> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/*
> + * DSA driver for:
> + * Hirschmann Hellcreek TSN switch.
> + *
> + * Copyright (C) 2019,2020 Hochschule Offenburg
> + * Copyright (C) 2019,2020 Linutronix GmbH
> + * Authors: Kurt Kanzenbach <kurt@linutronix.de>
> + *	    Kamil Alkhouri <kamil.alkhouri@hs-offenburg.de>
> + */
> +
> +#ifndef _HELLCREEK_HWTSTAMP_H_
> +#define _HELLCREEK_HWTSTAMP_H_
> +
> +#include <net/dsa.h>
> +#include "hellcreek.h"
> +
> +/* Timestamp Register */
> +#define PR_TS_RX_P1_STATUS_C	(0x1d * 2)
> +#define PR_TS_RX_P1_DATA_C	(0x1e * 2)
> +#define PR_TS_TX_P1_STATUS_C	(0x1f * 2)
> +#define PR_TS_TX_P1_DATA_C	(0x20 * 2)
> +#define PR_TS_RX_P2_STATUS_C	(0x25 * 2)
> +#define PR_TS_RX_P2_DATA_C	(0x26 * 2)
> +#define PR_TS_TX_P2_STATUS_C	(0x27 * 2)
> +#define PR_TS_TX_P2_DATA_C	(0x28 * 2)
> +
> +#define PR_TS_STATUS_TS_AVAIL	BIT(2)
> +#define PR_TS_STATUS_TS_LOST	BIT(3)
> +
> +#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> +
> +/* TX_TSTAMP_TIMEOUT: This limits the time spent polling for a TX
> + * timestamp. When working properly, hardware will produce a timestamp
> + * within 1ms. Software may enounter delays, so the timeout is set
> + * accordingly.
> + */
> +#define TX_TSTAMP_TIMEOUT	msecs_to_jiffies(40)
> +
> +int hellcreek_port_hwtstamp_set(struct dsa_switch *ds, int port,
> +				struct ifreq *ifr);
> +int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
> +				struct ifreq *ifr);
> +
> +bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *clone, unsigned int type);
> +bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *clone, unsigned int type);
> +
> +int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
> +			  struct ethtool_ts_info *info);
> +
> +long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp);
> +
> +int hellcreek_hwtstamp_setup(struct hellcreek *chip);
> +void hellcreek_hwtstamp_free(struct hellcreek *chip);
> +
> +#endif /* _HELLCREEK_HWTSTAMP_H_ */
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> index 856fcb9ba3c6..12ad956abd5c 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> @@ -12,14 +12,15 @@
>  #include <linux/ptp_clock_kernel.h>
>  #include "hellcreek.h"
>  #include "hellcreek_ptp.h"
> +#include "hellcreek_hwtstamp.h"
>  
> -static u16 hellcreek_ptp_read(struct hellcreek *hellcreek, unsigned int offset)
> +u16 hellcreek_ptp_read(struct hellcreek *hellcreek, unsigned int offset)
>  {
>  	return readw(hellcreek->ptp_base + offset);
>  }
>  
> -static void hellcreek_ptp_write(struct hellcreek *hellcreek, u16 data,
> -				unsigned int offset)
> +void hellcreek_ptp_write(struct hellcreek *hellcreek, u16 data,
> +			 unsigned int offset)
>  {
>  	writew(data, hellcreek->ptp_base + offset);
>  }
> @@ -61,6 +62,24 @@ static u64 __hellcreek_ptp_gettime(struct hellcreek *hellcreek)
>  	return ns;
>  }
>  
> +/* Retrieve the seconds parts in nanoseconds for a packet timestamped with @ns.
> + * There has to be a check whether an overflow occurred between the packet
> + * arrival and now. If so use the correct seconds (-1) for calculating the
> + * packet arrival time.
> + */
> +u64 hellcreek_ptp_gettime_seconds(struct hellcreek *hellcreek, u64 ns)
> +{
> +	u64 s;
> +
> +	__hellcreek_ptp_gettime(hellcreek);
> +	if (hellcreek->last_ts > ns)
> +		s = hellcreek->seconds * NSEC_PER_SEC;
> +	else
> +		s = (hellcreek->seconds - 1) * NSEC_PER_SEC;
> +
> +	return s;
> +}
> +
>  static int hellcreek_ptp_gettime(struct ptp_clock_info *ptp,
>  				 struct timespec64 *ts)
>  {
> @@ -238,17 +257,18 @@ int hellcreek_ptp_setup(struct hellcreek *hellcreek)
>  	 * accumulator_overflow_rate shall not exceed 62.5 MHz (which adjusts
>  	 * the nominal frequency by 6.25%)
>  	 */
> -	hellcreek->ptp_clock_info.max_adj   = 62500000;
> -	hellcreek->ptp_clock_info.n_alarm   = 0;
> -	hellcreek->ptp_clock_info.n_pins    = 0;
> -	hellcreek->ptp_clock_info.n_ext_ts  = 0;
> -	hellcreek->ptp_clock_info.n_per_out = 0;
> -	hellcreek->ptp_clock_info.pps	    = 0;
> -	hellcreek->ptp_clock_info.adjfine   = hellcreek_ptp_adjfine;
> -	hellcreek->ptp_clock_info.adjtime   = hellcreek_ptp_adjtime;
> -	hellcreek->ptp_clock_info.gettime64 = hellcreek_ptp_gettime;
> -	hellcreek->ptp_clock_info.settime64 = hellcreek_ptp_settime;
> -	hellcreek->ptp_clock_info.enable    = hellcreek_ptp_enable;
> +	hellcreek->ptp_clock_info.max_adj     = 62500000;
> +	hellcreek->ptp_clock_info.n_alarm     = 0;
> +	hellcreek->ptp_clock_info.n_pins      = 0;
> +	hellcreek->ptp_clock_info.n_ext_ts    = 0;
> +	hellcreek->ptp_clock_info.n_per_out   = 0;
> +	hellcreek->ptp_clock_info.pps	      = 0;
> +	hellcreek->ptp_clock_info.adjfine     = hellcreek_ptp_adjfine;
> +	hellcreek->ptp_clock_info.adjtime     = hellcreek_ptp_adjtime;
> +	hellcreek->ptp_clock_info.gettime64   = hellcreek_ptp_gettime;
> +	hellcreek->ptp_clock_info.settime64   = hellcreek_ptp_settime;
> +	hellcreek->ptp_clock_info.enable      = hellcreek_ptp_enable;
> +	hellcreek->ptp_clock_info.do_aux_work = hellcreek_hwtstamp_work;
>  
>  	hellcreek->ptp_clock = ptp_clock_register(&hellcreek->ptp_clock_info,
>  						  hellcreek->dev);
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.h b/drivers/net/dsa/hirschmann/hellcreek_ptp.h
> index 2dd8aaa532d0..e0eca1f4a494 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.h
> @@ -59,6 +59,10 @@
>  
>  int hellcreek_ptp_setup(struct hellcreek *hellcreek);
>  void hellcreek_ptp_free(struct hellcreek *hellcreek);
> +u16 hellcreek_ptp_read(struct hellcreek *hellcreek, unsigned int offset);
> +void hellcreek_ptp_write(struct hellcreek *hellcreek, u16 data,
> +			 unsigned int offset);
> +u64 hellcreek_ptp_gettime_seconds(struct hellcreek *hellcreek, u64 ns);
>  
>  #define ptp_to_hellcreek(ptp)					\
>  	container_of(ptp, struct hellcreek, ptp_clock_info)
> -- 
> 2.20.1
>

On Sun Oct 04 2020, Vladimir Oltean wrote:
> On Sun, Oct 04, 2020 at 01:29:08PM +0200, Kurt Kanzenbach wrote:
>> +/* Enabling/disabling TX and RX HW timestamping for different PTP messages is
>> + * not available in the switch. Thus, this function only serves as a check if
>> + * the user requested what is actually available or not
>> + */
>
> Correct me if I'm wrong, but to the user it makes zero difference
> whether the hardware takes timestamps or not.

Why not? I think it makes a difference to the user b/o the precision.

> What matters is whether the skb will be delivered to the stack with a
> hardware timestamp or not, so you should definitely accept a
> hwtstamp_config with TX and RX timestamping disabled.
>

Sorry, I cannot follow you here.

Thanks,
Kurt

On Tue, Oct 06, 2020 at 08:27:42AM +0200, Kurt Kanzenbach wrote:
> On Sun Oct 04 2020, Vladimir Oltean wrote:
> > On Sun, Oct 04, 2020 at 01:29:08PM +0200, Kurt Kanzenbach wrote:
> >> +/* Enabling/disabling TX and RX HW timestamping for different PTP messages is
> >> + * not available in the switch. Thus, this function only serves as a check if
> >> + * the user requested what is actually available or not
> >> + */
> >
> > Correct me if I'm wrong, but to the user it makes zero difference
> > whether the hardware takes timestamps or not.
> 
> Why not? I think it makes a difference to the user b/o the precision.
> 
> > What matters is whether the skb will be delivered to the stack with a
> > hardware timestamp or not, so you should definitely accept a
> > hwtstamp_config with TX and RX timestamping disabled.
> >
> 
> Sorry, I cannot follow you here.

What I meant to say is that there is no reason you should refuse the
disabling of hardware timestamping. Even if that operation does not
really prevent the hardware from taking the timestamps, you simply
ignore the timestamps in the driver.

On Tue Oct 06 2020, Vladimir Oltean wrote:
> On Tue, Oct 06, 2020 at 08:27:42AM +0200, Kurt Kanzenbach wrote:
>> On Sun Oct 04 2020, Vladimir Oltean wrote:
>> > On Sun, Oct 04, 2020 at 01:29:08PM +0200, Kurt Kanzenbach wrote:
>> >> +/* Enabling/disabling TX and RX HW timestamping for different PTP messages is
>> >> + * not available in the switch. Thus, this function only serves as a check if
>> >> + * the user requested what is actually available or not
>> >> + */
>> >
>> > Correct me if I'm wrong, but to the user it makes zero difference
>> > whether the hardware takes timestamps or not.
>> 
>> Why not? I think it makes a difference to the user b/o the precision.
>> 
>> > What matters is whether the skb will be delivered to the stack with a
>> > hardware timestamp or not, so you should definitely accept a
>> > hwtstamp_config with TX and RX timestamping disabled.
>> >
>> 
>> Sorry, I cannot follow you here.
>
> What I meant to say is that there is no reason you should refuse the
> disabling of hardware timestamping. Even if that operation does not
> really prevent the hardware from taking the timestamps, you simply
> ignore the timestamps in the driver.

That's the point. The user (or anybody else) cannot disable hardware
stamping, because it is always performed. So, why should it be allowed
to disable it even when it cannot be disabled?

Thanks,
Kurt

On Tue, Oct 06, 2020 at 03:30:36PM +0200, Kurt Kanzenbach wrote:
> That's the point. The user (or anybody else) cannot disable hardware
> stamping, because it is always performed. So, why should it be allowed
> to disable it even when it cannot be disabled?

Because your driver's user can attach a PTP PHY to your switch port, and
the network stack doesn't support multiple TX timestamps attached to the
same skb. They'll want the TX timestamp from the PHY and not from your
switch.

On Tue Oct 06 2020, Vladimir Oltean wrote:
> On Tue, Oct 06, 2020 at 03:30:36PM +0200, Kurt Kanzenbach wrote:
>> That's the point. The user (or anybody else) cannot disable hardware
>> stamping, because it is always performed. So, why should it be allowed
>> to disable it even when it cannot be disabled?
>
> Because your driver's user can attach a PTP PHY to your switch port, and
> the network stack doesn't support multiple TX timestamps attached to the
> same skb. They'll want the TX timestamp from the PHY and not from your
> switch.

Yeah, sure. That use case makes sense. What's the problem exactly?

Thanks,
Kurt

On Tue, Oct 06, 2020 at 03:56:31PM +0200, Kurt Kanzenbach wrote:
> On Tue Oct 06 2020, Vladimir Oltean wrote:
> > On Tue, Oct 06, 2020 at 03:30:36PM +0200, Kurt Kanzenbach wrote:
> >> That's the point. The user (or anybody else) cannot disable hardware
> >> stamping, because it is always performed. So, why should it be allowed
> >> to disable it even when it cannot be disabled?
> >
> > Because your driver's user can attach a PTP PHY to your switch port, and
> > the network stack doesn't support multiple TX timestamps attached to the
> > same skb. They'll want the TX timestamp from the PHY and not from your
> > switch.
> 
> Yeah, sure. That use case makes sense. What's the problem exactly?

The SO_TIMESTAMPING / SO_TIMESTAMPNS cmsg socket API simply doesn't have
any sort of identification for a hardware TX timestamp (where it came
from). So when you'll poll for TX timestamps, you'll receive a TX
timestamp from the PHY and another one from the switch, and those will
be in a race with one another, so you won't know which one is which.

On Tue Oct 06 2020, Vladimir Oltean wrote:
> On Tue, Oct 06, 2020 at 03:56:31PM +0200, Kurt Kanzenbach wrote:
>> On Tue Oct 06 2020, Vladimir Oltean wrote:
>> > On Tue, Oct 06, 2020 at 03:30:36PM +0200, Kurt Kanzenbach wrote:
>> >> That's the point. The user (or anybody else) cannot disable hardware
>> >> stamping, because it is always performed. So, why should it be allowed
>> >> to disable it even when it cannot be disabled?
>> >
>> > Because your driver's user can attach a PTP PHY to your switch port, and
>> > the network stack doesn't support multiple TX timestamps attached to the
>> > same skb. They'll want the TX timestamp from the PHY and not from your
>> > switch.
>> 
>> Yeah, sure. That use case makes sense. What's the problem exactly?
>
> The SO_TIMESTAMPING / SO_TIMESTAMPNS cmsg socket API simply doesn't have
> any sort of identification for a hardware TX timestamp (where it came
> from).

This is sounds like a problem. For instance the hellcreek switch has
actually three ptp hardware clocks and the time stamping can be
configured to use either one of them. How would the user space
distinguish what time stamp is taken by which clock? This is not a
problem at the moment, because currently only the synchronized clock is
exported to user space. See change log of this patch.

> So when you'll poll for TX timestamps, you'll receive a TX
> timestamp from the PHY and another one from the switch, and those will
> be in a race with one another, so you won't know which one is which.

OK. So what happens if the driver will accept to disable hardware
timestamping? Is there anything else that needs to be implemented? Are
there (good) examples?

Thanks,
Kurt

On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach wrote:
> On Tue Oct 06 2020, Vladimir Oltean wrote:
> > On Tue, Oct 06, 2020 at 03:56:31PM +0200, Kurt Kanzenbach wrote:
> >> Yeah, sure. That use case makes sense. What's the problem exactly?
> >
> > The SO_TIMESTAMPING / SO_TIMESTAMPNS cmsg socket API simply doesn't have
> > any sort of identification for a hardware TX timestamp (where it came
> > from).
> 
> This is sounds like a problem.

Yeah, tell me about it.

> For instance the hellcreek switch has actually three ptp hardware
> clocks and the time stamping can be configured to use either one of
> them.

The sja1105 also has a corrected and an uncorrected PTP clock that can
take timestamps. Initially I had thought I'd be going to spend some time
figuring out multi-PHC support, but now I don't see any practical reason
to use the uncorrected PHC for anything.

> How would the user space distinguish what time stamp is taken by
> which clock? This is not a problem at the moment, because currently
> only the synchronized clock is exported to user space. See change log
> of this patch.

It wouldn't, of course. You'd need to add the plumbing for that.

> > So when you'll poll for TX timestamps, you'll receive a TX
> > timestamp from the PHY and another one from the switch, and those will
> > be in a race with one another, so you won't know which one is which.
> 
> OK. So what happens if the driver will accept to disable hardware
> timestamping? Is there anything else that needs to be implemented? Are
> there (good) examples?

It needs to not call skb_complete_tx_timestamp() and friends.

For PHY timestamping, it also needs to invoke the correct methods for RX
and for TX, where the PHY timestamping hooks will get called. I don't
think that DSA is compatible yet with PHY timestamping, but it is
probably a trivial modification. Please read
Documentation/networking/timestamping.rst, we try to keep it fairly
comprehensive.

Thanks,
-Vladimir

On Wed Oct 07 2020, Vladimir Oltean wrote:
> On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach wrote:
>> For instance the hellcreek switch has actually three ptp hardware
>> clocks and the time stamping can be configured to use either one of
>> them.
>
> The sja1105 also has a corrected and an uncorrected PTP clock that can
> take timestamps. Initially I had thought I'd be going to spend some time
> figuring out multi-PHC support, but now I don't see any practical reason
> to use the uncorrected PHC for anything.

Just out of curiosity: How do you implement 802.1AS then? My
understanding is that the free-running clock has to be used for the
calculation of the peer delays and such meaning there should be a way to
get access to both PHCs or having some form of cross timestamping
available.

The hellcreek switch can take cross snapshots of all three ptp clocks in
hardware for that purpose.

>> > So when you'll poll for TX timestamps, you'll receive a TX
>> > timestamp from the PHY and another one from the switch, and those will
>> > be in a race with one another, so you won't know which one is which.
>> 
>> OK. So what happens if the driver will accept to disable hardware
>> timestamping? Is there anything else that needs to be implemented? Are
>> there (good) examples?
>
> It needs to not call skb_complete_tx_timestamp() and friends.
>
> For PHY timestamping, it also needs to invoke the correct methods for RX
> and for TX, where the PHY timestamping hooks will get called. I don't
> think that DSA is compatible yet with PHY timestamping, but it is
> probably a trivial modification.

Hmm? If DSA doesn't support PHY timestamping how are other DSA drivers
dealing with it then? I'm getting really confused.

Furthermore, there is no hellcreek hardware available with timestamping
capable PHYs. How am I supposed to even test this?

For now, until there is hardware available, PHY timestamping is not
supported with the hellcreek switch.

Thanks,
Kurt

On Thu, Oct 08, 2020 at 10:34:11AM +0200, Kurt Kanzenbach wrote:
> On Wed Oct 07 2020, Vladimir Oltean wrote:
> > On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach wrote:
> >> For instance the hellcreek switch has actually three ptp hardware
> >> clocks and the time stamping can be configured to use either one of
> >> them.
> >
> > The sja1105 also has a corrected and an uncorrected PTP clock that can
> > take timestamps. Initially I had thought I'd be going to spend some time
> > figuring out multi-PHC support, but now I don't see any practical reason
> > to use the uncorrected PHC for anything.
> 
> Just out of curiosity: How do you implement 802.1AS then? My
> understanding is that the free-running clock has to be used for the

Has to be? I couldn't find that wording in IEEE 802.1AS-2011.

> calculation of the peer delays and such meaning there should be a way to
> get access to both PHCs or having some form of cross timestamping
> available.
> 
> The hellcreek switch can take cross snapshots of all three ptp clocks in
> hardware for that purpose.

Well, at the end of the day, all the other TSN offloads (tc-taprio,
tc-gate) will still have to use the synchronized PTP clock, so what
we're doing is we're simply letting that clock be synchronized by ptp4l.

> >> > So when you'll poll for TX timestamps, you'll receive a TX
> >> > timestamp from the PHY and another one from the switch, and those will
> >> > be in a race with one another, so you won't know which one is which.
> >> 
> >> OK. So what happens if the driver will accept to disable hardware
> >> timestamping? Is there anything else that needs to be implemented? Are
> >> there (good) examples?
> >
> > It needs to not call skb_complete_tx_timestamp() and friends.
> >
> > For PHY timestamping, it also needs to invoke the correct methods for RX
> > and for TX, where the PHY timestamping hooks will get called. I don't
> > think that DSA is compatible yet with PHY timestamping, but it is
> > probably a trivial modification.
> 
> Hmm? If DSA doesn't support PHY timestamping how are other DSA drivers
> dealing with it then? I'm getting really confused.

They aren't dealing with it, of course.

> Furthermore, there is no hellcreek hardware available with timestamping
> capable PHYs. How am I supposed to even test this?
> 
> For now, until there is hardware available, PHY timestamping is not
> supported with the hellcreek switch.

I was just pointing out that this is something you'll certainly have to
change if somebody will want PHY timestamping.

Even without hardware, you _could_ probably test that DSA is doing the
right thing by simply adding the PTP timestamping ops to a PHY driver
that you own, and inject dummy timestamps. The expectation becomes that
user space gets those dummy timestamps, and not the ones emitted by your
switch.

On Thu Oct 08 2020, Vladimir Oltean wrote:
> On Thu, Oct 08, 2020 at 10:34:11AM +0200, Kurt Kanzenbach wrote:
>> On Wed Oct 07 2020, Vladimir Oltean wrote:
>> > On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach wrote:
>> >> For instance the hellcreek switch has actually three ptp hardware
>> >> clocks and the time stamping can be configured to use either one of
>> >> them.
>> >
>> > The sja1105 also has a corrected and an uncorrected PTP clock that can
>> > take timestamps. Initially I had thought I'd be going to spend some time
>> > figuring out multi-PHC support, but now I don't see any practical reason
>> > to use the uncorrected PHC for anything.
>> 
>> Just out of curiosity: How do you implement 802.1AS then? My
>> understanding is that the free-running clock has to be used for the
>
> Has to be? I couldn't find that wording in IEEE 802.1AS-2011.

It doesn't has to be, it *should* be. That's at least the outcome we had
after lots of discussions. Actually Kamil (on Cc) is the expert on this
topic.

>
>> calculation of the peer delays and such meaning there should be a way to
>> get access to both PHCs or having some form of cross timestamping
>> available.
>> 
>> The hellcreek switch can take cross snapshots of all three ptp clocks in
>> hardware for that purpose.
>
> Well, at the end of the day, all the other TSN offloads (tc-taprio,
> tc-gate) will still have to use the synchronized PTP clock, so what
> we're doing is we're simply letting that clock be synchronized by
> ptp4l.

Yes, the synchronized clock is of course needed for the traffic
scheduling and so on. This is what we do here in this code as well. Only
the synchronized one is exported to user space and used. However, the
multi PHCs issue should be addressed as well at some point.

>
>> >> > So when you'll poll for TX timestamps, you'll receive a TX
>> >> > timestamp from the PHY and another one from the switch, and those will
>> >> > be in a race with one another, so you won't know which one is which.
>> >> 
>> >> OK. So what happens if the driver will accept to disable hardware
>> >> timestamping? Is there anything else that needs to be implemented? Are
>> >> there (good) examples?
>> >
>> > It needs to not call skb_complete_tx_timestamp() and friends.
>> >
>> > For PHY timestamping, it also needs to invoke the correct methods for RX
>> > and for TX, where the PHY timestamping hooks will get called. I don't
>> > think that DSA is compatible yet with PHY timestamping, but it is
>> > probably a trivial modification.
>> 
>> Hmm? If DSA doesn't support PHY timestamping how are other DSA drivers
>> dealing with it then? I'm getting really confused.
>
> They aren't dealing with it, of course.
>
>> Furthermore, there is no hellcreek hardware available with timestamping
>> capable PHYs. How am I supposed to even test this?
>> 
>> For now, until there is hardware available, PHY timestamping is not
>> supported with the hellcreek switch.
>
> I was just pointing out that this is something you'll certainly have to
> change if somebody will want PHY timestamping.

Understood.

>
> Even without hardware, you _could_ probably test that DSA is doing the
> right thing by simply adding the PTP timestamping ops to a PHY driver
> that you own, and inject dummy timestamps. The expectation becomes that
> user space gets those dummy timestamps, and not the ones emitted by your
> switch.

Of course it can be mocked. Whenever somebody wants to do PHY
timestamping with a hellcreek switch this issue can be re-visited.

Thanks,
Kurt

Hello dears,

On Thu, 2020-10-08 at 12:01 +0200, Kurt Kanzenbach wrote:
> On Thu Oct 08 2020, Vladimir Oltean wrote:
> > On Thu, Oct 08, 2020 at 10:34:11AM +0200, Kurt Kanzenbach wrote:
> > > On Wed Oct 07 2020, Vladimir Oltean wrote:
> > > > On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach
> > > > wrote:
> > > > > For instance the hellcreek switch has actually three ptp
> > > > > hardware
> > > > > clocks and the time stamping can be configured to use either
> > > > > one of
> > > > > them.
> > > > 
> > > > The sja1105 also has a corrected and an uncorrected PTP clock
> > > > that can
> > > > take timestamps. Initially I had thought I'd be going to spend
> > > > some time
> > > > figuring out multi-PHC support, but now I don't see any
> > > > practical reason
> > > > to use the uncorrected PHC for anything.
> > > 
> > > Just out of curiosity: How do you implement 802.1AS then? My
> > > understanding is that the free-running clock has to be used for
> > > the
> > 
> > Has to be? I couldn't find that wording in IEEE 802.1AS-2011.
> 
> It doesn't has to be, it *should* be. That's at least the outcome we
> had
> after lots of discussions. Actually Kamil (on Cc) is the expert on
> this
> topic.

According to 802.1AS-2011 (10.1.1): "The LocalClock entity is a free-
running clock (see 3.3) that provides a common time to the time-aware
system, relative to an arbitrary epoch.", "... All timestamps are taken
relative to the LocalClock entity". The same statement holds true for
802.1AS-2020 (10.1.2.1).

> > > calculation of the peer delays and such meaning there should be a
> > > way to
> > > get access to both PHCs or having some form of cross timestamping
> > > available.
> > > 
> > > The hellcreek switch can take cross snapshots of all three ptp
> > > clocks in
> > > hardware for that purpose.
> > 
> > Well, at the end of the day, all the other TSN offloads (tc-taprio,
> > tc-gate) will still have to use the synchronized PTP clock, so what
> > we're doing is we're simply letting that clock be synchronized by
> > ptp4l.
> 
> Yes, the synchronized clock is of course needed for the traffic
> scheduling and so on. This is what we do here in this code as well.
> Only
> the synchronized one is exported to user space and used. However, the
> multi PHCs issue should be addressed as well at some point.
> 
> > > > > > So when you'll poll for TX timestamps, you'll receive a TX
> > > > > > timestamp from the PHY and another one from the switch, and
> > > > > > those will
> > > > > > be in a race with one another, so you won't know which one
> > > > > > is which.
> > > > > 
> > > > > OK. So what happens if the driver will accept to disable
> > > > > hardware
> > > > > timestamping? Is there anything else that needs to be
> > > > > implemented? Are
> > > > > there (good) examples?
> > > > 
> > > > It needs to not call skb_complete_tx_timestamp() and friends.
> > > > 
> > > > For PHY timestamping, it also needs to invoke the correct
> > > > methods for RX
> > > > and for TX, where the PHY timestamping hooks will get called. I
> > > > don't
> > > > think that DSA is compatible yet with PHY timestamping, but it
> > > > is
> > > > probably a trivial modification.
> > > 
> > > Hmm? If DSA doesn't support PHY timestamping how are other DSA
> > > drivers
> > > dealing with it then? I'm getting really confused.
> > 
> > They aren't dealing with it, of course.
> > 
> > > Furthermore, there is no hellcreek hardware available with
> > > timestamping
> > > capable PHYs. How am I supposed to even test this?
> > > 
> > > For now, until there is hardware available, PHY timestamping is
> > > not
> > > supported with the hellcreek switch.
> > 
> > I was just pointing out that this is something you'll certainly
> > have to
> > change if somebody will want PHY timestamping.
> 
> Understood.
> 
> > Even without hardware, you _could_ probably test that DSA is doing
> > the
> > right thing by simply adding the PTP timestamping ops to a PHY
> > driver
> > that you own, and inject dummy timestamps. The expectation becomes
> > that
> > user space gets those dummy timestamps, and not the ones emitted by
> > your
> > switch.
> 
> Of course it can be mocked. Whenever somebody wants to do PHY
> timestamping with a hellcreek switch this issue can be re-visited.
> 
> Thanks,
> Kurt

Hi Kamil,

On Thu, Oct 08, 2020 at 02:55:57PM +0200, Kamil Alkhouri wrote:
> Hello dears,
> 
> On Thu, 2020-10-08 at 12:01 +0200, Kurt Kanzenbach wrote:
> > On Thu Oct 08 2020, Vladimir Oltean wrote:
> > > On Thu, Oct 08, 2020 at 10:34:11AM +0200, Kurt Kanzenbach wrote:
> > > > On Wed Oct 07 2020, Vladimir Oltean wrote:
> > > > > On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach
> > > > > wrote:
> > > > > > For instance the hellcreek switch has actually three ptp
> > > > > > hardware
> > > > > > clocks and the time stamping can be configured to use either
> > > > > > one of
> > > > > > them.
> > > > > 
> > > > > The sja1105 also has a corrected and an uncorrected PTP clock
> > > > > that can
> > > > > take timestamps. Initially I had thought I'd be going to spend
> > > > > some time
> > > > > figuring out multi-PHC support, but now I don't see any
> > > > > practical reason
> > > > > to use the uncorrected PHC for anything.
> > > > 
> > > > Just out of curiosity: How do you implement 802.1AS then? My
> > > > understanding is that the free-running clock has to be used for
> > > > the
> > > 
> > > Has to be? I couldn't find that wording in IEEE 802.1AS-2011.
> > 
> > It doesn't has to be, it *should* be. That's at least the outcome we
> > had
> > after lots of discussions. Actually Kamil (on Cc) is the expert on
> > this
> > topic.
> 
> According to 802.1AS-2011 (10.1.1): "The LocalClock entity is a free-
> running clock (see 3.3) that provides a common time to the time-aware
> system, relative to an arbitrary epoch.", "... All timestamps are taken
> relative to the LocalClock entity". The same statement holds true for
> 802.1AS-2020 (10.1.2.1).

Nice having you part of the discussion.

IEEE 802.1AS-rev draft 8.0, clause F.3 PTP options:

	The physical adjustment of the frequency of the LocalClock
	entity (i.e., physical syntonization) is allowed but not
	required.

In fact, even if that wasn't explicitly written, I am having a hard time
understanding how the "B.1.1 Frequency accuracy" requirement for the
LocalClock could be satisfied as long as it is kept free-running.
Otherwise said, what should I do as a system designer if the
LocalClock's frequency is not within +/- 100 ppm offset to the TAI
frequency, and I'm not allowed to correct it.

By the way, how would you see the split between an unsynchronized and a
synchronized PHC be implemented in the Linux kernel?

Thanks,
-Vladimir

Hi Vladimir,

On Thu, 2020-10-08 at 18:09 +0300, Vladimir Oltean wrote:
> Hi Kamil,
> 
> On Thu, Oct 08, 2020 at 02:55:57PM +0200, Kamil Alkhouri wrote:
> > Hello dears,
> > 
> > On Thu, 2020-10-08 at 12:01 +0200, Kurt Kanzenbach wrote:
> > > On Thu Oct 08 2020, Vladimir Oltean wrote:
> > > > On Thu, Oct 08, 2020 at 10:34:11AM +0200, Kurt Kanzenbach
> > > > wrote:
> > > > > On Wed Oct 07 2020, Vladimir Oltean wrote:
> > > > > > On Wed, Oct 07, 2020 at 12:39:49PM +0200, Kurt Kanzenbach
> > > > > > wrote:
> > > > > > > For instance the hellcreek switch has actually three ptp
> > > > > > > hardware
> > > > > > > clocks and the time stamping can be configured to use
> > > > > > > either
> > > > > > > one of
> > > > > > > them.
> > > > > > 
> > > > > > The sja1105 also has a corrected and an uncorrected PTP
> > > > > > clock
> > > > > > that can
> > > > > > take timestamps. Initially I had thought I'd be going to
> > > > > > spend
> > > > > > some time
> > > > > > figuring out multi-PHC support, but now I don't see any
> > > > > > practical reason
> > > > > > to use the uncorrected PHC for anything.
> > > > > 
> > > > > Just out of curiosity: How do you implement 802.1AS then? My
> > > > > understanding is that the free-running clock has to be used
> > > > > for
> > > > > the
> > > > 
> > > > Has to be? I couldn't find that wording in IEEE 802.1AS-2011.
> > > 
> > > It doesn't has to be, it *should* be. That's at least the outcome
> > > we
> > > had
> > > after lots of discussions. Actually Kamil (on Cc) is the expert
> > > on
> > > this
> > > topic.
> > 
> > According to 802.1AS-2011 (10.1.1): "The LocalClock entity is a
> > free-
> > running clock (see 3.3) that provides a common time to the time-
> > aware
> > system, relative to an arbitrary epoch.", "... All timestamps are
> > taken
> > relative to the LocalClock entity". The same statement holds true
> > for
> > 802.1AS-2020 (10.1.2.1).
> 
> Nice having you part of the discussion.
> 
> IEEE 802.1AS-rev draft 8.0, clause F.3 PTP options:
> 
> 	The physical adjustment of the frequency of the LocalClock
> 	entity (i.e., physical syntonization) is allowed but not
> 	required.

what about phase adjustment?
I believe logical syntonization is a main part of 802.1AS-Rev and it is
actually mandatory (7.5.g). Even though physical syntonization is
allowed, the standard clearly states that it is slow and prone to gain
peaking effects (7.3.3). Therefore, it makes sense to use a free-
running clock to get the most benefit of AS-Rev when it comes to the
transport of synchronization information. 

> 
> In fact, even if that wasn't explicitly written, I am having a hard
> time
> understanding how the "B.1.1 Frequency accuracy" requirement for the
> LocalClock could be satisfied as long as it is kept free-running.
> Otherwise said, what should I do as a system designer if the
> LocalClock's frequency is not within +/- 100 ppm offset to the TAI
> frequency, and I'm not allowed to correct it.

B.1.1 defines the frequency accuracy of the local clock relative to TAI
and not to grandmaster. In my opinion, this is a physical requirement
of the quartz oscillator used to drive the time and it should be
fulfilled for all local clocks even for the ones in non-slave devices.

> 
> By the way, how would you see the split between an unsynchronized and
> a
> synchronized PHC be implemented in the Linux kernel?

I'm not an expert in kernel implementation but we have plans to discuss
possible approaches in the near future.

> 
> Thanks,
> -Vladimir

Thanks,
Kamil

On Mon, Oct 12, 2020 at 02:53:58PM +0200, Kamil Alkhouri wrote:
> > By the way, how would you see the split between an unsynchronized and
> > a
> > synchronized PHC be implemented in the Linux kernel?

If you want, you can run your PHC using the linuxptp "free_running"
option.  Then, you can use the TIME_STATUS_NP management request to
use the remote time signal in your application.

> I'm not an expert in kernel implementation but we have plans to discuss
> possible approaches in the near future.

I don't see any need for kernel changes in this area.

Thanks,
Richard

On Mon, Oct 12, 2020 at 02:42:54PM -0700, Richard Cochran wrote:
> If you want, you can run your PHC using the linuxptp "free_running"
> option.  Then, you can use the TIME_STATUS_NP management request to
> use the remote time signal in your application.

I was expecting some sort of reaction to this from Kamil or Kurt.

I don't think that 'using the remote time signal in an application' is
all that needs to be done with the gPTP time, at least for a switch with
the hardware features that hellcreek has. Ultimately it should be fed
back into the hardware, such that the scheduler based on 802.1Q clause
8.6.8.4 "Enhancements for scheduled traffic" has some time scale based
on which it can run. Running tc-taprio offload on top of an
unsynchronized clock is not something productive.

So the discussion is about how to have the cake and eat it at the same
time. Silicon vendors eager to follow the latest trends in standards are
implementing hybrid PTP clocks, where an unsynchronizable version of the
clock delivers MAC timestamps to the application stack, and a
synchronizable wrapper over that same clock is what gets fed into the
offloading engines, like the ones behind the tc-taprio and tc-gate
offload. Some of these vendors perform cross-timestamping (they deliver
a timestamp from the MAC with 2, or 3, or 4, timestamps, depending on
how many PHCs that MAC has wired to it), some don't, and just deliver a
single timestamp from a configurable source.

The operating system is supposed to ??? in order to synchronize the
synchronizable clock to the virtual time retrieved via TIME_STATUS_NP
that you're talking about. The question is what to replace that ???
with, of course.

> > I'm not an expert in kernel implementation but we have plans to discuss
> > possible approaches in the near future.
>
> I don't see any need for kernel changes in this area.

I'm not an expert in kernel implementation either, but perhaps in the
light of this, you can revisit the idea that kernel changes will not be
needed (or explain more, if you still think they aren't).

Since IEEE 60802 keeps talking about multiple time domains to be used
with 802.1AS-rev (a 'universal clock domain' and a 'working clock
domain'), a decision needs to be taken somewhere about which time base
you're going to use as a source for synchronizing your tc-taprio clock.
That decision should best be taken at the application level, so in my
opinion this is an argument that the application should have explicit
access to the unsynchronizable and to the synchronizable versions of the
PTP clock.

In the Linux kernel API, a network interface can have at most one PHC.

--------------

DISCLAIMER
Yes, I know full well that everyone can write a standard, but not
everyone can implement one. At the end of the day, I'm not trying to
make an argument whether the end result is worth making all these
changes. I'm only here to learn what other people are doing, how, and
most importantly, why.

On Wed, Oct 14, 2020 at 12:57:47PM +0300, Vladimir Oltean wrote:
> So the discussion is about how to have the cake and eat it at the same
> time.

And I wish for a pony.  With sparkles.  And a unicorn.  And a rainbow.

> Silicon vendors eager to follow the latest trends in standards are
> implementing hybrid PTP clocks, where an unsynchronizable version of the
> clock delivers MAC timestamps to the application stack, and a
> synchronizable wrapper over that same clock is what gets fed into the
> offloading engines, like the ones behind the tc-taprio and tc-gate
> offload. Some of these vendors perform cross-timestamping (they deliver
> a timestamp from the MAC with 2, or 3, or 4, timestamps, depending on
> how many PHCs that MAC has wired to it), some don't, and just deliver a
> single timestamp from a configurable source.

Sounds like it will be nearly impossible to make a single tc-taprio
framework that fits all the hardware variants.

> The operating system is supposed to ??? in order to synchronize the
> synchronizable clock to the virtual time retrieved via TIME_STATUS_NP
> that you're talking about. The question is what to replace that ???
> with, of course.

You have a choice.  Either you synchronize the local PHC to the global
TAI time base or not.  If you do synchronize the PHC, then everything
(like the globally scheduled time slots) just works.  If you decide to
follow the nonsensical idea (following 802.1-AS) and leave the PHC
free running, then you will have a difficult time scheduling those
time windows.

So it is all up to you.

> I'm not an expert in kernel implementation either, but perhaps in the
> light of this, you can revisit the idea that kernel changes will not be
> needed (or explain more, if you still think they aren't).

I am not opposed to kernel changes, but there must be:

- A clear statement of the background context, and
- an explanation of the issue to solved, and
- a realistic solution that will support the wide variety of HW. 

> DISCLAIMER
> Yes, I know full well that everyone can write a standard, but not
> everyone can implement one. At the end of the day, I'm not trying to
> make an argument whether the end result is worth making all these
> changes.

+1

That is the question.  You can easily solve this issue by simply
synchronizing the PHC to the global time base.

Thanks,
Richard

On Wed Oct 14 2020, Richard Cochran wrote:
> On Wed, Oct 14, 2020 at 12:57:47PM +0300, Vladimir Oltean wrote:
>> So the discussion is about how to have the cake and eat it at the same
>> time.
>
> And I wish for a pony.  With sparkles.  And a unicorn.  And a rainbow.
>
>> Silicon vendors eager to follow the latest trends in standards are
>> implementing hybrid PTP clocks, where an unsynchronizable version of the
>> clock delivers MAC timestamps to the application stack, and a
>> synchronizable wrapper over that same clock is what gets fed into the
>> offloading engines, like the ones behind the tc-taprio and tc-gate
>> offload. Some of these vendors perform cross-timestamping (they deliver
>> a timestamp from the MAC with 2, or 3, or 4, timestamps, depending on
>> how many PHCs that MAC has wired to it), some don't, and just deliver a
>> single timestamp from a configurable source.
>
> Sounds like it will be nearly impossible to make a single tc-taprio
> framework that fits all the hardware variants.

Why? All the gate operations work on the synchronized clock. I assume
all Qbv capable switches have a synchronized clock?

It's just that some switches have multiple PHCs instead of a single
one. It seems to be quite common to have a free-running as well as a
synchronized clock. In order for a better(?) or more accurate(?) ptp
implementation they expose not a single but rather multiple timestamps
from all PHCs (-> cross-timestamping) to user space for the ptp event
messages. That's at least my very limited understanding.

>
>> The operating system is supposed to ??? in order to synchronize the
>> synchronizable clock to the virtual time retrieved via TIME_STATUS_NP
>> that you're talking about. The question is what to replace that ???
>> with, of course.
>
> You have a choice.  Either you synchronize the local PHC to the global
> TAI time base or not.  If you do synchronize the PHC, then everything
> (like the globally scheduled time slots) just works.  If you decide to
> follow the nonsensical idea (following 802.1-AS) and leave the PHC
> free running, then you will have a difficult time scheduling those
> time windows.
>
> So it is all up to you.
>
>> I'm not an expert in kernel implementation either, but perhaps in the
>> light of this, you can revisit the idea that kernel changes will not be
>> needed (or explain more, if you still think they aren't).
>
> I am not opposed to kernel changes, but there must be:
>
> - A clear statement of the background context, and
> - an explanation of the issue to solved, and
> - a realistic solution that will support the wide variety of HW. 

Agreed.

Thanks,
Kurt