[PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Brandon Streiff]

This patch series adds support for PTP timestamping through the DSA
framework, as well as an implementation for mv8e6xxx switches.

This implementation was targeted at a National Instruments platform
that uses the Marvell 88E6341 (Topaz). I've tried to enable support on
other Marvell switches where the register interfaces seemed compatible,
but I don't have the hardware to verify their operation myself.

This series probably ties in well with Richard's comment last week
("Re: [RFC net-next 0/5] TSN: Add qdisc-based config interfaces for
traffic shapers") about figuring out proper interfaces for managing
switch-level PTP timestamps.

A couple patches that I expect may need further polishing:

- Patch #2: We expose the switch time as a PTP clock but don't support
  adjustment (max_adj=0). Our platform adjusted a systemwide oscillator
  from userspace, so we didn't need adjustment at this layer, but other
  PTP clock drivers support this and we probably should too.

- Patch #3: The GPIO config support is handled in a very simple manner.
  I suspect a longer term goal would be to use pinctrl here.

- Patch #6: the dsa_switch pointer and port index is plumbed from
  dsa_device_ops::rcv so that we can call the correct port_rxtstamp
  method. This involved instrumenting all of the *_tag_rcv functions in
  a way that's kind of a kludge and that I'm not terribly happy with.

This applies to net-next as of 14a0d032f4ec.

Feedback is appreciated.

-- brandon


Brandon Streiff (9):
  net: dsa: mv88e6xxx: add accessors for PTP/TAI registers
  net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock
  net: dsa: mv88e6xxx: add support for GPIO configuration
  net: dsa: mv88e6xxx: add support for event capture
  net: dsa: forward hardware timestamping ioctls to switch driver
  net: dsa: forward timestamping callbacks to switch drivers
  ptp: add offset for reserved field to header
  net: dsa: mv88e6xxx: add rx/tx timestamping support
  net: dsa: mv88e6xxx: add workaround for 6341 timestamping

 drivers/net/dsa/mv88e6xxx/Kconfig    |  10 +
 drivers/net/dsa/mv88e6xxx/Makefile   |   2 +
 drivers/net/dsa/mv88e6xxx/chip.c     |  65 +++++
 drivers/net/dsa/mv88e6xxx/chip.h     |  71 +++++
 drivers/net/dsa/mv88e6xxx/global2.c  | 244 ++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global2.h  |  59 +++-
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 548 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/hwtstamp.h | 171 +++++++++++
 drivers/net/dsa/mv88e6xxx/ptp.c      | 493 +++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/ptp.h      |  99 +++++++
 include/linux/ptp_classify.h         |   1 +
 include/net/dsa.h                    |  28 +-
 net/dsa/dsa.c                        |  39 ++-
 net/dsa/slave.c                      |  67 ++++-
 net/dsa/tag_brcm.c                   |   6 +-
 net/dsa/tag_dsa.c                    |   6 +-
 net/dsa/tag_edsa.c                   |   6 +-
 net/dsa/tag_ksz.c                    |   6 +-
 net/dsa/tag_lan9303.c                |   6 +-
 net/dsa/tag_mtk.c                    |   6 +-
 net/dsa/tag_qca.c                    |   6 +-
 net/dsa/tag_trailer.c                |   6 +-
 22 files changed, 1929 insertions(+), 16 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/hwtstamp.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/hwtstamp.h
 create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.h

-- 
2.1.4

[PATCH net-next RFC 1/9] net: dsa: mv88e6xxx: add accessors for PTP/TAI registers

[Brandon Streiff]

This patch implements support for accessing PTP/TAI registers through
the AVB register interface in the Global 2 register.

The register interface differs slightly between different models; older
models use a 3-bit operations field, while newer models use a 2-bit
field. The operations values and the special "global port" values are
different between the two. This is a similar split to the differences
in the "Ingress Rate" register between models, so, like in that case,
we call the two variants "6352" and "6390" and create an ops structure
to abstract between the two.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |   9 ++
 drivers/net/dsa/mv88e6xxx/chip.h    |  17 ++++
 drivers/net/dsa/mv88e6xxx/global2.c | 173 ++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global2.h |  27 +++++-
 4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c6678aa..67efa7b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2783,6 +2783,7 @@ static const struct mv88e6xxx_ops mv88e6240_ops = {
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.serdes_power = mv88e6352_serdes_power,
+	.avb_ops = &mv88e6352_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6290_ops = {
@@ -2819,6 +2820,7 @@ static const struct mv88e6xxx_ops mv88e6290_ops = {
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
 	.serdes_power = mv88e6390_serdes_power,
+	.avb_ops = &mv88e6390_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6320_ops = {
@@ -2852,6 +2854,7 @@ static const struct mv88e6xxx_ops mv88e6320_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6185_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6185_g1_vtu_loadpurge,
+	.avb_ops = &mv88e6352_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6321_ops = {
@@ -2883,6 +2886,7 @@ static const struct mv88e6xxx_ops mv88e6321_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6185_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6185_g1_vtu_loadpurge,
+	.avb_ops = &mv88e6352_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6341_ops = {
@@ -2918,6 +2922,7 @@ static const struct mv88e6xxx_ops mv88e6341_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.avb_ops = &mv88e6390_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6350_ops = {
@@ -2984,6 +2989,7 @@ static const struct mv88e6xxx_ops mv88e6351_ops = {
 	.reset = mv88e6352_g1_reset,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
+	.avb_ops = &mv88e6352_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6352_ops = {
@@ -3020,6 +3026,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.serdes_power = mv88e6352_serdes_power,
+	.avb_ops = &mv88e6352_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6390_ops = {
@@ -3058,6 +3065,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
 	.serdes_power = mv88e6390_serdes_power,
+	.avb_ops = &mv88e6390_avb_ops,
 };
 
 static const struct mv88e6xxx_ops mv88e6390x_ops = {
@@ -3096,6 +3104,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6390_g1_vtu_loadpurge,
 	.serdes_power = mv88e6390_serdes_power,
+	.avb_ops = &mv88e6390_avb_ops,
 };
 
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 334f6f7..d99d592 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -146,6 +146,7 @@ struct mv88e6xxx_vtu_entry {
 
 struct mv88e6xxx_bus_ops;
 struct mv88e6xxx_irq_ops;
+struct mv88e6xxx_avb_ops;
 
 struct mv88e6xxx_irq {
 	u16 masked;
@@ -342,6 +343,9 @@ struct mv88e6xxx_ops {
 			   struct mv88e6xxx_vtu_entry *entry);
 	int (*vtu_loadpurge)(struct mv88e6xxx_chip *chip,
 			     struct mv88e6xxx_vtu_entry *entry);
+
+	/* Interface to the AVB/PTP registers */
+	const struct mv88e6xxx_avb_ops *avb_ops;
 };
 
 struct mv88e6xxx_irq_ops {
@@ -353,6 +357,19 @@ struct mv88e6xxx_irq_ops {
 	void (*irq_free)(struct mv88e6xxx_chip *chip);
 };
 
+struct mv88e6xxx_avb_ops {
+	int (*port_ptp_read)(struct mv88e6xxx_chip *chip, int port, int addr,
+			     u16 *data, int len);
+	int (*port_ptp_write)(struct mv88e6xxx_chip *chip, int port, int addr,
+			      u16 data);
+	int (*ptp_read)(struct mv88e6xxx_chip *chip, int addr, u16 *data,
+			int len);
+	int (*ptp_write)(struct mv88e6xxx_chip *chip, int addr, u16 data);
+	int (*tai_read)(struct mv88e6xxx_chip *chip, int addr, u16 *data,
+			int len);
+	int (*tai_write)(struct mv88e6xxx_chip *chip, int addr, u16 data);
+};
+
 #define STATS_TYPE_PORT		BIT(0)
 #define STATS_TYPE_BANK0	BIT(1)
 #define STATS_TYPE_BANK1	BIT(2)
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index af07278..b6d0c71 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -590,6 +590,179 @@ int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
 	return 0;
 }
 
+/* Offset 0x16: AVB Command Register
+ * Offset 0x17: AVB Data Register
+ *
+ * There are two different versions of this register interface:
+ *    "6352": 3-bit "op" field, 4-bit "port" field.
+ *    "6390": 2-bit "op" field, 5-bit "port" field.
+ *
+ * The "op" codes are different between the two, as well as the special
+ * port fields for global PTP and TAI configuration.
+ */
+
+/* mv88e6xxx_g2_avb_read -- Read one or multiple 16-bit words.
+ * The hardware supports snapshotting up to four contiguous registers.
+ */
+static int mv88e6xxx_g2_avb_read(struct mv88e6xxx_chip *chip, u16 readop,
+				 u16 *data, int len)
+{
+	int err;
+	int i;
+
+	/* Hardware can only snapshot four words. */
+	if (unlikely(len > 4))
+		return -E2BIG;
+
+	err = mv88e6xxx_g2_update(chip, MV88E6352_G2_AVB_CMD, readop);
+	if (err)
+		return err;
+
+	for (i = 0; i < len; ++i) {
+		err = mv88e6xxx_g2_read(chip, MV88E6352_G2_AVB_DATA,
+					&data[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+/* mv88e6xxx_g2_avb_write -- Write one 16-bit word. */
+static int mv88e6xxx_g2_avb_write(struct mv88e6xxx_chip *chip, u16 writeop,
+				  u16 data)
+{
+	int err;
+
+	err = mv88e6xxx_g2_write(chip, MV88E6352_G2_AVB_DATA, data);
+	if (err)
+		return err;
+
+	return mv88e6xxx_g2_update(chip, MV88E6352_G2_AVB_CMD, writeop);
+}
+
+static int mv88e6352_port_ptp_read(struct mv88e6xxx_chip *chip, int port,
+				   int addr, u16 *data, int len)
+{
+	u16 readop = (len == 1 ? MV88E6352_G2_AVB_CMD_OP_READ :
+				 MV88E6352_G2_AVB_CMD_OP_READ_INCR) |
+		     (port << 8) | (MV88E6352_G2_AVB_CMD_BLOCK_PTP << 5) |
+		     addr;
+
+	return mv88e6xxx_g2_avb_read(chip, readop, data, len);
+}
+
+static int mv88e6352_port_ptp_write(struct mv88e6xxx_chip *chip, int port,
+				    int addr, u16 data)
+{
+	u16 writeop = MV88E6352_G2_AVB_CMD_OP_WRITE | (port << 8) |
+		      (MV88E6352_G2_AVB_CMD_BLOCK_PTP << 5) | addr;
+
+	return mv88e6xxx_g2_avb_write(chip, writeop, data);
+}
+
+static int mv88e6352_ptp_read(struct mv88e6xxx_chip *chip, int addr,
+			      u16 *data, int len)
+{
+	return mv88e6352_port_ptp_read(chip,
+				       MV88E6352_G2_AVB_CMD_PORT_PTPGLOBAL,
+				       addr, data, len);
+}
+
+static int mv88e6352_ptp_write(struct mv88e6xxx_chip *chip, int addr,
+			       u16 data)
+{
+	return mv88e6352_port_ptp_write(chip,
+					MV88E6352_G2_AVB_CMD_PORT_PTPGLOBAL,
+					addr, data);
+}
+
+static int mv88e6352_tai_read(struct mv88e6xxx_chip *chip, int addr,
+			      u16 *data, int len)
+{
+	return mv88e6352_port_ptp_read(chip,
+				       MV88E6352_G2_AVB_CMD_PORT_TAIGLOBAL,
+				       addr, data, len);
+}
+
+static int mv88e6352_tai_write(struct mv88e6xxx_chip *chip, int addr,
+			       u16 data)
+{
+	return mv88e6352_port_ptp_write(chip,
+					MV88E6352_G2_AVB_CMD_PORT_TAIGLOBAL,
+					addr, data);
+}
+
+const struct mv88e6xxx_avb_ops mv88e6352_avb_ops = {
+	.port_ptp_read		= mv88e6352_port_ptp_read,
+	.port_ptp_write		= mv88e6352_port_ptp_write,
+	.ptp_read		= mv88e6352_ptp_read,
+	.ptp_write		= mv88e6352_ptp_write,
+	.tai_read		= mv88e6352_tai_read,
+	.tai_write		= mv88e6352_tai_write,
+};
+
+static int mv88e6390_port_ptp_read(struct mv88e6xxx_chip *chip, int port,
+				   int addr, u16 *data, int len)
+{
+	u16 readop = (len == 1 ? MV88E6390_G2_AVB_CMD_OP_READ :
+				 MV88E6390_G2_AVB_CMD_OP_READ_INCR) |
+		     (port << 8) | (MV88E6352_G2_AVB_CMD_BLOCK_PTP << 5) |
+		     addr;
+
+	return mv88e6xxx_g2_avb_read(chip, readop, data, len);
+}
+
+static int mv88e6390_port_ptp_write(struct mv88e6xxx_chip *chip, int port,
+				    int addr, u16 data)
+{
+	u16 writeop = MV88E6390_G2_AVB_CMD_OP_WRITE | (port << 8) |
+		      (MV88E6352_G2_AVB_CMD_BLOCK_PTP << 5) | addr;
+
+	return mv88e6xxx_g2_avb_write(chip, writeop, data);
+}
+
+static int mv88e6390_ptp_read(struct mv88e6xxx_chip *chip, int addr,
+			      u16 *data, int len)
+{
+	return mv88e6390_port_ptp_read(chip,
+				       MV88E6390_G2_AVB_CMD_PORT_PTPGLOBAL,
+				       addr, data, len);
+}
+
+static int mv88e6390_ptp_write(struct mv88e6xxx_chip *chip, int addr,
+			       u16 data)
+{
+	return mv88e6390_port_ptp_write(chip,
+					MV88E6390_G2_AVB_CMD_PORT_PTPGLOBAL,
+					addr, data);
+}
+
+static int mv88e6390_tai_read(struct mv88e6xxx_chip *chip, int addr,
+			      u16 *data, int len)
+{
+	return mv88e6390_port_ptp_read(chip,
+				       MV88E6390_G2_AVB_CMD_PORT_TAIGLOBAL,
+				       addr, data, len);
+}
+
+static int mv88e6390_tai_write(struct mv88e6xxx_chip *chip, int addr,
+			       u16 data)
+{
+	return mv88e6390_port_ptp_write(chip,
+					MV88E6390_G2_AVB_CMD_PORT_TAIGLOBAL,
+					addr, data);
+}
+
+const struct mv88e6xxx_avb_ops mv88e6390_avb_ops = {
+	.port_ptp_read		= mv88e6390_port_ptp_read,
+	.port_ptp_write		= mv88e6390_port_ptp_write,
+	.ptp_read		= mv88e6390_ptp_read,
+	.ptp_write		= mv88e6390_ptp_write,
+	.tai_read		= mv88e6390_tai_read,
+	.tai_write		= mv88e6390_tai_write,
+};
+
 /* Offset 0x18: SMI PHY Command Register
  * Offset 0x19: SMI PHY Data Register
  */
diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
index 669f590..642d2b0 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -149,7 +149,26 @@
 #define MV88E6390_G2_EEPROM_ADDR_MASK	0xffff
 
 /* Offset 0x16: AVB Command Register */
-#define MV88E6352_G2_AVB_CMD		0x16
+#define MV88E6352_G2_AVB_CMD			0x16
+#define MV88E6352_G2_AVB_CMD_BUSY		0x8000
+#define MV88E6352_G2_AVB_CMD_OP_READ		0x4000
+#define MV88E6352_G2_AVB_CMD_OP_READ_INCR	0x6000
+#define MV88E6352_G2_AVB_CMD_OP_WRITE		0x3000
+#define MV88E6390_G2_AVB_CMD_OP_READ		0x0000
+#define MV88E6390_G2_AVB_CMD_OP_READ_INCR	0x4000
+#define MV88E6390_G2_AVB_CMD_OP_WRITE		0x6000
+#define MV88E6352_G2_AVB_CMD_PORT_MASK		0x0f00
+#define MV88E6352_G2_AVB_CMD_PORT_TAIGLOBAL	0xe
+#define MV88E6352_G2_AVB_CMD_PORT_PTPGLOBAL	0xf
+#define MV88E6390_G2_AVB_CMD_PORT_MASK		0x1f00
+#define MV88E6390_G2_AVB_CMD_PORT_TAIGLOBAL	0x1e
+#define MV88E6390_G2_AVB_CMD_PORT_PTPGLOBAL	0x1f
+#define MV88E6352_G2_AVB_CMD_BLOCK_PTP		0
+#define MV88E6352_G2_AVB_CMD_BLOCK_AVB		1
+#define MV88E6352_G2_AVB_CMD_BLOCK_QAV		2
+#define MV88E6352_G2_AVB_CMD_BLOCK_QVB		3
+#define MV88E6352_G2_AVB_CMD_BLOCK_MASK		0x00e0
+#define MV88E6352_G2_AVB_CMD_ADDR_MASK		0x001f
 
 /* Offset 0x17: AVB Data Register */
 #define MV88E6352_G2_AVB_DATA		0x17
@@ -267,6 +286,9 @@ int mv88e6xxx_g2_pot_clear(struct mv88e6xxx_chip *chip);
 extern const struct mv88e6xxx_irq_ops mv88e6097_watchdog_ops;
 extern const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops;
 
+extern const struct mv88e6xxx_avb_ops mv88e6352_avb_ops;
+extern const struct mv88e6xxx_avb_ops mv88e6390_avb_ops;
+
 #else /* !CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
 
 static inline int mv88e6xxx_g2_require(struct mv88e6xxx_chip *chip)
@@ -382,6 +404,9 @@ static inline int mv88e6xxx_g2_pot_clear(struct mv88e6xxx_chip *chip)
 static const struct mv88e6xxx_irq_ops mv88e6097_watchdog_ops = {};
 static const struct mv88e6xxx_irq_ops mv88e6390_watchdog_ops = {};
 
+static const struct mv88e6xxx_avb_ops mv88e6352_avb_ops = {};
+static const struct mv88e6xxx_avb_ops mv88e6390_avb_ops = {};
+
 #endif /* CONFIG_NET_DSA_MV88E6XXX_GLOBAL2 */
 
 #endif /* _MV88E6XXX_GLOBAL2_H */
-- 
2.1.4

Re: [PATCH net-next RFC 1/9] net: dsa: mv88e6xxx: add accessors for PTP/TAI registers

[Vivien Didelot]

Hi Brandon,

Brandon Streiff <brandon.streiff@ni.com> writes:

> +	.port_ptp_read		= mv88e6352_port_ptp_read,
> +	.port_ptp_write		= mv88e6352_port_ptp_write,
> +	.ptp_read		= mv88e6352_ptp_read,
> +	.ptp_write		= mv88e6352_ptp_write,
> +	.tai_read		= mv88e6352_tai_read,
> +	.tai_write		= mv88e6352_tai_write,

> +	.port_ptp_read		= mv88e6390_port_ptp_read,
> +	.port_ptp_write		= mv88e6390_port_ptp_write,
> +	.ptp_read		= mv88e6390_ptp_read,
> +	.ptp_write		= mv88e6390_ptp_write,
> +	.tai_read		= mv88e6390_tai_read,
> +	.tai_write		= mv88e6390_tai_write,

Only nitpick: please keep the mv88e63{52,90}_g2_avb_ prefix here.

Otherwise thanks for respecting the code organization, very clear patch:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>


             Vivien

Re: [PATCH net-next RFC 1/9] net: dsa: mv88e6xxx: add accessors for PTP/TAI registers

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:33AM -0500, Brandon Streiff wrote:
> This patch implements support for accessing PTP/TAI registers through

To avoid confusion, it would be helpful to mention what TAI stands for
here and also in the source code comments!

Thanks,
Richard

[PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Brandon Streiff]

This patch adds basic support for exposing the 32-bit timestamp counter
inside the mv88e6xxx switch code as a ptp_clock.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/Kconfig  |  10 +++
 drivers/net/dsa/mv88e6xxx/Makefile |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c   |  20 +++++
 drivers/net/dsa/mv88e6xxx/chip.h   |  16 ++++
 drivers/net/dsa/mv88e6xxx/ptp.c    | 180 +++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/ptp.h    |  83 +++++++++++++++++
 6 files changed, 310 insertions(+)
 create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.h

diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig
index 1aaa7a9..ae9e7f7 100644
--- a/drivers/net/dsa/mv88e6xxx/Kconfig
+++ b/drivers/net/dsa/mv88e6xxx/Kconfig
@@ -18,3 +18,13 @@ config NET_DSA_MV88E6XXX_GLOBAL2
 
 	  It is required on most chips. If the chip you compile the support for
 	  doesn't have such registers set, say N here. In doubt, say Y.
+
+config NET_DSA_MV88E6XXX_PTP
+	bool "PTP support for Marvell 88E6xxx"
+	default n
+	depends on NET_DSA_MV88E6XXX_GLOBAL2
+	imply NETWORK_PHY_TIMESTAMPING
+	imply PTP_1588_CLOCK
+	help
+	  Say Y to enable PTP hardware timestamping on Marvell 88E6xxx switch
+	  chips that support it.
diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index 5cd5551..47eda30 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -6,4 +6,5 @@ mv88e6xxx-objs += global1_vtu.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
 mv88e6xxx-objs += phy.o
 mv88e6xxx-objs += port.o
+mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
 mv88e6xxx-objs += serdes.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 67efa7b..2ed37d8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -38,6 +38,7 @@
 #include "global2.h"
 #include "phy.h"
 #include "port.h"
+#include "ptp.h"
 #include "serdes.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
@@ -2033,6 +2034,13 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	if (err)
 		goto unlock;
 
+	/* Setup PTP Hardware Clock */
+	if (chip->info->ptp_support) {
+		err = mv88e6xxx_ptp_setup(chip);
+		if (err)
+			goto unlock;
+	}
+
 unlock:
 	mutex_unlock(&chip->reg_lock);
 
@@ -3418,6 +3426,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_DSA,
+		.ptp_support = true,
 		.ops = &mv88e6191_ops,
 	},
 
@@ -3438,6 +3447,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_EDSA,
+		.ptp_support = true,
 		.ops = &mv88e6240_ops,
 	},
 
@@ -3458,6 +3468,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_DSA,
+		.ptp_support = true,
 		.ops = &mv88e6290_ops,
 	},
 
@@ -3477,6 +3488,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_EDSA,
+		.ptp_support = true,
 		.ops = &mv88e6320_ops,
 	},
 
@@ -3495,6 +3507,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.atu_move_port_mask = 0xf,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_EDSA,
+		.ptp_support = true,
 		.ops = &mv88e6321_ops,
 	},
 
@@ -3514,6 +3527,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_EDSA,
+		.ptp_support = true,
 		.ops = &mv88e6341_ops,
 	},
 
@@ -3574,6 +3588,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_EDSA,
+		.ptp_support = true,
 		.ops = &mv88e6352_ops,
 	},
 	[MV88E6390] = {
@@ -3593,6 +3608,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_DSA,
+		.ptp_support = true,
 		.ops = &mv88e6390_ops,
 	},
 	[MV88E6390X] = {
@@ -3612,6 +3628,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.pvt = true,
 		.multi_chip = true,
 		.tag_protocol = DSA_TAG_PROTO_DSA,
+		.ptp_support = true,
 		.ops = &mv88e6390x_ops,
 	},
 };
@@ -3949,6 +3966,9 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	if (chip->info->ptp_support)
+		mv88e6xxx_ptp_free(chip);
+
 	mv88e6xxx_phy_destroy(chip);
 	mv88e6xxx_unregister_switch(chip);
 	mv88e6xxx_mdios_unregister(chip);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index d99d592..648bd50 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -16,6 +16,8 @@
 #include <linux/irq.h>
 #include <linux/gpio/consumer.h>
 #include <linux/phy.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/timecounter.h>
 #include <net/dsa.h>
 
 #ifndef UINT64_MAX
@@ -126,6 +128,9 @@ struct mv88e6xxx_info {
 	 */
 	u8 atu_move_port_mask;
 	const struct mv88e6xxx_ops *ops;
+
+	/* Supports PTP */
+	bool ptp_support;
 };
 
 struct mv88e6xxx_atu_entry {
@@ -208,6 +213,17 @@ struct mv88e6xxx_chip {
 	int irq;
 	int device_irq;
 	int watchdog_irq;
+
+	/* This cyclecounter abstracts the switch PTP time.
+	 * reg_lock must be held for any operation that read()s.
+	 */
+	struct cyclecounter	tstamp_cc;
+	struct timecounter	tstamp_tc;
+	struct delayed_work	overflow_work;
+	unsigned long		last_overflow_check;
+
+	struct ptp_clock	*ptp_clock;
+	struct ptp_clock_info	ptp_clock_info;
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
new file mode 100644
index 0000000..155f8e9
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -0,0 +1,180 @@
+/*
+ * Marvell 88E6xxx Switch PTP support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 National Instruments
+ *      Erik Hons <erik.hons@ni.com>
+ *      Brandon Streiff <brandon.streiff@ni.com>
+ *      Dane Wagner <dane.wagner@ni.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "chip.h"
+#include "global2.h"
+#include "ptp.h"
+
+static int mv88e6xxx_tai_read(struct mv88e6xxx_chip *chip, int addr,
+			      u16 *data, int len)
+{
+	if (!chip->info->ops->avb_ops->tai_read)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->avb_ops->tai_read(chip, addr, data, len);
+}
+
+static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
+{
+	struct mv88e6xxx_chip *chip =
+		container_of(cc, struct mv88e6xxx_chip, tstamp_cc);
+	int err;
+	u16 phc_time[2];
+
+	err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_TIME_LO, phc_time,
+				 ARRAY_SIZE(phc_time));
+	if (err)
+		return 0;
+	else
+		return ((u32)phc_time[1] << 16) | phc_time[0];
+}
+
+static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	if (scaled_ppm == 0)
+		return 0;
+
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct mv88e6xxx_chip *chip =
+		container_of(ptp, struct mv88e6xxx_chip, ptp_clock_info);
+
+	mutex_lock(&chip->reg_lock);
+	timecounter_adjtime(&chip->tstamp_tc, delta);
+	mutex_unlock(&chip->reg_lock);
+
+	return 0;
+}
+
+static int mv88e6xxx_ptp_gettime(struct ptp_clock_info *ptp,
+				 struct timespec64 *ts)
+{
+	struct mv88e6xxx_chip *chip =
+		container_of(ptp, struct mv88e6xxx_chip, ptp_clock_info);
+	u64 ns;
+
+	mutex_lock(&chip->reg_lock);
+	ns = timecounter_read(&chip->tstamp_tc);
+	chip->last_overflow_check = jiffies;
+	mutex_unlock(&chip->reg_lock);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
+static int mv88e6xxx_ptp_settime(struct ptp_clock_info *ptp,
+				 const struct timespec64 *ts)
+{
+	struct mv88e6xxx_chip *chip =
+		container_of(ptp, struct mv88e6xxx_chip, ptp_clock_info);
+	u64 ns;
+
+	ns = timespec64_to_ns(ts);
+
+	mutex_lock(&chip->reg_lock);
+	timecounter_init(&chip->tstamp_tc, &chip->tstamp_cc, ns);
+	mutex_unlock(&chip->reg_lock);
+
+	return 0;
+}
+
+static int mv88e6xxx_ptp_enable(struct ptp_clock_info *ptp,
+				struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static int mv88e6xxx_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
+				enum ptp_pin_function func, unsigned int chan)
+{
+	return -EOPNOTSUPP;
+}
+
+/* The 32-bit timestamp counter overflows every ~34.3 seconds; this task
+ * forces periodic reads so that we don't miss any wraparounds.
+ */
+#define MV88E6XXX_TAI_OVERFLOW_PERIOD (34 * HZ / 2)
+static void mv88e6xxx_ptp_overflow_check(struct work_struct *work)
+{
+	struct delayed_work *dw = to_delayed_work(work);
+	struct mv88e6xxx_chip *chip =
+		container_of(dw, struct mv88e6xxx_chip, overflow_work);
+	bool timeout = time_is_before_jiffies(chip->last_overflow_check +
+					      MV88E6XXX_TAI_OVERFLOW_PERIOD);
+
+	if (timeout) {
+		mutex_lock(&chip->reg_lock);
+		timecounter_read(&chip->tstamp_tc);
+		chip->last_overflow_check = jiffies;
+		mutex_unlock(&chip->reg_lock);
+	}
+
+	schedule_delayed_work(&chip->overflow_work,
+			      MV88E6XXX_TAI_OVERFLOW_PERIOD);
+}
+
+int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
+{
+	/* Set up the cycle counter */
+	memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc));
+	chip->tstamp_cc.read	= mv88e6xxx_ptp_clock_read;
+	chip->tstamp_cc.mask	= CYCLECOUNTER_MASK(32);
+	/* Raw timestamps are in units of 8-ns clock periods. */
+	chip->tstamp_cc.mult	= 8;
+	chip->tstamp_cc.shift	= 0;
+
+	timecounter_init(&chip->tstamp_tc, &chip->tstamp_cc,
+			 ktime_to_ns(ktime_get_real()));
+
+	chip->last_overflow_check = jiffies;
+
+	INIT_DELAYED_WORK(&chip->overflow_work, mv88e6xxx_ptp_overflow_check);
+
+	chip->ptp_clock_info.owner = THIS_MODULE;
+	snprintf(chip->ptp_clock_info.name, sizeof(chip->ptp_clock_info.name),
+		 dev_name(chip->dev));
+	chip->ptp_clock_info.max_adj	= 0;
+
+	chip->ptp_clock_info.adjfine	= mv88e6xxx_ptp_adjfine;
+	chip->ptp_clock_info.adjtime	= mv88e6xxx_ptp_adjtime;
+	chip->ptp_clock_info.gettime64	= mv88e6xxx_ptp_gettime;
+	chip->ptp_clock_info.settime64	= mv88e6xxx_ptp_settime;
+	chip->ptp_clock_info.enable	= mv88e6xxx_ptp_enable;
+	chip->ptp_clock_info.verify	= mv88e6xxx_ptp_verify;
+
+	chip->ptp_clock = ptp_clock_register(&chip->ptp_clock_info, chip->dev);
+	if (IS_ERR(chip->ptp_clock))
+		return PTR_ERR(chip->ptp_clock);
+
+	schedule_delayed_work(&chip->overflow_work,
+			      MV88E6XXX_TAI_OVERFLOW_PERIOD);
+
+	return 0;
+}
+
+void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip)
+{
+	if (chip->ptp_clock) {
+		cancel_delayed_work_sync(&chip->overflow_work);
+
+		ptp_clock_unregister(chip->ptp_clock);
+		chip->ptp_clock = NULL;
+	}
+}
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
new file mode 100644
index 0000000..c662953
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -0,0 +1,83 @@
+/*
+ * Marvell 88E6xxx Switch PTP support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 National Instruments
+ *      Erik Hons <erik.hons@ni.com>
+ *      Brandon Streiff <brandon.streiff@ni.com>
+ *      Dane Wagner <dane.wagner@ni.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _MV88E6XXX_PTP_H
+#define _MV88E6XXX_PTP_H
+
+#include "chip.h"
+
+/* Offset 0x00: TAI Global Config */
+#define MV88E6XXX_TAI_CFG			0x00
+
+/* Offset 0x01: Timestamp Clock Period (ps) */
+#define MV88E6XXX_TAI_CLOCK_PERIOD		0x01
+
+/* Offset 0x02/0x03: Trigger Generation Amount */
+#define MV88E6XXX_TAI_TRIG_GEN_AMOUNT_LO	0x02
+#define MV88E6XXX_TAI_TRIG_GEN_AMOUNT_HI	0x03
+
+/* Offset 0x04: Clock Compensation */
+#define MV88E6XXX_TAI_TRIG_CLOCK_COMP		0x04
+
+/* Offset 0x05: Trigger Configuration */
+#define MV88E6XXX_TAI_TRIG_CFG			0x05
+
+/* Offset 0x06: Ingress Rate Limiter Clock Generation Amount */
+#define MV88E6XXX_TAI_IRL_AMOUNT		0x06
+
+/* Offset 0x07: Ingress Rate Limiter Compensation */
+#define MV88E6XXX_TAI_IRL_COMP			0x07
+
+/* Offset 0x08: Ingress Rate Limiter Compensation */
+#define MV88E6XXX_TAI_IRL_COMP_PS		0x08
+
+/* Offset 0x09: Event Status */
+#define MV88E6XXX_TAI_EVENT_STATUS		0x09
+
+/* Offset 0x0A/0x0B: Event Time */
+#define MV88E6XXX_TAI_EVENT_TIME_LO		0x0a
+#define MV88E6XXX_TAI_EVENT_TYPE_HI		0x0b
+
+/* Offset 0x0E/0x0F: PTP Global Time */
+#define MV88E6XXX_TAI_TIME_LO			0x0e
+#define MV88E6XXX_TAI_TIME_HI			0x0f
+
+/* Offset 0x10/0x11: Trig Generation Time */
+#define MV88E6XXX_TAI_TRIG_TIME_LO		0x10
+#define MV88E6XXX_TAI_TRIG_TIME_HI		0x11
+
+/* Offset 0x12: Lock Status */
+#define MV88E6XXX_TAI_LOCK_STATUS		0x12
+
+#ifdef CONFIG_NET_DSA_MV88E6XXX_PTP
+
+int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip);
+void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip);
+
+#else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */
+
+static inline int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
+{
+	return -EOPNOTSUPP;
+}
+
+static void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip)
+{
+}
+
+#endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */
+
+#endif /* _MV88E6XXX_PTP_H */
-- 
2.1.4

Re: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Andrew Lunn]

On Thu, Sep 28, 2017 at 10:25:34AM -0500, Brandon Streiff wrote:
> This patch adds basic support for exposing the 32-bit timestamp counter
> inside the mv88e6xxx switch code as a ptp_clock.
> 
> Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
> ---
>  drivers/net/dsa/mv88e6xxx/Kconfig  |  10 +++
>  drivers/net/dsa/mv88e6xxx/Makefile |   1 +
>  drivers/net/dsa/mv88e6xxx/chip.c   |  20 +++++
>  drivers/net/dsa/mv88e6xxx/chip.h   |  16 ++++
>  drivers/net/dsa/mv88e6xxx/ptp.c    | 180 +++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/ptp.h    |  83 +++++++++++++++++
>  6 files changed, 310 insertions(+)
>  create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.c
>  create mode 100644 drivers/net/dsa/mv88e6xxx/ptp.h
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/Kconfig b/drivers/net/dsa/mv88e6xxx/Kconfig
> index 1aaa7a9..ae9e7f7 100644
> --- a/drivers/net/dsa/mv88e6xxx/Kconfig
> +++ b/drivers/net/dsa/mv88e6xxx/Kconfig
> @@ -18,3 +18,13 @@ config NET_DSA_MV88E6XXX_GLOBAL2
>  
>  	  It is required on most chips. If the chip you compile the support for
>  	  doesn't have such registers set, say N here. In doubt, say Y.
> +
> +config NET_DSA_MV88E6XXX_PTP
> +	bool "PTP support for Marvell 88E6xxx"
> +	default n
> +	depends on NET_DSA_MV88E6XXX_GLOBAL2
> +	imply NETWORK_PHY_TIMESTAMPING

Hi Brandon

Cool to see this code.

One probably dumb question so far..

It is the MAC which is doing the time stamping, not they PHY?
So why NETWORK_PHY_TIMESTAMPING?

   Andrew

RE: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Brandon Streiff]

> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, September 28, 2017 11:57 AM
> 
> It is the MAC which is doing the time stamping, not they PHY?
> So why NETWORK_PHY_TIMESTAMPING?

NETWORK_PHY_TIMESTAMPING implies NET_PTP_CLASSIFY (which I do use) and net/core/timestamping.c (which I didn't). It probably makes more sense to just depend on NET_PTP_CLASSIFY directly.

-- brandon

Re: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Richard Cochran]

On Fri, Sep 29, 2017 at 03:28:02PM +0000, Brandon Streiff wrote:
> 
> NETWORK_PHY_TIMESTAMPING implies NET_PTP_CLASSIFY (which I do use)
> and net/core/timestamping.c (which I didn't). It probably makes more
> sense to just depend on NET_PTP_CLASSIFY directly.

Yes, that makes sense to do, if you can make it work.

With my driver I tried depending on NET_PTP_CLASSIFY, but there was
some Kconfig issue, and rather than figuring it out I did the lazy
thing and used NETWORK_PHY_TIMESTAMPING.

Thanks,
Richard

Re: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Andrew Lunn]

> +/* The 32-bit timestamp counter overflows every ~34.3 seconds; this task
> + * forces periodic reads so that we don't miss any wraparounds.
> + */
> +#define MV88E6XXX_TAI_OVERFLOW_PERIOD (34 * HZ / 2)
> +static void mv88e6xxx_ptp_overflow_check(struct work_struct *work)
> +{
> +	struct delayed_work *dw = to_delayed_work(work);
> +	struct mv88e6xxx_chip *chip =
> +		container_of(dw, struct mv88e6xxx_chip, overflow_work);
> +	bool timeout = time_is_before_jiffies(chip->last_overflow_check +
> +					      MV88E6XXX_TAI_OVERFLOW_PERIOD);
> +
> +	if (timeout) {

Why do you need this timeout? Do you think the kernel will call this
more often than required?

Also, if it did call this function early, you skip the read, and
reschedule. There is then a danger the next read is after the
wraparound.....

> +		mutex_lock(&chip->reg_lock);
> +		timecounter_read(&chip->tstamp_tc);
> +		chip->last_overflow_check = jiffies;
> +		mutex_unlock(&chip->reg_lock);
> +	}
> +
> +	schedule_delayed_work(&chip->overflow_work,
> +			      MV88E6XXX_TAI_OVERFLOW_PERIOD);
> +}

Thanks
	Andrew

RE: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Brandon Streiff]

> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, September 28, 2017 12:03 PM
> 
> > +	bool timeout = time_is_before_jiffies(chip->last_overflow_check +
> > +					      MV88E6XXX_TAI_OVERFLOW_PERIOD);
> > +
> > +	if (timeout) {
> 
> Why do you need this timeout? Do you think the kernel will call this
> more often than required?
> 
> Also, if it did call this function early, you skip the read, and
> reschedule. There is then a danger the next read is after the
> wraparound.....

That was, conceptually, a copy-paste from ixgbe_ptp.c as I was looking for how to implement the overflow accounting; that driver has a similar time_is_before_jiffies check in ixgbe_ptp_overflow_check.
 
Although now that I'm looking it over again, I'm also not certain of the need. Even if we're called more frequently than we expect, that doesn't seem to be harmful with regard to timekeeping. Hmm.

-- brandon

Re: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Richard Cochran]

On Fri, Sep 29, 2017 at 03:17:02PM +0000, Brandon Streiff wrote:
>  
> Although now that I'm looking it over again, I'm also not certain of
> the need. Even if we're called more frequently than we expect, that
> doesn't seem to be harmful with regard to timekeeping. Hmm.

Just keep it simple and drop the extra logic.  It doesn't hurt to
over-sample the clock.  Here is what I did:

