Hi,
this series adds PTP support to the b53 DSA driver for the BCM53128
switch using the BroadSync HD feature.
As there seems to be only one filter (either by Ethertype or DA) for
timestamping incoming packets, only L2 is supported.
To be able to use the timecounter infrastructure with a counter that
wraps around at a non-power of two point, patch 2 adds support for such
a custom point. Alternatively I could fix up the delta every time a
wrap-around occurs in the driver itself, but this way it can also be
useful for other hardware.
Thanks,
Martin
Kurt Kanzenbach (1):
net: dsa: b53: Add BroadSync HD register definitions
Martin Kaistra (6):
net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h
timecounter: allow for non-power of two overflow
net: dsa: b53: Add PHC clock support
net: dsa: b53: Add logic for RX timestamping
net: dsa: b53: Add logic for TX timestamping
net: dsa: b53: Expose PTP timestamping ioctls to userspace
drivers/net/dsa/b53/Kconfig | 7 +
drivers/net/dsa/b53/Makefile | 1 +
drivers/net/dsa/b53/b53_common.c | 21 ++
drivers/net/dsa/b53/b53_priv.h | 90 +-------
drivers/net/dsa/b53/b53_ptp.c | 366 +++++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 68 ++++++
drivers/net/dsa/b53/b53_regs.h | 38 ++++
include/linux/dsa/b53.h | 144 ++++++++++++
include/linux/timecounter.h | 3 +
kernel/time/timecounter.c | 3 +
net/dsa/tag_brcm.c | 85 ++++++-
11 files changed, 727 insertions(+), 99 deletions(-)
create mode 100644 drivers/net/dsa/b53/b53_ptp.c
create mode 100644 drivers/net/dsa/b53/b53_ptp.h
create mode 100644 include/linux/dsa/b53.h
--
2.20.1
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 | 38 ++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index b2c539a42154..c8a9d633f78b 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
@@ -479,6 +485,38 @@
#define JMS_MIN_SIZE 1518
#define JMS_MAX_SIZE 9724
+/*************************************************************************
+ * BroadSync HD Page Registers
+ *************************************************************************/
+
+#define B53_BROADSYNC_EN_CTRL1 0x00
+#define B53_BROADSYNC_EN_CTRL2 0x01
+#define B53_BROADSYNC_TS_REPORT_CTRL 0x02
+#define B53_BROADSYNC_PCP_CTRL 0x03
+#define B53_BROADSYNC_MAX_SDU 0x04
+#define B53_BROADSYNC_TIMEBASE1 0x10
+#define B53_BROADSYNC_TIMEBASE2 0x11
+#define B53_BROADSYNC_TIMEBASE3 0x12
+#define B53_BROADSYNC_TIMEBASE4 0x13
+#define B53_BROADSYNC_TIMEBASE_ADJ1 0x14
+#define B53_BROADSYNC_TIMEBASE_ADJ2 0x15
+#define B53_BROADSYNC_TIMEBASE_ADJ3 0x16
+#define B53_BROADSYNC_TIMEBASE_ADJ4 0x17
+#define B53_BROADSYNC_SLOT_CNT1 0x18
+#define B53_BROADSYNC_SLOT_CNT2 0x19
+#define B53_BROADSYNC_SLOT_CNT3 0x1a
+#define B53_BROADSYNC_SLOT_CNT4 0x1b
+#define B53_BROADSYNC_SLOT_ADJ1 0x1c
+#define B53_BROADSYNC_SLOT_ADJ2 0x1d
+#define B53_BROADSYNC_SLOT_ADJ3 0x1e
+#define B53_BROADSYNC_SLOT_ADJ4 0x1f
+#define B53_BROADSYNC_CLS5_BW_CTRL 0x30
+#define B53_BROADSYNC_CLS4_BW_CTRL 0x60
+#define B53_BROADSYNC_EGRESS_TS 0x90
+#define B53_BROADSYNC_EGRESS_TS_STS 0xd0
+#define B53_BROADSYNC_LINK_STS1 0xe0
+#define B53_BROADSYNC_LINK_STS2 0xe1
+
/*************************************************************************
* EEE Configuration Page Registers
*************************************************************************/
--
2.20.1
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
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 | 59 ++++++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 9 +++++
net/dsa/tag_brcm.c | 51 +++++++++++++++++++++++++++
4 files changed, 120 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 86ebaa522084..7cb4d1c9d6f7 100644
--- a/drivers/net/dsa/b53/b53_ptp.c
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -109,6 +109,64 @@ static void b53_ptp_overflow_check(struct work_struct *work)
schedule_delayed_work(&dev->overflow_work, B53_PTP_OVERFLOW_PERIOD);
}
+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;
+
+ 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 -1;
+}
+
+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;
+}
+
bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
unsigned int type)
{
@@ -172,6 +230,7 @@ int b53_ptp_init(struct b53_device *dev)
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))
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
index 3b3437870c55..c76e3f4018d0 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,11 @@ static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
return false;
}
+static inline bool b53_port_txtstamp(struct dsa_switch *ds, int port,
+ struct sk_buff *skb)
+{
+ return false;
+}
+
#endif
#endif
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 85dc47c22008..53cd0345df1b 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>
@@ -85,9 +86,14 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
unsigned int offset)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
+ struct b53_device *b53_dev = dp->ds->priv;
+ unsigned int type = ptp_classify_raw(skb);
u16 queue = skb_get_queue_mapping(skb);
+ struct b53_port_hwtstamp *ps;
u8 *brcm_tag;
+ ps = &b53_dev->ports[dp->index].port_hwtstamp;
+
/* The Ethernet switch we are interfaced with needs packets to be at
* least 64 bytes (including FCS) otherwise they will be discarded when
* they enter the switch port logic. When Broadcom tags are enabled, we
@@ -112,7 +118,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 +138,32 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
return skb;
}
+static int set_txtstamp(struct b53_device *dev,
+ struct b53_port_hwtstamp *ps,
+ int port,
+ u64 ns)
+{
+ struct skb_shared_hwtstamps shhwtstamps;
+ struct sk_buff *tmp_skb;
+
+ if (!ps->tx_skb)
+ return 0;
+
+ mutex_lock(&dev->ptp_mutex);
+ ns = timecounter_cyc2time(&dev->tc, ns);
+ mutex_unlock(&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 +181,9 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
unsigned int offset,
int *tag_len)
{
+ struct b53_port_hwtstamp *ps;
+ struct b53_device *b53_dev;
+ struct dsa_port *dp;
int source_port;
u8 *brcm_tag;
u32 tstamp;
@@ -174,6 +215,16 @@ 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 (*tag_len == 8 && brcm_tag[3] & 0x20) {
+ dp = dsa_slave_to_port(skb->dev);
+ b53_dev = dp->ds->priv;
+ ps = &b53_dev->ports[source_port].port_hwtstamp;
+
+ set_txtstamp(b53_dev, ps, source_port, tstamp);
+ return NULL;
+ }
+
/* Remove Broadcom tag and update checksum */
skb_pull_rcsum(skb, *tag_len);
--
2.20.1
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
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 | 34 ++++++++++++++++++++++----------
5 files changed, 93 insertions(+), 10 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 324335465232..86ebaa522084 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"
@@ -107,6 +109,32 @@ static void b53_ptp_overflow_check(struct work_struct *work)
schedule_delayed_work(&dev->overflow_work, 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 = BRCM_SKB_CB(skb)->meta_tstamp;
+ ns = timecounter_cyc2time(&dev->tc, ns);
+ mutex_unlock(&dev->ptp_mutex);
+
+ shwt = skb_hwtstamps(skb);
+ memset(shwt, 0, sizeof(*shwt));
+ shwt->hwtstamp = ns_to_ktime(ns);
+
+ return false;
+}
+
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..85dc47c22008 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,26 @@ 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)))
+ if (unlikely(!pskb_may_pull(skb, *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) {
+ 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 +175,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 +195,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 +307,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
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 | 92 +++++++++++++++++++++++++++++++-
drivers/net/dsa/b53/b53_ptp.h | 14 +++++
3 files changed, 106 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 7cb4d1c9d6f7..a0a91134d2d8 100644
--- a/drivers/net/dsa/b53/b53_ptp.c
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -263,12 +263,100 @@ 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;
}
+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:
+ case HWTSTAMP_FILTER_PTP_V2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+ case HWTSTAMP_FILTER_ALL:
+ 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;
+}
+
+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;
+}
+
void b53_ptp_exit(struct b53_device *dev)
{
cancel_delayed_work_sync(&dev->overflow_work);
diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
index c76e3f4018d0..51146d451361 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
On Thu, Nov 04, 2021 at 02:32:01PM +0100, Martin Kaistra wrote:
> +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:
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
This is incorrect. HWTSTAMP_FILTER_PTP_V2_EVENT includes support for
UDP/IPv4 and UDP/IPv6. Driver should return error here.
> + case HWTSTAMP_FILTER_ALL:
> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> + break;
> + default:
> + return -ERANGE;
> + }
Thanks,
Richard
On Thu, 4 Nov 2021 14:31:54 +0100 Martin Kaistra wrote:
> this series adds PTP support to the b53 DSA driver for the BCM53128
> switch using the BroadSync HD feature.
>
> As there seems to be only one filter (either by Ethertype or DA) for
> timestamping incoming packets, only L2 is supported.
>
> To be able to use the timecounter infrastructure with a counter that
> wraps around at a non-power of two point, patch 2 adds support for such
> a custom point. Alternatively I could fix up the delta every time a
> wrap-around occurs in the driver itself, but this way it can also be
> useful for other hardware.
Please make sure that the code builds as a module and that each patch
compiles cleanly with W=1 C=1 flags set - build the entire tree first
with W=1 C=1 cause there will be extra warning noise, then apply your
patches one by one and recompile, there should be no warnings since b53
itself builds cleanly.
Am 04.11.21 um 18:29 schrieb Jakub Kicinski:
> On Thu, 4 Nov 2021 14:31:54 +0100 Martin Kaistra wrote:
>> this series adds PTP support to the b53 DSA driver for the BCM53128
>> switch using the BroadSync HD feature.
>>
>> As there seems to be only one filter (either by Ethertype or DA) for
>> timestamping incoming packets, only L2 is supported.
>>
>> To be able to use the timecounter infrastructure with a counter that
>> wraps around at a non-power of two point, patch 2 adds support for such
>> a custom point. Alternatively I could fix up the delta every time a
>> wrap-around occurs in the driver itself, but this way it can also be
>> useful for other hardware.
>
> Please make sure that the code builds as a module and that each patch
> compiles cleanly with W=1 C=1 flags set - build the entire tree first
> with W=1 C=1 cause there will be extra warning noise, then apply your
> patches one by one and recompile, there should be no warnings since b53
> itself builds cleanly.
>
Sorry, I will fix that.
Thanks,
Martin
On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote:
> Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from
> this list, what about HWTSTAMP_FILTER_ALL?
AKK means time stamp every received frame, so your driver should
return an error in this case as well.
Thanks,
Richard
On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote:
> On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote:
> > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from
> > this list, what about HWTSTAMP_FILTER_ALL?
>
> AKK means time stamp every received frame, so your driver should
> return an error in this case as well.
What is the expected convention exactly? There are other drivers that
downgrade the user application's request to what they support, and at
least ptp4l does not error out, it just prints a warning.
On Fri, 5 Nov 2021 16:28:33 +0200 Vladimir Oltean wrote:
> On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote:
> > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote:
> > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from
> > > this list, what about HWTSTAMP_FILTER_ALL?
> >
> > AKK means time stamp every received frame, so your driver should
> > return an error in this case as well.
>
> What is the expected convention exactly? There are other drivers that
> downgrade the user application's request to what they support, and at
> least ptp4l does not error out, it just prints a warning.
Which is sad because that's one of the best documented parts of our API:
Desired behavior is passed into the kernel and to a specific device by
calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose
ifr_data points to a struct hwtstamp_config. The tx_type and
rx_filter are hints to the driver what it is expected to do. If
the requested fine-grained filtering for incoming packets is not
supported, the driver may time stamp more than just the requested types
of packets.
Drivers are free to use a more permissive configuration than the requested
configuration. It is expected that drivers should only implement directly the
most generic mode that can be supported. For example if the hardware can
support HWTSTAMP_FILTER_V2_EVENT, then it should generally always upscale
HWTSTAMP_FILTER_V2_L2_SYNC_MESSAGE, and so forth, as HWTSTAMP_FILTER_V2_EVENT
is more generic (and more useful to applications).
A driver which supports hardware time stamping shall update the struct
with the actual, possibly more permissive configuration. If the
requested packets cannot be time stamped, then nothing should be
changed and ERANGE shall be returned (in contrast to EINVAL, which
indicates that SIOCSHWTSTAMP is not supported at all).
https://www.kernel.org/doc/html/latest/networking/timestamping.html#hardware-timestamping-configuration-siocshwtstamp-and-siocghwtstamp
Am 04.11.21 um 18:42 schrieb Richard Cochran:
> On Thu, Nov 04, 2021 at 02:32:01PM +0100, Martin Kaistra wrote:
>> +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:
>> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
>> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
>> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>
> This is incorrect. HWTSTAMP_FILTER_PTP_V2_EVENT includes support for
> UDP/IPv4 and UDP/IPv6. Driver should return error here.
Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ)
from this list, what about HWTSTAMP_FILTER_ALL?
>
>> + case HWTSTAMP_FILTER_ALL:
>> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>> + break;
>> + default:
>> + return -ERANGE;
>> + }
>
> Thanks,
> Richard
>
On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote:
> On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote:
> > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from
> > this list, what about HWTSTAMP_FILTER_ALL?
>
> AKK means time stamp every received frame, so your driver should
s/AKK/ALL/
> return an error in this case as well.
>
> Thanks,
> Richard
>
On Fri, Nov 05, 2021 at 08:09:39AM -0700, Jakub Kicinski wrote:
> On Fri, 5 Nov 2021 16:28:33 +0200 Vladimir Oltean wrote:
> > On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote:
> > > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote:
> > > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from
> > > > this list, what about HWTSTAMP_FILTER_ALL?
> > >
> > > AKK means time stamp every received frame, so your driver should
> > > return an error in this case as well.
> >
> > What is the expected convention exactly? There are other drivers that
> > downgrade the user application's request to what they support, and at
> > least ptp4l does not error out, it just prints a warning.
>
> Which is sad because that's one of the best documented parts of our API:
>
> Desired behavior is passed into the kernel and to a specific device by
> calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose
> ifr_data points to a struct hwtstamp_config. The tx_type and
> rx_filter are hints to the driver what it is expected to do. If
> the requested fine-grained filtering for incoming packets is not
> supported, the driver may time stamp more than just the requested types
> of packets.
>
> Drivers are free to use a more permissive configuration than the requested
> configuration. It is expected that drivers should only implement directly the
> most generic mode that can be supported. For example if the hardware can
> support HWTSTAMP_FILTER_V2_EVENT, then it should generally always upscale
> HWTSTAMP_FILTER_V2_L2_SYNC_MESSAGE, and so forth, as HWTSTAMP_FILTER_V2_EVENT
> is more generic (and more useful to applications).
>
> A driver which supports hardware time stamping shall update the struct
> with the actual, possibly more permissive configuration. If the
> requested packets cannot be time stamped, then nothing should be
> changed and ERANGE shall be returned (in contrast to EINVAL, which
> indicates that SIOCSHWTSTAMP is not supported at all).
>
> https://www.kernel.org/doc/html/latest/networking/timestamping.html#hardware-timestamping-configuration-siocshwtstamp-and-siocghwtstamp
Yeah, sorry, I've been all over that documentation file for the past few
days, but I missed that section. "that's one of the best documented
parts of our API" is a nice euphemism for all the SO_TIMESTAMPING flags :)
On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote:
> What is the expected convention exactly? There are other drivers that
> downgrade the user application's request to what they support, and at
> least ptp4l does not error out, it just prints a warning.
Drivers may upgrade, but they may not downgrade.
Which drivers downgrade? We need to fix those buggy drivers.
Thanks,
Richard
On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote:
> On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote:
> > What is the expected convention exactly? There are other drivers that
> > downgrade the user application's request to what they support, and at
> > least ptp4l does not error out, it just prints a warning.
>
> Drivers may upgrade, but they may not downgrade.
>
> Which drivers downgrade? We need to fix those buggy drivers.
>
> Thanks,
> Richard
Just a quick example
https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178
I haven't studied the whole tree, but I'm sure there are many more.
On 11/4/2021 6:31 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]>
> ---
> drivers/net/dsa/b53/b53_regs.h | 38 ++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
> index b2c539a42154..c8a9d633f78b 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
>
> @@ -479,6 +485,38 @@
> #define JMS_MIN_SIZE 1518
> #define JMS_MAX_SIZE 9724
>
> +/*************************************************************************
> + * BroadSync HD Page Registers
> + *************************************************************************/
> +
> +#define B53_BROADSYNC_EN_CTRL1 0x00
> +#define B53_BROADSYNC_EN_CTRL2 0x01
This is a single register which is 16-bit wide, can you also add a
comment to that extent like what is done for other register definitions?
> +#define B53_BROADSYNC_TS_REPORT_CTRL 0x02
> +#define B53_BROADSYNC_PCP_CTRL 0x03
> +#define B53_BROADSYNC_MAX_SDU 0x04
> +#define B53_BROADSYNC_TIMEBASE1 0x10
Single register which is 32-bit wide, no need to define the
TIMEBASE1..4, just call it timebase.
> +#define B53_BROADSYNC_TIMEBASE2 0x11
> +#define B53_BROADSYNC_TIMEBASE3 0x12
> +#define B53_BROADSYNC_TIMEBASE4 0x13
> +#define B53_BROADSYNC_TIMEBASE_ADJ1 0x14
Likewise.
> +#define B53_BROADSYNC_TIMEBASE_ADJ2 0x15
> +#define B53_BROADSYNC_TIMEBASE_ADJ3 0x16
> +#define B53_BROADSYNC_TIMEBASE_ADJ4 0x17
> +#define B53_BROADSYNC_SLOT_CNT1 0x18
> +#define B53_BROADSYNC_SLOT_CNT2 0x19
> +#define B53_BROADSYNC_SLOT_CNT3 0x1a > +#define B53_BROADSYNC_SLOT_CNT4 0x1b
Likewise, 32-bit register.
> +#define B53_BROADSYNC_SLOT_ADJ1 0x1c
> +#define B53_BROADSYNC_SLOT_ADJ2 0x1d
> +#define B53_BROADSYNC_SLOT_ADJ3 0x1e
> +#define B53_BROADSYNC_SLOT_ADJ4 0x1f
And likewise
> +#define B53_BROADSYNC_CLS5_BW_CTRL 0x30
> +#define B53_BROADSYNC_CLS4_BW_CTRL 0x60
> +#define B53_BROADSYNC_EGRESS_TS 0x90
> +#define B53_BROADSYNC_EGRESS_TS_STS 0xd0
> +#define B53_BROADSYNC_LINK_STS1 0xe0
> +#define B53_BROADSYNC_LINK_STS2 0xe1
Likewise this is a 16-bit register.
> +
> /*************************************************************************
> * EEE Configuration Page Registers
> *************************************************************************/
>
--
Florian
On 11/4/2021 6:31 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 | 34 ++++++++++++++++++++++----------
> 5 files changed, 93 insertions(+), 10 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 324335465232..86ebaa522084 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"
>
> @@ -107,6 +109,32 @@ static void b53_ptp_overflow_check(struct work_struct *work)
> schedule_delayed_work(&dev->overflow_work, 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 = BRCM_SKB_CB(skb)->meta_tstamp;
> + ns = timecounter_cyc2time(&dev->tc, ns);
Why not fold this directly:
ns = timecoutner_cyc2time(&dev->tc, (u64)BRCM_SKB_CB(skb)->meta_stamp)
> + mutex_unlock(&dev->ptp_mutex);
> +
> + shwt = skb_hwtstamps(skb);
> + memset(shwt, 0, sizeof(*shwt));
> + shwt->hwtstamp = ns_to_ktime(ns);
> +
> + return false;
> +}
> +
> 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..85dc47c22008 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,26 @@ 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;
I believe you have to do this in a two step process, first check that
you can pull 4 bytes, read the Broadcom tag's opcode, then pull the
additional 4 bytes if the opcode is 1.
>
> - if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
> + if (unlikely(!pskb_may_pull(skb, *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) {
> + 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 +175,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 +195,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 +307,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 = {
>
--
Florian
On 11/4/2021 6:32 AM, 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]>
> ---
[snip]
> +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;
> +
> + for (i = 0; i < ds->num_ports; i++) {
> + struct b53_port_hwtstamp *ps;
> +
> + if (!dsa_is_user_port(ds, i))
> + continue;
Can you also check on !dsa_port_is_unused()?
[snip]
> #endif
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index 85dc47c22008..53cd0345df1b 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>
>
> @@ -85,9 +86,14 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> unsigned int offset)
> {
> struct dsa_port *dp = dsa_slave_to_port(dev);
> + struct b53_device *b53_dev = dp->ds->priv;
> + unsigned int type = ptp_classify_raw(skb);
> u16 queue = skb_get_queue_mapping(skb);
> + struct b53_port_hwtstamp *ps;
> u8 *brcm_tag;
>
> + ps = &b53_dev->ports[dp->index].port_hwtstamp;
The dsa_port structure as a priv member which would be well suited to
store &b53_dev->ports[dp->index].port_hwtstamp and avoid traversing
multiple layers of objects here. You don't need to need b53_device at
all, and even if you did, you could easily add a back pointer to it in
port_hwstamp.
This applies below as well in brcm_tag_rcv_ll
[snip]
> +
> /* Frames with this tag have one of these two layouts:
> * -----------------------------------
> * | MAC DA | MAC SA | 4b tag | Type | DSA_TAG_PROTO_BRCM
> @@ -143,6 +181,9 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
> unsigned int offset,
> int *tag_len)
> {
> + struct b53_port_hwtstamp *ps;
> + struct b53_device *b53_dev;
> + struct dsa_port *dp;
> int source_port;
> u8 *brcm_tag;
> u32 tstamp;
> @@ -174,6 +215,16 @@ 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 (*tag_len == 8 && brcm_tag[3] & 0x20) {
Can we have an unlikely() here, because this is unlikely to happen
except for switches that do support PTP, and we only have 53128 so far.
Also a define for this 0x20 would be nice, it is the timestamp bit for
the packet.
--
Florian
On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote:
> On Sat, Nov 06, 2021 at 02:36:06AM +0200, Vladimir Oltean wrote:
> > On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote:
> > > On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote:
> > > > What is the expected convention exactly? There are other drivers that
> > > > downgrade the user application's request to what they support, and at
> > > > least ptp4l does not error out, it just prints a warning.
> > >
> > > Drivers may upgrade, but they may not downgrade.
> > >
> > > Which drivers downgrade? We need to fix those buggy drivers.
> > >
> > > Thanks,
> > > Richard
> >
> > Just a quick example
> > https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178
>
> switch (cfg.rx_filter) {
> case HWTSTAMP_FILTER_NONE:
> break;
> case HWTSTAMP_FILTER_ALL:
> case HWTSTAMP_FILTER_SOME:
> case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> case HWTSTAMP_FILTER_NTP_ALL:
> case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> case HWTSTAMP_FILTER_PTP_V2_EVENT:
> case HWTSTAMP_FILTER_PTP_V2_SYNC:
> case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> break;
> default:
> mutex_unlock(&ocelot->ptp_lock);
> return -ERANGE;
> }
>
> That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The
> change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple
> oversight, and the driver can be easily fixed.
>
> Thanks,
> Richard
It's essentially the same pattern as what Martin is introducing for b53.
On Sat, Nov 06, 2021 at 02:36:06AM +0200, Vladimir Oltean wrote:
> On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote:
> > On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote:
> > > What is the expected convention exactly? There are other drivers that
> > > downgrade the user application's request to what they support, and at
> > > least ptp4l does not error out, it just prints a warning.
> >
> > Drivers may upgrade, but they may not downgrade.
> >
> > Which drivers downgrade? We need to fix those buggy drivers.
> >
> > Thanks,
> > Richard
>
> Just a quick example
> https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
break;
case HWTSTAMP_FILTER_ALL:
case HWTSTAMP_FILTER_SOME:
case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
case HWTSTAMP_FILTER_NTP_ALL:
case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
case HWTSTAMP_FILTER_PTP_V2_EVENT:
case HWTSTAMP_FILTER_PTP_V2_SYNC:
case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
break;
default:
mutex_unlock(&ocelot->ptp_lock);
return -ERANGE;
}
That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The
change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple
oversight, and the driver can be easily fixed.
Thanks,
Richard
Am 06.11.21 um 03:50 schrieb Florian Fainelli:
>
>
> On 11/4/2021 6:32 AM, 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]>
>> ---
>
> [snip]
>
>> +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;
>> +
>> + for (i = 0; i < ds->num_ports; i++) {
>> + struct b53_port_hwtstamp *ps;
>> +
>> + if (!dsa_is_user_port(ds, i))
>> + continue;
>
> Can you also check on !dsa_port_is_unused()?
After the currently implemented check, dp->type should be
DSA_PORT_TYPE_USER, so it can't be DSA_PORT_TYPE_UNUSED, right?
Thanks,
Martin
On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote:
> On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote:
> > switch (cfg.rx_filter) {
> > case HWTSTAMP_FILTER_NONE:
> > break;
> > case HWTSTAMP_FILTER_ALL:
> > case HWTSTAMP_FILTER_SOME:
> > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > case HWTSTAMP_FILTER_NTP_ALL:
> > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > break;
> > default:
> > mutex_unlock(&ocelot->ptp_lock);
> > return -ERANGE;
> > }
> >
> > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The
> > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple
> > oversight, and the driver can be easily fixed.
> >
> > Thanks,
> > Richard
>
> It's essentially the same pattern as what Martin is introducing for b53.
Uh, no it isn't. The present patch has:
+ case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+ case HWTSTAMP_FILTER_PTP_V2_EVENT:
+ case HWTSTAMP_FILTER_PTP_V2_SYNC:
+ case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+ case HWTSTAMP_FILTER_ALL:
+ config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
There is an important difference between
HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT
Notice the "L2" in there.
Thanks,
Richard
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 | 7 ++
drivers/net/dsa/b53/Makefile | 1 +
drivers/net/dsa/b53/b53_common.c | 17 +++
drivers/net/dsa/b53/b53_ptp.c | 191 +++++++++++++++++++++++++++++++
drivers/net/dsa/b53/b53_ptp.h | 35 ++++++
include/linux/dsa/b53.h | 14 +++
6 files changed, 265 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..5297d73dc3ed 100644
--- a/drivers/net/dsa/b53/Kconfig
+++ b/drivers/net/dsa/b53/Kconfig
@@ -45,3 +45,10 @@ 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
+ help
+ Select to enable support for PTP
diff --git a/drivers/net/dsa/b53/Makefile b/drivers/net/dsa/b53/Makefile
index b1be13023ae4..c49783e4a459 100644
--- a/drivers/net/dsa/b53/Makefile
+++ b/drivers/net/dsa/b53/Makefile
@@ -6,3 +6,4 @@ 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
+obj-$(CONFIG_B53_PTP) += b53_ptp.o
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..324335465232
--- /dev/null
+++ b/drivers/net/dsa/b53/b53_ptp.c
@@ -0,0 +1,191 @@
+// 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_TIMEBASE1, &ts);
+
+ return ts;
+}
+
+static int b53_ptp_enable(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ return -EOPNOTSUPP;
+}
+
+static void b53_ptp_overflow_check(struct work_struct *work)
+{
+ struct delayed_work *dw = to_delayed_work(work);
+ struct b53_device *dev =
+ container_of(dw, struct b53_device, overflow_work);
+
+ mutex_lock(&dev->ptp_mutex);
+ timecounter_read(&dev->tc);
+ mutex_unlock(&dev->ptp_mutex);
+
+ schedule_delayed_work(&dev->overflow_work, B53_PTP_OVERFLOW_PERIOD);
+}
+
+int b53_ptp_init(struct b53_device *dev)
+{
+ mutex_init(&dev->ptp_mutex);
+
+ INIT_DELAYED_WORK(&dev->overflow_work, b53_ptp_overflow_check);
+
+ /* Enable BroadSync HD for all ports */
+ b53_write16(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_EN_CTRL1, 0x00ff);
+
+ /* Enable BroadSync HD Time Stamping Reporting (Egress) */
+ b53_write8(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TS_REPORT_CTRL, 0x01);
+
+ /* Enable BroadSync HD Time Stamping for PTPv2 ingress */
+
+ /* MPORT_CTRL0 | MPORT0_TS_EN */
+ b53_write16(dev, B53_ARLCTRL_PAGE, 0x0e, (1 << 15) | 0x01);
+ /* Forward to IMP port 8 */
+ b53_write64(dev, B53_ARLCTRL_PAGE, 0x18, (1 << 8));
+ /* PTPv2 Ether Type */
+ b53_write64(dev, B53_ARLCTRL_PAGE, 0x10, (u64)0x88f7 << 48);
+
+ /* Setup PTP clock */
+ 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.n_alarm = 0;
+ dev->ptp_clock_info.n_pins = 0;
+ dev->ptp_clock_info.n_ext_ts = 0;
+ dev->ptp_clock_info.n_per_out = 0;
+ dev->ptp_clock_info.pps = 0;
+ 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 = 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;
+
+ b53_write32(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TIMEBASE_ADJ1, 40);
+
+ timecounter_init(&dev->tc, &dev->cc, ktime_to_ns(ktime_get_real()));
+
+ schedule_delayed_work(&dev->overflow_work, B53_PTP_OVERFLOW_PERIOD);
+
+ return 0;
+}
+
+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;
+}
+
+void b53_ptp_exit(struct b53_device *dev)
+{
+ cancel_delayed_work_sync(&dev->overflow_work);
+ if (dev->ptp_clock)
+ ptp_clock_unregister(dev->ptp_clock);
+ dev->ptp_clock = NULL;
+}
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
On Thu, Nov 04, 2021 at 02:31:58PM +0100, Martin Kaistra wrote:
> +static void b53_ptp_overflow_check(struct work_struct *work)
> +{
> + struct delayed_work *dw = to_delayed_work(work);
> + struct b53_device *dev =
> + container_of(dw, struct b53_device, overflow_work);
> +
> + mutex_lock(&dev->ptp_mutex);
> + timecounter_read(&dev->tc);
> + mutex_unlock(&dev->ptp_mutex);
> +
> + schedule_delayed_work(&dev->overflow_work, B53_PTP_OVERFLOW_PERIOD);
> +}
> +
> +int b53_ptp_init(struct b53_device *dev)
> +{
> + mutex_init(&dev->ptp_mutex);
> +
> + INIT_DELAYED_WORK(&dev->overflow_work, b53_ptp_overflow_check);
...
> @@ -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;
Instead of generic work, consider implementing
ptp_clock_info::do_aux_work instead.
The advantage is that you get a named kernel thread that can be given
scheduling priority administratively.
Thanks,
Richard
On Thu, Nov 04, 2021 at 10:28:43AM -0700, Richard Cochran wrote:
> Instead of generic work, consider implementing
> ptp_clock_info::do_aux_work instead.
>
> The advantage is that you get a named kernel thread that can be given
> scheduling priority administratively.
I see you are using do_aux_work in Patch 6. You could use the kthread
for both overflow avoidance and transmit time stamps.
> Thanks,
> Richard
On Mon, Nov 08, 2021 at 06:48:24AM -0800, Richard Cochran wrote:
> On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote:
> > On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote:
> > > switch (cfg.rx_filter) {
> > > case HWTSTAMP_FILTER_NONE:
> > > break;
> > > case HWTSTAMP_FILTER_ALL:
> > > case HWTSTAMP_FILTER_SOME:
> > > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> > > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > > case HWTSTAMP_FILTER_NTP_ALL:
> > > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > > case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > > case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > > break;
> > > default:
> > > mutex_unlock(&ocelot->ptp_lock);
> > > return -ERANGE;
> > > }
> > >
> > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The
> > > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple
> > > oversight, and the driver can be easily fixed.
> > >
> > > Thanks,
> > > Richard
> >
> > It's essentially the same pattern as what Martin is introducing for b53.
>
> Uh, no it isn't. The present patch has:
>
> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + case HWTSTAMP_FILTER_ALL:
> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>
> There is an important difference between
> HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT
>
> Notice the "L2" in there.
Richard, when the request is PTP_V2_EVENT and the response is
PTP_V2_L2_EVENT, is that an upgrade or a downgrade? PTP_V2_EVENT also
includes PTP_V2_L4_EVENT.
On Thu Nov 25 2021, Vladimir Oltean wrote:
> On Mon, Nov 08, 2021 at 06:48:24AM -0800, Richard Cochran wrote:
>> On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote:
>> > On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote:
>> > > switch (cfg.rx_filter) {
>> > > case HWTSTAMP_FILTER_NONE:
>> > > break;
>> > > case HWTSTAMP_FILTER_ALL:
>> > > case HWTSTAMP_FILTER_SOME:
>> > > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
>> > > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
>> > > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
>> > > case HWTSTAMP_FILTER_NTP_ALL:
>> > > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>> > > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
>> > > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
>> > > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
>> > > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
>> > > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
>> > > case HWTSTAMP_FILTER_PTP_V2_EVENT:
>> > > case HWTSTAMP_FILTER_PTP_V2_SYNC:
>> > > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>> > > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> > > break;
>> > > default:
>> > > mutex_unlock(&ocelot->ptp_lock);
>> > > return -ERANGE;
>> > > }
>> > >
>> > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The
>> > > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple
>> > > oversight, and the driver can be easily fixed.
>> > >
>> > > Thanks,
>> > > Richard
>> >
>> > It's essentially the same pattern as what Martin is introducing for b53.
>>
>> Uh, no it isn't. The present patch has:
>>
>> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
>> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
>> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
>> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
>> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
>> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>> + case HWTSTAMP_FILTER_ALL:
>> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
>>
>> There is an important difference between
>> HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT
>>
>> Notice the "L2" in there.
>
> Richard, when the request is PTP_V2_EVENT and the response is
> PTP_V2_L2_EVENT, is that an upgrade or a downgrade?
It is a downgrade, isn't it?
> PTP_V2_EVENT also includes PTP_V2_L4_EVENT.
Yes, exactly.
Thanks,
Kurt
On Fri, Nov 26, 2021 at 09:42:32AM +0100, Kurt Kanzenbach wrote:
> On Thu Nov 25 2021, Vladimir Oltean wrote:
> > Richard, when the request is PTP_V2_EVENT and the response is
> > PTP_V2_L2_EVENT, is that an upgrade or a downgrade?
>
> It is a downgrade, isn't it?
Yes. "Any kind of PTP Event" is a superset of "Any Layer-2 Event".
When userland asks for "any kind", then it wants to run PTP over IPv4,
IPv6, or Layer2, maybe even more than one at the same time. If the
driver changes that to Layer2 only, then the PTP possibilities have
been downgraded.
Thanks,
Richard
On Fri, 26 Nov 2021 at 18:31, Richard Cochran <[email protected]> wrote:
>
> On Fri, Nov 26, 2021 at 09:42:32AM +0100, Kurt Kanzenbach wrote:
> > On Thu Nov 25 2021, Vladimir Oltean wrote:
> > > Richard, when the request is PTP_V2_EVENT and the response is
> > > PTP_V2_L2_EVENT, is that an upgrade or a downgrade?
> >
> > It is a downgrade, isn't it?
>
> Yes. "Any kind of PTP Event" is a superset of "Any Layer-2 Event".
>
> When userland asks for "any kind", then it wants to run PTP over IPv4,
> IPv6, or Layer2, maybe even more than one at the same time. If the
> driver changes that to Layer2 only, then the PTP possibilities have
> been downgraded.
Well, when I said that it's essentially the same pattern, this is what
I was talking about. The b53 driver downgrades everything and the
kitchen sink to HWTSTAMP_FILTER_PTP_V2_L2_EVENT, the ocelot driver to
HWTSTAMP_FILTER_PTP_V2_EVENT, and both are buggy for the same reason.
I don't see why you mention that there is an important difference
between HWTSTAMP_FILTER_PTP_V2_L2_EVENT and
HWTSTAMP_FILTER_PTP_V2_EVENT. I know there is, but the _pattern_ is
the same.
I'm still missing something obvious, aren't I?
On Fri, Nov 26, 2021 at 06:42:57PM +0200, Vladimir Oltean wrote:
> I'm still missing something obvious, aren't I?
You said there are "many more" drivers with this bug, but I'm saying
that most drivers correctly upgrade the ioctl request.
So far we have b53 and ocelot doing the buggy downgrade. I guess it
will require a tree wide audit to discover the "many more"...
Thanks,
Richard
On Fri, Nov 26, 2021 at 09:03:48AM -0800, Richard Cochran wrote:
> On Fri, Nov 26, 2021 at 06:42:57PM +0200, Vladimir Oltean wrote:
> > I'm still missing something obvious, aren't I?
>
> You said there are "many more" drivers with this bug, but I'm saying
> that most drivers correctly upgrade the ioctl request.
>
> So far we have b53 and ocelot doing the buggy downgrade. I guess it
> will require a tree wide audit to discover the "many more"...
Ah, yes, I assure you that there are many more drivers doing wacky
stuff, for example sja1105 will take any RX filter that isn't NONE, and
then reports it back as PTP_V2_L2_EVENT.
https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/sja1105/sja1105_ptp.c#L89
Somehow at this stage I don't even want to know about any other drivers,
since I might feel the urge to patch them and I don't really have the
necessary free time for that right now :D