linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO
@ 2024-03-12 10:02 Chintan Vankar
  2024-03-12 10:02 ` [PATCH v2 2/3] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets Chintan Vankar
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Chintan Vankar @ 2024-03-12 10:02 UTC (permalink / raw)
  To: Dan Carpenter, Siddharth Vadapalli, Heiner Kallweit,
	Chintan Vankar, Vladimir Oltean, Grygorii Strashko, Andrew Lunn,
	Roger Quadros, Richard Cochran, Paolo Abeni, Jakub Kicinski,
	Eric Dumazet, David S. Miller
  Cc: linux-kernel, netdev

CPTS module supports capturing timestamp for every packet it receives,
add a new function named "am65_cpts_rx_find_ts()" to get the timestamp
of received packets from CPTS FIFO.

Add another function named "am65_cpts_rx_timestamp()" which internally
calls "am65_cpts_rx_find_ts()" function and timestamps the received
PTP packets.

Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---

Changes from v1 to v2:
- Add skb_reset_mac_header() in am65_cpts_rx_timestamp() to configure
  skb MAC header properties for packets which are to be timestamped.

 drivers/net/ethernet/ti/am65-cpts.c | 89 +++++++++++++++++++++--------
 drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
 2 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index c66618d91c28..6c1d571c5e0b 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -859,29 +859,6 @@ static long am65_cpts_ts_work(struct ptp_clock_info *ptp)
 	return delay;
 }
 
-/**
- * am65_cpts_rx_enable - enable rx timestamping
- * @cpts: cpts handle
- * @en: enable
- *
- * This functions enables rx packets timestamping. The CPTS can timestamp all
- * rx packets.
- */
-void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en)
-{
-	u32 val;
-
-	mutex_lock(&cpts->ptp_clk_lock);
-	val = am65_cpts_read32(cpts, control);
-	if (en)
-		val |= AM65_CPTS_CONTROL_TSTAMP_EN;
-	else
-		val &= ~AM65_CPTS_CONTROL_TSTAMP_EN;
-	am65_cpts_write32(cpts, val, control);
-	mutex_unlock(&cpts->ptp_clk_lock);
-}
-EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);
-
 static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
 {
 	unsigned int ptp_class = ptp_classify_raw(skb);
@@ -906,6 +883,72 @@ static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
 	return 1;
 }
 
+static u64 am65_cpts_find_rx_ts(struct am65_cpts *cpts, struct sk_buff *skb,
+				int ev_type, u32 skb_mtype_seqid)
+{
+	struct list_head *this, *next;
+	struct am65_cpts_event *event;
+	unsigned long flags;
+	u32 mtype_seqid;
+	u64 ns = 0;
+
+	am65_cpts_fifo_read(cpts);
+	spin_lock_irqsave(&cpts->lock, flags);
+	list_for_each_safe(this, next, &cpts->events) {
+		event = list_entry(this, struct am65_cpts_event, list);
+		if (time_after(jiffies, event->tmo)) {
+			list_del_init(&event->list);
+			list_add(&event->list, &cpts->pool);
+			continue;
+		}
+
+		mtype_seqid = event->event1 &
+			      (AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK |
+			       AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK |
+			       AM65_CPTS_EVENT_1_EVENT_TYPE_MASK);
+
+		if (mtype_seqid == skb_mtype_seqid) {
+			ns = event->timestamp;
+			list_del_init(&event->list);
+			list_add(&event->list, &cpts->pool);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&cpts->lock, flags);
+
+	return ns;
+}
+
+void am65_cpts_rx_timestamp(struct am65_cpts *cpts, struct sk_buff *skb)
+{
+	struct am65_cpts_skb_cb_data *skb_cb = (struct am65_cpts_skb_cb_data *)skb->cb;
+	struct skb_shared_hwtstamps *ssh;
+	int ret;
+	u64 ns;
+
+	/* am65_cpts_rx_timestamp() is called before eth_type_trans(), so
+	 * skb MAC Hdr properties are not configured yet. Hence need to
+	 * reset skb MAC header here
+	 */
+	skb_reset_mac_header(skb);
+	ret = am65_skb_get_mtype_seqid(skb, &skb_cb->skb_mtype_seqid);
+	if (!ret)
+		return; /* if not PTP class packet */
+
+	skb_cb->skb_mtype_seqid |= (AM65_CPTS_EV_RX << AM65_CPTS_EVENT_1_EVENT_TYPE_SHIFT);
+
+	dev_dbg(cpts->dev, "%s mtype seqid %08x\n", __func__, skb_cb->skb_mtype_seqid);
+
+	ns = am65_cpts_find_rx_ts(cpts, skb, AM65_CPTS_EV_RX, skb_cb->skb_mtype_seqid);
+	if (!ns)
+		return;
+
+	ssh = skb_hwtstamps(skb);
+	memset(ssh, 0, sizeof(*ssh));
+	ssh->hwtstamp = ns_to_ktime(ns);
+}
+EXPORT_SYMBOL_GPL(am65_cpts_rx_timestamp);
+
 /**
  * am65_cpts_tx_timestamp - save tx packet for timestamping
  * @cpts: cpts handle
diff --git a/drivers/net/ethernet/ti/am65-cpts.h b/drivers/net/ethernet/ti/am65-cpts.h
index 6e14df0be113..6099d772799d 100644
--- a/drivers/net/ethernet/ti/am65-cpts.h
+++ b/drivers/net/ethernet/ti/am65-cpts.h
@@ -22,9 +22,9 @@ void am65_cpts_release(struct am65_cpts *cpts);
 struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
 				   struct device_node *node);
 int am65_cpts_phc_index(struct am65_cpts *cpts);
+void am65_cpts_rx_timestamp(struct am65_cpts *cpts, struct sk_buff *skb);
 void am65_cpts_tx_timestamp(struct am65_cpts *cpts, struct sk_buff *skb);
 void am65_cpts_prep_tx_timestamp(struct am65_cpts *cpts, struct sk_buff *skb);
-void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en);
 u64 am65_cpts_ns_gettime(struct am65_cpts *cpts);
 int am65_cpts_estf_enable(struct am65_cpts *cpts, int idx,
 			  struct am65_cpts_estf_cfg *cfg);
@@ -48,17 +48,18 @@ static inline int am65_cpts_phc_index(struct am65_cpts *cpts)
 	return -1;
 }
 
-static inline void am65_cpts_tx_timestamp(struct am65_cpts *cpts,
+static inline void am65_cpts_rx_timestamp(struct am65_cpts *cpts,
 					  struct sk_buff *skb)
 {
 }
 
-static inline void am65_cpts_prep_tx_timestamp(struct am65_cpts *cpts,
-					       struct sk_buff *skb)
+static inline void am65_cpts_tx_timestamp(struct am65_cpts *cpts,
+					  struct sk_buff *skb)
 {
 }
 
-static inline void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en)
+static inline void am65_cpts_prep_tx_timestamp(struct am65_cpts *cpts,
+					       struct sk_buff *skb)
 {
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/3] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets
  2024-03-12 10:02 [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Chintan Vankar
@ 2024-03-12 10:02 ` Chintan Vankar
  2024-03-12 10:02 ` [PATCH v2 3/3] net: ethernet: ti: am65-cpsw-ethtool: Update rx_filters for device's timestamping Chintan Vankar
  2024-03-12 10:50 ` [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Paolo Abeni
  2 siblings, 0 replies; 5+ messages in thread
From: Chintan Vankar @ 2024-03-12 10:02 UTC (permalink / raw)
  To: Dan Carpenter, Siddharth Vadapalli, Heiner Kallweit,
	Chintan Vankar, Vladimir Oltean, Grygorii Strashko, Andrew Lunn,
	Roger Quadros, Richard Cochran, Paolo Abeni, Jakub Kicinski,
	Eric Dumazet, David S. Miller
  Cc: linux-kernel, netdev

The current mechanism of timestamping, am65-cpsw-nuss driver
timestamps all received packets by setting the TSTAMP_EN bit
in CPTS_CONTROL register, which directs the CPTS module to
timestamp all received packets, followed by passing timestamp
via DMA descriptors. This mechanism is responsible for triggering
errata i2401: "CPSW: Host Timestamps Cause CPSW Port to Lock up"

To prevent port lock up, disable TSTAMP_EN bit in CPTS_CONTROL
register. The workaround for timestamping received packets is to
utilize the CPTS Event FIFO that records timestamps corresponding to
certain events, with one such event being the reception of an
Ethernet packet with the EtherType field set to Precision Time
Protocol (PTP).

Fixes: b1f66a5bee07 ("net: ethernet: ti: am65-cpsw-nuss: enable packet timestamping support")
Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---

Changes from v1 to v2:
- Removed skb_reset_mac_header() from am65_cpsw_nuss_rx_packets()
  to call it for the packets which are to be timestamped.

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 50 +++++++++++-------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 2939a21ca74f..7809bb814934 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -101,6 +101,12 @@
 #define AM65_CPSW_PN_TS_CTL_TX_HOST_TS_EN	BIT(11)
 #define AM65_CPSW_PN_TS_CTL_MSG_TYPE_EN_SHIFT	16
 
+#define AM65_CPSW_PN_TS_CTL_RX_ANX_F_EN		BIT(0)
+#define AM65_CPSW_PN_TS_CTL_RX_VLAN_LT1_EN	BIT(1)
+#define AM65_CPSW_PN_TS_CTL_RX_VLAN_LT2_EN	BIT(2)
+#define AM65_CPSW_PN_TS_CTL_RX_ANX_D_EN		BIT(3)
+#define AM65_CPSW_PN_TS_CTL_RX_ANX_E_EN		BIT(9)
+
 /* AM65_CPSW_PORTN_REG_TS_SEQ_LTYPE_REG register fields */
 #define AM65_CPSW_PN_TS_SEQ_ID_OFFSET_SHIFT	16
 
@@ -124,6 +130,11 @@
 	 AM65_CPSW_PN_TS_CTL_TX_ANX_E_EN |	\
 	 AM65_CPSW_PN_TS_CTL_TX_ANX_F_EN)
 