/* Covers both a 100 or a 125 MHz input clock. */
#define MV88E635X_OVERFLOW_PERIOD (HZ * 16)

static void mv88e635x_overflow_check(struct work_struct *ws)
{
	struct timespec64 ts;
	struct mv88e6xxx_chip *ps =
		container_of(ws, struct mv88e6xxx_chip, oflow_work.work);

	mv88e635x_ptp_gettime(&ps->ptp_info, &ts);
	pr_debug("mv88e635x overflow check at %lld.%09lu\n",
		 ts.tv_sec, ts.tv_nsec);
	schedule_delayed_work(&ps->oflow_work, MV88E635X_OVERFLOW_PERIOD);
}

Thanks,
Richard

Re: [PATCH net-next RFC 2/9] net: dsa: mv88e6xxx: expose switch time as a PTP hardware clock

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:34AM -0500, Brandon Streiff wrote:
> +static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	if (scaled_ppm == 0)
> +		return 0;
> +
> +	return -EOPNOTSUPP;
> +}

We really want to have an adjustable clock here.  More below.

> +int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
> +{
> +	/* Set up the cycle counter */
> +	memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc));
> +	chip->tstamp_cc.read	= mv88e6xxx_ptp_clock_read;
> +	chip->tstamp_cc.mask	= CYCLECOUNTER_MASK(32);
> +	/* Raw timestamps are in units of 8-ns clock periods. */
> +	chip->tstamp_cc.mult	= 8;
> +	chip->tstamp_cc.shift	= 0;

First of all, the switch can use an external clock, and so at the very
least, the period should be a macro so that if and when we support the
external clock, the macro may be converted into a variable.

Secondly, the mult/shift should be chosen to allow the finest possible
frequency adjustment.  Here is what I did:

---
#define N 28
#define CC_MULT (8 << N)

int mv88e635x_setup(struct dsa_switch *ds)
{
	struct mv88e6xxx_chip *ps = ds->priv;

	ps->cc.read = mv88e635x_global_time_read;
	ps->cc.mask = CLOCKSOURCE_MASK(32);
	ps->cc.mult = CC_MULT;
	ps->cc.shift = N;
	timecounter_init(&ps->tc, &ps->cc, ktime_to_ns(ktime_get_real()));
	...
}

static int mv88e635x_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
{
	u64 adj;
	u32 diff, mult;
	int neg_adj = 0;
	struct mv88e6xxx_chip *ps =
		container_of(ptp, struct mv88e6xxx_chip, ptp_info);

	if (ppb < 0) {
		neg_adj = 1;
		ppb = -ppb;
	}
	mult = CC_MULT;
	adj = mult;
	adj *= ppb;
	diff = div_u64(adj, 1000000000ULL);

	mutex_lock(&ps->clock_mutex);
	timecounter_read(&ps->tc);
	ps->cc.mult = neg_adj ? mult - diff : mult + diff;
	mutex_unlock(&ps->clock_mutex);

	return 0;
}
---

(This is the legacy adjfreq method, but you can easily convert it into
 the adjfine method.)

Of course, this means that you'll have to drop the periodic output
signal code.

Thanks,
Richard

[PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Brandon Streiff]

The Scratch/Misc register is a windowed interface that provides access
to the GPIO configuration. Provide a new method for configuration of
GPIO functions.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    | 13 +++++++
 drivers/net/dsa/mv88e6xxx/chip.h    |  8 +++++
 drivers/net/dsa/mv88e6xxx/global2.c | 71 +++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global2.h | 32 +++++++++++++++++
 4 files changed, 124 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 2ed37d8..4a37b26 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3218,6 +3218,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6341",
 		.num_databases = 4096,
 		.num_ports = 6,
+		.num_gpio = 11,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3297,6 +3298,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6172",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3337,6 +3339,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6176",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3375,6 +3378,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6190",
 		.num_databases = 4096,
 		.num_ports = 11,	/* 10 + Z80 */
+		.num_gpio = 16,
 		.max_vid = 8191,
 		.port_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -3395,6 +3399,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6190X",
 		.num_databases = 4096,
 		.num_ports = 11,	/* 10 + Z80 */
+		.num_gpio = 16,
 		.max_vid = 8191,
 		.port_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -3436,6 +3441,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6240",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3457,6 +3463,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6290",
 		.num_databases = 4096,
 		.num_ports = 11,	/* 10 + Z80 */
+		.num_gpio = 16,
 		.max_vid = 8191,
 		.port_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -3478,6 +3485,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6320",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3498,6 +3506,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6321",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3517,6 +3526,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6341",
 		.num_databases = 4096,
 		.num_ports = 6,
+		.num_gpio = 11,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3577,6 +3587,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6352",
 		.num_databases = 4096,
 		.num_ports = 7,
+		.num_gpio = 15,
 		.max_vid = 4095,
 		.port_base_addr = 0x10,
 		.global1_addr = 0x1b,
@@ -3597,6 +3608,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6390",
 		.num_databases = 4096,
 		.num_ports = 11,	/* 10 + Z80 */
+		.num_gpio = 16,
 		.max_vid = 8191,
 		.port_base_addr = 0x0,
 		.global1_addr = 0x1b,
@@ -3617,6 +3629,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.name = "Marvell 88E6390X",
 		.num_databases = 4096,
 		.num_ports = 11,	/* 10 + Z80 */
+		.num_gpio = 16,
 		.max_vid = 8191,
 		.port_base_addr = 0x0,
 		.global1_addr = 0x1b,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 648bd50..5f132e2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -41,6 +41,8 @@
 #define MV88E6XXX_MAX_PVT_SWITCHES	32
 #define MV88E6XXX_MAX_PVT_PORTS		16
 
+#define MV88E6XXX_MAX_GPIO	16
+
 enum mv88e6xxx_egress_mode {
 	MV88E6XXX_EGRESS_MODE_UNMODIFIED,
 	MV88E6XXX_EGRESS_MODE_UNTAGGED,
@@ -107,6 +109,7 @@ struct mv88e6xxx_info {
 	const char *name;
 	unsigned int num_databases;
 	unsigned int num_ports;
+	unsigned int num_gpio;
 	unsigned int max_vid;
 	unsigned int port_base_addr;
 	unsigned int global1_addr;
@@ -417,6 +420,11 @@ static inline u16 mv88e6xxx_port_mask(struct mv88e6xxx_chip *chip)
 	return GENMASK(mv88e6xxx_num_ports(chip) - 1, 0);
 }
 
+static inline unsigned int mv88e6xxx_num_gpio(struct mv88e6xxx_chip *chip)
+{
+	return chip->info->num_gpio;
+}
+
 int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val);
 int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val);
 int mv88e6xxx_update(struct mv88e6xxx_chip *chip, int addr, int reg,
diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index b6d0c71..fe2c970 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -971,6 +971,77 @@ int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip, struct mii_bus *bus,
 						   val);
 }
 
+/* Offset 0x1A: Scratch and Misc. Register */
+static int mv88e6xxx_g2_scratch_reg_read(struct mv88e6xxx_chip *chip,
+					 int reg, u8 *data)
+{
+	int err;
+	u16 value;
+
+	err = mv88e6xxx_g2_write(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC,
+				 reg << 8);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, &value);
+	if (err)
+		return err;
+
+	*data = (value & MV88E6XXX_G2_SCRATCH_MISC_DATA_MASK);
+
+	return 0;
+}
+
+static int mv88e6xxx_g2_scratch_reg_write(struct mv88e6xxx_chip *chip,
+					  int reg, u8 data)
+{
+	u16 value = (reg << 8) | data;
+
+	return mv88e6xxx_g2_update(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, value);
+}
+
+/* Configures the specified pin for the specified function. This function
+ * does not unset other pins configured for the same function. If multiple
+ * pins are configured for the same function, the lower-index pin gets
+ * that function and the higher-index pin goes back to being GPIO.
+ */
+int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip, int pin,
+				 int func, int dir)
+{
+	int mode_reg = MV88E6XXX_G2_SCRATCH_GPIO_MODE(pin);
+	int dir_reg = MV88E6XXX_G2_SCRATCH_GPIO_DIR(pin);
+	int err;
+	u8 val;
+
+	if (pin < 0 || pin >= mv88e6xxx_num_gpio(chip))
+		return -ERANGE;
+
+	/* Set function first */
+	err = mv88e6xxx_g2_scratch_reg_read(chip, mode_reg, &val);
+	if (err)
+		return err;
+
+	/* Zero bits in the field for this GPIO and OR in new config */
+	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_MODE_MASK(pin);
+	val |= (func << MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin));
+
+	err = mv88e6xxx_g2_scratch_reg_write(chip, mode_reg, val);
+	if (err)
+		return err;
+
+	/* Set direction */
+	err = mv88e6xxx_g2_scratch_reg_read(chip, dir_reg, &val);
+	if (err)
+		return err;
+
+	/* Zero bits in the field for this GPIO and OR in new config */
+	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_DIR_MASK(pin);
+	val |= (dir << MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin));
+
+	return mv88e6xxx_g2_scratch_reg_write(chip, dir_reg, val);
+}
+
+/* Offset 0x1B: Watchdog Control */
 static int mv88e6097_watchdog_action(struct mv88e6xxx_chip *chip, int irq)
 {
 	u16 reg;
diff --git a/drivers/net/dsa/mv88e6xxx/global2.h b/drivers/net/dsa/mv88e6xxx/global2.h
index 642d2b0..f192fc6 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.h
+++ b/drivers/net/dsa/mv88e6xxx/global2.h
@@ -242,6 +242,29 @@
 #define MV88E6352_G2_NOEGR_POLICY	0x2000
 #define MV88E6390_G2_LAG_ID_4		0x2000
 
+/* Scratch/Misc registers accessed through MV88E6XXX_G2_SCRATCH_MISC */
+#define MV88E6XXX_G2_SCRATCH_GPIO_CONFIG_LO	0x60
+#define MV88E6XXX_G2_SCRATCH_GPIO_CONFIG_HI	0x61
+#define MV88E6XXX_G2_SCRATCH_GPIO_DIR(pin)	(0x62 + ((pin) / 8))
+#define MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin) \
+			((pin) & 0x7)
+#define MV88E6XXX_G2_SCRATCH_GPIO_DIR_MASK(pin)	\
+			(1 << MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin))
+#define MV88E6XXX_G2_SCRATCH_GPIO_DIR_OUT	0
+#define MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN	1
+#define MV88E6XXX_G2_SCRATCH_GPIO_DATA(pin)	(0x64 + ((pin) / 8))
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE(pin)	(0x68 + ((pin) / 2))
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin) \
+			((pin) & 0x1 ? 4 : 0)
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_MASK(pin) \
+			(0x7 << MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin))
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO	0
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_TRIG	1
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_EVREQ	2
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_EXTCLK	3
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_RXCLK0	4
+#define MV88E6XXX_G2_SCRATCH_GPIO_MODE_RXCLK1	5
+
 #ifdef CONFIG_NET_DSA_MV88E6XXX_GLOBAL2
 
 static inline int mv88e6xxx_g2_require(struct mv88e6xxx_chip *chip)
@@ -270,6 +293,9 @@ int mv88e6xxx_g2_get_eeprom16(struct mv88e6xxx_chip *chip,
 int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
 			      struct ethtool_eeprom *eeprom, u8 *data);
 
+int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip, int pin,
+				 int func, int dir);
+
 int mv88e6xxx_g2_pvt_write(struct mv88e6xxx_chip *chip, int src_dev,
 			   int src_port, u16 data);
 int mv88e6xxx_g2_misc_4_bit_port(struct mv88e6xxx_chip *chip);
@@ -361,6 +387,12 @@ static inline int mv88e6xxx_g2_set_eeprom16(struct mv88e6xxx_chip *chip,
 	return -EOPNOTSUPP;
 }
 
+static inline int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip,
+					       int pin, int func, int dir)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int mv88e6xxx_g2_pvt_write(struct mv88e6xxx_chip *chip,
 					 int src_dev, int src_port, u16 data)
 {
-- 
2.1.4

Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Florian Fainelli]

On 09/28/2017 08:25 AM, Brandon Streiff wrote:
> The Scratch/Misc register is a windowed interface that provides access
> to the GPIO configuration. Provide a new method for configuration of
> GPIO functions.
> 
> Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
> ---

> +/* Offset 0x1A: Scratch and Misc. Register */
> +static int mv88e6xxx_g2_scratch_reg_read(struct mv88e6xxx_chip *chip,
> +					 int reg, u8 *data)
> +{
> +	int err;
> +	u16 value;
> +
> +	err = mv88e6xxx_g2_write(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC,
> +				 reg << 8);
> +	if (err)
> +		return err;
> +
> +	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, &value);
> +	if (err)
> +		return err;
> +
> +	*data = (value & MV88E6XXX_G2_SCRATCH_MISC_DATA_MASK);
> +
> +	return 0;
> +}

With the write and read acquiring and then releasing the lock
immediately, is no there room for this sequence to be interrupted in the
middle and end-up returning inconsistent reads?

> +
> +static int mv88e6xxx_g2_scratch_reg_write(struct mv88e6xxx_chip *chip,
> +					  int reg, u8 data)
> +{
> +	u16 value = (reg << 8) | data;
> +
> +	return mv88e6xxx_g2_update(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, value);
> +}
> +
> +/* Configures the specified pin for the specified function. This function
> + * does not unset other pins configured for the same function. If multiple
> + * pins are configured for the same function, the lower-index pin gets
> + * that function and the higher-index pin goes back to being GPIO.
> + */
> +int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip, int pin,
> +				 int func, int dir)
> +{
> +	int mode_reg = MV88E6XXX_G2_SCRATCH_GPIO_MODE(pin);
> +	int dir_reg = MV88E6XXX_G2_SCRATCH_GPIO_DIR(pin);
> +	int err;
> +	u8 val;
> +
> +	if (pin < 0 || pin >= mv88e6xxx_num_gpio(chip))
> +		return -ERANGE;
> +
> +	/* Set function first */
> +	err = mv88e6xxx_g2_scratch_reg_read(chip, mode_reg, &val);
> +	if (err)
> +		return err;
> +
> +	/* Zero bits in the field for this GPIO and OR in new config */
> +	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_MODE_MASK(pin);
> +	val |= (func << MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin));
> +
> +	err = mv88e6xxx_g2_scratch_reg_write(chip, mode_reg, val);
> +	if (err)
> +		return err;
> +
> +	/* Set direction */
> +	err = mv88e6xxx_g2_scratch_reg_read(chip, dir_reg, &val);
> +	if (err)
> +		return err;
> +
> +	/* Zero bits in the field for this GPIO and OR in new config */
> +	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_DIR_MASK(pin);
> +	val |= (dir << MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin));
> +
> +	return mv88e6xxx_g2_scratch_reg_write(chip, dir_reg, val);
> +}

Would there be any value in implementing a proper gpiochip structure
here such that other pieces of SW can see this GPIO controller as a
provider and you can reference it from e.g: Device Tree using GPIO
descriptors?
-- 
Florian

Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Andrew Lunn]

On Thu, Sep 28, 2017 at 10:45:03AM -0700, Florian Fainelli wrote:
> On 09/28/2017 08:25 AM, Brandon Streiff wrote:
> > The Scratch/Misc register is a windowed interface that provides access
> > to the GPIO configuration. Provide a new method for configuration of
> > GPIO functions.
> > 
> > Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
> > ---
> 
> > +/* Offset 0x1A: Scratch and Misc. Register */
> > +static int mv88e6xxx_g2_scratch_reg_read(struct mv88e6xxx_chip *chip,
> > +					 int reg, u8 *data)
> > +{
> > +	int err;
> > +	u16 value;
> > +
> > +	err = mv88e6xxx_g2_write(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC,
> > +				 reg << 8);
> > +	if (err)
> > +		return err;
> > +
> > +	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, &value);
> > +	if (err)
> > +		return err;
> > +
> > +	*data = (value & MV88E6XXX_G2_SCRATCH_MISC_DATA_MASK);
> > +
> > +	return 0;
> > +}
> 
> With the write and read acquiring and then releasing the lock
> immediately, is no there room for this sequence to be interrupted in the
> middle and end-up returning inconsistent reads?

Hi Florian

The general pattern in this code is that the lock chip->reg_lock is
taken at a higher level. That protects against other threads. The
driver tends to do that at the highest levels, at the entry points
into the driver. I've not yet checked this code follows the pattern
yet. However, we have a check in the low level to ensure the lock has
been taken. So it seems likely the lock is held.
 
> Would there be any value in implementing a proper gpiochip structure
> here such that other pieces of SW can see this GPIO controller as a
> provider and you can reference it from e.g: Device Tree using GPIO
> descriptors?

That would be my preference as well, or maybe a pinctrl driver.

     Andrew

Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Vivien Didelot]

Hi Brandon,

>> Would there be any value in implementing a proper gpiochip structure
>> here such that other pieces of SW can see this GPIO controller as a
>> provider and you can reference it from e.g: Device Tree using GPIO
>> descriptors?
>
> That would be my preference as well, or maybe a pinctrl driver.

Indeed seeing a gpio_chip or a pinctrl controller registered from a
gpio.c or pinctrl.c file in a separate patchset would be great.


Thanks,

        Vivien

RE: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

[Brandon Streiff]

> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, September 28, 2017 1:01 PM
>
> > With the write and read acquiring and then releasing the lock
> > immediately, is no there room for this sequence to be interrupted in the
> > middle and end-up returning inconsistent reads?
> 
> The general pattern in this code is that the lock chip->reg_lock is
> taken at a higher level. That protects against other threads. The
> driver tends to do that at the highest levels, at the entry points
> into the driver. I've not yet checked this code follows the pattern
> yet. However, we have a check in the low level to ensure the lock has
> been taken. So it seems likely the lock is held.

Yes, the expectation here is that an upper layer takes the reg_lock. All the functions in ptp.c that call this function do that. If they did not, then assert_reg_lock() gets very angry. :)

Perhaps using __must_hold() and similar annotations would also help document the requirements, but we don't seem to use those in this driver today.
 
-- brandon

[PATCH net-next RFC 4/9] net: dsa: mv88e6xxx: add support for event capture

[Brandon Streiff]

