Peer to Peer One-Step time stamping

This series adds support for PTP (IEEE 1588) P2P one-step time stamping along with a driver for a hardware device that supports this.

If the hardware supports p2p one-step, it subtracts the ingress time stamp value from the Pdelay_Request correction field. The user space software stack then simply copies the correction field into the Pdelay_Response, and on transmission the hardware adds the egress time stamp into the correction field.

- Patch 1 adds the new option.
- Patches 2-4 adds support for MII time stamping in non-PHY devices.
- Patch 5 adds a driver implementing the new option.

Earlier today I posted user space support as an RFC on the linuxptp-devel list. Comments and review are most welcome.

Thanks,
Richard

Richard Cochran (5):
net: Introduce peer to peer one step PTP time stamping.
net: phy: Move time stamping interface into the generic mdio layer.
net: Introduce field for the MII time stamper.
net: Use the generic MII time stamper when available.
net: mdio: Add a driver for InES time stamping IP core.

 Documentation/devicetree/bindings/net/ines-ptp.txt |  42 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c   |   1 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/dp83640.c                          |  29 +-
 drivers/net/phy/ines_ptp.c                         | 857 +++++++++++++++++++++
 drivers/net/phy/mdio_bus.c                         |  51 +-
 drivers/net/phy/phy.c                              |   6 +-
 drivers/ptp/Kconfig                                |  10 +
 include/linux/mdio.h                               |  23 +
 include/linux/netdevice.h                          |   1 +
 include/linux/phy.h                                |  23 -
 include/uapi/linux/net_tstamp.h                    |   8 +
 net/Kconfig                                        |   8 +-
 net/core/dev_ioctl.c                               |   1 +
 net/core/ethtool.c                                 |   5 +-
 net/core/timestamping.c                            |  36 +-
 16 files changed, 1034 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ines-ptp.txt
 create mode 100644 drivers/net/phy/ines_ptp.c

2.11.0

The 1588 standard defines one step operation for both Sync and PDelay_Resp messages. Up until now, hardware with P2P one step has been rare, and kernel support was lacking. This patch adds support of the mode in anticipation of new hardware developments.

---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
 include/uapi/linux/net_tstamp.h                  | 8 ++++++++
 net/core/dev_ioctl.c                             | 1 +
 3 files changed, 10 insertions(+)

index 74fc9af4aadb..c6295e5c16af 100644
@@ -15379,6 +15379,7 @@ int bnx2x_configure_ptp_filters(struct bnx2x *bp)
 		       NIG_REG_P0_TLLH_PTP_RULE_MASK, 0x3EEE);
 		break;
 	case HWTSTAMP_TX_ONESTEP_SYNC:
+	case HWTSTAMP_TX_ONESTEP_P2P:
 		BNX2X_ERR("One-step timestamping is not supported\n");
 		return -ERANGE;
 	}

index 4fe104b2411f..f89b5a836c2a 100644
@@ -90,6 +90,14 @@ enum hwtstamp_tx_types {
 	 * queue.
 	 */
 	HWTSTAMP_TX_ONESTEP_SYNC,
+
+	/*
+	 * Same as HWTSTAMP_TX_ONESTEP_SYNC, but also enables time
+	 * stamp insertion directly into PDelay_Resp packets. In this
+	 * case, neither transmitted Sync nor PDelay_Resp packets will
+	 * receive a time stamp via the socket error queue.
+	 */
+	HWTSTAMP_TX_ONESTEP_P2P,
 };
 
 /* possible values for hwtstamp_config->rx_filter */

index 0ab1af04296c..cdda085e4b47 100644
@@ -187,6 +187,7 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
 	case HWTSTAMP_TX_OFF:
 	case HWTSTAMP_TX_ON:
 	case HWTSTAMP_TX_ONESTEP_SYNC:
+	case HWTSTAMP_TX_ONESTEP_P2P:
 		tx_type_valid = 1;
 		break;
 	}

2.11.0

There are different ways of obtaining hardware time stamps on network packets. The ingress and egress times can be measured in the MAC, in the PHY, or by a device listening on the MII bus. Up until now, the kernel has support for MAC and PHY time stamping, but not for other MII bus devices.

This patch moves the PHY time stamping interface into the generic mdio device in order to support MII time stamping hardware.

---
 drivers/net/phy/dp83640.c | 29 ++++++++++++++++++++---------
 drivers/net/phy/phy.c     |  4 ++--
 include/linux/mdio.h      | 23 +++++++++++++++++++++++
 include/linux/phy.h       | 23 -----------------------
 net/core/ethtool.c        |  4 ++--
 net/core/timestamping.c   |  8 ++++----
 6 files changed, 51 insertions(+), 40 deletions(-)

index 654f42d00092..79aeb5eb471a 100644
@@ -215,6 +215,10 @@ static LIST_HEAD(phyter_clocks);
 static DEFINE_MUTEX(phyter_clocks_lock);
 
 static void rx_timestamp_work(struct work_struct *work);