+#define AM65_CPSW_TS_RX_ANX_ALL_EN		\
+	(AM65_CPSW_PN_TS_CTL_RX_ANX_D_EN |	\
+	 AM65_CPSW_PN_TS_CTL_RX_ANX_E_EN |	\
+	 AM65_CPSW_PN_TS_CTL_RX_ANX_F_EN)
+
 #define AM65_CPSW_ALE_AGEOUT_DEFAULT	30
 /* Number of TX/RX descriptors */
 #define AM65_CPSW_MAX_TX_DESC	500
@@ -749,18 +760,6 @@ static int am65_cpsw_nuss_ndo_slave_open(struct net_device *ndev)
 	return ret;
 }
 
-static void am65_cpsw_nuss_rx_ts(struct sk_buff *skb, u32 *psdata)
-{
-	struct skb_shared_hwtstamps *ssh;
-	u64 ns;
-
-	ns = ((u64)psdata[1] << 32) | psdata[0];
-
-	ssh = skb_hwtstamps(skb);
-	memset(ssh, 0, sizeof(*ssh));
-	ssh->hwtstamp = ns_to_ktime(ns);
-}
-
 /* RX psdata[2] word format - checksum information */
 #define AM65_CPSW_RX_PSD_CSUM_ADD	GENMASK(15, 0)
 #define AM65_CPSW_RX_PSD_CSUM_ERR	BIT(16)