This patch adds support for configuring mv88e6xxx GPIO lines as PTP
pins, so that they may be used for time stamping external events or for
periodic output.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/chip.h |   4 +
 drivers/net/dsa/mv88e6xxx/ptp.c  | 317 ++++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/ptp.h  |  16 ++
 3 files changed, 335 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 5f132e2..b763778 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -227,6 +227,10 @@ struct mv88e6xxx_chip {
 
 	struct ptp_clock	*ptp_clock;
 	struct ptp_clock_info	ptp_clock_info;
+	struct delayed_work	tai_event_work;
+	struct ptp_pin_desc	pin_config[MV88E6XXX_MAX_GPIO];
+	u16 trig_config;
+	u16 evcap_config;
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.c b/drivers/net/dsa/mv88e6xxx/ptp.c
index 155f8e9..1422d85 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.c
+++ b/drivers/net/dsa/mv88e6xxx/ptp.c
@@ -18,6 +18,8 @@
 #include "global2.h"
 #include "ptp.h"
 
+#define TAI_EVENT_WORK_INTERVAL msecs_to_jiffies(100)
+
 static int mv88e6xxx_tai_read(struct mv88e6xxx_chip *chip, int addr,
 			      u16 *data, int len)
 {
@@ -27,6 +29,14 @@ static int mv88e6xxx_tai_read(struct mv88e6xxx_chip *chip, int addr,
 	return chip->info->ops->avb_ops->tai_read(chip, addr, data, len);
 }
 
+static int mv88e6xxx_tai_write(struct mv88e6xxx_chip *chip, int addr, u16 data)
+{
+	if (!chip->info->ops->avb_ops->tai_write)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->avb_ops->tai_write(chip, addr, data);
+}
+
 static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
 {
 	struct mv88e6xxx_chip *chip =
@@ -42,6 +52,144 @@ static u64 mv88e6xxx_ptp_clock_read(const struct cyclecounter *cc)
 		return ((u32)phc_time[1] << 16) | phc_time[0];
 }
 
+static int mv88e6xxx_disable_trig(struct mv88e6xxx_chip *chip)
+{
+	int err;
+	u16 global_config;
+
+	chip->trig_config = 0;
+	global_config = (chip->evcap_config | chip->trig_config);
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_CFG, global_config);
+
+	return err;
+}
+
+static int mv88e6xxx_config_periodic_trig(struct mv88e6xxx_chip *chip,
+					  u32 ns, u16 picos)
+{
+	int err;
+	u16 global_config;
+
+	if (picos >= 1000)
+		return -ERANGE;
+
+	/* TRIG generation is in units of 8 ns clock periods. Convert ns
+	 * and ps into 8 ns clock periods and up to 8000 additional ps
+	 */
+	picos += (ns & 0x7) * 1000;
+	ns = ns >> 3;
+
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_TRIG_GEN_AMOUNT_LO,
+				  ns & 0xffff);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_TRIG_GEN_AMOUNT_HI,
+				  ns >> 16);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_TRIG_CLOCK_COMP,
+				  picos);
+	if (err)
+		return err;
+
+	chip->trig_config = MV88E6XXX_TAI_CFG_TRIG_ENABLE;
+	global_config = (chip->evcap_config | chip->trig_config);
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_CFG, global_config);
+
+	return err;
+}
+
+/* mv88e6xxx_config_eventcap - configure TAI event capture
+ * @event: PTP_CLOCK_PPS (internal) or PTP_CLOCK_EXTTS (external)
+ * @rising: zero for falling-edge trigger, else rising-edge trigger
+ *
+ * This will also reset the capture sequence counter.
+ */
+static int mv88e6xxx_config_eventcap(struct mv88e6xxx_chip *chip, int event,
+				     int rising)
+{
+	u16 global_config;
+	u16 cap_config;
+	int err;
+
+	chip->evcap_config = MV88E6XXX_TAI_CFG_CAP_OVERWRITE |
+			     MV88E6XXX_TAI_CFG_CAP_CTR_START;
+	if (!rising)
+		chip->evcap_config |= MV88E6XXX_TAI_CFG_EVREQ_FALLING;
+
+	global_config = (chip->evcap_config | chip->trig_config);
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_CFG, global_config);
+	if (err)
+		return err;
+
+	if (event == PTP_CLOCK_PPS) {
+		cap_config = MV88E6XXX_TAI_EVENT_STATUS_CAP_TRIG;
+	} else if (event == PTP_CLOCK_EXTTS) {
+		/* if STATUS_CAP_TRIG is unset we capture PTP_EVREQ events */
+		cap_config = 0;
+	} else {
+		return -EINVAL;
+	}
+
+	/* Write the capture config; this also clears the capture counter */
+	err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS,
+				  cap_config);
+
+	return err;
+}
+
+static void mv88e6xxx_tai_event_work(struct work_struct *ugly)
+{
+	struct delayed_work *dw = to_delayed_work(ugly);
+	struct mv88e6xxx_chip *chip =
+		container_of(dw, struct mv88e6xxx_chip, tai_event_work);
+	u16 ev_status[4];
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+
+	err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_EVENT_STATUS,
+				 ev_status, ARRAY_SIZE(ev_status));
+	if (err) {
+		mutex_unlock(&chip->reg_lock);
+		return;
+	}
+
+	if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_ERROR)
+		dev_warn(chip->dev, "missed event capture\n");
+
+	if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_VALID) {
+		struct ptp_clock_event ev;
+		u32 raw_ts = ((u32)ev_status[2] << 16) | ev_status[1];
+
+		/* Clear the valid bit so the next timestamp can come in */
+		ev_status[0] &= ~MV88E6XXX_TAI_EVENT_STATUS_VALID;
+		err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS,
+					  ev_status[0]);
+
+		if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_CAP_TRIG) {
+			/* TAI is configured to timestamp internal events.
+			 * This will be a PPS event.
+			 */
+			ev.type = PTP_CLOCK_PPS;
+		} else {
+			/* Otherwise this is an external timestamp */
+			ev.type = PTP_CLOCK_EXTTS;
+		}
+		/* We only have one timestamping channel. */
+		ev.index = 0;
+		ev.timestamp = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
+
+		ptp_clock_event(chip->ptp_clock, &ev);
+	}
+
+	mutex_unlock(&chip->reg_lock);
+
+	schedule_delayed_work(&chip->tai_event_work, TAI_EVENT_WORK_INTERVAL);
+}
+
 static int mv88e6xxx_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
 	if (scaled_ppm == 0)
