[net-next, v2, 0/2] enetc: support PTP Sync packet one-step timestamping

[net-next, v2, 0/2] enetc: support PTP Sync packet one-step timestamping

[Yangbo Lu]

This patch-set is to add one-step timestamping support for PTP Sync
packet. Since ENETC single-step register has to be configured dynamically
per packet for correctionField offeset and UDP checksum update, current
one-step timestamping packet has to be sent only when the last one
completes transmitting on hardware. So, on the TX the patch implements
below process:

- For one-step timestamping packet, queue to skb queue.
- Start a work to transmit skbs in queue.
- For other skbs, transmit immediately.
- mutex lock used to ensure the last one-step timestamping packet has
  already been transmitted on hardware before transmitting current one.

Changes for v2:
	- Rebased.
	- Fixed issues from patchwork checks.
	- netif_tx_lock for one-step timestamping packet sending.

Yangbo Lu (2):
  enetc: mark TX timestamp type per skb
  enetc: support PTP Sync packet one-step timestamping

 drivers/net/ethernet/freescale/enetc/enetc.c  | 212 ++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h  |  24 +-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |   3 +-
 .../net/ethernet/freescale/enetc/enetc_hw.h   |   7 +
 4 files changed, 220 insertions(+), 26 deletions(-)


base-commit: 3cd52c1e32fe7dfee09815ced702db9ee9f84ec9
-- 
2.25.1

[net-next, v2, 1/2] enetc: mark TX timestamp type per skb

[Yangbo Lu]

Mark TX timestamp type per skb on skb->cb[0], instead of
global variable for all skbs. This is a preparation for
one step timestamp support.

For one-step timestamping enablement, there will be both
one-step and two-step PTP messages to transfer. And a skb
queue is needed for one-step PTP messages making sure
start to send current message only after the last one
completed on hardware. (ENETC single-step register has to
be dynamically configured per message.) So, marking TX
timestamp type per skb is required.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- Rebased.
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 10 ++++++----
 drivers/net/ethernet/freescale/enetc/enetc.h | 11 +++++++----
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 57049ae97201..61bd2981517c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -67,8 +67,7 @@ static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
 	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);
 }
 
-static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb,
-			      int active_offloads)
+static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 {
 	struct enetc_tx_swbd *tx_swbd;
 	skb_frag_t *frag;
@@ -101,7 +100,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 	count++;
 
 	do_vlan = skb_vlan_tag_present(skb);
-	do_tstamp = (active_offloads & ENETC_F_TX_TSTAMP) &&
+	do_tstamp = (skb->cb[0] & ENETC_F_TX_TSTAMP) &&
 		    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP);
 	tx_swbd->do_tstamp = do_tstamp;
 	tx_swbd->check_wb = tx_swbd->do_tstamp;
@@ -221,6 +220,9 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct enetc_bdr *tx_ring;
 	int count;
 
+	/* cb[0] used for TX timestamp type */
+	skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
+
 	tx_ring = priv->tx_ring[skb->queue_mapping];
 
 	if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
@@ -234,7 +236,7 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	}
 
 	enetc_lock_mdio();
-	count = enetc_map_tx_buffs(tx_ring, skb, priv->active_offloads);
+	count = enetc_map_tx_buffs(tx_ring, skb);
 	enetc_unlock_mdio();
 
 	if (unlikely(!count))
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 05474f46b0d9..96889529383e 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -271,12 +271,15 @@ struct psfp_cap {
 	u32 max_psfp_meter;
 };
 
+#define ENETC_F_TX_TSTAMP_MASK	0xff
 /* TODO: more hardware offloads */
 enum enetc_active_offloads {
-	ENETC_F_RX_TSTAMP	= BIT(0),
-	ENETC_F_TX_TSTAMP	= BIT(1),
-	ENETC_F_QBV             = BIT(2),
-	ENETC_F_QCI		= BIT(3),
+	/* 8 bits reserved for TX timestamp types (hwtstamp_tx_types) */
+	ENETC_F_TX_TSTAMP	= BIT(0),
+
+	ENETC_F_RX_TSTAMP	= BIT(8),
+	ENETC_F_QBV		= BIT(9),
+	ENETC_F_QCI		= BIT(10),
 };
 
 /* interrupt coalescing modes */
-- 
2.25.1

[net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Yangbo Lu]

This patch is to add support for PTP Sync packet one-step timestamping.
Since ENETC single-step register has to be configured dynamically per
packet for correctionField offeset and UDP checksum update, current
one-step timestamping packet has to be sent only when the last one
completes transmitting on hardware. So, on the TX below things are done
by the patch:

- For one-step timestamping packet, queue to skb queue.
- Start a work to transmit skbs in queue.
- For other skbs, transmit immediately.
- mutex lock used to ensure the last one-step timestamping packet has
  already been transmitted on hardware before transmitting current one.

