LinuxLists.cc - [PATCH v2 0/7] Add PTP support for BCM53128 switch

[permalink] [raw]

Subject: [PATCH v2 7/7] net: dsa: b53: Expose PTP timestamping ioctls to userspace

Allow userspace to use the PTP support. Currently only L2 is supported.

Signed-off-by: Martin Kaistra <[email protected]>
Reviewed-by: Kurt Kanzenbach <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 2 +
drivers/net/dsa/b53/b53_ptp.c | 90 +++++++++++++++++++++++++++++++-
drivers/net/dsa/b53/b53_ptp.h | 14 +++++
3 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 56a9de89b38b..3e7e5f83cc84 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2302,6 +2302,8 @@ static const struct dsa_switch_ops b53_switch_ops = {
.get_ts_info = b53_get_ts_info,
.port_rxtstamp = b53_port_rxtstamp,
.port_txtstamp = b53_port_txtstamp,
+ .port_hwtstamp_set = b53_port_hwtstamp_set,
+ .port_hwtstamp_get = b53_port_hwtstamp_get,
};

struct b53_chip_data {
diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
index 5567135ba8b9..f611ac219fb5 100644
--- a/drivers/net/dsa/b53/b53_ptp.c
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -260,13 +260,99 @@ int b53_get_ts_info(struct dsa_switch *ds, int port,
info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
SOF_TIMESTAMPING_RX_HARDWARE |
SOF_TIMESTAMPING_RAW_HARDWARE;
- info->tx_types = BIT(HWTSTAMP_TX_OFF);
- info->rx_filters = BIT(HWTSTAMP_FILTER_NONE);
+ info->tx_types = BIT(HWTSTAMP_TX_ON);
+ info->rx_filters = BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT);

return 0;
}
EXPORT_SYMBOL(b53_get_ts_info);

+static int b53_set_hwtstamp_config(struct b53_device *dev, int port,
+ struct hwtstamp_config *config)
+{
+ struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
+ bool tstamp_enable = false;
+
+ clear_bit_unlock(B53_HWTSTAMP_ENABLED, &ps->state);
+
+ /* Reserved for future extensions */
+ if (config->flags)
+ return -EINVAL;
+
+ switch (config->tx_type) {
+ case HWTSTAMP_TX_ON:
+ tstamp_enable = true;
+ break;
+ case HWTSTAMP_TX_OFF:
+ tstamp_enable = false;
+ break;
+ default:
+ return -ERANGE;
+ }
+
+ switch (config->rx_filter) {
+ case HWTSTAMP_FILTER_NONE:
+ tstamp_enable = false;
+ break;
+ case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+ config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+ break;
+ default:
+ return -ERANGE;
+ }
+
+ if (ps->tx_skb) {
+ dev_kfree_skb_any(ps->tx_skb);
+ ps->tx_skb = NULL;
+ }
+ clear_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
+
+ if (tstamp_enable)
+ set_bit(B53_HWTSTAMP_ENABLED, &ps->state);
+
+ return 0;
+}
+
+int b53_port_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
+{
+ struct b53_device *dev = ds->priv;
+ struct b53_port_hwtstamp *ps;
+ struct hwtstamp_config config;
+ int err;
+
+ ps = &dev->ports[port].port_hwtstamp;
+
+ if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+ return -EFAULT;
+
+ err = b53_set_hwtstamp_config(dev, port, &config);
+ if (err)
+ return err;
+
+ /* Save the chosen configuration to be returned later */
+ memcpy(&ps->tstamp_config, &config, sizeof(config));
+
+ return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT :
+ 0;
+}
+EXPORT_SYMBOL(b53_port_hwtstamp_set);
+
+int b53_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
+{
+ struct b53_device *dev = ds->priv;
+ struct b53_port_hwtstamp *ps;
+ struct hwtstamp_config *config;
+
+ ps = &dev->ports[port].port_hwtstamp;
+ config = &ps->tstamp_config;
+
+ return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ? -EFAULT :
+ 0;
+}
+EXPORT_SYMBOL(b53_port_hwtstamp_get);
+
void b53_ptp_exit(struct b53_device *dev)
{
if (dev->ptp_clock)
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
index f888f0a2022a..3a341f752e31 100644
--- a/drivers/net/dsa/b53/b53_ptp.h
+++ b/drivers/net/dsa/b53/b53_ptp.h
@@ -17,6 +17,8 @@ int b53_ptp_init(struct b53_device *dev);
void b53_ptp_exit(struct b53_device *dev);
int b53_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
+int b53_port_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr);
+int b53_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr);
bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
unsigned int type);
void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
@@ -38,6 +40,18 @@ static inline int b53_get_ts_info(struct dsa_switch *ds, int port,
return -EOPNOTSUPP;
}

+static inline int b53_port_hwtstamp_set(struct dsa_switch *ds, int port,
+ struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int b53_port_hwtstamp_get(struct dsa_switch *ds, int port,
+ struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
+
static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
struct sk_buff *skb, unsigned int type)
{
--
2.20.1

2021-11-09 19:34:12

[permalink] [raw]

Subject: [PATCH v2 5/7] net: dsa: b53: Add logic for RX timestamping

Packets received by the tagger with opcode=1 contain the 32-bit timestamp
according to the timebase register. This timestamp is saved in
BRCM_SKB_CB(skb)->meta_tstamp. b53_port_rxtstamp() takes this
and puts the full time information from the timecounter into
shwt->hwtstamp.

Signed-off-by: Martin Kaistra <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 1 +
drivers/net/dsa/b53/b53_ptp.c | 28 +++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 10 +++++++++
include/linux/dsa/b53.h | 30 +++++++++++++++++++++++++++
net/dsa/tag_brcm.c | 35 ++++++++++++++++++++++++--------
5 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index ed590efbd3bf..a9408f9cd414 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2300,6 +2300,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
.port_max_mtu = b53_get_max_mtu,
.port_change_mtu = b53_change_mtu,
.get_ts_info = b53_get_ts_info,
+ .port_rxtstamp = b53_port_rxtstamp,
};

struct b53_chip_data {
diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
index 8629c510b1a0..f8dd8d484d93 100644
--- a/drivers/net/dsa/b53/b53_ptp.c
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -6,6 +6,8 @@
* Copyright (C) 2021 Linutronix GmbH
*/

+#include <linux/ptp_classify.h>
+
#include "b53_priv.h"
#include "b53_ptp.h"

@@ -106,6 +108,32 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
return B53_PTP_OVERFLOW_PERIOD;
}

+bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
+ unsigned int type)
+{
+ struct b53_device *dev = ds->priv;
+ struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
+ struct skb_shared_hwtstamps *shwt;
+ u64 ns;
+
+ if (type != PTP_CLASS_V2_L2)
+ return false;
+
+ if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
+ return false;
+
+ mutex_lock(&dev->ptp_mutex);
+ ns = timecounter_cyc2time(&dev->tc, BRCM_SKB_CB(skb)->meta_tstamp);
+ mutex_unlock(&dev->ptp_mutex);
+
+ shwt = skb_hwtstamps(skb);
+ memset(shwt, 0, sizeof(*shwt));
+ shwt->hwtstamp = ns_to_ktime(ns);
+
+ return false;
+}
+EXPORT_SYMBOL(b53_port_rxtstamp);
+
int b53_ptp_init(struct b53_device *dev)
{
mutex_init(&dev->ptp_mutex);
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
index 5cd2fd9621a2..3b3437870c55 100644
--- a/drivers/net/dsa/b53/b53_ptp.h
+++ b/drivers/net/dsa/b53/b53_ptp.h
@@ -9,11 +9,15 @@

#include "b53_priv.h"

+#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
+
#ifdef CONFIG_B53_PTP
int b53_ptp_init(struct b53_device *dev);
void b53_ptp_exit(struct b53_device *dev);
int b53_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
+bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
+ unsigned int type);
#else /* !CONFIG_B53_PTP */

static inline int b53_ptp_init(struct b53_device *dev)
@@ -31,5 +35,11 @@ static inline int b53_get_ts_info(struct dsa_switch *ds, int port,
return -EOPNOTSUPP;
}

+static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb, unsigned int type)
+{
+ return false;
+}
+
#endif
#endif
diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
index 85aa6d9dc53d..542e5e3040d6 100644
--- a/include/linux/dsa/b53.h
+++ b/include/linux/dsa/b53.h
@@ -46,9 +46,32 @@ struct b53_io_ops {
struct phylink_link_state *state);
};

+/* state flags for b53_port_hwtstamp::state */
+enum {
+ B53_HWTSTAMP_ENABLED,
+ B53_HWTSTAMP_TX_IN_PROGRESS,
+};
+
+struct b53_port_hwtstamp {
+ /* Port index */
+ int port_id;
+
+ /* Timestamping state */
+ unsigned long state;
+
+ /* Resources for transmit timestamping */
+ unsigned long tx_tstamp_start;
+ struct sk_buff *tx_skb;
+
+ /* Current timestamp configuration */
+ struct hwtstamp_config tstamp_config;
+};
+
struct b53_port {
u16 vlan_ctl_mask;
struct ethtool_eee eee;
+ /* Per-port timestamping resources */
+ struct b53_port_hwtstamp port_hwtstamp;
};

struct b53_vlan {
@@ -112,3 +135,10 @@ struct b53_device {
#define B53_PTP_OVERFLOW_PERIOD (HZ / 2)
struct delayed_work overflow_work;
};
+
+struct brcm_skb_cb {
+ struct sk_buff *clone;
+ u32 meta_tstamp;
+};
+
+#define BRCM_SKB_CB(skb) ((struct brcm_skb_cb *)(skb)->cb)
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 96dbb8ee2fee..d611c1073deb 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -9,6 +9,7 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
#include <linux/slab.h>
+#include <linux/dsa/b53.h>

#include "dsa_priv.h"

@@ -31,7 +32,10 @@
/* 6th byte in the tag */
#define BRCM_LEG_PORT_ID (0xf)

-/* Newer Broadcom tag (4 bytes) */
+/* Newer Broadcom tag (4 bytes)
+ * For egress, when opcode = 0001, additional 4 bytes are used for
+ * the time stamp.
+ */
#define BRCM_TAG_LEN 4

/* Tag is constructed and desconstructed using byte by byte access
@@ -136,19 +140,29 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
*/
static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
struct net_device *dev,
- unsigned int offset)
+ unsigned int offset,
+ int *tag_len)
{
int source_port;
u8 *brcm_tag;
+ u32 tstamp;
+
+ *tag_len = 8;

if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
return NULL;

brcm_tag = skb->data - offset;

- /* The opcode should never be different than 0b000 */
- if (unlikely((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK))
- return NULL;
+ if ((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK) {
+ if (unlikely(!pskb_may_pull(skb, *tag_len)))
+ return NULL;
+
+ tstamp = brcm_tag[4] << 24 | brcm_tag[5] << 16 | brcm_tag[6] << 8 | brcm_tag[7];
+ BRCM_SKB_CB(skb)->meta_tstamp = tstamp;
+ } else {
+ *tag_len = BRCM_TAG_LEN;
+ }

/* We should never see a reserved reason code without knowing how to
* handle it
@@ -164,7 +178,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
return NULL;

/* Remove Broadcom tag and update checksum */
- skb_pull_rcsum(skb, BRCM_TAG_LEN);
+ skb_pull_rcsum(skb, *tag_len);

dsa_default_offload_fwd_mark(skb);

@@ -184,13 +198,14 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb,
static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
struct sk_buff *nskb;
+ int tag_len;

/* skb->data points to the EtherType, the tag is right before it */
- nskb = brcm_tag_rcv_ll(skb, dev, 2);
+ nskb = brcm_tag_rcv_ll(skb, dev, 2, &tag_len);
if (!nskb)
return nskb;

- dsa_strip_etype_header(skb, BRCM_TAG_LEN);
+ dsa_strip_etype_header(skb, tag_len);

return nskb;
}
@@ -295,8 +310,10 @@ static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
static struct sk_buff *brcm_tag_rcv_prepend(struct sk_buff *skb,
struct net_device *dev)
{
+ int tag_len;
+
/* tag is prepended to the packet */
- return brcm_tag_rcv_ll(skb, dev, ETH_HLEN);
+ return brcm_tag_rcv_ll(skb, dev, ETH_HLEN, &tag_len);
}

static const struct dsa_device_ops brcm_prepend_netdev_ops = {
--
2.20.1

2021-11-09 19:34:11

[permalink] [raw]

Subject: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

In order to get the switch to generate a timestamp for a transmitted
packet, we need to set the TS bit in the BRCM tag. The switch will then
create a status frame, which gets send back to the cpu.
In b53_port_txtstamp() we put the skb into a waiting position.

When a status frame is received, we extract the timestamp and put the time
according to our timecounter into the waiting skb. When
TX_TSTAMP_TIMEOUT is reached and we have no means to correctly get back
a full timestamp, we cancel the process.

As the status frame doesn't contain a reference to the original packet,
only one packet with timestamp request can be sent at a time.

Signed-off-by: Martin Kaistra <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 1 +
drivers/net/dsa/b53/b53_ptp.c | 56 ++++++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 8 +++++
net/dsa/tag_brcm.c | 46 ++++++++++++++++++++++++++
4 files changed, 111 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a9408f9cd414..56a9de89b38b 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2301,6 +2301,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
.port_change_mtu = b53_change_mtu,
.get_ts_info = b53_get_ts_info,
.port_rxtstamp = b53_port_rxtstamp,
+ .port_txtstamp = b53_port_txtstamp,
};

struct b53_chip_data {
diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
index f8dd8d484d93..5567135ba8b9 100644
--- a/drivers/net/dsa/b53/b53_ptp.c
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -100,14 +100,65 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
{
struct b53_device *dev =
container_of(ptp, struct b53_device, ptp_clock_info);
+ struct dsa_switch *ds = dev->ds;
+ int i;

mutex_lock(&dev->ptp_mutex);
timecounter_read(&dev->tc);
mutex_unlock(&dev->ptp_mutex);

+ for (i = 0; i < ds->num_ports; i++) {
+ struct b53_port_hwtstamp *ps;
+
+ if (!dsa_is_user_port(ds, i))
+ continue;
+
+ ps = &dev->ports[i].port_hwtstamp;
+
+ if (test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state) &&
+ time_is_before_jiffies(ps->tx_tstamp_start +
+ TX_TSTAMP_TIMEOUT)) {
+ dev_err(dev->dev,
+ "Timeout while waiting for Tx timestamp!\n");
+ dev_kfree_skb_any(ps->tx_skb);
+ ps->tx_skb = NULL;
+ clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS,
+ &ps->state);
+ }
+ }
+
return B53_PTP_OVERFLOW_PERIOD;
}

+void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
+{
+ struct b53_device *dev = ds->priv;
+ struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
+ struct sk_buff *clone;
+ unsigned int type;
+
+ type = ptp_classify_raw(skb);
+
+ if (type != PTP_CLASS_V2_L2)
+ return;
+
+ if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
+ return;
+
+ clone = skb_clone_sk(skb);
+ if (!clone)
+ return;
+
+ if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
+ kfree_skb(clone);
+ return;
+ }
+
+ ps->tx_skb = clone;
+ ps->tx_tstamp_start = jiffies;
+}
+EXPORT_SYMBOL(b53_port_txtstamp);
+
bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
unsigned int type)
{
@@ -136,6 +187,8 @@ EXPORT_SYMBOL(b53_port_rxtstamp);

int b53_ptp_init(struct b53_device *dev)
{
+ struct dsa_port *dp;
+
mutex_init(&dev->ptp_mutex);

/* Enable BroadSync HD for all ports */
@@ -191,6 +244,9 @@ int b53_ptp_init(struct b53_device *dev)

ptp_schedule_worker(dev->ptp_clock, 0);

+ dsa_switch_for_each_port(dp, dev->ds)
+ dp->priv = &dev->ports[dp->index].port_hwtstamp;
+
return 0;
}
EXPORT_SYMBOL(b53_ptp_init);
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
index 3b3437870c55..f888f0a2022a 100644
--- a/drivers/net/dsa/b53/b53_ptp.h
+++ b/drivers/net/dsa/b53/b53_ptp.h
@@ -10,6 +10,7 @@
#include "b53_priv.h"

#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
+#define TX_TSTAMP_TIMEOUT msecs_to_jiffies(40)

#ifdef CONFIG_B53_PTP
int b53_ptp_init(struct b53_device *dev);
@@ -18,6 +19,8 @@ int b53_get_ts_info(struct dsa_switch *ds, int port,
struct ethtool_ts_info *info);
bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
unsigned int type);
+void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
+
#else /* !CONFIG_B53_PTP */

