[PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King]

Add PTP basic support for Marvell 88E151x PHYs.  These PHYs support
timestamping the egress and ingress of packets, but does not support
any packet modification, nor do we support any filtering beyond
selecting packets that the hardware recognises as PTP/802.1AS.

The PHYs support hardware pins for providing an external clock for the
TAI counter, and a separate pin that can be used for event capture or
generation of a trigger (either a pulse or periodic).  This code does
not support either of these modes.

We currently use a delayed work to poll for the timestamps which is
far from ideal, but we also provide a function that can be called from
an interrupt handler - which would be good to tie into the main Marvell
PHY driver.

The driver takes inspiration from the Marvell 88E6xxx DSA and DP83640
drivers.  The hardware is very similar to the implementation found in
the 88E6xxx DSA driver, but the access methods are very different,
although it may be possible to create a library that both can use
along with accessor functions.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---

This does not wire up IRQ support yet - I don't think I have a system
I can test that with, but I need to check.

Tested with linuxptp; using phc2sys to synchronise the clock on the PHY
with system time, and that seems to work reasonably well - but every so
often (randomly) there seems to be a large delay in reading the time,
which results in a large shift in the frequency setting, and then it
takes a while to re-settle.  I'm not sure where that is coming from.

I also haven't checked for other Marvell PHYs supporting this; I know
the 88E1111 does not, and the 154x family also seem not to support it.
However, the 151x family for x=0,2,4,8 do.  The door is partly left
open in the code to support a common TAI with multiple ports, but that
would require further work - but as the 151x PHYs are only single port
PHYs, it seemed pointless to add support immediately.

The 151x family does have two pins that are related to the PTP/TAI
support:
- a pin which can be used for event capture, generating a pulse on
  TAI counter match, or generating a 50% duty cycle clock.  In the
  latter case, defining when the first edge occurs does not seem to
  be possible.
- a pin which can be supplied an external clock instead of using the
  internal 125MHz clock to increment the TAI counter.

Getting the Kconfig for this correct has been a struggle - particularly
the combination where PTP support is modular.  It's rather odd to have
the Marvell PTP support asked before the Marvell PHY support.  I
couldn't work out any other reasonable way to ensure that we always
have a valid configuration, without leading to stupidities such as
having the PTP and Marvell PTP support modular, but non-functional
because Marvell PHY is built-in.

With this driver, there appears to be three instances of a ptp_header()
like function in the kernel - I think that ought to become a helper in
generic code.

It seems that the Marvell PHY PTP is very similar to that found in
their DSA chips, which suggests maybe we should share the code, but
different access methods would be required.

There's a hack in here to implement the gettimex64() support, as
gettime64() is marked as deprecated - this is the best solution I could
come up with to support the new function while still using the
timecounter/cyclecounter support.

 drivers/net/phy/Kconfig       |  12 +
 drivers/net/phy/Makefile      |   2 +
 drivers/net/phy/marvell.c     |  28 +-
 drivers/net/phy/marvell_ptp.c | 691 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/marvell_ptp.h |  20 +
 drivers/net/phy/marvell_tai.c | 279 ++++++++++++++
 drivers/net/phy/marvell_tai.h |  36 ++
 7 files changed, 1066 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/marvell_ptp.c
 create mode 100644 drivers/net/phy/marvell_ptp.h
 create mode 100644 drivers/net/phy/marvell_tai.c
 create mode 100644 drivers/net/phy/marvell_tai.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index dd20c2c27c2f..e5bd5b779a0d 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -451,8 +451,20 @@ config LXT_PHY
 	help
 	  Currently supports the lxt970, lxt971
 
+config MARVELL_PHY_PTP
+	tristate "Marvell PHY PTP support"
+	depends on NETWORK_PHY_TIMESTAMPING && PTP_1588_CLOCK
+	help
+	  Support PHY timestamping on Marvell 88E1510, 88E1512, 88E1514
+	  and 88E1518 PHYs.
+
+	  N.B. In order for this to be fully functional, your MAC driver
+	  must call the skb_tx_timestamp() function.
+
 config MARVELL_PHY
 	tristate "Marvell PHYs"
+	# This can't be builtin if the PTP support is modular
+	depends on MARVELL_PHY_PTP=n || MARVELL_PHY_PTP
 	help
 	  Currently has a driver for the 88E1011S
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index d84bab489a53..00ba7be63246 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -87,6 +87,8 @@ obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
 obj-$(CONFIG_LSI_ET1011C_PHY)	+= et1011c.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o
 obj-$(CONFIG_MARVELL_PHY)	+= marvell.o
+obj-$(CONFIG_MARVELL_PHY_PTP)	+= marvell-ptp.o
+marvell-ptp-y			:= marvell_ptp.o marvell_tai.o
 obj-$(CONFIG_MARVELL_10G_PHY)	+= marvell10g.o
 obj-$(CONFIG_MESON_GXL_PHY)	+= meson-gxl.o
 obj-$(CONFIG_MICREL_KS8995MA)	+= spi_ks8995.o
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bb86ac0bd092..ef1ac619db3c 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -37,6 +37,8 @@
 #include <asm/irq.h>
 #include <linux/uaccess.h>
 
+#include "marvell_ptp.h"
+
 #define MII_MARVELL_PHY_PAGE		22
 #define MII_MARVELL_COPPER_PAGE		0x00
 #define MII_MARVELL_FIBER_PAGE		0x01