And the configuration for one-step timestamping on ENETC before
transmitting is,

- Set one-step timestamping flag in extension BD.
- Write 30 bits current timestamp in tstamp field of extension BD.
- Update PTP Sync packet originTimestamp field with current timestamp.
- Configure single-step register for correctionField offeset and UDP
  checksum update.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- Rebased.
	- Fixed issues from patchwork checks.
	- netif_tx_lock for one-step timestamping packet sending.
---
 drivers/net/ethernet/freescale/enetc/enetc.c  | 210 ++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h  |  21 +-
 .../ethernet/freescale/enetc/enetc_ethtool.c  |   3 +-
 .../net/ethernet/freescale/enetc/enetc_hw.h   |   7 +
 4 files changed, 215 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 61bd2981517c..a14ce943e14c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -6,6 +6,7 @@
 #include <linux/tcp.h>
 #include <linux/udp.h>
 #include <linux/vmalloc.h>
+#include <linux/ptp_classify.h>
 #include <net/pkt_sched.h>
 
 static struct sk_buff *enetc_tx_swbd_get_skb(struct enetc_tx_swbd *tx_swbd)
@@ -67,15 +68,52 @@ static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
 	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);
 }
 
+static int enetc_ptp_parse(struct sk_buff *skb, u8 *udp,
+			   u8 *msgtype, u8 *twostep,
+			   u16 *correction_offset, u16 *body_offset)
+{
+	unsigned int ptp_class;
+	struct ptp_header *hdr;
+	unsigned int type;
+	u8 *base;
+
+	ptp_class = ptp_classify_raw(skb);
+	if (ptp_class == PTP_CLASS_NONE)
+		return -EINVAL;
+
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
+		return -EINVAL;
+
+	type = ptp_class & PTP_CLASS_PMASK;
+	if (type == PTP_CLASS_IPV4 || type == PTP_CLASS_IPV6)
+		*udp = 1;
+	else
+		*udp = 0;
+
+	*msgtype = ptp_get_msgtype(hdr, ptp_class);
+	*twostep = hdr->flag_field[0] & 0x2;
+
+	base = skb_mac_header(skb);
+	*correction_offset = (u8 *)&hdr->correction - base;
+	*body_offset = (u8 *)hdr + sizeof(struct ptp_header) - base;
+
+	return 0;
+}
+
 static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 {
+	bool do_vlan, do_onestep_tstamp = false, do_twostep_tstamp = false;
+	struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
+	struct enetc_hw *hw = &priv->si->hw;
 	struct enetc_tx_swbd *tx_swbd;
-	skb_frag_t *frag;
 	int len = skb_headlen(skb);
 	union enetc_tx_bd temp_bd;
+	u8 msgtype, twostep, udp;
 	union enetc_tx_bd *txbd;
-	bool do_vlan, do_tstamp;
+	u16 offset1, offset2;
 	int i, count = 0;
+	skb_frag_t *frag;
 	unsigned int f;
 	dma_addr_t dma;
 	u8 flags = 0;
@@ -100,12 +138,21 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 	count++;
 
 	do_vlan = skb_vlan_tag_present(skb);
-	do_tstamp = (skb->cb[0] & ENETC_F_TX_TSTAMP) &&
-		    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP);
-	tx_swbd->do_tstamp = do_tstamp;
-	tx_swbd->check_wb = tx_swbd->do_tstamp;
+	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
+		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep, &offset1,
+				    &offset2) ||
+		    msgtype != PTP_MSGTYPE_SYNC || twostep)
+			WARN_ONCE(1, "Bad packet for one-step timestamping\n");
+		else
+			do_onestep_tstamp = true;
+	} else if (skb->cb[0] & ENETC_F_TX_TSTAMP) {
+		do_twostep_tstamp = true;
+	}
+
+	tx_swbd->do_twostep_tstamp = do_twostep_tstamp;
+	tx_swbd->check_wb = tx_swbd->do_twostep_tstamp;
 
-	if (do_vlan || do_tstamp)
+	if (do_vlan || do_onestep_tstamp || do_twostep_tstamp)
 		flags |= ENETC_TXBD_FLAGS_EX;
 
 	if (tx_ring->tsd_enable)
@@ -142,7 +189,40 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 			e_flags |= ENETC_TXBD_E_FLAGS_VLAN_INS;
 		}
 