+static  int dp83640_ts_info(struct mdio_device *m, struct ethtool_ts_info *i);
+static  int dp83640_hwtstamp(struct mdio_device *m, struct ifreq *i);
+static bool dp83640_rxtstamp(struct mdio_device *m, struct sk_buff *s, int t);
+static void dp83640_txtstamp(struct mdio_device *m, struct sk_buff *s, int t);
 
 /* extended register access functions */
 

@@ -1162,6 +1166,12 @@ static int dp83640_probe(struct phy_device *phydev)
 		list_add_tail(&dp83640->list, &clock->phylist);
 
 	dp83640_clock_put(clock);
+
+	phydev->mdio.ts_info = dp83640_ts_info;
+	phydev->mdio.hwtstamp = dp83640_hwtstamp;
+	phydev->mdio.rxtstamp = dp83640_rxtstamp;
+	phydev->mdio.txtstamp = dp83640_txtstamp;
+
 	return 0;
 
 no_register:

@@ -1288,8 +1298,9 @@ static int dp83640_config_intr(struct phy_device *phydev)
 	}
 }
 
-static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
+static int dp83640_hwtstamp(struct mdio_device *mdev, struct ifreq *ifr)
 {
+	struct phy_device *phydev = container_of(mdev, struct phy_device, mdio);
 	struct dp83640_private *dp83640 = phydev->priv;
 	struct hwtstamp_config cfg;
 	u16 txcfg0, rxcfg0;

@@ -1397,9 +1408,10 @@ static void rx_timestamp_work(struct work_struct *work)
 		schedule_delayed_work(&dp83640->ts_work, SKB_TIMESTAMP_TIMEOUT);
 }
 