@@ -2589,6 +2591,27 @@ static int m88e1121_probe(struct phy_device *phydev)
 {
 	int err;
 
+	err = marvell_probe(phydev);
+	if (err)
+		return err;
+
+	err = marvell_ptp_probe(phydev);
+	if (err)
+		return err;
+
+	err = m88e1510_hwmon_probe(phydev);
+	if (err) {
+		marvell_ptp_remove(phydev);
+		return err;
+	}
+
+	return 0;
+}
+
+static int m88e154x_probe(struct phy_device *phydev)
+{
+	int err;
+
 	err = marvell_probe(phydev);
 	if (err)
 		return err;
@@ -2823,6 +2846,7 @@ static struct phy_driver marvell_drivers[] = {
 		.features = PHY_GBIT_FIBRE_FEATURES,
 		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e1510_probe,
+		.remove = marvell_ptp_remove,
 		.config_init = m88e1510_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
@@ -2851,7 +2875,7 @@ static struct phy_driver marvell_drivers[] = {
 		.name = "Marvell 88E1540",
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
-		.probe = m88e1510_probe,
+		.probe = m88e154x_probe,
 		.config_init = marvell_config_init,
 		.config_aneg = m88e1510_config_aneg,
 		.read_status = marvell_read_status,
@@ -2875,7 +2899,7 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id = MARVELL_PHY_ID_88E1545,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1545",
-		.probe = m88e1510_probe,
+		.probe = m88e154x_probe,
 		/* PHY_GBIT_FEATURES */
 		.flags = PHY_POLL_CABLE_TEST,
 		.config_init = marvell_config_init,
diff --git a/drivers/net/phy/marvell_ptp.c b/drivers/net/phy/marvell_ptp.c
new file mode 100644
index 000000000000..b61a9f975179
--- /dev/null
+++ b/drivers/net/phy/marvell_ptp.c
@@ -0,0 +1,691 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell PTP driver for 88E1510, 88E1512, 88E1514 and 88E1518 PHYs
+ *
+ * Ideas taken from 88E6xxx DSA and DP83640 drivers. This file
+ * implements the packet timestamping support only (PTP).  TAI
+ * support is separate.
+ */
+#include <linux/if_vlan.h>
+#include <linux/list.h>
+#include <linux/netdevice.h>
+#include <linux/net_tstamp.h>
+#include <linux/phy.h>
+#include <linux/ptp_classify.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/uaccess.h>
+
+#include "marvell_ptp.h"
+#include "marvell_tai.h"
+
+#define TX_TIMEOUT_MS	40
+#define RX_TIMEOUT_MS	40
+
+#define MARVELL_PAGE_PTP_PORT_1			8
+#define PTP1_PORT_CONFIG_0			0
+#define PTP1_PORT_CONFIG_0_DISTSPECCHECK	BIT(11)
+#define PTP1_PORT_CONFIG_0_DISTSOVERWRITE	BIT(1)
+#define PTP1_PORT_CONFIG_0_DISPTP		BIT(0)
+#define PTP1_PORT_CONFIG_1			1
+#define PTP1_PORT_CONFIG_1_IPJUMP(x)		(((x) & 0x3f) << 8)
+#define PTP1_PORT_CONFIG_1_ETJUMP(x)		((x) & 0x1f)
+#define PTP1_PORT_CONFIG_2			2
+#define PTP1_PORT_CONFIG_2_DEPINTEN		BIT(1)
+#define PTP1_PORT_CONFIG_2_ARRINTEN		BIT(0)
+#define PTP1_ARR_STATUS0			8
+#define PTP1_ARR_STATUS1			12
+
+#define MARVELL_PAGE_PTP_PORT_2			9
+#define PTP2_DEP_STATUS				0
+
+struct marvell_ptp_cb {
+	unsigned long timeout;
+	u16 seq;
+};
+#define MARVELL_PTP_CB(skb)	((struct marvell_ptp_cb *)(skb)->cb)
+
+struct marvell_rxts {
+	struct list_head node;
+	u64 ns;
+	u16 seq;
+};
+
+struct marvell_ptp {
+	struct marvell_tai *tai;
+	struct list_head tai_node;
+	struct phy_device *phydev;
+	struct mii_timestamper mii_ts;
+
+	/* We only support one outstanding transmit skb */
+	struct sk_buff *tx_skb;
+	enum hwtstamp_tx_types tx_type;
+
+	struct mutex rx_mutex;
+	struct list_head rx_free;
+	struct list_head rx_pend;
+	struct sk_buff_head rx_queue;
+	enum hwtstamp_rx_filters rx_filter;
+	struct marvell_rxts rx_ts[64];
+
+	struct delayed_work ts_work;
+};
+
+struct marvell_ts {
+	u32 time;
+	u16 stat;
+#define MV_STATUS_INTSTATUS_MASK		0x0006
+#define MV_STATUS_INTSTATUS_NORMAL		0x0000
+#define MV_STATUS_VALID				BIT(0)
+	u16 seq;
+};
+
+/* Read the status, timestamp and PTP common header sequence from the PHY.
+ * Apparently, reading these are atomic, but there is no mention how the
+ * PHY treats this access as atomic. So, we set the DisTSOverwrite bit
+ * when configuring the PHY.
+ */
+static int marvell_read_tstamp(struct phy_device *phydev,
+			       struct marvell_ts *ts,
+			       uint8_t page, uint8_t reg)
+{
+	int oldpage;
+	int ret;
+
+	/* Read status register */
+	oldpage = phy_select_page(phydev, page);
+	if (oldpage >= 0) {
+		ret = __phy_read(phydev, reg);
+		if (ret < 0)
+			goto restore;
+
+		ts->stat = ret;
+		if (!(ts->stat & MV_STATUS_VALID)) {
+			ret = 0;
+			goto restore;
+		}
+
+		/* Read low timestamp */
+		ret = __phy_read(phydev, reg + 1);
+		if (ret < 0)
+			goto restore;
+
+		ts->time = ret;
+
+		/* Read high timestamp */
+		ret = __phy_read(phydev, reg + 2);
+		if (ret < 0)
+			goto restore;
+
+		ts->time |= ret << 16;
+
+		/* Read sequence */
+		ret = __phy_read(phydev, reg + 3);
+		if (ret < 0)
+			goto restore;
+
+		ts->seq = ret;
+
+		/* Clear valid */
+		__phy_write(phydev, reg, 0);
+	}
+restore:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+/* Shouldn't the ptp/networking provide this? */
+static u8 *ptp_header(struct sk_buff *skb, int type)
+{
+	u8 *data = skb_mac_header(skb);
+	u8 *ptr = data;
+
+	if (type & PTP_CLASS_VLAN)
+		ptr += VLAN_HLEN;
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		ptr += IPV4_HLEN(ptr) + UDP_HLEN;
+		break;
+
+	case PTP_CLASS_IPV6:
+		ptr += IP6_HLEN + UDP_HLEN;
+		break;
+
+	case PTP_CLASS_L2:
+		break;
+
+	default:
+		return NULL;
+	}
+
+	if (skb->len < ptr - data + 34)
+		return NULL;
+
+	return ptr + ETH_HLEN;
+}
+
+/* Extract the sequence ID */
+static u16 ptp_seqid(u8 *ptp_hdr)
+{
+	__be16 *seqp = (__be16 *)(ptp_hdr + OFF_PTP_SEQUENCE_ID);
+
+	return be16_to_cpup(seqp);
+}
+
+static struct marvell_ptp *mii_ts_to_ptp(struct mii_timestamper *mii_ts)
+{
+	return container_of(mii_ts, struct marvell_ptp, mii_ts);
+}
+
+/* Deliver a skb with its timestamp back to the networking core */
+static void marvell_ptp_rx(struct sk_buff *skb, u64 ns)
+{
+	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+
+	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+	shhwtstamps->hwtstamp = ns_to_ktime(ns);
+	netif_rx_ni(skb);
+}
+
+/* Get a rx timestamp entry. Try the free list, and if that fails,
+ * steal the oldest off the pending list.
+ */
+static struct marvell_rxts *marvell_ptp_get_rxts(struct marvell_ptp *ptp)
+{
+	if (!list_empty(&ptp->rx_free))
+		return list_first_entry(&ptp->rx_free, struct marvell_rxts,
+					node);
+
+	return list_last_entry(&ptp->rx_pend, struct marvell_rxts, node);
+}
+
+/* Check for a rx timestamp entry, try to find the corresponding skb and
+ * deliver it, otherwise add the rx timestamp to the queue of pending
+ * timestamps.
+ */
+static int marvell_ptp_rx_ts(struct marvell_ptp *ptp)
+{
+	struct marvell_rxts *rxts;
+	struct marvell_ts ts;
+	struct sk_buff *skb;
+	bool found = false;
+	u64 ns;
+	int err;
+
+	err = marvell_read_tstamp(ptp->phydev, &ts, MARVELL_PAGE_PTP_PORT_1,
+				  PTP1_ARR_STATUS0);
+	if (err <= 0)
+		return 0;
+
+	if ((ts.stat & MV_STATUS_INTSTATUS_MASK) !=
+	    MV_STATUS_INTSTATUS_NORMAL) {
+		dev_warn(&ptp->phydev->mdio.dev,
+			 "rx timestamp overrun (%x)\n", ts.stat);
+		return -1;
+	}
+
+	ns = marvell_tai_cyc2time(ptp->tai, ts.time);
+
+	mutex_lock(&ptp->rx_mutex);
+
+	/* Search the rx queue for a matching skb */
+	skb_queue_walk(&ptp->rx_queue, skb) {
+		if (MARVELL_PTP_CB(skb)->seq == ts.seq) {
+			__skb_unlink(skb, &ptp->rx_queue);
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		rxts = marvell_ptp_get_rxts(ptp);
+		rxts->ns = ns;
+		rxts->seq = ts.seq;
+		list_move(&rxts->node, &ptp->rx_pend);
+	}
+
+	mutex_unlock(&ptp->rx_mutex);
+
+	if (found)
+		marvell_ptp_rx(skb, ns);
+
+	return 1;
+}
+
+/* Check whether the packet is suitable for timestamping, and if so,
+ * try to find a pending timestamp for it. If no timestamp is found,
+ * queue the packet with a timeout.
+ */
+static bool marvell_ptp_rxtstamp(struct mii_timestamper *mii_ts,
+				 struct sk_buff *skb, int type)
+{
+	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
+	struct marvell_rxts *rxts;
+	bool found = false;
+	u8 *ptp_hdr;
+	u16 seqid;
+	u64 ns;
+
+	if (ptp->rx_filter == HWTSTAMP_FILTER_NONE)
+		return false;
+
+	ptp_hdr = ptp_header(skb, type);
+	if (!ptp_hdr)
+		return false;
+
+	seqid = ptp_seqid(ptp_hdr);
+
+	mutex_lock(&ptp->rx_mutex);
+
+	/* Search the pending receive timestamps for a matching seqid */
+	list_for_each_entry(rxts, &ptp->rx_pend, node) {
+		if (rxts->seq == seqid) {
+			found = true;
+			ns = rxts->ns;
+			/* Move this timestamp entry to the free list */
+			list_move_tail(&rxts->node, &ptp->rx_free);
+			break;
+		}
+	}
+
+	if (!found) {
+		/* Store the seqid and queue the skb. Do this under the lock
+		 * to ensure we don't miss any timestamps appended to the
+		 * rx_pend list.
+		 */
+		MARVELL_PTP_CB(skb)->seq = seqid;
+		MARVELL_PTP_CB(skb)->timeout = jiffies +
+			msecs_to_jiffies(RX_TIMEOUT_MS);
+		__skb_queue_tail(&ptp->rx_queue, skb);
+	}
+
+	mutex_unlock(&ptp->rx_mutex);
+
+	if (found) {
+		/* We found the corresponding timestamp. If we can add the
+		 * timestamp, do we need to go through the netif_rx_ni()
+		 * path, or would it be more efficient to add the timestamp
+		 * and return "false" here?
+		 */
+		marvell_ptp_rx(skb, ns);
+	} else {
+		schedule_delayed_work(&ptp->ts_work, 2);
+	}
+
+	return true;
+}
+
+/* Move any expired skbs on to our own list, and then hand the contents of
+ * our list to netif_rx_ni() - this avoids calling netif_rx_ni() with our
+ * mutex held.
+ */
+static void marvell_ptp_rx_expire(struct marvell_ptp *ptp)
+{
+	struct sk_buff_head list;
+	struct sk_buff *skb;
+
+	__skb_queue_head_init(&list);
+
+	mutex_lock(&ptp->rx_mutex);
+	while ((skb = skb_dequeue(&ptp->rx_queue)) != NULL) {
+		if (!time_is_before_jiffies(MARVELL_PTP_CB(skb)->timeout)) {
+			__skb_queue_head(&ptp->rx_queue, skb);
+			break;
+		}
+		__skb_queue_tail(&list, skb);
+	}
+	mutex_unlock(&ptp->rx_mutex);
+
+	while ((skb = __skb_dequeue(&list)) != NULL)
+		netif_rx_ni(skb);
+}
+
+/* Complete the transmit timestamping; this is called to read the transmit
+ * timestamp from the PHY, and report back the transmitted timestamp.
+ */
+static int marvell_ptp_txtstamp_complete(struct marvell_ptp *ptp)
+{
+	struct skb_shared_hwtstamps shhwtstamps;
+	struct sk_buff *skb = ptp->tx_skb;
+	struct marvell_ts ts;
+	int err;
+	u64 ns;
+
+	err = marvell_read_tstamp(ptp->phydev, &ts, MARVELL_PAGE_PTP_PORT_2,
+				  PTP2_DEP_STATUS);
+	if (err < 0)
+		goto fail;
+
+	if (err == 0) {
+		if (time_is_before_jiffies(MARVELL_PTP_CB(skb)->timeout)) {
+			dev_warn(&ptp->phydev->mdio.dev,
+				 "tx timestamp timeout\n");
+			goto free;
+		}
+		return 0;
+	}
+
+	/* Check the status */
+	if ((ts.stat & MV_STATUS_INTSTATUS_MASK) !=
+	    MV_STATUS_INTSTATUS_NORMAL) {
+		dev_warn(&ptp->phydev->mdio.dev,
+			 "tx timestamp overrun (%x)\n", ts.stat);
+		goto free;
+	}
+
+	/* Reject if the sequence number doesn't match */
+	if (ts.seq != MARVELL_PTP_CB(skb)->seq) {
+		dev_warn(&ptp->phydev->mdio.dev,
+			 "tx timestamp unexpected sequence id\n");
+		goto free;
+	}
+
+	ptp->tx_skb = NULL;
+
+	/* Set the timestamp */
+	ns = marvell_tai_cyc2time(ptp->tai, ts.time);
+	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+	shhwtstamps.hwtstamp = ns_to_ktime(ns);
+	skb_complete_tx_timestamp(skb, &shhwtstamps);
+	return 1;
+
+fail:
+	dev_err_ratelimited(&ptp->phydev->mdio.dev,
+			    "failed reading PTP: %pe\n", ERR_PTR(err));
+free:
+	dev_kfree_skb_any(skb);
+	ptp->tx_skb = NULL;
+	return -1;
+}
+
+/* Check whether the skb will be timestamped on transmit; we only support
+ * a single outstanding skb. Add it if the slot is available.
+ */
+static bool marvell_ptp_do_txtstamp(struct mii_timestamper *mii_ts,
+				    struct sk_buff *skb, int type)
+{
+	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
+	u8 *ptp_hdr;
+
+	if (ptp->tx_type != HWTSTAMP_TX_ON)
+		return false;
+
+	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+		return false;
+
+	ptp_hdr = ptp_header(skb, type);
+	if (!ptp_hdr)
+		return false;
+
+	MARVELL_PTP_CB(skb)->seq = ptp_seqid(ptp_hdr);
+	MARVELL_PTP_CB(skb)->timeout = jiffies +
+		msecs_to_jiffies(TX_TIMEOUT_MS);
+
+	if (cmpxchg(&ptp->tx_skb, NULL, skb) != NULL)
+		return false;
+
+	/* DP83640 marks the skb for hw timestamping. Since the MAC driver
+	 * may call skb_tx_timestamp() but may not support timestamping
+	 * itself, it may not set this flag. So, we need to do this here.
+	 */
+	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+	schedule_delayed_work(&ptp->ts_work, 2);
+
+	return true;
+}
+
+static void marvell_ptp_txtstamp(struct mii_timestamper *mii_ts,
+				 struct sk_buff *skb, int type)
+{
+	if (!marvell_ptp_do_txtstamp(mii_ts, skb, type))
+		kfree_skb(skb);
+}
+
+static int marvell_ptp_hwtstamp(struct mii_timestamper *mii_ts,
+				struct ifreq *ifreq)
+{
+	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
+	struct hwtstamp_config config;
+	u16 cfg0 = PTP1_PORT_CONFIG_0_DISPTP;
+	u16 cfg2 = 0;
+	int err;
+
+	if (copy_from_user(&config, ifreq->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	if (config.flags)
+		return -EINVAL;
+
+	switch (config.tx_type) {
+	case HWTSTAMP_TX_OFF:
+		break;
+
+	case HWTSTAMP_TX_ON:
+		cfg0 = 0;
+		cfg2 |= PTP1_PORT_CONFIG_2_DEPINTEN;
+		break;
+
+	default:
+		return -ERANGE;
+	}
+
+	switch (config.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	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:
+		/* We accept 802.1AS, IEEE 1588v1 and IEEE 1588v2. We could
+		 * filter on 802.1AS using the transportSpecific field, but
+		 * that affects the transmit path too.
+		 */
+		config.rx_filter = HWTSTAMP_FILTER_SOME;
+		cfg0 = 0;
+		cfg2 |= PTP1_PORT_CONFIG_2_ARRINTEN;
+		break;
+
+	default:
+		return -ERANGE;
+	}
+
+	err = phy_modify_paged(ptp->phydev, MARVELL_PAGE_PTP_PORT_1,
+			       PTP1_PORT_CONFIG_0,
+			       PTP1_PORT_CONFIG_0_DISPTP, cfg0);
+	if (err)
+		return err;
+
+	err = phy_write_paged(ptp->phydev, MARVELL_PAGE_PTP_PORT_1,
+			      PTP1_PORT_CONFIG_2, cfg2);
+	if (err)
+		return err;
+
+	ptp->tx_type = config.tx_type;
+	ptp->rx_filter = config.rx_filter;
+
+	return copy_to_user(ifreq->ifr_data, &config, sizeof(config)) ?
+		-EFAULT : 0;
+}
+
+static int marvell_ptp_ts_info(struct mii_timestamper *mii_ts,
+			       struct ethtool_ts_info *ts_info)
+{
+	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
+
+	ts_info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+				   SOF_TIMESTAMPING_RX_HARDWARE |
+				   SOF_TIMESTAMPING_RAW_HARDWARE;
+	ts_info->phc_index = ptp_clock_index(ptp->tai->ptp_clock);
+	ts_info->tx_types = BIT(HWTSTAMP_TX_OFF) |
+			    BIT(HWTSTAMP_TX_ON);
+	ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
+			      BIT(HWTSTAMP_FILTER_SOME);
+
+	return 0;
+}
+
+static int marvell_ptp_port_config(struct phy_device *phydev)
+{
+	int err;
+
+	/* Disable transport specific check (if the PTP common header)
+	 * Disable timestamp overwriting (so we can read a stable entry.)
+	 * Disable PTP
+	 */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_PORT_1,
+			      PTP1_PORT_CONFIG_0,
+			      PTP1_PORT_CONFIG_0_DISTSPECCHECK |
+			      PTP1_PORT_CONFIG_0_DISTSOVERWRITE |
+			      PTP1_PORT_CONFIG_0_DISPTP);
+	if (err < 0)
+		return err;
+
+	/* Set ether-type jump to 12 (to ether protocol)
+	 * Set IP jump to 2 (to skip over ether protocol)
+	 * Does this mean it won't pick up on VLAN packets?
+	 */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_PORT_1,
+			      PTP1_PORT_CONFIG_1,
+			      PTP1_PORT_CONFIG_1_ETJUMP(12) |
+			      PTP1_PORT_CONFIG_1_IPJUMP(2));
+	if (err < 0)
+		return err;
+
+	/* Disable all interrupts */
+	phy_write_paged(phydev, MARVELL_PAGE_PTP_PORT_1,
+			PTP1_PORT_CONFIG_2, 0);
+
+	return 0;
+}
+
+static void marvell_ptp_port_disable(struct phy_device *phydev)
+{
+	/* Disable PTP */
+	phy_write_paged(phydev, MARVELL_PAGE_PTP_PORT_1,
+			PTP1_PORT_CONFIG_0, PTP1_PORT_CONFIG_0_DISPTP);
+
+	/* Disable interrupts */
+	phy_write_paged(phydev, MARVELL_PAGE_PTP_PORT_1,
+			PTP1_PORT_CONFIG_2, 0);
+}
+
+/* This function should be called from the PHY threaded interrupt
+ * handler to process any stored timestamps in a timely manner.
+ * The presence of an interrupt has an effect on how quickly a
+ * timestamp requiring received packet will be processed.
+ */
+irqreturn_t marvell_ptp_irq(struct phy_device *phydev)
+{
+	struct marvell_ptp *ptp;
+	irqreturn_t ret = IRQ_NONE;
+
+	if (!phydev->mii_ts)
+		return ret;
+
+	ptp = mii_ts_to_ptp(phydev->mii_ts);
+	if (marvell_ptp_rx_ts(ptp))
+		ret = IRQ_HANDLED;
+
+	if (ptp->tx_skb && marvell_ptp_txtstamp_complete(ptp))
+		ret = IRQ_HANDLED;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(marvell_ptp_irq);
+
+static void marvell_ptp_worker(struct work_struct *work)
+{
+	struct marvell_ptp *ptp = container_of(work, struct marvell_ptp,
+					       ts_work.work);
+
+	marvell_ptp_rx_ts(ptp);
+
+	if (ptp->tx_skb)
+		marvell_ptp_txtstamp_complete(ptp);
+
+	marvell_ptp_rx_expire(ptp);
+
+	if (!skb_queue_empty(&ptp->rx_queue) || ptp->tx_skb)
+		schedule_delayed_work(&ptp->ts_work, 2);
+}
+
+int marvell_ptp_probe(struct phy_device *phydev)
+{
+	struct marvell_ptp *ptp;
+	int err, i;
+
+	ptp = devm_kzalloc(&phydev->mdio.dev, sizeof(*ptp), GFP_KERNEL);
+	if (!ptp)
+		return -ENOMEM;
+
+	ptp->phydev = phydev;
+	ptp->mii_ts.rxtstamp = marvell_ptp_rxtstamp;
+	ptp->mii_ts.txtstamp = marvell_ptp_txtstamp;
+	ptp->mii_ts.hwtstamp = marvell_ptp_hwtstamp;
+	ptp->mii_ts.ts_info = marvell_ptp_ts_info;
+
+	INIT_DELAYED_WORK(&ptp->ts_work, marvell_ptp_worker);
+	mutex_init(&ptp->rx_mutex);
+	INIT_LIST_HEAD(&ptp->rx_free);
+	INIT_LIST_HEAD(&ptp->rx_pend);
+	skb_queue_head_init(&ptp->rx_queue);
+
+	for (i = 0; i < ARRAY_SIZE(ptp->rx_ts); i++)
+		list_add_tail(&ptp->rx_ts[i].node, &ptp->rx_free);
+
+	/* Get the TAI for this PHY. */
+	err = marvell_tai_get(&ptp->tai, phydev);
+	if (err)
+		return err;
+
+	/* Configure this PTP port */
+	err = marvell_ptp_port_config(phydev);
+	if (err) {
+		marvell_tai_put(ptp->tai);
+		return err;
+	}
+
+	phydev->mii_ts = &ptp->mii_ts;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(marvell_ptp_probe);
+
+void marvell_ptp_remove(struct phy_device *phydev)
+{
+	struct marvell_ptp *ptp;
+
+	if (!phydev->mii_ts)
+		return;
+
+	/* Disconnect from the net subsystem - we assume there is no
+	 * packet activity at this point.
+	 */
+	ptp = mii_ts_to_ptp(phydev->mii_ts);
+	phydev->mii_ts = NULL;
+
+	cancel_delayed_work_sync(&ptp->ts_work);
+
+	/* Free or dequeue all pending skbs */
+	if (ptp->tx_skb)
+		kfree_skb(ptp->tx_skb);
+
+	skb_queue_purge(&ptp->rx_queue);
+
+	/* Ensure that the port is disabled */
+	marvell_ptp_port_disable(phydev);
+	marvell_tai_put(ptp->tai);
+}
+EXPORT_SYMBOL_GPL(marvell_ptp_remove);
+
+MODULE_AUTHOR("Russell King");
+MODULE_DESCRIPTION("Marvell PHY PTP library");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/net/phy/marvell_ptp.h b/drivers/net/phy/marvell_ptp.h
new file mode 100644
index 000000000000..1ae65bb5c298
--- /dev/null
+++ b/drivers/net/phy/marvell_ptp.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef MARVELL_PTP_H
+#define MARVELL_PTP_H
+
+#if IS_ENABLED(CONFIG_MARVELL_PHY_PTP)
+void marvell_ptp_check(struct phy_device *phydev);
+int marvell_ptp_probe(struct phy_device *phydev);
+void marvell_ptp_remove(struct phy_device *phydev);
+#else
+static inline int marvell_ptp_probe(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static inline void marvell_ptp_remove(struct phy_device *phydev)
+{
+}
+#endif
+
+#endif
diff --git a/drivers/net/phy/marvell_tai.c b/drivers/net/phy/marvell_tai.c
new file mode 100644
index 000000000000..f6608a0986ca
--- /dev/null
+++ b/drivers/net/phy/marvell_tai.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell PTP driver for 88E1510, 88E1512, 88E1514 and 88E1518 PHYs
+ *
+ * This file implements TAI support as a PTP clock. Timecounter/cyclecounter
+ * representation taken from Marvell 88E6xxx DSA driver. We may need to share
+ * the TAI between multiple PHYs in a multiport PHY.
+ */
+#include <linux/ktime.h>
+#include <linux/slab.h>
+#include <linux/phy.h>
+#include "marvell_tai.h"
+
+#define MARVELL_PAGE_MISC			6
+#define GCR					20
+#define GCR_PTP_POWER_DOWN			BIT(9)
+#define GCR_PTP_REF_CLOCK_SOURCE		BIT(8)
+#define GCR_PTP_INPUT_SOURCE			BIT(7)
+#define GCR_PTP_OUTPUT				BIT(6)
+
+#define MARVELL_PAGE_TAI_GLOBAL			12
+#define TAI_CONFIG_0				0
+#define TAI_CONFIG_0_EVENTCAPOV			BIT(15)
+#define TAI_CONFIG_0_TRIGGENINTEN		BIT(9)
+#define TAI_CONFIG_0_EVENTCAPINTEN		BIT(8)
+
+#define TAI_CONFIG_9				9
+#define TAI_CONFIG_9_EVENTERR			BIT(9)
+#define TAI_CONFIG_9_EVENTCAPVALID		BIT(8)
+
+#define TAI_EVENT_CAPTURE_TIME_LO		10
+#define TAI_EVENT_CAPTURE_TIME_HI		11
+
+#define MARVELL_PAGE_PTP_GLOBAL			14
+#define PTPG_CONFIG_0				0
+#define PTPG_CONFIG_1				1
+#define PTPG_CONFIG_2				2
+#define PTPG_CONFIG_3				3
+#define PTPG_CONFIG_3_TSATSFD			BIT(0)
+#define PTPG_STATUS				8
+#define PTPG_READPLUS_COMMAND			14
+#define PTPG_READPLUS_DATA			15
+
+static DEFINE_SPINLOCK(tai_list_lock);
+static LIST_HEAD(tai_list);
+
+static struct marvell_tai *cc_to_tai(const struct cyclecounter *cc)
+{
+	return container_of(cc, struct marvell_tai, cyclecounter);
+}
+
+/* Read the global time registers using the readplus command */
+static u64 marvell_tai_clock_read(const struct cyclecounter *cc)
+{
+	struct marvell_tai *tai = cc_to_tai(cc);
+	struct phy_device *phydev = tai->phydev;
+	int err, oldpage, lo, hi;
+
+	oldpage = phy_select_page(phydev, MARVELL_PAGE_PTP_GLOBAL);
+	if (oldpage >= 0) {
+		/* 88e151x says to write 0x8e0e */
+		ptp_read_system_prets(tai->sts);
+		err = __phy_write(phydev, PTPG_READPLUS_COMMAND, 0x8e0e);
+		ptp_read_system_postts(tai->sts);
+		lo = __phy_read(phydev, PTPG_READPLUS_DATA);
+		hi = __phy_read(phydev, PTPG_READPLUS_DATA);
+	}
+	err = phy_restore_page(phydev, oldpage, err);
+
+	if (err || lo < 0 || hi < 0)
+		return 0;
+
+	return lo | hi << 16;
+}
+
+u64 marvell_tai_cyc2time(struct marvell_tai *tai, u32 cyc)
+{
+	u64 ns;
+
+	mutex_lock(&tai->mutex);
+	ns = timecounter_cyc2time(&tai->timecounter, cyc);
+	mutex_unlock(&tai->mutex);
+
+	return ns;
+}
+
+static struct marvell_tai *ptp_to_tai(struct ptp_clock_info *ptp)
+{
+	return container_of(ptp, struct marvell_tai, caps);
+}
+
+static int marvell_tai_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct marvell_tai *tai = ptp_to_tai(ptp);
+	bool neg;
+	u32 diff;
+	u64 adj;
+
+	neg = scaled_ppm < 0;
+	if (neg)
+		scaled_ppm = -scaled_ppm;
+
+	adj = tai->cc_mult_num;
+	adj *= scaled_ppm;
+	diff = div_u64(adj, tai->cc_mult_den);
+
+	mutex_lock(&tai->mutex);
+	timecounter_read(&tai->timecounter);
+	tai->cyclecounter.mult = neg ? tai->cc_mult - diff :
+				       tai->cc_mult + diff;
+	mutex_unlock(&tai->mutex);
+
+	return 0;
+}
+
+static int marvell_tai_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct marvell_tai *tai = ptp_to_tai(ptp);
+
+	mutex_lock(&tai->mutex);
+	timecounter_adjtime(&tai->timecounter, delta);
+	mutex_unlock(&tai->mutex);
+
+	return 0;
+}
+
+static int marvell_tai_gettimex64(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts,
+				  struct ptp_system_timestamp *sts)
+{
+	struct marvell_tai *tai = ptp_to_tai(ptp);
+	u64 ns;
+
+	mutex_lock(&tai->mutex);
+	tai->sts = sts;
+	ns = timecounter_read(&tai->timecounter);
+	tai->sts = NULL;
+	mutex_unlock(&tai->mutex);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
+static int marvell_tai_settime64(struct ptp_clock_info *ptp,
+				 const struct timespec64 *ts)
+{
+	struct marvell_tai *tai = ptp_to_tai(ptp);
+	u64 ns = timespec64_to_ns(ts);
+
+	mutex_lock(&tai->mutex);
+	timecounter_init(&tai->timecounter, &tai->cyclecounter, ns);
+	mutex_unlock(&tai->mutex);
+
+	return 0;
+}
+
+/* Periodically read the timecounter to keep the time refreshed. */
+static long marvell_tai_aux_work(struct ptp_clock_info *ptp)
+{
+	struct marvell_tai *tai = ptp_to_tai(ptp);
+
+	mutex_lock(&tai->mutex);
+	timecounter_read(&tai->timecounter);
+	mutex_unlock(&tai->mutex);
+
+	return tai->half_overflow_period;
+}
+
+/* Configure the global (shared between ports) configuration for the PHY. */
+static int marvell_tai_global_config(struct phy_device *phydev)
+{
+	int err;
+
+	/* Power up PTP */
+	err = phy_modify_paged(phydev, MARVELL_PAGE_MISC, GCR,
+			       GCR_PTP_POWER_DOWN, 0);
+	if (err)
+		return err;
+
+	/* Set ether-type for IEEE1588 packets */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_GLOBAL,
+			      PTPG_CONFIG_0, ETH_P_1588);
+	if (err < 0)
+		return err;
+
+	/* MsdIDTSEn - Enable timestamping on all PTP MessageIDs */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_GLOBAL,
+			      PTPG_CONFIG_1, ~0);
+	if (err < 0)
+		return err;
+
+	/* TSArrPtr - Point to Arr0 registers */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_GLOBAL,
+			      PTPG_CONFIG_2, 0);
+	if (err < 0)
+		return err;
+
+	/* TSAtSFD - timestamp at SFD */
+	err = phy_write_paged(phydev, MARVELL_PAGE_PTP_GLOBAL,
+			      PTPG_CONFIG_3, PTPG_CONFIG_3_TSATSFD);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+int marvell_tai_get(struct marvell_tai **taip, struct phy_device *phydev)
+{
+	struct marvell_tai *tai;
+	unsigned long overflow_ms;
+	int err;
+
+	err = marvell_tai_global_config(phydev);
+	if (err < 0)
+		return err;
+
+	tai = kzalloc(sizeof(*tai), GFP_KERNEL);
+	if (!tai)
+		return -ENOMEM;
+
+	mutex_init(&tai->mutex);
+
+	tai->phydev = phydev;
+
+	/* This assumes a 125MHz clock */
+	tai->cc_mult = 8 << 28;
+	tai->cc_mult_num = 1 << 9;
+	tai->cc_mult_den = 15625U;
+
+	tai->cyclecounter.read = marvell_tai_clock_read;
+	tai->cyclecounter.mask = CYCLECOUNTER_MASK(32);
+	tai->cyclecounter.mult = tai->cc_mult;
+	tai->cyclecounter.shift = 28;
+
+	overflow_ms = (1ULL << 32 * tai->cc_mult * 1000) >>
+			tai->cyclecounter.shift;
+	tai->half_overflow_period = msecs_to_jiffies(overflow_ms / 2);
+
+	timecounter_init(&tai->timecounter, &tai->cyclecounter,
+			 ktime_to_ns(ktime_get_real()));
+
+	tai->caps.owner = THIS_MODULE;
+	snprintf(tai->caps.name, sizeof(tai->caps.name), "Marvell PHY");
+	/* max_adj of 1000000 is what MV88E6xxx DSA uses */
+	tai->caps.max_adj = 1000000;
+	tai->caps.adjfine = marvell_tai_adjfine;
+	tai->caps.adjtime = marvell_tai_adjtime;
+	tai->caps.gettimex64 = marvell_tai_gettimex64;
+	tai->caps.settime64 = marvell_tai_settime64;
+	tai->caps.do_aux_work = marvell_tai_aux_work;
+
+	tai->ptp_clock = ptp_clock_register(&tai->caps, &phydev->mdio.dev);
+	if (IS_ERR(tai->ptp_clock)) {
+		kfree(tai);
+		return PTR_ERR(tai->ptp_clock);
+	}
+
+	ptp_schedule_worker(tai->ptp_clock, tai->half_overflow_period);
+
+	spin_lock(&tai_list_lock);
+	list_add_tail(&tai->tai_node, &tai_list);
+	spin_unlock(&tai_list_lock);
+
+	*taip = tai;
+
+	return 0;
+}
+
+void marvell_tai_put(struct marvell_tai *tai)
+{
+	ptp_clock_unregister(tai->ptp_clock);
+
+	spin_lock(&tai_list_lock);
+	list_del(&tai->tai_node);
+	spin_unlock(&tai_list_lock);
+
+	kfree(tai);
+}
diff --git a/drivers/net/phy/marvell_tai.h b/drivers/net/phy/marvell_tai.h
new file mode 100644
index 000000000000..9752f6cd58da
--- /dev/null
+++ b/drivers/net/phy/marvell_tai.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef MARVELL_TAI_H
+#define MARVELL_TAI_H
+
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/timecounter.h>
+
+struct phy_device;
+
+struct marvell_tai {
+	struct list_head tai_node;
+	struct phy_device *phydev;
+
+	struct ptp_clock_info caps;
+	struct ptp_clock *ptp_clock;
+
+	u32 cc_mult_num;
+	u32 cc_mult_den;
+	u32 cc_mult;
+
+	struct mutex mutex;
+	struct timecounter timecounter;
+	struct cyclecounter cyclecounter;
+	long half_overflow_period;
+
+	/* Used while reading the TAI */
+	struct ptp_system_timestamp *sts;
+};
+
+u64 marvell_tai_cyc2time(struct marvell_tai *tai, u32 cyc);
+int marvell_tai_get(struct marvell_tai **taip, struct phy_device *phydev);
+void marvell_tai_put(struct marvell_tai *tai);
+
+#endif
-- 
2.20.1

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Andrew Lunn]

> Getting the Kconfig for this correct has been a struggle - particularly
> the combination where PTP support is modular.  It's rather odd to have
> the Marvell PTP support asked before the Marvell PHY support.  I
> couldn't work out any other reasonable way to ensure that we always
> have a valid configuration, without leading to stupidities such as
> having the PTP and Marvell PTP support modular, but non-functional
> because Marvell PHY is built-in.

Hi Russell

How much object code is this adding? All the other PHYs which support
PTP just make it part of the PHY driver, not a standalone module. That
i guess simplifies the conditions. 

Looking at DSDT, it lists

        case MAD_88E1340S:
        case MAD_88E1340:
        case MAD_88E1340M:
        case MAD_SWG65G : 
	case MAD_88E151x:

as being MAD_PHY_PTP_TAI_CAPABLE;

and

	case MAD_88E1548
        case MAD_88E1680:
        case MAD_88E1680M:

as MAD_PHY_1STEP_PTP_CAPABLE;

So maybe we can wire this up to a few more PHYs to 'lower' the
overhead a bit?

> It seems that the Marvell PHY PTP is very similar to that found in
> their DSA chips, which suggests maybe we should share the code, but
> different access methods would be required.

That makes the Kconfig even more complex :-(

     Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Wed, Jul 15, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> > Getting the Kconfig for this correct has been a struggle - particularly
> > the combination where PTP support is modular.  It's rather odd to have
> > the Marvell PTP support asked before the Marvell PHY support.  I
> > couldn't work out any other reasonable way to ensure that we always
> > have a valid configuration, without leading to stupidities such as
> > having the PTP and Marvell PTP support modular, but non-functional
> > because Marvell PHY is built-in.
> 
> Hi Russell
> 
> How much object code is this adding? All the other PHYs which support
> PTP just make it part of the PHY driver, not a standalone module. That
> i guess simplifies the conditions. 

Taking an arm64 build, the PHY driver is 16k and the PTP driver comes
in just under 8k.

> Looking at DSDT, it lists
> 
>         case MAD_88E1340S:
>         case MAD_88E1340:
>         case MAD_88E1340M:
>         case MAD_SWG65G : 
> 	case MAD_88E151x:
> 
> as being MAD_PHY_PTP_TAI_CAPABLE;
> 
> and
> 
> 	case MAD_88E1548
>         case MAD_88E1680:
>         case MAD_88E1680M:
> 
> as MAD_PHY_1STEP_PTP_CAPABLE;
> 
> So maybe we can wire this up to a few more PHYs to 'lower' the
> overhead a bit?

That's interesting, the 1548 information (covering 1543 and 1545 as
well) I have doesn't mention anything about PTP.

> > It seems that the Marvell PHY PTP is very similar to that found in
> > their DSA chips, which suggests maybe we should share the code, but
> > different access methods would be required.
> 
> That makes the Kconfig even more complex :-(

We already have that complexity due to the fact that we are interacting
with two subsystems.  The 88e6xxx Kconfig entry has:

config NET_DSA_MV88E6XXX_PTP
        bool "PTP support for Marvell 88E6xxx"
        default n
        depends on NET_DSA_MV88E6XXX_GLOBAL2
        depends on PTP_1588_CLOCK
        imply NETWORK_PHY_TIMESTAMPING

and I've been wondering what that means when PTP_1588_CLOCK=m while
NET_DSA_MV88E6XXX_GLOBAL2=y and NET_DSA_MV88E6XXX=y.  If this is
selectable, then it seems to be misleading the user - you can't have
the PTP subsystem modular, and have PTP drivers built-in to the
kernel.

Yes, we have the inteligence to be able to make the various PTP calls
be basically no-ops, but it's not nice.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Wed, Jul 15, 2020 at 07:56:19PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jul 15, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> > > Getting the Kconfig for this correct has been a struggle - particularly
> > > the combination where PTP support is modular.  It's rather odd to have
> > > the Marvell PTP support asked before the Marvell PHY support.  I
> > > couldn't work out any other reasonable way to ensure that we always
> > > have a valid configuration, without leading to stupidities such as
> > > having the PTP and Marvell PTP support modular, but non-functional
> > > because Marvell PHY is built-in.
> > 
> > Hi Russell
> > 
> > How much object code is this adding? All the other PHYs which support
> > PTP just make it part of the PHY driver, not a standalone module. That
> > i guess simplifies the conditions. 
> 
> Taking an arm64 build, the PHY driver is 16k and the PTP driver comes
> in just under 8k.
> 
> > Looking at DSDT, it lists
> > 
> >         case MAD_88E1340S:
> >         case MAD_88E1340:
> >         case MAD_88E1340M:
> >         case MAD_SWG65G : 
> > 	case MAD_88E151x:
> > 
> > as being MAD_PHY_PTP_TAI_CAPABLE;
> > 
> > and
> > 
> > 	case MAD_88E1548
> >         case MAD_88E1680:
> >         case MAD_88E1680M:
> > 
> > as MAD_PHY_1STEP_PTP_CAPABLE;
> > 
> > So maybe we can wire this up to a few more PHYs to 'lower' the
> > overhead a bit?
> 
> That's interesting, the 1548 information (covering 1543 and 1545 as
> well) I have doesn't mention anything about PTP.
> 
> > > It seems that the Marvell PHY PTP is very similar to that found in
> > > their DSA chips, which suggests maybe we should share the code, but
> > > different access methods would be required.
> > 
> > That makes the Kconfig even more complex :-(
> 
> We already have that complexity due to the fact that we are interacting
> with two subsystems.  The 88e6xxx Kconfig entry has:
> 
> config NET_DSA_MV88E6XXX_PTP
>         bool "PTP support for Marvell 88E6xxx"
>         default n
>         depends on NET_DSA_MV88E6XXX_GLOBAL2
>         depends on PTP_1588_CLOCK
>         imply NETWORK_PHY_TIMESTAMPING
> 
> and I've been wondering what that means when PTP_1588_CLOCK=m while
> NET_DSA_MV88E6XXX_GLOBAL2=y and NET_DSA_MV88E6XXX=y.  If this is
> selectable, then it seems to be misleading the user - you can't have
> the PTP subsystem modular, and have PTP drivers built-in to the
> kernel.
> 
> Yes, we have the inteligence to be able to make the various PTP calls
> be basically no-ops, but it's not nice.

Sure enough:

CONFIG_NET_DSA_MV88E6XXX=y
CONFIG_NET_DSA_MV88E6XXX_GLOBAL2=y
CONFIG_NET_DSA_MV88E6XXX_PTP=y
CONFIG_PTP_1588_CLOCK=m

is a possible configuration, but all the PTP calls from mv88e6xxx are
stubbed out (since the IS_REACHABLE() in linux/ptp_clock_kernel.h is
false.)

The DP83640 PHY works around this by introducing a hard dependency on
PTP:

config DP83640_PHY
	tristate "Driver for the National Semiconductor DP83640 PHYTER"
	depends on NETWORK_PHY_TIMESTAMPING
	depends on PHYLIB
	depends on PTP_1588_CLOCK

A possible solution to this would be for the PTP core code to be a
separate Kconfig symbol that is not offered to the user, but is
selected by the various drivers and Kconfig entries that need that
support.  That way, it would automatically be modular or built-in
as required by its users.

PTP_1588_CLOCK could then be used as a global enable for PTP support
where appropriate.  We can also avoid all the Kconfig complexity
introduced by having two independent subsystems either of which could
be modular.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Tue, Jul 14, 2020 at 05:26:28PM +0100, Russell King wrote:

> With this driver, there appears to be three instances of a ptp_header()
> like function in the kernel - I think that ought to become a helper in
> generic code.

Yes, and the hellcreek DSA driver will add yet another.  I'll be happy
if patches that refactor everything this magically appear!

> +/* Read the status, timestamp and PTP common header sequence from the PHY.
> + * Apparently, reading these are atomic, but there is no mention how the
> + * PHY treats this access as atomic. So, we set the DisTSOverwrite bit
> + * when configuring the PHY.
> + */
> +static int marvell_read_tstamp(struct phy_device *phydev,
> +			       struct marvell_ts *ts,
> +			       uint8_t page, uint8_t reg)
> +{
> +	int oldpage;
> +	int ret;
> +
> +	/* Read status register */
> +	oldpage = phy_select_page(phydev, page);
> +	if (oldpage >= 0) {

Suggest avoiding the IfOk anti-pattern.

> +		ret = __phy_read(phydev, reg);
> +		if (ret < 0)
> +			goto restore;
> +
> +		ts->stat = ret;
> +		if (!(ts->stat & MV_STATUS_VALID)) {
> +			ret = 0;
> +			goto restore;
> +		}
> +
> +		/* Read low timestamp */
> +		ret = __phy_read(phydev, reg + 1);
> +		if (ret < 0)
> +			goto restore;
> +
> +		ts->time = ret;
> +
> +		/* Read high timestamp */
> +		ret = __phy_read(phydev, reg + 2);
> +		if (ret < 0)
> +			goto restore;
> +
> +		ts->time |= ret << 16;
> +
> +		/* Read sequence */
> +		ret = __phy_read(phydev, reg + 3);
> +		if (ret < 0)
> +			goto restore;
> +
> +		ts->seq = ret;
> +
> +		/* Clear valid */
> +		__phy_write(phydev, reg, 0);
> +	}
> +restore:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}


> +/* Check whether the packet is suitable for timestamping, and if so,
> + * try to find a pending timestamp for it. If no timestamp is found,
> + * queue the packet with a timeout.
> + */
> +static bool marvell_ptp_rxtstamp(struct mii_timestamper *mii_ts,
> +				 struct sk_buff *skb, int type)
> +{
> +	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
> +	struct marvell_rxts *rxts;
> +	bool found = false;
> +	u8 *ptp_hdr;
> +	u16 seqid;
> +	u64 ns;
> +
> +	if (ptp->rx_filter == HWTSTAMP_FILTER_NONE)
> +		return false;
> +
> +	ptp_hdr = ptp_header(skb, type);
> +	if (!ptp_hdr)
> +		return false;
> +
> +	seqid = ptp_seqid(ptp_hdr);
> +
> +	mutex_lock(&ptp->rx_mutex);
> +
> +	/* Search the pending receive timestamps for a matching seqid */
> +	list_for_each_entry(rxts, &ptp->rx_pend, node) {
> +		if (rxts->seq == seqid) {
> +			found = true;
> +			ns = rxts->ns;
> +			/* Move this timestamp entry to the free list */
> +			list_move_tail(&rxts->node, &ptp->rx_free);
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		/* Store the seqid and queue the skb. Do this under the lock
> +		 * to ensure we don't miss any timestamps appended to the
> +		 * rx_pend list.
> +		 */
> +		MARVELL_PTP_CB(skb)->seq = seqid;
> +		MARVELL_PTP_CB(skb)->timeout = jiffies +
> +			msecs_to_jiffies(RX_TIMEOUT_MS);
> +		__skb_queue_tail(&ptp->rx_queue, skb);
> +	}
> +
> +	mutex_unlock(&ptp->rx_mutex);
> +
> +	if (found) {
> +		/* We found the corresponding timestamp. If we can add the
> +		 * timestamp, do we need to go through the netif_rx_ni()
> +		 * path, or would it be more efficient to add the timestamp
> +		 * and return "false" here?
> +		 */

The caller expects the driver to deliver the skb.  If you return the
skb with your hwtstamp value, there is no guarantee that the caller
won't clobber it.

> +		marvell_ptp_rx(skb, ns);
> +	} else {
> +		schedule_delayed_work(&ptp->ts_work, 2);

Instead of using a separate work item, consider combining this with
the counter overflow logic in caps.do_aux_work = marvell_tai_aux_work.
The advantage of do_aux_work is that you get your own kernel thread
which can be given scheduling priority via chrt on busy systems.

> +	}
> +
> +	return true;
> +}


> +/* Check whether the skb will be timestamped on transmit; we only support
> + * a single outstanding skb. Add it if the slot is available.
> + */
> +static bool marvell_ptp_do_txtstamp(struct mii_timestamper *mii_ts,
> +				    struct sk_buff *skb, int type)
> +{
> +	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
> +	u8 *ptp_hdr;
> +
> +	if (ptp->tx_type != HWTSTAMP_TX_ON)
> +		return false;
> +
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +		return false;
> +
> +	ptp_hdr = ptp_header(skb, type);
> +	if (!ptp_hdr)
> +		return false;
> +
> +	MARVELL_PTP_CB(skb)->seq = ptp_seqid(ptp_hdr);
> +	MARVELL_PTP_CB(skb)->timeout = jiffies +
> +		msecs_to_jiffies(TX_TIMEOUT_MS);
> +
> +	if (cmpxchg(&ptp->tx_skb, NULL, skb) != NULL)
> +		return false;
> +
> +	/* DP83640 marks the skb for hw timestamping. Since the MAC driver
> +	 * may call skb_tx_timestamp() but may not support timestamping
> +	 * itself, it may not set this flag. So, we need to do this here.
> +	 */

The purpose of this flag is to prevent a SW time stamp from being
delivered when the user dialed HW time stamps only.  The PHY driver is
expected to set this flag all by itself regardless of the MAC driver.
Unfortunately the stack doesn't support simultaneous MAC and PHY time
stamping, and so it is up to the MAC driver to avoid setting this flag
when a time stamping PHY is attached.

> +	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +	schedule_delayed_work(&ptp->ts_work, 2);
> +
> +	return true;
> +}
> +
> +static void marvell_ptp_txtstamp(struct mii_timestamper *mii_ts,
> +				 struct sk_buff *skb, int type)
> +{
> +	if (!marvell_ptp_do_txtstamp(mii_ts, skb, type))
> +		kfree_skb(skb);
> +}
> +
> +static int marvell_ptp_hwtstamp(struct mii_timestamper *mii_ts,
> +				struct ifreq *ifreq)
> +{
> +	struct marvell_ptp *ptp = mii_ts_to_ptp(mii_ts);
> +	struct hwtstamp_config config;
> +	u16 cfg0 = PTP1_PORT_CONFIG_0_DISPTP;
> +	u16 cfg2 = 0;
> +	int err;
> +
> +	if (copy_from_user(&config, ifreq->ifr_data, sizeof(config)))
> +		return -EFAULT;
> +
> +	if (config.flags)
> +		return -EINVAL;
> +
> +	switch (config.tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		break;
> +
> +	case HWTSTAMP_TX_ON:
> +		cfg0 = 0;
> +		cfg2 |= PTP1_PORT_CONFIG_2_DEPINTEN;
> +		break;
> +
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config.rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		break;
> +
> +	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +	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:
> +		/* We accept 802.1AS, IEEE 1588v1 and IEEE 1588v2. We could
> +		 * filter on 802.1AS using the transportSpecific field, but
> +		 * that affects the transmit path too.
> +		 */
> +		config.rx_filter = HWTSTAMP_FILTER_SOME;

I think you want HWTSTAMP_FILTER_PTP_V2_EVENT here.

From Documentation/networking/timestamping.rst...

   Drivers are free to use a more permissive configuration than the requested
   configuration. It is expected that drivers should only implement directly the
   most generic mode that can be supported. For example if the hardware can
   support HWTSTAMP_FILTER_V2_EVENT, then it should generally always upscale
   HWTSTAMP_FILTER_V2_L2_SYNC_MESSAGE, and so forth, as HWTSTAMP_FILTER_V2_EVENT
   is more generic (and more useful to applications).

   A driver which supports hardware time stamping shall update the struct
   with the actual, possibly more permissive configuration. If the
   requested packets cannot be time stamped, then nothing should be
   changed and ERANGE shall be returned (in contrast to EINVAL, which
   indicates that SIOCSHWTSTAMP is not supported at all).

> +		Cfg0 = 0;
> +		cfg2 |= PTP1_PORT_CONFIG_2_ARRINTEN;
> +		break;
> +
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	err = phy_modify_paged(ptp->phydev, MARVELL_PAGE_PTP_PORT_1,
> +			       PTP1_PORT_CONFIG_0,
> +			       PTP1_PORT_CONFIG_0_DISPTP, cfg0);
> +	if (err)
> +		return err;
> +
> +	err = phy_write_paged(ptp->phydev, MARVELL_PAGE_PTP_PORT_1,
> +			      PTP1_PORT_CONFIG_2, cfg2);
> +	if (err)
> +		return err;
> +
> +	ptp->tx_type = config.tx_type;
> +	ptp->rx_filter = config.rx_filter;
> +
> +	return copy_to_user(ifreq->ifr_data, &config, sizeof(config)) ?
> +		-EFAULT : 0;
> +}


> +int marvell_tai_get(struct marvell_tai **taip, struct phy_device *phydev)
> +{
> +	struct marvell_tai *tai;
> +	unsigned long overflow_ms;
> +	int err;
> +
> +	err = marvell_tai_global_config(phydev);
> +	if (err < 0)
> +		return err;
> +
> +	tai = kzalloc(sizeof(*tai), GFP_KERNEL);
> +	if (!tai)
> +		return -ENOMEM;
> +
> +	mutex_init(&tai->mutex);
> +
> +	tai->phydev = phydev;
> +
> +	/* This assumes a 125MHz clock */
> +	tai->cc_mult = 8 << 28;
> +	tai->cc_mult_num = 1 << 9;
> +	tai->cc_mult_den = 15625U;
> +
> +	tai->cyclecounter.read = marvell_tai_clock_read;
> +	tai->cyclecounter.mask = CYCLECOUNTER_MASK(32);
> +	tai->cyclecounter.mult = tai->cc_mult;
> +	tai->cyclecounter.shift = 28;
> +
> +	overflow_ms = (1ULL << 32 * tai->cc_mult * 1000) >>
> +			tai->cyclecounter.shift;
> +	tai->half_overflow_period = msecs_to_jiffies(overflow_ms / 2);
> +
> +	timecounter_init(&tai->timecounter, &tai->cyclecounter,
> +			 ktime_to_ns(ktime_get_real()));
> +
> +	tai->caps.owner = THIS_MODULE;
> +	snprintf(tai->caps.name, sizeof(tai->caps.name), "Marvell PHY");
> +	/* max_adj of 1000000 is what MV88E6xxx DSA uses */
> +	tai->caps.max_adj = 1000000;
> +	tai->caps.adjfine = marvell_tai_adjfine;
> +	tai->caps.adjtime = marvell_tai_adjtime;
> +	tai->caps.gettimex64 = marvell_tai_gettimex64;
> +	tai->caps.settime64 = marvell_tai_settime64;
> +	tai->caps.do_aux_work = marvell_tai_aux_work;
> +
> +	tai->ptp_clock = ptp_clock_register(&tai->caps, &phydev->mdio.dev);
> +	if (IS_ERR(tai->ptp_clock)) {
> +		kfree(tai);
> +		return PTR_ERR(tai->ptp_clock);
> +	}
> +
> +	ptp_schedule_worker(tai->ptp_clock, tai->half_overflow_period);

ptp_clock_register() can return NULL, and so you should check for that
case before passing tai->ptp_clock here.

 * ptp_clock_register() - register a PTP hardware clock driver
 *
 * @info:   Structure describing the new clock.
 * @parent: Pointer to the parent device of the new clock.
 *
 * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
 * support is missing at the configuration level, this function
 * returns NULL, and drivers are expected to gracefully handle that
 * case separately.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Thu, Jul 16, 2020 at 12:33:54PM +0100, Russell King - ARM Linux admin wrote:
> 
> PTP_1588_CLOCK could then be used as a global enable for PTP support
> where appropriate.  We can also avoid all the Kconfig complexity
> introduced by having two independent subsystems either of which could
> be modular.

I'm sorry about the Kconfig confusion.  I think it all started with
the tiny-fication work and the new "select" Kconfig keyword.  The
purpose of all this was, IIRC, to allow leaving dynamic posix clocks
out of the kernel, but it still makes my head spin.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Kurt Kanzenbach]

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

Hi Richard,

On Thu Jul 16 2020, Richard Cochran wrote:
> On Tue, Jul 14, 2020 at 05:26:28PM +0100, Russell King wrote:
>
>> With this driver, there appears to be three instances of a ptp_header()
>> like function in the kernel - I think that ought to become a helper in
>> generic code.
>
> Yes, and the hellcreek DSA driver will add yet another.  I'll be happy
> if patches that refactor everything this magically appear!

I'll post the next version of the hellcreek DSA driver probably next
week. I can include a generic ptp_header() function if you like in that
patch series. But, where to put it? ptp core or maybe ptp_classify?

Thanks,
Kurt

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

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Fri, Jul 17, 2020 at 09:54:07AM +0200, Kurt Kanzenbach wrote:
> I'll post the next version of the hellcreek DSA driver probably next
> week. I can include a generic ptp_header() function if you like in that
> patch series. But, where to put it? ptp core or maybe ptp_classify?

Either place is fine with me.  Maybe it makes most sense in ptp_classify?
Please put the re-factoring in a separate patches, before the new
driver.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Fri, Jul 17, 2020 at 07:24:46PM -0700, Richard Cochran wrote:
> On Fri, Jul 17, 2020 at 09:54:07AM +0200, Kurt Kanzenbach wrote:
> > I'll post the next version of the hellcreek DSA driver probably next
> > week. I can include a generic ptp_header() function if you like in that
> > patch series. But, where to put it? ptp core or maybe ptp_classify?
> 
> Either place is fine with me.  Maybe it makes most sense in ptp_classify?
> Please put the re-factoring in a separate patches, before the new
> driver.

And maybe the new header parsing routine should provide a pointer to a
structure with the message layout.  Something similar to this (from
the linuxptp user stack) might work.

struct ptp_header {
	uint8_t             tsmt; /* transportSpecific | messageType */
	uint8_t             ver;  /* reserved          | versionPTP  */
	UInteger16          messageLength;
	UInteger8           domainNumber;
	Octet               reserved1;
	Octet               flagField[2];
	Integer64           correction;
	UInteger32          reserved2;
	struct PortIdentity sourcePortIdentity;
	UInteger16          sequenceId;
	UInteger8           control;
	Integer8            logMessageInterval;
} PACKED;


Of course, the structure should use the kernel types that include the
big endian annotations.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Kurt Kanzenbach]

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

On Mon Jul 20 2020, Richard Cochran wrote:
> On Fri, Jul 17, 2020 at 07:24:46PM -0700, Richard Cochran wrote:
>> On Fri, Jul 17, 2020 at 09:54:07AM +0200, Kurt Kanzenbach wrote:
>> > I'll post the next version of the hellcreek DSA driver probably next
>> > week. I can include a generic ptp_header() function if you like in that
>> > patch series. But, where to put it? ptp core or maybe ptp_classify?
>> 
>> Either place is fine with me.  Maybe it makes most sense in ptp_classify?
>> Please put the re-factoring in a separate patches, before the new
>> driver.
>
> And maybe the new header parsing routine should provide a pointer to a
> structure with the message layout.  Something similar to this (from
> the linuxptp user stack) might work.
>
> struct ptp_header {
> 	uint8_t             tsmt; /* transportSpecific | messageType */
> 	uint8_t             ver;  /* reserved          | versionPTP  */
> 	UInteger16          messageLength;
> 	UInteger8           domainNumber;
> 	Octet               reserved1;
> 	Octet               flagField[2];
> 	Integer64           correction;
> 	UInteger32          reserved2;
> 	struct PortIdentity sourcePortIdentity;
> 	UInteger16          sequenceId;
> 	UInteger8           control;
> 	Integer8            logMessageInterval;
> } PACKED;

Yes, makes sense. It should work.

>
>
> Of course, the structure should use the kernel types that include the
> big endian annotations.

Sure.

Thanks,
Kurt

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

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Tue, Jul 14, 2020 at 05:26:28PM +0100, Russell King wrote:
> The driver takes inspiration from the Marvell 88E6xxx DSA and DP83640
> drivers.  The hardware is very similar to the implementation found in
> the 88E6xxx DSA driver, but the access methods are very different,
> although it may be possible to create a library that both can use
> along with accessor functions.

I think this definitely needs to become a library - I've just been
looking at mvneta in Armada 388.  One of the three interfaces has
PTP support, and it looks like the register set is again very
similar to the 88E151x and 88E6xxx PTP/TAI register layouts, but
has yet another different access method.

We certainly don't want three copies of almost identical code...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Tue, Jul 14, 2020 at 05:26:28PM +0100, Russell King wrote:
> Add PTP basic support for Marvell 88E151x PHYs.  These PHYs support
> timestamping the egress and ingress of packets, but does not support
> any packet modification, nor do we support any filtering beyond
> selecting packets that the hardware recognises as PTP/802.1AS.

A question has come up concerning the selection of PTP timestamping
sources within a network device.

I have the situation on a couple of devices where there are multiple
places that can do PTP timestamping:

- the PHY (slow to access, only event capture which may not be wired,
   doesn't seem to synchronise well - delay of 58000, frequency changes
   every second between +/-1500ppb.)
- the Ethernet MAC (fast to access, supports event capture and trigger
   generation which also may not be wired, synchronises well, delay of
   700, frequency changes every second +/- 40ppb.)

How do we deal with this situation - from what I can see from the
ethtool API, we have to make a choice about which to use.  How do we
make that choice?

It's not a case of "just implement one" since hardware may have both
available on a particular ethernet interface or just one available.

Do we need a property to indicate whether we wish to use the PHY
or MAC PTP stamping, or something more elaborate?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> How do we deal with this situation - from what I can see from the
> ethtool API, we have to make a choice about which to use.  How do we
> make that choice?

Unfortunately the stack does not implement simultaneous MAC + PHY time
stamping.  If your board has both, then you make the choice to use the
PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.

(Also some MAC drivers do not defer to the PHY properly.  Sometimes
you can work around that by de-selecting the MAC's PTP function in the
Kconfig if possible, but otherwise you need to patch the MAC driver.)
 
> Do we need a property to indicate whether we wish to use the PHY
> or MAC PTP stamping, or something more elaborate?

To do this at run time would require quite some work, I expect.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > How do we deal with this situation - from what I can see from the
> > ethtool API, we have to make a choice about which to use.  How do we
> > make that choice?
> 
> Unfortunately the stack does not implement simultaneous MAC + PHY time
> stamping.  If your board has both, then you make the choice to use the
> PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.

Which is more or less what I said in my email.  However, the important
question about how to select between the two, which is really what I'm
after, has not been addressed.

> (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> you can work around that by de-selecting the MAC's PTP function in the
> Kconfig if possible, but otherwise you need to patch the MAC driver.)

... which really doesn't work if you have a board where only some
network interfaces have a PHY with PTP support, but all have PTP
support in the MAC.

If all MACs or the majority of MACs use a common PTP clock, it seems
to me that you would want to use the MACs rather than the PHY,
especially if the PHY doesn't offer as good a quality PTP clock as
is available from the MAC.

Randomly patching the kernel is out of the question, for arm based
systems we want one kernel that works correctly across as many
platforms as possible, and Kconfig choices to set platform specific
details are basically unacceptable, let alone patching the kernel to
make those decisions.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Wed, Jul 29, 2020 at 02:28:32PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> > On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > > How do we deal with this situation - from what I can see from the
> > > ethtool API, we have to make a choice about which to use.  How do we
> > > make that choice?
> > 
> > Unfortunately the stack does not implement simultaneous MAC + PHY time
> > stamping.  If your board has both, then you make the choice to use the
> > PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.
> 
> Which is more or less what I said in my email.  However, the important
> question about how to select between the two, which is really what I'm
> after, has not been addressed.
> 
> > (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> > you can work around that by de-selecting the MAC's PTP function in the
> > Kconfig if possible, but otherwise you need to patch the MAC driver.)
> 
> ... which really doesn't work if you have a board where only some
> network interfaces have a PHY with PTP support, but all have PTP
> support in the MAC.
> 
> If all MACs or the majority of MACs use a common PTP clock, it seems
> to me that you would want to use the MACs rather than the PHY,
> especially if the PHY doesn't offer as good a quality PTP clock as
> is available from the MAC.
> 
> Randomly patching the kernel is out of the question, for arm based
> systems we want one kernel that works correctly across as many
> platforms as possible, and Kconfig choices to set platform specific
> details are basically unacceptable, let alone patching the kernel to
> make those decisions.

The way PHY PTP support is handled is _really_ not nice.

If we have a phylib driver that supports timestamping, and a MAC driver
that also supports timestamping, then what we end up with is very
messed up.

1. The SIOCGHWTSTAMP/SIOCSHWTSTAMP calls are directed through
   ndo_do_ioctl().  The MAC driver gets first call on whether to
   intercept these or not.  See dev_ifsioc().

2. The ethtool ETHTOOL_GET_TS_INFO call, however, is given to the
   PHY in preference to the MAC driver - there is no way for the MAC
   driver to gain preference.  See __ethtool_get_ts_info().

So, if we have this situation (and yes, I do), then the SIOC*HWTSTAMP
calls get implemented by the MAC driver, and takes effect at the MAC
driver, while the ethtool ETHTOOL_GET_TS_INFO call returns results
from the PHY driver.

That means the MAC driver's timestamping will be controllable from
userspace, but userspace sees the abilities of the PHY driver's
timestamping, and potentially directed to the wrong PTP clock
instance.

What I see elsewhere in ethtool is that the MAC has the ability to
override the phylib provided functionality - for example,
__ethtool_get_sset_count(), __ethtool_get_strings(), and
ethtool_get_phy_stats().  Would it be possible to do the same in
 __ethtool_get_ts_info(), so at least a MAC driver can then decide
whether to propagate the ethtool request to phylib or not, just like
it can do with the SIOC*HWTSTAMP ioctls?  Essentially, reversing the
order of:

        if (phy_has_tsinfo(phydev))
                return phy_ts_info(phydev, info);
        if (ops->get_ts_info)
                return ops->get_ts_info(dev, info);

?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Vladimir Oltean]

On Wed, Jul 29, 2020 at 11:07:48PM +0100, Russell King - ARM Linux admin wrote:
> 
> The way PHY PTP support is handled is _really_ not nice.
> 
> If we have a phylib driver that supports timestamping, and a MAC driver
> that also supports timestamping, then what we end up with is very
> messed up.
> 
> 1. The SIOCGHWTSTAMP/SIOCSHWTSTAMP calls are directed through
>    ndo_do_ioctl().  The MAC driver gets first call on whether to
>    intercept these or not.  See dev_ifsioc().
> 
> 2. The ethtool ETHTOOL_GET_TS_INFO call, however, is given to the
>    PHY in preference to the MAC driver - there is no way for the MAC
>    driver to gain preference.  See __ethtool_get_ts_info().
> 
> So, if we have this situation (and yes, I do), then the SIOC*HWTSTAMP
> calls get implemented by the MAC driver, and takes effect at the MAC
> driver, while the ethtool ETHTOOL_GET_TS_INFO call returns results
> from the PHY driver.
> 
> That means the MAC driver's timestamping will be controllable from
> userspace, but userspace sees the abilities of the PHY driver's
> timestamping, and potentially directed to the wrong PTP clock
> instance.
> 
> What I see elsewhere in ethtool is that the MAC has the ability to
> override the phylib provided functionality - for example,
> __ethtool_get_sset_count(), __ethtool_get_strings(), and
> ethtool_get_phy_stats().  Would it be possible to do the same in
>  __ethtool_get_ts_info(), so at least a MAC driver can then decide
> whether to propagate the ethtool request to phylib or not, just like
> it can do with the SIOC*HWTSTAMP ioctls?  Essentially, reversing the
> order of:
> 
>         if (phy_has_tsinfo(phydev))
>                 return phy_ts_info(phydev, info);
>         if (ops->get_ts_info)
>                 return ops->get_ts_info(dev, info);
> 
> ?
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

When we were discussing this in the other thread:
https://www.spinics.net/lists/netdev/msg669699.html
what I proposed was to introduce a NETIF_F_PHY_HWTSTAMP net_device flag
that could get set when the driver is capable of doing the right thing
with PHY timestamping. Then ethtool could look at that flag.
Additionally, if we could make this flag configurable as a netdev
feature with ethtool, then you could turn it off and even if you have an
attached PTP PHY, you'll use the PHC of the MAC.
You're free to leave your thoughts in that thread.

Thanks,
-Vladimir

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King - ARM Linux admin]

On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > How do we deal with this situation - from what I can see from the
> > ethtool API, we have to make a choice about which to use.  How do we
> > make that choice?
> 
> Unfortunately the stack does not implement simultaneous MAC + PHY time
> stamping.  If your board has both, then you make the choice to use the
> PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.
> 
> (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> you can work around that by de-selecting the MAC's PTP function in the
> Kconfig if possible, but otherwise you need to patch the MAC driver.)
>  
> > Do we need a property to indicate whether we wish to use the PHY
> > or MAC PTP stamping, or something more elaborate?
> 
> To do this at run time would require quite some work, I expect.

Okay, I'm falling into horrible multicast issues with DSA switches
while trying to test.

Some of my platforms have IP_MULTICAST=y, others have IP_MULTICAST=n.
This causes some to send IGMP messages when binding to the multicast
address, others do not.

Those that do cause the DSA switch to add a static database entry
causing all traffic for that multicast address to be only directed to
the port(s) that the machine(s) with IP_MULTICAST=y kernels are
connected to, depriving all IP_MULTICAST=n machines from seeing those
packets.

Maybe, with modern networking technology, it's about time that the
kernel configuration help recommended that kernels should be built
with IP_MULTICAST=y ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King - ARM Linux admin]

On Thu, Jul 30, 2020 at 12:06:13PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> > On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > > How do we deal with this situation - from what I can see from the
> > > ethtool API, we have to make a choice about which to use.  How do we
> > > make that choice?
> > 
> > Unfortunately the stack does not implement simultaneous MAC + PHY time
> > stamping.  If your board has both, then you make the choice to use the
> > PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.
> > 
> > (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> > you can work around that by de-selecting the MAC's PTP function in the
> > Kconfig if possible, but otherwise you need to patch the MAC driver.)
> >  
> > > Do we need a property to indicate whether we wish to use the PHY
> > > or MAC PTP stamping, or something more elaborate?
> > 
> > To do this at run time would require quite some work, I expect.
> 
> Okay, I'm falling into horrible multicast issues with DSA switches
> while trying to test.
> 
> Some of my platforms have IP_MULTICAST=y, others have IP_MULTICAST=n.
> This causes some to send IGMP messages when binding to the multicast
> address, others do not.
> 
> Those that do cause the DSA switch to add a static database entry
> causing all traffic for that multicast address to be only directed to
> the port(s) that the machine(s) with IP_MULTICAST=y kernels are
> connected to, depriving all IP_MULTICAST=n machines from seeing those
> packets.
> 
> Maybe, with modern networking technology, it's about time that the
> kernel configuration help recommended that kernels should be built
> with IP_MULTICAST=y ?

Hmm, and even with IP_MULTICAST=y, I still can't get the "timestamping"
program from the kernel sources to work.

On two different machines, I'm running:

# ./timestamping32 eno0 SOF_TIMESTAMPING_RX_HARDWARE SOF_TIMESTAMPING_RAW_HARDWARE

On one machine (arm32) this works - it can see the traffic it generates
and receives from the other machine.

On the other machine (arm64), it sees _no_ traffic at all, but tcpdump
on that machine can see the traffic being received:

12:36:40.002065 00:51:82:11:33:03 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
length 166: (tos 0x0, ttl 1, id 15045, offset 0, flags [DF], proto UDP (17), length 152)
    192.168.3.1.319 > 224.0.1.130.319: [bad udp cksum 0xa5c1 -> 0x7aaa!] UDP, length 124
12:36:41.105391 00:50:43:02:03:02 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
length 166: (tos 0x0, ttl 1, id 9715, offset 0, flags [DF], proto UDP (17), length 152)
    192.168.3.2.319 > 224.0.1.130.319: [udp sum ok] UDP, length 124

The bad udp cksum is due to checksum offloadong on transmit - 3.1 is the
arm64 host running that tcpdump.

When I look at /proc/net/snmp, I can see the IP InReceives incrementing
but not the IP InDelivers nor UDP InDatagrams counter:

Ip: 2 64 1602 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
Udp: 572 0 0 480 0 0 0 4
Ip: 2 64 1603 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
Udp: 572 0 0 480 0 0 0 4

I don't have any firewall rules on the machine.

I've checked rp_filter on the appropriate interfaces... it's disabled
on the ARM64 machine.  The only other difference in interface config
is accept_source_route, which is enabled on the ARM64 machine.

So, something in the IPv4 layer on ARM64 is silently discarding
multicast UDP PTP packets, and I've no idea what... and I'm coming to
the conclusion that this is all way too much effort and way too
unreliable to be worth spending any more time trying to make work.

I'll send out what I have in the hope that maybe someone will find it
useful and maybe able to complete the work.  However, with these
problems, it is totally unusable for me, and hence I can't test it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King - ARM Linux admin]

On Thu, Jul 30, 2020 at 12:54:19PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Jul 30, 2020 at 12:06:13PM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> > > On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > > > How do we deal with this situation - from what I can see from the
> > > > ethtool API, we have to make a choice about which to use.  How do we
> > > > make that choice?
> > > 
> > > Unfortunately the stack does not implement simultaneous MAC + PHY time
> > > stamping.  If your board has both, then you make the choice to use the
> > > PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.
> > > 
> > > (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> > > you can work around that by de-selecting the MAC's PTP function in the
> > > Kconfig if possible, but otherwise you need to patch the MAC driver.)
> > >  
> > > > Do we need a property to indicate whether we wish to use the PHY
> > > > or MAC PTP stamping, or something more elaborate?
> > > 
> > > To do this at run time would require quite some work, I expect.
> > 
> > Okay, I'm falling into horrible multicast issues with DSA switches
> > while trying to test.
> > 
> > Some of my platforms have IP_MULTICAST=y, others have IP_MULTICAST=n.
> > This causes some to send IGMP messages when binding to the multicast
> > address, others do not.
> > 
> > Those that do cause the DSA switch to add a static database entry
> > causing all traffic for that multicast address to be only directed to
> > the port(s) that the machine(s) with IP_MULTICAST=y kernels are
> > connected to, depriving all IP_MULTICAST=n machines from seeing those
> > packets.
> > 
> > Maybe, with modern networking technology, it's about time that the
> > kernel configuration help recommended that kernels should be built
> > with IP_MULTICAST=y ?
> 
> Hmm, and even with IP_MULTICAST=y, I still can't get the "timestamping"
> program from the kernel sources to work.
> 
> On two different machines, I'm running:
> 
> # ./timestamping32 eno0 SOF_TIMESTAMPING_RX_HARDWARE SOF_TIMESTAMPING_RAW_HARDWARE
> 
> On one machine (arm32) this works - it can see the traffic it generates
> and receives from the other machine.
> 
> On the other machine (arm64), it sees _no_ traffic at all, but tcpdump
> on that machine can see the traffic being received:
> 
> 12:36:40.002065 00:51:82:11:33:03 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
> length 166: (tos 0x0, ttl 1, id 15045, offset 0, flags [DF], proto UDP (17), length 152)
>     192.168.3.1.319 > 224.0.1.130.319: [bad udp cksum 0xa5c1 -> 0x7aaa!] UDP, length 124
> 12:36:41.105391 00:50:43:02:03:02 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
> length 166: (tos 0x0, ttl 1, id 9715, offset 0, flags [DF], proto UDP (17), length 152)
>     192.168.3.2.319 > 224.0.1.130.319: [udp sum ok] UDP, length 124
> 
> The bad udp cksum is due to checksum offloadong on transmit - 3.1 is the
> arm64 host running that tcpdump.
> 
> When I look at /proc/net/snmp, I can see the IP InReceives incrementing
> but not the IP InDelivers nor UDP InDatagrams counter:
> 
> Ip: 2 64 1602 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
> Udp: 572 0 0 480 0 0 0 4
> Ip: 2 64 1603 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
> Udp: 572 0 0 480 0 0 0 4
> 
> I don't have any firewall rules on the machine.
> 
> I've checked rp_filter on the appropriate interfaces... it's disabled
> on the ARM64 machine.  The only other difference in interface config
> is accept_source_route, which is enabled on the ARM64 machine.
> 
> So, something in the IPv4 layer on ARM64 is silently discarding
> multicast UDP PTP packets, and I've no idea what... and I'm coming to
> the conclusion that this is all way too much effort and way too
> unreliable to be worth spending any more time trying to make work.
> 
> I'll send out what I have in the hope that maybe someone will find it
> useful and maybe able to complete the work.  However, with these
> problems, it is totally unusable for me, and hence I can't test it.

To prove the point...

Two arm32 machines connected back to back through eno2, both running
the basic test:
# ./timestamping eno2

see each other.

Try that same test between arm32 and arm64 connected in a similar
manner, and the arm32 machine can see the UDP packets from the arm64
machine, but the arm64 machine discards the UDP packets from the arm32
machine in the IP layer running the exact same commands.

Both kernels and timestamping programs built from the same sources.

So, to summarise where I am:

- TAI (PHC) support:
  * Works on Marvell 88E1512 PHY, with one event capture.
  * Works on mvneta on Armada 388, no event capture implemented.
  * Works on mvpp2.2 on Armada 8040.  Event capture is possible, but
    interferes with the operation of the driver; no way to mask the
    external event trigger interference. Generating a "trigger" which
    can be set to a defined time and pulse with uses the same pin, and
    is also always seen as an external event, interfering with the
    operation of the hardware. There are two "clock" generation
    capabilities which one of which is described as "PPS", but these
    can't be aligned to a second boundary.

- PTP transmit timestamping:
  * Works on Marvell 88E1512 PHY.
  * Unimplemented on mvneta on Armada 388 - I am unable to get the PTP
    port registers to respond, and the documentation gives no hints;
    the PTP global registers and TAI registers respond however. No
    errata hint at this being a problem.
  * Unimplemented on mvpp2.2 on Armada 8040 - I haven't got far enough
    to implement this.

- PTP receive timestamping:
  * I think this works on Marvell 88E1512 PHY, but I'm now not sure.
  * Implemented on mvpp2.2 on Armada 8040 - but unable to test (see
    above issues with the IP layer discarding what seem to be totally
    correct packets on ARM64.)
  * Unimplemented on mvneta on Armada 388 - see above (PTP tx).

- DSA switches can effectively block multicast packets if only some
  kernels on the network have IP_MULTICAST=y and others have
  IP_MULTICAST=n, which adds a level of complexity and unreliability.
  (The answer is likely to always have IP_MULTICAST=y, and maybe the
  kernel should default to that - or in this day and age, the option
  ought to be removed.)

Hence why I'm at the point of giving up; I don't see that PTP will be
of very limited benefit on my network with all these issues, and in
any case, NTP has been "good enough" for the last 20+ years.  Given
that only a limited number of machines will be able to implement PTP
support anyway, NTP will have to run along side it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Thu, Jul 30, 2020 at 12:54:19PM +0100, Russell King - ARM Linux admin wrote:
> So, something in the IPv4 layer on ARM64 is silently discarding
> multicast UDP PTP packets, and I've no idea what... and I'm coming to
> the conclusion that this is all way too much effort and way too
> unreliable to be worth spending any more time trying to make work.

I've also seen this on the Marvell switch.  I'm not sure if the root
cause is in the Linux stack or in the mcast forwarding logic in the
switch.  Using the Marvell, I can make a layer2 TC or a UDPv4 BC, but
not a UDPv4 TC.

I haven't had time to dig deeper, and I think many people are happy
with a layer2 TC as that is called out in the TSN world.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Wed, Jul 29, 2020 at 11:07:48PM +0100, Russell King - ARM Linux admin wrote:
> What I see elsewhere in ethtool is that the MAC has the ability to
> override the phylib provided functionality - for example,
> __ethtool_get_sset_count(), __ethtool_get_strings(), and
> ethtool_get_phy_stats().  Would it be possible to do the same in
>  __ethtool_get_ts_info(), so at least a MAC driver can then decide
> whether to propagate the ethtool request to phylib or not, just like
> it can do with the SIOC*HWTSTAMP ioctls?  Essentially, reversing the
> order of:
> 
>         if (phy_has_tsinfo(phydev))
>                 return phy_ts_info(phydev, info);
>         if (ops->get_ts_info)
>                 return ops->get_ts_info(dev, info);
> 
> ?

I don't see a simple solution.  I think no matter what, the MAC
drivers need work to allow PHY time stamping, and the great majority
of users and driver authors are happy with MAC time stamping.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Thu, Jul 30, 2020 at 08:53:26AM -0700, Richard Cochran wrote:
> On Wed, Jul 29, 2020 at 11:07:48PM +0100, Russell King - ARM Linux admin wrote:
> > What I see elsewhere in ethtool is that the MAC has the ability to
> > override the phylib provided functionality - for example,
> > __ethtool_get_sset_count(), __ethtool_get_strings(), and
> > ethtool_get_phy_stats().  Would it be possible to do the same in
> >  __ethtool_get_ts_info(), so at least a MAC driver can then decide
> > whether to propagate the ethtool request to phylib or not, just like
> > it can do with the SIOC*HWTSTAMP ioctls?  Essentially, reversing the
> > order of:
> > 
> >         if (phy_has_tsinfo(phydev))
> >                 return phy_ts_info(phydev, info);
> >         if (ops->get_ts_info)
> >                 return ops->get_ts_info(dev, info);
> > 
> > ?
> 
> I don't see a simple solution.  I think no matter what, the MAC
> drivers need work to allow PHY time stamping, and the great majority
> of users and driver authors are happy with MAC time stamping.

What I ended up doing was:

        if (ops->get_ts_info) {
                ret = ops->get_ts_info(dev, info);
                if (ret != -EOPNOTSUPP)
                        return ret;
        }
        if (phy_has_tsinfo(phydev))
                return phy_ts_info(phydev, info);
...

which gives the MAC first refusal.  If the MAC wishes to defer to
phylib or the default, it can just return -EOPNOTSUPP.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Thu, Jul 30, 2020 at 07:38:00PM +0100, Russell King - ARM Linux admin wrote:
> What I ended up doing was:
> 
>         if (ops->get_ts_info) {
>                 ret = ops->get_ts_info(dev, info);
>                 if (ret != -EOPNOTSUPP)
>                         return ret;
>         }
>         if (phy_has_tsinfo(phydev))
>                 return phy_ts_info(phydev, info);
> ...
> 
> which gives the MAC first refusal.  If the MAC wishes to defer to
> phylib or the default, it can just return -EOPNOTSUPP.

I guess that makes sense.  If someone designs a board that happens to
have a PHY with unwanted time stamping fcunctionality, then at least
the MAC time stamping function will work.  If the designers really
want PHY time stamping, then they are likely to have to patch the MAC
driver in any case.

So I'm not against such a change.  It would be important to keep the
current "PHY-friendly" MAC drivers still friendly, and so they would
need patching as part of the change.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King - ARM Linux admin]

On Thu, Jul 30, 2020 at 12:32:45PM -0700, Richard Cochran wrote:
> On Thu, Jul 30, 2020 at 07:38:00PM +0100, Russell King - ARM Linux admin wrote:
> > What I ended up doing was:
> > 
> >         if (ops->get_ts_info) {
> >                 ret = ops->get_ts_info(dev, info);
> >                 if (ret != -EOPNOTSUPP)
> >                         return ret;
> >         }
> >         if (phy_has_tsinfo(phydev))
> >                 return phy_ts_info(phydev, info);
> > ...
> > 
> > which gives the MAC first refusal.  If the MAC wishes to defer to
> > phylib or the default, it can just return -EOPNOTSUPP.
> 
> I guess that makes sense.  If someone designs a board that happens to
> have a PHY with unwanted time stamping fcunctionality, then at least
> the MAC time stamping function will work.  If the designers really
> want PHY time stamping, then they are likely to have to patch the MAC
> driver in any case.
> 
> So I'm not against such a change.  It would be important to keep the
> current "PHY-friendly" MAC drivers still friendly, and so they would
> need patching as part of the change.

That would only be necessary if they also provide the get_ts_info
method.

So, I guess I need to find all drivers that refer to phylink or phylib
functions, that also implement get_ts_info method and review them.
I would expect that to be very small, since there's currently little
point implementing PTP at both the PHY and the MAC for the reason I've
raised earlier in this thread.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

> Hmm, and even with IP_MULTICAST=y, I still can't get the "timestamping"
> program from the kernel sources to work.
> 
> On two different machines, I'm running:
> 
> # ./timestamping32 eno0 SOF_TIMESTAMPING_RX_HARDWARE SOF_TIMESTAMPING_RAW_HARDWARE
> 
> On one machine (arm32) this works - it can see the traffic it generates
> and receives from the other machine.
> 
> On the other machine (arm64), it sees _no_ traffic at all, but tcpdump
> on that machine can see the traffic being received:
> 
> 12:36:40.002065 00:51:82:11:33:03 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
> length 166: (tos 0x0, ttl 1, id 15045, offset 0, flags [DF], proto UDP (17), length 152)
>     192.168.3.1.319 > 224.0.1.130.319: [bad udp cksum 0xa5c1 -> 0x7aaa!] UDP, length 124
> 12:36:41.105391 00:50:43:02:03:02 > 01:00:5e:00:01:82, ethertype IPv4 (0x0800),
> length 166: (tos 0x0, ttl 1, id 9715, offset 0, flags [DF], proto UDP (17), length 152)
>     192.168.3.2.319 > 224.0.1.130.319: [udp sum ok] UDP, length 124
> 
> The bad udp cksum is due to checksum offloadong on transmit - 3.1 is the
> arm64 host running that tcpdump.
> 
> When I look at /proc/net/snmp, I can see the IP InReceives incrementing
> but not the IP InDelivers nor UDP InDatagrams counter:
> 
> Ip: 2 64 1602 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
> Udp: 572 0 0 480 0 0 0 4
> Ip: 2 64 1603 0 3 0 0 0 297 531 0 46 0 0 0 0 0 0 0
> Udp: 572 0 0 480 0 0 0 4
> 
> I don't have any firewall rules on the machine.

Hi Russell

A shot in the dark....

I remember somewhere in the stack BPF is used to identify PTP
packets. Is it maybe getting JITed wrongly on arm64? Maybe disable the
BPF JIT and see if it starts working?

    Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

Hello RMK,

> Hence why I'm at the point of giving up; I don't see that PTP will be
> of very limited benefit on my network with all these issues, and in
> any case, NTP has been "good enough" for the last 20+ years.  Given
> that only a limited number of machines will be able to implement PTP
> support anyway, NTP will have to run along side it.

I see this patch has been abandoned.
I am testing it with a ZynqMP board (macb ethernet) and it seems to more or
less work. It got tx timestamp timeout at initialization but after some times
(~20 seconds) ptp4l manages to set it working. Also the IEEE 802.3
network PTP mode is not working, it constantly throw rx timestamp overrun
errors.
I will aim at fixing these issues and adding support to interrupts. It would be
good to have it accepted mainline. What do you think is missing for that? I see
you faced issues with few Armada SOM and IP_MULTICAST, is it still the case?

I am new to the PTP and Ethernet APIs I will try to not speak nonsense but
please correct me if I do.


Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Mon, Feb 27, 2023 at 03:40:37PM +0100, Köry Maincent wrote:
> Hello RMK,
> 
> > Hence why I'm at the point of giving up; I don't see that PTP will be
> > of very limited benefit on my network with all these issues, and in
> > any case, NTP has been "good enough" for the last 20+ years.  Given
> > that only a limited number of machines will be able to implement PTP
> > support anyway, NTP will have to run along side it.
> 
> I see this patch has been abandoned.
> I am testing it with a ZynqMP board (macb ethernet) and it seems to more or
> less work. It got tx timestamp timeout at initialization but after some times
> (~20 seconds) ptp4l manages to set it working. Also the IEEE 802.3
> network PTP mode is not working, it constantly throw rx timestamp overrun
> errors.
> I will aim at fixing these issues and adding support to interrupts. It would be
> good to have it accepted mainline. What do you think is missing for that?

It isn't formally abandoned, but is permanently on-hold as merging
Marvell PHY PTP support into mainline _will_ regress the superior PTP
support on the Macchiatobin platform for the reasons outlined in:

https://lore.kernel.org/netdev/20200729220748.GW1605@shell.armlinux.org.uk/

Attempting to fix this problem was basically rejected by the PTP
maintainer, and thus we're at a deadlock over the issue, and Marvell
PHY PTP support can never be merged into mainline.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Mon, 27 Feb 2023 15:20:05 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> > I see this patch has been abandoned.
> > I am testing it with a ZynqMP board (macb ethernet) and it seems to more or
> > less work. It got tx timestamp timeout at initialization but after some
> > times (~20 seconds) ptp4l manages to set it working. Also the IEEE 802.3
> > network PTP mode is not working, it constantly throw rx timestamp overrun
> > errors.
> > I will aim at fixing these issues and adding support to interrupts. It
> > would be good to have it accepted mainline. What do you think is missing
> > for that?  
> 
> It isn't formally abandoned, but is permanently on-hold as merging
> Marvell PHY PTP support into mainline _will_ regress the superior PTP
> support on the Macchiatobin platform for the reasons outlined in:
> 
> https://lore.kernel.org/netdev/20200729220748.GW1605@shell.armlinux.org.uk/
> 
> Attempting to fix this problem was basically rejected by the PTP
> maintainer, and thus we're at a deadlock over the issue, and Marvell
> PHY PTP support can never be merged into mainline.

As I understand, if the PHY support PTP, it is prioritize to the PTP of the MAC.
As quote in the mail thread it seems there was discussion in netdev about
moving phy_has_hwtstamp to core and allowing ethtool to choose which one to
use. I don't know if the decision have been made about it since, but it seems
nothing has been sent to mainline. Meanwhile, why do we not move forward on
this patch with the current PTP behavior and updates it when new core PTP change
will be sent mailine?

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Mon, Feb 27, 2023 at 06:30:13PM +0100, Köry Maincent wrote:
> On Mon, 27 Feb 2023 15:20:05 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > > I see this patch has been abandoned.
> > > I am testing it with a ZynqMP board (macb ethernet) and it seems to more or
> > > less work. It got tx timestamp timeout at initialization but after some
> > > times (~20 seconds) ptp4l manages to set it working. Also the IEEE 802.3
> > > network PTP mode is not working, it constantly throw rx timestamp overrun
> > > errors.
> > > I will aim at fixing these issues and adding support to interrupts. It
> > > would be good to have it accepted mainline. What do you think is missing
> > > for that?  
> > 
> > It isn't formally abandoned, but is permanently on-hold as merging
> > Marvell PHY PTP support into mainline _will_ regress the superior PTP
> > support on the Macchiatobin platform for the reasons outlined in:
> > 
> > https://lore.kernel.org/netdev/20200729220748.GW1605@shell.armlinux.org.uk/
> > 
> > Attempting to fix this problem was basically rejected by the PTP
> > maintainer, and thus we're at a deadlock over the issue, and Marvell
> > PHY PTP support can never be merged into mainline.
> 
> As I understand, if the PHY support PTP, it is prioritize to the PTP of the MAC.
> As quote in the mail thread it seems there was discussion in netdev about
> moving phy_has_hwtstamp to core and allowing ethtool to choose which one to
> use. I don't know if the decision have been made about it since, but it seems
> nothing has been sent to mainline. Meanwhile, why do we not move forward on
> this patch with the current PTP behavior and updates it when new core PTP change
> will be sent mailine?

Clearly, we have an issue with communication. Let me repeat:

Merging support for Marvell PHY PTP *will* *regress* the Macchiatobin
platform, making PTP *unusable* because *some* API calls will go to
the *PHY* PTP instance while *other* API calls go to the *MAC* PTP
instance.

Since merging Marvell PHY PTP support will cause a regression, the only
way to fix that regression at the moment is by reverting the merge of
Marvell PHY PTP support as there is no acceptable solution to the above
problem - I've attempted to fix it and the patch was rejected.

Sorry, but no, mainline does not get Marvell PHY PTP support before
the generic code is fixed for this blocking issue.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Mon, Feb 27, 2023 at 03:20:05PM +0000, Russell King (Oracle) wrote:

> Attempting to fix this problem was basically rejected by the PTP
> maintainer, and thus we're at a deadlock over the issue, and Marvell
> PHY PTP support can never be merged into mainline.

FWIW, here was my attempt to solve the issue by making the PHY/MAC
layer selectable at run time, while preserving PHY as the default.

https://lore.kernel.org/netdev/20220103232555.19791-1-richardcochran@gmail.com/

There was a fair amount of discussion, and it seemed to me that
everyone wanted a pony.

If anyone wants to take up that series, or present a more creative
solution, then by all means, please go ahead.  I still have a working
board with the PHYTER, and so I would be happy to help validate the
new feature.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Mon, Feb 27, 2023 at 11:45:58AM -0800, Richard Cochran wrote:
> On Mon, Feb 27, 2023 at 03:20:05PM +0000, Russell King (Oracle) wrote:
> 
> > Attempting to fix this problem was basically rejected by the PTP
> > maintainer, and thus we're at a deadlock over the issue, and Marvell
> > PHY PTP support can never be merged into mainline.
> 
> FWIW, here was my attempt to solve the issue by making the PHY/MAC
> layer selectable at run time, while preserving PHY as the default.
> 
> https://lore.kernel.org/netdev/20220103232555.19791-1-richardcochran@gmail.com/

Hi Richard,

Looking at that link, I'm only seeing that message, with none of
the patches nor the discussion. Digging back in my mailbox, I
find that the patches weren't threaded to the cover message, which
makes it quite difficult to go back and review the discussion.

Patch 1 (no comments)
https://lore.kernel.org/netdev/20220103232555.19791-2-richardcochran@gmail.com
Patch 2 (one comment from me suggesting moving a variable)
https://lore.kernel.org/netdev/20220103232555.19791-3-richardcochran@gmail.com
Patch 3 (lots of comments)
https://lore.kernel.org/netdev/20220103232555.19791-4-richardcochran@gmail.com
Patch 4 (no comments)
https://lore.kernel.org/netdev/20220103232555.19791-5-richardcochran@gmail.com

Looking back briefly at the discussion on patch 3, was the reason
this approach died due to the request to have something more flexible,
supporting multiple hardware timestamps per packet?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Mon, Feb 27, 2023 at 08:09:05PM +0000, Russell King (Oracle) wrote:

> Looking at that link, I'm only seeing that message, with none of
> the patches nor the discussion. Digging back in my mailbox, I
> find that the patches weren't threaded to the cover message, which
> makes it quite difficult to go back and review the discussion.

Sorry about that.  By accident I omitted --thread=shallow that time.

> Looking back briefly at the discussion on patch 3, was the reason
> this approach died due to the request to have something more flexible,
> supporting multiple hardware timestamps per packet?

I still think the approach will work, but I guess I got distracted
with other stuff and forgot about it.

The "multiple hardware timestamps per packet" is a nice idea, but it
would require a new user API, and so selectable MAC/PHY on the
existing API is still needed.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Mon, Feb 27, 2023 at 12:19:22PM -0800, Richard Cochran wrote:
> On Mon, Feb 27, 2023 at 08:09:05PM +0000, Russell King (Oracle) wrote:
> 
> > Looking at that link, I'm only seeing that message, with none of
> > the patches nor the discussion. Digging back in my mailbox, I
> > find that the patches weren't threaded to the cover message, which
> > makes it quite difficult to go back and review the discussion.
> 
> Sorry about that.  By accident I omitted --thread=shallow that time.
> 
> > Looking back briefly at the discussion on patch 3, was the reason
> > this approach died due to the request to have something more flexible,
> > supporting multiple hardware timestamps per packet?
> 
> I still think the approach will work, but I guess I got distracted
> with other stuff and forgot about it.
> 
> The "multiple hardware timestamps per packet" is a nice idea, but it
> would require a new user API, and so selectable MAC/PHY on the
> existing API is still needed.

I agree - even when we have support for multiple hardware timestamps,
we still need the existing API to work in a sensible way, and we need
a way to choose which hardware timestamp we want the existing API to
report.

So yes, it's a nice idea to support multiple hardware timestamps, but
I think that's an entirely separate problem to solving the current
issue, which is a blocking issue to adding support for PTP on some
platforms.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Tue, 28 Feb 2023 12:07:09 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> So yes, it's a nice idea to support multiple hardware timestamps, but
> I think that's an entirely separate problem to solving the current
> issue, which is a blocking issue to adding support for PTP on some
> platforms.

Alright, Richard can I continue your work on it and send new revisions of your
patch series or do you prefer to continue on your own?
Also your series rise the question of which timestamping should be the default,
MAC or PHY, without breaking any past or future compatibilities.
There is question of using Kconfig or devicetree but each of them seems to have
drawbacks:
https://lore.kernel.org/netdev/ad4a8d3efbeaacf241a19bfbca5976f9@walle.cc/ 

Do you or Russell have any new thought about it?

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Tue, Feb 28, 2023 at 02:16:30PM +0100, Köry Maincent wrote:
> On Tue, 28 Feb 2023 12:07:09 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > So yes, it's a nice idea to support multiple hardware timestamps, but
> > I think that's an entirely separate problem to solving the current
> > issue, which is a blocking issue to adding support for PTP on some
> > platforms.
> 
> Alright, Richard can I continue your work on it and send new revisions of your
> patch series or do you prefer to continue on your own?
> Also your series rise the question of which timestamping should be the default,
> MAC or PHY, without breaking any past or future compatibilities.
> There is question of using Kconfig or devicetree but each of them seems to have
> drawbacks:
> https://lore.kernel.org/netdev/ad4a8d3efbeaacf241a19bfbca5976f9@walle.cc/ 
> 
> Do you or Russell have any new thought about it?

The only thought I have is that maybe a MAC driver should be able to
override the default, then at least we have a way to deal with this
on a case by case basis. However, that's just pulling an idea out of
the air.

I think what might be useful as a first step is to go through the
various networking devices to work out which support PTP today, and
tabulate the result. There shouldn't be any cases where we have both
the MAC and PHY having PTP support (for the API reasons I've already
stated) but if there are, that needs to be highlighted.

Then we can see what the default should be, and then which MAC
drivers would need to override the default.

It would probably be a good idea to document that in the kernel's
Documentation subdirectory so when e.g. a PHY driver gains PTP
support, we have some idea which MAC drivers may be impacted.

Does that sound sensible?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Tue, 28 Feb 2023 13:36:36 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> > Also your series rise the question of which timestamping should be the
> > default, MAC or PHY, without breaking any past or future compatibilities.
> > There is question of using Kconfig or devicetree but each of them seems to
> > have drawbacks:
> > https://lore.kernel.org/netdev/ad4a8d3efbeaacf241a19bfbca5976f9@walle.cc/ 
> > 
> > Do you or Russell have any new thought about it?  
> 
> The only thought I have is that maybe a MAC driver should be able to
> override the default, then at least we have a way to deal with this
> on a case by case basis. However, that's just pulling an idea out of
> the air.
>
> I think what might be useful as a first step is to go through the
> various networking devices to work out which support PTP today, and
> tabulate the result. There shouldn't be any cases where we have both
> the MAC and PHY having PTP support (for the API reasons I've already
> stated) but if there are, that needs to be highlighted.

The only defconfig on that case present on mainline is socfpga_defconfig
(MICREL_PHY and STMMAC_ETH). In fact PHY timestamping is really not spread.
There is only one defconfig which support PTP on both MAC and PHY, therefore
setting the PTP on MAC by default seems the best choice. It won't change the
default behavior when adding new PHY PTP support. Does adding documentation
about socfpga_defconfig is sufficient or should we need to add something
specific to change the default PTP on this case.

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Tue, Feb 28, 2023 at 02:16:30PM +0100, Köry Maincent wrote:
> On Tue, 28 Feb 2023 12:07:09 +0000
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > So yes, it's a nice idea to support multiple hardware timestamps, but
> > I think that's an entirely separate problem to solving the current
> > issue, which is a blocking issue to adding support for PTP on some
> > platforms.
> 
> Alright, Richard can I continue your work on it and send new revisions of your
> patch series or do you prefer to continue on your own?

If you can work this, please do.  I can help review and test.

> Also your series rise the question of which timestamping should be the default,
> MAC or PHY, without breaking any past or future compatibilities.
> There is question of using Kconfig or devicetree but each of them seems to have
> drawbacks:
> https://lore.kernel.org/netdev/ad4a8d3efbeaacf241a19bfbca5976f9@walle.cc/ 
> 
> Do you or Russell have any new thought about it?

The overall default must be PHY, because that is the legacy condition.
The options to change the default are:

1. device tree: Bad because the default is a configuration and does
   not describe the hardware.

2. Kconfig: Override PHY default at compile time.

3. Module Param: Configure default on kernel command line.

4. Letting drivers override PHY at run time.

5. other?

It would be possible to support both 2 and 3, with command line having
the last word.

I don't like #4 because it would cleaner if every time stamping driver
would simply implement the internal APIs, without having special hooks
for various boards.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

> The overall default must be PHY, because that is the legacy condition.
> The options to change the default are:
> 
> 1. device tree: Bad because the default is a configuration and does
>    not describe the hardware.

There are also lots of systems which don't use DT, e.g. ACPI, or are
just plain PCI or USB devices which don't have any configuration at
all.

> 2. Kconfig: Override PHY default at compile time.

Per MAC/PHY combination? That does not scale.

> 3. Module Param: Configure default on kernel command line.

Module params are consider bad in the networking stack. DaveM always
NACKs them.

> 4. Letting drivers override PHY at run time.

This is what Russell suggested.

> 5. other?
> 
> It would be possible to support both 2 and 3, with command line having
> the last word.
> 
> I don't like #4 because it would cleaner if every time stamping driver
> would simply implement the internal APIs, without having special hooks
> for various boards.

I agree that stamping drivers should implement the API. All i think
Russell is suggesting is that MAC drivers have an API call they can
make on the PTP core to tell it to direct all calls to the MAC
implementation of the time stamper, not the PHY implementation of the
time stamper. This might need some changes to the internal APIs, such
that all kernel API calls should go through the PTP core, and the PTP
core then directs them by default to the PHY, but on request sends it
to the MAC stamper.

     Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Tue, Feb 28, 2023 at 07:16:24AM -0800, Richard Cochran wrote:
> On Tue, Feb 28, 2023 at 02:16:30PM +0100, Köry Maincent wrote:
> > On Tue, 28 Feb 2023 12:07:09 +0000
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > So yes, it's a nice idea to support multiple hardware timestamps, but
> > > I think that's an entirely separate problem to solving the current
> > > issue, which is a blocking issue to adding support for PTP on some
> > > platforms.
> > 
> > Alright, Richard can I continue your work on it and send new revisions of your
> > patch series or do you prefer to continue on your own?
> 
> If you can work this, please do.  I can help review and test.
> 
> > Also your series rise the question of which timestamping should be the default,
> > MAC or PHY, without breaking any past or future compatibilities.
> > There is question of using Kconfig or devicetree but each of them seems to have
> > drawbacks:
> > https://lore.kernel.org/netdev/ad4a8d3efbeaacf241a19bfbca5976f9@walle.cc/ 
> > 
> > Do you or Russell have any new thought about it?
> 
> The overall default must be PHY, because that is the legacy condition.
> The options to change the default are:
> 
> 1. device tree: Bad because the default is a configuration and does
>    not describe the hardware.

I'm quite sure the DT maintainers will frown upon that.

> 2. Kconfig: Override PHY default at compile time.

That doesn't work for Arm (32 or 64 bit) kernels as we want a single
kernel image that will work correctly on any platform, so as soon as
we have two platforms that needs the Kconfig set differently, this
becomes a problem.

> 3. Module Param: Configure default on kernel command line.

I understand module parameters are frowned upon by netdev.

> 4. Letting drivers override PHY at run time.

I think this is the only sensible solution - we know for example that
mvpp2 will prefer its PTP implementation as it is (a) higher resolution
and (b) has more flexibility than what can be provided by the Marvell
PHYs that it is often used with.

> 5. other?

Another possible solution to this would be to introduce a rating for
each PTP clock in a similar way that we do for the kernel's
clocksources, and the one with the highest rating becomes the default. 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Michael Walle]