static inline int b53_ptp_init(struct b53_device *dev)
@@ -41,5 +44,10 @@ static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
return false;
}

+static inline void b53_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
+{
+}
+
#endif
#endif
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index d611c1073deb..a44ac81fa097 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -8,6 +8,7 @@
#include <linux/dsa/brcm.h>
#include <linux/etherdevice.h>
#include <linux/list.h>
+#include <linux/ptp_classify.h>
#include <linux/slab.h>
#include <linux/dsa/b53.h>

@@ -76,6 +77,7 @@
#define BRCM_EG_TC_SHIFT 5
#define BRCM_EG_TC_MASK 0x7
#define BRCM_EG_PID_MASK 0x1f
+#define BRCM_EG_T_R 0x20

#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM) || \
IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
@@ -85,6 +87,8 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
unsigned int offset)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
+ unsigned int type = ptp_classify_raw(skb);
+ struct b53_port_hwtstamp *ps = dp->priv;
u16 queue = skb_get_queue_mapping(skb);
u8 *brcm_tag;

@@ -112,7 +116,13 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
*/
brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
((queue & BRCM_IG_TC_MASK) << BRCM_IG_TC_SHIFT);
+
brcm_tag[1] = 0;
+
+ if (type == PTP_CLASS_V2_L2 &&
+ test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
+ brcm_tag[1] = 1 << BRCM_IG_TS_SHIFT;
+
brcm_tag[2] = 0;
if (dp->index == 8)
brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
@@ -126,6 +136,33 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
return skb;
}

+static int set_txtstamp(struct dsa_port *dp,
+ int port,
+ u64 ns)
+{
+ struct b53_device *b53_dev = dp->ds->priv;
+ struct b53_port_hwtstamp *ps = dp->priv;
+ struct skb_shared_hwtstamps shhwtstamps;
+ struct sk_buff *tmp_skb;
+
+ if (!ps->tx_skb)
+ return 0;
+
+ mutex_lock(&b53_dev->ptp_mutex);
+ ns = timecounter_cyc2time(&b53_dev->tc, ns);
+ mutex_unlock(&b53_dev->ptp_mutex);
+
+ memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+ shhwtstamps.hwtstamp = ns_to_ktime(ns);
+ tmp_skb = ps->tx_skb;
+ ps->tx_skb = NULL;
+
+ clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
+ skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
+
+ return 0;
+}
+
/* Frames with this tag have one of these two layouts:
* -----------------------------------
* | MAC DA | MAC SA | 4b tag | Type | DSA_TAG_PROTO_BRCM
@@ -143,6 +180,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
unsigned int offset,
int *tag_len)
{
+ struct dsa_port *dp;
int source_port;
u8 *brcm_tag;
u32 tstamp;
@@ -177,6 +215,14 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
if (!skb->dev)
return NULL;

+ /* Check whether this is a status frame */
+ if (unlikely(*tag_len == 8 && brcm_tag[3] & BRCM_EG_T_R)) {
+ dp = dsa_slave_to_port(skb->dev);
+
+ set_txtstamp(dp, source_port, tstamp);
+ return NULL;
+ }
+
/* Remove Broadcom tag and update checksum */
skb_pull_rcsum(skb, *tag_len);

--
2.20.1

2021-11-09 19:36:33

[permalink] [raw]

Subject: Re: [PATCH v2 0/7] Add PTP support for BCM53128 switch

On Tue, Nov 09, 2021 at 10:50:02AM +0100, Martin Kaistra wrote:
> Ideally, for the B53=m case, I would have liked to include the PTP
> support in the b53_module itself, however I couldn't find a way to do
> that without renaming either the common source file or the module, which
> I didn't want to do.
>
> Instead, b53_ptp will be allowed as a loadable module, but only if
> b53_common is also a module, otherwise it will be built-in.

Does this not work?

obj-$(CONFIG_B53) += b53_common.o

ifdef CONFIG_B53_PTP
b53_common-objs += b53_ptp.o
endif