-static bool dp83640_rxtstamp(struct phy_device *phydev,
+static bool dp83640_rxtstamp(struct mdio_device *mdev,
 			     struct sk_buff *skb, int type)
 {
+	struct phy_device *phydev = container_of(mdev, struct phy_device, mdio);
 	struct dp83640_private *dp83640 = phydev->priv;
 	struct dp83640_skb_info *skb_info = (struct dp83640_skb_info *)skb->cb;
 	struct list_head *this, *next;

@@ -1446,9 +1458,10 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 	return true;
 }
 
-static void dp83640_txtstamp(struct phy_device *phydev,
+static void dp83640_txtstamp(struct mdio_device *mdev,
 			     struct sk_buff *skb, int type)
 {
+	struct phy_device *phydev = container_of(mdev, struct phy_device, mdio);
 	struct dp83640_private *dp83640 = phydev->priv;
 
 	switch (dp83640->hwts_tx_en) {

@@ -1471,9 +1484,11 @@ static void dp83640_txtstamp(struct phy_device *phydev,
 	}
 }
 
-static int dp83640_ts_info(struct phy_device *dev, struct ethtool_ts_info *info)
+static int dp83640_ts_info(struct mdio_device *mdev,
+			   struct ethtool_ts_info *info)
 {
-	struct dp83640_private *dp83640 = dev->priv;
+	struct phy_device *phydev = container_of(mdev, struct phy_device, mdio);
+	struct dp83640_private *dp83640 = phydev->priv;
 
 	info->so_timestamping =
 		SOF_TIMESTAMPING_TX_HARDWARE |

@@ -1504,10 +1519,6 @@ static struct phy_driver dp83640_driver = {
 	.config_init	= dp83640_config_init,
 	.ack_interrupt  = dp83640_ack_interrupt,
 	.config_intr    = dp83640_config_intr,
-	.ts_info	= dp83640_ts_info,
-	.hwtstamp	= dp83640_hwtstamp,
-	.rxtstamp	= dp83640_rxtstamp,
-	.txtstamp	= dp83640_txtstamp,
 };
 
 static int __init dp83640_init(void)

index c2d9027be863..466bf88053ce 100644
@@ -457,8 +457,8 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->drv && phydev->drv->hwtstamp)
-			return phydev->drv->hwtstamp(phydev, ifr);
+		if (phydev->mdio.hwtstamp)
+			return phydev->mdio.hwtstamp(&phydev->mdio, ifr);
 		/* fall through */
 
 	default:

index 2cfffe586885..70a82c973aed 100644
@@ -37,6 +37,29 @@ struct mdio_device {
 	void (*device_free)(struct mdio_device *mdiodev);
 	void (*device_remove)(struct mdio_device *mdiodev);
 
+	/* Handles ethtool queries for hardware time stamping. */
+	int (*ts_info)(struct mdio_device *dev, struct ethtool_ts_info *ti);
+
+	/* Handles SIOCSHWTSTAMP ioctl for hardware time stamping. */
+	int  (*hwtstamp)(struct mdio_device *dev, struct ifreq *ifr);
+
+	/*
+	 * Requests a Rx timestamp for 'skb'. If the skb is accepted,
+	 * the mdio device promises to deliver it using netif_rx() as
+	 * soon as a timestamp becomes available. One of the
+	 * PTP_CLASS_ values is passed in 'type'. The function must
+	 * return true if the skb is accepted for delivery.
+	 */
+	bool (*rxtstamp)(struct mdio_device *dev, struct sk_buff *skb, int type);
+
+	/*
+	 * Requests a Tx timestamp for 'skb'. The mdio device promises
+	 * to deliver it using skb_complete_tx_timestamp() as soon as a
+	 * timestamp becomes available. One of the PTP_CLASS_ values
+	 * is passed in 'type'.
+	 */
+	void (*txtstamp)(struct mdio_device *dev, struct sk_buff *skb, int type);
+
 	/* Bus address of the MDIO device (0-31) */
 	int addr;
 	int flags;

index 5a9b1753fdc5..77d3f703b8ce 100644
@@ -570,29 +570,6 @@ struct phy_driver {
 	 */
 	int (*match_phy_device)(struct phy_device *phydev);
 
-	/* Handles ethtool queries for hardware time stamping. */
-	int (*ts_info)(struct phy_device *phydev, struct ethtool_ts_info *ti);
-
-	/* Handles SIOCSHWTSTAMP ioctl for hardware time stamping. */
-	int  (*hwtstamp)(struct phy_device *phydev, struct ifreq *ifr);
-
-	/*
-	 * Requests a Rx timestamp for 'skb'. If the skb is accepted,
-	 * the phy driver promises to deliver it using netif_rx() as
-	 * soon as a timestamp becomes available. One of the
-	 * PTP_CLASS_ values is passed in 'type'. The function must
-	 * return true if the skb is accepted for delivery.
-	 */
-	bool (*rxtstamp)(struct phy_device *dev, struct sk_buff *skb, int type);
-
-	/*
-	 * Requests a Tx timestamp for 'skb'. The phy driver promises
-	 * to deliver it using skb_complete_tx_timestamp() as soon as a
-	 * timestamp becomes available. One of the PTP_CLASS_ values
-	 * is passed in 'type'.
-	 */
-	void (*txtstamp)(struct phy_device *dev, struct sk_buff *skb, int type);
-
 	/* Some devices (e.g. qnap TS-119P II) require PHY register changes to
 	 * enable Wake on LAN, so set_wol is provided to be called in the
 	 * ethernet driver's set_wol function. */

index 157cd9efa4be..b4b81eaa15a9 100644
@@ -2218,8 +2218,8 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)
 	memset(&info, 0, sizeof(info));
 	info.cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phydev && phydev->drv && phydev->drv->ts_info) {
-		err = phydev->drv->ts_info(phydev, &info);
+	if (phydev && phydev->mdio.ts_info) {
+		err = phydev->mdio.ts_info(&phydev->mdio, &info);
 	} else if (ops->get_ts_info) {
 		err = ops->get_ts_info(dev, &info);
 	} else {

index 42689d5c468c..b8857028e652 100644
@@ -46,11 +46,11 @@ void skb_clone_tx_timestamp(struct sk_buff *skb)
 		return;
 
 	phydev = skb->dev->phydev;
-	if (likely(phydev->drv->txtstamp)) {
+	if (likely(phydev->mdio.txtstamp)) {
 		clone = skb_clone_sk(skb);
 		if (!clone)
 			return;
-		phydev->drv->txtstamp(phydev, clone, type);
+		phydev->mdio.txtstamp(&phydev->mdio, clone, type);
 	}
 }
 EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);

@@ -76,8 +76,8 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 		return false;
 
 	phydev = skb->dev->phydev;
-	if (likely(phydev->drv->rxtstamp))
-		return phydev->drv->rxtstamp(phydev, skb, type);
+	if (likely(phydev->mdio.rxtstamp))
+		return phydev->mdio.rxtstamp(&phydev->mdio, skb, type);
 
 	return false;
 }

2.11.0

This patch adds a new field to the network device structure to reference a time stamping device on the MII bus. By decoupling the time stamping function from the PHY device, we pave the way to allowing a non-PHY device to take this role.

---
 drivers/net/phy/mdio_bus.c | 51 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/netdevice.h  |  1 +
 2 files changed, 51 insertions(+), 1 deletion(-)

index 24b5511222c8..fdac8c8ac272 100644
@@ -717,6 +717,47 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static bool mdiodev_supports_timestamping(struct mdio_device *mdiodev)
+{
+	if (mdiodev->ts_info  && mdiodev->hwtstamp &&
+	    mdiodev->rxtstamp && mdiodev->txtstamp)
+		return true;
+	else
+		return false;
+}
+
+static int mdiobus_netdev_notification(struct notifier_block *nb,
+				       unsigned long msg, void *ptr)
+{
+	struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
+	struct phy_device *phydev = netdev->phydev;
+	struct mdio_device *mdev;
+	struct mii_bus *bus;
+	int i;
+
+	if (netdev->mdiots || msg != NETDEV_UP || !phydev)
+		return NOTIFY_DONE;
+
+	/*
+	 * Examine the MII bus associated with the PHY that is
+	 * attached to the MAC.  If there is a time stamping device
+	 * on the bus, then connect it to the network device.
+	 */
+	bus = phydev->mdio.bus;
+
+	for (i = 0; i < PHY_MAX_ADDR; i++) {
+		mdev = bus->mdio_map[i];
+		if (!mdev)
+			continue;
+		if (mdiodev_supports_timestamping(mdev)) {
+			netdev->mdiots = mdev;
+			return NOTIFY_OK;
+		}
+	}
+
+	return NOTIFY_DONE;
+}
+
 #ifdef CONFIG_PM
 static int mdio_bus_suspend(struct device *dev)
 {

@@ -772,6 +813,10 @@ struct bus_type mdio_bus_type = {
 };
 EXPORT_SYMBOL(mdio_bus_type);
 
+static struct notifier_block mdiobus_netdev_notifier __read_mostly = {
+	.notifier_call = mdiobus_netdev_notification,
+};
+
 int __init mdio_bus_init(void)
 {
 	int ret;

@@ -782,14 +827,18 @@ int __init mdio_bus_init(void)
 		if (ret)
 			class_unregister(&mdio_bus_class);
 	}
+	if (ret)
+		return ret;
+
+	return register_netdevice_notifier(&mdiobus_netdev_notifier);
 
-	return ret;
 }
 EXPORT_SYMBOL_GPL(mdio_bus_init);
 
 #if IS_ENABLED(CONFIG_PHYLIB)
 void mdio_bus_exit(void)
 {
+	unregister_netdevice_notifier(&mdiobus_netdev_notifier);
 	class_unregister(&mdio_bus_class);
 	bus_unregister(&mdio_bus_type);
 }

index 5fbb9f1da7fd..223d691aa0b0 100644
@@ -1943,6 +1943,7 @@ struct net_device {
 	struct netprio_map __rcu *priomap;
 #endif
 	struct phy_device	*phydev;
+	struct mdio_device	*mdiots;
 	struct lock_class_key	*qdisc_tx_busylock;
 	struct lock_class_key	*qdisc_running_key;
 	bool			proto_down;

2.11.0

Now that the infrastructure is in place, convert the PHY time stamping logic to use the generic MII device. This change has the added benefit of simplifying the code somewhat.

---
 drivers/net/phy/phy.c   |  6 ++++--
 net/Kconfig             |  8 ++++----
 net/core/ethtool.c      |  5 ++---
 net/core/timestamping.c | 36 ++++++++++--------------------------
 4 files changed, 20 insertions(+), 35 deletions(-)

index 466bf88053ce..df80c6b14478 100644
@@ -397,6 +397,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
 	bool change_autoneg = false;
+	struct net_device *netdev;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:

@@ -457,8 +458,9 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->mdio.hwtstamp)
-			return phydev->mdio.hwtstamp(&phydev->mdio, ifr);
+		netdev = phydev->attached_dev;
+		if (netdev->mdiots->hwtstamp)
+			return netdev->mdiots->hwtstamp(netdev->mdiots, ifr);
 		/* fall through */
 
 	default:

index 0428f12c25c2..e38403cd010c 100644
@@ -102,12 +102,12 @@ config NET_PTP_CLASSIFY
 	def_bool n
 
 config NETWORK_PHY_TIMESTAMPING
-	bool "Timestamping in PHY devices"
+	bool "Timestamping in PHY and MII bus devices"
 	select NET_PTP_CLASSIFY
 	help
-	  This allows timestamping of network packets by PHYs with
-	  hardware timestamping capabilities. This option adds some
-	  overhead in the transmit and receive paths.
+	  This allows timestamping of network packets by PHYs or other
+	  MII bus devices with hardware timestamping capabilities. This
+	  option adds some overhead in the transmit and receive paths.
 
 	  If you are unsure how to answer this question, answer N.
 

index b4b81eaa15a9..507e56abecb7 100644
@@ -2213,13 +2213,12 @@ static int ethtool_get_ts_info(struct net_device *dev, void __user *useraddr)
 	int err = 0;
 	struct ethtool_ts_info info;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
-	struct phy_device *phydev = dev->phydev;
 
 	memset(&info, 0, sizeof(info));
 	info.cmd = ETHTOOL_GET_TS_INFO;
 
-	if (phydev && phydev->mdio.ts_info) {
-		err = phydev->mdio.ts_info(&phydev->mdio, &info);
+	if (dev->mdiots && dev->mdiots->ts_info) {
+		err = dev->mdiots->ts_info(dev->mdiots, &info);
 	} else if (ops->get_ts_info) {
 		err = ops->get_ts_info(dev, &info);
 	} else {

index b8857028e652..ecb0ecb03740 100644
@@ -23,44 +23,32 @@
 #include <linux/skbuff.h>
 #include <linux/export.h>
 
-static unsigned int classify(const struct sk_buff *skb)
-{
-	if (likely(skb->dev && skb->dev->phydev &&
-		   skb->dev->phydev->drv))
-		return ptp_classify_raw(skb);
-	else
-		return PTP_CLASS_NONE;
-}
-
 void skb_clone_tx_timestamp(struct sk_buff *skb)
 {
-	struct phy_device *phydev;
 	struct sk_buff *clone;
 	unsigned int type;
 
-	if (!skb->sk)
+	if (!skb->sk || !skb->dev || !skb->dev->mdiots)
 		return;
 
-	type = classify(skb);
+	type = ptp_classify_raw(skb);
+
 	if (type == PTP_CLASS_NONE)
 		return;
 
-	phydev = skb->dev->phydev;
-	if (likely(phydev->mdio.txtstamp)) {
-		clone = skb_clone_sk(skb);
-		if (!clone)
-			return;
-		phydev->mdio.txtstamp(&phydev->mdio, clone, type);
-	}
+	clone = skb_clone_sk(skb);
+	if (!clone)
+		return;
+
+	skb->dev->mdiots->txtstamp(skb->dev->mdiots, clone, type);
 }
 EXPORT_SYMBOL_GPL(skb_clone_tx_timestamp);
 
 bool skb_defer_rx_timestamp(struct sk_buff *skb)
 {
-	struct phy_device *phydev;
 	unsigned int type;
 
-	if (!skb->dev || !skb->dev->phydev || !skb->dev->phydev->drv)
+	if (!skb->dev || !skb->dev->mdiots)
 		return false;
 
 	if (skb_headroom(skb) < ETH_HLEN)

@@ -75,10 +63,6 @@ bool skb_defer_rx_timestamp(struct sk_buff *skb)
 	if (type == PTP_CLASS_NONE)
 		return false;
 
-	phydev = skb->dev->phydev;
-	if (likely(phydev->mdio.rxtstamp))
-		return phydev->mdio.rxtstamp(&phydev->mdio, skb, type);
-
-	return false;
+	return skb->dev->mdiots->rxtstamp(skb->dev->mdiots, skb, type);
 }
 EXPORT_SYMBOL_GPL(skb_defer_rx_timestamp);

2.11.0

The InES at the ZHAW offers a PTP time stamping IP core. The FPGA logic recognizes and time stamps PTP frames on the MII bus. This patch adds a driver for the core along with a device tree binding to allow hooking the driver to MAC devices.

---
 Documentation/devicetree/bindings/net/ines-ptp.txt |  42 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/ines_ptp.c                         | 857 +++++++++++++++++++++
 drivers/ptp/Kconfig                                |  10 +
 4 files changed, 910 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ines-ptp.txt
 create mode 100644 drivers/net/phy/ines_ptp.c

new file mode 100644
index 000000000000..ed7b1d773ded
@@ -0,0 +1,42 @@
+ZHAW InES PTP time stamping IP core
+
+The IP core needs two different kinds of nodes.  The control node
+lives somewhere in the memory map and specifies the address of the
+control registers.  There can be up to three port nodes placed on the
+mdio bus.  They associate a particular MAC with a port index within
+the IP core.
+
+Required properties of the control node:
+
+- compatible:		"ines,ptp-ctrl"
+- reg:			physical address and size of the register bank
+- phandle:		globally unique handle for the ports to point to
+
+Required properties of the port nodes:
+
+- compatible:		"ines,ptp-port"
+- ctrl-handle:		points to the control node
+- port-index:		port channel within the IP core
+- reg:			phy address. This is required even though the
+			device does not respond to mdio operations
+
+Example:
+
+	timestamper@60000000 {
+		compatible = "ines,ptp-ctrl";
+		reg = <0x60000000 0x80>;
+		phandle = <0x10>;
+	};
+
+	ethernet@80000000 {
+		...
+		mdio {
+			...
+			timestamper@1f {
+				compatible = "ines,ptp-port";
+				ctrl-handle = <0x10>;
+				port-index = <0>;
+				reg = <0x1f>;
+			};
+		};
+	};

index 01acbcb2c798..e286bb822295 100644
@@ -61,6 +61,7 @@ obj-$(CONFIG_DP83848_PHY)	+= dp83848.o
 obj-$(CONFIG_DP83867_PHY)	+= dp83867.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o
 obj-$(CONFIG_ICPLUS_PHY)	+= icplus.o
+obj-$(CONFIG_INES_PTP_TSTAMP)	+= ines_ptp.o
 obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
 obj-$(CONFIG_LSI_ET1011C_PHY)	+= et1011c.o
 obj-$(CONFIG_LXT_PHY)		+= lxt.o

index a21ad10d613c..10ef161dd2a1 100644
@@ -88,6 +88,16 @@ config DP83640_PHY
 	  In order for this to work, your MAC driver must also
 	  implement the skb_tx_timestamp() function.
 
+config INES_PTP_TSTAMP
+	tristate "ZHAW InES PTP time stamping IP core"
+	depends on NETWORK_PHY_TIMESTAMPING
+	depends on PHYLIB
+	depends on PTP_1588_CLOCK
+	---help---
+	  This driver adds support for using the ZHAW InES 1588 IP
+	  core.  This clock is only useful if the MII bus of your MAC
+	  is wired up to the core.
+
 config PTP_1588_CLOCK_PCH
 	tristate "Intel PCH EG20T as PTP clock"
 	depends on X86_32 || COMPILE_TEST

2.11.0

Hi Richard

Can you point us at some documentation for this.

I think Florian and I want to better understand how this device works, in order to understand your other changes.

Andrew

Yes. Anything else doesn't make sense, don't you think?

Also, reading 1588, it isn't clear whether supporting only 1-step Sync without 1-step P2P is even intended. There is only a "one-step clock", and it is described as doing both.

Thanks,
Richard

The overall one-step functionality is described IEEE 1588.

The device is from here:

https://www.zhaw.ch/en/engineering/institutes-centres/ines/products-and-services/ptp-ieee-1588/ptp-hardware/#c43991

The only other docs that I have is a PDF of the register layout, but I don't think I can redistribute that. Actually, there really isn't any detail in that doc at all.

Thanks,
Richard

Hi Richard

O.K, so lets do the 20 questions approach.

As far as i can see, this is not an MDIO device. It is not connected to the MDIO bus, it has no MDIO registers, you don't even pass a valid MDIO address in device tree.

It it actually an MII bus snooper? Does it snoop, or is it actually in the MII bus, and can modify packets, i.e. insert time stamps as frames pass over the MII bus?

When the driver talks about having three ports, does that mean it can be on three different MII busses?

Thanks
Andrew

The question could be asked of:

struct mdio_device {
	int (*bus_match)(struct device *dev, struct device_driver *drv);
	void (*device_free)(struct mdio_device *mdiodev);
	void (*device_remove)(struct mdio_device *mdiodev);
}

I saw how this is done for the phy, etc, but I don't see any benefit of doing it that way. It would add an extra layer (or two) of indirection and save the space four pointer functions. Is that trade-off worth it?

Thanks,
Richard

The phydev is the only thing that associates a netdev with an MII bus.

Why would it need to be cleared?

We have no guarantee the mdio device has been probed yet.

We only need the phydev to get from the netdev to the mii bus.

It would be strange and wrong to "steal" the phy's mdio struct, IMHO. After all, we just got support for non-PHY mdio devices. The natural solution is to use it.

Thanks,
Richard

:)

Right. There might very well be other products out there that *do* use MDIO commands. I know that there are MII time stamping asics and ip cores on the market, but I don't know all of their creative design details.

It acts like a "snooper" to provide out of band time stamps, but it also can modify packets when for the one-step functionality.

Yes.

HTH,
Richard

O.K, so i suggest we stop trying to model this thing as an MDIO device. It is really an MMIO device.

So i suggest we leave the design for those until we actual see one.

O.K, so here is how i think it should be done. It is a device which offers services to other devices. It is not that different to an interrupt controller, a GPIO controller, etc. Lets follow how they work in device tree....

The device itself is just another MMIO mapped device in the SoC:

    	timestamper@60000000 {
		compatible = "ines,ptp-ctrl";
                reg = <0x60000000 0x80>;
		#address-cells = <1>;
		#size-cells = <0>;
	};

The MAC drivers are clients of this device. They then use a phandle and specifier:

	eth0: ethernet-controller@72000 {
		compatible = "marvell,kirkwood-eth";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x72000 0x4000>;

		timerstamper = <&timestamper 2>
	}

The 2 indicates this MAC is using port 2.

The MAC driver can then do the standard device tree things to follow the phandle to get access to the device and use the API it exports.

Andrew

But that would require hacking every last MAC driver.

I happy to improve the modeling, but the solution should be generic and work for every MAC driver.

Thanks,
Richard

Well, the solution is generic, in that the phandle can point to a device anywhere. It could be MMIO, it could be on an MDIO bus, etc. You just need to make sure you API makes no assumption about how the device driver talks to the hardware.

How clever is this device? Can it tell the difference between 1000Base-X and SGMII? Can it figure out that the MAC is repeating every bit 100 times and so has dropped to 10Mbits? Does it understand EEE? Does it need to know if RGMII or RGMII-ID is being used?

Can such a device really operation without the MAC being involved? My feeling is it needs to understand how the MII bus is being used. It might also be that the device is less capable than the MAC, so you need to turn off some of the MAC features. I think you are going to need the MAC actively involved in this.

Andrew

This was my understanding as well, but given the limited hardware which can do sync but not pdelay messages, I just wanted to make sure we were on the same page.

Thanks,
Jake

Something else to think about. There are a number of MAC drivers which don't use phylib. All the intel drivers for example. They have their own MDIO and PHY code. And recently there have been a number of MAC drivers for hardware capable of > 1GBps which do all the PHY control in firmware.

A phydev is optional, the MAC is mandatory.

Andrew

Let me qualify that a bit...

So MACs that have a built in PHY won't work, but we don't care because there is no way to hang another MII device in there anyhow.

We already require phylib for NETWORK_PHY_TIMESTAMPING, and so we expect that here, too.

Many of these IP core things will be targeting arm with device tree, and I want that to "just work" without MAC changes.

(This is exactly the same situation with DSA, BTW.)

If someone attaches an MII time stamper to a MACs whose driver does their own thing without phylib, then they are going to have to hack the MAC driver in any case. Such hacks will never be acceptable for mainline because they are design specific. We really don't have to worry about this case.

Thanks,
Richard

Actually, there was another reason not to put these methods into the driver structure. In contrast to phy_device, mdio_device doesn't have a pointer to the driver structure. It looks like there is no way to start from a mdio_device and get to a mdio_driver. Please correct me if I'm wrong.

I propose keeping those methods in the mdio_device, wrapping them in #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING. That way, they do not add any burden to the vast majority of users.

Thoughts?

Thanks,
Richard

You are right in that this field is wasted space for most users.

In V2 this will be inside #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING.

Thanks,
Richard

This device isn't configurable at run time for any of those AFAICT. Those decisions are made when the IP core is synthesized as part of the HW design.

You are right in that this particular device *does* need to know the link speed. I have neglected that part for this RFC. I'm looking for a notification based method of informing the device of link speed changes, but without hacking any MAC driver.

In general, we might see devices one day that care about things like EEE for example, but let's cross that bridge when we come to it. In the case of EEE, when the user enables it via ethtool we can tell the time stamping device directly without hacking the MAC driver.

Thanks,
Richard

Hi Richard

Should we be designing an API for this specific device, or should we think of the general case?

The general case would be a device which passes through MII signals, and has some sort of control interface.

The control interface could be MMIO as here, MDIO, I2C, SPI, etc.

The MII interfaces could be MII, RMII, GMII, RGMII, SGMII, QSGMII, XGMII, etc. Generally, a device which can do GMII can also do RGMII, etc.

The device probably needs to know about the MII bus. If it is synthesized as part of the hardware design, it might be able to get this information directly from the MAC IP core. An external device probably needs to be told. Especially when it comes to RGII, where the clocking is 'interesting'.

As you already said, you need to know link speed. Do we want to be able to restrict the MAC to specific link speeds? The Marvell MACs can do a 2.5Gbps SGMII, by speeding up the clock. I can image a PTP device not supporting this, the MII breaks, but PHY looks happy. To limit this cleanly, you at least need to mask out the unsupported auto-neg offered speeds.

With EEE there is probably two cases:

1) The PTP device understands it, but needs to be told it has been enabled.
2) The PTP device breaks when EEE is enabled, so EEE needs to be disabled.

To me, 1) seems pretty unlikely. But 2) is possible. Disabling EEE again means changing the auto-neg parameters, so that EEE is not offered.