@@ -841,9 +840,6 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common *common,
 	skb->dev = ndev;
 
 	psdata = cppi5_hdesc_get_psdata(desc_rx);
-	/* add RX timestamp */
-	if (port->rx_ts_enabled)
-		am65_cpsw_nuss_rx_ts(skb, psdata);
 	csum_info = psdata[2];
 	dev_dbg(dev, "%s rx csum_info:%#x\n", __func__, csum_info);
 
@@ -856,6 +852,8 @@ static int am65_cpsw_nuss_rx_packets(struct am65_cpsw_common *common,
 		ndev_priv = netdev_priv(ndev);
 		am65_cpsw_nuss_set_offload_fwd_mark(skb, ndev_priv->offload_fwd_mark);
 		skb_put(skb, pkt_len);
+		if (port->rx_ts_enabled)
+			am65_cpts_rx_timestamp(common->cpts, skb);
 		skb->protocol = eth_type_trans(skb, ndev);
 		am65_cpsw_nuss_rx_csum(skb, csum_info);
 		napi_gro_receive(&common->napi_rx, skb);
@@ -1334,7 +1332,6 @@ static int am65_cpsw_nuss_ndo_slave_set_mac_address(struct net_device *ndev,
 static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
 				       struct ifreq *ifr)
 {
-	struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
 	struct am65_cpsw_port *port = am65_ndev_to_port(ndev);
 	u32 ts_ctrl, seq_id, ts_ctrl_ltype2, ts_vlan_ltype;
 	struct hwtstamp_config cfg;
@@ -1358,11 +1355,6 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
 	case HWTSTAMP_FILTER_NONE:
 		port->rx_ts_enabled = false;
 		break;
-	case HWTSTAMP_FILTER_ALL:
-	case HWTSTAMP_FILTER_SOME:
-	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
-	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
@@ -1372,10 +1364,13 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-	case HWTSTAMP_FILTER_NTP_ALL:
 		port->rx_ts_enabled = true;
-		cfg.rx_filter = HWTSTAMP_FILTER_ALL;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
 		break;
+	case HWTSTAMP_FILTER_ALL:
+	case HWTSTAMP_FILTER_SOME:
+	case HWTSTAMP_FILTER_NTP_ALL:
+		return -EOPNOTSUPP;
 	default:
 		return -ERANGE;
 	}
@@ -1405,6 +1400,10 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
 		ts_ctrl |= AM65_CPSW_TS_TX_ANX_ALL_EN |
 			   AM65_CPSW_PN_TS_CTL_TX_VLAN_LT1_EN;
 
+	if (port->rx_ts_enabled)
+		ts_ctrl |= AM65_CPSW_TS_RX_ANX_ALL_EN |
+			   AM65_CPSW_PN_TS_CTL_RX_VLAN_LT1_EN;
+
 	writel(seq_id, port->port_base + AM65_CPSW_PORTN_REG_TS_SEQ_LTYPE_REG);
 	writel(ts_vlan_ltype, port->port_base +
 	       AM65_CPSW_PORTN_REG_TS_VLAN_LTYPE_REG);
@@ -1412,9 +1411,6 @@ static int am65_cpsw_nuss_hwtstamp_set(struct net_device *ndev,
 	       AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2);
 	writel(ts_ctrl, port->port_base + AM65_CPSW_PORTN_REG_TS_CTL);
 
-	/* en/dis RX timestamp */
-	am65_cpts_rx_enable(common->cpts, port->rx_ts_enabled);
-
 	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
 }
 