@@ -95,16 +243,163 @@ static int mv88e6xxx_ptp_settime(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int mv88e6xxx_ptp_enable_extts(struct mv88e6xxx_chip *chip,
+				      struct ptp_clock_request *rq, int on)
+{
+	int rising = (rq->extts.flags & PTP_RISING_EDGE);
+	int pin;
+	int err;
+
+	pin = ptp_find_pin(chip->ptp_clock, PTP_PF_EXTTS, rq->extts.index);
+
+	if (pin < 0)
+		return -EBUSY;
+
+	mutex_lock(&chip->reg_lock);
+
+	if (on) {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_EVREQ,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
+		if (err)
+			goto out;
+
+		schedule_delayed_work(&chip->tai_event_work,
+				      TAI_EVENT_WORK_INTERVAL);
+
+		err = mv88e6xxx_config_eventcap(chip, PTP_CLOCK_EXTTS,
+						rising);
+	} else {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
+
+		cancel_delayed_work_sync(&chip->tai_event_work);
+	}
+
+out:
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static int mv88e6xxx_ptp_enable_perout(struct mv88e6xxx_chip *chip,
+				       struct ptp_clock_request *rq, int on)
+{
+	struct timespec ts;
+	u64 ns;
+	int pin;
+	int err;
+
+	pin = ptp_find_pin(chip->ptp_clock, PTP_PF_PEROUT, rq->extts.index);
+
+	if (pin < 0)
+		return -EBUSY;
+
+	ts.tv_sec = rq->perout.period.sec;
+	ts.tv_nsec = rq->perout.period.nsec;
+	ns = timespec_to_ns(&ts);
+
+	if (ns > U32_MAX)
+		return -ERANGE;
+
+	mutex_lock(&chip->reg_lock);
+
+	err = mv88e6xxx_config_periodic_trig(chip, (u32)ns, 0);
+	if (err)
+		goto out;
+
+	if (on) {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_TRIG,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_OUT);
+	} else {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
+	}
+
+out:
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static int mv88e6xxx_ptp_enable_pps(struct mv88e6xxx_chip *chip,
+				    struct ptp_clock_request *rq, int on)
+{
+	int pin;
+	int err;
+
+	pin = ptp_find_pin(chip->ptp_clock, PTP_PF_PEROUT, rq->extts.index);
+
+	if (pin < 0)
+		return -EBUSY;
+
+	mutex_lock(&chip->reg_lock);
+
+	if (on) {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_TRIG,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_OUT);
+		if (err)
+			goto out;
+		err = mv88e6xxx_config_periodic_trig(chip,
+						     NSEC_PER_SEC, 0);
+		if (err)
+			goto out;
+
+		schedule_delayed_work(&chip->tai_event_work, 0);
+
+		err = mv88e6xxx_config_eventcap(chip, PTP_CLOCK_PPS, 1);
+	} else {
+		err = mv88e6xxx_g2_set_gpio_config(
+			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO,
+			MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
+		if (err)
+			goto out;
+
+		err = mv88e6xxx_disable_trig(chip);
+
+		cancel_delayed_work_sync(&chip->tai_event_work);
+	}
+
+out:
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
 static int mv88e6xxx_ptp_enable(struct ptp_clock_info *ptp,
 				struct ptp_clock_request *rq, int on)
 {
-	return -EOPNOTSUPP;
+	struct mv88e6xxx_chip *chip =
+		container_of(ptp, struct mv88e6xxx_chip, ptp_clock_info);
+
+	switch (rq->type) {
+	case PTP_CLK_REQ_EXTTS:
+		return mv88e6xxx_ptp_enable_extts(chip, rq, on);
+	case PTP_CLK_REQ_PEROUT:
+		return mv88e6xxx_ptp_enable_perout(chip, rq, on);
+	case PTP_CLK_REQ_PPS:
+		return mv88e6xxx_ptp_enable_pps(chip, rq, on);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int mv88e6xxx_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin,
 				enum ptp_pin_function func, unsigned int chan)
 {
-	return -EOPNOTSUPP;
+	switch (func) {
+	case PTP_PF_NONE:
+	case PTP_PF_EXTTS:
+	case PTP_PF_PEROUT:
+		break;
+	case PTP_PF_PHYSYNC:
+		return -EOPNOTSUPP;
+	}
+	return 0;
 }
 
 /* The 32-bit timestamp counter overflows every ~34.3 seconds; this task
@@ -132,6 +427,8 @@ static void mv88e6xxx_ptp_overflow_check(struct work_struct *work)
 
 int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
 {
+	int i;
+
 	/* Set up the cycle counter */
 	memset(&chip->tstamp_cc, 0, sizeof(chip->tstamp_cc));
 	chip->tstamp_cc.read	= mv88e6xxx_ptp_clock_read;
@@ -146,12 +443,27 @@ int mv88e6xxx_ptp_setup(struct mv88e6xxx_chip *chip)
 	chip->last_overflow_check = jiffies;
 
 	INIT_DELAYED_WORK(&chip->overflow_work, mv88e6xxx_ptp_overflow_check);
+	INIT_DELAYED_WORK(&chip->tai_event_work, mv88e6xxx_tai_event_work);
 
 	chip->ptp_clock_info.owner = THIS_MODULE;
 	snprintf(chip->ptp_clock_info.name, sizeof(chip->ptp_clock_info.name),
 		 dev_name(chip->dev));
 	chip->ptp_clock_info.max_adj	= 0;
 
+	chip->ptp_clock_info.n_ext_ts	= 1;
+	chip->ptp_clock_info.n_per_out	= 1;
+	chip->ptp_clock_info.n_pins	= mv88e6xxx_num_gpio(chip);
+	chip->ptp_clock_info.pps	= 1;
+
+	for (i = 0; i < chip->ptp_clock_info.n_pins; ++i) {
+		struct ptp_pin_desc *ppd = &chip->pin_config[i];
+
+		snprintf(ppd->name, sizeof(ppd->name), "mv88e6xxx_gpio%d", i);
+		ppd->index = i;
+		ppd->func = PTP_PF_NONE;
+	}
+	chip->ptp_clock_info.pin_config = chip->pin_config;
+
 	chip->ptp_clock_info.adjfine	= mv88e6xxx_ptp_adjfine;
 	chip->ptp_clock_info.adjtime	= mv88e6xxx_ptp_adjtime;
 	chip->ptp_clock_info.gettime64	= mv88e6xxx_ptp_gettime;
@@ -173,6 +485,7 @@ void mv88e6xxx_ptp_free(struct mv88e6xxx_chip *chip)
 {
 	if (chip->ptp_clock) {
 		cancel_delayed_work_sync(&chip->overflow_work);
+		cancel_delayed_work_sync(&chip->tai_event_work);
 
 		ptp_clock_unregister(chip->ptp_clock);
 		chip->ptp_clock = NULL;
diff --git a/drivers/net/dsa/mv88e6xxx/ptp.h b/drivers/net/dsa/mv88e6xxx/ptp.h
index c662953..33b1842 100644
--- a/drivers/net/dsa/mv88e6xxx/ptp.h
+++ b/drivers/net/dsa/mv88e6xxx/ptp.h
@@ -21,6 +21,18 @@
 
 /* Offset 0x00: TAI Global Config */
 #define MV88E6XXX_TAI_CFG			0x00
+#define MV88E6XXX_TAI_CFG_CAP_OVERWRITE		0x8000
+#define MV88E6XXX_TAI_CFG_CAP_CTR_START		0x4000
+#define MV88E6XXX_TAI_CFG_EVREQ_FALLING		0x2000
+#define MV88E6XXX_TAI_CFG_TRIG_ACTIVE_LO	0x1000
+#define MV88E6XXX_TAI_CFG_IRL_ENABLE		0x0400
+#define MV88E6XXX_TAI_CFG_TRIG_IRQ_EN		0x0200
+#define MV88E6XXX_TAI_CFG_EVREQ_IRQ_EN		0x0100
+#define MV88E6XXX_TAI_CFG_TRIG_LOCK		0x0080
+#define MV88E6XXX_TAI_CFG_BLOCK_UPDATE		0x0008
+#define MV88E6XXX_TAI_CFG_MULTI_PTP		0x0004
+#define MV88E6XXX_TAI_CFG_TRIG_MODE_ONESHOT	0x0002
+#define MV88E6XXX_TAI_CFG_TRIG_ENABLE		0x0001
 
 /* Offset 0x01: Timestamp Clock Period (ps) */
 #define MV88E6XXX_TAI_CLOCK_PERIOD		0x01
@@ -46,6 +58,10 @@
 
 /* Offset 0x09: Event Status */
 #define MV88E6XXX_TAI_EVENT_STATUS		0x09
+#define MV88E6XXX_TAI_EVENT_STATUS_CAP_TRIG	0x4000
+#define MV88E6XXX_TAI_EVENT_STATUS_ERROR	0x0200
+#define MV88E6XXX_TAI_EVENT_STATUS_VALID	0x0100
+#define MV88E6XXX_TAI_EVENT_STATUS_CTR_MASK	0x00ff
 
 /* Offset 0x0A/0x0B: Event Time */
 #define MV88E6XXX_TAI_EVENT_TIME_LO		0x0a
-- 
2.1.4

Re: [PATCH net-next RFC 4/9] net: dsa: mv88e6xxx: add support for event capture

[Richard Cochran]

There are some issues here.

On Thu, Sep 28, 2017 at 10:25:36AM -0500, Brandon Streiff wrote:
> +static int mv88e6xxx_config_periodic_trig(struct mv88e6xxx_chip *chip,
> +					  u32 ns, u16 picos)
> +{
> +	int err;
> +	u16 global_config;
> +
> +	if (picos >= 1000)
> +		return -ERANGE;
> +
> +	/* TRIG generation is in units of 8 ns clock periods. Convert ns
> +	 * and ps into 8 ns clock periods and up to 8000 additional ps
> +	 */
> +	picos += (ns & 0x7) * 1000;
> +	ns = ns >> 3;

Again, the 8 nanosecounds shouldn't be hard coded.

	...

> +	return err;
> +}

> +static void mv88e6xxx_tai_event_work(struct work_struct *ugly)
> +{
> +	struct delayed_work *dw = to_delayed_work(ugly);
> +	struct mv88e6xxx_chip *chip =
> +		container_of(dw, struct mv88e6xxx_chip, tai_event_work);
> +	u16 ev_status[4];
> +	int err;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	err = mv88e6xxx_tai_read(chip, MV88E6XXX_TAI_EVENT_STATUS,
> +				 ev_status, ARRAY_SIZE(ev_status));
> +	if (err) {
> +		mutex_unlock(&chip->reg_lock);
> +		return;
> +	}
> +
> +	if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_ERROR)
> +		dev_warn(chip->dev, "missed event capture\n");
> +
> +	if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_VALID) {

Avoid IfOk.

> +		struct ptp_clock_event ev;
> +		u32 raw_ts = ((u32)ev_status[2] << 16) | ev_status[1];
> +
> +		/* Clear the valid bit so the next timestamp can come in */
> +		ev_status[0] &= ~MV88E6XXX_TAI_EVENT_STATUS_VALID;
> +		err = mv88e6xxx_tai_write(chip, MV88E6XXX_TAI_EVENT_STATUS,
> +					  ev_status[0]);
> +
> +		if (ev_status[0] & MV88E6XXX_TAI_EVENT_STATUS_CAP_TRIG) {
> +			/* TAI is configured to timestamp internal events.
> +			 * This will be a PPS event.
> +			 */
> +			ev.type = PTP_CLOCK_PPS;
> +		} else {
> +			/* Otherwise this is an external timestamp */
> +			ev.type = PTP_CLOCK_EXTTS;
> +		}
> +		/* We only have one timestamping channel. */
> +		ev.index = 0;
> +		ev.timestamp = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
> +
> +		ptp_clock_event(chip->ptp_clock, &ev);
> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	schedule_delayed_work(&chip->tai_event_work, TAI_EVENT_WORK_INTERVAL);
> +}
> +

> +static int mv88e6xxx_ptp_enable_perout(struct mv88e6xxx_chip *chip,
> +				       struct ptp_clock_request *rq, int on)
> +{
> +	struct timespec ts;
> +	u64 ns;
> +	int pin;
> +	int err;
> +
> +	pin = ptp_find_pin(chip->ptp_clock, PTP_PF_PEROUT, rq->extts.index);
> +
> +	if (pin < 0)
> +		return -EBUSY;
> +
> +	ts.tv_sec = rq->perout.period.sec;
> +	ts.tv_nsec = rq->perout.period.nsec;
> +	ns = timespec_to_ns(&ts);
> +
> +	if (ns > U32_MAX)
> +		return -ERANGE;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	err = mv88e6xxx_config_periodic_trig(chip, (u32)ns, 0);

Here you ignore the phase of the signal given in the trq->perout.start
field.  That is not what the user expects.  For periodic outputs where
the phase cannot be set, we really would need a new ioctl.

However, in this case, you should just drop this functionality.  I
understand that this works with your adjustable external oscillator,
but we cannot support that in mainline (at least, not yet).

Thanks,
Richard


> +	if (err)
> +		goto out;
> +
> +	if (on) {
> +		err = mv88e6xxx_g2_set_gpio_config(
> +			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_TRIG,
> +			MV88E6XXX_G2_SCRATCH_GPIO_DIR_OUT);
> +	} else {
> +		err = mv88e6xxx_g2_set_gpio_config(
> +			chip, pin, MV88E6XXX_G2_SCRATCH_GPIO_MODE_GPIO,
> +			MV88E6XXX_G2_SCRATCH_GPIO_DIR_IN);
> +	}
> +
> +out:
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return err;
> +}

Re: [PATCH net-next RFC 4/9] net: dsa: mv88e6xxx: add support for event capture

[Levi Pearson]

On Sun, Oct 8, 2017 at 9:06 AM, Richard Cochran
<richardcochran@gmail.com> wrote:

>> +static int mv88e6xxx_ptp_enable_perout(struct mv88e6xxx_chip *chip,
>> +                                    struct ptp_clock_request *rq, int on)
>> +{
>> +     struct timespec ts;
>> +     u64 ns;
>> +     int pin;
>> +     int err;
>> +
>> +     pin = ptp_find_pin(chip->ptp_clock, PTP_PF_PEROUT, rq->extts.index);
>> +
>> +     if (pin < 0)
>> +             return -EBUSY;
>> +
>> +     ts.tv_sec = rq->perout.period.sec;
>> +     ts.tv_nsec = rq->perout.period.nsec;
>> +     ns = timespec_to_ns(&ts);
>> +
>> +     if (ns > U32_MAX)
>> +             return -ERANGE;
>> +
>> +     mutex_lock(&chip->reg_lock);
>> +
>> +     err = mv88e6xxx_config_periodic_trig(chip, (u32)ns, 0);
>
> Here you ignore the phase of the signal given in the trq->perout.start
> field.  That is not what the user expects.  For periodic outputs where
> the phase cannot be set, we really would need a new ioctl.
>
> However, in this case, you should just drop this functionality.  I
> understand that this works with your adjustable external oscillator,
> but we cannot support that in mainline (at least, not yet).

I've been working with this patchset and just came across this
limitation as well. The periodic timer output is the basis of the Qbv
t0 timer in devices that support Qbv, and setting this up with the
correct start time is pretty important in that context. The hardware
does support setting a start time, but it must be specified according
to the cycle count of the free-running timer rather than a nanosecond
value. I think this can be worked out from the values stored in the
timecounter struct and I'm writing some code for it now, but if you've
already written something I'd be happy to integrate that instead.

Another issue related to this is that while the free-running counter
in the hardware can't be easily adjusted, the periodic event generator
*can* be finely adjusted (via picosecond and sub-picosecond
accumulators) to correct for drift between the local clock and the PTP
grandmaster time. So to be semantically correct, this needs to be both
started at the right time *and* it needs to have the periodic
corrections made so that the fine correction parameters in the
hardware keep it adjusted to be synchronous with PTP grandmaster time.

So, taking this functionality out in the first pass seems like a good
move for Brandon to take, but I'm working on a complete implementation
for it. I think I've got a handle on how to do it, but if you have any
suggestions, I'm open to them.


Levi

Re: [PATCH net-next RFC 4/9] net: dsa: mv88e6xxx: add support for event capture

[Richard Cochran]

On Mon, Oct 09, 2017 at 04:08:50PM -0600, Levi Pearson wrote:
> Another issue related to this is that while the free-running counter
> in the hardware can't be easily adjusted, the periodic event generator
> *can* be finely adjusted (via picosecond and sub-picosecond
> accumulators) to correct for drift between the local clock and the PTP
> grandmaster time. So to be semantically correct, this needs to be both
> started at the right time *and* it needs to have the periodic
> corrections made so that the fine correction parameters in the
> hardware keep it adjusted to be synchronous with PTP grandmaster time.

So if the accumulators are safe to adjust on the fly, then the
adjfine() method will have to program them with every adjustment.

Thanks,
Richard

[PATCH net-next RFC 5/9] net: dsa: forward hardware timestamping ioctls to switch driver

[Brandon Streiff]

This patch adds support to the dsa slave network device so that
switch drivers can implement the SIOC[GS]HWTSTAMP ioctls and the
ethtool timestamp-info interface.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 include/net/dsa.h | 15 +++++++++++++++
 net/dsa/slave.c   | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8dee216..1163af1 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -19,6 +19,7 @@
 #include <linux/workqueue.h>
 #include <linux/of.h>
 #include <linux/ethtool.h>
+#include <linux/net_tstamp.h>
 #include <net/devlink.h>
 #include <net/switchdev.h>
 
@@ -329,6 +330,12 @@ struct dsa_switch_ops {
 			   struct ethtool_wolinfo *w);
 
 	/*
+	 * ethtool timestamp info
+	 */
+	int	(*get_ts_info)(struct dsa_switch *ds, int port,
+			       struct ethtool_ts_info *ts);
+
+	/*
 	 * Suspend and resume
 	 */
 	int	(*suspend)(struct dsa_switch *ds);
@@ -434,6 +441,14 @@ struct dsa_switch_ops {
 					 int port, struct net_device *br);
 	void	(*crosschip_bridge_leave)(struct dsa_switch *ds, int sw_index,
 					  int port, struct net_device *br);
+
+	/*
+	 * PTP functionality
+	 */
+	int	(*port_hwtstamp_get)(struct dsa_switch *ds, int port,
+				     struct ifreq *ifr);
+	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
+				     struct ifreq *ifr);
 };
 
 struct dsa_switch_driver {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index bf8800d..2cf6a83 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -264,10 +264,34 @@ dsa_slave_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->dp->ds;
+	int port = p->dp->index;
+
 	if (!dev->phydev)
 		return -ENODEV;
 
-	return phy_mii_ioctl(dev->phydev, ifr, cmd);
+	switch (cmd) {
+	case SIOCGMIIPHY:
+	case SIOCGMIIREG:
+	case SIOCSMIIREG:
+		if (dev->phydev)
+			return phy_mii_ioctl(dev->phydev, ifr, cmd);
+		else
+			return -EOPNOTSUPP;
+	case SIOCGHWTSTAMP:
+		if (ds->ops->port_hwtstamp_get)
+			return ds->ops->port_hwtstamp_get(ds, port, ifr);
+		else
+			return -EOPNOTSUPP;
+	case SIOCSHWTSTAMP:
+		if (ds->ops->port_hwtstamp_set)
+			return ds->ops->port_hwtstamp_set(ds, port, ifr);
+		else
+			return -EOPNOTSUPP;
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
 static int dsa_slave_port_attr_set(struct net_device *dev,
@@ -876,6 +900,18 @@ static int dsa_slave_set_rxnfc(struct net_device *dev,
 	return ds->ops->set_rxnfc(ds, p->dp->index, nfc);
 }
 
+static int dsa_slave_get_ts_info(struct net_device *dev,
+				 struct ethtool_ts_info *ts)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->dp->ds;
+
+	if (!ds->ops->get_ts_info)
+		return -EOPNOTSUPP;
+
+	return ds->ops->get_ts_info(ds, p->dp->index, ts);
+}
+
 static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_drvinfo		= dsa_slave_get_drvinfo,
 	.get_regs_len		= dsa_slave_get_regs_len,
@@ -896,6 +932,7 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
 	.get_rxnfc		= dsa_slave_get_rxnfc,
 	.set_rxnfc		= dsa_slave_set_rxnfc,
+	.get_ts_info		= dsa_slave_get_ts_info,
 };
 
 static const struct net_device_ops dsa_slave_netdev_ops = {
-- 
2.1.4

Re: [PATCH net-next RFC 5/9] net: dsa: forward hardware timestamping ioctls to switch driver

[Florian Fainelli]

On 09/28/2017 08:25 AM, Brandon Streiff wrote:
> This patch adds support to the dsa slave network device so that
> switch drivers can implement the SIOC[GS]HWTSTAMP ioctls and the
> ethtool timestamp-info interface.
> 
> Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
> ---

>  struct dsa_switch_driver {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index bf8800d..2cf6a83 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -264,10 +264,34 @@ dsa_slave_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>  
>  static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  {
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->dp->ds;
> +	int port = p->dp->index;
> +
>  	if (!dev->phydev)
>  		return -ENODEV;
>  
> -	return phy_mii_ioctl(dev->phydev, ifr, cmd);
> +	switch (cmd) {
> +	case SIOCGMIIPHY:
> +	case SIOCGMIIREG:
> +	case SIOCSMIIREG:
> +		if (dev->phydev)
> +			return phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		else
> +			return -EOPNOTSUPP;
> +	case SIOCGHWTSTAMP:
> +		if (ds->ops->port_hwtstamp_get)
> +			return ds->ops->port_hwtstamp_get(ds, port, ifr);
> +		else
> +			return -EOPNOTSUPP;
> +	case SIOCSHWTSTAMP:
> +		if (ds->ops->port_hwtstamp_set)
> +			return ds->ops->port_hwtstamp_set(ds, port, ifr);
> +		else
> +			return -EOPNOTSUPP;
> +	default:
> +		return -EOPNOTSUPP;
> +	}

This echoes back to Andrew's comments in patch 2, but we may have to
prefer PHY timestamping over MAC timestamping if both are available?
Richard, is that usually how the preference should be made?
--
Florian

Re: [PATCH net-next RFC 5/9] net: dsa: forward hardware timestamping ioctls to switch driver

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:34AM -0700, Florian Fainelli wrote:
> This echoes back to Andrew's comments in patch 2, but we may have to
> prefer PHY timestamping over MAC timestamping if both are available?
> Richard, is that usually how the preference should be made?

No, if the MAC supports time stamping, then it will take precedence,
because the MAC driver doesn't know that the PHY also supports this.
In the case where a board design includes the PHYTER (the one and only
PHY PHC) and a MAC PHC, the user must de-select the MAC support in the
Kconfig in order to use the PHYTER.

So in general, we don't support PHC/timestamping simultaneously in the
MAC and PHY.  It would be a lot of work to support this, and the user
timestamping API would have to be extended yet again, and so I think
it is not worth the effort.

Getting back to this patch, it should fall back to PHY timestamping
when the switch device doesn't support timestamping:

	case SIOCGHWTSTAMP:
		if (ds->ops->port_hwtstamp_get)
			return ds->ops->port_hwtstamp_get(ds, port, ifr);
		else
			return phy_mii_ioctl(dev->phydev, ifr, cmd);

That way, if someone combines a PHYTER with a non-PTP capable switch,
it will just work.

Thanks,
Richard

Re: [PATCH net-next RFC 5/9] net: dsa: forward hardware timestamping ioctls to switch driver

[Vivien Didelot]

Hi Brandon,

Brandon Streiff <brandon.streiff@ni.com> writes:

>  static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  {
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->dp->ds;
> +	int port = p->dp->index;
> +
>  	if (!dev->phydev)
>  		return -ENODEV;

Move this check below:

>  
> -	return phy_mii_ioctl(dev->phydev, ifr, cmd);
> +	switch (cmd) {
> +	case SIOCGMIIPHY:
> +	case SIOCGMIIREG:
> +	case SIOCSMIIREG:
> +		if (dev->phydev)
> +			return phy_mii_ioctl(dev->phydev, ifr, cmd);
> +		else
> +			return -EOPNOTSUPP;

                if (!dev->phydev)
                        return -ENODEV;

                return phy_mii_ioctl(dev->phydev, ifr, cmd);

> +	case SIOCGHWTSTAMP:
> +		if (ds->ops->port_hwtstamp_get)
> +			return ds->ops->port_hwtstamp_get(ds, port, ifr);
> +		else
> +			return -EOPNOTSUPP;

Here you can replace the else statement with break;

> +	case SIOCSHWTSTAMP:
> +		if (ds->ops->port_hwtstamp_set)
> +			return ds->ops->port_hwtstamp_set(ds, port, ifr);
> +		else
> +			return -EOPNOTSUPP;

Same here;

> +	default:
> +		return -EOPNOTSUPP;
> +	}

Then drop the default case and return -EOPNOTSUPP after the switch.

>  }


Thanks,

        Vivien

[PATCH net-next RFC 6/9] net: dsa: forward timestamping callbacks to switch drivers

[Brandon Streiff]

Forward the rx/tx timestamp machinery from the dsa infrastructure to the
switch driver.

On the rx side, defer delivery of skbs until we have an rx timestamp.
This mimicks the behavior of skb_defer_rx_timestamp. The implementation
does have to thread through the tagging protocol handlers because
it is where that we know which switch and port the skb goes to.

On the tx side, identify PTP packets, clone them, and pass them to the
underlying switch driver before we transmit. This mimicks the behavior
of skb_tx_timestamp.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 include/net/dsa.h     | 13 +++++++++++--
 net/dsa/dsa.c         | 39 ++++++++++++++++++++++++++++++++++++++-
 net/dsa/slave.c       | 25 +++++++++++++++++++++++++
 net/dsa/tag_brcm.c    |  6 +++++-
 net/dsa/tag_dsa.c     |  6 +++++-
 net/dsa/tag_edsa.c    |  6 +++++-
 net/dsa/tag_ksz.c     |  6 +++++-
 net/dsa/tag_lan9303.c |  6 +++++-
 net/dsa/tag_mtk.c     |  6 +++++-
 net/dsa/tag_qca.c     |  6 +++++-
 net/dsa/tag_trailer.c |  6 +++++-
 11 files changed, 114 insertions(+), 11 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 1163af1..4daf7f7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -101,11 +101,14 @@ struct dsa_platform_data {
 };
 
 struct packet_type;
+struct dsa_switch;
 
 struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt);
+			       struct packet_type *pt,
+			       struct dsa_switch **src_dev,
+			       int *src_port);
 	int (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
 			    int *offset);
 };
@@ -134,7 +137,9 @@ struct dsa_switch_tree {
 	/* Copy of tag_ops->rcv for faster access in hot path */
 	struct sk_buff *	(*rcv)(struct sk_buff *skb,
 				       struct net_device *dev,
-				       struct packet_type *pt);
+				       struct packet_type *pt,
+				       struct dsa_switch **src_dev,
+				       int *src_port);
 
 	/*
 	 * The switch port to which the CPU is attached.
@@ -449,6 +454,10 @@ struct dsa_switch_ops {
 				     struct ifreq *ifr);
 	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
 				     struct ifreq *ifr);
+	void	(*port_txtstamp)(struct dsa_switch *ds, int port,
+				 struct sk_buff *clone, unsigned int type);
+	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
+				 struct sk_buff *skb, unsigned int type);
 };
 
 struct dsa_switch_driver {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 81c852e..42e7286 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -22,6 +22,7 @@
 #include <linux/netdevice.h>
 #include <linux/sysfs.h>
 #include <linux/phy_fixed.h>
+#include <linux/ptp_classify.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
 
@@ -157,6 +158,37 @@ struct net_device *dsa_dev_to_net_device(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dsa_dev_to_net_device);
 
+/* Determine if we should defer delivery of skb until we have a rx timestamp.
+ *
+ * Called from dsa_switch_rcv. For now, this will only work if tagging is
+ * enabled on the switch. Normally the MAC driver would retrieve the hardware
+ * timestamp when it reads the packet out of the hardware. However in a DSA
+ * switch, the DSA driver owning the interface to which the packet is
+ * delivered is never notified unless we do so here.
+ */
+static bool dsa_skb_defer_rx_timestamp(struct dsa_switch *ds, int port,
+				       struct sk_buff *skb)
+{
+	unsigned int type;
+
+	if (skb_headroom(skb) < ETH_HLEN)
+		return false;
+
+	__skb_push(skb, ETH_HLEN);
+
+	type = ptp_classify_raw(skb);
+
+	__skb_pull(skb, ETH_HLEN);
+
+	if (type == PTP_CLASS_NONE)
+		return false;
+
+	if (likely(ds->ops->port_rxtstamp))
+		return ds->ops->port_rxtstamp(ds, port, skb, type);
+
+	return false;
+}
+
 static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 			  struct packet_type *pt, struct net_device *unused)
 {
@@ -164,6 +196,8 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	struct sk_buff *nskb = NULL;
 	struct pcpu_sw_netstats *s;
 	struct dsa_slave_priv *p;
+	struct dsa_switch *ds = NULL;
+	int source_port;
 
 	if (unlikely(dst == NULL)) {
 		kfree_skb(skb);
@@ -174,7 +208,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
-	nskb = dst->rcv(skb, dev, pt);
+	nskb = dst->rcv(skb, dev, pt, &ds, &source_port);
 	if (!nskb) {
 		kfree_skb(skb);
 		return 0;
@@ -192,6 +226,9 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	s->rx_bytes += skb->len;
 	u64_stats_update_end(&s->syncp);
 
+	if (dsa_skb_defer_rx_timestamp(ds, source_port, skb))
+		return 0;
+
 	netif_receive_skb(skb);
 
 	return 0;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2cf6a83..a278335 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -22,6 +22,7 @@
 #include <net/tc_act/tc_mirred.h>
 #include <linux/if_bridge.h>
 #include <linux/netpoll.h>
+#include <linux/ptp_classify.h>
 
 #include "dsa_priv.h"
 
@@ -407,6 +408,25 @@ static inline netdev_tx_t dsa_slave_netpoll_send_skb(struct net_device *dev,
 	return NETDEV_TX_OK;
 }
 
+static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
+				 struct sk_buff *skb)
+{
+	struct dsa_switch *ds = p->dp->ds;
+	struct sk_buff *clone;
+	unsigned int type;
+
+	type = ptp_classify_raw(skb);
+	if (type == PTP_CLASS_NONE)
+		return;
+
+	if (ds->ops->port_txtstamp) {
+		clone = skb_clone_sk(skb);
+		if (!clone)
+			return;
+		ds->ops->port_txtstamp(ds, p->dp->index, clone, type);
+	}
+}
+
 static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -419,6 +439,11 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	s->tx_bytes += skb->len;
 	u64_stats_update_end(&s->syncp);
 
+	/* Identify PTP protocol packets, clone them, and pass them to the
+	 * switch driver
+	 */
+	dsa_skb_tx_timestamp(p, skb);
+
 	/* Transmit function may have to reallocate the original SKB,
 	 * in which case it must have freed it. Only free it here on error.
 	 */
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index dbb0164..1c8b1c9 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -90,7 +90,8 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
 }
 
 static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				    struct packet_type *pt)
+				    struct packet_type *pt,
+				    struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
@@ -131,6 +132,9 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index fbf9ca9..2abcd01 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -65,7 +65,8 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt)
+			       struct packet_type *pt,
+			       struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
@@ -155,6 +156,9 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 76367ba..bc529e5 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -78,7 +78,8 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
-				struct packet_type *pt)
+				struct packet_type *pt,
+				struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
@@ -174,6 +175,9 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 010ca0a..7760862 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -78,7 +78,8 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt)
+			       struct packet_type *pt,
+			       struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
@@ -96,6 +97,9 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 0b98261..adabd48 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -68,7 +68,8 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
-			struct packet_type *pt)
+			struct packet_type *pt,
+			struct dsa_switch **src_dev, int *src_port)
 {
 	u16 *lan9303_tag;
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
@@ -123,6 +124,9 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 	/* forward the packet to the dedicated interface */
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index ec8ee5f..321d1a6 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -44,7 +44,8 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 }
 
 static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt)
+				   struct packet_type *pt,
+				   struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
@@ -83,6 +84,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[port].netdev;
 
+	*src_dev = ds;
+	*src_port = port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 1d4c707..de51fe6e 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -63,7 +63,8 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt)
+				   struct packet_type *pt,
+				   struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
@@ -107,6 +108,9 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	/* Update skb & forward the frame accordingly */
 	skb->dev = ds->ports[port].netdev;
 
+	*src_dev = ds;
+	*src_port = port;
+
 	return skb;
 }
 
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index d2fd492..ca5960f 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -56,7 +56,8 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt)
+				   struct packet_type *pt,
+				   struct dsa_switch **src_dev, int *src_port)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_port *cpu_dp = dsa_get_cpu_port(dst);
@@ -80,6 +81,9 @@ static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = ds->ports[source_port].netdev;
 
+	*src_dev = ds;
+	*src_port = source_port;
+
 	return skb;
 }
 
-- 
2.1.4

Re: [PATCH net-next RFC 6/9] net: dsa: forward timestamping callbacks to switch drivers