As far as i can see, you have three basic problems:

1) How do you associate the PTP device to the netdev?
2) How do you get the information you need to configure the PTP device
3) How do you limit the MAC/PHY to what the PTP device can do.

For 1) you need some sort of phandle, either in the MAC, or the PHY section of device tree. There is currently no generic code for handling MAC OF nodes. phylib however does do all the generic work for PHY nodes in DT.

2) you can get some of the information you need via notifiers. e.g, NETDEV_CHANGE tells you something has happen, up/down, etc. But i'm not sure it is 100% reliable. A PHY might be able to do 1G->100M without going down and back up again, so you don't get a NETDEV_CHANGE event. There is no notification of how the MII bus is being used. So i think notifiers is probably not the best bet.

phylib does have all this information. It is phylib that calls the MAC with link speed information. When the MAC connects to the PHY, it passes the MII mode, and when the PHY requests the MII mode changes, phylib knows. The MAC has to call phy_init_eee() to see if the PHY is EEE capable. phylib also tells the MAC what speeds the PHY is capable off, so it is in the position to mask out speeds the PTP device does not support, etc.

So i really think you need to cleanly integrate into phylib and phylink.

Andrew

Yes, yes.

Hm, I don't think this is important.

Right, so phylib can operate on phydev->attached_dev->mdiots;