>> 4. Letting drivers override PHY at run time.
>
> I think this is the only sensible solution - we know for example that
> mvpp2 will prefer its PTP implementation as it is (a) higher resolution
> and (b) has more flexibility than what can be provided by the Marvell
> PHYs that it is often used with.

Please also consider that there might be one switch with a shared
PHC and multiple PHYs, each with its own PHC. In this case, it is a
property of the board wether PHY timestamping actually works, because
it will need some kind of synchronization between all the PHYs. Whereas
the MAC timestamping just works. Or we dodge that problem now and these
kind of drivers might not use PHY timestamping. But AFAIK microchip is
working on PHY timestamping for their quad PHYs which are used together
with a LAN9668 switch.

-michael

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Tue, Feb 28, 2023 at 05:44:35PM +0100, Michael Walle wrote:
> >> 4. Letting drivers override PHY at run time.
> >
> > I think this is the only sensible solution - we know for example that
> > mvpp2 will prefer its PTP implementation as it is (a) higher resolution
> > and (b) has more flexibility than what can be provided by the Marvell
> > PHYs that it is often used with.
> 
> Please also consider that there might be one switch with a shared
> PHC and multiple PHYs, each with its own PHC.

Doesn't the PTP API already allow that? The PHC is a separate API from
the network hardware timestamping - and the netdev/PHY is required
to implement the ethtool get_ts_info API that provides userspace with
the index to the PHC associated with the interface.