(haven't tried though)

2021-11-09 20:06:17

[permalink] [raw]

Subject: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

In order to access the b53 structs from net/dsa/tag_brcm.c move the
definitions from drivers/net/dsa/b53/b53_priv.h to the new file
include/linux/dsa/b53.h.

Signed-off-by: Martin Kaistra <[email protected]>
---
drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
2 files changed, 101 insertions(+), 89 deletions(-)
create mode 100644 include/linux/dsa/b53.h

diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 579da74ada64..1652e489b737 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -23,44 +23,13 @@
#include <linux/mutex.h>
#include <linux/phy.h>
#include <linux/etherdevice.h>
-#include <net/dsa.h>
+#include <linux/dsa/b53.h>

#include "b53_regs.h"

-struct b53_device;
struct net_device;
struct phylink_link_state;

-struct b53_io_ops {
- int (*read8)(struct b53_device *dev, u8 page, u8 reg, u8 *value);
- int (*read16)(struct b53_device *dev, u8 page, u8 reg, u16 *value);
- int (*read32)(struct b53_device *dev, u8 page, u8 reg, u32 *value);
- int (*read48)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
- int (*read64)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
- int (*write8)(struct b53_device *dev, u8 page, u8 reg, u8 value);
- int (*write16)(struct b53_device *dev, u8 page, u8 reg, u16 value);
- int (*write32)(struct b53_device *dev, u8 page, u8 reg, u32 value);
- int (*write48)(struct b53_device *dev, u8 page, u8 reg, u64 value);
- int (*write64)(struct b53_device *dev, u8 page, u8 reg, u64 value);
- int (*phy_read16)(struct b53_device *dev, int addr, int reg, u16 *value);
- int (*phy_write16)(struct b53_device *dev, int addr, int reg, u16 value);
- int (*irq_enable)(struct b53_device *dev, int port);
- void (*irq_disable)(struct b53_device *dev, int port);
- u8 (*serdes_map_lane)(struct b53_device *dev, int port);
- int (*serdes_link_state)(struct b53_device *dev, int port,
- struct phylink_link_state *state);
- void (*serdes_config)(struct b53_device *dev, int port,
- unsigned int mode,
- const struct phylink_link_state *state);
- void (*serdes_an_restart)(struct b53_device *dev, int port);
- void (*serdes_link_set)(struct b53_device *dev, int port,
- unsigned int mode, phy_interface_t interface,
- bool link_up);
- void (*serdes_phylink_validate)(struct b53_device *dev, int port,
- unsigned long *supported,
- struct phylink_link_state *state);
-};
-
#define B53_INVALID_LANE 0xff

enum {
@@ -89,63 +58,6 @@ enum {
#define B53_N_PORTS 9
#define B53_N_PORTS_25 6

-struct b53_port {
- u16 vlan_ctl_mask;
- struct ethtool_eee eee;
-};
-
-struct b53_vlan {
- u16 members;
- u16 untag;
- bool valid;
-};
-
-struct b53_device {
- struct dsa_switch *ds;
- struct b53_platform_data *pdata;
- const char *name;
-
- struct mutex reg_mutex;
- struct mutex stats_mutex;
- struct mutex arl_mutex;
- const struct b53_io_ops *ops;
-
- /* chip specific data */
- u32 chip_id;
- u8 core_rev;
- u8 vta_regs[3];
- u8 duplex_reg;
- u8 jumbo_pm_reg;
- u8 jumbo_size_reg;
- int reset_gpio;
- u8 num_arl_bins;
- u16 num_arl_buckets;
- enum dsa_tag_protocol tag_protocol;
-
- /* used ports mask */
- u16 enabled_ports;
- unsigned int imp_port;
-
- /* connect specific data */
- u8 current_page;
- struct device *dev;
- u8 serdes_lane;
-
- /* Master MDIO bus we got probed from */
- struct mii_bus *bus;
-
- void *priv;
-
- /* run time configuration */
- bool enable_jumbo;
-
- unsigned int num_vlans;
- struct b53_vlan *vlans;
- bool vlan_enabled;
- unsigned int num_ports;
- struct b53_port *ports;
-};
-
#define b53_for_each_port(dev, i) \
for (i = 0; i < B53_N_PORTS; i++) \
if (dev->enabled_ports & BIT(i))
diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
new file mode 100644
index 000000000000..af782a1da362
--- /dev/null
+++ b/include/linux/dsa/b53.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (C) 2011-2013 Jonas Gorski <[email protected]>
+ *
+ * Included by drivers/net/dsa/b53/b53_priv.h and net/dsa/tag_brcm.c
+ */
+
+#include <net/dsa.h>
+
+struct b53_device;
+struct phylink_link_state;
+
+struct b53_io_ops {
+ int (*read8)(struct b53_device *dev, u8 page, u8 reg, u8 *value);
+ int (*read16)(struct b53_device *dev, u8 page, u8 reg, u16 *value);
+ int (*read32)(struct b53_device *dev, u8 page, u8 reg, u32 *value);
+ int (*read48)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
+ int (*read64)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
+ int (*write8)(struct b53_device *dev, u8 page, u8 reg, u8 value);
+ int (*write16)(struct b53_device *dev, u8 page, u8 reg, u16 value);
+ int (*write32)(struct b53_device *dev, u8 page, u8 reg, u32 value);
+ int (*write48)(struct b53_device *dev, u8 page, u8 reg, u64 value);
+ int (*write64)(struct b53_device *dev, u8 page, u8 reg, u64 value);
+ int (*phy_read16)(struct b53_device *dev, int addr, int reg,
+ u16 *value);
+ int (*phy_write16)(struct b53_device *dev, int addr, int reg,
+ u16 value);
+ int (*irq_enable)(struct b53_device *dev, int port);
+ void (*irq_disable)(struct b53_device *dev, int port);
+ u8 (*serdes_map_lane)(struct b53_device *dev, int port);
+ int (*serdes_link_state)(struct b53_device *dev, int port,
+ struct phylink_link_state *state);
+ void (*serdes_config)(struct b53_device *dev, int port,
+ unsigned int mode,
+ const struct phylink_link_state *state);
+ void (*serdes_an_restart)(struct b53_device *dev, int port);
+ void (*serdes_link_set)(struct b53_device *dev, int port,
+ unsigned int mode, phy_interface_t interface,
+ bool link_up);
+ void (*serdes_phylink_validate)(struct b53_device *dev, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state);
+};
+
+struct b53_port {
+ u16 vlan_ctl_mask;
+ struct ethtool_eee eee;
+};
+
+struct b53_vlan {
+ u16 members;
+ u16 untag;
+ bool valid;
+};
+
+struct b53_device {
+ struct dsa_switch *ds;
+ struct b53_platform_data *pdata;
+ const char *name;
+
+ struct mutex reg_mutex;
+ struct mutex stats_mutex;
+ struct mutex arl_mutex;
+ const struct b53_io_ops *ops;
+
+ /* chip specific data */
+ u32 chip_id;
+ u8 core_rev;
+ u8 vta_regs[3];
+ u8 duplex_reg;
+ u8 jumbo_pm_reg;
+ u8 jumbo_size_reg;
+ int reset_gpio;
+ u8 num_arl_bins;
+ u16 num_arl_buckets;
+ enum dsa_tag_protocol tag_protocol;
+
+ /* used ports mask */
+ u16 enabled_ports;
+ unsigned int imp_port;
+
+ /* connect specific data */
+ u8 current_page;
+ struct device *dev;
+ u8 serdes_lane;
+
+ /* Master MDIO bus we got probed from */
+ struct mii_bus *bus;
+
+ void *priv;
+
+ /* run time configuration */
+ bool enable_jumbo;
+
+ unsigned int num_vlans;
+ struct b53_vlan *vlans;
+ bool vlan_enabled;
+ unsigned int num_ports;
+ struct b53_port *ports;
+};
--
2.20.1

2021-11-09 20:06:17

[permalink] [raw]

Subject: [PATCH v2 1/7] net: dsa: b53: Add BroadSync HD register definitions

From: Kurt Kanzenbach <[email protected]>

Add register definitions for the BroadSync HD features of
BCM53128. These will be used to enable PTP support.

Signed-off-by: Kurt Kanzenbach <[email protected]>
Signed-off-by: Martin Kaistra <[email protected]>
---
drivers/net/dsa/b53/b53_regs.h | 71 ++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index b2c539a42154..0deb11a7c9cd 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -50,6 +50,12 @@
/* Jumbo Frame Registers */
#define B53_JUMBO_PAGE 0x40

+/* BroadSync HD Register Page */
+#define B53_BROADSYNC_PAGE 0x90
+
+/* Traffic Remarking Register Page */
+#define B53_TRAFFICREMARKING_PAGE 0x91
+
/* EEE Control Registers Page */
#define B53_EEE_PAGE 0x92

@@ -260,6 +266,27 @@
/* Broadcom header TX control (16 bit) */
#define B53_BRCM_HDR_TX_DIS 0x62

+/*************************************************************************
+ * ARL Control Registers
+ *************************************************************************/
+
+/* Multiport Control Register (16 bit) */
+#define B53_MPORT_CTRL 0x0e
+#define MPORT_CTRL_DIS_FORWARD 0
+#define MPORT_CTRL_CMP_ETYPE 1
+#define MPORT_CTRL_CMP_ADDR 2
+#define MPORT_CTRL_CMP_ADDR_ETYPE 3
+#define MPORT_CTRL_SHIFT(x) ((x) << 1)
+#define MPORT_CTRL_MASK 0x2
+#define MPORT0_TS_EN BIT(15)
+
+/* Multiport Address N (N = 0–5) Register (64 bit) */
+#define B53_MPORT_ADDR(n) (0x10 + ((n) << 4))
+#define MPORT_ETYPE(x) ((u64)(x) << 48)
+
+/* Multiport Vector N (N = 0–5) Register (32 bit) */
+#define B53_MPORT_VCTR(n) (0x18 + ((n) << 4))
+
/*************************************************************************
* ARL Access Page Registers
*************************************************************************/
@@ -479,6 +506,50 @@
#define JMS_MIN_SIZE 1518
#define JMS_MAX_SIZE 9724

+/*************************************************************************
+ * BroadSync HD Page Registers
+ *************************************************************************/
+
+/* BroadSync HD Enable Control Register (16 bit) */
+#define B53_BROADSYNC_EN_CTRL 0x00
+
+/* BroadSync HD Time Stamp Report Control Register */
+#define B53_BROADSYNC_TS_REPORT_CTRL 0x02
+#define TSRPT_PKT_EN BIT(0)
+
+/* BroadSync HD PCP Value Control Register */
+#define B53_BROADSYNC_PCP_CTRL 0x03
+
+/* BroadSync HD Max Packet Size Register */
+#define B53_BROADSYNC_MAX_SDU 0x04
+
+/* BroadSync HD Time Base Register (32 bit) */
+#define B53_BROADSYNC_TIMEBASE 0x10
+
+/* BroadSync HD Time Base Adjustment Register (32 bit) */
+#define B53_BROADSYNC_TIMEBASE_ADJ 0x14
+
+/* BroadSync HD Slot Number and Tick Counter Register (32 bit) */
+#define B53_BROADSYNC_SLOT_CNT 0x18
+
+/* BroadSync HD Slot Adjustment Register (32 bit) */
+#define B53_BROADSYNC_SLOT_ADJ 0x1c
+
+/* BroadSync HD Class 5 Bandwidth Control Register */
+#define B53_BROADSYNC_CLS5_BW_CTRL 0x30
+
+/* BroadSync HD Class 4 Bandwidth Control Register */
+#define B53_BROADSYNC_CLS4_BW_CTRL 0x60
+
+/* BroadSync HD Egress Time Stamp Register */
+#define B53_BROADSYNC_EGRESS_TS 0x90
+
+/* BroadSync HD Egress Time Stamp Status Register */
+#define B53_BROADSYNC_EGRESS_TS_STS 0xd0
+
+/* BroadSync HD Link Status Register (16 bit) */
+#define B53_BROADSYNC_LINK_STS 0xe0
+
/*************************************************************************
* EEE Configuration Page Registers
*************************************************************************/
--
2.20.1

2021-11-09 20:06:17

[permalink] [raw]

Subject: [PATCH v2 3/7] timecounter: allow for non-power of two overflow

Some hardware counters which are used as clocks have an overflow point
which is not a power of two. In order to be able to use the cycle
counter infrastructure with such hardware, add support for more generic
overflow logic.

Signed-off-by: Martin Kaistra <[email protected]>
---
include/linux/timecounter.h | 3 +++
kernel/time/timecounter.c | 3 +++
2 files changed, 6 insertions(+)

diff --git a/include/linux/timecounter.h b/include/linux/timecounter.h
index c6540ceea143..c71196a742b3 100644
--- a/include/linux/timecounter.h
+++ b/include/linux/timecounter.h
@@ -26,12 +26,15 @@
* see CYCLECOUNTER_MASK() helper macro
* @mult: cycle to nanosecond multiplier
* @shift: cycle to nanosecond divisor (power of two)
+ * @overflow_point: non-power of two overflow point (optional),
+ * smaller than mask
*/
struct cyclecounter {
u64 (*read)(const struct cyclecounter *cc);
u64 mask;
u32 mult;
u32 shift;
+ u64 overflow_point;
};

/**
diff --git a/kernel/time/timecounter.c b/kernel/time/timecounter.c
index e6285288d765..afd2910a9724 100644
--- a/kernel/time/timecounter.c
+++ b/kernel/time/timecounter.c
@@ -39,6 +39,9 @@ static u64 timecounter_read_delta(struct timecounter *tc)
/* calculate the delta since the last timecounter_read_delta(): */
cycle_delta = (cycle_now - tc->cycle_last) & tc->cc->mask;

+ if (tc->cc->overflow_point && (cycle_now - tc->cycle_last) > tc->cc->mask)
+ cycle_delta -= tc->cc->mask - tc->cc->overflow_point;
+
/* convert to nanoseconds: */
ns_offset = cyclecounter_cyc2ns(tc->cc, cycle_delta,
tc->mask, &tc->frac);
--
2.20.1

2021-11-09 20:06:17

[permalink] [raw]

Subject: [PATCH v2 4/7] net: dsa: b53: Add PHC clock support

The BCM53128 switch has an internal clock, which can be used for
timestamping. Add support for it.

The 32-bit free running clock counts nanoseconds. In order to account
for the wrap-around at 999999999 (0x3B9AC9FF) while using the cycle
counter infrastructure, we need to set a 30bit mask and use the
overflow_point property.

Enable the Broadsync HD timestamping feature in b53_ptp_init() for PTPv2
Ethertype (0x88f7).

Signed-off-by: Martin Kaistra <[email protected]>
---
drivers/net/dsa/b53/Kconfig | 8 ++
drivers/net/dsa/b53/Makefile | 4 +
drivers/net/dsa/b53/b53_common.c | 17 +++
drivers/net/dsa/b53/b53_ptp.c | 196 +++++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 35 ++++++
include/linux/dsa/b53.h | 14 +++
6 files changed, 274 insertions(+)
create mode 100644 drivers/net/dsa/b53/b53_ptp.c
create mode 100644 drivers/net/dsa/b53/b53_ptp.h

diff --git a/drivers/net/dsa/b53/Kconfig b/drivers/net/dsa/b53/Kconfig
index 90b525160b71..71009c93db13 100644
--- a/drivers/net/dsa/b53/Kconfig
+++ b/drivers/net/dsa/b53/Kconfig
@@ -45,3 +45,11 @@ config B53_SERDES
default ARCH_BCM_NSP
help
Select to enable support for SerDes on e.g: Northstar Plus SoCs.
+
+config B53_PTP
+ bool "B53 PTP support"
+ depends on B53
+ depends on PTP_1588_CLOCK
+ default n
+ help
+ Select to enable support for PTP
diff --git a/drivers/net/dsa/b53/Makefile b/drivers/net/dsa/b53/Makefile
index b1be13023ae4..07c9ac1ce9e8 100644
--- a/drivers/net/dsa/b53/Makefile
+++ b/drivers/net/dsa/b53/Makefile
@@ -6,3 +6,7 @@ obj-$(CONFIG_B53_MDIO_DRIVER) += b53_mdio.o
obj-$(CONFIG_B53_MMAP_DRIVER) += b53_mmap.o
obj-$(CONFIG_B53_SRAB_DRIVER) += b53_srab.o
obj-$(CONFIG_B53_SERDES) += b53_serdes.o
+
+ifdef CONFIG_B53_PTP
+obj-$(CONFIG_B53) += b53_ptp.o
+endif
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index af4761968733..ed590efbd3bf 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -31,6 +31,7 @@

#include "b53_regs.h"
#include "b53_priv.h"
+#include "b53_ptp.h"

struct b53_mib_desc {
u8 size;
@@ -1131,12 +1132,24 @@ static int b53_setup(struct dsa_switch *ds)
b53_disable_port(ds, port);
}

+ if (dev->broadsync_hd) {
+ ret = b53_ptp_init(dev);
+ if (ret) {
+ dev_err(ds->dev, "failed to initialize PTP\n");
+ return ret;
+ }
+ }
+
return b53_setup_devlink_resources(ds);
}

static void b53_teardown(struct dsa_switch *ds)
{
+ struct b53_device *dev = ds->priv;
+
dsa_devlink_resources_unregister(ds);
+ if (dev->broadsync_hd)
+ b53_ptp_exit(ds->priv);
}

static void b53_force_link(struct b53_device *dev, int port, int link)
@@ -2286,6 +2299,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
.port_mdb_del = b53_mdb_del,
.port_max_mtu = b53_get_max_mtu,
.port_change_mtu = b53_change_mtu,
+ .get_ts_info = b53_get_ts_info,
};

struct b53_chip_data {
@@ -2301,6 +2315,7 @@ struct b53_chip_data {
u8 duplex_reg;
u8 jumbo_pm_reg;
u8 jumbo_size_reg;
+ bool broadsync_hd;
};

#define B53_VTA_REGS \
@@ -2421,6 +2436,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
.duplex_reg = B53_DUPLEX_STAT_GE,
.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
+ .broadsync_hd = true,
},
{
.chip_id = BCM63XX_DEVICE_ID,
@@ -2589,6 +2605,7 @@ static int b53_switch_init(struct b53_device *dev)
dev->num_vlans = chip->vlans;
dev->num_arl_bins = chip->arl_bins;
dev->num_arl_buckets = chip->arl_buckets;
+ dev->broadsync_hd = chip->broadsync_hd;
break;
}
}
diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
new file mode 100644
index 000000000000..8629c510b1a0
--- /dev/null
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: ISC
+/*
+ * B53 switch PTP support
+ *
+ * Author: Martin Kaistra <[email protected]>
+ * Copyright (C) 2021 Linutronix GmbH
+ */
+
+#include "b53_priv.h"
+#include "b53_ptp.h"
+
+static int b53_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+ struct b53_device *dev =
+ container_of(ptp, struct b53_device, ptp_clock_info);
+ u64 ns;
+
+ mutex_lock(&dev->ptp_mutex);
+ ns = timecounter_read(&dev->tc);
+ mutex_unlock(&dev->ptp_mutex);
+
+ *ts = ns_to_timespec64(ns);
+
+ return 0;
+}
+
+static int b53_ptp_settime(struct ptp_clock_info *ptp,
+ const struct timespec64 *ts)
+{
+ struct b53_device *dev =
+ container_of(ptp, struct b53_device, ptp_clock_info);
+ u64 ns;
+
+ ns = timespec64_to_ns(ts);
+
+ mutex_lock(&dev->ptp_mutex);
+ timecounter_init(&dev->tc, &dev->cc, ns);
+ mutex_unlock(&dev->ptp_mutex);
+
+ return 0;
+}
+
+static int b53_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+ struct b53_device *dev =
+ container_of(ptp, struct b53_device, ptp_clock_info);
+ u64 adj, diff;
+ u32 mult;
+ bool neg_adj = false;
+
+ if (scaled_ppm < 0) {
+ neg_adj = true;
+ scaled_ppm = -scaled_ppm;
+ }
+
+ mult = (1 << 28);
+ adj = 64;
+ adj *= (u64)scaled_ppm;
+ diff = div_u64(adj, 15625ULL);
+
+ mutex_lock(&dev->ptp_mutex);
+ timecounter_read(&dev->tc);
+ dev->cc.mult = neg_adj ? mult - diff : mult + diff;
+ mutex_unlock(&dev->ptp_mutex);
+
+ return 0;
+}
+
+static int b53_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+ struct b53_device *dev =
+ container_of(ptp, struct b53_device, ptp_clock_info);
+
+ mutex_lock(&dev->ptp_mutex);
+ timecounter_adjtime(&dev->tc, delta);
+ mutex_unlock(&dev->ptp_mutex);
+
+ return 0;
+}
+
+static u64 b53_ptp_read(const struct cyclecounter *cc)
+{
+ struct b53_device *dev = container_of(cc, struct b53_device, cc);
+ u32 ts;
+
+ b53_read32(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TIMEBASE, &ts);
+
+ return ts;
+}
+
+static int b53_ptp_enable(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ return -EOPNOTSUPP;
+}
+
+static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
+{
+ struct b53_device *dev =
+ container_of(ptp, struct b53_device, ptp_clock_info);
+
+ mutex_lock(&dev->ptp_mutex);
+ timecounter_read(&dev->tc);
+ mutex_unlock(&dev->ptp_mutex);
+
+ return B53_PTP_OVERFLOW_PERIOD;
+}
+
+int b53_ptp_init(struct b53_device *dev)
+{
+ mutex_init(&dev->ptp_mutex);
+
+ /* Enable BroadSync HD for all ports */
+ b53_write16(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_EN_CTRL,
+ dev->enabled_ports);
+
+ /* Enable BroadSync HD Time Stamping Reporting (Egress) */
+ b53_write8(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TS_REPORT_CTRL,
+ TSRPT_PKT_EN);
+
+ /* Enable BroadSync HD Time Stamping for PTPv2 ingress */
+
+ /* MPORT_CTRL0 | MPORT0_TS_EN */
+ b53_write16(dev, B53_ARLCTRL_PAGE, B53_MPORT_CTRL,
+ MPORT0_TS_EN |
+ (MPORT_CTRL_CMP_ETYPE << MPORT_CTRL_SHIFT(0)));
+ /* Forward to IMP port */
+ b53_write32(dev, B53_ARLCTRL_PAGE, B53_MPORT_VCTR(0),
+ BIT(dev->imp_port));
+ /* PTPv2 Ether Type */
+ b53_write64(dev, B53_ARLCTRL_PAGE, B53_MPORT_ADDR(0),
+ MPORT_ETYPE(ETH_P_1588));
+
+ /* Setup PTP clock */
+ memset(&dev->ptp_clock_info, 0, sizeof(dev->ptp_clock_info));
+
+ dev->ptp_clock_info.owner = THIS_MODULE;
+ snprintf(dev->ptp_clock_info.name, sizeof(dev->ptp_clock_info.name),
+ dev_name(dev->dev));
+
+ dev->ptp_clock_info.max_adj = 1000000000ULL;
+ dev->ptp_clock_info.adjfine = b53_ptp_adjfine;
+ dev->ptp_clock_info.adjtime = b53_ptp_adjtime;
+ dev->ptp_clock_info.gettime64 = b53_ptp_gettime;
+ dev->ptp_clock_info.settime64 = b53_ptp_settime;
+ dev->ptp_clock_info.enable = b53_ptp_enable;
+ dev->ptp_clock_info.do_aux_work = b53_hwtstamp_work;
+
+ dev->ptp_clock = ptp_clock_register(&dev->ptp_clock_info, dev->dev);
+ if (IS_ERR(dev->ptp_clock))
+ return PTR_ERR(dev->ptp_clock);
+
+ /* The switch provides a 32 bit free running counter. Use the Linux
+ * cycle counter infrastructure which is suited for such scenarios.
+ */
+ dev->cc.read = b53_ptp_read;
+ dev->cc.mask = CYCLECOUNTER_MASK(30);
+ dev->cc.overflow_point = 999999999;
+ dev->cc.mult = (1 << 28);
+ dev->cc.shift = 28;
+
+ timecounter_init(&dev->tc, &dev->cc, ktime_to_ns(ktime_get_real()));
+
+ ptp_schedule_worker(dev->ptp_clock, 0);
+
+ return 0;
+}
+EXPORT_SYMBOL(b53_ptp_init);
+
+int b53_get_ts_info(struct dsa_switch *ds, int port,
+ struct ethtool_ts_info *info)
+{
+ struct b53_device *dev = ds->priv;
+
+ info->phc_index = dev->ptp_clock ? ptp_clock_index(dev->ptp_clock) : -1;
+ info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+ SOF_TIMESTAMPING_RX_HARDWARE |
+ SOF_TIMESTAMPING_RAW_HARDWARE;
+ info->tx_types = BIT(HWTSTAMP_TX_OFF);
+ info->rx_filters = BIT(HWTSTAMP_FILTER_NONE);
+
+ return 0;
+}
+EXPORT_SYMBOL(b53_get_ts_info);
+
+void b53_ptp_exit(struct b53_device *dev)
+{
+ if (dev->ptp_clock)
+ ptp_clock_unregister(dev->ptp_clock);
+ dev->ptp_clock = NULL;
+}
+EXPORT_SYMBOL(b53_ptp_exit);
+
+MODULE_AUTHOR("Martin Kaistra <[email protected]>");
+MODULE_DESCRIPTION("B53 Switch PTP support");
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
new file mode 100644
index 000000000000..5cd2fd9621a2
--- /dev/null
+++ b/drivers/net/dsa/b53/b53_ptp.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Author: Martin Kaistra <[email protected]>
+ * Copyright (C) 2021 Linutronix GmbH
+ */
+
+#ifndef _B53_PTP_H
+#define _B53_PTP_H
+
+#include "b53_priv.h"
+
+#ifdef CONFIG_B53_PTP
+int b53_ptp_init(struct b53_device *dev);
+void b53_ptp_exit(struct b53_device *dev);
+int b53_get_ts_info(struct dsa_switch *ds, int port,
+ struct ethtool_ts_info *info);
+#else /* !CONFIG_B53_PTP */
+
+static inline int b53_ptp_init(struct b53_device *dev)
+{
+ return 0;
+}
+
+static inline void b53_ptp_exit(struct b53_device *dev)
+{
+}
+
+static inline int b53_get_ts_info(struct dsa_switch *ds, int port,
+ struct ethtool_ts_info *info)
+{
+ return -EOPNOTSUPP;
+}
+
+#endif
+#endif
diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
index af782a1da362..85aa6d9dc53d 100644
--- a/include/linux/dsa/b53.h
+++ b/include/linux/dsa/b53.h
@@ -1,10 +1,14 @@
/* SPDX-License-Identifier: ISC */
/*
* Copyright (C) 2011-2013 Jonas Gorski <[email protected]>
+ * Copyright (C) 2021 Linutronix GmbH
*
* Included by drivers/net/dsa/b53/b53_priv.h and net/dsa/tag_brcm.c
*/