So I think I've done that, more or less, but I'd like to hear your ideas on how to make it cleaner...

Thanks,
Richard

So you are happy that the PTP device will cause the MC/PHY link to break down when it is asked to do something it cannot do? Take for example a Marvell MAC connected to a Marvell PHY doing 2.5Gbps SGMII because it can. But say the PTP device cannot be clocked that fast, and the MII links break.... You as a PTP maintainer might be happy with that, but as a PHY maintainer, i'm not too happy with this.

So first off, it is not an MDIO device. You current code is a horrible hack which gets a NACK. Use a phandle, and have of_mdiobus_register_phy() follow the phandle to get the device.

To keep lifecycle issues simple, i would also keep it in phydev, not netdev.

mdiots as a name is completely wrong. It is not an MDIO device. Maybe in the future some devices will be MDIO, or I2C, or SPI. Just call it ptpdev. This ptpdev needs to be control bus agnostic. You need a ptpdev core API exposing functions like ptpdev_hwtstamp, ptpdev_rxtstamp, ptpdev_txtstamp, ptpdev_link_change, which take a ptpdev. Have phy_link_change call ptpdev_link_change. You have the flexibility in that if in the future you do care that your ptpdev breaks the MAC-PHY link, you can add a ptpdev_validate_advertise, which allows the ptpdev to mask out link modes it does not support.

