[PATCH net-next RFC V1 0/5] Peer to Peer One-Step time stamping

[PATCH net-next RFC V1 0/5] Peer to Peer One-Step time stamping

[Richard Cochran]

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

[PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP time stamping.

[Richard Cochran]

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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 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(+)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 74fc9af4aadb..c6295e5c16af 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -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;
 	}
diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
index 4fe104b2411f..f89b5a836c2a 100644
--- a/include/uapi/linux/net_tstamp.h
+++ b/include/uapi/linux/net_tstamp.h
@@ -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 */
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 0ab1af04296c..cdda085e4b47 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -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

[PATCH net-next RFC V1 2/5] net: phy: Move time stamping interface into the generic mdio layer.

[Richard Cochran]

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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 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(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..79aeb5eb471a 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -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)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c2d9027be863..466bf88053ce 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 2cfffe586885..70a82c973aed 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -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;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a9b1753fdc5..77d3f703b8ce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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. */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 157cd9efa4be..b4b81eaa15a9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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 {
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 42689d5c468c..b8857028e652 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -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

[PATCH net-next RFC V1 3/5] net: Introduce field for the MII time stamper.

[Richard Cochran]

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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/phy/mdio_bus.c | 51 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/netdevice.h  |  1 +
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 24b5511222c8..fdac8c8ac272 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -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);
 }
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5fbb9f1da7fd..223d691aa0b0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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

[PATCH net-next RFC V1 4/5] net: Use the generic MII time stamper when available.