+#include <linux/ptp_clock_kernel.h>
+#include <linux/timecounter.h>
+#include <linux/workqueue.h>
#include <net/dsa.h>

struct b53_device;
@@ -97,4 +101,14 @@ struct b53_device {
bool vlan_enabled;
unsigned int num_ports;
struct b53_port *ports;
+
+ /* PTP */
+ bool broadsync_hd;
+ struct ptp_clock *ptp_clock;
+ struct ptp_clock_info ptp_clock_info;
+ struct cyclecounter cc;
+ struct timecounter tc;
+ struct mutex ptp_mutex;
+#define B53_PTP_OVERFLOW_PERIOD (HZ / 2)
+ struct delayed_work overflow_work;
};
--
2.20.1

2021-11-09 20:50:01

[permalink] [raw]

Subject: Re: [PATCH v2 0/7] Add PTP support for BCM53128 switch

Am 09.11.21 um 11:39 schrieb Vladimir Oltean:
> On Tue, Nov 09, 2021 at 10:50:02AM +0100, Martin Kaistra wrote:
>> Ideally, for the B53=m case, I would have liked to include the PTP
>> support in the b53_module itself, however I couldn't find a way to do
>> that without renaming either the common source file or the module, which
>> I didn't want to do.
>>
>> Instead, b53_ptp will be allowed as a loadable module, but only if
>> b53_common is also a module, otherwise it will be built-in.
>
> Does this not work?
>
> obj-$(CONFIG_B53) += b53_common.o
>
> ifdef CONFIG_B53_PTP
> b53_common-objs += b53_ptp.o
> endif
>
> (haven't tried though)
>

I get:

arm-linux-gnueabihf-ld -EL -r -o drivers/net/dsa/b53/b53_common.o
drivers/net/dsa/b53/b53_ptp.o

and

ERROR: modpost: "b53_switch_register" [drivers/net/dsa/b53/b53_mdio.ko]
undefined!

ERROR: modpost: "b53_switch_alloc" [drivers/net/dsa/b53/b53_mdio.ko]
undefined!

ERROR: modpost: "b53_switch_register" [drivers/net/dsa/b53/b53_spi.ko]
undefined!

ERROR: modpost: "b53_switch_alloc" [drivers/net/dsa/b53/b53_spi.ko]
undefined!

It seems to me, that b53_common.c does not get included at all.

2021-11-09 21:25:40

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Tue, Nov 09, 2021 at 10:50:08AM +0100, Martin Kaistra wrote:
> In order to get the switch to generate a timestamp for a transmitted
> packet, we need to set the TS bit in the BRCM tag. The switch will then
> create a status frame, which gets send back to the cpu.
> In b53_port_txtstamp() we put the skb into a waiting position.
>
> When a status frame is received, we extract the timestamp and put the time
> according to our timecounter into the waiting skb. When
> TX_TSTAMP_TIMEOUT is reached and we have no means to correctly get back
> a full timestamp, we cancel the process.
>
> As the status frame doesn't contain a reference to the original packet,
> only one packet with timestamp request can be sent at a time.
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---
> drivers/net/dsa/b53/b53_common.c | 1 +
> drivers/net/dsa/b53/b53_ptp.c | 56 ++++++++++++++++++++++++++++++++
> drivers/net/dsa/b53/b53_ptp.h | 8 +++++
> net/dsa/tag_brcm.c | 46 ++++++++++++++++++++++++++
> 4 files changed, 111 insertions(+)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index a9408f9cd414..56a9de89b38b 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2301,6 +2301,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
> .port_change_mtu = b53_change_mtu,
> .get_ts_info = b53_get_ts_info,
> .port_rxtstamp = b53_port_rxtstamp,
> + .port_txtstamp = b53_port_txtstamp,
> };
>
> struct b53_chip_data {
> diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
> index f8dd8d484d93..5567135ba8b9 100644
> --- a/drivers/net/dsa/b53/b53_ptp.c
> +++ b/drivers/net/dsa/b53/b53_ptp.c
> @@ -100,14 +100,65 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
> {
> struct b53_device *dev =
> container_of(ptp, struct b53_device, ptp_clock_info);
> + struct dsa_switch *ds = dev->ds;
> + int i;
>
> mutex_lock(&dev->ptp_mutex);
> timecounter_read(&dev->tc);
> mutex_unlock(&dev->ptp_mutex);
>
> + for (i = 0; i < ds->num_ports; i++) {
> + struct b53_port_hwtstamp *ps;
> +
> + if (!dsa_is_user_port(ds, i))
> + continue;

dsa_switch_for_each_user_port and replace i with dp->index.
This makes for a more efficient iteration through the port list.

> +
> + ps = &dev->ports[i].port_hwtstamp;
> +
> + if (test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state) &&
> + time_is_before_jiffies(ps->tx_tstamp_start +
> + TX_TSTAMP_TIMEOUT)) {
> + dev_err(dev->dev,
> + "Timeout while waiting for Tx timestamp!\n");
> + dev_kfree_skb_any(ps->tx_skb);
> + ps->tx_skb = NULL;
> + clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS,
> + &ps->state);
> + }
> + }
> +
> return B53_PTP_OVERFLOW_PERIOD;
> }
>
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> +{
> + struct b53_device *dev = ds->priv;
> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> + struct sk_buff *clone;
> + unsigned int type;
> +
> + type = ptp_classify_raw(skb);
> +
> + if (type != PTP_CLASS_V2_L2)
> + return;
> +
> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> + return;
> +
> + clone = skb_clone_sk(skb);
> + if (!clone)
> + return;
> +
> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {

Is it ok if you simply don't timestamp a second skb which may be sent
while the first one is in flight, I wonder? What PTP profiles have you
tested with? At just one PTP packet at a time, the switch isn't giving
you a lot. Is it a hardware limitation?

> + kfree_skb(clone);
> + return;
> + }
> +
> + ps->tx_skb = clone;
> + ps->tx_tstamp_start = jiffies;
> +}
> +EXPORT_SYMBOL(b53_port_txtstamp);
> +
> bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> unsigned int type)
> {
> @@ -136,6 +187,8 @@ EXPORT_SYMBOL(b53_port_rxtstamp);
>
> int b53_ptp_init(struct b53_device *dev)
> {
> + struct dsa_port *dp;
> +
> mutex_init(&dev->ptp_mutex);
>
> /* Enable BroadSync HD for all ports */
> @@ -191,6 +244,9 @@ int b53_ptp_init(struct b53_device *dev)
>
> ptp_schedule_worker(dev->ptp_clock, 0);
>
> + dsa_switch_for_each_port(dp, dev->ds)
> + dp->priv = &dev->ports[dp->index].port_hwtstamp;
> +
> return 0;
> }
> EXPORT_SYMBOL(b53_ptp_init);
> diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
> index 3b3437870c55..f888f0a2022a 100644
> --- a/drivers/net/dsa/b53/b53_ptp.h
> +++ b/drivers/net/dsa/b53/b53_ptp.h
> @@ -10,6 +10,7 @@
> #include "b53_priv.h"
>
> #define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> +#define TX_TSTAMP_TIMEOUT msecs_to_jiffies(40)
>
> #ifdef CONFIG_B53_PTP
> int b53_ptp_init(struct b53_device *dev);
> @@ -18,6 +19,8 @@ int b53_get_ts_info(struct dsa_switch *ds, int port,
> struct ethtool_ts_info *info);
> bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> unsigned int type);
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
> +
> #else /* !CONFIG_B53_PTP */
>
> static inline int b53_ptp_init(struct b53_device *dev)
> @@ -41,5 +44,10 @@ static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
> return false;
> }
>
> +static inline void b53_port_txtstamp(struct dsa_switch *ds, int port,
> + struct sk_buff *skb)
> +{
> +}
> +
> #endif
> #endif
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index d611c1073deb..a44ac81fa097 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -8,6 +8,7 @@
> #include <linux/dsa/brcm.h>
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> +#include <linux/ptp_classify.h>
> #include <linux/slab.h>
> #include <linux/dsa/b53.h>
>
> @@ -76,6 +77,7 @@
> #define BRCM_EG_TC_SHIFT 5
> #define BRCM_EG_TC_MASK 0x7
> #define BRCM_EG_PID_MASK 0x1f
> +#define BRCM_EG_T_R 0x20
>
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM) || \
> IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> @@ -85,6 +87,8 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> unsigned int offset)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> + unsigned int type = ptp_classify_raw(skb);
> + struct b53_port_hwtstamp *ps = dp->priv;
> u16 queue = skb_get_queue_mapping(skb);
> u8 *brcm_tag;
>
> @@ -112,7 +116,13 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> */
> brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
> ((queue & BRCM_IG_TC_MASK) << BRCM_IG_TC_SHIFT);
> +
> brcm_tag[1] = 0;
> +
> + if (type == PTP_CLASS_V2_L2 &&
> + test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
> + brcm_tag[1] = 1 << BRCM_IG_TS_SHIFT;
> +
> brcm_tag[2] = 0;
> if (dp->index == 8)
> brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
> @@ -126,6 +136,33 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> return skb;
> }
>
> +static int set_txtstamp(struct dsa_port *dp,
> + int port,
> + u64 ns)
> +{
> + struct b53_device *b53_dev = dp->ds->priv;
> + struct b53_port_hwtstamp *ps = dp->priv;
> + struct skb_shared_hwtstamps shhwtstamps;
> + struct sk_buff *tmp_skb;
> +
> + if (!ps->tx_skb)
> + return 0;
> +
> + mutex_lock(&b53_dev->ptp_mutex);
> + ns = timecounter_cyc2time(&b53_dev->tc, ns);
> + mutex_unlock(&b53_dev->ptp_mutex);

This is called from brcm_tag_rcv_ll which runs in softirq context.
You can't take a sleeping mutex, sorry.
Please test your patches with CONFIG_DEBUG_ATOMIC_SLEEP and friends
(lockdep etc).

> +
> + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> + shhwtstamps.hwtstamp = ns_to_ktime(ns);
> + tmp_skb = ps->tx_skb;
> + ps->tx_skb = NULL;
> +
> + clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> + skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +
> + return 0;
> +}
> +
> /* Frames with this tag have one of these two layouts:
> * -----------------------------------
> * | MAC DA | MAC SA | 4b tag | Type | DSA_TAG_PROTO_BRCM
> @@ -143,6 +180,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> unsigned int offset,
> int *tag_len)
> {
> + struct dsa_port *dp;
> int source_port;
> u8 *brcm_tag;
> u32 tstamp;
> @@ -177,6 +215,14 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> if (!skb->dev)
> return NULL;
>
> + /* Check whether this is a status frame */
> + if (unlikely(*tag_len == 8 && brcm_tag[3] & BRCM_EG_T_R)) {
> + dp = dsa_slave_to_port(skb->dev);
> +
> + set_txtstamp(dp, source_port, tstamp);
> + return NULL;
> + }
> +
> /* Remove Broadcom tag and update checksum */
> skb_pull_rcsum(skb, *tag_len);
>
> --
> 2.20.1
>