Your ptp device, when probing needs to register with the ptpdev core, passing a generic ptpdev_ops for the operations its support. How it talks to the device, MMIO, SPI, I2C is hidden within the device driver.

You can then clean up the code in timestamping.c. Code like:

        phydev = skb->dev->phydev;
        if (likely(phydev->drv->txtstamp)) {
                clone = skb_clone_sk(skb);
                if (!clone)
                        return;
                phydev->drv->txtstamp(phydev, clone, type);
        }

violates the layering, and the locking looks broken. Add a phy_txtstamp() call to phylib. It can then either call into the PHY driver, or use the call the ptpdev API, or -EOPNOTSUP.

Andrew

No, but I do expect the system designer to use common sense. No need to over engineer this now just to catch hypothetical future problems.

...

I am well aware of what terms MDIO and MII represent, but our current code is hopelessly confused. Case in point:

	static inline struct mii_bus *mdiobus_alloc(void);

The kernel's 'struct mii_bus' is really only about MDIO and not about the rest of MII. Clearly MII time stamping devices belong on the MII bus, so that is where I put them.

Or maybe mii_bus should first be renamed to mdio_bus? That you pave the way for introducing a representation of the MII bus.

Well, this name is not ideal, since time stamping devices in general can time stamp any frame, not just PTP frames.