> In this case, it is a
> property of the board wether PHY timestamping actually works, because
> it will need some kind of synchronization between all the PHYs.

How is this any different from e.g. a platform where there are
multiple network interfaces each with their own independent PHC
such as Macchiatobin, where there are two CP110 dies, each with
their own group of three ethernet adapters, and each die has its
own PHC shared between the three ethernet adapters?

Hardware synchronisation between the two PHCs isn't possible, but
they might tick at the same rate (it's something that hasn't been
checked.) However, the hardware signals aren't that helpful because
there's no way to make e.g. the rising edge always be at the start
of a second. So the synchronisation has to be done in software.

I don't think PHCs need to be synchronised in hardware to "actually
work". Take an example of a PC with two network cards, both having
their own independent PHC.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Michael Walle]

Am 2023-02-28 17:58, schrieb Russell King (Oracle):
> On Tue, Feb 28, 2023 at 05:44:35PM +0100, Michael Walle wrote:
>> >> 4. Letting drivers override PHY at run time.
>> >
>> > I think this is the only sensible solution - we know for example that
>> > mvpp2 will prefer its PTP implementation as it is (a) higher resolution
>> > and (b) has more flexibility than what can be provided by the Marvell
>> > PHYs that it is often used with.
>> 
>> Please also consider that there might be one switch with a shared
>> PHC and multiple PHYs, each with its own PHC.
> 
> Doesn't the PTP API already allow that? The PHC is a separate API from
> the network hardware timestamping - and the netdev/PHY is required
> to implement the ethtool get_ts_info API that provides userspace with
> the index to the PHC associated with the interface.