2021-11-09 22:42:19

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

On 11/9/21 10:05 AM, Florian Fainelli wrote:
> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
>> include/linux/dsa/b53.h.
>>
>> Signed-off-by: Martin Kaistra <[email protected]>
>> ---
>> drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
>> include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
>> 2 files changed, 101 insertions(+), 89 deletions(-)
>> create mode 100644 include/linux/dsa/b53.h
>
> All you really access is the b53_port_hwtstamp structure within the
> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.

You do access b53_dev in the TX part, still, I would like to find a more
elegant solution to exposing everything here, can you create a
b53_timecounter_cyc2time() function that is exported to modules but does
not require exposing the b53_device to net/dsa/tag_brcm.c?
--
Florian

2021-11-10 00:11:34

[permalink] [raw]

Subject: Re: [PATCH v2 4/7] net: dsa: b53: Add PHC clock support

On 11/9/21 1:50 AM, Martin Kaistra wrote:
> The BCM53128 switch has an internal clock, which can be used for
> timestamping. Add support for it.
>
> The 32-bit free running clock counts nanoseconds. In order to account
> for the wrap-around at 999999999 (0x3B9AC9FF) while using the cycle
> counter infrastructure, we need to set a 30bit mask and use the
> overflow_point property.
>
> Enable the Broadsync HD timestamping feature in b53_ptp_init() for PTPv2
> Ethertype (0x88f7).
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---

[snip]

> struct b53_mib_desc {
> u8 size;
> @@ -1131,12 +1132,24 @@ static int b53_setup(struct dsa_switch *ds)
> b53_disable_port(ds, port);
> }
>
> + if (dev->broadsync_hd) {
> + ret = b53_ptp_init(dev);
> + if (ret) {
> + dev_err(ds->dev, "failed to initialize PTP\n");
> + return ret;
> + }

Can you fold the check for dev->broadsync_hd within b53_ptp_init() as
requested before? And likewise for b53_ptp_exit.
--
Florian

2021-11-10 00:11:37

[permalink] [raw]

Subject: Re: [PATCH v2 0/7] Add PTP support for BCM53128 switch

On 11/9/21 3:13 AM, Martin Kaistra wrote:
>
>
> Am 09.11.21 um 11:39 schrieb Vladimir Oltean:
>> On Tue, Nov 09, 2021 at 10:50:02AM +0100, Martin Kaistra wrote:
>>> Ideally, for the B53=m case, I would have liked to include the PTP
>>> support in the b53_module itself, however I couldn't find a way to do
>>> that without renaming either the common source file or the module, which
>>> I didn't want to do.
>>>
>>> Instead, b53_ptp will be allowed as a loadable module, but only if
>>> b53_common is also a module, otherwise it will be built-in.
>>
>> Does this not work?
>>
>> obj-$(CONFIG_B53) += b53_common.o
>>
>> ifdef CONFIG_B53_PTP
>> b53_common-objs += b53_ptp.o
>> endif
>>
>> (haven't tried though)
>>
>
> I get:
>
> arm-linux-gnueabihf-ld -EL -r -o drivers/net/dsa/b53/b53_common.o
> drivers/net/dsa/b53/b53_ptp.o
>
>
>
> and
>
>
>
> ERROR: modpost: "b53_switch_register" [drivers/net/dsa/b53/b53_mdio.ko]
> undefined!
>
> ERROR: modpost: "b53_switch_alloc" [drivers/net/dsa/b53/b53_mdio.ko]
> undefined!
>
> ERROR: modpost: "b53_switch_register" [drivers/net/dsa/b53/b53_spi.ko]
> undefined!
>
> ERROR: modpost: "b53_switch_alloc" [drivers/net/dsa/b53/b53_spi.ko]
> undefined!
>
> It seems to me, that b53_common.c does not get included at all.

You need to play tricks with '-' and '_' and do something like this:

obj-$(CONFIG_B53) += b53-common.o

b53-common-objs += b53_common.o b53_ptp.o

and that should result in a b53-common.ko which would be accepted by
modprobe b53_common or b53-common AFAICT.
--
Florian

2021-11-10 00:11:45

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <[email protected]> wrote:
>
> On 11/9/21 10:05 AM, Florian Fainelli wrote:
> > On 11/9/21 1:50 AM, Martin Kaistra wrote:
> >> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> >> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> >> include/linux/dsa/b53.h.
> >>
> >> Signed-off-by: Martin Kaistra <[email protected]>
> >> ---
> >> drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
> >> include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
> >> 2 files changed, 101 insertions(+), 89 deletions(-)
> >> create mode 100644 include/linux/dsa/b53.h
> >
> > All you really access is the b53_port_hwtstamp structure within the
> > tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
>
> You do access b53_dev in the TX part, still, I would like to find a more
> elegant solution to exposing everything here, can you create a
> b53_timecounter_cyc2time() function that is exported to modules but does
> not require exposing the b53_device to net/dsa/tag_brcm.c?
> --
> Florian

Switch drivers can't export symbols to tagging protocol drivers, remember?
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

2021-11-10 00:11:47

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

On 11/9/21 1:50 AM, Martin Kaistra wrote:
> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> include/linux/dsa/b53.h.
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---
> drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
> include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+), 89 deletions(-)
> create mode 100644 include/linux/dsa/b53.h

All you really access is the b53_port_hwtstamp structure within the
tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
--
Florian

2021-11-10 00:11:55

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] net: dsa: b53: Add BroadSync HD register definitions

On 11/9/21 1:50 AM, Martin Kaistra wrote:
> From: Kurt Kanzenbach <[email protected]>
>
> Add register definitions for the BroadSync HD features of
> BCM53128. These will be used to enable PTP support.
>
> Signed-off-by: Kurt Kanzenbach <[email protected]>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---

[snip]

> +/*************************************************************************
> + * ARL Control Registers
> + *************************************************************************/
> +
> +/* Multiport Control Register (16 bit) */
> +#define B53_MPORT_CTRL 0x0e
> +#define MPORT_CTRL_DIS_FORWARD 0
> +#define MPORT_CTRL_CMP_ETYPE 1
> +#define MPORT_CTRL_CMP_ADDR 2
> +#define MPORT_CTRL_CMP_ADDR_ETYPE 3
> +#define MPORT_CTRL_SHIFT(x) ((x) << 1)
> +#define MPORT_CTRL_MASK 0x2

The mask should be 0x3 since this is a 2-bit wide field.
--
Florian

2021-11-10 00:12:07

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

On 11/9/21 10:15 AM, Vladimir Oltean wrote:
> On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <[email protected]> wrote:
>>
>> On 11/9/21 10:05 AM, Florian Fainelli wrote:
>>> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>>>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
>>>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
>>>> include/linux/dsa/b53.h.
>>>>
>>>> Signed-off-by: Martin Kaistra <[email protected]>
>>>> ---
>>>> drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
>>>> include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
>>>> 2 files changed, 101 insertions(+), 89 deletions(-)
>>>> create mode 100644 include/linux/dsa/b53.h
>>>
>>> All you really access is the b53_port_hwtstamp structure within the
>>> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
>>
>> You do access b53_dev in the TX part, still, I would like to find a more
>> elegant solution to exposing everything here, can you create a
>> b53_timecounter_cyc2time() function that is exported to modules but does
>> not require exposing the b53_device to net/dsa/tag_brcm.c?
>> --
>> Florian
>
> Switch drivers can't export symbols to tagging protocol drivers, remember?
> https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

I do now :) How about a function pointer in dsa_switch_ops that driver
can hook onto?
--
Florian

2021-11-10 00:12:10

[permalink] [raw]

Subject: Re: [PATCH v2 5/7] net: dsa: b53: Add logic for RX timestamping