[Florian Fainelli]

On 09/28/2017 08:25 AM, Brandon Streiff wrote:
> Forward the rx/tx timestamp machinery from the dsa infrastructure to the
> switch driver.
> 
> On the rx side, defer delivery of skbs until we have an rx timestamp.
> This mimicks the behavior of skb_defer_rx_timestamp. The implementation
> does have to thread through the tagging protocol handlers because
> it is where that we know which switch and port the skb goes to.
> 
> On the tx side, identify PTP packets, clone them, and pass them to the
> underlying switch driver before we transmit. This mimicks the behavior
> of skb_tx_timestamp.
> 
> Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
> ---
>  include/net/dsa.h     | 13 +++++++++++--
>  net/dsa/dsa.c         | 39 ++++++++++++++++++++++++++++++++++++++-
>  net/dsa/slave.c       | 25 +++++++++++++++++++++++++
>  net/dsa/tag_brcm.c    |  6 +++++-
>  net/dsa/tag_dsa.c     |  6 +++++-
>  net/dsa/tag_edsa.c    |  6 +++++-
>  net/dsa/tag_ksz.c     |  6 +++++-
>  net/dsa/tag_lan9303.c |  6 +++++-
>  net/dsa/tag_mtk.c     |  6 +++++-
>  net/dsa/tag_qca.c     |  6 +++++-
>  net/dsa/tag_trailer.c |  6 +++++-
>  11 files changed, 114 insertions(+), 11 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 1163af1..4daf7f7 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -101,11 +101,14 @@ struct dsa_platform_data {
>  };
>  
>  struct packet_type;
> +struct dsa_switch;
>  
>  struct dsa_device_ops {
>  	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
>  	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
> -			       struct packet_type *pt);
> +			       struct packet_type *pt,
> +			       struct dsa_switch **src_dev,
> +			       int *src_port);
>  	int (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
>  			    int *offset);
>  };
> @@ -134,7 +137,9 @@ struct dsa_switch_tree {
>  	/* Copy of tag_ops->rcv for faster access in hot path */
>  	struct sk_buff *	(*rcv)(struct sk_buff *skb,
>  				       struct net_device *dev,
> -				       struct packet_type *pt);
> +				       struct packet_type *pt,
> +				       struct dsa_switch **src_dev,
> +				       int *src_port);
>  
>  	/*
>  	 * The switch port to which the CPU is attached.
> @@ -449,6 +454,10 @@ struct dsa_switch_ops {
>  				     struct ifreq *ifr);
>  	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
>  				     struct ifreq *ifr);
> +	void	(*port_txtstamp)(struct dsa_switch *ds, int port,
> +				 struct sk_buff *clone, unsigned int type);
> +	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
> +				 struct sk_buff *skb, unsigned int type);
>  };
>  
>  struct dsa_switch_driver {
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 81c852e..42e7286 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -22,6 +22,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/sysfs.h>
>  #include <linux/phy_fixed.h>
> +#include <linux/ptp_classify.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/etherdevice.h>
>  
> @@ -157,6 +158,37 @@ struct net_device *dsa_dev_to_net_device(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dsa_dev_to_net_device);
>  
> +/* Determine if we should defer delivery of skb until we have a rx timestamp.
> + *
> + * Called from dsa_switch_rcv. For now, this will only work if tagging is
> + * enabled on the switch. Normally the MAC driver would retrieve the hardware
> + * timestamp when it reads the packet out of the hardware. However in a DSA
> + * switch, the DSA driver owning the interface to which the packet is
> + * delivered is never notified unless we do so here.
> + */
> +static bool dsa_skb_defer_rx_timestamp(struct dsa_switch *ds, int port,
> +				       struct sk_buff *skb)

You should not need the port information here because it's already
implied from skb->dev which points to the DSA slave network device, see
below.

> +{
> +	unsigned int type;
> +
> +	if (skb_headroom(skb) < ETH_HLEN)
> +		return false;

Are you positive this is necessary? Because we called dst->rcv() we have
called eth_type_trans() which already made sure about that

> +
> +	__skb_push(skb, ETH_HLEN);
> +
> +	type = ptp_classify_raw(skb);
> +
> +	__skb_pull(skb, ETH_HLEN);
> +
> +	if (type == PTP_CLASS_NONE)
> +		return false;
> +
> +	if (likely(ds->ops->port_rxtstamp))
> +		return ds->ops->port_rxtstamp(ds, port, skb, type);
> +
> +	return false;
> +}

Can we also have a fast-path bypass in case time stamping is not
supported by the switch so we don't have to even try to classify this
packet only to realize we don't have a port_rxtsamp() operation later?
You can either gate this with a compile-time option, or use e.g: a
static key or something like an early test?

> +
>  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  			  struct packet_type *pt, struct net_device *unused)
>  {
> @@ -164,6 +196,8 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	struct sk_buff *nskb = NULL;
>  	struct pcpu_sw_netstats *s;
>  	struct dsa_slave_priv *p;
> +	struct dsa_switch *ds = NULL;
> +	int source_port;
>  
>  	if (unlikely(dst == NULL)) {
>  		kfree_skb(skb);
> @@ -174,7 +208,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!skb)
>  		return 0;
>  
> -	nskb = dst->rcv(skb, dev, pt);
> +	nskb = dst->rcv(skb, dev, pt, &ds, &source_port);

I don't think this is necessary, what dst->rcv() does is actually
properly assign skb->dev to the correct dsa slave network device, which
has the information about the port number already in its private context.

>  	if (!nskb) {
>  		kfree_skb(skb);
>  		return 0;
> @@ -192,6 +226,9 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	s->rx_bytes += skb->len;
>  	u64_stats_update_end(&s->syncp);
>  
> +	if (dsa_skb_defer_rx_timestamp(ds, source_port, skb))
> +		return 0;

Can we just propagate an integer return value from
dsa_skb_defer_rx_timestamp()?

> +
>  	netif_receive_skb(skb);
>  
>  	return 0;
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 2cf6a83..a278335 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -22,6 +22,7 @@
>  #include <net/tc_act/tc_mirred.h>
>  #include <linux/if_bridge.h>
>  #include <linux/netpoll.h>
> +#include <linux/ptp_classify.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -407,6 +408,25 @@ static inline netdev_tx_t dsa_slave_netpoll_send_skb(struct net_device *dev,
>  	return NETDEV_TX_OK;
>  }
>  
> +static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
> +				 struct sk_buff *skb)
> +{
> +	struct dsa_switch *ds = p->dp->ds;
> +	struct sk_buff *clone;
> +	unsigned int type;
> +
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
> +		return;

If we don't have a port_txtstamp option, is there even value in
classifying this packet?
-- 
Florian

RE: [PATCH net-next RFC 6/9] net: dsa: forward timestamping callbacks to switch drivers

[Brandon Streiff]

> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Thursday, September 28, 2017 12:40 PM
>
> Can we also have a fast-path bypass in case time stamping is not
> supported by the switch so we don't have to even try to classify this
> packet only to realize we don't have a port_rxtsamp() operation later?
> You can either gate this with a compile-time option, or use e.g: a
> static key or something like an early test?

I was trying to follow the existing pattern for skb_defer_rx_timestamp, but that function be turned into a stub by not configuring NETWORK_PHY_TIMESTAMPING. Maybe a similar compile-time token is appropriate.

> >
> > -   nskb = dst->rcv(skb, dev, pt);
> > +   nskb = dst->rcv(skb, dev, pt, &ds, &source_port);
>
> I don't think this is necessary, what dst->rcv() does is actually
> properly assign skb->dev to the correct dsa slave network device, which
> has the information about the port number already in its private context.

Yes, looking in that private context seems like it'd be a better approach (and avoids having to touch all the taggers). I'll look into that further.

> > +   type = ptp_classify_raw(skb);
> > +   if (type == PTP_CLASS_NONE)
> > +           return;
>
> If we don't have a port_txtstamp option, is there even value in
> classifying this packet?

There isn't. This could also use a bypass just like the RX case.

-- brandon

[PATCH net-next RFC 7/9] ptp: add offset for reserved field to header

[Brandon Streiff]

There is a four-byte "reserved" field at octet 16 in PTPv2.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 include/linux/ptp_classify.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index a079656..b9b0073 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h
@@ -47,6 +47,7 @@
 #define PTP_EV_PORT 319
 #define PTP_GEN_BIT 0x08 /* indicates general message, if set in message type */
 
+#define OFF_PTP_RESERVED	16 /* PTPv2 only */
 #define OFF_PTP_SOURCE_UUID	22 /* PTPv1 only */
 #define OFF_PTP_SEQUENCE_ID	30
 #define OFF_PTP_CONTROL		32 /* PTPv1 only */
-- 
2.1.4

[PATCH net-next RFC 8/9] net: dsa: mv88e6xxx: add rx/tx timestamping support

[Brandon Streiff]

This patch implements RX/TX timestamping support.

The Marvell PTP hardware supports RX timestamping individual message
types, but for simplicity we only support the EVENT receive filter since
few if any clients bother with the more specific filter types.

We also utilize a feature of the "generation 3" PTP hardware that lets
us to embed the timestamp value into one of the reserved fields in the
PTP header. This lets us extract the timestamp out of the header and
avoid an SMI access in the RX codepath. (This implementation does not
presently support the older generations.)

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile   |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c     |  16 +-
 drivers/net/dsa/mv88e6xxx/chip.h     |  26 ++
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 535 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/hwtstamp.h | 162 +++++++++++
 5 files changed, 738 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/hwtstamp.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/hwtstamp.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index 47eda30..9bd0175 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -4,6 +4,7 @@ mv88e6xxx-objs += global1.o
 mv88e6xxx-objs += global1_atu.o
 mv88e6xxx-objs += global1_vtu.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
+mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += hwtstamp.o
 mv88e6xxx-objs += phy.o
 mv88e6xxx-objs += port.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4a37b26..9a0ee56 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -36,6 +36,7 @@
 #include "chip.h"
 #include "global1.h"
 #include "global2.h"
+#include "hwtstamp.h"
 #include "phy.h"
 #include "port.h"
 #include "ptp.h"
@@ -2034,11 +2035,15 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	if (err)
 		goto unlock;
 
-	/* Setup PTP Hardware Clock */
+	/* Setup PTP Hardware Clock and timestamping */
 	if (chip->info->ptp_support) {
 		err = mv88e6xxx_ptp_setup(chip);
 		if (err)
 			goto unlock;
+
+		err = mv88e6xxx_hwtstamp_setup(chip);
+		if (err)
+			goto unlock;
 	}
 
 unlock:
@@ -3851,6 +3856,11 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_mdb_del           = mv88e6xxx_port_mdb_del,
 	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
 	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
+	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
+	.port_hwtstamp_get	= mv88e6xxx_port_hwtstamp_get,
+	.port_txtstamp		= mv88e6xxx_port_txtstamp,
+	.port_rxtstamp		= mv88e6xxx_port_rxtstamp,
+	.get_ts_info		= mv88e6xxx_get_ts_info,
 };
 
 static struct dsa_switch_driver mv88e6xxx_switch_drv = {
@@ -3979,8 +3989,10 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
 	struct mv88e6xxx_chip *chip = ds->priv;
 
-	if (chip->info->ptp_support)
+	if (chip->info->ptp_support) {
+		mv88e6xxx_hwtstamp_free(chip);
 		mv88e6xxx_ptp_free(chip);
+	}
 
 	mv88e6xxx_phy_destroy(chip);
 	mv88e6xxx_unregister_switch(chip);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index b763778..52e449b 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -163,6 +163,29 @@ struct mv88e6xxx_irq {
 	unsigned int nirqs;
 };
 
+/* state flags for mv88e6xxx_port_hwtstamp::state */
+enum {
+	MV88E6XXX_HWTSTAMP_ENABLED,
+	MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
+};
+
+struct mv88e6xxx_port_hwtstamp {
+	/* Port index */
+	int port_id;
+
+	/* Timestamping state */
+	unsigned long state;
+
+	/* Resources for transmit timestamping */
+	struct work_struct tx_tstamp_work;
+	unsigned long tx_tstamp_start;
+	struct sk_buff *tx_skb;
+	u16 tx_seq_id;
+
+	/* Current timestamp configuration */
+	struct hwtstamp_config tstamp_config;
+};
+
 struct mv88e6xxx_chip {
 	const struct mv88e6xxx_info *info;
 
@@ -231,6 +254,9 @@ struct mv88e6xxx_chip {
 	struct ptp_pin_desc	pin_config[MV88E6XXX_MAX_GPIO];
 	u16 trig_config;
 	u16 evcap_config;
+
+	/* Per-port timestamping resources. */
+	struct mv88e6xxx_port_hwtstamp port_hwtstamp[DSA_MAX_PORTS];
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
new file mode 100644
index 0000000..825d6f0
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -0,0 +1,535 @@
+/*
+ * Marvell 88E6xxx Switch hardware timestamping support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 National Instruments
+ *      Erik Hons <erik.hons@ni.com>
+ *      Brandon Streiff <brandon.streiff@ni.com>
+ *      Dane Wagner <dane.wagner@ni.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include "chip.h"
+#include "global2.h"
+#include "hwtstamp.h"
+#include <linux/ptp_classify.h>
+
+static int mv88e6xxx_port_ptp_read(struct mv88e6xxx_chip *chip, int port,
+				   int addr, u16 *data, int len)
+{
+	if (!chip->info->ops->avb_ops->port_ptp_read)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->avb_ops->port_ptp_read(chip, port, addr,
+						       data, len);
+}
+
+static int mv88e6xxx_port_ptp_write(struct mv88e6xxx_chip *chip, int port,
+				    int addr, u16 data)
+{
+	if (!chip->info->ops->avb_ops->port_ptp_write)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->avb_ops->port_ptp_write(chip, port, addr,
+							data);
+}
+
+static int mv88e6xxx_ptp_write(struct mv88e6xxx_chip *chip, int addr,
+			       u16 data)
+{
+	if (!chip->info->ops->avb_ops->ptp_write)
+		return -EOPNOTSUPP;
+
+	return chip->info->ops->avb_ops->ptp_write(chip, addr, data);
+}
+
+/* TX_TSTAMP_TIMEOUT: This limits the time spent polling for a TX
+ * timestamp. When working properly, hardware will produce a timestamp
+ * within 1ms. Software may enounter delays due to MDIO contention, so
+ * the timeout is set accordingly.
+ */
+#define TX_TSTAMP_TIMEOUT	msecs_to_jiffies(20)
+
+int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
+			  struct ethtool_ts_info *info)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	if (!chip->info->ptp_support)
+		return -EOPNOTSUPP;
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+	info->phc_index = ptp_clock_index(chip->ptp_clock);
+	info->tx_types =
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON);
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_SYNC) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_DELAY_REQ);
+
+	return 0;
+}
+
+static int mv88e6xxx_set_hwtstamp_config(struct mv88e6xxx_chip *chip, int port,
+					 struct hwtstamp_config *config)
+{
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+	bool tstamp_enable = false;
+	u16 port_config0;
+	int err;
+
+	/* Prevent the TX/RX paths from trying to interact with the
+	 * timestamp hardware while we reconfigure it.
+	 */
+	clear_bit_unlock(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
+
+	/* reserved for future extensions */
+	if (config->flags)
+		return -EINVAL;
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		tstamp_enable = false;
+		break;
+	case HWTSTAMP_TX_ON:
+		tstamp_enable = true;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	/* The switch supports timestamping both L2 and L4; one cannot be
+	 * disabled independently of the other.
+	 */
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		tstamp_enable = false;
+		break;
+	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:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	default:
+		config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	if (tstamp_enable) {
+		/* Disable transportSpecific value matching, so that packets
+		 * with either 1588 (0) and 802.1AS (1) will be timestamped.
+		 */
+		port_config0 = MV88E6XXX_PORT_PTP_CFG0_DISABLE_TSPEC_MATCH;
+	} else {
+		/* Disable PTP. This disables both RX and TX timestamping. */
+		port_config0 = MV88E6XXX_PORT_PTP_CFG0_DISABLE_PTP;
+	}
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
+				       port_config0);
+	mutex_unlock(&chip->reg_lock);
+
+	if (err < 0)
+		return err;
+
+	/* Once hardware has been configured, enable timestamp checks
+	 * in the RX/TX paths.
+	 */
+	if (tstamp_enable)
+		set_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
+
+	return 0;
+}
+
+int mv88e6xxx_port_hwtstamp_set(struct dsa_switch *ds, int port,
+				struct ifreq *ifr)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+	struct hwtstamp_config config;
+	int err;
+
+	if (!chip->info->ptp_support)
+		return -EOPNOTSUPP;
+
+	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
+		return -EINVAL;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	err = mv88e6xxx_set_hwtstamp_config(chip, 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 mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
+				struct ifreq *ifr)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+	struct hwtstamp_config *config = &ps->tstamp_config;
+
+	if (!chip->info->ptp_support)
+		return -EOPNOTSUPP;
+
+	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
+		return -EINVAL;
+
+	return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ?
+		-EFAULT : 0;
+}
+
+/* Get the start of the PTP header in this skb */
+static u8 *_get_ptp_header(struct sk_buff *skb, unsigned int type)
+{
+	unsigned int offset = 0;
+	u8 *data = skb_mac_header(skb);
+
+	if (type & PTP_CLASS_VLAN)
+		offset += VLAN_HLEN;
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
+		break;
+	case PTP_CLASS_IPV6:
+		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
+		break;
+	case PTP_CLASS_L2:
+		offset += ETH_HLEN;
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Ensure that the entire header is present in this packet. */
+	if (skb->len + ETH_HLEN < offset + 34)
+		return ERR_PTR(-EINVAL);
+
+	return data + offset;
+}
+
+static bool mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
+				    struct sk_buff *skb, unsigned int type)
+{
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+	u8 *ptp_hdr, *msgtype;
+	bool ret;
+
+	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
+		return false;
+
+	ptp_hdr = _get_ptp_header(skb, type);
+	if (IS_ERR(ptp_hdr))
+		return false;
+
+	if (unlikely(type & PTP_CLASS_V1))
+		msgtype = ptp_hdr + OFF_PTP_CONTROL;
+	else
+		msgtype = ptp_hdr;
+
+	ret = test_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
+
+	dev_dbg(chip->dev,
+		"p%d: PTP message classification 0x%x type 0x%x, tstamp? %d",
+		port, type, *msgtype, (int)ret);
+
+	return ret;
+}
+
+/* rxtstamp will be called in interrupt context so we don't to do
+ * anything like read PTP registers over SMI.
+ */
+bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
+			     struct sk_buff *skb, unsigned int type)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct skb_shared_hwtstamps *shhwtstamps;
+	__be32 *ptp_rx_ts;
+	u8 *ptp_hdr;
+	u32 raw_ts;
+	u64 ns;
+
+	if (!chip->info->ptp_support)
+		return false;
+
+	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
+		return false;
+
+	if (!mv88e6xxx_should_tstamp(chip, port, skb, type))
+		return false;
+
+	shhwtstamps = skb_hwtstamps(skb);
+	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+
+	/* Because we configured the arrival timestamper to put the counter
+	 * into the 32-bit "reserved" field of the PTP header, we can retrieve
+	 * the value from the packet directly instead of having to retrieve it
+	 * via SMI.
+	 */
+	ptp_hdr = _get_ptp_header(skb, type);
+	if (IS_ERR(ptp_hdr))
+		return false;
+	ptp_rx_ts = (__be32 *)(ptp_hdr + OFF_PTP_RESERVED);
+	raw_ts = __be32_to_cpu(*ptp_rx_ts);
+	ns = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
+	shhwtstamps->hwtstamp = ns_to_ktime(ns);
+
+	dev_dbg(chip->dev, "p%d: rxtstamp %llx\n", port, ns);
+
+	return false;
+}
+
+static void mv88e6xxx_txtstamp_work(struct work_struct *ugly)
+{
+	struct mv88e6xxx_port_hwtstamp *ps = container_of(
+		ugly, struct mv88e6xxx_port_hwtstamp, tx_tstamp_work);
+	struct mv88e6xxx_chip *chip = container_of(
+		ps, struct mv88e6xxx_chip, port_hwtstamp[ps->port_id]);
+	struct sk_buff *tmp_skb;
+	unsigned long tmp_tstamp_start;
+	int err;
+	u16 departure_block[4];
+	u16 tmp_seq_id;
+
+	if (!test_bit(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
+		return;
+
+	tmp_skb = ps->tx_skb;
+	tmp_seq_id = ps->tx_seq_id;
+	tmp_tstamp_start = ps->tx_tstamp_start;
+
+	if (!tmp_skb)
+		return;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_ptp_read(chip, ps->port_id,
+				      MV88E6XXX_PORT_PTP_DEP_STS,
+				      departure_block,
+				      ARRAY_SIZE(departure_block));
+	mutex_unlock(&chip->reg_lock);
+
+	if (err)
+		goto free_and_clear_skb;
+
+	if (departure_block[0] & MV88E6XXX_PTP_TS_VALID) {
+		struct skb_shared_hwtstamps shhwtstamps;
+		u64 ns;
+		u32 time_raw;
+		u16 status;
+
+		/* We have the timestamp; go ahead and clear valid now */
+		mutex_lock(&chip->reg_lock);
+		mv88e6xxx_port_ptp_write(chip, ps->port_id,
+					 MV88E6XXX_PORT_PTP_DEP_STS, 0);
+		mutex_unlock(&chip->reg_lock);
+
+		status = departure_block[0] &
+				MV88E6XXX_PTP_TS_STATUS_MASK;
+		if (status != MV88E6XXX_PTP_TS_STATUS_NORMAL) {
+			dev_warn(chip->dev, "p%d: tx timestamp overrun\n",
+				 ps->port_id);
+			goto free_and_clear_skb;
+		}
+
+		if (departure_block[3] != tmp_seq_id) {
+			dev_warn(chip->dev, "p%d: unexpected sequence id\n",
+				 ps->port_id);
+			goto free_and_clear_skb;
+		}
+
+		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+		time_raw = ((u32)departure_block[2] << 16) |
+			   departure_block[1];
+		ns = timecounter_cyc2time(&chip->tstamp_tc, time_raw);
+		shhwtstamps.hwtstamp = ns_to_ktime(ns);
+
+		dev_dbg(chip->dev,
+			"p%d: txtstamp %llx status 0x%04x skb ID 0x%04x hw ID 0x%04x\n",
+			ps->port_id, ktime_to_ns(shhwtstamps.hwtstamp),
+			departure_block[0], tmp_seq_id, departure_block[3]);
+
+		/* skb_complete_tx_timestamp() will free up the client to make
+		 * another timestamp-able transmit. We have to be ready for it
+		 * -- by clearing the ps->tx_skb "flag" -- beforehand.
+		 */
+
+		ps->tx_skb = NULL;
+		clear_bit_unlock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
+
+		skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
+
+	} else {
+		if (time_is_before_jiffies(
+			    tmp_tstamp_start + TX_TSTAMP_TIMEOUT)) {
+			dev_warn(chip->dev, "p%d: clearing tx timestamp hang\n",
+				 ps->port_id);
+			goto free_and_clear_skb;
+		}
+
+		/* The timestamp should be available quickly, while getting it
+		 * is high priority and time bounded to only 10ms. A poll is
+		 * warranted and this is the nicest way to realize it in a work
+		 * item.
+		 */
+
+		queue_work(system_highpri_wq, &ps->tx_tstamp_work);
+	}
+
+	return;
+
+free_and_clear_skb:
+	ps->tx_skb = NULL;
+	clear_bit_unlock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
+
+	dev_kfree_skb_any(tmp_skb);
+}
+
+void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+			     struct sk_buff *clone, unsigned int type)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+
+	if (!chip->info->ptp_support)
+		return;
+
+	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
+		goto out;
+
+	if (unlikely(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
+	    mv88e6xxx_should_tstamp(chip, port, clone, type)) {
+		__be16 *seq_ptr = (__be16 *)(_get_ptp_header(clone, type) +
+					     OFF_PTP_SEQUENCE_ID);
+
+		if (!test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
+					   &ps->state)) {
+			ps->tx_skb = clone;
+			ps->tx_tstamp_start = jiffies;
+			ps->tx_seq_id = be16_to_cpup(seq_ptr);
+
+			/* Fetching the timestamp is high-priority work because
+			 * 802.1AS bounds the time for a response.
+			 *
+			 * No need to check result of queue_work(). ps->tx_skb
+			 * check ensures work item is not pending (it may be
+			 * waiting to exit)
+			 */
+			queue_work(system_highpri_wq, &ps->tx_tstamp_work);
+			return;
+		}
+
+		/* Otherwise we're already in progress... */
+		dev_dbg(chip->dev,
+			"p%d: tx timestamp already in progress, discarding",
+			port);
+	}
+
+out:
+	/* We don't need it after all. */
+	kfree_skb(clone);
+}
+
+static int mv88e6xxx_hwtstamp_port_setup(struct mv88e6xxx_chip *chip, int port)
+{
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+
+	ps->port_id = port;
+	INIT_WORK(&ps->tx_tstamp_work, mv88e6xxx_txtstamp_work);
+
+	return mv88e6xxx_port_ptp_write(chip, port, MV88E6XXX_PORT_PTP_CFG0,
+					MV88E6XXX_PORT_PTP_CFG0_DISABLE_PTP);
+}
+
+static void mv88e6xxx_hwtstamp_port_free(struct mv88e6xxx_chip *chip, int port)
+{
+	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
+
+	cancel_work_sync(&ps->tx_tstamp_work);
+}
+
+int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
+{
+	int i;
+	int err;
+
+	/* Disable timestamping on all ports. */
+	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
+		err = mv88e6xxx_hwtstamp_port_setup(chip, i);
+		if (err)
+			return err;
+	}
+
+	/* MV88E6XXX_PTP_MSG_TYPE is a mask of PTP message types to
+	 * timestamp. This affects all ports that have timestamping enabled,
+	 * but the timestamp config is per-port; thus we configure all events
+	 * here and only support the HWTSTAMP_FILTER_*_EVENT filter types.
+	 */
+	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_MSGTYPE,
+				  MV88E6XXX_PTP_MSGTYPE_ALL_EVENT);
+	if (err)
+		return err;
+
+	/* Each event type will be timestamped using ARRIVAL0. */
+	err = mv88e6xxx_ptp_write(chip, MV88E6XXX_PTP_TS_ARRIVAL_PTR, 0x0);
+	if (err)
+		return err;
+
+	/* Configure the switch to embed the (32-bit) arrival timestamps in
+	 * the packets, in the "reserved" field of the PTP header at octet 16
+	 * (OFF_PTP_RESERVED), and disable interrupts. When we do the per-port
+	 * configuration later, we will also allow overwrites (by not setting
+	 * the DISABLE_OVERWRITE bit). This combination lets us handle
+	 * back-to-back RX packets easily, because we don't have to do an SMI
+	 * access to retrieve the timestamp.
+	 */
+	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
+		u16 val = MV88E6XXX_PORT_PTP_CFG2_EMBED_ARRIVAL;
+
+		err = mv88e6xxx_port_ptp_write(chip, i,
+					       MV88E6XXX_PORT_PTP_CFG2, val);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip)
+{
+	int i;
+
+	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i)
+		mv88e6xxx_hwtstamp_port_free(chip, i);
+}
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
new file mode 100644
index 0000000..1d24220
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -0,0 +1,162 @@
+/*
+ * Marvell 88E6xxx Switch hardware timestamping support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2017 National Instruments
+ *      Erik Hons <erik.hons@ni.com>
+ *      Brandon Streiff <brandon.streiff@ni.com>
+ *      Dane Wagner <dane.wagner@ni.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _MV88E6XXX_HWTSTAMP_H
+#define _MV88E6XXX_HWTSTAMP_H
+
+#include "chip.h"
+
+/* Global PTP registers */
+/* Offset 0x00: PTP EtherType */
+#define MV88E6XXX_PTP_ETHERTYPE	0x00
+
+/* Offset 0x01: Message Type Timestamp Enables */
+#define MV88E6XXX_PTP_MSGTYPE			0x01
+#define MV88E6XXX_PTP_MSGTYPE_SYNC		0x0001
+#define MV88E6XXX_PTP_MSGTYPE_DELAY_REQ		0x0002
+#define MV88E6XXX_PTP_MSGTYPE_PDLAY_REQ		0x0004
+#define MV88E6XXX_PTP_MSGTYPE_PDLAY_RES		0x0008
+#define MV88E6XXX_PTP_MSGTYPE_ALL_EVENT		0x000f
+
+/* Offset 0x02: Timestamp Arrival Capture Pointers */
+#define MV88E6XXX_PTP_TS_ARRIVAL_PTR	0x02
+
+/* Offset 0x08: PTP Interrupt Status */
+#define MV88E6XXX_PTP_IRQ_STATUS	0x08
+
+/* Per-Port PTP Registers */
+/* Offset 0x00: PTP Configuration 0 */
+#define MV88E6XXX_PORT_PTP_CFG0				0x00
+#define MV88E6XXX_PORT_PTP_CFG0_TSPEC_SHIFT		12
+#define MV88E6XXX_PORT_PTP_CFG0_TSPEC_MASK		0xf000
+#define MV88E6XXX_PORT_PTP_CFG0_TSPEC_1588		0x0000
+#define MV88E6XXX_PORT_PTP_CFG0_TSPEC_8021AS		0x1000
+#define MV88E6XXX_PORT_PTP_CFG0_DISABLE_TSPEC_MATCH	0x0800
+#define MV88E6XXX_PORT_PTP_CFG0_DISABLE_OVERWRITE	0x0002
+#define MV88E6XXX_PORT_PTP_CFG0_DISABLE_PTP		0x0001
+
+/* Offset 0x01: PTP Configuration 1 */
+#define MV88E6XXX_PORT_PTP_CFG1	0x01
+
+/* Offset 0x02: PTP Configuration 2 */
+#define MV88E6XXX_PORT_PTP_CFG2				0x02
+#define MV88E6XXX_PORT_PTP_CFG2_EMBED_ARRIVAL		0x1000
+#define MV88E6XXX_PORT_PTP_CFG2_DEP_IRQ_EN		0x0002
+#define MV88E6XXX_PORT_PTP_CFG2_ARR_IRQ_EN		0x0001
+
+/* Offset 0x03: PTP LED Configuration */
+#define MV88E6XXX_PORT_PTP_LED_CFG	0x03
+
+/* Offset 0x08: PTP Arrival 0 Status */
+#define MV88E6XXX_PORT_PTP_ARR0_STS	0x08
+
+/* Offset 0x09/0x0A: PTP Arrival 0 Time */
+#define MV88E6XXX_PORT_PTP_ARR0_TIME_LO	0x09
+#define MV88E6XXX_PORT_PTP_ARR0_TIME_HI	0x0a
+
+/* Offset 0x0B: PTP Arrival 0 Sequence ID */
+#define MV88E6XXX_PORT_PTP_ARR0_SEQID	0x0b
+
+/* Offset 0x0C: PTP Arrival 1 Status */
+#define MV88E6XXX_PORT_PTP_ARR1_STS	0x0c
+
+/* Offset 0x0D/0x0E: PTP Arrival 1 Time */
+#define MV88E6XXX_PORT_PTP_ARR1_TIME_LO	0x0d
+#define MV88E6XXX_PORT_PTP_ARR1_TIME_HI	0x0e
+
+/* Offset 0x0F: PTP Arrival 1 Sequence ID */
+#define MV88E6XXX_PORT_PTP_ARR1_SEQID	0x0f
+
+/* Offset 0x10: PTP Departure Status */
+#define MV88E6XXX_PORT_PTP_DEP_STS	0x10
+
+/* Offset 0x11/0x12: PTP Deperture Time */
+#define MV88E6XXX_PORT_PTP_DEP_TIME_LO	0x11
+#define MV88E6XXX_PORT_PTP_DEP_TIME_HI	0x12
+
+/* Offset 0x13: PTP Departure Sequence ID */
+#define MV88E6XXX_PORT_PTP_DEP_SEQID	0x13
+
+/* Status fields for arrival and depature timestamp status registers */
+#define MV88E6XXX_PTP_TS_STATUS_MASK		0x0006
+#define MV88E6XXX_PTP_TS_STATUS_NORMAL		0x0000
+#define MV88E6XXX_PTP_TS_STATUS_OVERWITTEN	0x0002
+#define MV88E6XXX_PTP_TS_STATUS_DISCARDED	0x0004
+#define MV88E6XXX_PTP_TS_VALID			0x0001
+
+#ifdef CONFIG_NET_DSA_MV88E6XXX_PTP
+
+int mv88e6xxx_port_hwtstamp_set(struct dsa_switch *ds, int port,
+				struct ifreq *ifr);
+int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
+				struct ifreq *ifr);
+
+bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
+			     struct sk_buff *clone, unsigned int type);
+void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+			     struct sk_buff *clone, unsigned int type);
+
+int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
+			  struct ethtool_ts_info *info);
+
+int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip);
+void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip);
+
+#else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */
+
+static inline int mv88e6xxx_port_hwtstamp_set(struct dsa_switch *ds,
+					      int port, struct ifreq *ifr)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds,
+					      int port, struct ifreq *ifr)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
+					   struct sk_buff *clone,
+					   unsigned int type)
+{
+	return false;
+}
+
+static inline void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
+					   struct sk_buff *clone,
+					   unsigned int type)
+{
+}
+
+static inline int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
+					struct ethtool_ts_info *info);
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void mv88e6xxx_hwtstamp_free(struct mv88e6xxx_chip *chip)
+{
+}
+
+#endif /* CONFIG_NET_DSA_MV88E6XXX_PTP */
+
+#endif /* _MV88E6XXX_HWTSTAMP_H */
-- 
2.1.4

