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 <[email protected]>
---
drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
2 files changed, 67 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index c66618d91c28..92a3b40e60d6 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,67 @@ 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;
+
+ 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
The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
As a workaround, Disable timestamping on all RX packets and timestamp
only PTP packets.
Set Time Sync Receive bits in Time Sync control register so that
packets can be determined as a valid Ethernet Receive Event for time
synchronization.
Update the RX filter configuration to indicate Hardware Timestamping
support only for PTP packets.
Replace "am65_cpsw_rx_ts()" function with "am65_cpts_rx_timestamp()"
function which timestamps only PTP packets.
Fixes: b1f66a5bee07 ("net: ethernet: ti: am65-cpsw-nuss: enable packet timestamping support")
Signed-off-by: Chintan Vankar <[email protected]>
---
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 51 +++++++++++-------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 9d2f4ac783e4..ab843fb64b93 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,9 @@ 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);
+ skb_reset_mac_header(skb);
+ 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 +1333,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 +1356,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 +1365,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 +1401,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 +1412,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 +1428,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
On 2/15/2024 3:09 AM, Chintan Vankar wrote:
> 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 <[email protected]>
> ---
Reviewed-by: Jacob Keller <[email protected]>
On 2/15/2024 3:09 AM, Chintan Vankar wrote:
> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
>
What's different about timestamping only PTP packets that prevents this
port lock up?
On 15/02/2024 13:09, Chintan Vankar wrote:
> 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 <[email protected]>
> ---
> drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
> drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
> 2 files changed, 67 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c66618d91c28..92a3b40e60d6 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);
> -
This function is used in am65-cpsw-nuss.c so this patch will
break build.
This looks like preparation for the workaround in next patch
which affects only some platforms. So please restrict this limitation
only to those platforms that are affected.
> 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,67 @@ 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;
> +
> + 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)
> {
> }
>
--
cheers,
-roger
Hi,
On 15/02/2024 13:09, Chintan Vankar wrote:
> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
Does this affect all platforms using am65-cpsw?
If this affects only some platforms then you need to restrict the
workaround to those platforms only.
>
> As a workaround, Disable timestamping on all RX packets and timestamp
> only PTP packets.
This does not exactly reflect what you are doing here.
I think you need to clarify that you are disabling in-band RX timestamp
mechanism on all packets and using out of band mechanism to get the
RX timestamps only for certain type of packets.
>
> Set Time Sync Receive bits in Time Sync control register so that
> packets can be determined as a valid Ethernet Receive Event for time
> synchronization.
>
> Update the RX filter configuration to indicate Hardware Timestamping
> support only for PTP packets.
>
> Replace "am65_cpsw_rx_ts()" function with "am65_cpts_rx_timestamp()"
> function which timestamps only PTP packets.
>
> Fixes: b1f66a5bee07 ("net: ethernet: ti: am65-cpsw-nuss: enable packet timestamping support")
> Signed-off-by: Chintan Vankar <[email protected]>
> ---
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 51 +++++++++++-------------
> 1 file changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 9d2f4ac783e4..ab843fb64b93 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,9 @@ 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);
> + skb_reset_mac_header(skb);
Why do you need to add skb_reset_mac_header here?
> + if (port->rx_ts_enabled)
> + am65_cpts_rx_timestamp(common->cpts, skb);
The timestamp should be added before you do skb_put().
> skb->protocol = eth_type_trans(skb, ndev);
> am65_cpsw_nuss_rx_csum(skb, csum_info);
> napi_gro_receive(&common->napi_rx, skb);
> @@ -1334,7 +1333,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 +1356,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:
Can't PTP_V1 be supported?
> case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> @@ -1372,10 +1365,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 +1401,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 +1412,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 +1428,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;
> }
--
cheers,
-roger
On 15/02/2024 13:09, Chintan Vankar wrote:
> 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 <[email protected]>
> ---
> drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
> drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
> 2 files changed, 67 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c66618d91c28..92a3b40e60d6 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,67 @@ 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);
am65_cpts_fifo_read() is called from the CPTS interrupt handler and the
event is popped out of the FIFO and pushed into an event list.
Doesn't this race with that interrupt handler?
Can't you use that event list instead of reading cpts_fifo directly?
Something like am65_cpts_match_tx_ts()?
> + 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;
> +
> + 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)
> {
> }
>
--
cheers,
-roger
On 19/02/24 16:54, Roger Quadros wrote:
>
> On 15/02/2024 13:09, Chintan Vankar wrote:
>> 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 <[email protected]>
>> ---
>> drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
>> drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
>> 2 files changed, 67 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>> index c66618d91c28..92a3b40e60d6 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,67 @@ 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);
> am65_cpts_fifo_read() is called from the CPTS interrupt handler and the
> event is popped out of the FIFO and pushed into an event list.
>
> Doesn't this race with that interrupt handler?
Could you clarify why there be a race condition ?
> Can't you use that event list instead of reading cpts_fifo directly?
>
> Something like am65_cpts_match_tx_ts()?
>
>> + 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;
>> +
>> + 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)
>> {
>> }
>>
On 24/02/2024 10:59, Siddharth Vadapalli wrote:
> On Mon, Feb 19, 2024 at 12:59:55PM +0200, Roger Quadros wrote:
>> Hi,
>>
>> On 15/02/2024 13:09, Chintan Vankar wrote:
>>> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
>>> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
>>
> ...
>
>>>
>>> @@ -856,6 +852,9 @@ 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);
>>> + skb_reset_mac_header(skb);
>>
>> Why do you need to add skb_reset_mac_header here?
>>
>>> + if (port->rx_ts_enabled)
>>> + am65_cpts_rx_timestamp(common->cpts, skb);
>>
>> The timestamp should be added before you do skb_put().
>
> Roger,
>
> Could you please clarify why the timestamp should be added before
> skb_put()?
My bad. hwtimestamps lies after skb->end so it doesn't matter.
--
cheers,
-roger
On 26/02/2024 11:08, Chintan Vankar wrote:
>
> On 19/02/24 16:54, Roger Quadros wrote:
>>
>> On 15/02/2024 13:09, Chintan Vankar wrote:
>>> 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 <[email protected]>
>>> ---
>>> drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
>>> drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
>>> 2 files changed, 67 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>>> index c66618d91c28..92a3b40e60d6 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,67 @@ 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);
>> am65_cpts_fifo_read() is called from the CPTS interrupt handler and the
>> event is popped out of the FIFO and pushed into an event list.
>>
>> Doesn't this race with that interrupt handler?
> Could you clarify why there be a race condition ?
I'm not sure so was asking. The question is, do you have to call
am65_cpts_fifo_read() here? If yes, could you please add a comment why.
>> Can't you use that event list instead of reading cpts_fifo directly?
>>
>> Something like am65_cpts_match_tx_ts()?
>>
>>> + 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;
>>> +
>>> + 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)
>>> {
>>> }
>>>
--
cheers,
-roger
On 19/02/24 16:29, Roger Quadros wrote:
> Hi,
>
> On 15/02/2024 13:09, Chintan Vankar wrote:
>> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
>> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
> Does this affect all platforms using am65-cpsw?
Yes. It affects all platforms using am65-cpsw. I will update a
commit message for the same.
> If this affects only some platforms then you need to restrict the
> workaround to those platforms only.
>
>> As a workaround, Disable timestamping on all RX packets and timestamp
>> only PTP packets.
> This does not exactly reflect what you are doing here.
>
> I think you need to clarify that you are disabling in-band RX timestamp
> mechanism on all packets and using out of band mechanism to get the
> RX timestamps only for certain type of packets.
>
>> Set Time Sync Receive bits in Time Sync control register so that
>> packets can be determined as a valid Ethernet Receive Event for time
>> synchronization.
>>
>> Update the RX filter configuration to indicate Hardware Timestamping
>> support only for PTP packets.
>>
>> Replace "am65_cpsw_rx_ts()" function with "am65_cpts_rx_timestamp()"
>> function which timestamps only PTP packets.
>>
>> Fixes: b1f66a5bee07 ("net: ethernet: ti: am65-cpsw-nuss: enable packet timestamping support")
>> Signed-off-by: Chintan Vankar <[email protected]>
>> ---
>> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 51 +++++++++++-------------
>> 1 file changed, 24 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 9d2f4ac783e4..ab843fb64b93 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,9 @@ 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);
>> + skb_reset_mac_header(skb);
> Why do you need to add skb_reset_mac_header here?
>
>> + if (port->rx_ts_enabled)
>> + am65_cpts_rx_timestamp(common->cpts, skb);
> The timestamp should be added before you do skb_put().
>
>> skb->protocol = eth_type_trans(skb, ndev);
>> am65_cpsw_nuss_rx_csum(skb, csum_info);
>> napi_gro_receive(&common->napi_rx, skb);
>> @@ -1334,7 +1333,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 +1356,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:
> Can't PTP_V1 be supported?
No. As per TRM:
https://www.ti.com/lit/ug/spruim2h/spruim2h.pdf,
section 12.2.1.4.7 Common Platform Time Sync(CPTS)
only IEEE 1588-2008 which is PTP v2 is supported.
>
>> case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>> case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
>> case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>> @@ -1372,10 +1365,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 +1401,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 +1412,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 +1428,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;
>> }
On Mon, Feb 19, 2024 at 12:59:55PM +0200, Roger Quadros wrote:
> Hi,
>
> On 15/02/2024 13:09, Chintan Vankar wrote:
> > The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
> > an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
>
..
> >
> > @@ -856,6 +852,9 @@ 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);
> > + skb_reset_mac_header(skb);
>
> Why do you need to add skb_reset_mac_header here?
>
> > + if (port->rx_ts_enabled)
> > + am65_cpts_rx_timestamp(common->cpts, skb);
>
> The timestamp should be added before you do skb_put().
Roger,
Could you please clarify why the timestamp should be added before
skb_put()?
Regards,
Siddharth.
On 17/02/24 03:51, Jacob Keller wrote:
>
>
> On 2/15/2024 3:09 AM, Chintan Vankar wrote:
>> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
>> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
>>
>
> What's different about timestamping only PTP packets that prevents this
> port lock up?
The difference is the way we are timestamping the packets. Instead of
getting the timestamp from CPTS module, we are getting the timestamp
from CPTS Event FIFO.
In 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 was responsible for the CPSW port to lock up in certain
condition. We are preventing port lock up by disabling TSTAMP_EN bit in
CPTS_CONTROL register.
The mechanism we are following in this patch, utilizes the CPTS Event
FIFO that records timestamps corresponding to certain events, with one
such event being the reception of an Ethernet packet with EtherType
field set to PTP.
On 26/02/24 18:19, Roger Quadros wrote:
>
>
> On 26/02/2024 11:08, Chintan Vankar wrote:
>>
>> On 19/02/24 16:54, Roger Quadros wrote:
>>>
>>> On 15/02/2024 13:09, Chintan Vankar wrote:
>>>> 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 <[email protected]>
>>>> ---
>>>> drivers/net/ethernet/ti/am65-cpts.c | 84 +++++++++++++++++++++--------
>>>> drivers/net/ethernet/ti/am65-cpts.h | 11 ++--
>>>> 2 files changed, 67 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>>>> index c66618d91c28..92a3b40e60d6 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,67 @@ 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);
>>> am65_cpts_fifo_read() is called from the CPTS interrupt handler and the
>>> event is popped out of the FIFO and pushed into an event list.
>>>
>>> Doesn't this race with that interrupt handler?
>> Could you clarify why there be a race condition ?
>
> I'm not sure so was asking. The question is, do you have to call
> am65_cpts_fifo_read() here? If yes, could you please add a comment why.
Yes. We need to call "am65_cpts_fifo_read()" here. If we don't call it
here then there will be a race condition for the event to be added to
the list before "am65_cpts_get_rx_ts()" function is called.
If we have to implement it without invoking "am65_cpts_fifo_read()",
then there has to be a check to ensure that the interrupt associated
with the RX Event has successfully added that event to the list.
Otherwise, due to the nature of "spin_lock_irqsave", when the interrupt
has acquired the lock on one CPU, the "am65_cpts_get_rx_ts()" function
running on another CPU might acquire the lock from the interrupt and
search for the RX Event which the "am65_cpts_fifo_read()" invoked by
the interrupt was supposed to add in the event list. This is racy due
to which it is possible that the event is not yet added when
"am65_cpts_get_rx_ts()" is invoked. Therefore, to ensure that the
event is surely present, invoking "am65_cpts_fifo_read()" is
required.
>
>>> Can't you use that event list instead of reading cpts_fifo directly?
>>>
>>> Something like am65_cpts_match_tx_ts()?
>>>
>>>> + 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;
>>>> +
>>>> + 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)
>>>> {
>>>> }
>>>>
>
> -----Original Message-----
> From: Chintan Vankar <[email protected]>
> Sent: Friday, March 1, 2024 2:49 AM
> To: Keller, Jacob E <[email protected]>; Dan Carpenter
> <[email protected]>; Roger Quadros <[email protected]>; Siddharth
> Vadapalli <[email protected]>; Richard Cochran <[email protected]>;
> Paolo Abeni <[email protected]>; Jakub Kicinski <[email protected]>; Eric
> Dumazet <[email protected]>; David S. Miller <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH net-next 2/2] net: ethernet: ti: am65-cpsw: Enable RX HW
> timestamp only for PTP packets
>
>
>
> On 17/02/24 03:51, Jacob Keller wrote:
> >
> >
> > On 2/15/2024 3:09 AM, Chintan Vankar wrote:
> >> The CPSW peripherals on J7AHP, J7VCL, J7AEP, J7ES, AM64 SoCs have
> >> an errata i2401 "CPSW: Host Timestamps Cause CPSW Port to Lock up".
> >>
> >
> > What's different about timestamping only PTP packets that prevents this
> > port lock up?
>
> The difference is the way we are timestamping the packets. Instead of
> getting the timestamp from CPTS module, we are getting the timestamp
> from CPTS Event FIFO.
>
> In 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 was responsible for the CPSW port to lock up in certain
> condition. We are preventing port lock up by disabling TSTAMP_EN bit in
> CPTS_CONTROL register.
>
> The mechanism we are following in this patch, utilizes the CPTS Event
> FIFO that records timestamps corresponding to certain events, with one
> such event being the reception of an Ethernet packet with EtherType
> field set to PTP.
Ok that explains it. Thanks!
Regards,
Jake