[PATCH 0/7] Add PTP support for BCM53128 switch

[PATCH 0/7] Add PTP support for BCM53128 switch

[Martin Kaistra]

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

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

[Martin Kaistra]

From: Kurt Kanzenbach <kurt@linutronix.de>

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

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
---
 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

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

[Florian Fainelli]

On 11/4/2021 6:31 AM, Martin Kaistra wrote:
> From: Kurt Kanzenbach <kurt@linutronix.de>
> 
> Add register definitions for the BroadSync HD features of
> BCM53128. These will be used to enable PTP support.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> ---
>   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

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
---
 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 <jogo@openwrt.org>
+ *
+ * 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

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
---
 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

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
---
 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 <martin.kaistra@linutronix.de>
+ * 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 <martin.kaistra@linutronix.de>
+ * 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 <jogo@openwrt.org>
+ * 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

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

[Richard Cochran]

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

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

[Richard Cochran]

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

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

[Florian Fainelli]

On 11/4/2021 6:31 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 <martin.kaistra@linutronix.de>
> ---

[snip]


> +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);

Can you do this for all enabled user ports instead of each port, that 
way it is clera that this register is supposed to be a bitmask of ports 
for which you desire PTP timestamping to be enabled?

> +
> +	/* Enable BroadSync HD Time Stamping Reporting (Egress) */
> +	b53_write8(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TS_REPORT_CTRL, 0x01);

Can you add a define for this bit in b53_regs.h and name it:

#define TSRPT_PKT_EN	BIT(0)

which will enable timestamp reporting towards the IMP port.

> +
> +	/* Enable BroadSync HD Time Stamping for PTPv2 ingress */
> +
> +	/* MPORT_CTRL0 | MPORT0_TS_EN */
> +	b53_write16(dev, B53_ARLCTRL_PAGE, 0x0e, (1 << 15) | 0x01);

Please add a definition for 0x0e which is the multi-port control 
register and is 16-bit wide.

Bit 15 is MPORT0_TS_EN and it will ensure that packets matching 
multiport 0 (address or ethertype) will be timestamped.

and then add a macro or generic definitions that are applicable to all 
multiport control registers, something like:

#define MPORT_CTRL_DIS_FORWARD	0
#define MPORT_CTRL_CMP_ADDR	1
#define MPORT_CTRL_CMP_ETYPE	2
#define MPORT_CTRL_CMP_ADDR_ETYPE 3

#define MPORT_CTRL_SHIFT(x)	((x) << 2)
#define MPORT_CTRL_MASK		0x3

> +	/* Forward to IMP port 8 */
> +	b53_write64(dev, B53_ARLCTRL_PAGE, 0x18, (1 << 8));

0x18 is the multiport vector N register so we would want a macro to 
define the multiprot vector being used (up to 6 of them), and this is a 
32-bit register, not a 64-bit register. The 8 here should be checked 
against the actual CPU port index number, it is 8 for you, it could be 5 
for someone else, or 7, even.

> +	/* PTPv2 Ether Type */
> +	b53_write64(dev, B53_ARLCTRL_PAGE, 0x10, (u64)0x88f7 << 48);

Use ETH_P_1588 here and 0x10 deserves a define which is the multiport 
address N register. Likewise, we need a base offset of 0x10 and then a 
macro to address the 6 multiports that exists.