Re: [PATCH net-next RFC 8/9] net: dsa: mv88e6xxx: add rx/tx timestamping support

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:40AM -0500, Brandon Streiff wrote:

> +static bool mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip, int port,
> +				    struct sk_buff *skb, unsigned int type)
> +{
> +	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
> +	u8 *ptp_hdr, *msgtype;
> +	bool ret;
> +
> +	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
> +		return false;
> +
> +	ptp_hdr = _get_ptp_header(skb, type);
> +	if (IS_ERR(ptp_hdr))
> +		return false;
> +
> +	if (unlikely(type & PTP_CLASS_V1))
> +		msgtype = ptp_hdr + OFF_PTP_CONTROL;
> +	else
> +		msgtype = ptp_hdr;
> +
> +	ret = test_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);

This should be the first test, don't you think?

> +	dev_dbg(chip->dev,
> +		"p%d: PTP message classification 0x%x type 0x%x, tstamp? %d",
> +		port, type, *msgtype, (int)ret);
> +
> +	return ret;
> +}
> +
> +/* rxtstamp will be called in interrupt context so we don't to do
> + * anything like read PTP registers over SMI.
> + */
> +bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *skb, unsigned int type)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct skb_shared_hwtstamps *shhwtstamps;
> +	__be32 *ptp_rx_ts;
> +	u8 *ptp_hdr;
> +	u32 raw_ts;
> +	u64 ns;
> +
> +	if (!chip->info->ptp_support)
> +		return false;
> +
> +	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
> +		return false;

This test is duplicated in mv88e6xxx_should_tstamp().