On 11/9/21 1:50 AM, Martin Kaistra wrote:
> Packets received by the tagger with opcode=1 contain the 32-bit timestamp
> according to the timebase register. This timestamp is saved in
> BRCM_SKB_CB(skb)->meta_tstamp. b53_port_rxtstamp() takes this
> and puts the full time information from the timecounter into
> shwt->hwtstamp.
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---
> drivers/net/dsa/b53/b53_common.c | 1 +
> drivers/net/dsa/b53/b53_ptp.c | 28 +++++++++++++++++++++++++
> drivers/net/dsa/b53/b53_ptp.h | 10 +++++++++
> include/linux/dsa/b53.h | 30 +++++++++++++++++++++++++++
> net/dsa/tag_brcm.c | 35 ++++++++++++++++++++++++--------
> 5 files changed, 95 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index ed590efbd3bf..a9408f9cd414 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2300,6 +2300,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
> .port_max_mtu = b53_get_max_mtu,
> .port_change_mtu = b53_change_mtu,
> .get_ts_info = b53_get_ts_info,
> + .port_rxtstamp = b53_port_rxtstamp,
> };
>
> struct b53_chip_data {
> diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
> index 8629c510b1a0..f8dd8d484d93 100644
> --- a/drivers/net/dsa/b53/b53_ptp.c
> +++ b/drivers/net/dsa/b53/b53_ptp.c
> @@ -6,6 +6,8 @@
> * Copyright (C) 2021 Linutronix GmbH
> */
>
> +#include <linux/ptp_classify.h>
> +
> #include "b53_priv.h"
> #include "b53_ptp.h"
>
> @@ -106,6 +108,32 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
> return B53_PTP_OVERFLOW_PERIOD;
> }
>
> +bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> + unsigned int type)
> +{
> + struct b53_device *dev = ds->priv;
> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> + struct skb_shared_hwtstamps *shwt;
> + u64 ns;

I had asked you to store b53_port_hwtstamp into dp->priv, any reason for
not doing that?

> +
> + if (type != PTP_CLASS_V2_L2)
> + return false;
> +
> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> + return false;
> +
> + mutex_lock(&dev->ptp_mutex);
> + ns = timecounter_cyc2time(&dev->tc, BRCM_SKB_CB(skb)->meta_tstamp);
> + mutex_unlock(&dev->ptp_mutex);
> +
> + shwt = skb_hwtstamps(skb);
> + memset(shwt, 0, sizeof(*shwt));
> + shwt->hwtstamp = ns_to_ktime(ns);
> +
> + return false;
> +}
> +EXPORT_SYMBOL(b53_port_rxtstamp);
> +
> int b53_ptp_init(struct b53_device *dev)
> {
> mutex_init(&dev->ptp_mutex);
> diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
> index 5cd2fd9621a2..3b3437870c55 100644
> --- a/drivers/net/dsa/b53/b53_ptp.h
> +++ b/drivers/net/dsa/b53/b53_ptp.h
> @@ -9,11 +9,15 @@
>
> #include "b53_priv.h"
>
> +#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> +
> #ifdef CONFIG_B53_PTP
> int b53_ptp_init(struct b53_device *dev);
> void b53_ptp_exit(struct b53_device *dev);
> int b53_get_ts_info(struct dsa_switch *ds, int port,
> struct ethtool_ts_info *info);
> +bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> + unsigned int type);
> #else /* !CONFIG_B53_PTP */
>
> static inline int b53_ptp_init(struct b53_device *dev)
> @@ -31,5 +35,11 @@ static inline int b53_get_ts_info(struct dsa_switch *ds, int port,
> return -EOPNOTSUPP;
> }
>
> +static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
> + struct sk_buff *skb, unsigned int type)
> +{
> + return false;
> +}
> +
> #endif
> #endif
> diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
> index 85aa6d9dc53d..542e5e3040d6 100644
> --- a/include/linux/dsa/b53.h
> +++ b/include/linux/dsa/b53.h
> @@ -46,9 +46,32 @@ struct b53_io_ops {
> struct phylink_link_state *state);
> };
>
> +/* state flags for b53_port_hwtstamp::state */
> +enum {
> + B53_HWTSTAMP_ENABLED,
> + B53_HWTSTAMP_TX_IN_PROGRESS,
> +};
> +
> +struct b53_port_hwtstamp {
> + /* Port index */
> + int port_id;

unsigned int;

> +
> + /* Timestamping state */
> + unsigned long state;
> +
> + /* Resources for transmit timestamping */
> + unsigned long tx_tstamp_start;
> + struct sk_buff *tx_skb;
> +
> + /* Current timestamp configuration */
> + struct hwtstamp_config tstamp_config;
> +};
> +
> struct b53_port {
> u16 vlan_ctl_mask;
> struct ethtool_eee eee;
> + /* Per-port timestamping resources */
> + struct b53_port_hwtstamp port_hwtstamp;
> };
>
> struct b53_vlan {
> @@ -112,3 +135,10 @@ struct b53_device {
> #define B53_PTP_OVERFLOW_PERIOD (HZ / 2)
> struct delayed_work overflow_work;
> };
> +
> +struct brcm_skb_cb {
> + struct sk_buff *clone;
> + u32 meta_tstamp;
> +};
> +
> +#define BRCM_SKB_CB(skb) ((struct brcm_skb_cb *)(skb)->cb)
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index 96dbb8ee2fee..d611c1073deb 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -9,6 +9,7 @@
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> #include <linux/slab.h>
> +#include <linux/dsa/b53.h>
>
> #include "dsa_priv.h"
>
> @@ -31,7 +32,10 @@
> /* 6th byte in the tag */
> #define BRCM_LEG_PORT_ID (0xf)
>
> -/* Newer Broadcom tag (4 bytes) */
> +/* Newer Broadcom tag (4 bytes)
> + * For egress, when opcode = 0001, additional 4 bytes are used for
> + * the time stamp.
> + */
> #define BRCM_TAG_LEN 4
>
> /* Tag is constructed and desconstructed using byte by byte access
> @@ -136,19 +140,29 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> */
> static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> struct net_device *dev,
> - unsigned int offset)
> + unsigned int offset,
> + int *tag_len)

unsigned int tag_len.
--
Florian

2021-11-10 00:13:52

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

On Tue, Nov 09, 2021 at 10:20:25AM -0800, Florian Fainelli wrote:
> On 11/9/21 10:15 AM, Vladimir Oltean wrote:
> > On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <[email protected]> wrote:
> >>
> >> On 11/9/21 10:05 AM, Florian Fainelli wrote:
> >>> On 11/9/21 1:50 AM, Martin Kaistra wrote:
> >>>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> >>>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> >>>> include/linux/dsa/b53.h.
> >>>>
> >>>> Signed-off-by: Martin Kaistra <[email protected]>
> >>>> ---
> >>>> drivers/net/dsa/b53/b53_priv.h | 90 +----------------------------
> >>>> include/linux/dsa/b53.h | 100 +++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 101 insertions(+), 89 deletions(-)
> >>>> create mode 100644 include/linux/dsa/b53.h
> >>>
> >>> All you really access is the b53_port_hwtstamp structure within the
> >>> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
> >>
> >> You do access b53_dev in the TX part, still, I would like to find a more
> >> elegant solution to exposing everything here, can you create a
> >> b53_timecounter_cyc2time() function that is exported to modules but does
> >> not require exposing the b53_device to net/dsa/tag_brcm.c?
> >> --
> >> Florian
> >
> > Switch drivers can't export symbols to tagging protocol drivers, remember?
> > https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
>
> I do now :) How about a function pointer in dsa_switch_ops that driver
> can hook onto?

IMHO, the honest answer to this is to admit that it's not quite okay to
timestamp a single packet at a time and simply not timestamp any packet
that might be concurrent with that, no warning or questions asked. We
should queue that second packet if it cannot be marked for timestamping
right away.

Which brings me to my second point, there used to be a generic deferred
xmit mechanism in DSA that also happened to solve this problem because
yes, there was a function pointer in dsa_switch_ops for it.
But you and Richard didn't quite like it, so I removed it.
https://patchwork.ozlabs.org/cover/1215617/

2021-11-10 07:21:21

by Kurt Kanzenbach

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

Hi Vladimir,

On Tue Nov 09 2021, Vladimir Oltean wrote:
>> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
>> +{
>> + struct b53_device *dev = ds->priv;
>> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
>> + struct sk_buff *clone;
>> + unsigned int type;
>> +
>> + type = ptp_classify_raw(skb);
>> +
>> + if (type != PTP_CLASS_V2_L2)
>> + return;
>> +
>> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
>> + return;
>> +
>> + clone = skb_clone_sk(skb);
>> + if (!clone)
>> + return;
>> +
>> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
>
> Is it ok if you simply don't timestamp a second skb which may be sent
> while the first one is in flight, I wonder? What PTP profiles have you
> tested with? At just one PTP packet at a time, the switch isn't giving
> you a lot.

PTP only generates a couple of messages per second which need to be
timestamped. Therefore, this behavior shouldn't be a problem.

hellcreek (and mv88e6xxx) do the same thing, simply because the device
can only hold only one Tx timestamp. If we'd allow more than one PTP
packet in flight, there will be correlation problems. I've tested with
default and gPTP profile without any problems. What PTP profiles do have
in mind?

> Is it a hardware limitation?

Not for the b53. It will generate status frames for each to be
timestamped packet. However, I don't see the need to allow more than one
Tx packet per port to be timestamped at the moment.

Thanks,
Kurt

Attachments:

signature.asc (877.00 B)

2021-11-10 08:21:47

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] net: dsa: b53: Add BroadSync HD register definitions

Am 09.11.21 um 19:04 schrieb Florian Fainelli:
> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>> From: Kurt Kanzenbach <[email protected]>
>>
>> Add register definitions for the BroadSync HD features of
>> BCM53128. These will be used to enable PTP support.
>>
>> Signed-off-by: Kurt Kanzenbach <[email protected]>
>> Signed-off-by: Martin Kaistra <[email protected]>
>> ---
>
> [snip]
>
>> +/*************************************************************************
>> + * ARL Control Registers
>> + *************************************************************************/
>> +
>> +/* Multiport Control Register (16 bit) */
>> +#define B53_MPORT_CTRL 0x0e
>> +#define MPORT_CTRL_DIS_FORWARD 0
>> +#define MPORT_CTRL_CMP_ETYPE 1
>> +#define MPORT_CTRL_CMP_ADDR 2
>> +#define MPORT_CTRL_CMP_ADDR_ETYPE 3
>> +#define MPORT_CTRL_SHIFT(x) ((x) << 1)
>> +#define MPORT_CTRL_MASK 0x2
>
> The mask should be 0x3 since this is a 2-bit wide field.
>

Correct, thanks.
Currently, this mask is not used, as I am just writing
MPORT0_TS_EN |
(MPORT_CTRL_CMP_ETYPE << MPORT_CTRL_SHIFT(0))
to the register. Should I keep the definition anyway?

2021-11-10 08:45:54

[permalink] [raw]

Subject: Re: [PATCH v2 5/7] net: dsa: b53: Add logic for RX timestamping