> +
> +	/* 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;

memset the structure ahead of time so you only need explicit 
initialization where needed?

> +	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);

You are writing the default value of the register, is that of any use?
-- 
Florian

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

[Martin Kaistra]

Am 06.11.21 um 03:32 schrieb Florian Fainelli:
> 
> 
> On 11/4/2021 6:31 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 <martin.kaistra@linutronix.de>
>> ---
> 
> [snip]
> 
> 
>> +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);
> 
> Can you do this for all enabled user ports instead of each port, that 
> way it is clera that this register is supposed to be a bitmask of ports 
> for which you desire PTP timestamping to be enabled?
> 
>> +
>> +    /* Enable BroadSync HD Time Stamping Reporting (Egress) */
>> +    b53_write8(dev, B53_BROADSYNC_PAGE, B53_BROADSYNC_TS_REPORT_CTRL, 
>> 0x01);
> 
> Can you add a define for this bit in b53_regs.h and name it:
> 
> #define TSRPT_PKT_EN    BIT(0)
> 
> which will enable timestamp reporting towards the IMP port.
> 
>> +
>> +    /* Enable BroadSync HD Time Stamping for PTPv2 ingress */
>> +
>> +    /* MPORT_CTRL0 | MPORT0_TS_EN */
>> +    b53_write16(dev, B53_ARLCTRL_PAGE, 0x0e, (1 << 15) | 0x01);
> 
> Please add a definition for 0x0e which is the multi-port control 
> register and is 16-bit wide.
> 
> Bit 15 is MPORT0_TS_EN and it will ensure that packets matching 
> multiport 0 (address or ethertype) will be timestamped.
> 
> and then add a macro or generic definitions that are applicable to all 
> multiport control registers, something like:
> 
> #define MPORT_CTRL_DIS_FORWARD    0
> #define MPORT_CTRL_CMP_ADDR    1
> #define MPORT_CTRL_CMP_ETYPE    2
> #define MPORT_CTRL_CMP_ADDR_ETYPE 3
> 
> #define MPORT_CTRL_SHIFT(x)    ((x) << 2)
> #define MPORT_CTRL_MASK        0x3
> 
>> +    /* Forward to IMP port 8 */
>> +    b53_write64(dev, B53_ARLCTRL_PAGE, 0x18, (1 << 8));
> 
> 0x18 is the multiport vector N register so we would want a macro to 
> define the multiprot vector being used (up to 6 of them), and this is a 
> 32-bit register, not a 64-bit register. The 8 here should be checked 
> against the actual CPU port index number, it is 8 for you, it could be 5 
> for someone else, or 7, even.
> 
>> +    /* PTPv2 Ether Type */
>> +    b53_write64(dev, B53_ARLCTRL_PAGE, 0x10, (u64)0x88f7 << 48);
> 
> Use ETH_P_1588 here and 0x10 deserves a define which is the multiport 
> address N register. Likewise, we need a base offset of 0x10 and then a 
> macro to address the 6 multiports that exists.
> 
>> +
>> +    /* 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;
> 
> memset the structure ahead of time so you only need explicit 
> initialization where needed?
> 
>> +    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);
> 
> You are writing the default value of the register, is that of any use?

Appearently not, I just tested it without this line and it seems to work 
fine.

It just seemed strange to me, that while the datasheet mentions 40 as 
the default value, when reading the register without writing this 
initial value, I just get back 0.

I'll remove the line for v2.

Thanks,
Martin

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
---
 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

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

[Florian Fainelli]

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 <martin.kaistra@linutronix.de>
> ---
>   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

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
---
 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

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

[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 <martin.kaistra@linutronix.de>
> ---

[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

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

[Martin Kaistra]

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 <martin.kaistra@linutronix.de>
>> ---
> 
> [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

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

[Martin Kaistra]

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

Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 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

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

[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.

> +	case HWTSTAMP_FILTER_ALL:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}

Thanks,
Richard

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

[Martin Kaistra]

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
> 

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

[Richard Cochran]

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
 

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

[Richard Cochran]

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
>  

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

[Vladimir Oltean]

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.

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

[Jakub Kicinski]

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

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

[Vladimir Oltean]

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 :)

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

[Richard Cochran]

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

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

[Vladimir Oltean]

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.

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

[Richard Cochran]

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

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

[Vladimir Oltean]

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.

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

[Richard Cochran]

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

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

[Vladimir Oltean]

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.

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

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

[Richard Cochran]

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

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

[Vladimir Oltean]

On Fri, 26 Nov 2021 at 18:31, Richard Cochran <richardcochran@gmail.com> 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?

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

[Richard Cochran]

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

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

[Vladimir Oltean]

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

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

[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.

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

[Martin Kaistra]

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