-		if (do_tstamp) {
+		if (do_onestep_tstamp) {
+			u32 lo, hi, val;
+			u64 sec, nsec;
+			u8 *data;
+
+			lo = enetc_rd_hot(hw, ENETC_SICTR0);
+			hi = enetc_rd_hot(hw, ENETC_SICTR1);
+			sec = (u64)hi << 32 | lo;
+			nsec = do_div(sec, 1000000000);
+
+			/* Configure extension BD */
+			temp_bd.ext.tstamp = cpu_to_le32(lo & 0x3fffffff);
+			e_flags |= ENETC_TXBD_E_FLAGS_ONE_STEP_PTP;
+
+			/* Update originTimestamp field of Sync packet
+			 * - 48 bits seconds field
+			 * - 32 bits nanseconds field
+			 */
+			data = skb_mac_header(skb);
+			*(__be16 *)(data + offset2) =
+				htons((sec >> 32) & 0xffff);
+			*(__be32 *)(data + offset2 + 2) =
+				htonl(sec & 0xffffffff);
+			*(__be32 *)(data + offset2 + 6) = htonl(nsec);
+
+			/* Configure single-step register */
+			val = ENETC_PM0_SINGLE_STEP_EN;
+			val |= ENETC_SET_SINGLE_STEP_OFFSET(offset1);
+			if (udp)
+				val |= ENETC_PM0_SINGLE_STEP_CH;
+
+			enetc_port_wr(hw, ENETC_PM0_SINGLE_STEP, val);
+			enetc_port_wr(hw, ENETC_PM1_SINGLE_STEP, val);
+		} else if (do_twostep_tstamp) {
 			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 			e_flags |= ENETC_TXBD_E_FLAGS_TWO_STEP_PTP;
 		}
@@ -214,15 +294,13 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb)
 	return 0;
 }
 
-netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
+				    struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct enetc_bdr *tx_ring;
 	int count;
 
-	/* cb[0] used for TX timestamp type */
-	skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
-
 	tx_ring = priv->tx_ring[skb->queue_mapping];
 
 	if (unlikely(skb_shinfo(skb)->nr_frags > ENETC_MAX_SKB_FRAGS))
@@ -252,6 +330,39 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	return NETDEV_TX_OK;
 }
 
+netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	u8 udp, msgtype, twostep;
+	u16 offset1, offset2;
+
+	/* Mark tx timestamp type on skb->cb[0] if requires */
+	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK)) {
+		skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
+	} else {
+		skb->cb[0] = 0;
+	}
+
+	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
+		/* For one-step PTP sync packet, queue it */
+		if (!enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
+				     &offset1, &offset2)) {
+			if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0) {
+				skb_queue_tail(&priv->tx_skbs, skb);
+				queue_work(priv->enetc_ptp_wq,
+					   &priv->tx_onestep_tstamp);
+				return NETDEV_TX_OK;
+			}
+		}
+
+		/* Fall back to two-step timestamp for other packets */
+		skb->cb[0] = ENETC_F_TX_TSTAMP;
+	}
+
+	return enetc_start_xmit(skb, ndev);
+}
+
 static irqreturn_t enetc_msix(int irq, void *data)
 {
 	struct enetc_int_vector	*v = data;
@@ -392,10 +503,11 @@ static void enetc_recycle_xdp_tx_buff(struct enetc_bdr *tx_ring,
 static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 {
 	struct net_device *ndev = tx_ring->ndev;
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int tx_frm_cnt = 0, tx_byte_cnt = 0;
 	struct enetc_tx_swbd *tx_swbd;
 	int i, bds_to_clean;
-	bool do_tstamp;
+	bool do_twostep_tstamp;
 	u64 tstamp = 0;
 
 	i = tx_ring->next_to_clean;
@@ -403,7 +515,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 
 	bds_to_clean = enetc_bd_ready_count(tx_ring, i);
 
-	do_tstamp = false;
+	do_twostep_tstamp = false;
 
 	while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
 		struct xdp_frame *xdp_frame = enetc_tx_swbd_get_xdp_frame(tx_swbd);
@@ -416,10 +528,10 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 			txbd = ENETC_TXBD(*tx_ring, i);
 
 			if (txbd->flags & ENETC_TXBD_FLAGS_W &&
-			    tx_swbd->do_tstamp) {
+			    tx_swbd->do_twostep_tstamp) {
 				enetc_get_tx_tstamp(&priv->si->hw, txbd,
 						    &tstamp);
-				do_tstamp = true;
+				do_twostep_tstamp = true;
 			}
 		}
 
@@ -432,9 +544,12 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
 			xdp_return_frame(xdp_frame);
 			tx_swbd->xdp_frame = NULL;
 		} else if (skb) {
-			if (unlikely(do_tstamp)) {
+			if (unlikely(tx_swbd->skb->cb[0] &
+				     ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
+				mutex_unlock(&priv->onestep_tstamp_lock);
+			} else if (unlikely(do_twostep_tstamp)) {
 				enetc_tstamp_tx(skb, tstamp);
-				do_tstamp = false;
+				do_twostep_tstamp = false;
 			}
 			napi_consume_skb(skb, napi_budget);
 			tx_swbd->skb = NULL;
@@ -1863,6 +1978,47 @@ static int enetc_phylink_connect(struct net_device *ndev)
 	return 0;
 }
 
+static void enetc_tx_onestep_tstamp(struct work_struct *work)
+{
+	struct enetc_ndev_priv *priv;
+	struct sk_buff *skb;
+
+	priv = container_of(work, struct enetc_ndev_priv, tx_onestep_tstamp);
+
+	while (true) {
+		skb = skb_dequeue(&priv->tx_skbs);
+		if (!skb)
+			return;
+
+		/* Lock before TX one-step timestamping packet, and release
+		 * when the packet has been sent on hardware, or transmit
+		 * failure.
+		 */
+		mutex_lock(&priv->onestep_tstamp_lock);
+
+		netif_tx_lock(priv->ndev);
+		enetc_start_xmit(skb, priv->ndev);
+		netif_tx_unlock(priv->ndev);
+	}
+}
+
+static int enetc_tx_onestep_tstamp_init(struct enetc_ndev_priv *priv)
+{
+	priv->enetc_ptp_wq = alloc_workqueue("enetc_ptp_wq", 0, 0);
+	if (!priv->enetc_ptp_wq)
+		return -ENOMEM;
+
+	INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
+	skb_queue_head_init(&priv->tx_skbs);
+
+	return 0;
+}
+
+static void enetc_tx_onestep_tstamp_deinit(struct enetc_ndev_priv *priv)
+{
+	destroy_workqueue(priv->enetc_ptp_wq);
+}
+
 void enetc_start(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
@@ -1907,6 +2063,10 @@ int enetc_open(struct net_device *ndev)
 	if (err)
 		goto err_alloc_rx;
 
+	err = enetc_tx_onestep_tstamp_init(priv);
+	if (err)
+		goto err_tx_onestep_tstamp;
+
 	err = netif_set_real_num_tx_queues(ndev, priv->num_tx_rings);
 	if (err)
 		goto err_set_queues;
@@ -1921,6 +2081,8 @@ int enetc_open(struct net_device *ndev)
 	return 0;
 
 err_set_queues:
+	enetc_tx_onestep_tstamp_deinit(priv);
+err_tx_onestep_tstamp:
 	enetc_free_rx_resources(priv);
 err_alloc_rx:
 	enetc_free_tx_resources(priv);
@@ -1967,6 +2129,7 @@ int enetc_close(struct net_device *ndev)
 	if (priv->phylink)
 		phylink_disconnect_phy(priv->phylink);
 	enetc_free_rxtx_rings(priv);
+	enetc_tx_onestep_tstamp_deinit(priv);
 	enetc_free_rx_resources(priv);
 	enetc_free_tx_resources(priv);
 	enetc_free_irqs(priv);
@@ -2213,11 +2376,16 @@ static int enetc_hwtstamp_set(struct net_device *ndev, struct ifreq *ifr)
 
 	switch (config.tx_type) {
 	case HWTSTAMP_TX_OFF:
-		priv->active_offloads &= ~ENETC_F_TX_TSTAMP;
+		priv->active_offloads &= ~ENETC_F_TX_TSTAMP_MASK;
 		break;
 	case HWTSTAMP_TX_ON:
+		priv->active_offloads &= ~ENETC_F_TX_TSTAMP_MASK;
 		priv->active_offloads |= ENETC_F_TX_TSTAMP;
 		break;
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		priv->active_offloads &= ~ENETC_F_TX_TSTAMP_MASK;
+		priv->active_offloads |= ENETC_F_TX_ONESTEP_SYNC_TSTAMP;
+		break;
 	default:
 		return -ERANGE;
 	}
@@ -2248,7 +2416,9 @@ static int enetc_hwtstamp_get(struct net_device *ndev, struct ifreq *ifr)
 
 	config.flags = 0;
 
-	if (priv->active_offloads & ENETC_F_TX_TSTAMP)
+	if (priv->active_offloads & ENETC_F_TX_ONESTEP_SYNC_TSTAMP)
+		config.tx_type = HWTSTAMP_TX_ONESTEP_SYNC;
+	else if (priv->active_offloads & ENETC_F_TX_TSTAMP)
 		config.tx_type = HWTSTAMP_TX_ON;
 	else
 		config.tx_type = HWTSTAMP_TX_OFF;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 96889529383e..1b90461f288c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -30,7 +30,7 @@ struct enetc_tx_swbd {
 	enum dma_data_direction dir;
 	u8 is_dma_page:1;
 	u8 check_wb:1;
-	u8 do_tstamp:1;
+	u8 do_twostep_tstamp:1;
 	u8 is_eof:1;
 	u8 is_xdp_tx:1;
 	u8 is_xdp_redirect:1;
@@ -275,11 +275,12 @@ struct psfp_cap {
 /* TODO: more hardware offloads */
 enum enetc_active_offloads {
 	/* 8 bits reserved for TX timestamp types (hwtstamp_tx_types) */
-	ENETC_F_TX_TSTAMP	= BIT(0),
+	ENETC_F_TX_TSTAMP		= BIT(0),
+	ENETC_F_TX_ONESTEP_SYNC_TSTAMP	= BIT(1),
 
-	ENETC_F_RX_TSTAMP	= BIT(8),
-	ENETC_F_QBV		= BIT(9),
-	ENETC_F_QCI		= BIT(10),
+	ENETC_F_RX_TSTAMP		= BIT(8),
+	ENETC_F_QBV			= BIT(9),
+	ENETC_F_QCI			= BIT(10),
 };
 
 /* interrupt coalescing modes */
@@ -324,6 +325,16 @@ struct enetc_ndev_priv {
 	u32 tx_ictt;
 
 	struct bpf_prog *xdp_prog;
+
+	/* The single-step register has to be configured dynamically per
+	 * packet. So, before transmit current packet, use a mutex lock
+	 * to make sure the lock is released by last one-step timestamping
+	 * packet completing transmitting on hardware.
+	 */
+	struct mutex		onestep_tstamp_lock;
+	struct workqueue_struct	*enetc_ptp_wq;
+	struct work_struct	tx_onestep_tstamp;
+	struct sk_buff_head	tx_skbs;
 };
 
 /* Messaging */
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 7cc81b453bd7..49835e878bbb 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -671,7 +671,8 @@ static int enetc_get_ts_info(struct net_device *ndev,
 				SOF_TIMESTAMPING_RAW_HARDWARE;
 
 	info->tx_types = (1 << HWTSTAMP_TX_OFF) |
-			 (1 << HWTSTAMP_TX_ON);
+			 (1 << HWTSTAMP_TX_ON) |
+			 (1 << HWTSTAMP_TX_ONESTEP_SYNC);
 	info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
 			   (1 << HWTSTAMP_FILTER_ALL);
 #else
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 00938f7960a4..04ac7fc23ead 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -239,6 +239,12 @@ enum enetc_bdr_type {TX, RX};
 
 #define ENETC_PM_IMDIO_BASE	0x8030
 
+#define ENETC_PM0_SINGLE_STEP		0x80c0
+#define ENETC_PM1_SINGLE_STEP		0x90c0
+#define ENETC_PM0_SINGLE_STEP_CH	BIT(7)
+#define ENETC_PM0_SINGLE_STEP_EN	BIT(31)
+#define ENETC_SET_SINGLE_STEP_OFFSET(v)	(((v) & 0xff) << 8)
+
 #define ENETC_PM0_IF_MODE	0x8300
 #define ENETC_PM0_IFM_RG	BIT(2)
 #define ENETC_PM0_IFM_RLP	(BIT(5) | BIT(11))
@@ -548,6 +554,7 @@ static inline void enetc_clear_tx_bd(union enetc_tx_bd *txbd)
 
 /* Extension flags */
 #define ENETC_TXBD_E_FLAGS_VLAN_INS	BIT(0)
+#define ENETC_TXBD_E_FLAGS_ONE_STEP_PTP	BIT(1)
 #define ENETC_TXBD_E_FLAGS_TWO_STEP_PTP	BIT(2)
 
 union enetc_rx_bd {
-- 
2.25.1

Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Jakub Kicinski]

On Thu,  8 Apr 2021 19:13:50 +0800 Yangbo Lu wrote:
> This patch is to add support for PTP Sync packet one-step timestamping.
> Since ENETC single-step register has to be configured dynamically per
> packet for correctionField offeset and UDP checksum update, current
> one-step timestamping packet has to be sent only when the last one
> completes transmitting on hardware. So, on the TX below things are done
> by the patch:
> 
> - For one-step timestamping packet, queue to skb queue.
> - Start a work to transmit skbs in queue.
> - For other skbs, transmit immediately.
> - mutex lock used to ensure the last one-step timestamping packet has
>   already been transmitted on hardware before transmitting current one.
> 
> And the configuration for one-step timestamping on ENETC before
> transmitting is,
> 
> - Set one-step timestamping flag in extension BD.
> - Write 30 bits current timestamp in tstamp field of extension BD.
> - Update PTP Sync packet originTimestamp field with current timestamp.
> - Configure single-step register for correctionField offeset and UDP
>   checksum update.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

> @@ -432,9 +544,12 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
>  			xdp_return_frame(xdp_frame);
>  			tx_swbd->xdp_frame = NULL;
>  		} else if (skb) {
> -			if (unlikely(do_tstamp)) {
> +			if (unlikely(tx_swbd->skb->cb[0] &
> +				     ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
> +				mutex_unlock(&priv->onestep_tstamp_lock);
> +			} else if (unlikely(do_twostep_tstamp)) {
>  				enetc_tstamp_tx(skb, tstamp);
> -				do_tstamp = false;
> +				do_twostep_tstamp = false;
>  			}
>  			napi_consume_skb(skb, napi_budget);
>  			tx_swbd->skb = NULL;
> @@ -1863,6 +1978,47 @@ static int enetc_phylink_connect(struct net_device *ndev)
>  	return 0;
>  }
>  
> +static void enetc_tx_onestep_tstamp(struct work_struct *work)
> +{
> +	struct enetc_ndev_priv *priv;
> +	struct sk_buff *skb;
> +
> +	priv = container_of(work, struct enetc_ndev_priv, tx_onestep_tstamp);
> +
> +	while (true) {
> +		skb = skb_dequeue(&priv->tx_skbs);
> +		if (!skb)
> +			return;
> +
> +		/* Lock before TX one-step timestamping packet, and release
> +		 * when the packet has been sent on hardware, or transmit
> +		 * failure.
> +		 */
> +		mutex_lock(&priv->onestep_tstamp_lock);

Using a lock to wake up a producer is not a great idea. It usually
breaks advanced features like priority inheritance. Probably doesn't
matter for a struct mutex, but I think it may still make lockdep
complain.

Why not make it work with a flag?

start_xmit:

	if (skb->cb[0] & ONESTEP) {
		if (priv->flags & ONESTEP_BUSY) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return ...;
		}
		priv->flags |= ONESTEP_BUSY;
	}

clean_tx:

	/* don't clear ONESTEP_BUSY, we need the tx lock */
	if (skb->cb[0] & ONESTEP)
		queue_work(...);

work:

	netif_tx_lock()
	skb = skb_dequeue();
	if (skb)
		start_xmit(skb)
	else
		priv->flags &= ~ONESTEP_BUSY;
	netif_tx_unlock()

> +		netif_tx_lock(priv->ndev);
> +		enetc_start_xmit(skb, priv->ndev);
> +		netif_tx_unlock(priv->ndev);
> +	}
> +}
> +
> +static int enetc_tx_onestep_tstamp_init(struct enetc_ndev_priv *priv)
> +{
> +	priv->enetc_ptp_wq = alloc_workqueue("enetc_ptp_wq", 0, 0);
> +	if (!priv->enetc_ptp_wq)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
> +	skb_queue_head_init(&priv->tx_skbs);
> +
> +	return 0;
> +}
> +
> +static void enetc_tx_onestep_tstamp_deinit(struct enetc_ndev_priv *priv)
> +{
> +	destroy_workqueue(priv->enetc_ptp_wq);
> +}

Why allocate a separate workqueue for one work? You can queue your
work on the system workqueue.

Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Jakub Kicinski]

On Thu, 8 Apr 2021 09:02:50 -0700 Jakub Kicinski wrote:
> 		if (priv->flags & ONESTEP_BUSY) {
> 			skb_queue_tail(&priv->tx_skbs, skb);
> 			return ...;
> 		}
> 		priv->flags |= ONESTEP_BUSY;

Ah, if you have multiple queues this needs to be under a separate
spinlock, 'cause netif_tx_lock() won't be enough.

RE: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Claudiu Manoil]

>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Thursday, April 8, 2021 7:07 PM
>To: Y.b. Lu <yangbo.lu@nxp.com>
>Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
>Richard Cochran <richardcochran@gmail.com>; Claudiu Manoil
><claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>;
>Russell King <linux@armlinux.org.uk>
>Subject: Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step
>timestamping
>
>On Thu, 8 Apr 2021 09:02:50 -0700 Jakub Kicinski wrote:
>> 		if (priv->flags & ONESTEP_BUSY) {
>> 			skb_queue_tail(&priv->tx_skbs, skb);
>> 			return ...;
>> 		}
>> 		priv->flags |= ONESTEP_BUSY;
>
>Ah, if you have multiple queues this needs to be under a separate
>spinlock, 'cause netif_tx_lock() won't be enough.

Hi Yangbo,

Please try test_and_set_bit_lock()/ clear_bit_unlock() based on Jakub's
suggestion, and see if it works for you / whether it can replace the mutex.

Thanks,
Claudiu

Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Jakub Kicinski]

On Fri, 9 Apr 2021 06:37:53 +0000 Claudiu Manoil wrote:
> >On Thu, 8 Apr 2021 09:02:50 -0700 Jakub Kicinski wrote:  
> >> 		if (priv->flags & ONESTEP_BUSY) {
> >> 			skb_queue_tail(&priv->tx_skbs, skb);
> >> 			return ...;
> >> 		}
> >> 		priv->flags |= ONESTEP_BUSY;  
> >
> >Ah, if you have multiple queues this needs to be under a separate
> >spinlock, 'cause netif_tx_lock() won't be enough.  
> 
> Please try test_and_set_bit_lock()/ clear_bit_unlock() based on Jakub's
> suggestion, and see if it works for you / whether it can replace the mutex.

I was thinking that with multiple queues just a bit won't be sufficient
because:

xmit:				work:
test_bit... // already set
				dequeue // empty
enqueue
				clear_bit()

That frame will never get sent, no?

Note that skb_queue already has a lock so you'd just need to make that
lock protect the flag/bit as well, overall the number of locks remains
the same. Take the queue's lock, check the flag, use
__skb_queue_tail(), release etc.

Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Claudiu Manoil]

On 09.04.2021 19:09, Jakub Kicinski wrote:
> On Fri, 9 Apr 2021 06:37:53 +0000 Claudiu Manoil wrote:
>>> On Thu, 8 Apr 2021 09:02:50 -0700 Jakub Kicinski wrote:
>>>> 		if (priv->flags & ONESTEP_BUSY) {
>>>> 			skb_queue_tail(&priv->tx_skbs, skb);
>>>> 			return ...;
>>>> 		}
>>>> 		priv->flags |= ONESTEP_BUSY;
>>>
>>> Ah, if you have multiple queues this needs to be under a separate
>>> spinlock, 'cause netif_tx_lock() won't be enough.
>>
>> Please try test_and_set_bit_lock()/ clear_bit_unlock() based on Jakub's
>> suggestion, and see if it works for you / whether it can replace the mutex.
> 
> I was thinking that with multiple queues just a bit won't be sufficient
> because:
> 
> xmit:				work:
> test_bit... // already set
> 				dequeue // empty
> enqueue
> 				clear_bit()
> 
> That frame will never get sent, no?

I don't see any issue with Yangbo's initial design actually, I was just
suggesting him to replace the mutex with a bit lock, based on your comments.
That means:
xmit:		work:				clean_tx_ring: //Tx conf
skb_queue_tail()		
		skb_dequeue()
		test_and_set_bit_lock()
						clear_bit_unlock()

The skb queue is one per device, as it needs to serialize ptp skbs
for that device (due to the restriction that a ptp packet cannot be 
enqueued for transmission if there's another ptp packet waiting
for transmission in a h/w descriptor ring).

If multiple ptp skbs are coming in from different xmit queues at the 
same time (same device), they are enqueued in the common priv->tx_skbs 
queue (skb_queue_tail() is protected by locks), and the worker thread is 
started.
The worker dequeues the first ptp skb, and places the packet in the h/w 
descriptor ring for transmission. Then dequeues the second skb and waits 
at the lock (or mutex or whatever lock is preferred).
Upon transmission of the ptp packet the lock is released by the Tx 
confirmation napi thread (clean_tx_ring()) and the next PTP skb can be 
placed in the corresponding descriptor ring for transmission by the 
worker thread.

So the way I understood your comments is that you'd rather use a spin 
lock in the worker thread instead of a mutex.

> 
> Note that skb_queue already has a lock so you'd just need to make that
> lock protect the flag/bit as well, overall the number of locks remains
> the same. Take the queue's lock, check the flag, use
> __skb_queue_tail(), release etc.
> 

This is a good optimization idea indeed, to use the priv->tx_skb skb 
list's spin lock, instead of adding another lock.

Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Jakub Kicinski]

On Fri, 9 Apr 2021 22:32:49 +0300 Claudiu Manoil wrote:
> On 09.04.2021 19:09, Jakub Kicinski wrote:
> > On Fri, 9 Apr 2021 06:37:53 +0000 Claudiu Manoil wrote:  
> >> Please try test_and_set_bit_lock()/ clear_bit_unlock() based on Jakub's
> >> suggestion, and see if it works for you / whether it can replace the mutex.  
> > 
> > I was thinking that with multiple queues just a bit won't be sufficient
> > because:
> > 
> > xmit:				work:
> > test_bit... // already set
> > 				dequeue // empty
> > enqueue
> > 				clear_bit()
> > 
> > That frame will never get sent, no?  
> 
> I don't see any issue with Yangbo's initial design actually, I was just
> suggesting him to replace the mutex with a bit lock, based on your comments.
> That means:
> xmit:		work:				clean_tx_ring: //Tx conf
> skb_queue_tail()		
> 		skb_dequeue()
> 		test_and_set_bit_lock()
> 						clear_bit_unlock()
> 
> The skb queue is one per device, as it needs to serialize ptp skbs
> for that device (due to the restriction that a ptp packet cannot be 
> enqueued for transmission if there's another ptp packet waiting
> for transmission in a h/w descriptor ring).
> 
> If multiple ptp skbs are coming in from different xmit queues at the 
> same time (same device), they are enqueued in the common priv->tx_skbs 
> queue (skb_queue_tail() is protected by locks), and the worker thread is 
> started.
> The worker dequeues the first ptp skb, and places the packet in the h/w 
> descriptor ring for transmission. Then dequeues the second skb and waits 
> at the lock (or mutex or whatever lock is preferred).
> Upon transmission of the ptp packet the lock is released by the Tx 
> confirmation napi thread (clean_tx_ring()) and the next PTP skb can be 
> placed in the corresponding descriptor ring for transmission by the 
> worker thread.

I see. I thought you were commenting on my scheme. Yes, if only the
worker is allowed to send there is no race, that should work well.

In my suggestion I was trying to allow the first frame to be sent
directly without going via the queue and requiring the worker to be
scheduled in.

> So the way I understood your comments is that you'd rather use a spin 
> lock in the worker thread instead of a mutex.

Not exactly, my main objection was that the mutex was used for wake up.
Worker locks it, completion path unlocks it.

Your suggestion of using a bit works well. Just Instead of a loop the
worker needs to send a single skb, and completion needs to schedule it
again.

> > Note that skb_queue already has a lock so you'd just need to make that
> > lock protect the flag/bit as well, overall the number of locks remains
> > the same. Take the queue's lock, check the flag, use
> > __skb_queue_tail(), release etc.
> 
> This is a good optimization idea indeed, to use the priv->tx_skb skb 
> list's spin lock, instead of adding another lock.

RE: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

[Y.b. Lu]

Hi Jakub and Claudiu,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2021年4月10日 3:44
> To: Claudiu Manoil <claudiu.manoil@gmail.com>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Y.b. Lu <yangbo.lu@nxp.com>;
> netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Richard
> Cochran <richardcochran@gmail.com>; Vladimir Oltean
> <vladimir.oltean@nxp.com>; Russell King <linux@armlinux.org.uk>
> Subject: Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step
> timestamping
> 
> On Fri, 9 Apr 2021 22:32:49 +0300 Claudiu Manoil wrote:
> > On 09.04.2021 19:09, Jakub Kicinski wrote:
> > > On Fri, 9 Apr 2021 06:37:53 +0000 Claudiu Manoil wrote:
> > >> Please try test_and_set_bit_lock()/ clear_bit_unlock() based on
> > >> Jakub's suggestion, and see if it works for you / whether it can replace the
> mutex.
> > >
> > > I was thinking that with multiple queues just a bit won't be
> > > sufficient
> > > because:
> > >
> > > xmit:				work:
> > > test_bit... // already set
> > > 				dequeue // empty
> > > enqueue
> > > 				clear_bit()
> > >
> > > That frame will never get sent, no?
> >
> > I don't see any issue with Yangbo's initial design actually, I was
> > just suggesting him to replace the mutex with a bit lock, based on your
> comments.
> > That means:
> > xmit:		work:				clean_tx_ring: //Tx conf
> > skb_queue_tail()
> > 		skb_dequeue()
> > 		test_and_set_bit_lock()
> > 						clear_bit_unlock()
> >
> > The skb queue is one per device, as it needs to serialize ptp skbs for
> > that device (due to the restriction that a ptp packet cannot be
> > enqueued for transmission if there's another ptp packet waiting for
> > transmission in a h/w descriptor ring).
> >
> > If multiple ptp skbs are coming in from different xmit queues at the
> > same time (same device), they are enqueued in the common priv->tx_skbs
> > queue (skb_queue_tail() is protected by locks), and the worker thread
> > is started.
> > The worker dequeues the first ptp skb, and places the packet in the
> > h/w descriptor ring for transmission. Then dequeues the second skb and
> > waits at the lock (or mutex or whatever lock is preferred).
> > Upon transmission of the ptp packet the lock is released by the Tx
> > confirmation napi thread (clean_tx_ring()) and the next PTP skb can be
> > placed in the corresponding descriptor ring for transmission by the
> > worker thread.
> 
> I see. I thought you were commenting on my scheme. Yes, if only the worker is
> allowed to send there is no race, that should work well.
> 
> In my suggestion I was trying to allow the first frame to be sent directly
> without going via the queue and requiring the worker to be scheduled in.
> 
> > So the way I understood your comments is that you'd rather use a spin
> > lock in the worker thread instead of a mutex.
> 
> Not exactly, my main objection was that the mutex was used for wake up.
> Worker locks it, completion path unlocks it.
> 
> Your suggestion of using a bit works well. Just Instead of a loop the worker
> needs to send a single skb, and completion needs to schedule it again.

Thank you so much for your suggestion and guide. So I used bit lock, and also followed Jakub's suggestion.
The difference was, I moved the flag cleaning before transmitting single skb from skb queue.
Otherwise, skb in queue would never be transmitted in start_xmit().
Please help to review v3 I sent:)

work:

	netif_tx_lock()
    clear_bit_unlock(); // put cleaning here
	skb = skb_dequeue();
	if (skb)
		start_xmit(skb)
//	else
//		priv->flags &= ~ONESTEP_BUSY;
	netif_tx_unlock()

Thanks.

> 
> > > Note that skb_queue already has a lock so you'd just need to make
> > > that lock protect the flag/bit as well, overall the number of locks
> > > remains the same. Take the queue's lock, check the flag, use
> > > __skb_queue_tail(), release etc.
> >
> > This is a good optimization idea indeed, to use the priv->tx_skb skb
> > list's spin lock, instead of adding another lock.