Am 09.11.21 um 19:07 schrieb Florian Fainelli:
> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>> Packets received by the tagger with opcode=1 contain the 32-bit timestamp
>> according to the timebase register. This timestamp is saved in
>> BRCM_SKB_CB(skb)->meta_tstamp. b53_port_rxtstamp() takes this
>> and puts the full time information from the timecounter into
>> shwt->hwtstamp.
>>
>> Signed-off-by: Martin Kaistra <[email protected]>
>> ---
>> drivers/net/dsa/b53/b53_common.c | 1 +
>> drivers/net/dsa/b53/b53_ptp.c | 28 +++++++++++++++++++++++++
>> drivers/net/dsa/b53/b53_ptp.h | 10 +++++++++
>> include/linux/dsa/b53.h | 30 +++++++++++++++++++++++++++
>> net/dsa/tag_brcm.c | 35 ++++++++++++++++++++++++--------
>> 5 files changed, 95 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index ed590efbd3bf..a9408f9cd414 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -2300,6 +2300,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
>> .port_max_mtu = b53_get_max_mtu,
>> .port_change_mtu = b53_change_mtu,
>> .get_ts_info = b53_get_ts_info,
>> + .port_rxtstamp = b53_port_rxtstamp,
>> };
>>
>> struct b53_chip_data {
>> diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
>> index 8629c510b1a0..f8dd8d484d93 100644
>> --- a/drivers/net/dsa/b53/b53_ptp.c
>> +++ b/drivers/net/dsa/b53/b53_ptp.c
>> @@ -6,6 +6,8 @@
>> * Copyright (C) 2021 Linutronix GmbH
>> */
>>
>> +#include <linux/ptp_classify.h>
>> +
>> #include "b53_priv.h"
>> #include "b53_ptp.h"
>>
>> @@ -106,6 +108,32 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
>> return B53_PTP_OVERFLOW_PERIOD;
>> }
>>
>> +bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
>> + unsigned int type)
>> +{
>> + struct b53_device *dev = ds->priv;
>> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
>> + struct skb_shared_hwtstamps *shwt;
>> + u64 ns;
>
> I had asked you to store b53_port_hwtstamp into dp->priv, any reason for
> not doing that?

I am sorry, I must have misunderstood you. tag_brcm.c ist now accessing
b53_port_hwtstamp via dp->priv (like in the sja1105 driver). It should
also be possible, to store it only in there.

>
>> +
>> + if (type != PTP_CLASS_V2_L2)
>> + return false;
>> +
>> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
>> + return false;
>> +
>> + mutex_lock(&dev->ptp_mutex);
>> + ns = timecounter_cyc2time(&dev->tc, BRCM_SKB_CB(skb)->meta_tstamp);
>> + mutex_unlock(&dev->ptp_mutex);
>> +
>> + shwt = skb_hwtstamps(skb);
>> + memset(shwt, 0, sizeof(*shwt));
>> + shwt->hwtstamp = ns_to_ktime(ns);
>> +
>> + return false;
>> +}
>> +EXPORT_SYMBOL(b53_port_rxtstamp);
>> +
>> int b53_ptp_init(struct b53_device *dev)
>> {
>> mutex_init(&dev->ptp_mutex);
>> diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
>> index 5cd2fd9621a2..3b3437870c55 100644
>> --- a/drivers/net/dsa/b53/b53_ptp.h
>> +++ b/drivers/net/dsa/b53/b53_ptp.h
>> @@ -9,11 +9,15 @@
>>
>> #include "b53_priv.h"
>>
>> +#define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
>> +
>> #ifdef CONFIG_B53_PTP
>> int b53_ptp_init(struct b53_device *dev);
>> void b53_ptp_exit(struct b53_device *dev);
>> int b53_get_ts_info(struct dsa_switch *ds, int port,
>> struct ethtool_ts_info *info);
>> +bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
>> + unsigned int type);
>> #else /* !CONFIG_B53_PTP */
>>
>> static inline int b53_ptp_init(struct b53_device *dev)
>> @@ -31,5 +35,11 @@ static inline int b53_get_ts_info(struct dsa_switch *ds, int port,
>> return -EOPNOTSUPP;
>> }
>>
>> +static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
>> + struct sk_buff *skb, unsigned int type)
>> +{
>> + return false;
>> +}
>> +
>> #endif
>> #endif
>> diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
>> index 85aa6d9dc53d..542e5e3040d6 100644
>> --- a/include/linux/dsa/b53.h
>> +++ b/include/linux/dsa/b53.h
>> @@ -46,9 +46,32 @@ struct b53_io_ops {
>> struct phylink_link_state *state);
>> };
>>
>> +/* state flags for b53_port_hwtstamp::state */
>> +enum {
>> + B53_HWTSTAMP_ENABLED,
>> + B53_HWTSTAMP_TX_IN_PROGRESS,
>> +};
>> +
>> +struct b53_port_hwtstamp {
>> + /* Port index */
>> + int port_id;
>
> unsigned int;
>
>> +
>> + /* Timestamping state */
>> + unsigned long state;
>> +
>> + /* Resources for transmit timestamping */
>> + unsigned long tx_tstamp_start;
>> + struct sk_buff *tx_skb;
>> +
>> + /* Current timestamp configuration */
>> + struct hwtstamp_config tstamp_config;
>> +};
>> +
>> struct b53_port {
>> u16 vlan_ctl_mask;
>> struct ethtool_eee eee;
>> + /* Per-port timestamping resources */
>> + struct b53_port_hwtstamp port_hwtstamp;
>> };
>>
>> struct b53_vlan {
>> @@ -112,3 +135,10 @@ struct b53_device {
>> #define B53_PTP_OVERFLOW_PERIOD (HZ / 2)
>> struct delayed_work overflow_work;
>> };
>> +
>> +struct brcm_skb_cb {
>> + struct sk_buff *clone;
>> + u32 meta_tstamp;
>> +};
>> +
>> +#define BRCM_SKB_CB(skb) ((struct brcm_skb_cb *)(skb)->cb)
>> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
>> index 96dbb8ee2fee..d611c1073deb 100644
>> --- a/net/dsa/tag_brcm.c
>> +++ b/net/dsa/tag_brcm.c
>> @@ -9,6 +9,7 @@
>> #include <linux/etherdevice.h>
>> #include <linux/list.h>
>> #include <linux/slab.h>
>> +#include <linux/dsa/b53.h>
>>
>> #include "dsa_priv.h"
>>
>> @@ -31,7 +32,10 @@
>> /* 6th byte in the tag */
>> #define BRCM_LEG_PORT_ID (0xf)
>>
>> -/* Newer Broadcom tag (4 bytes) */
>> +/* Newer Broadcom tag (4 bytes)
>> + * For egress, when opcode = 0001, additional 4 bytes are used for
>> + * the time stamp.
>> + */
>> #define BRCM_TAG_LEN 4
>>
>> /* Tag is constructed and desconstructed using byte by byte access
>> @@ -136,19 +140,29 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
>> */
>> static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
>> struct net_device *dev,
>> - unsigned int offset)
>> + unsigned int offset,
>> + int *tag_len)
>
> unsigned int tag_len.
>

2021-11-10 13:01:27

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Tue, Nov 09, 2021 at 10:50:08AM +0100, Martin Kaistra wrote:
> In order to get the switch to generate a timestamp for a transmitted
> packet, we need to set the TS bit in the BRCM tag. The switch will then
> create a status frame, which gets send back to the cpu.
> In b53_port_txtstamp() we put the skb into a waiting position.
>
> When a status frame is received, we extract the timestamp and put the time
> according to our timecounter into the waiting skb. When
> TX_TSTAMP_TIMEOUT is reached and we have no means to correctly get back
> a full timestamp, we cancel the process.
>
> As the status frame doesn't contain a reference to the original packet,
> only one packet with timestamp request can be sent at a time.
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---
> drivers/net/dsa/b53/b53_common.c | 1 +
> drivers/net/dsa/b53/b53_ptp.c | 56 ++++++++++++++++++++++++++++++++
> drivers/net/dsa/b53/b53_ptp.h | 8 +++++
> net/dsa/tag_brcm.c | 46 ++++++++++++++++++++++++++
> 4 files changed, 111 insertions(+)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index a9408f9cd414..56a9de89b38b 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2301,6 +2301,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
> .port_change_mtu = b53_change_mtu,
> .get_ts_info = b53_get_ts_info,
> .port_rxtstamp = b53_port_rxtstamp,
> + .port_txtstamp = b53_port_txtstamp,
> };
>
> struct b53_chip_data {
> diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
> index f8dd8d484d93..5567135ba8b9 100644
> --- a/drivers/net/dsa/b53/b53_ptp.c
> +++ b/drivers/net/dsa/b53/b53_ptp.c
> @@ -100,14 +100,65 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
> {
> struct b53_device *dev =
> container_of(ptp, struct b53_device, ptp_clock_info);
> + struct dsa_switch *ds = dev->ds;
> + int i;
>
> mutex_lock(&dev->ptp_mutex);
> timecounter_read(&dev->tc);
> mutex_unlock(&dev->ptp_mutex);
>
> + for (i = 0; i < ds->num_ports; i++) {
> + struct b53_port_hwtstamp *ps;
> +
> + if (!dsa_is_user_port(ds, i))
> + continue;
> +
> + ps = &dev->ports[i].port_hwtstamp;
> +
> + if (test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state) &&
> + time_is_before_jiffies(ps->tx_tstamp_start +
> + TX_TSTAMP_TIMEOUT)) {
> + dev_err(dev->dev,
> + "Timeout while waiting for Tx timestamp!\n");
> + dev_kfree_skb_any(ps->tx_skb);
> + ps->tx_skb = NULL;
> + clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS,
> + &ps->state);
> + }
> + }
> +
> return B53_PTP_OVERFLOW_PERIOD;
> }
>
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> +{
> + struct b53_device *dev = ds->priv;
> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> + struct sk_buff *clone;
> + unsigned int type;
> +
> + type = ptp_classify_raw(skb);
> +
> + if (type != PTP_CLASS_V2_L2)
> + return;
> +
> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> + return;
> +
> + clone = skb_clone_sk(skb);
> + if (!clone)
> + return;
> +
> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
> + kfree_skb(clone);
> + return;
> + }
> +
> + ps->tx_skb = clone;
> + ps->tx_tstamp_start = jiffies;
> +}
> +EXPORT_SYMBOL(b53_port_txtstamp);
> +
> bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> unsigned int type)
> {
> @@ -136,6 +187,8 @@ EXPORT_SYMBOL(b53_port_rxtstamp);
>
> int b53_ptp_init(struct b53_device *dev)
> {
> + struct dsa_port *dp;
> +
> mutex_init(&dev->ptp_mutex);
>
> /* Enable BroadSync HD for all ports */
> @@ -191,6 +244,9 @@ int b53_ptp_init(struct b53_device *dev)
>
> ptp_schedule_worker(dev->ptp_clock, 0);
>
> + dsa_switch_for_each_port(dp, dev->ds)
> + dp->priv = &dev->ports[dp->index].port_hwtstamp;
> +
> return 0;
> }
> EXPORT_SYMBOL(b53_ptp_init);
> diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
> index 3b3437870c55..f888f0a2022a 100644
> --- a/drivers/net/dsa/b53/b53_ptp.h
> +++ b/drivers/net/dsa/b53/b53_ptp.h
> @@ -10,6 +10,7 @@
> #include "b53_priv.h"
>
> #define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> +#define TX_TSTAMP_TIMEOUT msecs_to_jiffies(40)
>
> #ifdef CONFIG_B53_PTP
> int b53_ptp_init(struct b53_device *dev);
> @@ -18,6 +19,8 @@ int b53_get_ts_info(struct dsa_switch *ds, int port,
> struct ethtool_ts_info *info);
> bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
> unsigned int type);
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
> +
> #else /* !CONFIG_B53_PTP */
>
> static inline int b53_ptp_init(struct b53_device *dev)
> @@ -41,5 +44,10 @@ static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
> return false;
> }
>
> +static inline void b53_port_txtstamp(struct dsa_switch *ds, int port,
> + struct sk_buff *skb)
> +{
> +}
> +
> #endif
> #endif
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index d611c1073deb..a44ac81fa097 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -8,6 +8,7 @@
> #include <linux/dsa/brcm.h>
> #include <linux/etherdevice.h>
> #include <linux/list.h>
> +#include <linux/ptp_classify.h>
> #include <linux/slab.h>
> #include <linux/dsa/b53.h>
>
> @@ -76,6 +77,7 @@
> #define BRCM_EG_TC_SHIFT 5
> #define BRCM_EG_TC_MASK 0x7
> #define BRCM_EG_PID_MASK 0x1f
> +#define BRCM_EG_T_R 0x20
>
> #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM) || \
> IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> @@ -85,6 +87,8 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> unsigned int offset)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> + unsigned int type = ptp_classify_raw(skb);
> + struct b53_port_hwtstamp *ps = dp->priv;
> u16 queue = skb_get_queue_mapping(skb);
> u8 *brcm_tag;
>
> @@ -112,7 +116,13 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> */
> brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
> ((queue & BRCM_IG_TC_MASK) << BRCM_IG_TC_SHIFT);
> +
> brcm_tag[1] = 0;
> +
> + if (type == PTP_CLASS_V2_L2 &&
> + test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state))

Introducing dp->priv must be one of the most stupid things I've ever done.
I'm sorry for this, I'll try to remove it over the weekend.
Here you are dereferencing dp->priv without checking for NULL. But you
are only populating dp->priv if CONFIG_B53_PTP is enabled. So if a
"malicious" user space program sends a PTP event packet without
requesting timestamping, and PTP support is turned off, the kernel is toast.

In other news, you should not blindly timestamp any packet that looks
like PTP. You should look at (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP),
AND (this is important) whether the HWTSTAMP_TX_ON ioctl flag was passed
to _your_ driver. Otherwise, PHY timestamping will be broken and people
will wonder why (it still is, currently, because DSA doesn't call
skb_tx_timestamp(). but that is way easier to debug and fix than it
would be to get timestamps from the unintended PHC). Or even
PTP timestamping of a DSA switch cascaded beneath a b53 port, in a
disjoint tree setup. Please take a look at chapter "3.2.4 Other caveats
for MAC drivers" from Documentation/networking/timestamping.rst (a bit
ironic, I know, I've been RTFM'ed with this same file in the same
thread).

> + brcm_tag[1] = 1 << BRCM_IG_TS_SHIFT;
> +
> brcm_tag[2] = 0;
> if (dp->index == 8)
> brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
> @@ -126,6 +136,33 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> return skb;
> }
>
> +static int set_txtstamp(struct dsa_port *dp,
> + int port,
> + u64 ns)
> +{
> + struct b53_device *b53_dev = dp->ds->priv;
> + struct b53_port_hwtstamp *ps = dp->priv;
> + struct skb_shared_hwtstamps shhwtstamps;
> + struct sk_buff *tmp_skb;
> +
> + if (!ps->tx_skb)
> + return 0;
> +
> + mutex_lock(&b53_dev->ptp_mutex);
> + ns = timecounter_cyc2time(&b53_dev->tc, ns);
> + mutex_unlock(&b53_dev->ptp_mutex);
> +
> + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> + shhwtstamps.hwtstamp = ns_to_ktime(ns);
> + tmp_skb = ps->tx_skb;
> + ps->tx_skb = NULL;
> +
> + clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> + skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +
> + return 0;
> +}
> +
> /* Frames with this tag have one of these two layouts:
> * -----------------------------------
> * | MAC DA | MAC SA | 4b tag | Type | DSA_TAG_PROTO_BRCM
> @@ -143,6 +180,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> unsigned int offset,
> int *tag_len)
> {
> + struct dsa_port *dp;
> int source_port;
> u8 *brcm_tag;
> u32 tstamp;
> @@ -177,6 +215,14 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> if (!skb->dev)
> return NULL;
>
> + /* Check whether this is a status frame */
> + if (unlikely(*tag_len == 8 && brcm_tag[3] & BRCM_EG_T_R)) {
> + dp = dsa_slave_to_port(skb->dev);
> +
> + set_txtstamp(dp, source_port, tstamp);
> + return NULL;
> + }
> +
> /* Remove Broadcom tag and update checksum */
> skb_pull_rcsum(skb, *tag_len);
>
> --
> 2.20.1
>