@@ -1431,7 +1427,7 @@ static int am65_cpsw_nuss_hwtstamp_get(struct net_device *ndev,
 	cfg.tx_type = port->tx_ts_enabled ?
 		      HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
 	cfg.rx_filter = port->rx_ts_enabled ?
-			HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE;
+			HWTSTAMP_FILTER_PTP_V2_EVENT : HWTSTAMP_FILTER_NONE;
 
 	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 3/3] net: ethernet: ti: am65-cpsw-ethtool: Update rx_filters for device's timestamping
  2024-03-12 10:02 [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Chintan Vankar
  2024-03-12 10:02 ` [PATCH v2 2/3] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets Chintan Vankar
@ 2024-03-12 10:02 ` Chintan Vankar
  2024-03-12 10:50 ` [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Paolo Abeni
  2 siblings, 0 replies; 5+ messages in thread
From: Chintan Vankar @ 2024-03-12 10:02 UTC (permalink / raw)
  To: Dan Carpenter, Siddharth Vadapalli, Heiner Kallweit,
	Chintan Vankar, Vladimir Oltean, Grygorii Strashko, Andrew Lunn,
	Roger Quadros, Richard Cochran, Paolo Abeni, Jakub Kicinski,
	Eric Dumazet, David S. Miller
  Cc: linux-kernel, netdev

Update supported hwtstamp_rx_filters values for device's
timestamping.

Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---

Changes from v1 to v2:
- This patch is newly added in v2.

 drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
index d6ce2c9f0a8d..a1d0935d1ebe 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
@@ -695,6 +695,17 @@ static int am65_cpsw_get_ethtool_ts_info(struct net_device *ndev,
 					 struct ethtool_ts_info *info)
 {
 	struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
+	unsigned int ptp_v2_filter;
+
+	ptp_v2_filter = BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_L4_SYNC)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
+			BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_L2_SYNC)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
+			BIT(HWTSTAMP_FILTER_PTP_V2_EVENT)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_SYNC)	 |
+			BIT(HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
 
 	if (!IS_ENABLED(CONFIG_TI_K3_AM65_CPTS))
 		return ethtool_op_get_ts_info(ndev, info);
@@ -708,7 +719,7 @@ static int am65_cpsw_get_ethtool_ts_info(struct net_device *ndev,
 		SOF_TIMESTAMPING_RAW_HARDWARE;
 	info->phc_index = am65_cpts_phc_index(common->cpts);
 	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
-	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | BIT(HWTSTAMP_FILTER_ALL);
+	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | ptp_v2_filter;
 	return 0;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO
  2024-03-12 10:02 [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Chintan Vankar
  2024-03-12 10:02 ` [PATCH v2 2/3] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets Chintan Vankar
  2024-03-12 10:02 ` [PATCH v2 3/3] net: ethernet: ti: am65-cpsw-ethtool: Update rx_filters for device's timestamping Chintan Vankar
@ 2024-03-12 10:50 ` Paolo Abeni
  2024-03-18 11:12   ` Chintan Vankar
  2 siblings, 1 reply; 5+ messages in thread
From: Paolo Abeni @ 2024-03-12 10:50 UTC (permalink / raw)
  To: Chintan Vankar, Dan Carpenter, Siddharth Vadapalli,
	Heiner Kallweit, Vladimir Oltean, Grygorii Strashko, Andrew Lunn,
	Roger Quadros, Richard Cochran, Jakub Kicinski, Eric Dumazet,
	David S. Miller
  Cc: linux-kernel, netdev

On Tue, 2024-03-12 at 15:32 +0530, Chintan Vankar wrote:
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c66618d91c28..6c1d571c5e0b 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -859,29 +859,6 @@ static long am65_cpts_ts_work(struct ptp_clock_info *ptp)
>  	return delay;
>  }
>  
> -/**
> - * am65_cpts_rx_enable - enable rx timestamping
> - * @cpts: cpts handle
> - * @en: enable
> - *
> - * This functions enables rx packets timestamping. The CPTS can timestamp all
> - * rx packets.
> - */
> -void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en)
> -{
> -	u32 val;
> -
> -	mutex_lock(&cpts->ptp_clk_lock);
> -	val = am65_cpts_read32(cpts, control);
> -	if (en)
> -		val |= AM65_CPTS_CONTROL_TSTAMP_EN;
> -	else
> -		val &= ~AM65_CPTS_CONTROL_TSTAMP_EN;
> -	am65_cpts_write32(cpts, val, control);
> -	mutex_unlock(&cpts->ptp_clk_lock);
> -}
> -EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);

It looks like the above chunk will cause a transient build break, as
the function is still in use and the caller will be removed by the next
patch. I guess you should move this chunk there.

> -
>  static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>  {
>  	unsigned int ptp_class = ptp_classify_raw(skb);
> @@ -906,6 +883,72 @@ static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>  	return 1;
>  }
>  
> +static u64 am65_cpts_find_rx_ts(struct am65_cpts *cpts, struct sk_buff *skb,
> +				int ev_type, u32 skb_mtype_seqid)
> +{
> +	struct list_head *this, *next;
> +	struct am65_cpts_event *event;
> +	unsigned long flags;
> +	u32 mtype_seqid;
> +	u64 ns = 0;
> +
> +	am65_cpts_fifo_read(cpts);
> +	spin_lock_irqsave(&cpts->lock, flags);
> +	list_for_each_safe(this, next, &cpts->events) {
> +		event = list_entry(this, struct am65_cpts_event, list);
> +		if (time_after(jiffies, event->tmo)) {
> +			list_del_init(&event->list);
> +			list_add(&event->list, &cpts->pool);
> +			continue;
> +		}
> +
> +		mtype_seqid = event->event1 &
> +			      (AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK |
> +			       AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK |
> +			       AM65_CPTS_EVENT_1_EVENT_TYPE_MASK);
> +
> +		if (mtype_seqid == skb_mtype_seqid) {
> +			ns = event->timestamp;
> +			list_del_init(&event->list);
> +			list_add(&event->list, &cpts->pool);
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&cpts->lock, flags);

Ouch, you have to acquire an additional lock per packet and a lot of
cacheline dithering.

Not strictly necessary for this series, but I suggest to invest some
time reconsidering this schema, it looks bad from performance pov.

Possibly protecting the list with RCU and leaving the recycle to the
producer could help.

Additionally net-next is currently closed for the merge window, please
post the new revision (including the target tree in the subj prefix)
when net-next will re-open in ~2weeks.

Cheers,

Paolo


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO
  2024-03-12 10:50 ` [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Paolo Abeni
@ 2024-03-18 11:12   ` Chintan Vankar
  0 siblings, 0 replies; 5+ messages in thread
From: Chintan Vankar @ 2024-03-18 11:12 UTC (permalink / raw)
  To: Paolo Abeni, Dan Carpenter, Siddharth Vadapalli, Heiner Kallweit,
	Vladimir Oltean, Grygorii Strashko, Andrew Lunn, Roger Quadros,
	Richard Cochran, Jakub Kicinski, Eric Dumazet, David S. Miller
  Cc: linux-kernel, netdev



On 12/03/24 16:20, Paolo Abeni wrote:
> On Tue, 2024-03-12 at 15:32 +0530, Chintan Vankar wrote:
>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>> index c66618d91c28..6c1d571c5e0b 100644
>> --- a/drivers/net/ethernet/ti/am65-cpts.c
>> +++ b/drivers/net/ethernet/ti/am65-cpts.c
>> @@ -859,29 +859,6 @@ static long am65_cpts_ts_work(struct ptp_clock_info *ptp)
>>   	return delay;
>>   }
>>   
>> -/**
>> - * am65_cpts_rx_enable - enable rx timestamping
>> - * @cpts: cpts handle
>> - * @en: enable
>> - *
>> - * This functions enables rx packets timestamping. The CPTS can timestamp all
>> - * rx packets.
>> - */
>> -void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en)
>> -{
>> -	u32 val;
>> -
>> -	mutex_lock(&cpts->ptp_clk_lock);
>> -	val = am65_cpts_read32(cpts, control);
>> -	if (en)
>> -		val |= AM65_CPTS_CONTROL_TSTAMP_EN;
>> -	else
>> -		val &= ~AM65_CPTS_CONTROL_TSTAMP_EN;
>> -	am65_cpts_write32(cpts, val, control);
>> -	mutex_unlock(&cpts->ptp_clk_lock);
>> -}
>> -EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);
> 
> It looks like the above chunk will cause a transient build break, as
> the function is still in use and the caller will be removed by the next
> patch. I guess you should move this chunk there.

Thank you for your suggestion. I will update the patch with this
change in the next version.

> 
>> -
>>   static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>>   {
>>   	unsigned int ptp_class = ptp_classify_raw(skb);
>> @@ -906,6 +883,72 @@ static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>>   	return 1;
>>   }
>>   
>> +static u64 am65_cpts_find_rx_ts(struct am65_cpts *cpts, struct sk_buff *skb,
>> +				int ev_type, u32 skb_mtype_seqid)
>> +{
>> +	struct list_head *this, *next;
>> +	struct am65_cpts_event *event;
>> +	unsigned long flags;
>> +	u32 mtype_seqid;
>> +	u64 ns = 0;
>> +
>> +	am65_cpts_fifo_read(cpts);
>> +	spin_lock_irqsave(&cpts->lock, flags);
>> +	list_for_each_safe(this, next, &cpts->events) {
>> +		event = list_entry(this, struct am65_cpts_event, list);
>> +		if (time_after(jiffies, event->tmo)) {
>> +			list_del_init(&event->list);
>> +			list_add(&event->list, &cpts->pool);
>> +			continue;
>> +		}
>> +
>> +		mtype_seqid = event->event1 &
>> +			      (AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK |
>> +			       AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK |
>> +			       AM65_CPTS_EVENT_1_EVENT_TYPE_MASK);
>> +
>> +		if (mtype_seqid == skb_mtype_seqid) {
>> +			ns = event->timestamp;
>> +			list_del_init(&event->list);
>> +			list_add(&event->list, &cpts->pool);
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&cpts->lock, flags);
> 
> Ouch, you have to acquire an additional lock per packet and a lot of
> cacheline dithering.
> 
> Not strictly necessary for this series, but I suggest to invest some
> time reconsidering this schema, it looks bad from performance pov.
> 
> Possibly protecting the list with RCU and leaving the recycle to the
> producer could help.

Thank you for pointing out this. I will consider your point and spend
some time on it.

> 
> Additionally net-next is currently closed for the merge window, please
> post the new revision (including the target tree in the subj prefix)
> when net-next will re-open in ~2weeks.
> 
Thank you for information. I will update the patch and post its next
version with above mentioned changes.

> Cheers,
> 
> Paolo
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-03-18 11:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-12 10:02 [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Chintan Vankar
2024-03-12 10:02 ` [PATCH v2 2/3] net: ethernet: ti: am65-cpsw: Enable RX HW timestamp only for PTP packets Chintan Vankar
2024-03-12 10:02 ` [PATCH v2 3/3] net: ethernet: ti: am65-cpsw-ethtool: Update rx_filters for device's timestamping Chintan Vankar
2024-03-12 10:50 ` [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO Paolo Abeni
2024-03-18 11:12   ` Chintan Vankar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).