Yes, but the source for the timestamp is the PHC. If the PHCs are
not synchronized, the timestamps won't be either. With a shared PHC,
the synchronization is already a given.

>> In this case, it is a
>> property of the board wether PHY timestamping actually works, because
>> it will need some kind of synchronization between all the PHYs.
> 
> How is this any different from e.g. a platform where there are
> multiple network interfaces each with their own independent PHC
> such as Macchiatobin, where there are two CP110 dies, each with
> their own group of three ethernet adapters, and each die has its
> own PHC shared between the three ethernet adapters?
> 
> Hardware synchronisation between the two PHCs isn't possible, but
> they might tick at the same rate (it's something that hasn't been
> checked.) However, the hardware signals aren't that helpful because
> there's no way to make e.g. the rising edge always be at the start
> of a second. So the synchronisation has to be done in software.
> 
> I don't think PHCs need to be synchronised in hardware to "actually
> work". Take an example of a PC with two network cards, both having
> their own independent PHC.

That might be true if you just want to use PTP as a time sync protocol,
but keep in mind that there is also the time aware scheduler which uses
the PHC as its time source, too. If you want to use PHY timestamping
in this case, the PHCs needs to be synchronized. Honestly, I'm not 
really
sure, how that is supposed to work.
All I'm trying to say, is there might also be some board constraints so
the MAC driver might not always be telling what is best, PHY or MAC.