2021-11-10 13:09:24

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

Hi Kurt,

On Wed, Nov 10, 2021 at 08:14:32AM +0100, Kurt Kanzenbach wrote:
> Hi Vladimir,
>
> On Tue Nov 09 2021, Vladimir Oltean wrote:
> >> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> >> +{
> >> + struct b53_device *dev = ds->priv;
> >> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> >> + struct sk_buff *clone;
> >> + unsigned int type;
> >> +
> >> + type = ptp_classify_raw(skb);
> >> +
> >> + if (type != PTP_CLASS_V2_L2)
> >> + return;
> >> +
> >> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> >> + return;
> >> +
> >> + clone = skb_clone_sk(skb);
> >> + if (!clone)
> >> + return;
> >> +
> >> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
> >
> > Is it ok if you simply don't timestamp a second skb which may be sent
> > while the first one is in flight, I wonder? What PTP profiles have you
> > tested with? At just one PTP packet at a time, the switch isn't giving
> > you a lot.
>
> PTP only generates a couple of messages per second which need to be
> timestamped. Therefore, this behavior shouldn't be a problem.
>
> hellcreek (and mv88e6xxx) do the same thing, simply because the device
> can only hold only one Tx timestamp. If we'd allow more than one PTP
> packet in flight, there will be correlation problems. I've tested with
> default and gPTP profile without any problems. What PTP profiles do have
> in mind?

First of all, let's separate "more than one packet in flight" at the
hardware/driver level vs user space level. Even if there is any hardware
requirement to not request TX timestamping for the 2nd frame until the
1st has been acked, that shouldn't necessarily have an implication upon
what user space sees. After all, we don't tell user space anything about
the realities of the hardware it's running on.

So it is true that ptp4l is single threaded and always polls
synchronously for the reception of a TX timestamp on the error queue
before proceeding to do anything else. But writing a kernel driver to
the specification of a single user space program is questionable.
Especially with the SOF_TIMESTAMPING_OPT_ID flag of the SO_TIMESTAMPING
socket option, it is quite possible to write a different PTP stack that
handles TX timestamps differently. It sends event messages on their
respective timer expiry (sync, peer delay request, whatever), and
processes TX timestamps as they come, asynchronously instead of blocking.
That other PTP stack would not work reliably with this driver (or with
mv88e6xxx, or with hellcreek).

> > Is it a hardware limitation?
>
> Not for the b53. It will generate status frames for each to be
> timestamped packet. However, I don't see the need to allow more than one
> Tx packet per port to be timestamped at the moment.
>
> Thanks,
> Kurt

2021-11-10 13:33:17

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Wed, Nov 10, 2021 at 03:05:45PM +0200, Vladimir Oltean wrote:
> Hi Kurt,
>
> On Wed, Nov 10, 2021 at 08:14:32AM +0100, Kurt Kanzenbach wrote:
> > Hi Vladimir,
> >
> > On Tue Nov 09 2021, Vladimir Oltean wrote:
> > >> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> > >> +{
> > >> + struct b53_device *dev = ds->priv;
> > >> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> > >> + struct sk_buff *clone;
> > >> + unsigned int type;
> > >> +
> > >> + type = ptp_classify_raw(skb);
> > >> +
> > >> + if (type != PTP_CLASS_V2_L2)
> > >> + return;
> > >> +
> > >> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> > >> + return;
> > >> +
> > >> + clone = skb_clone_sk(skb);
> > >> + if (!clone)
> > >> + return;
> > >> +
> > >> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
> > >
> > > Is it ok if you simply don't timestamp a second skb which may be sent
> > > while the first one is in flight, I wonder? What PTP profiles have you
> > > tested with? At just one PTP packet at a time, the switch isn't giving
> > > you a lot.
> >
> > PTP only generates a couple of messages per second which need to be
> > timestamped. Therefore, this behavior shouldn't be a problem.
> >
> > hellcreek (and mv88e6xxx) do the same thing, simply because the device
> > can only hold only one Tx timestamp. If we'd allow more than one PTP
> > packet in flight, there will be correlation problems. I've tested with
> > default and gPTP profile without any problems. What PTP profiles do have
> > in mind?
>
> First of all, let's separate "more than one packet in flight" at the
> hardware/driver level vs user space level. Even if there is any hardware
> requirement to not request TX timestamping for the 2nd frame until the
> 1st has been acked, that shouldn't necessarily have an implication upon
> what user space sees. After all, we don't tell user space anything about
> the realities of the hardware it's running on.
>
> So it is true that ptp4l is single threaded and always polls
> synchronously for the reception of a TX timestamp on the error queue
> before proceeding to do anything else. But writing a kernel driver to
> the specification of a single user space program is questionable.
> Especially with the SOF_TIMESTAMPING_OPT_ID flag of the SO_TIMESTAMPING
> socket option, it is quite possible to write a different PTP stack that
> handles TX timestamps differently. It sends event messages on their
> respective timer expiry (sync, peer delay request, whatever), and
> processes TX timestamps as they come, asynchronously instead of blocking.
> That other PTP stack would not work reliably with this driver (or with
> mv88e6xxx, or with hellcreek).

Another example that may be closer to you is using vclocks and multiple
ptp4l instances in multiple domains, over the same ports. Since ptp4l
doesn't claim exclusive ownership of an interface, there you don't even
need to have a different stack to see issues with the timestamps of one
ptp4l instance getting dropped just because a different one happened to
send an event message at the same time.

> > > Is it a hardware limitation?
> >
> > Not for the b53. It will generate status frames for each to be
> > timestamped packet. However, I don't see the need to allow more than one
> > Tx packet per port to be timestamped at the moment.
> >
> > Thanks,
> > Kurt
>
>

2021-11-10 13:49:43

by Kurt Kanzenbach

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Wed Nov 10 2021, Vladimir Oltean wrote:
> Hi Kurt,
>
> On Wed, Nov 10, 2021 at 08:14:32AM +0100, Kurt Kanzenbach wrote:
>> Hi Vladimir,
>>
>> On Tue Nov 09 2021, Vladimir Oltean wrote:
>> >> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
>> >> +{
>> >> + struct b53_device *dev = ds->priv;
>> >> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
>> >> + struct sk_buff *clone;
>> >> + unsigned int type;
>> >> +
>> >> + type = ptp_classify_raw(skb);
>> >> +
>> >> + if (type != PTP_CLASS_V2_L2)
>> >> + return;
>> >> +
>> >> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
>> >> + return;
>> >> +
>> >> + clone = skb_clone_sk(skb);
>> >> + if (!clone)
>> >> + return;
>> >> +
>> >> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
>> >
>> > Is it ok if you simply don't timestamp a second skb which may be sent
>> > while the first one is in flight, I wonder? What PTP profiles have you
>> > tested with? At just one PTP packet at a time, the switch isn't giving
>> > you a lot.
>>
>> PTP only generates a couple of messages per second which need to be
>> timestamped. Therefore, this behavior shouldn't be a problem.
>>
>> hellcreek (and mv88e6xxx) do the same thing, simply because the device
>> can only hold only one Tx timestamp. If we'd allow more than one PTP
>> packet in flight, there will be correlation problems. I've tested with
>> default and gPTP profile without any problems. What PTP profiles do have
>> in mind?
>
> First of all, let's separate "more than one packet in flight" at the
> hardware/driver level vs user space level. Even if there is any hardware
> requirement to not request TX timestamping for the 2nd frame until the
> 1st has been acked, that shouldn't necessarily have an implication upon
> what user space sees. After all, we don't tell user space anything about
> the realities of the hardware it's running on.

Fair enough.

>
> So it is true that ptp4l is single threaded and always polls
> synchronously for the reception of a TX timestamp on the error queue
> before proceeding to do anything else. But writing a kernel driver to
> the specification of a single user space program is questionable.
> Especially with the SOF_TIMESTAMPING_OPT_ID flag of the SO_TIMESTAMPING
> socket option, it is quite possible to write a different PTP stack that
> handles TX timestamps differently. It sends event messages on their
> respective timer expiry (sync, peer delay request, whatever), and
> processes TX timestamps as they come, asynchronously instead of blocking.
> That other PTP stack would not work reliably with this driver (or with
> mv88e6xxx, or with hellcreek).

Yeah, a PTP stack which e.g. runs delay measurements independently from
the other tasks (sync, announce, ...) may run into trouble with such as
an implementation. I'm wondering how would you solve that problem for
hardware such as hellcreek? If a packet for timestamping is "in-flight"
the Tx path for a concurrent frame would have to be delayed. That might
be a surprise to the user as well.

Thanks,
Kurt

Attachments:

signature.asc (877.00 B)

2021-11-10 14:02:34

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Wed, Nov 10, 2021 at 02:47:19PM +0100, Kurt Kanzenbach wrote:
> On Wed Nov 10 2021, Vladimir Oltean wrote:
> > Hi Kurt,
> >
> > On Wed, Nov 10, 2021 at 08:14:32AM +0100, Kurt Kanzenbach wrote:
> >> Hi Vladimir,
> >>
> >> On Tue Nov 09 2021, Vladimir Oltean wrote:
> >> >> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> >> >> +{
> >> >> + struct b53_device *dev = ds->priv;
> >> >> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> >> >> + struct sk_buff *clone;
> >> >> + unsigned int type;
> >> >> +
> >> >> + type = ptp_classify_raw(skb);
> >> >> +
> >> >> + if (type != PTP_CLASS_V2_L2)
> >> >> + return;
> >> >> +
> >> >> + if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> >> >> + return;
> >> >> +
> >> >> + clone = skb_clone_sk(skb);
> >> >> + if (!clone)
> >> >> + return;
> >> >> +
> >> >> + if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {
> >> >
> >> > Is it ok if you simply don't timestamp a second skb which may be sent
> >> > while the first one is in flight, I wonder? What PTP profiles have you
> >> > tested with? At just one PTP packet at a time, the switch isn't giving
> >> > you a lot.
> >>
> >> PTP only generates a couple of messages per second which need to be
> >> timestamped. Therefore, this behavior shouldn't be a problem.
> >>
> >> hellcreek (and mv88e6xxx) do the same thing, simply because the device
> >> can only hold only one Tx timestamp. If we'd allow more than one PTP
> >> packet in flight, there will be correlation problems. I've tested with
> >> default and gPTP profile without any problems. What PTP profiles do have
> >> in mind?
> >
> > First of all, let's separate "more than one packet in flight" at the
> > hardware/driver level vs user space level. Even if there is any hardware
> > requirement to not request TX timestamping for the 2nd frame until the
> > 1st has been acked, that shouldn't necessarily have an implication upon
> > what user space sees. After all, we don't tell user space anything about
> > the realities of the hardware it's running on.
>
> Fair enough.
>
> >
> > So it is true that ptp4l is single threaded and always polls
> > synchronously for the reception of a TX timestamp on the error queue
> > before proceeding to do anything else. But writing a kernel driver to
> > the specification of a single user space program is questionable.
> > Especially with the SOF_TIMESTAMPING_OPT_ID flag of the SO_TIMESTAMPING
> > socket option, it is quite possible to write a different PTP stack that
> > handles TX timestamps differently. It sends event messages on their
> > respective timer expiry (sync, peer delay request, whatever), and
> > processes TX timestamps as they come, asynchronously instead of blocking.
> > That other PTP stack would not work reliably with this driver (or with
> > mv88e6xxx, or with hellcreek).
>
> Yeah, a PTP stack which e.g. runs delay measurements independently from
> the other tasks (sync, announce, ...) may run into trouble with such as
> an implementation. I'm wondering how would you solve that problem for
> hardware such as hellcreek? If a packet for timestamping is "in-flight"
> the Tx path for a concurrent frame would have to be delayed. That might
> be a surprise to the user as well.

Yes, of course it would have to be delayed. But it would at least see
the timestamp, on the other hand...

Christian Eggers, while working on the mythical ksz9477 PTP support, had
the same kind of hardware to work with, and he unconditionally deferred
PTP packets, then in a kthread owned by the driver (not the tagger), he
initialized a completion structure, sent the packet, slept until the
completion was done (signaled by an irq thread in his case, can be
signaled by the tagger in Martin's case), and proceeded with the next
packet. See ksz9477_defer_xmit() here:
https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/

It might look like I'm trying really hard to sell deferred xmit here,
and that's not the case - I really hate it myself. But when putting two
and two together, it really seems to be what is needed.

2021-11-10 15:12:46

by Richard Cochran

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Wed, Nov 10, 2021 at 03:05:45PM +0200, Vladimir Oltean wrote:

> So it is true that ptp4l is single threaded and always polls
> synchronously for the reception of a TX timestamp on the error queue
> before proceeding to do anything else. But writing a kernel driver to
> the specification of a single user space program is questionable.

There are a number of HW devices on the market that only support one
outstanding Tx time stamp. The implementation of ptp4l follows this
limitation because a) it allows ptp4l to "just work" with most HW, and
b) there is as yet no practical advantage to asynchronous Tx time
stamping.

The premise of (b) might change if you had a GM serving hundreds or
thousands of unicast clients, for example.

In any case, I agree that the driver should enable the capabilities of
the HW and not impose artificial limitations.

Thanks,
Richard

2021-11-10 15:25:51