[Richard Cochran]

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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 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(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 466bf88053ce..df80c6b14478 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
diff --git a/net/Kconfig b/net/Kconfig
index 0428f12c25c2..e38403cd010c 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -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.
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b4b81eaa15a9..507e56abecb7 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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 {
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index b8857028e652..ecb0ecb03740 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -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

[PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

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.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 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

diff --git a/Documentation/devicetree/bindings/net/ines-ptp.txt b/Documentation/devicetree/bindings/net/ines-ptp.txt
new file mode 100644
index 000000000000..ed7b1d773ded
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ines-ptp.txt
@@ -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>;
+			};
+		};
+	};
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 01acbcb2c798..e286bb822295 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -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
diff --git a/drivers/net/phy/ines_ptp.c b/drivers/net/phy/ines_ptp.c
new file mode 100644
index 000000000000..4f66459d4417
--- /dev/null
+++ b/drivers/net/phy/ines_ptp.c
@@ -0,0 +1,857 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 MOSER-BAER AG
+ */
+#define pr_fmt(fmt) "InES_PTP: " fmt
+
+#include <linux/ethtool.h>
+#include <linux/export.h>
+#include <linux/if_vlan.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/net_tstamp.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/ptp_classify.h>
+#include <linux/ptp_clock_kernel.h>
+#include <linux/stddef.h>
+
+MODULE_DESCRIPTION("Driver for the ZHAW InES PTP time stamping IP core");
+MODULE_AUTHOR("Richard Cochran <richardcochran@gmail.com>");
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL");
+
+/* GLOBAL register */
+#define MCAST_MAC_SELECT_SHIFT	2
+#define MCAST_MAC_SELECT_MASK	0x3
+#define IO_RESET		BIT(1)
+#define PTP_RESET		BIT(0)
+
+/* VERSION register */
+#define IF_MAJOR_VER_SHIFT	12
+#define IF_MAJOR_VER_MASK	0xf
+#define IF_MINOR_VER_SHIFT	8
+#define IF_MINOR_VER_MASK	0xf
+#define FPGA_MAJOR_VER_SHIFT	4
+#define FPGA_MAJOR_VER_MASK	0xf
+#define FPGA_MINOR_VER_SHIFT	0
+#define FPGA_MINOR_VER_MASK	0xf
+
+/* INT_STAT register */
+#define RX_INTR_STATUS_3	BIT(5)
+#define RX_INTR_STATUS_2	BIT(4)
+#define RX_INTR_STATUS_1	BIT(3)
+#define TX_INTR_STATUS_3	BIT(2)
+#define TX_INTR_STATUS_2	BIT(1)
+#define TX_INTR_STATUS_1	BIT(0)
+
+/* INT_MSK register */
+#define RX_INTR_MASK_3		BIT(5)
+#define RX_INTR_MASK_2		BIT(4)
+#define RX_INTR_MASK_1		BIT(3)
+#define TX_INTR_MASK_3		BIT(2)
+#define TX_INTR_MASK_2		BIT(1)
+#define TX_INTR_MASK_1		BIT(0)
+
+/* BUF_STAT register */
+#define RX_FIFO_NE_3		BIT(5)
+#define RX_FIFO_NE_2		BIT(4)
+#define RX_FIFO_NE_1		BIT(3)
+#define TX_FIFO_NE_3		BIT(2)
+#define TX_FIFO_NE_2		BIT(1)
+#define TX_FIFO_NE_1		BIT(0)
+
+/* PORT_CONF register */
+#define CM_ONE_STEP		BIT(6)
+#define PHY_SPEED_SHIFT		4
+#define PHY_SPEED_MASK		0x3
+#define P2P_DELAY_WR_POS_SHIFT	2
+#define P2P_DELAY_WR_POS_MASK	0x3
+#define PTP_MODE_SHIFT		0
+#define PTP_MODE_MASK		0x3
+
+/* TS_STAT_TX register */
+#define TS_ENABLE		BIT(15)
+#define DATA_READ_POS_SHIFT	8
+#define DATA_READ_POS_MASK	0x1f
+#define DISCARDED_EVENTS_SHIFT	4
+#define DISCARDED_EVENTS_MASK	0xf
+
+#define INES_N_PORTS		3
+#define INES_REGISTER_SIZE	0x80
+#define INES_PORT_OFFSET	0x20
+#define INES_PORT_SIZE		0x20
+#define INES_FIFO_DEPTH		90
+#define INES_MAX_EVENTS		100
+
+#define BC_PTP_V1		0
+#define BC_PTP_V2		1
+#define TC_E2E_PTP_V2		2
+#define TC_P2P_PTP_V2		3
+
+#define OFF_PTP_CLOCK_ID	20
+#define OFF_PTP_PORT_NUM	28
+
+#define PHY_SPEED_10		0
+#define PHY_SPEED_100		1
+#define PHY_SPEED_1000		2
+
+#define PORT_CONF \
+	((PHY_SPEED_1000 << PHY_SPEED_SHIFT) | (BC_PTP_V2 << PTP_MODE_SHIFT))
+
+#define ines_read32(s, r)	__raw_readl(&s->regs->r)
+#define ines_write32(s, v, r)	__raw_writel(v, &s->regs->r)
+
+#define MESSAGE_TYPE_SYNC		1
+#define MESSAGE_TYPE_P_DELAY_REQ	2
+#define MESSAGE_TYPE_P_DELAY_RESP	3
+#define MESSAGE_TYPE_DELAY_REQ		4
+
+#define SYNC				0x0
+#define DELAY_REQ			0x1
+#define PDELAY_REQ			0x2
+#define PDELAY_RESP			0x3
+
+static LIST_HEAD(ines_clocks);
+static DEFINE_MUTEX(ines_clocks_lock);
+
+struct ines_global_registers {
+	u32 id;
+	u32 test;
+	u32 global;
+	u32 version;
+	u32 test2;
+	u32 int_stat;
+	u32 int_msk;
+	u32 buf_stat;
+};
+
+struct ines_port_registers {
+	u32 port_conf;
+	u32 p_delay;
+	u32 ts_stat_tx;
+	u32 ts_stat_rx;
+	u32 ts_tx;
+	u32 ts_rx;
+};
+
+struct ines_timestamp {
+	struct list_head list;
+	unsigned long	tmo;
+	u16		tag;
+	u64		sec;
+	u64		nsec;
+	u64		clkid;
+	u16		portnum;
+	u16		seqid;
+};
+
+struct ines_port {
+	struct ines_port_registers	*regs;
+	struct ines_clock		*clock;
+	bool				rxts_enabled;
+	bool				txts_enabled;
+	unsigned int			index;
+	struct delayed_work		ts_work;
+	/* lock protects event list and tx_skb */
+	spinlock_t			lock;
+	struct sk_buff			*tx_skb;
+	struct list_head		events;
+	struct list_head		pool;
+	struct ines_timestamp		pool_data[INES_MAX_EVENTS];
+};
+
+struct ines_clock {
+	struct ines_port		port[INES_N_PORTS];
+	struct ines_global_registers	*regs;
+	void __iomem			*base;
+	struct device_node		*node;
+	struct list_head		list;
+};
+
+static bool ines_match(struct sk_buff *skb, unsigned int ptp_class,
+		       struct ines_timestamp *ts);
+static int ines_rxfifo_read(struct ines_port *port);
+static u64 ines_rxts64(struct ines_port *port, unsigned int words);
+static bool ines_timestamp_expired(struct ines_timestamp *ts);
+static u64 ines_txts64(struct ines_port *port, unsigned int words);
+static void ines_txtstamp_work(struct work_struct *work);
+static bool is_sync_pdelay_resp(struct sk_buff *skb, int type);
+static u8 tag_to_msgtype(u8 tag);
+
+static void ines_clock_cleanup(struct ines_clock *clock)
+{
+	struct ines_port *port;
+	int i;
+
+	for (i = 0; i < INES_N_PORTS; i++) {
+		port = &clock->port[i];
+		cancel_delayed_work_sync(&port->ts_work);
+	}
+}
+
+static int ines_clock_init(struct ines_clock *clock, struct device_node *node,
+			   void __iomem *addr)
+{
+	unsigned long port_addr;
+	struct ines_port *port;
+	int i, j;
+
+	INIT_LIST_HEAD(&clock->list);
+	clock->node = node;
+	clock->base = addr;
+	clock->regs = clock->base;
+
+	for (i = 0; i < INES_N_PORTS; i++) {
+		port = &clock->port[i];
+		port_addr = (unsigned long) clock->base +
+			INES_PORT_OFFSET + i * INES_PORT_SIZE;
+		port->regs = (struct ines_port_registers *) port_addr;
+		port->clock = clock;
+		port->index = i;
+		INIT_DELAYED_WORK(&port->ts_work, ines_txtstamp_work);
+		spin_lock_init(&port->lock);
+		INIT_LIST_HEAD(&port->events);
+		INIT_LIST_HEAD(&port->pool);
+		for (j = 0; j < INES_MAX_EVENTS; j++)
+			list_add(&port->pool_data[j].list, &port->pool);
+	}
+
+	ines_write32(clock, 0xBEEF, test);
+	ines_write32(clock, 0xBEEF, test2);
+
+	pr_debug("ID      0x%x\n", ines_read32(clock, id));
+	pr_debug("TEST    0x%x\n", ines_read32(clock, test));
+	pr_debug("VERSION 0x%x\n", ines_read32(clock, version));
+	pr_debug("TEST2   0x%x\n", ines_read32(clock, test2));
+
+	for (i = 0; i < INES_N_PORTS; i++) {
+		port = &clock->port[i];
+		ines_write32(port, PORT_CONF, port_conf);
+	}
+
+	return 0;
+}
+
+static void ines_dump_ts(char *label, struct ines_timestamp *ts)
+{
+#ifdef DEBUG
+	pr_err("%s timestamp, tag=0x%04hx t=%llu.%9llu c=0x%llx p=%hu s=%hu\n",
+	       label, ts->tag, ts->sec, ts->nsec,
+	       ts->clkid, ts->portnum, ts->seqid);
+#endif
+}
+
+static struct ines_port *ines_find_port(struct device_node *node, u32 index)
+{
+	struct ines_port *port = NULL;
+	struct ines_clock *clock;
+	struct list_head *this;
+
+	if (index > INES_N_PORTS - 1)
+		return NULL;
+
+	mutex_lock(&ines_clocks_lock);
+	list_for_each(this, &ines_clocks) {
+		clock = list_entry(this, struct ines_clock, list);
+		if (clock->node == node) {
+			port = &clock->port[index];
+			break;
+		}
+	}
+	mutex_unlock(&ines_clocks_lock);
+	return port;
+}
+
+static u64 ines_find_rxts(struct ines_port *port, struct sk_buff *skb, int type)
+{
+	struct list_head *this, *next;
+	struct ines_timestamp *ts;
+	unsigned long flags;
+	u64 ns = 0;
+
+	if (type == PTP_CLASS_NONE)
+		return 0;
+
+	spin_lock_irqsave(&port->lock, flags);
+	ines_rxfifo_read(port);
+	list_for_each_safe(this, next, &port->events) {
+		ts = list_entry(this, struct ines_timestamp, list);
+		if (ines_timestamp_expired(ts)) {
+			list_del_init(&ts->list);
+			list_add(&ts->list, &port->pool);
+			continue;
+		}
+		if (ines_match(skb, type, ts)) {
+			ns = ts->sec * 1000000000ULL + ts->nsec;
+			list_del_init(&ts->list);
+			list_add(&ts->list, &port->pool);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return ns;
+}
+
+static u64 ines_find_txts(struct ines_port *port, struct sk_buff *skb)
+{
+	unsigned int class = ptp_classify_raw(skb), i;
+	u32 data_rd_pos, buf_stat, mask, ts_stat_tx;
+	struct ines_timestamp ts;
+	unsigned long flags;
+	u64 ns = 0;
+
+	mask = TX_FIFO_NE_1 << port->index;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	for (i = 0; i < INES_FIFO_DEPTH; i++) {
+
+		buf_stat = ines_read32(port->clock, buf_stat);
+		if (!(buf_stat & mask)) {
+			pr_debug("Tx timestamp FIFO unexpectedly empty\n");
+			break;
+		}
+		ts_stat_tx = ines_read32(port, ts_stat_tx);
+		data_rd_pos = (ts_stat_tx >> DATA_READ_POS_SHIFT) &
+			DATA_READ_POS_MASK;
+		if (data_rd_pos) {
+			pr_err("unexpected Tx read pos %u\n", data_rd_pos);
+			break;
+		}
+
+		ts.tag     = ines_read32(port, ts_tx);
+		ts.sec     = ines_txts64(port, 3);
+		ts.nsec    = ines_txts64(port, 2);
+		ts.clkid   = ines_txts64(port, 4);
+		ts.portnum = ines_read32(port, ts_tx);
+		ts.seqid   = ines_read32(port, ts_tx);
+
+		ines_dump_ts("Tx", &ts);
+
+		if (ines_match(skb, class, &ts)) {
+			ns = ts.sec * 1000000000ULL + ts.nsec;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&port->lock, flags);
+	return ns;
+}
+
+static int ines_hwtstamp(struct mdio_device *m, struct ifreq *ifr)
+{
+	u32 cm_one_step = 0, port_conf, ts_stat_rx, ts_stat_tx;
+	struct ines_port *port = dev_get_drvdata(&m->dev);
+	struct hwtstamp_config cfg;
+	unsigned long flags;
+
+	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+		return -EFAULT;
+
+	/* reserved for future extensions */
+	if (cfg.flags)
+		return -EINVAL;
+
+	switch (cfg.tx_type) {
+	case HWTSTAMP_TX_OFF:
+		ts_stat_tx = 0;
+		break;
+	case HWTSTAMP_TX_ON:
+		ts_stat_tx = TS_ENABLE;
+		break;
+	case HWTSTAMP_TX_ONESTEP_P2P:
+		ts_stat_tx = TS_ENABLE;
+		cm_one_step = CM_ONE_STEP;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (cfg.rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		ts_stat_rx = 0;
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+		return -ERANGE;
+	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:
+		ts_stat_rx = TS_ENABLE;
+		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	port_conf = ines_read32(port, port_conf);
+	port_conf &= ~CM_ONE_STEP;
+	port_conf |= cm_one_step;
+
+	ines_write32(port, port_conf, port_conf);
+	ines_write32(port, ts_stat_rx, ts_stat_rx);
+	ines_write32(port, ts_stat_tx, ts_stat_tx);
+
+	port->rxts_enabled = ts_stat_rx == TS_ENABLE ? true : false;
+	port->txts_enabled = ts_stat_tx == TS_ENABLE ? true : false;
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+}
+
+static bool ines_match(struct sk_buff *skb, unsigned int ptp_class,
+		       struct ines_timestamp *ts)
+{
+	u8 *msgtype, *data = skb_mac_header(skb);
+	unsigned int offset = 0;
+	u16 *portn, *seqid;
+	u64 *clkid;
+
+	if (unlikely(ptp_class & PTP_CLASS_V1))
+		return false;
+
+	if (ptp_class & PTP_CLASS_VLAN)
+		offset += VLAN_HLEN;
+
+	switch (ptp_class & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
+		break;
+	case PTP_CLASS_IPV6:
+		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
+		break;
+	case PTP_CLASS_L2:
+		offset += ETH_HLEN;
+		break;
+	default:
+		return false;
+	}
+
+	if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
+		return false;
+
+	msgtype = data + offset;
+	clkid = (u64 *)(data + offset + OFF_PTP_CLOCK_ID);
+	portn = (u16 *)(data + offset + OFF_PTP_PORT_NUM);
+	seqid = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID);
+
+	if (tag_to_msgtype(ts->tag & 0x7) != (*msgtype & 0xf)) {
+		pr_debug("msgtype mismatch ts %hhu != skb %hhu\n",
+			 tag_to_msgtype(ts->tag & 0x7), *msgtype & 0xf);
+		return false;
+	}
+	if (cpu_to_be64(ts->clkid) != *clkid) {
+		pr_debug("clkid mismatch ts %llx != skb %llx\n",
+			 cpu_to_be64(ts->clkid), *clkid);
+		return false;
+	}
+	if (ts->portnum != ntohs(*portn)) {
+		pr_debug("portn mismatch ts %hu != skb %hu\n",
+			 ts->portnum, ntohs(*portn));
+		return false;
+	}
+	if (ts->seqid != ntohs(*seqid)) {
+		pr_debug("seqid mismatch ts %hu != skb %hu\n",
+			 ts->seqid, ntohs(*seqid));
+		return false;
+	}
+
+	return true;
+}
+
+static bool ines_rxtstamp(struct mdio_device *m, struct sk_buff *skb, int type)
+{
+	struct ines_port *port = dev_get_drvdata(&m->dev);
+	struct skb_shared_hwtstamps *ssh;
+	u64 ns;
+
+	if (!port->rxts_enabled)
+		return false;
+
+	ns = ines_find_rxts(port, skb, type);
+	if (!ns)
+		return false;
+
+	ssh = skb_hwtstamps(skb);
+	ssh->hwtstamp = ns_to_ktime(ns);
+	netif_rx(skb);
+
+	return true;
+}
+
+static int ines_rxfifo_read(struct ines_port *port)
+{
+	u32 data_rd_pos, buf_stat, mask, ts_stat_rx;
+	struct ines_timestamp *ts;
+	unsigned int i;
+
+	mask = RX_FIFO_NE_1 << port->index;
+
+	for (i = 0; i < INES_FIFO_DEPTH; i++) {
+		if (list_empty(&port->pool)) {
+			pr_err("event pool is empty\n");
+			return -1;
+		}
+		buf_stat = ines_read32(port->clock, buf_stat);
+		if (!(buf_stat & mask))
+			break;
+
+		ts_stat_rx = ines_read32(port, ts_stat_rx);
+		data_rd_pos = (ts_stat_rx >> DATA_READ_POS_SHIFT) &
+			DATA_READ_POS_MASK;
+		if (data_rd_pos) {
+			pr_err("unexpected Rx read pos %u\n", data_rd_pos);
+			break;
+		}
+
+		ts = list_first_entry(&port->pool, struct ines_timestamp, list);
+		ts->tmo     = jiffies + HZ;
+		ts->tag     = ines_read32(port, ts_rx);
+		ts->sec     = ines_rxts64(port, 3);
+		ts->nsec    = ines_rxts64(port, 2);
+		ts->clkid   = ines_rxts64(port, 4);
+		ts->portnum = ines_read32(port, ts_rx);
+		ts->seqid   = ines_read32(port, ts_rx);
+
+		ines_dump_ts("Rx", ts);
+
+		list_del_init(&ts->list);
+		list_add_tail(&ts->list, &port->events);
+	}
+
+	return 0;
+}
+
+static u64 ines_rxts64(struct ines_port *port, unsigned int words)
+{
+	unsigned int i;
+	u64 result;
+	u16 word;
+
+	word = ines_read32(port, ts_rx);
+	result = word;
+	words--;
+	for (i = 0; i < words; i++) {
+		word = ines_read32(port, ts_rx);
+		result <<= 16;
+		result |= word;
+	}
+	return result;
+}
+
+static bool ines_timestamp_expired(struct ines_timestamp *ts)
+{
+	return time_after(jiffies, ts->tmo);
+}
+
+static int ines_ts_info(struct mdio_device *m, struct ethtool_ts_info *info)
+{
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_TX_SOFTWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RX_SOFTWARE |
+		SOF_TIMESTAMPING_SOFTWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	info->phc_index = -1;
+
+	info->tx_types =
+		(1 << HWTSTAMP_TX_OFF) |
+		(1 << HWTSTAMP_TX_ON) |
+		(1 << HWTSTAMP_TX_ONESTEP_P2P);
+
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+
+	return 0;
+}
+
+static u64 ines_txts64(struct ines_port *port, unsigned int words)
+{
+	unsigned int i;
+	u64 result;
+	u16 word;
+
+	word = ines_read32(port, ts_tx);
+	result = word;
+	words--;
+	for (i = 0; i < words; i++) {
+		word = ines_read32(port, ts_tx);
+		result <<= 16;
+		result |= word;
+	}
+	return result;
+}
+
+static bool ines_txts_onestep(struct ines_port *port, struct sk_buff *skb, int type)
+{
+	unsigned long flags;
+	u32 port_conf;
+
+	spin_lock_irqsave(&port->lock, flags);
+	port_conf = ines_read32(port, port_conf);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	if (port_conf & CM_ONE_STEP)
+		return is_sync_pdelay_resp(skb, type);
+
+	return false;
+}
+
+static void ines_txtstamp(struct mdio_device *m, struct sk_buff *skb, int type)
+{
+	struct ines_port *port = dev_get_drvdata(&m->dev);
+	struct sk_buff *old_skb;
+	unsigned long flags;
+
+	if (!port->txts_enabled || ines_txts_onestep(port, skb, type)) {
+		kfree_skb(skb);
+		return;
+	}
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (port->tx_skb)
+		old_skb = port->tx_skb;
+
+	port->tx_skb = skb;
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	if (old_skb)
+		kfree_skb(old_skb);
+
+	schedule_delayed_work(&port->ts_work, 1);
+}
+
+static void ines_txtstamp_work(struct work_struct *work)
+{
+	struct ines_port *port =
+		container_of(work, struct ines_port, ts_work.work);
+	struct skb_shared_hwtstamps ssh;
+	struct sk_buff *skb;
+	unsigned long flags;
+	u64 ns;
+
+	spin_lock_irqsave(&port->lock, flags);
+	skb = port->tx_skb;
+	port->tx_skb = NULL;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	ns = ines_find_txts(port, skb);
+	if (!ns) {
+		kfree_skb(skb);
+		return;
+	}
+	ssh.hwtstamp = ns_to_ktime(ns);
+	skb_complete_tx_timestamp(skb, &ssh);
+}
+
+static bool is_sync_pdelay_resp(struct sk_buff *skb, int type)
+{
+	u8 *data = skb->data, *msgtype;
+	unsigned int offset = 0;
+
+	if (type & PTP_CLASS_VLAN)
+		offset += VLAN_HLEN;
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
+		break;
+	case PTP_CLASS_IPV6:
+		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
+		break;
+	case PTP_CLASS_L2:
+		offset += ETH_HLEN;
+		break;
+	default:
+		return 0;
+	}
+
+	if (type & PTP_CLASS_V1)
+		offset += OFF_PTP_CONTROL;
+
+	if (skb->len < offset + 1)
+		return 0;
+
+	msgtype = data + offset;
+
+	switch ((*msgtype & 0xf)) {
+	case SYNC:
+	case PDELAY_RESP:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static u8 tag_to_msgtype(u8 tag)
+{
+	switch (tag) {
+	case MESSAGE_TYPE_SYNC:
+		return SYNC;
+	case MESSAGE_TYPE_P_DELAY_REQ:
+		return PDELAY_REQ;
+	case MESSAGE_TYPE_P_DELAY_RESP:
+		return PDELAY_RESP;
+	case MESSAGE_TYPE_DELAY_REQ:
+		return DELAY_REQ;
+	}
+	return 0xf;
+}
+
+static int ines_ptp_ctrl_probe(struct platform_device *pld)
+{
+	struct ines_clock *clock;
+	struct resource *res;
+	void __iomem *addr;
+	int err = 0;
+
+	res = platform_get_resource(pld, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pld->dev, "missing memory resource\n");
+		return -EINVAL;
+	}
+	addr = devm_ioremap_resource(&pld->dev, res);
+	if (IS_ERR(addr)) {
+		err = PTR_ERR(addr);
+		goto out;
+	}
+	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
+	if (!clock) {
+		err = -ENOMEM;
+		goto out;
+	}
+	if (ines_clock_init(clock, pld->dev.of_node, addr)) {
+		kfree(clock);
+		err = -ENOMEM;
+		goto out;
+	}
+	mutex_lock(&ines_clocks_lock);
+	list_add_tail(&ines_clocks, &clock->list);
+	mutex_unlock(&ines_clocks_lock);
+
+	dev_set_drvdata(&pld->dev, clock);
+out:
+	return err;
+}
+
+static int ines_ptp_ctrl_remove(struct platform_device *pld)
+{
+	struct ines_clock *clock = dev_get_drvdata(&pld->dev);
+
+	mutex_lock(&ines_clocks_lock);
+	list_del(&clock->list);
+	mutex_unlock(&ines_clocks_lock);
+	ines_clock_cleanup(clock);
+	kfree(clock);
+	return 0;
+}
+
+static int ines_ptp_port_probe(struct mdio_device *mdiodev)
+{
+	struct device_node *node;
+	struct ines_port *port;
+	int err = 0;
+	u32 index;
+
+	if (of_property_read_u32(mdiodev->dev.of_node, "port-index", &index)) {
+		dev_err(&mdiodev->dev, "missing port-index\n");
+		return -EINVAL;
+	}
+	node = of_parse_phandle(mdiodev->dev.of_node, "ctrl-handle", 0);
+	if (IS_ERR(node)) {
+		dev_err(&mdiodev->dev, "missing ctrl-handle\n");
+		return PTR_ERR(node);
+	}
+	port = ines_find_port(node, index);
+	if (!port) {
+		dev_err(&mdiodev->dev, "missing port\n");
+		err = -ENODEV;
+		goto out;
+	}
+	mdiodev->ts_info  = ines_ts_info;
+	mdiodev->hwtstamp = ines_hwtstamp;
+	mdiodev->rxtstamp = ines_rxtstamp;
+	mdiodev->txtstamp = ines_txtstamp;
+	dev_set_drvdata(&mdiodev->dev, port);
+out:
+	of_node_put(node);
+	return err;
+}
+
+static void ines_ptp_port_remove(struct mdio_device *mdiodev)
+{
+}
+
+static const struct of_device_id ines_ptp_ctrl_of_match[] = {
+	{ .compatible = "ines,ptp-ctrl" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, ines_ptp_ctrl_of_match);
+
+static struct platform_driver ines_ptp_ctrl_driver = {
+	.probe  = ines_ptp_ctrl_probe,
+	.remove = ines_ptp_ctrl_remove,
+	.driver = {
+		.name = "ines_ptp_ctrl",
+		.of_match_table = of_match_ptr(ines_ptp_ctrl_of_match),
+	},
+};
+
+static const struct of_device_id ines_ptp_port_of_match[] = {
+	{ .compatible = "ines,ptp-port" },
+	{ }
+};
+
+static struct mdio_driver ines_ptp_port_driver = {
+	.probe	= ines_ptp_port_probe,
+	.remove = ines_ptp_port_remove,
+	.mdiodrv.driver = {
+		.name = "ines_ptp_port",
+		.of_match_table = ines_ptp_port_of_match,
+	},
+};
+
+static int __init ines_ptp_init(void)
+{
+	int err;
+
+	err = platform_driver_register(&ines_ptp_ctrl_driver);
+	if (err)
+		return err;
+
+	err = mdio_driver_register(&ines_ptp_port_driver);
+	if (err)
+		platform_driver_unregister(&ines_ptp_ctrl_driver);
+
+	return err;
+}
+
+static void __exit ines_ptp_cleanup(void)
+{
+	mdio_driver_unregister(&ines_ptp_port_driver);
+	platform_driver_unregister(&ines_ptp_ctrl_driver);
+}
+
+module_init(ines_ptp_init);
+module_exit(ines_ptp_cleanup);
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index a21ad10d613c..10ef161dd2a1 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -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

Re: [PATCH net-next RFC V1 2/5] net: phy: Move time stamping interface into the generic mdio layer.

[Florian Fainelli]

On 03/21/2018 11:58 AM, Richard Cochran wrote:
> 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.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  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(-)
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index 654f42d00092..79aeb5eb471a 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -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;

Why is this implemented a the mdio_device level and not at the
mdio_driver level? This looks like the wrong level at which this is done.
--
Florian

Re: [PATCH net-next RFC V1 3/5] net: Introduce field for the MII time stamper.

[Florian Fainelli]

On 03/21/2018 11:58 AM, Richard Cochran wrote:
> 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.
> 
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>  drivers/net/phy/mdio_bus.c | 51 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/netdevice.h  |  1 +
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 24b5511222c8..fdac8c8ac272 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -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;

You are still assuming that we have a phy_device somehow, whereas you
parch series wants to solve that for generic MDIO devices, that is a bit
confusing.

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

What guarantees that netdev->mdiots gets cleared? Also, why is this done
with a notifier instead of through phy_{connect,attach,disconnect}? It
looks like we still have this requirement of the mdio TS device being a
phy_device somehow, I am confused here...

> +		}
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
>  #ifdef CONFIG_PM
>  static int mdio_bus_suspend(struct device *dev)
>  {

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5fbb9f1da7fd..223d691aa0b0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1943,6 +1943,7 @@ struct net_device {
>  	struct netprio_map __rcu *priomap;
>  #endif
>  	struct phy_device	*phydev;
> +	struct mdio_device	*mdiots;

phy_device embedds a mdio_device, can you find a way to rework the PHY
PTP code to utilize the phy_device's mdio instance so do not introduce
yet another pointer in that big structure that net_device already is?

>  	struct lock_class_key	*qdisc_tx_busylock;
>  	struct lock_class_key	*qdisc_running_key;
>  	bool			proto_down;
> 


-- 
Florian

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Wed, Mar 21, 2018 at 11:58:18AM -0700, Richard Cochran wrote:
> 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.

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

RE: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP time stamping.

[Keller, Jacob E]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Richard Cochran
> Sent: Wednesday, March 21, 2018 11:58 AM
> To: netdev@vger.kernel.org
> Cc: devicetree@vger.kernel.org; Andrew Lunn <andrew@lunn.ch>; David Miller
> <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>; Mark Rutland
> <mark.rutland@arm.com>; Miroslav Lichvar <mlichvar@redhat.com>; Rob
> Herring <robh+dt@kernel.org>; Willem de Bruijn <willemb@google.com>
> Subject: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP
> +
> +	/*
> +	 * 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,
>  };
> 

I am guessing that we expect all devices which support onestep P2P messages, will always support onestep SYNC as well?

Thanks,
Jake

Re: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP time stamping.

[Richard Cochran]

On Wed, Mar 21, 2018 at 08:05:36PM +0000, Keller, Jacob E wrote:
> I am guessing that we expect all devices which support onestep P2P messages, will always support onestep SYNC as well?

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Wed, Mar 21, 2018 at 08:33:15PM +0100, Andrew Lunn wrote:
> Can you point us at some documentation for this.

The overall one-step functionality is described IEEE 1588.

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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

Hi Richard

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

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

Re: [PATCH net-next RFC V1 2/5] net: phy: Move time stamping interface into the generic mdio layer.

[Richard Cochran]

On Wed, Mar 21, 2018 at 12:10:07PM -0700, Florian Fainelli wrote:
> > +	phydev->mdio.ts_info = dp83640_ts_info;
> > +	phydev->mdio.hwtstamp = dp83640_hwtstamp;
> > +	phydev->mdio.rxtstamp = dp83640_rxtstamp;
> > +	phydev->mdio.txtstamp = dp83640_txtstamp;
> 
> Why is this implemented a the mdio_device level and not at the
> mdio_driver level? This looks like the wrong level at which this is done.

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

Re: [PATCH net-next RFC V1 3/5] net: Introduce field for the MII time stamper.

[Richard Cochran]

On Wed, Mar 21, 2018 at 12:12:00PM -0700, Florian Fainelli wrote:
> > +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;
> 
> You are still assuming that we have a phy_device somehow, whereas you
> parch series wants to solve that for generic MDIO devices, that is a bit
> confusing.

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

> > +
> > +	/*
> > +	 * 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;
> 
> What guarantees that netdev->mdiots gets cleared?

Why would it need to be cleared?

> Also, why is this done
> with a notifier instead of through phy_{connect,attach,disconnect}?

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

> It
> looks like we still have this requirement of the mdio TS device being a
> phy_device somehow, I am confused here...

We only need the phydev to get from the netdev to the mii bus.
 
> > +		}
> > +	}
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> >  #ifdef CONFIG_PM
> >  static int mdio_bus_suspend(struct device *dev)
> >  {
> 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 5fbb9f1da7fd..223d691aa0b0 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1943,6 +1943,7 @@ struct net_device {
> >  	struct netprio_map __rcu *priomap;
> >  #endif
> >  	struct phy_device	*phydev;
> > +	struct mdio_device	*mdiots;
> 
> phy_device embedds a mdio_device, can you find a way to rework the PHY
> PTP code to utilize the phy_device's mdio instance so do not introduce
> yet another pointer in that big structure that net_device already is?

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Wed, Mar 21, 2018 at 10:44:36PM +0100, Andrew Lunn wrote:
> 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.

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 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?

It acts like a "snooper" to provide out of band time stamps, but it
also can modify packets when for the one-step functionality.
 
> When the driver talks about having three ports, does that mean it can
> be on three different MII busses?

Yes.

HTH,
Richard

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Wed, Mar 21, 2018 at 02:57:29PM -0700, Richard Cochran wrote:
> On Wed, Mar 21, 2018 at 10:44:36PM +0100, Andrew Lunn wrote:
> > 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.
> 
> Right.

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

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

So i suggest we leave the design for those until we actual see one.
  
> > 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?
> 
> It acts like a "snooper" to provide out of band time stamps, but it
> also can modify packets when for the one-step functionality.
>  
> > When the driver talks about having three ports, does that mean it can
> > be on three different MII busses?

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Wed, Mar 21, 2018 at 11:16:52PM +0100, Andrew Lunn wrote:
> 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.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Wed, Mar 21, 2018 at 03:47:02PM -0700, Richard Cochran wrote:
> On Wed, Mar 21, 2018 at 11:16:52PM +0100, Andrew Lunn wrote:
> > 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.
> 
> 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.

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

RE: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP time stamping.

[Keller, Jacob E]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Wednesday, March 21, 2018 2:26 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; Andrew Lunn
> <andrew@lunn.ch>; David Miller <davem@davemloft.net>; Florian Fainelli
> <f.fainelli@gmail.com>; Mark Rutland <mark.rutland@arm.com>; Miroslav
> Lichvar <mlichvar@redhat.com>; Rob Herring <robh+dt@kernel.org>; Willem de
> Bruijn <willemb@google.com>
> Subject: Re: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step
> PTP time stamping.
> 
> On Wed, Mar 21, 2018 at 08:05:36PM +0000, Keller, Jacob E wrote:
> > I am guessing that we expect all devices which support onestep P2P messages,
> will always support onestep SYNC as well?
> 
> 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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Wed, Mar 21, 2018 at 03:47:02PM -0700, Richard Cochran wrote:
> On Wed, Mar 21, 2018 at 11:16:52PM +0100, Andrew Lunn wrote:
> > 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.
> 
> 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.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Thu, Mar 22, 2018 at 01:43:49AM +0100, Andrew Lunn wrote:
> On Wed, Mar 21, 2018 at 03:47:02PM -0700, Richard Cochran wrote:
> > I happy to improve the modeling, but the solution should be generic
> > and work for every MAC driver.

Let me qualify that a bit...
 
> 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.

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

Re: [PATCH net-next RFC V1 2/5] net: phy: Move time stamping interface into the generic mdio layer.

[Richard Cochran]

On Wed, Mar 21, 2018 at 02:45:13PM -0700, Richard Cochran wrote:
> On Wed, Mar 21, 2018 at 12:10:07PM -0700, Florian Fainelli wrote:
> > > +	phydev->mdio.ts_info = dp83640_ts_info;
> > > +	phydev->mdio.hwtstamp = dp83640_hwtstamp;
> > > +	phydev->mdio.rxtstamp = dp83640_rxtstamp;
> > > +	phydev->mdio.txtstamp = dp83640_txtstamp;
> > 
> > Why is this implemented a the mdio_device level and not at the
> > mdio_driver level? This looks like the wrong level at which this is done.
> 
> 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?

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

Re: [PATCH net-next RFC V1 3/5] net: Introduce field for the MII time stamper.

[Richard Cochran]

On Wed, Mar 21, 2018 at 12:12:00PM -0700, Florian Fainelli wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 5fbb9f1da7fd..223d691aa0b0 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1943,6 +1943,7 @@ struct net_device {
> >  	struct netprio_map __rcu *priomap;
> >  #endif
> >  	struct phy_device	*phydev;
> > +	struct mdio_device	*mdiots;
> 
> phy_device embedds a mdio_device, can you find a way to rework the PHY
> PTP code to utilize the phy_device's mdio instance so do not introduce
> yet another pointer in that big structure that net_device already is?

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Thu, Mar 22, 2018 at 12:50:07AM +0100, Andrew Lunn wrote:
> 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?

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.

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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Sat, Mar 24, 2018 at 10:12:19AM -0700, Richard Cochran wrote:
> On Thu, Mar 22, 2018 at 12:50:07AM +0100, Andrew Lunn wrote:
> > 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?
> 
> 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.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Sat, Mar 24, 2018 at 07:48:58PM +0100, Andrew Lunn wrote:
> 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

Yes, yes.

> 3) How do you limit the MAC/PHY to what the PTP device can do.

Hm, I don't think this is important.
 
> 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.

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

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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

On Sat, Mar 24, 2018 at 09:51:52PM -0700, Richard Cochran wrote:
> On Sat, Mar 24, 2018 at 07:48:58PM +0100, Andrew Lunn wrote:
> > 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
> 
> Yes, yes.
> 
> > 3) How do you limit the MAC/PHY to what the PTP device can do.
> 
> Hm, I don't think this is important.

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.

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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote:
> > > 3) How do you limit the MAC/PHY to what the PTP device can do.
> > 
> > Hm, I don't think this is important.
> 
> 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?

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.

> > Right, so phylib can operate on phydev->attached_dev->mdiots;
> 
> So first off, it is not an MDIO 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.

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.

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

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

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

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote:
> To keep lifecycle issues simple, i would also keep it in phydev, not
> netdev.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Florian Fainelli]

On 03/25/2018 03:10 PM, Richard Cochran wrote:
> On Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote:
>>>> 3) How do you limit the MAC/PHY to what the PTP device can do.
>>>
>>> Hm, I don't think this is important.
>>
>> 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?
> 
> 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.
> 
>>> Right, so phylib can operate on phydev->attached_dev->mdiots;
>>
>> So first off, it is not an MDIO 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.
> 
> 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.

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

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

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.

> 
>> 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.
> 
> Well, this name is not ideal, since time stamping devices in general
> can time stamp any frame, not just PTP frames.
> 
>> 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.
> 
> 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?

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

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

-- 
Florian

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

> > 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.
> 
> Are you sure the locking is broken?  If so, how?

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

> > 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.
> 
> Well, this name is not ideal, since time stamping devices in general
> can time stamp any frame, not just PTP frames.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Sun, Mar 25, 2018 at 04:01:49PM -0700, Florian Fainelli wrote:
> 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...

That won't work at all, AFAICT.  There is only one mii_bus per netdev,
that is one that is attached to the phydev.
 
> 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.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Mon, Mar 26, 2018 at 01:01:58AM +0200, Andrew Lunn wrote:
> The phylib core code will take the phydev lock before calling into the
> driver. By violating the layering, we are missing on this lock.

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.

> Maybe the one driver which currently implements these calls does not
> need locking.

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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Andrew Lunn]

> On Mon, Apr 02, 2018 at 08:55:27PM -0700, Richard Cochran wrote:
> On Sun, Mar 25, 2018 at 04:01:49PM -0700, Florian Fainelli wrote:
> > 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...
> 
> That won't work at all, AFAICT.  There is only one mii_bus per netdev,
> that is one that is attached to the phydev.


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

Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

[Richard Cochran]

On Tue, Apr 03, 2018 at 03:13:19PM +0200, Andrew Lunn wrote:
> Have you tried implementing it using a phandle in the phy node,
> pointing to the time stamping device?

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