-michael

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Tue, Feb 28, 2023 at 09:13:57PM +0100, Michael Walle wrote:

> All I'm trying to say, is there might also be some board constraints so
> the MAC driver might not always be telling what is best, PHY or MAC.

+1

I can't see how the MAC, in general, could possibly determine whether
it should be preferred.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Tue, Feb 28, 2023 at 04:33:25PM +0100, Andrew Lunn wrote:
> > 2. Kconfig: Override PHY default at compile time.
> 
> Per MAC/PHY combination? That does not scale.

No, I meant a single, global default that overrides the implicit
default of using the PHY.


> > 3. Module Param: Configure default on kernel command line.
> 
> Module params are consider bad in the networking stack. DaveM always
> NACKs them.

It could be a regular command line option?

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Tue, Feb 28, 2023 at 04:27:10PM +0000, Russell King (Oracle) wrote:

> Another possible solution to this would be to introduce a rating for
> each PTP clock in a similar way that we do for the kernel's
> clocksources, and the one with the highest rating becomes the default. 

Hm, that is an idea.

It would only need a command line override in case the user disagrees
with the kernel's hard coded rating.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Tue, 28 Feb 2023 16:27:10 +0000 Russell King (Oracle) wrote:
> > 5. other?  
> 
> Another possible solution to this would be to introduce a rating for
> each PTP clock in a similar way that we do for the kernel's
> clocksources, and the one with the highest rating becomes the default. 