> +	if (!mv88e6xxx_should_tstamp(chip, port, skb, type))
> +		return false;
> +
> +	shhwtstamps = skb_hwtstamps(skb);
> +	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +
> +	/* Because we configured the arrival timestamper to put the counter
> +	 * into the 32-bit "reserved" field of the PTP header, we can retrieve
> +	 * the value from the packet directly instead of having to retrieve it
> +	 * via SMI.
> +	 */
> +	ptp_hdr = _get_ptp_header(skb, type);
> +	if (IS_ERR(ptp_hdr))
> +		return false;
> +	ptp_rx_ts = (__be32 *)(ptp_hdr + OFF_PTP_RESERVED);
> +	raw_ts = __be32_to_cpu(*ptp_rx_ts);
> +	ns = timecounter_cyc2time(&chip->tstamp_tc, raw_ts);
> +	shhwtstamps->hwtstamp = ns_to_ktime(ns);
> +
> +	dev_dbg(chip->dev, "p%d: rxtstamp %llx\n", port, ns);
> +
> +	return false;
> +}
> +
> +static void mv88e6xxx_txtstamp_work(struct work_struct *ugly)
> +{
> +	struct mv88e6xxx_port_hwtstamp *ps = container_of(
> +		ugly, struct mv88e6xxx_port_hwtstamp, tx_tstamp_work);
> +	struct mv88e6xxx_chip *chip = container_of(
> +		ps, struct mv88e6xxx_chip, port_hwtstamp[ps->port_id]);
> +	struct sk_buff *tmp_skb;
> +	unsigned long tmp_tstamp_start;
> +	int err;
> +	u16 departure_block[4];
> +	u16 tmp_seq_id;
> +
> +	if (!test_bit(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
> +		return;
> +
> +	tmp_skb = ps->tx_skb;
> +	tmp_seq_id = ps->tx_seq_id;
> +	tmp_tstamp_start = ps->tx_tstamp_start;
> +
> +	if (!tmp_skb)
> +		return;
> +
> +	mutex_lock(&chip->reg_lock);
> +	err = mv88e6xxx_port_ptp_read(chip, ps->port_id,
> +				      MV88E6XXX_PORT_PTP_DEP_STS,
> +				      departure_block,
> +				      ARRAY_SIZE(departure_block));
> +	mutex_unlock(&chip->reg_lock);
> +
> +	if (err)
> +		goto free_and_clear_skb;
> +
> +	if (departure_block[0] & MV88E6XXX_PTP_TS_VALID) {

You can avoid the IfOk anti-pattern here.  Make the test for !VALID
and move the 'else' block up.

> +		struct skb_shared_hwtstamps shhwtstamps;
> +		u64 ns;
> +		u32 time_raw;
> +		u16 status;
> +
> +		/* We have the timestamp; go ahead and clear valid now */
> +		mutex_lock(&chip->reg_lock);
> +		mv88e6xxx_port_ptp_write(chip, ps->port_id,
> +					 MV88E6XXX_PORT_PTP_DEP_STS, 0);
> +		mutex_unlock(&chip->reg_lock);
> +
> +		status = departure_block[0] &
> +				MV88E6XXX_PTP_TS_STATUS_MASK;
> +		if (status != MV88E6XXX_PTP_TS_STATUS_NORMAL) {
> +			dev_warn(chip->dev, "p%d: tx timestamp overrun\n",
> +				 ps->port_id);
> +			goto free_and_clear_skb;
> +		}
> +
> +		if (departure_block[3] != tmp_seq_id) {
> +			dev_warn(chip->dev, "p%d: unexpected sequence id\n",
> +				 ps->port_id);
> +			goto free_and_clear_skb;
> +		}
> +
> +		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +		time_raw = ((u32)departure_block[2] << 16) |
> +			   departure_block[1];
> +		ns = timecounter_cyc2time(&chip->tstamp_tc, time_raw);
> +		shhwtstamps.hwtstamp = ns_to_ktime(ns);
> +
> +		dev_dbg(chip->dev,
> +			"p%d: txtstamp %llx status 0x%04x skb ID 0x%04x hw ID 0x%04x\n",
> +			ps->port_id, ktime_to_ns(shhwtstamps.hwtstamp),
> +			departure_block[0], tmp_seq_id, departure_block[3]);
> +
> +		/* skb_complete_tx_timestamp() will free up the client to make
> +		 * another timestamp-able transmit. We have to be ready for it
> +		 * -- by clearing the ps->tx_skb "flag" -- beforehand.
> +		 */
> +
> +		ps->tx_skb = NULL;
> +		clear_bit_unlock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> +
> +		skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +
> +	} else {
> +		if (time_is_before_jiffies(
> +			    tmp_tstamp_start + TX_TSTAMP_TIMEOUT)) {
> +			dev_warn(chip->dev, "p%d: clearing tx timestamp hang\n",
> +				 ps->port_id);
> +			goto free_and_clear_skb;
> +		}
> +
> +		/* The timestamp should be available quickly, while getting it
> +		 * is high priority and time bounded to only 10ms. A poll is
> +		 * warranted and this is the nicest way to realize it in a work
> +		 * item.
> +		 */
> +		queue_work(system_highpri_wq, &ps->tx_tstamp_work);
> +	}
> +
> +	return;
> +
> +free_and_clear_skb:
> +	ps->tx_skb = NULL;
> +	clear_bit_unlock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> +
> +	dev_kfree_skb_any(tmp_skb);
> +}
> +
> +void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *clone, unsigned int type)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
> +
> +	if (!chip->info->ptp_support)
> +		return;
> +
> +	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
> +		goto out;

This test is duplicated in mv88e6xxx_should_tstamp().

> +	if (unlikely(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
> +	    mv88e6xxx_should_tstamp(chip, port, clone, type)) {

Please avoid the IfOk anti-pattern here as well.

> +		__be16 *seq_ptr = (__be16 *)(_get_ptp_header(clone, type) +
> +					     OFF_PTP_SEQUENCE_ID);
> +
> +		if (!test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
> +					   &ps->state)) {
> +			ps->tx_skb = clone;
> +			ps->tx_tstamp_start = jiffies;
> +			ps->tx_seq_id = be16_to_cpup(seq_ptr);
> +
> +			/* Fetching the timestamp is high-priority work because
> +			 * 802.1AS bounds the time for a response.
> +			 *
> +			 * No need to check result of queue_work(). ps->tx_skb
> +			 * check ensures work item is not pending (it may be
> +			 * waiting to exit)
> +			 */
> +			queue_work(system_highpri_wq, &ps->tx_tstamp_work);
> +			return;
> +		}
> +
> +		/* Otherwise we're already in progress... */
> +		dev_dbg(chip->dev,
> +			"p%d: tx timestamp already in progress, discarding",
> +			port);
> +	}
> +
> +out:
> +	/* We don't need it after all. */
> +	kfree_skb(clone);

How about moving this logic should into the caller, letting the tx
callback return a code that tells whether the clone was accepted or
not?

> +}

Thanks,
Richard

Re: [PATCH net-next RFC 8/9] net: dsa: mv88e6xxx: add rx/tx timestamping support

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:40AM -0500, Brandon Streiff wrote:
> We also utilize a feature of the "generation 3" PTP hardware that lets
> us to embed the timestamp value into one of the reserved fields in the
> PTP header. This lets us extract the timestamp out of the header and
> avoid an SMI access in the RX codepath. (This implementation does not
> presently support the older generations.)

That is fine for the later models, but we really need the code to read
over MDIO as well.  You added .ptp_support = true for those older
switches, and so the present series won't work.

If it helps, maybe I can adapt the relevant code from my driver to
your work.

Thanks,
Richard

Re: [PATCH net-next RFC 8/9] net: dsa: mv88e6xxx: add rx/tx timestamping support

[Richard Cochran]

On Thu, Sep 28, 2017 at 10:25:40AM -0500, Brandon Streiff wrote:
> +void mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> +			     struct sk_buff *clone, unsigned int type)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
> +
> +	if (!chip->info->ptp_support)
> +		return;
> +
> +	if (port < 0 || port >= mv88e6xxx_num_ports(chip))
> +		goto out;
> +
> +	if (unlikely(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
> +	    mv88e6xxx_should_tstamp(chip, port, clone, type)) {
> +		__be16 *seq_ptr = (__be16 *)(_get_ptp_header(clone, type) +
> +					     OFF_PTP_SEQUENCE_ID);
> +
> +		if (!test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
> +					   &ps->state)) {
> +			ps->tx_skb = clone;
> +			ps->tx_tstamp_start = jiffies;
> +			ps->tx_seq_id = be16_to_cpup(seq_ptr);
> +
> +			/* Fetching the timestamp is high-priority work because
> +			 * 802.1AS bounds the time for a response.

Can you please use this?

commit d9535cb7b7603aeb549c697ecdf92024e4d0a650
Author: Grygorii Strashko <grygorii.strashko@ti.com>
Date:   Fri Jul 28 17:30:02 2017 -0500

    ptp: introduce ptp auxiliary worker
    
    Many PTP drivers required to perform some asynchronous or periodic work,
    like periodically handling PHC counter overflow or handle delayed timestamp
    for RX/TX network packets. In most of the cases, such work is implemented
    using workqueues. Unfortunately, Kernel workqueues might introduce
    significant delay in work scheduling under high system load and on -RT,
    which could cause misbehavior of PTP drivers due to internal counter
    overflow, for example, and there is no way to tune its execution policy and
    priority manuallly.
    
    Hence, The kthread_worker can be used insted of workqueues, as it create
    separte named kthread for each worker and its its execution policy and
    priority can be configured using chrt tool.

> +			 * No need to check result of queue_work(). ps->tx_skb
> +			 * check ensures work item is not pending (it may be
> +			 * waiting to exit)
> +			 */
> +			queue_work(system_highpri_wq, &ps->tx_tstamp_work);
> +			return;
> +		}
> +
> +		/* Otherwise we're already in progress... */
> +		dev_dbg(chip->dev,
> +			"p%d: tx timestamp already in progress, discarding",
> +			port);
> +	}
> +
> +out:
> +	/* We don't need it after all. */
> +	kfree_skb(clone);
> +}

Thanks,
Richard

[PATCH net-next RFC 9/9] net: dsa: mv88e6xxx: add workaround for 6341 timestamping

[Brandon Streiff]

88E6341 devices default to timestamping at the PHY, but due to a
hardware issue, timestamps via this component are unreliable. For
this family, configure the PTP hardware to force the timestamping
to occur at the MAC.

Signed-off-by: Brandon Streiff <brandon.streiff@ni.com>
---
 drivers/net/dsa/mv88e6xxx/hwtstamp.c | 13 +++++++++++++
 drivers/net/dsa/mv88e6xxx/hwtstamp.h |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 825d6f0..b6aee88 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -523,6 +523,19 @@ int mv88e6xxx_hwtstamp_setup(struct mv88e6xxx_chip *chip)
 			return err;
 	}
 
+	/* 88E6341 devices default to timestamping at the PHY, but this has
+	 * a hardware issue that results in unreliable timestamps. Force
+	 * these devices to timestamp at the MAC.
+	 */
+	if (chip->info->family == MV88E6XXX_FAMILY_6341) {
+		u16 val = MV88E6341_PTP_CFG_UPDATE |
+			  MV88E6341_PTP_CFG_MODE_IDX |
+			  MV88E6341_PTP_CFG_MODE_TS_AT_MAC;
+		err = mv88e6xxx_ptp_write(chip, MV88E6341_PTP_CFG, val);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index 1d24220..550711a 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -34,6 +34,15 @@
 /* Offset 0x02: Timestamp Arrival Capture Pointers */
 #define MV88E6XXX_PTP_TS_ARRIVAL_PTR	0x02
 
+/* Offset 0x07: PTP Global Configuration */
+#define MV88E6341_PTP_CFG			0x07
+#define MV88E6341_PTP_CFG_UPDATE		0x8000
+#define MV88E6341_PTP_CFG_IDX_MASK		0x7f00
+#define MV88E6341_PTP_CFG_DATA_MASK		0x00ff
+#define MV88E6341_PTP_CFG_MODE_IDX		0x0
+#define MV88E6341_PTP_CFG_MODE_TS_AT_PHY	0x00
+#define MV88E6341_PTP_CFG_MODE_TS_AT_MAC	0x80
+
 /* Offset 0x08: PTP Interrupt Status */
 #define MV88E6XXX_PTP_IRQ_STATUS	0x08
 
-- 
2.1.4

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Andrew Lunn]

> - Patch #3: The GPIO config support is handled in a very simple manner.
>   I suspect a longer term goal would be to use pinctrl here.

I assume ptp already has the core code to use pinctrl and Linux
standard GPIOs? What does the device tree binding look like? How do
you specify the GPIOs to use?

What we want to avoid is defining an ABI now, otherwise it is going to
be hard to swap to pinctrl later.

> - Patch #6: the dsa_switch pointer and port index is plumbed from
>   dsa_device_ops::rcv so that we can call the correct port_rxtstamp
>   method. This involved instrumenting all of the *_tag_rcv functions in
>   a way that's kind of a kludge and that I'm not terribly happy with.

Yes, this is ugly. I will see if i can find a better way to do
this. 

      Andrew

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Florian Fainelli]

On 09/28/2017 10:36 AM, Andrew Lunn wrote:
>> - Patch #3: The GPIO config support is handled in a very simple manner.
>>   I suspect a longer term goal would be to use pinctrl here.
> 
> I assume ptp already has the core code to use pinctrl and Linux
> standard GPIOs? What does the device tree binding look like? How do
> you specify the GPIOs to use?
> 
> What we want to avoid is defining an ABI now, otherwise it is going to
> be hard to swap to pinctrl later.
> 
>> - Patch #6: the dsa_switch pointer and port index is plumbed from
>>   dsa_device_ops::rcv so that we can call the correct port_rxtstamp
>>   method. This involved instrumenting all of the *_tag_rcv functions in
>>   a way that's kind of a kludge and that I'm not terribly happy with.
> 
> Yes, this is ugly. I will see if i can find a better way to do
> this. 

See my reply in patch 6, I may be missing something, but once
dst->rdcv() has been called, skb->dev points to the slave network device
which already contains the switch port and switch information in
dsa_slave_priv, so that should lift the need for asking the individual
taggers' rcv() callback to tell us about it.
-- 
Florian

RE: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Brandon Streiff]

> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Thursday, September 28, 2017 12:36 PM
>
> I assume ptp already has the core code to use pinctrl and Linux
> standard GPIOs? What does the device tree binding look like? How do
> you specify the GPIOs to use?
> 
> What we want to avoid is defining an ABI now, otherwise it is going to
> be hard to swap to pinctrl later.

A ptp_clock_info has an array of struct ptp_pin_desc which defines "pins" with a name ("Hardware specific human readable pin name"), an index, and a bitmask of valid functions. The ptp_pin_desc structure is shared with usermode for the PTP_PIN_GETFUNC and PTP_PIN_SETFUNC ioctls. The pins are also exposed in sysfs (see Documentation/ABI/testing/sysfs-ptp). The underlying implementation for configuring the hardware is left up to the PHC driver. I don't see any drivers today that use the PHC pin API as a layer over pinctrl/gpiochip, but there's no reason that that couldn't be the case.

For mv88e6xxx, we name the pins using the pattern "mv88e6xxx_gpio%d"; this appears to be in line with current practice (igb_ptp.c uses "SDP%d", mlx5 driver uses "mlx5_pps%d"). Usermode code appears to be expected to determine which pin it needs to use. (Our current userspace code, for instance, knows that it needs to find "mv88e6xxx_gpio8".)

For mv88e6xxx, Device Tree does feel like a better option here for declaring names, functions, and pin usages. Not all platforms that use the PTP API use Device Tree though.

-- brandon

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

Brandon,

On Thu, Sep 28, 2017 at 10:25:32AM -0500, Brandon Streiff wrote:
> - Patch #2: We expose the switch time as a PTP clock but don't support
>   adjustment (max_adj=0).

The driver should implement a cyclecounter/timecounter.

> Our platform adjusted a systemwide oscillator
>   from userspace, so we didn't need adjustment at this layer, but other
>   PTP clock drivers support this and we probably should too.

We don't currently have any way to support this kind of HW or anything
like an external VCO.  I would like to find a way to do this, but that
is a different kettle of fish as it might require changes in the PHC
subsystem.  For this driver, I think we should get it merged using the
cyclecounter/timecounter (as that will benefit lots of users) and
worry about the external oscillator later.
 
> Feedback is appreciated.

I happy to see this series.  I just finished porting an out-of-tree
PHC driver for the Marvell mv88e635x, and I want to mainline it, but I
also have a few uglies.

Unfortunately I am in the middle of a move right now, and so my review
of this series might have to wait a bit.  However, I am looking
forward to comparing notes, and then getting this support in.

Thanks,
Richard

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Fri, Sep 29, 2017 at 05:43:23AM -0400, Richard Cochran wrote:
> I happy to see this series.  I just finished porting an out-of-tree
> PHC driver for the Marvell mv88e635x, and I want to mainline it, but I
> also have a few uglies.

This series looks really good.  I won't even post my mine, as that
would now be too embarrassing.

I will try to get my hands on some HW, perhaps by the end of October,
in order to test and complete your driver...

Thanks,
Richard

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Sun, Oct 08, 2017 at 11:38:21AM -0400, Richard Cochran wrote:
> I will try to get my hands on some HW, perhaps by the end of October,
> in order to test and complete your driver...

I now have a 88E6352 to test your series on.  Unfortunately, it
doesn't really work. Here is what I did.

1. Gave one of the external switch ports an address (ifconfig ext0
   192.168.1.111)

2. Ran ptp4l with option 'free_running 1'.

When I run with Layer2 transport and the switch as master, it seems to
work.  Any other combination of role + transport fails.

| Switch Role | Tranport | Result                                         |
|-------------+----------+------------------------------------------------|
| master      | UDPv4    | no Delay_Resp appear at slave                  |
| master      | UDPv6    | no Delay_Resp appear at slave                  |
| master      | layer2   | seems okay                                     |
| slave       | layer2   | Announce messages not getting through to host? |

Have you tested any of this?

Do I need some special switch configuration first?

 
Thanks,
Richard

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Andrew Lunn]

On Mon, Nov 06, 2017 at 06:55:46AM -0800, Richard Cochran wrote:
> On Sun, Oct 08, 2017 at 11:38:21AM -0400, Richard Cochran wrote:
> > I will try to get my hands on some HW, perhaps by the end of October,
> > in order to test and complete your driver...
> 
> I now have a 88E6352 to test your series on.  Unfortunately, it
> doesn't really work. Here is what I did.
> 
> 1. Gave one of the external switch ports an address (ifconfig ext0
>    192.168.1.111)

Hi Richard

I assume you have tested basic networking? You can ping the other
machines in the network?

With DSA, users sometimes forget to set the DSA master interface up.
Then nothing works.

	 Andrew

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Mon, Nov 06, 2017 at 04:04:22PM +0100, Andrew Lunn wrote:
> I assume you have tested basic networking? You can ping the other
> machines in the network?

Yes, I ssh into the device via the external switch port interface.

Thanks,
Richard

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Mon, Nov 06, 2017 at 06:55:46AM -0800, Richard Cochran wrote:
> When I run with Layer2 transport and the switch as master, it seems to
> work.  Any other combination of role + transport fails.

Oops, I had "slaveOnly" set in my PC's configuration.  So layer2 seems
to work as expected.

Have you tested UDPv4?  It doesn't work.

Thanks,
Richard

RE: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Brandon Streiff]

> Oops, I had "slaveOnly" set in my PC's configuration.  So layer2 seems
> to work as expected.
> 
> Have you tested UDPv4?  It doesn't work.

I have not. Our usage has been focused on 802.1AS; the ptp4l settings we
use are the following:

transportSpecific    0x1
ptp_dst_mac          01:80:C2:00:00:0E
p2p_dst_mac          01:80:C2:00:00:0E
network_transport    L2
delay_mechanism      P2P
time_stamping        hardware

One thing that we're not doing (and probably should be) is configuring
multicast frames to 01:1B:19:00:00:00 to be destined to the CPU port.
(01:80:C2:00:00:0E is used for management, so the *_mgmt_rsvd2cpu()
functions give us that "for free".) That might be necessary to make 1588
L2 work properly. I don't know if that would affect 1588 L4, or if
there's anything else missing to make L4 timestamping work from the HW
perspective.

-- brandon

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Andrew Lunn]

On Tue, Nov 07, 2017 at 08:56:05PM +0000, Brandon Streiff wrote:
> > Oops, I had "slaveOnly" set in my PC's configuration.  So layer2 seems
> > to work as expected.
> > 
> > Have you tested UDPv4?  It doesn't work.
> 
> I have not. Our usage has been focused on 802.1AS; the ptp4l settings we
> use are the following:
> 
> transportSpecific    0x1
> ptp_dst_mac          01:80:C2:00:00:0E
> p2p_dst_mac          01:80:C2:00:00:0E
> network_transport    L2
> delay_mechanism      P2P
> time_stamping        hardware
> 
> One thing that we're not doing (and probably should be) is configuring
> multicast frames to 01:1B:19:00:00:00 to be destined to the CPU port.
> (01:80:C2:00:00:0E is used for management, so the *_mgmt_rsvd2cpu()
> functions give us that "for free".) That might be necessary to make 1588
> L2 work properly. I don't know if that would affect 1588 L4, or if
> there's anything else missing to make L4 timestamping work from the HW
> perspective.

Is the application performing a join on the group? If so, on which
interface?

I've not tested many multicast applications with DSA. It is possible
we have bugs.

   Andrew

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Andrew Lunn]

> One thing that we're not doing (and probably should be) is
> configuring multicast frames to 01:1B:19:00:00:00 to be destined to
> the CPU port.

So i did a quick test. If the application joins 224.0.1.129 on the
slave interface, the switch will pass the packets to the host and to
the application.

      Andrew

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Wed, Nov 08, 2017 at 04:02:26AM +0100, Andrew Lunn wrote:
> So i did a quick test. If the application joins 224.0.1.129 on the
> slave interface, the switch will pass the packets to the host and to
> the application.

The application does join that group on the external (slave)
interface.  I'll find out why the delay request mechanism isn't
working...

Thanks,
Richard

Re: [PATCH net-next RFC 0/9] net: dsa: PTP timestamping for mv88e6xxx

[Richard Cochran]

On Tue, Nov 07, 2017 at 07:23:27PM -0800, Richard Cochran wrote:
> The application does join that group on the external (slave)
> interface.  I'll find out why the delay request mechanism isn't
> working...

Looking back, I now recall that the series lets the HW embed the time
stamps into the protocol buffers.  In the case of UDP, this
invalidates the checksum unless the HW corrects it.  I'll bet that is
what is happening...
 
Thanks,
Richard