Are you sure the locking is broken? If so, how?

(This code path has been reviewed more than once.)

Can you please explain the layering and how this code breaks it?

(This code goes back to 2.6.36 and perhaps predates the layers that were added later on.)

Thanks,
Richard

Okay.

Since we don't have any representation for MII anyhow, it seems equally fitting to attach this to the PHY's data structure as to the MAC's.

Thanks,
Richard

They do, if and only if their control path goes through the MDIO bus, this one does not, see below.

It would have been ideal to name this structure mdio_bus, because that is what it indeed is. An argument could be made this is true about a lot of devices though, e.g: PCIe end point drivers do register a pci_driver/device, not a pcie_driver/device...

Andrew still has a valid point though that devices are child of their control bus, not data bus. The data bus here is MII, but the control bus is here through MMIO register, therefore platform device in Linux's device driver model so we would expect the

The best that I can think about and it still is a hack in some way, is to you have your time stamping driver create a proxy mii_bus whose purpose is just to hook to mdio/phy_device events (such as link changes) in order to do what is necessary, or at least, this would indicate its transparent nature towards the MDIO/MDC lines...

What is not great in your current binding is that you created a notion of "port" which is tied to the timestamper device, whereas it really seems to be a property of the Ethernet controller, where the datapath being timestamped resides.