Why not ethtool? Sorry if I'm missing something obvious..

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Tue, Feb 28, 2023 at 02:26:48PM -0800, Jakub Kicinski wrote:
> On Tue, 28 Feb 2023 16:27:10 +0000 Russell King (Oracle) wrote:
> > > 5. other?  
> > 
> > Another possible solution to this would be to introduce a rating for
> > each PTP clock in a similar way that we do for the kernel's
> > clocksources, and the one with the highest rating becomes the default. 
> 
> Why not ethtool? Sorry if I'm missing something obvious..

If we make the "default" fixed, such as "we always default to PHY"
then merge Marvell PHY PTP support, then on Macchiatobin, we will
end up switching from the current mvpp2-based PTP support to using
the inferior PHY PTP support, resulting in a loss of precision
and accuracy. That's a regression.

So, while we generally want PHY PTP support to be the default,
there are legitimate reasons for wanting in some circumstances
for the MAC to be the default.

The problem with an ethtool control is it's an extra step that
the user has to do to restore the accuracy that they had under
today's kernels, and I don't think that's something that they
should be involved in doing.

Providing controls to userspace is all very well _if_ there is
a way for users to make sensible decisions - which means giving
them information such as "on this hardware, MAC PTP is preferable
because it has better accuracy and precision, but on some other
hardware, PHY PTP is preferable" and such a document would get
very very long.

IMHO, it's better if the kernel automatically selects a sensible
default _and_ gives the user the ability to override it e.g. via
ethtool.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Tue, 28 Feb 2023 22:40:05 +0000 Russell King (Oracle) wrote:
> IMHO, it's better if the kernel automatically selects a sensible
> default _and_ gives the user the ability to override it e.g. via
> ethtool.

Oh, I see, makes sense.

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Tue, 28 Feb 2023 14:59:11 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 28 Feb 2023 22:40:05 +0000 Russell King (Oracle) wrote:
> > IMHO, it's better if the kernel automatically selects a sensible
> > default _and_ gives the user the ability to override it e.g. via
> > ethtool.  
> 
> Oh, I see, makes sense.

I suppose the idea of Russell to rate each PTP clocks may be the best one, as
all others solutions have drawbacks. Does using the PTP clock period value (in
picoseconds) is enough to decide which PTP is the best one? It is hardware
specific therefore it is legitimate to be set by the MAC and PHY drivers.

Regards,

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Wed, Mar 01, 2023 at 05:04:08PM +0100, Köry Maincent wrote:
> I suppose the idea of Russell to rate each PTP clocks may be the best one, as
> all others solutions have drawbacks. Does using the PTP clock period value (in
> picoseconds) is enough to decide which PTP is the best one? It is hardware
> specific therefore it is legitimate to be set by the MAC and PHY drivers.

It is not that simple.  In fact, I've never seen an objective
comparision of different HW.  The vendors surely have no reason to
conduct such a study.  Also, the data sheets never make any claim
about synchronization quality.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Köry Maincent]

Hello Russell,

> I have the situation on a couple of devices where there are multiple
> places that can do PTP timestamping:
> 
> - the PHY (slow to access, only event capture which may not be wired,
>    doesn't seem to synchronise well - delay of 58000, frequency changes
>    every second between +/-1500ppb.)
> - the Ethernet MAC (fast to access, supports event capture and trigger
>    generation which also may not be wired, synchronises well, delay of
>    700, frequency changes every second +/- 40ppb.)

Does this test have been made with the marvell 88E151x PHY?
On my side with a zynqMP SOM (cadence MACB MAC) the synchronization of the PHY
PTP clock is way better: +/-50ppb. Do you have an idea about the difference? 
Which link partner were you using? stm32mp157 hardware PTP on my side.

On the thread mail we are talking about PTP rate scale but as we can see you
and I have really not the same synchronisation on this PHY PTP. How
could we set a PTP rate scale in the driver if on two different board the
result vary a lot? Although I don't understand why.

I have also differences with the PTP MAC but it is expected as it is not the
same MAC.

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Wed, Mar 01, 2023 at 08:36:37PM -0800, Richard Cochran wrote:
> On Wed, Mar 01, 2023 at 05:04:08PM +0100, Köry Maincent wrote:
> > I suppose the idea of Russell to rate each PTP clocks may be the best one, as
> > all others solutions have drawbacks. Does using the PTP clock period value (in
> > picoseconds) is enough to decide which PTP is the best one? It is hardware
> > specific therefore it is legitimate to be set by the MAC and PHY drivers.
> 
> It is not that simple.  In fact, I've never seen an objective
> comparision of different HW.  The vendors surely have no reason to
> conduct such a study.  Also, the data sheets never make any claim
> about synchronization quality.

mvpp2 (MAC) hardware has a counter in hardware that we can read the
timestamps from, and it supports being fine-tuned and stepped in
hardware. So the hardware gives us the timestamps with no shenanigans.
The hardware timestamp is incremented by a programmable amount
(including a 32-bit fractional part) every 3ns.

The mvpp2 hardware captures the timestamp when receiving packets, and
places the timestamp in the buffer descriptors - there is no filtering
and this timestamping essentially comes for free from the software
perspective. There is no requirement to read any registers before the
next packet that needs to be timestamped, and the timestamps can not
be confused which packet they are intended for. Since these timestamps
are in the buffer descriptors, they are available as soon as the packet
is ready to be passed to the networking layers.

Timestamps can be inserted into transmitted packets and the checksums
updated by the hardware. The hardware has capability to do a number
of operations on the packets when inserting the timestamp, although
I never wrote software support for this (I wanted some way to
positively test these but I don't think I had a way to do that.)
Multiple transmit packets can be queued for timestamping and the
hardware will cope.

Hardware signals are supported. The hardware has event capture,
capabilities, which snapshots the counter, and this should give high
accuracy. However, for generation of events, e.g. PPS, I have observed
that there is no way to ensure that the PPS signal is aligned to the
start of a second. That said, the signal is subject to the fine
adjustments of the hardware counter - so increasing the hardware
counter's increment correctly shortens the PPS signal period.

All accesses to the hardware are fast, being MMIO, which gives a
very stable reading when the hardware clock is adjusted by ptp4l.

The hardware timestamp counter is shared across all three ethernet
interfaces of the CP110 die, and there can be more than one CP110
die in a SoC.


Marvell PHY hardware is qutie different. The counter is updated every
8ns, and merely increments by one each time. There is no fractional
adjustment, meaning that there is no fine adjustment of the hardware.
Fine adjustment is performed by using the kernel's timecounter
infrastructure.

The counter is accessed over MDIO which appears to introduce a lot of
variability in latency, which transfers over to read accesses to the
counter, and thus the read timestamp. This makes it harder for ptp4l to
synchronise the counter, and from my testing, introduces a lot of
noise.

The PHY can't modify the packet in any way, but merely captures the
counter when the packet is transmitted. The PHY needs to be programmed
with the byte offset from the start of the packet to the ethernet
protocol, and the ethernet protocol to the IP header - suggesting
that the PTP hardware needs to know whether packets will or will not
be incorporating a VLAN header (this is explicitly mentioned in e.g.
the 88e151x manual.)

The PHY's filtering applies to both the transmit and receive paths,
and as there is only one set of capture registers for a transmit
packet, this may become a problem, especially if we are not using
interrupts for the PHY. Without interrupts, at best we poll the PHY
every 2ms (but could be longer due to MDIO access times.)

For receive, the PHY implement two capture register sets. The PHY
needs to parse the packet (using the offsets into the packet above)
in order to retrieve the MessageID field which is then used to work
out which receive capture register to write the packet's counter
value to. This means if one receives several PTP packets in quick
succession, there is a chance to lose timestamps.

The situation with hardware signals will be similar to PP2, in that
event capture takes a snapshot of the hardware counter, which can
then be translated. However, as the hardware counter does not get
adjusted, signal generation is tied to the unadjusted rate at which
the hardware ticks at, which means that asking for a PPS, the
generated signal will drift as the PHY 125MHz clock drifts.


In conclusion, not only does PP2 have a 3ns tick vs 8ns for PHY, PP2
can more reliably capture the timestamps, reading the hardware counter
value has less noise, and thus can be synchronised by ptp4l with far
less random variance than the PHY implementation.

Therefore, I believe that the Marvell PHY PTP implementation is all
round inferior to that found in the Marvell PP2 MAC, and hence why I
believe that the PP2 MAC implementation should be used by default over
the PHY.


(In essence, because of all the noise when trying the Marvell PHY with
ptp4l, I came to the conlusion that NTP was a far better solution to
time synchronisation between machines than PTP would ever be due to
the nose induced by MDIO access. However, I should also state that I
basically gave up with PTP in the end because hardware support is
overall poor, and NTP just works - and I'd still have to run NTP for
the machines that have no PTP capabilities. PTP probably only makes
sense if one has a nice expensive grand master PTP clock on ones
network, and all the machines one wants to synchronise have decent
PTP implementations.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Thu, 2 Mar 2023 11:49:26 +0000 Russell King (Oracle) wrote:
> (In essence, because of all the noise when trying the Marvell PHY with
> ptp4l, I came to the conlusion that NTP was a far better solution to
> time synchronisation between machines than PTP would ever be due to
> the nose induced by MDIO access. However, I should also state that I
> basically gave up with PTP in the end because hardware support is
> overall poor, and NTP just works - and I'd still have to run NTP for
> the machines that have no PTP capabilities. PTP probably only makes
> sense if one has a nice expensive grand master PTP clock on ones
> network, and all the machines one wants to synchronise have decent
> PTP implementations.)

Don't wanna waste too much of your time with the questions since
I haven't done much research but - wouldn't MAC timestamp be a better
choice more often (as long as it's a real, to-spec PTP stamp)? 
Are we picking PHY for historical reasons?

Not that flipping the default would address the problem of regressing
some setups..

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Thu, 2 Mar 2023 08:49:32 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 2 Mar 2023 11:49:26 +0000 Russell King (Oracle) wrote:
> > (In essence, because of all the noise when trying the Marvell PHY with
> > ptp4l, I came to the conlusion that NTP was a far better solution to
> > time synchronisation between machines than PTP would ever be due to
> > the nose induced by MDIO access. However, I should also state that I
> > basically gave up with PTP in the end because hardware support is
> > overall poor, and NTP just works - and I'd still have to run NTP for
> > the machines that have no PTP capabilities. PTP probably only makes
> > sense if one has a nice expensive grand master PTP clock on ones
> > network, and all the machines one wants to synchronise have decent
> > PTP implementations.)  
> 
> Don't wanna waste too much of your time with the questions since
> I haven't done much research but - wouldn't MAC timestamp be a better
> choice more often (as long as it's a real, to-spec PTP stamp)? 
> Are we picking PHY for historical reasons?
> 
> Not that flipping the default would address the problem of regressing
> some setups..

I have measured it with the Marvell PHY and MACB MAC but it is the contrary on
my side:
https://lkml.kernel.org/netdev/20230302113752.057a3213@kmaincent-XPS-13-7390/
Also PHY default seems more logical as it is nearer to the physical link, but
still I am interesting by the answer as I am not a PTP expert. Is really PTP
MAC often more precise than PTP PHY?

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Thu, 2 Mar 2023 18:06:16 +0100 Köry Maincent wrote:
> I have measured it with the Marvell PHY and MACB MAC but it is the contrary on
> my side:
> https://lkml.kernel.org/netdev/20230302113752.057a3213@kmaincent-XPS-13-7390/
> Also PHY default seems more logical as it is nearer to the physical link, but
> still I am interesting by the answer as I am not a PTP expert. Is really PTP
> MAC often more precise than PTP PHY?

Do you happen to have a datasheet for MACB? The time stamping
capability is suspiciously saved in a variable called hw_dma_cap
which may indicate it's a DMA time stamp not a true PTP stamp.

Quite a few NICs/MACs support DMA time stamps because it's far 
easier to take the stamp where the descriptor writing logic is
(DMA block) than take it at the end of the MAC and haul it all 
the way thru the pipeline back to the DMA block.

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Thu, Mar 02, 2023 at 08:49:32AM -0800, Jakub Kicinski wrote:
> On Thu, 2 Mar 2023 11:49:26 +0000 Russell King (Oracle) wrote:
> > (In essence, because of all the noise when trying the Marvell PHY with
> > ptp4l, I came to the conlusion that NTP was a far better solution to
> > time synchronisation between machines than PTP would ever be due to
> > the nose induced by MDIO access. However, I should also state that I
> > basically gave up with PTP in the end because hardware support is
> > overall poor, and NTP just works - and I'd still have to run NTP for
> > the machines that have no PTP capabilities. PTP probably only makes
> > sense if one has a nice expensive grand master PTP clock on ones
> > network, and all the machines one wants to synchronise have decent
> > PTP implementations.)
> 
> Don't wanna waste too much of your time with the questions since
> I haven't done much research but - wouldn't MAC timestamp be a better
> choice more often (as long as it's a real, to-spec PTP stamp)? 
> Are we picking PHY for historical reasons?
> 
> Not that flipping the default would address the problem of regressing
> some setups..

There is the argument in PTP that having the PHY do the timestamping
results in the timestamps being "closer to the wire" meaning that
they will not be subject to any delays between the packet leaving
the MAC and being transmitted on the wire. Some PHYs have FIFOs or
other buffering which introduces a delay which PTP doesn't desire.

TI has a blog on this:

https://e2e.ti.com/blogs_/b/analogwire/posts/how-to-implement-ieee-1588-time-stamping-in-an-ethernet-transceiver

However, what is failed to be mentioned there is that yes, doing
PTP at the PHY means one can accurately trigger a capture of the
timestamp from the PHC when the SFD is detected on the media. That
is a great advantage, but is really only half the story.

If the PHC (hardware counter) in the PHY can't be accurately
synchronised, for example, it has a 1.5ppm second-to-second
variability, then the resulting timestamps will also have that
same variability. Another way to look at it is they will appear to
have 1.5ppm of noise. If this noise is random, then it becomes
difficult to filter that noise out, and results in jitter.

However, timestamping at the MAC may have only 40ppb of variability,
but have a resulting delay through the PHY. The delay through the
PHY will likely be constant, so the timestamps gathered at the MAC
will have a constant error but have much less noise.

Things change if one can use hardware signals to synchronise the
PHC, because then we become less reliant on a variable latency
accessing the PHY over the MDIO bus. The hardware event capture
allows the PHC to be captured on a hardware signal, software can
then read that timestamp, and if the hardware event is a PPS,
then that can be used to ensure that the PHC is ticking at the
correct rate. If the PPS is also aligned to a second boundary,
then the hardware PHC can also be aligned. With both, the latency
of the MDIO bus becomes irrelevant, and PTP at the PHY becomes a
far more preferable option.

Note that things which can influence the latency over the MDIO bus
include how many PHYs or other devices are also on it, and the
rate at which accesses to those PHYs are performed. Then there's
latency from kernel locking and scheduling. Maybe interrupt
latency if the MDIO bus driver uses interrupts.

When I created the generic Marvell PHY PTP layer (my patch set
for Marvell PHYs) I tried very hard to eliminate as many of these
sources of variable latency as possible - such as avoiding taking
and releasing locks on every MDIO bus access. Even with that, I
could not get the PHY's PHC to synchronise any better than 1.5ppm,
vs 40ppb for the PP2 MAC's PHC.

The last bit of consideration is whether the PHCs can be synchronised
in hardware. If one has multiple ethernet interfaces, no hardware
synchronisation of PHY PHCs, but also have MAC PTP support which is
shared across the ethernet interfaces, why would one want to use the
PHY with software based synchronisation of their individual PHCs.

So, to wrap this email up... if one has hardware purposely designed
for PTP support, which uses the PTP hardware signals from the
various PHCs, then one would want to use the PHY based timestamping.
If one doesn't have that, then one would want to use something that
gives the best timestamps, which may not necessarily be the PHY.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Russell King (Oracle)]

On Thu, Mar 02, 2023 at 11:37:52AM +0100, Köry Maincent wrote:
> Hello Russell,
> 
> > I have the situation on a couple of devices where there are multiple
> > places that can do PTP timestamping:
> > 
> > - the PHY (slow to access, only event capture which may not be wired,
> >    doesn't seem to synchronise well - delay of 58000, frequency changes
> >    every second between +/-1500ppb.)
> > - the Ethernet MAC (fast to access, supports event capture and trigger
> >    generation which also may not be wired, synchronises well, delay of
> >    700, frequency changes every second +/- 40ppb.)
> 
> Does this test have been made with the marvell 88E151x PHY?

Yes. Remember, however, that there are many factors that influence
the access latency (as I detailed in my chronologically previous
reply on the other thread.)

> On my side with a zynqMP SOM (cadence MACB MAC) the synchronization of the PHY
> PTP clock is way better: +/-50ppb. Do you have an idea about the difference? 
> Which link partner were you using? stm32mp157 hardware PTP on my side.

Both of my results were through synchronising the PHC with the
machine's local clock (which had already been synchronised via NTP),
thereby creating a "grand master" for my network. That's the only
way to setup a grand master short of purchasing specific kit for
that job (and from what I can see, that's $$$$$ - and out of reach
for your average volunteer kernel maintainer).

I also rigged up the PP2 to provide a PPS signal, connected that to
my TXCO-based frequency counter, measuring the PPS period - which
gives independent measurement of the stability of the PHC. It wasn't
possible to do that with the PHY since the hardware signals aren't
exposed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Thu, Mar 02, 2023 at 11:49:26AM +0000, Russell King (Oracle) wrote:

> Therefore, I believe that the Marvell PHY PTP implementation is all
> round inferior to that found in the Marvell PP2 MAC, and hence why I
> believe that the PP2 MAC implementation should be used by default over
> the PHY.

Yeah, that phy sure sounds like a lemon.

> (In essence, because of all the noise when trying the Marvell PHY with
> ptp4l, I came to the conlusion that NTP was a far better solution to
> time synchronisation between machines than PTP would ever be due to
> the nose induced by MDIO access. However, I should also state that I
> basically gave up with PTP in the end because hardware support is
> overall poor, and NTP just works - and I'd still have to run NTP for
> the machines that have no PTP capabilities. PTP probably only makes
> sense if one has a nice expensive grand master PTP clock on ones
> network, and all the machines one wants to synchronise have decent
> PTP implementations.)

Yes, NTP is really what most people need, and with PTP you really must
carefully select the hardware.  There is lots of PTP junk on the
market.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Thu, Mar 02, 2023 at 08:49:32AM -0800, Jakub Kicinski wrote:

> Don't wanna waste too much of your time with the questions since
> I haven't done much research but - wouldn't MAC timestamp be a better
> choice more often (as long as it's a real, to-spec PTP stamp)? 
> Are we picking PHY for historical reasons?

I think the default rating for MACs can be higher, but in the case of
at least one PHY, the TI PHYTER (dp83640), the PHY is the far better
choice.  This PHY surpasses any MAC on the market, and so it should
have a high rating.

For example, I've seen boards with the TI am335x, initially selected
because of the built in CPTS PTP feature, which the designers were
forced augment with the PHYTERs simply because of inadequate SoC
implementation.

In other words, if you find a board with the PHYTER, then it is there
for a reason.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

[Richard Cochran]

On Thu, Mar 02, 2023 at 11:37:52AM +0100, Köry Maincent wrote:

> On my side with a zynqMP SOM (cadence MACB MAC) the synchronization of the PHY
> PTP clock is way better: +/-50ppb. Do you have an idea about the difference? 
> Which link partner were you using? stm32mp157 hardware PTP on my side.

The predecessor Zynq 7000 SoC had a PTP MAC that was unusable.  So
much so, that the vendor simply deleted the PTP chapters from the data
sheet without a word.  IIRC, the newer MP has a different implementation,
but maybe they also flubbed that one?

In any case, this is another example of how a good PHY implementation
covers for a poor MAC one.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Michael Walle]

> > > (In essence, because of all the noise when trying the Marvell PHY with
> > > ptp4l, I came to the conlusion that NTP was a far better solution to
> > > time synchronisation between machines than PTP would ever be due to
> > > the nose induced by MDIO access. However, I should also state that I
> > > basically gave up with PTP in the end because hardware support is
> > > overall poor, and NTP just works - and I'd still have to run NTP for
> > > the machines that have no PTP capabilities. PTP probably only makes
> > > sense if one has a nice expensive grand master PTP clock on ones
> > > network, and all the machines one wants to synchronise have decent
> > > PTP implementations.)
> > 
> > Don't wanna waste too much of your time with the questions since
> > I haven't done much research but - wouldn't MAC timestamp be a better
> > choice more often (as long as it's a real, to-spec PTP stamp)? 
> > Are we picking PHY for historical reasons?
> > 
> > Not that flipping the default would address the problem of regressing
> > some setups..

Another thing I pointed out some time ago is that you might have a
working setup with MAC timestamping. Then comes along a patch which
adds PHY timestamping for the PHY you are using and it might break your
setup, because of the "PHY timestamping first" rule.

> There is the argument in PTP that having the PHY do the timestamping
> results in the timestamps being "closer to the wire" meaning that
> they will not be subject to any delays between the packet leaving
> the MAC and being transmitted on the wire. Some PHYs have FIFOs or
> other buffering which introduces a delay which PTP doesn't desire.
> 
> TI has a blog on this:
> 
> https://e2e.ti.com/blogs_/b/analogwire/posts/how-to-implement-ieee-1588-time-stamping-in-an-ethernet-transceiver
> 
> However, what is failed to be mentioned there is that yes, doing
> PTP at the PHY means one can accurately trigger a capture of the
> timestamp from the PHC when the SFD is detected on the media. That
> is a great advantage, but is really only half the story.
> 
> If the PHC (hardware counter) in the PHY can't be accurately
> synchronised, for example, it has a 1.5ppm second-to-second
> variability, then the resulting timestamps will also have that
> same variability. Another way to look at it is they will appear to
> have 1.5ppm of noise. If this noise is random, then it becomes
> difficult to filter that noise out, and results in jitter.

Exactly this.

> However, timestamping at the MAC may have only 40ppb of variability,
> but have a resulting delay through the PHY. The delay through the
> PHY will likely be constant, so the timestamps gathered at the MAC
> will have a constant error but have much less noise.

One thing to consider is also the quality of the oscillator of each
part. E.g. we design boards which have special temperture controlled
oscillators. How would you know where that one is connected to, MAC?
PHY? Both?

> Things change if one can use hardware signals to synchronise the
> PHC, because then we become less reliant on a variable latency
> accessing the PHY over the MDIO bus. The hardware event capture
> allows the PHC to be captured on a hardware signal, software can
> then read that timestamp, and if the hardware event is a PPS,
> then that can be used to ensure that the PHC is ticking at the
> correct rate. If the PPS is also aligned to a second boundary,
> then the hardware PHC can also be aligned. With both, the latency
> of the MDIO bus becomes irrelevant, and PTP at the PHY becomes a
> far more preferable option.
> 
> Note that things which can influence the latency over the MDIO bus
> include how many PHYs or other devices are also on it, and the
> rate at which accesses to those PHYs are performed. Then there's
> latency from kernel locking and scheduling. Maybe interrupt
> latency if the MDIO bus driver uses interrupts.
> 
> When I created the generic Marvell PHY PTP layer (my patch set
> for Marvell PHYs) I tried very hard to eliminate as many of these
> sources of variable latency as possible - such as avoiding taking
> and releasing locks on every MDIO bus access. Even with that, I
> could not get the PHY's PHC to synchronise any better than 1.5ppm,
> vs 40ppb for the PP2 MAC's PHC.
> 
> The last bit of consideration is whether the PHCs can be synchronised
> in hardware. If one has multiple ethernet interfaces, no hardware
> synchronisation of PHY PHCs, but also have MAC PTP support which is
> shared across the ethernet interfaces, why would one want to use the
> PHY with software based synchronisation of their individual PHCs.
> 
> So, to wrap this email up... if one has hardware purposely designed
> for PTP support, which uses the PTP hardware signals from the
> various PHCs, then one would want to use the PHY based timestamping.
> If one doesn't have that, then one would want to use something that
> gives the best timestamps, which may not necessarily be the PHY.

Which would make that a property of the board and not something
the software could figure out most of the time.

IMHO it really depends on how precise one want to go. Just saying
PHY timestamping is better isn't true. Vendors might go great
lengths to design their boards to do very well regarding precision
including PHY timestamping and maybe some kind of synchronization.

But in genernal, MAC timestamping - if available and not totally
broken - is the better choice IMHO. If it wasn't for the legacy rule,
I'd prefer MAC timestamping and just use PHY timestamping if MAC
timestamping is not available. If both are available, PHY
timestamping should be an opt-in.

-michael

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Thu, 2 Mar 2023 09:23:20 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> Do you happen to have a datasheet for MACB? The time stamping
> capability is suspiciously saved in a variable called hw_dma_cap
> which may indicate it's a DMA time stamp not a true PTP stamp.
> 
> Quite a few NICs/MACs support DMA time stamps because it's far 
> easier to take the stamp where the descriptor writing logic is
> (DMA block) than take it at the end of the MAC and haul it all 
> the way thru the pipeline back to the DMA block.

I don't have the datasheet but indeed you seem to have right. From the support
commit message: 
> Time stamps are obtained from the dma buffer descriptors

I suppose it is less precise as using true PTP stamp as it is an hardware block
further. Is that right?

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

I'm not sure we are making much progress here...

Lets divide an conquer. As far as i can see we have the following bits
of work to do:

1) Kernel internal plumbing to allow multiple time stampers for one
netdev. The PTP core probably needs to be the mux for all kAPI calls,
and any internal calling between components. This might mean changes
to all MAC drivers supporting PTP and time stampers. But i don't think
there is anything too controversial here, just plumbing work.

2) Some method to allow user space to control which time stamper is
used. Either an extension of the existing IOCTL interface, or maybe
ethtool. Depending on how ambitious we want to be, add a netlink API
to eventually replace the IOCTL interface?

3) Add a device tree binding to control which time stamper is
used. Probably a MAC property. Also probably not too controversial.

4) Some solution to the default choice if there is no DT property.

	Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Fri, 3 Mar 2023 14:20:20 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> I'm not sure we are making much progress here...
> 
> Lets divide an conquer. As far as i can see we have the following bits
> of work to do:
> 
> 1) Kernel internal plumbing to allow multiple time stampers for one
> netdev. The PTP core probably needs to be the mux for all kAPI calls,
> and any internal calling between components. This might mean changes
> to all MAC drivers supporting PTP and time stampers. But i don't think
> there is anything too controversial here, just plumbing work.
> 
> 2) Some method to allow user space to control which time stamper is
> used. Either an extension of the existing IOCTL interface, or maybe
> ethtool. Depending on how ambitious we want to be, add a netlink API
> to eventually replace the IOCTL interface?

Isn't the patch series (with small revisions) from Richard sufficient for this
two points?
https://lkml.kernel.org/netdev/Y%2F0N4ZcUl8pG7awc@shell.armlinux.org.uk/

> 3) Add a device tree binding to control which time stamper is
> used. Probably a MAC property. Also probably not too controversial.
> 
> 4) Some solution to the default choice if there is no DT property.

And in cases of architectures which do not support DT how do we deal with it?

Regards,
Köry

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

On Fri, Mar 03, 2023 at 02:34:55PM +0100, Köry Maincent wrote:
> On Fri, 3 Mar 2023 14:20:20 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > I'm not sure we are making much progress here...
> > 
> > Lets divide an conquer. As far as i can see we have the following bits
> > of work to do:
> > 
> > 1) Kernel internal plumbing to allow multiple time stampers for one
> > netdev. The PTP core probably needs to be the mux for all kAPI calls,
> > and any internal calling between components. This might mean changes
> > to all MAC drivers supporting PTP and time stampers. But i don't think
> > there is anything too controversial here, just plumbing work.
> > 
> > 2) Some method to allow user space to control which time stamper is
> > used. Either an extension of the existing IOCTL interface, or maybe
> > ethtool. Depending on how ambitious we want to be, add a netlink API
> > to eventually replace the IOCTL interface?
> 
> Isn't the patch series (with small revisions) from Richard sufficient for this
> two points?
> https://lkml.kernel.org/netdev/Y%2F0N4ZcUl8pG7awc@shell.armlinux.org.uk/

I've not looked at it. How about you make the small revisions and post
it. So long as the Marvell PHY PTP code is not merged, it should be
safe to work on these parts and not cause regressions.

> > 3) Add a device tree binding to control which time stamper is
> > used. Probably a MAC property. Also probably not too controversial.
> > 
> > 4) Some solution to the default choice if there is no DT property.
> 
> And in cases of architectures which do not support DT how do we deal with it?

x86 has ACPI. It seems like ACPI folks are happy to simply stuff DT
properties into ACPI nodes, rather than come up with a native ACPI
way. But we don't really need to support ACPI until somebody has an
ACPI board which needs to configure the time stamper.

Harder is things like USB, which mostly never has DT properties. But a
USB dongle tends to have known components, both the MAC and the PHY is
known, and so the dongle driver could make a call into the PTP core to
select which time stamper should be used. But i would suggest we solve
this problem when we actually reach it, not now.

     Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Russell King (Oracle)]

On Fri, Mar 03, 2023 at 02:20:20PM +0100, Andrew Lunn wrote:
> I'm not sure we are making much progress here...
> 
> Lets divide an conquer. As far as i can see we have the following bits
> of work to do:
> 
> 1) Kernel internal plumbing to allow multiple time stampers for one
> netdev. The PTP core probably needs to be the mux for all kAPI calls,
> and any internal calling between components. This might mean changes
> to all MAC drivers supporting PTP and time stampers. But i don't think
> there is anything too controversial here, just plumbing work.
> 
> 2) Some method to allow user space to control which time stamper is
> used. Either an extension of the existing IOCTL interface, or maybe
> ethtool. Depending on how ambitious we want to be, add a netlink API
> to eventually replace the IOCTL interface?
> 
> 3) Add a device tree binding to control which time stamper is
> used. Probably a MAC property. Also probably not too controversial.
> 
> 4) Some solution to the default choice if there is no DT property.

I thought (4) was what we have been discussing... but the problem
seems to be that folk want to drill down into the fine detail about
whether PHY or MAC timestamping is better than the other.

As I've already stated, I doubt DT maintainers will wear having a
DT property for this, on the grounds that it's very much a software
control rather than a hardware description.

On the other hand, if DT described the connectivity of the PTP
hardware signals, then one could probably make a decision based
upon that, and as that's describing hardware, it would be less
problematical during DT review.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

> > 4) Some solution to the default choice if there is no DT property.
> 
> I thought (4) was what we have been discussing... but the problem
> seems to be that folk want to drill down into the fine detail about
> whether PHY or MAC timestamping is better than the other.
> 
> As I've already stated, I doubt DT maintainers will wear having a
> DT property for this, on the grounds that it's very much a software
> control rather than a hardware description.

We can argue it is describing the hardware. In that hardware blob X
has a better implementation of PTP than hardware blob Y. That could be
because of the basic features of the blob, like resolution, adjustable
clocks, or board specific features, like temperature compensated
crystals, etc.

There could also be a linkage to the default choice. If we go with the
idea of giving each PTP stamper a quality indicator, and the highest
quality wins, we can have the DT property change this quality property
because it knows of external things, like the lack of a temperature
compensated crystal.

	Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Fri, Mar 03, 2023 at 05:34:35PM +0100, Andrew Lunn wrote:
> > > 4) Some solution to the default choice if there is no DT property.
> > 
> > I thought (4) was what we have been discussing... but the problem
> > seems to be that folk want to drill down into the fine detail about
> > whether PHY or MAC timestamping is better than the other.
> > 
> > As I've already stated, I doubt DT maintainers will wear having a
> > DT property for this, on the grounds that it's very much a software
> > control rather than a hardware description.
> 
> We can argue it is describing the hardware. In that hardware blob X
> has a better implementation of PTP than hardware blob Y. That could be
> because of the basic features of the blob, like resolution, adjustable
> clocks, or board specific features, like temperature compensated
> crystals, etc.

IMO a DTS property is the most user friendly solution, even if it
isn't strictly hardware description.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Richard Cochran]

On Fri, Mar 03, 2023 at 09:32:49AM -0800, Richard Cochran wrote:
> On Fri, Mar 03, 2023 at 05:34:35PM +0100, Andrew Lunn wrote:
> > > > 4) Some solution to the default choice if there is no DT property.
> > > 
> > > I thought (4) was what we have been discussing... but the problem
> > > seems to be that folk want to drill down into the fine detail about
> > > whether PHY or MAC timestamping is better than the other.
> > > 
> > > As I've already stated, I doubt DT maintainers will wear having a
> > > DT property for this, on the grounds that it's very much a software
> > > control rather than a hardware description.
> > 
> > We can argue it is describing the hardware. In that hardware blob X
> > has a better implementation of PTP than hardware blob Y. That could be
> > because of the basic features of the blob, like resolution, adjustable
> > clocks, or board specific features, like temperature compensated
> > crystals, etc.
> 
> IMO a DTS property is the most user friendly solution, even if it
> isn't strictly hardware description.

Afte all, the engineer who designs the board will know which is the
better choice for use case the board targets.

Thanks,
Richard

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Fri, 3 Mar 2023 14:12:47 +0100 Köry Maincent wrote:
> > Do you happen to have a datasheet for MACB? The time stamping
> > capability is suspiciously saved in a variable called hw_dma_cap
> > which may indicate it's a DMA time stamp not a true PTP stamp.
> > 
> > Quite a few NICs/MACs support DMA time stamps because it's far 
> > easier to take the stamp where the descriptor writing logic is
> > (DMA block) than take it at the end of the MAC and haul it all 
> > the way thru the pipeline back to the DMA block.  
> 
> I don't have the datasheet but indeed you seem to have right. From the support
> commit message: 
> > Time stamps are obtained from the dma buffer descriptors  

That's just saying that the time stamp is communicated via 
a descriptor rather than a register, so not a strong proof 
of where it's taken.

> I suppose it is less precise as using true PTP stamp as it is an hardware block
> further. Is that right?

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Jakub Kicinski]

On Fri, 3 Mar 2023 14:03:03 +0000 Russell King (Oracle) wrote:
> > 4) Some solution to the default choice if there is no DT property.  
> 
> I thought (4) was what we have been discussing... but the problem
> seems to be that folk want to drill down into the fine detail about
> whether PHY or MAC timestamping is better than the other.

There seems to be no solid heuristic. Maybe it's better to give up 
and force the user to pick (for new drivers)?

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

Hello Russell,

On Mon, 27 Feb 2023 15:20:05 +0000
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Mon, Feb 27, 2023 at 03:40:37PM +0100, Köry Maincent wrote:
> > Hello RMK,
> >   
> > > Hence why I'm at the point of giving up; I don't see that PTP will be
> > > of very limited benefit on my network with all these issues, and in
> > > any case, NTP has been "good enough" for the last 20+ years.  Given
> > > that only a limited number of machines will be able to implement PTP
> > > support anyway, NTP will have to run along side it.  
> > 
> > I see this patch has been abandoned.
> > I am testing it with a ZynqMP board (macb ethernet) and it seems to more or
> > less work. It got tx timestamp timeout at initialization but after some
> > times (~20 seconds) ptp4l manages to set it working. Also the IEEE 802.3
> > network PTP mode is not working, it constantly throw rx timestamp overrun
> > errors.
> > I will aim at fixing these issues and adding support to interrupts. It
> > would be good to have it accepted mainline. What do you think is missing
> > for that?  
> 
> It isn't formally abandoned, but is permanently on-hold as merging
> Marvell PHY PTP support into mainline _will_ regress the superior PTP
> support on the Macchiatobin platform for the reasons outlined in:
> 
> https://lore.kernel.org/netdev/20200729220748.GW1605@shell.armlinux.org.uk/
> 
> Attempting to fix this problem was basically rejected by the PTP
> maintainer, and thus we're at a deadlock over the issue, and Marvell
> PHY PTP support can never be merged into mainline.

As we are currently moving forward on PTP core to resolve this issue, I would
like to investigate your PHY PTP patch in parallel. Indeed it does not work very
well on my side.

The PTP UDP v4 and v6 work only if I add "--tx_timestamp_timeout 20" and the
PTP IEEE 802.3 (802.1AS) does not work at all.
On PTP IEEE 802.3 network transport ("ptp4l -2") I get continuously rx timestamp
overrun:
Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)
Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)

I know it's been a long time but does it ring a bell on your memory?

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Andrew Lunn]

> As we are currently moving forward on PTP core to resolve this issue, I would
> like to investigate your PHY PTP patch in parallel. Indeed it does not work very
> well on my side.
> 
> The PTP UDP v4 and v6 work only if I add "--tx_timestamp_timeout 20" and the
> PTP IEEE 802.3 (802.1AS) does not work at all.
> On PTP IEEE 802.3 network transport ("ptp4l -2") I get continuously rx timestamp
> overrun:
> Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)
> Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)
> 
> I know it's been a long time but does it ring a bell on your memory?

How are you talking to the PHY? I had issues with slow MDIO busses,
especially those embedded within an Ethernet switch. You end up with
MDIO over MDIO which has a lot of latency. I _think_ i added some
patches to ptp4l to deal with this, but i forget exactly what landed.

	Andrew

Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support [multicast/DSA issues]

[Köry Maincent]

On Thu, 27 Apr 2023 18:50:47 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > As we are currently moving forward on PTP core to resolve this issue, I
> > would like to investigate your PHY PTP patch in parallel. Indeed it does
> > not work very well on my side.
> > 
> > The PTP UDP v4 and v6 work only if I add "--tx_timestamp_timeout 20" and the
> > PTP IEEE 802.3 (802.1AS) does not work at all.
> > On PTP IEEE 802.3 network transport ("ptp4l -2") I get continuously rx
> > timestamp overrun:
> > Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)
> > Marvell 88E1510 ff0d0000.ethernet-ffffffff:01: rx timestamp overrun (5)
> > 
> > I know it's been a long time but does it ring a bell on your memory?  
> 
> How are you talking to the PHY? I had issues with slow MDIO busses,
> especially those embedded within an Ethernet switch. You end up with
> MDIO over MDIO which has a lot of latency. I _think_ i added some
> patches to ptp4l to deal with this, but i forget exactly what landed.

I am talking to the PHY through a simple MDIO bus.
The current PTP support from Russell does not support interrupts. Could it be
the cause of needing bigger tx timestamp timeout?