Tangential: the existing PHY time stamping logic should probably be generalized to a mdio_device (which the phy_device is a specialized superset of) instead of to the phy_device. This would still allow existing use cases but it would also allow us to support possible "pure MDIO" devices would that become some thing in the future.

I see it both ways, you either invoke an operation to timestamp which goes into an abstract "timestamping device" which invokes a driver to determine the actual operation, or you can do what you did which was to augment struct net_device with a phy_device, under the premise this will be the only type of timestamping capable device we will ever see.

This is no longer holding true, your patches are a testament to that, so now you need another member: mdiots, as you can see, there is now overlap between the two, because a phy_device should arguably be encapsulating a mdiots device object...

Florian

As a general rule of thumb, locking is performed in the core when possible. Experience has shown that device driver writers get locking wrong more often than core code writers. So doing it once in the core results in less bugs.

The phylib core code will take the phydev lock before calling into the driver. By violating the layering, we are missing on this lock.

Maybe the one driver which currently implements these calls does not need locking. But after the Marvell DSA work, we have most of the code needed to implement support for the Marvell PHY PTP. It has pretty similar registers. That would need the phydev lock to be held, because we need to swap to different pages, so any other calls happening in parallel are going to see registers from the wrong page. I don't want to have to put these locks in the driver, that leads to bugs. The core should do it.

Andrew

Hi Richard

I don't really care about the name. I care about the semantics of the API. How about ieee1588_rxtstamp, ieee1588_txtstamp, etc.

Andrew

That won't work at all, AFAICT. There is only one mii_bus per netdev, that is one that is attached to the phydev.

So this is exactly what I did. The time stamping methods were pushed down into the mdio_device. The active device (mdiots pointer) points either to a non-PHY mdio_device or to the mdio_device embedded in the phydev.

Thanks,
Richard

That lock protects the fields within the struct phy_device, like the state field. None of the time stamping methods need to read or write any part of that data structure.

Actually it is not true that the core always takes the lock before calling the driver methods. See .ack_interrupt for example.

It has locking. If a specific device needs locking to protect the integrity of its registers or other internal data structures, then it can and should implement those locks.

Thanks,
Richard

Hi Richard

Have you tried implementing it using a phandle in the phy node, pointing to the time stamping device?

I think it makes a much better architecture.

Andrew

Not yet, but I'll take this up for V2, after the merge window...

Thinking about MII, it really is a 1:1 connection between the MAC and the PHY. It has no representation in the current code, at least not yet. It is too bad about the naming of mii_bus, oh well. While hanging this thing off of the PHY isn't really great modeling (it isn't a sub-device of the PHY in any sense), still this will work well enough to enable the new functionality.

Thanks,
Richard