DP83867E: Gigabit link reported up with broken wires in cable

Hi Roger,

Thank you for this query.

Can register 0x1 be read multiple times to clear any past values? I see 0x1 = 0x7949 indicating link down.

Are the C&D channels completely disconnected, or with some amount of degradation? It's possible that with slight degradation, the auto-neg pulses are still successfully transmitted to get link up in 1G, but communication will fail.

Speed optimization requires link-up process to fail, so I would like to focus on the conditions for 1G link to falsely come up in this case.

Thank you,

Evan

Hi Evan,

Thank you for your quick reply.  This is causing problems with units in the field.  To replicate the problem in our labs, we took a 5m CAT-5 able and cut one of the wires in the C and/or D pairs.

When we use this cable, we always get a 1G link established to a switch.  We've tested the same cable and switch with one of our products using a different PHY and that always establishes the link at 100M.  This other PHY seems to detect the broken wires and downgrade the link speed automatically.  

I'm trying to get my head around the 802.3 spec, but it seems to say that the auto-negotiation is backwards compatible with 100M and 10M, so only on the A+B channels.  This would explain why this scenario can occur.  If the auto-negotiation doesn't use C or D channels, then it won't detect the broken connections by itself.  There would need to be some additional functionality that kicks in to detect the C or D channel failure.

The log that I provided of the register reads is produced as the kernel starts and brings up the PHY, not during normal operation.  Unfortunately our MAC driver doesn't support SIOCGMIIREG and similar IOCTLs, so I'll have to either add that functionality to it or add some hack into the PHY itself to be able to read the registers during normal operation.

Does the above make sense, and have I understood the auto-negotiation correctly?

Thanks,

Roger

Hi Roger,

Thanks for clarifying. Your understanding of auto-negotiation is correct - speed optimization is the additional functionality that detects a fault on channels before downgrading speed.

I believe speed optimization enable is not taking effect in your driver / device tree. The enable bit for this 0x14[9] = '0' in the shared register log.

Please write 0x14[9] = '1' and share results when testing with the same cable.

Thank you,

Evan

Hi Evan,

Thank you for your reply.  I _think_ that I'm setting that bit already.  The default value for CFG2(0x14) is 0x29c7.  I've modified the PHY driver to set bit 9, so you see in the log above:

[ 5.992204] Enabling DP83867 speed optimisation enhancement
[ 5.997810] 0x14: 0x2bc7

and this changes from 0x28c7 to 0x2bc7.  If my maths is correct, this sets SPEED_OPT_EN=1.

This was the first thing that I tried as part of the investigation but unfortunately, we still have the issue.

From what I can see, the above settings enable both SPEED_OPT_EN and SPEED_OPT_ENHANCED_EN, but according to SPEED_OPT_ATTEMPT_CNT in the same register, we have to have 4 link establishment failures before the speed optimisation is performed.  I can change SPEED_OPT_ATTEMPT_CNT to 00 which means a single link failure, but I don't think we are getting _any_ link failures.  We always get the link established as 1G due to the auto-negotiation handshake.

I feel that I'm missing something here, because if speed optimisation requires a link failure, but you can have an auto-negotiation for 1G with channels C and/or D not working, then I don't see what would trigger the link failure.  I can't see a mechanism that can detect the broken link on channels C and/or D and then mark the link as failed.

The datasheet refers to "energy detectors" and "fast link drop", which is configured using FLD_CFG (0x2d) and FLD_THR_CFG(0x2E).  Is this what we should be doing?  The description of FLD_EN in the FLD_CFG register suggests that we have to set this low until the link has been established, then set it high to enable FLD.  That would mean that we need code somewhere to handle this, and I can't see any code that does this in the Linux PHY driver, not even in recent kernels.  The driver doesn't even define the FLD_CFG register.

Is that what we are missing?  Some CPU code that enables FLD, which will then fail due to missing energy on channel C and/or D, and that in turn triggers the down shift?

Thanks,

Roger

Hi Roger,

Evan Mayhew said:

Speed optimization requires link-up process to fail, so I would like to focus on the conditions for 1G link to falsely come up in this case.

I'd like to clarify this statement - speed optimization can trigger either through link failure, or through detecting low energy on channels C and D due to a fault in enhanced mode. The configuration 0x2bc7 is correct, and I expect speed optimization to trigger here if C&D are disconnected. 

Please test with SPEED_OPT_ATTEMPT_CNT[11:10] = '00'. It's unclear to me why speed optimization is not triggering here. Replicating this setup with a 2-pair cable and 0x14 = 0x2BC7, I see speed downgrade occurs:

Thank you,

Evan

Hi Roger,

Thanks for confirming the resolution here!

Can you share the driver file/changes you have made so we can look to resolve this on other Kernel versions as well?

Best regards,

Evan

Hi Evan,

No problem, I just need to tidy them up a bit first.  I should be able to get them over to you later next week.

As part of this investigation, I've added TDR to the PHY driver via sysfs files, do you want that too?  My problem with the TDR is that I can't run it if the link is up... and I can't see any way to force the link down from software so that I can perform a TDR without manually disconnecting the cable.  I've tried setting the PHY into standby via PHYCR[2] but I still get a TDR FAIL.  The context here is a failed cable in the field, so I want to locate the fault _before_ dispatching and engineer and them climbing up a pole.

Thanks,

Roger

Hi Roger,

Sounds good! If you can include TDR functionality as well that would be valuable for us to reference.

There should be a few ways to kill link from software side with register access, please let me know if any of these work for you:

1) Swap mirror mode value (0x31[0] = 'x')

2) Enable PHY loopback (0x0[14] = '1)

3) Disable speed advertisements (0x4[8:5] = '00' for 10/100M, 0x9[9:8] = '00' for 1G), then restart auto-negotiation with 0x0[9] = '1'.

Thank you,

Evan

Hi Evan,

Please find attached two patch files that work with Linux 4.9.291.  

First is the patch for the downshift:

Index: a/drivers/net/phy/dp83867.c
===================================================================
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -26,9 +26,11 @@
 #define DP83867_DEVADDR		0x1f
 
 #define MII_DP83867_PHYCTRL	0x10
+#define MII_DP83867_PHYSTS	0x11
 #define MII_DP83867_MICR	0x12
 #define MII_DP83867_ISR		0x13
 #define MII_DP83867_ISR		0x13
+#define MII_DP83867_CFG2	0x14
 #define MII_DP83867_LEDCR1	0x18
 #define DP83867_CTRL		0x1f
 #define DP83867_CFG3		0x1e
@@ -55,6 +57,18 @@
 #define MII_DP83867_MICR_POL_CHNG_INT_EN	BIT(1)
 #define MII_DP83867_MICR_JABBER_INT_EN		BIT(0)
 
+/* MII_DP83867_CFG2 control bits*/
+#define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_MASK	(0x03 << 10)
+#define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_8	(0x03 << 10)
+#define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_4	(0x02 << 10)
+#define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_2	(0x01 << 10)
+#define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_1	(0x00 << 10)
+#define MII_DP83867_CFG2_SPEED_OPT_EN		BIT(9)
+#define MII_DP83867_CFG2_SPEED_OPT_ENHANCED_EN	BIT(8)
+
+/* DP83867_CFG3 control bits*/
+#define DP83867_CFG3_INT_OE		BIT(7)
+
 /* RGMIICTL bits */
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
 #define DP83867_RGMII_RX_CLK_DELAY_EN		BIT(0)
@@ -145,26 +159,25 @@ static int dp83867_of_init(struct phy_de
 }
 #endif /* CONFIG_OF_MDIO */
 
+static int dp83867_probe(struct phy_device *phydev)
+{
+	struct dp83867_private *priv;
+	int ret;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+	return dp83867_of_init(phydev);
+}
+
 static int dp83867_config_init(struct phy_device *phydev)
 {
-	struct dp83867_private *dp83867;
+	struct dp83867_private *dp83867 = (struct dp83867_private *)phydev->priv;
 	int ret, val, bs;
 	u16 delay;
 
-	if (!phydev->priv) {
-		dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),
-				       GFP_KERNEL);
-		if (!dp83867)
-			return -ENOMEM;
-
-		phydev->priv = dp83867;
-		ret = dp83867_of_init(phydev);
-		if (ret)
-			return ret;
-	} else {
-		dp83867 = (struct dp83867_private *)phydev->priv;
-	}
-
 	/* LED Settings:
 		LED GPIO: Reserved.
 		LED 2: Reserved.
@@ -229,10 +242,16 @@ static int dp83867_config_init(struct ph
 	/* Enable Interrupt output INT_OE in CFG3 register */
 	if (phy_interrupt_is_valid(phydev)) {
 		val = phy_read(phydev, DP83867_CFG3);
-		val |= BIT(7);
+		val |= DP83867_CFG3_INT_OE;
 		phy_write(phydev, DP83867_CFG3, val);
 	}
 
+	/* Enable speed downshift after a single link failure */
+	val = phy_read(phydev, MII_DP83867_CFG2);
+	val &= ~MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_MASK;
+	val |= MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_1 | MII_DP83867_CFG2_SPEED_OPT_EN | MII_DP83867_CFG2_SPEED_OPT_ENHANCED_EN;
+	phy_write(phydev, MII_DP83867_CFG2, val);
+
 	return 0;
 }
 
@@ -247,6 +266,38 @@ static int dp83867_phy_reset(struct phy_
 	return dp83867_config_init(phydev);
 }
 
+static int dp83867_read_status(struct phy_device *phydev)
+{
+	// Do the standard stuff first
+	int res = genphy_read_status(phydev);
+	if (res) {
+		return res;
+	}
+
+	/*
+	This chip is smart and has downshift, so use the extended status
+	register to get the true speed and duplex.
+	*/
+	res = phy_read(phydev, MII_DP83867_PHYSTS);
+	switch ((res & 0xC000) >> 14) {
+	case 0:
+		phydev->speed = SPEED_10;
+		break;
+	case 1:
+		phydev->speed = SPEED_100;
+		break;
+	case 2:
+		phydev->speed = SPEED_1000;
+		break;
+	default:
+		phydev->speed = SPEED_UNKNOWN;
+		break;
+	}
+
+	phydev->duplex = (res & 0x2000)?DUPLEX_FULL:DUPLEX_HALF;
+	return 0;
+}
+
 static struct phy_driver dp83867_driver[] = {
 	{
 		.phy_id		= DP83867_PHY_ID,
@@ -255,6 +306,7 @@ static struct phy_driver dp83867_driver[
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 
+		.probe		= dp83867_probe,
 		.config_init	= dp83867_config_init,
 		.soft_reset	= dp83867_phy_reset,
 
@@ -263,7 +315,7 @@ static struct phy_driver dp83867_driver[
 		.config_intr	= dp83867_config_intr,
 
 		.config_aneg	= genphy_config_aneg,
-		.read_status	= genphy_read_status,
+		.read_status	= dp83867_read_status,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 	},

and here is the patch for the TDR, which assumes the above patch is also applied:

Index: a/drivers/net/phy/dp83867.c
===================================================================
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/ctype.h>
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
 #include <linux/mii.h>
@@ -25,6 +26,7 @@
 #define DP83867_PHY_ID		0x2000a231
 #define DP83867_DEVADDR		0x1f
 
+#define MII_DP83867_CFG1	0x09
 #define MII_DP83867_PHYCTRL	0x10
 #define MII_DP83867_PHYSTS	0x11
 #define MII_DP83867_MICR	0x12
@@ -36,10 +38,26 @@
 #define DP83867_CFG3		0x1e
 
 /* Extended Registers */
+#define DP83867_CFG4		0x0031
 #define DP83867_RGMIICTL	0x0032
 #define DP83867_STRAP_STS1	0x006E
 #define DP83867_RGMIIDCTL	0x0086
 
+#define DP83867_TDR_GEN_CFG1		0x180
+#define DP83867_TDR_PEAKS_LOC_BASE	0x190
+#define DP83867_TDR_PEAKS_AMP_BASE	0x19A
+#define DP83867_TDR_GEN_STATUS		0x1A4
+
+#define DP83867_TDR_PEAKS_LOC_COUNT	10
+#define DP83867_TDR_PEAKS_AMP_COUNT	10
+
+#define DP83867_TDR_FAULT_LOC_OFFSET	16
+#define DP83867_TDR_FAULT_LOC_MULT		8
+#define DP83867_TDR_FAULT_LOC_PROP_DELAY	5
+
+#define DP83867_TDR_GEN_STATUS_CROSS_CHAN_A 	8
+#define DP83867_TDR_GEN_STATUS_OVERFLOW_CHAN_A 	4
+
 #define DP83867_SW_RESET	BIT(15)
 #define DP83867_SW_RESTART	BIT(14)
 
@@ -57,6 +75,9 @@
 #define MII_DP83867_MICR_POL_CHNG_INT_EN	BIT(1)
 #define MII_DP83867_MICR_JABBER_INT_EN		BIT(0)
 
+/* MII_DP83867_CFG1 control bits */
+#define MII_DP83867_CFG1_TDR_AUTORUN		BIT(7)
+
 /* MII_DP83867_CFG2 control bits*/
 #define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_MASK	(0x03 << 10)
 #define MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_8	(0x03 << 10)
@@ -68,6 +89,9 @@
 
 /* DP83867_CFG3 control bits*/
 #define DP83867_CFG3_INT_OE		BIT(7)
+#define DP83867_CFG3_TDR_FAIL		BIT(2)
+#define DP83867_CFG3_TDR_DONE		BIT(1)
+#define DP83867_CFG3_TDR_START		BIT(0)
 
 /* RGMIICTL bits */
 #define DP83867_RGMII_TX_CLK_DELAY_EN		BIT(1)
@@ -81,6 +105,9 @@
 #define DP83867_PHYCR_FIFO_DEPTH_MASK		(3 << 14)
 #define DP83867_PHYCR_RESERVED_MASK		BIT(11)
 
+/* DP83867_CFG4 bits */
+#define DP83867_CFG4_MIRROR 	BIT(0)
+
 /* RGMIIDCTL bits */
 #define DP83867_RGMII_TX_CLK_DELAY_SHIFT	4
 
@@ -159,6 +186,303 @@ static int dp83867_of_init(struct phy_de
 }
 #endif /* CONFIG_OF_MDIO */
 
+#define DP83867_ATTR(_name, _mode, _show, _store)			\
+const struct device_attribute dp83867_attr_##_name = {		\
+	.attr = {.name = __stringify(_name), .mode = _mode },	\
+	.show	= _show,	\
+	.store	= _store,	\
+};
+
+#define DP83867_ATTR_RO(_name) \
+	DP83867_ATTR(_name, S_IRUGO, _name##_show, NULL)
+
+#define DP83867_ATTR_RW(_name) \
+	DP83867_ATTR(_name, S_IRUGO|S_IWUSR|S_IWGRP, _name##_show, _name##_store)
+
+#define DP83867_ATTR_WO(_name) \
+	DP83867_ATTR(_name, S_IWUSR|S_IWGRP, NULL, _name##_store)
+
+static ssize_t reg_generic_show(struct device *dev, struct device_attribute *attr, char *buf, int addr)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int val = phy_read(phydev, addr);
+		if (val < 0)
+			return val;
+
+	return sprintf(buf, "0x%04x\n", val);
+}
+
+static ssize_t reg_generic_show_bit(struct device *dev, struct device_attribute *attr, char *buf, int addr, int bit_mask)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int val = phy_read(phydev, addr);
+	if (val < 0)
+		return val;
+
+	return sprintf(buf, "%i\n", (val & bit_mask)?1:0);
+}
+
+static ssize_t reg_generic_store_bit(struct device *dev, struct device_attribute *attr, const char *buf, size_t size, int addr, int bit_mask)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	ssize_t ret;
+	int val;
+	bool flag;
+
+	ret = kstrtobool(buf, &flag);
+	if (ret) {
+		return ret;
+	}
+
+	val = phy_read(phydev, addr);
+
+	dev_info(dev, "Reg 0x%0x, old_val=0x%04x, mask=0x%04x, flag=%i", addr, val, bit_mask, (int)flag);
+
+	if (flag) {
+		val |= bit_mask;
+	}
+	else {
+		val &= ~bit_mask;
+	}
+
+	dev_info(dev, "Reg 0x%0x, new_val=0x%04x", addr, val);
+	ret = phy_write(phydev, addr, val);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t reg_indirect_show_bit(struct device *dev, struct device_attribute *attr, char *buf, int addr, int bit_mask)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int val = phy_read_mmd_indirect(phydev, addr, DP83867_DEVADDR);
+	if (val < 0)
+		return val;
+
+	return sprintf(buf, "%i\n", (val & bit_mask)?1:0);
+}
+
+static ssize_t reg_indirect_store_bit(struct device *dev, struct device_attribute *attr, const char *buf, size_t size, int addr, int bit_mask)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	ssize_t ret;
+	int val;
+	bool flag;
+
+	ret = kstrtobool(buf, &flag);
+	if (ret) {
+		return ret;
+	}
+
+	val = phy_read_mmd_indirect(phydev, addr, DP83867_DEVADDR);
+
+	dev_info(dev, "Reg 0x%0x, old_val=0x%04x, mask=0x%04x, flag=%i", addr, val, bit_mask, (int)flag);
+
+	if (flag) {
+		val |= bit_mask;
+	}
+	else {
+		val &= ~bit_mask;
+	}
+
+	dev_info(dev, "Reg 0x%0x, new_val=0x%04x", addr, val);
+	phy_write_mmd_indirect(phydev, addr, DP83867_DEVADDR, val);
+	return size;
+}
+
+static ssize_t reg_bmsr_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show(dev, attr, buf, MII_BMSR);
+}
+
+static ssize_t reg_physts_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show(dev, attr, buf, MII_DP83867_PHYSTS);
+}
+
+static ssize_t reg_cfg2_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show(dev, attr, buf, MII_DP83867_CFG2);
+}
+
+static ssize_t reg_cfg3_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show(dev, attr, buf, DP83867_CFG3);
+}
+
+static ssize_t reg_phyctrl_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show(dev, attr, buf, MII_DP83867_PHYCTRL);
+}
+
+static ssize_t loopback_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_generic_show_bit(dev, attr, buf, MII_BMCR, BMCR_LOOPBACK);
+}
+
+static ssize_t loopback_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size)
+{
+	return reg_generic_store_bit(dev, attr, buf, size, MII_BMCR, BMCR_LOOPBACK);
+}
+
+static ssize_t mirror_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return reg_indirect_show_bit(dev, attr, buf, DP83867_CFG4, DP83867_CFG4_MIRROR);
+}
+
+static ssize_t mirror_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size)
+{
+	return reg_indirect_store_bit(dev, attr, buf, size, DP83867_CFG4, DP83867_CFG4_MIRROR);
+}
+
+static ssize_t reg_tdr_cfg_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int val = phy_read_mmd_indirect(phydev, DP83867_TDR_GEN_CFG1, DP83867_DEVADDR);
+		if (val < 0)
+			return val;
+
+	return sprintf(buf, "0x%04x\n", val);
+}
+
+static ssize_t tdr_start_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t size)
+{
+	return reg_generic_store_bit(dev, attr, buf, size, DP83867_CFG3, DP83867_CFG3_TDR_START);
+}
+
+static ssize_t tdr_status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int val = phy_read(phydev, DP83867_CFG3);
+
+	return sprintf(buf, (val & DP83867_CFG3_TDR_FAIL)?"FAIL\n":((val & DP83867_CFG3_TDR_DONE)?"DONE\n":"BUSY\n"));
+}
+
+static ssize_t tdr_results_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct mdio_device *mdio_dev = container_of(dev, struct mdio_device, dev);
+	struct phy_device  *phydev = container_of(mdio_dev, struct phy_device, mdio);
+
+	int chan_locs[4][5], chan_amps[4][5];
+	int chan, i, status, res, offs;
+	ssize_t pos;
+
+	for(offs=0, chan=0; chan < 4; chan += 2, offs += 5) {
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_LOC_BASE + offs + 0, DP83867_DEVADDR);
+		chan_locs[chan + 0][0] = res & 0xFF;
+		chan_locs[chan + 0][1] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_LOC_BASE + offs + 1, DP83867_DEVADDR);
+		chan_locs[chan + 0][2] = res & 0xFF;
+		chan_locs[chan + 0][3] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_LOC_BASE + offs + 2, DP83867_DEVADDR);
+		chan_locs[chan + 0][4] = res & 0xFF;
+		chan_locs[chan + 1][0] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_LOC_BASE + offs + 3, DP83867_DEVADDR);
+		chan_locs[chan + 1][1] = res & 0xFF;
+		chan_locs[chan + 1][2] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_LOC_BASE + offs + 4, DP83867_DEVADDR);
+		chan_locs[chan + 1][3] = res & 0xFF;
+		chan_locs[chan + 1][4] = (res >> 8) & 0xFF;
+	}
+
+	for(offs=0, chan=0; chan < 4; chan += 2, offs += 5) {
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_AMP_BASE + offs + 0, DP83867_DEVADDR);
+		chan_amps[chan + 0][0] = res & 0xFF;
+		chan_amps[chan + 0][1] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_AMP_BASE + offs + 1, DP83867_DEVADDR);
+		chan_amps[chan + 0][2] = res & 0xFF;
+		chan_amps[chan + 0][3] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_AMP_BASE + offs + 2, DP83867_DEVADDR);
+		chan_amps[chan + 0][4] = res & 0xFF;
+		chan_amps[chan + 1][0] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_AMP_BASE + offs + 3, DP83867_DEVADDR);
+		chan_amps[chan + 1][1] = res & 0xFF;
+		chan_amps[chan + 1][2] = (res >> 8) & 0xFF;
+
+		res = phy_read_mmd_indirect(phydev, DP83867_TDR_PEAKS_AMP_BASE + offs + 4, DP83867_DEVADDR);
+		chan_amps[chan + 1][3] = res & 0xFF;
+		chan_amps[chan + 1][4] = (res >> 8) & 0xFF;
+	}
+
+	status = phy_read_mmd_indirect(phydev, DP83867_TDR_GEN_STATUS, DP83867_DEVADDR);
+
+	pos = 0;
+	for (chan=0; chan < 4; chan++) {
+		pos += sprintf(buf + pos, "CHAN %c:", 'A' + chan);
+		for (i=0; i < 5; i++) {
+			if (chan_locs[chan][i]) {
+				/*
+				See TI Apps Note "Time Domain Reflectometry with DP83867 and DP83869" (snla334)
+
+				That isn't quite right as the registers don't match ours but it's the closest I can find
+				to something useful.
+				*/
+				pos += sprintf(buf + pos,
+					" %i@%im", chan_amps[chan][i],
+					(DP83867_TDR_FAULT_LOC_MULT * (chan_locs[chan][i] - DP83867_TDR_FAULT_LOC_OFFSET))/(2 * DP83867_TDR_FAULT_LOC_PROP_DELAY)
+				);
+			}
+		}
+		if (status & BIT(DP83867_TDR_GEN_STATUS_CROSS_CHAN_A + chan)) {
+			pos += sprintf(buf + pos, " CROSS");
+		}
+		if (status & BIT(DP83867_TDR_GEN_STATUS_OVERFLOW_CHAN_A + chan)) {
+			pos += sprintf(buf + pos, " OVERFLOW");
+		}
+		pos += sprintf(buf + pos, "\n");
+	}
+	return pos;
+}
+
+DP83867_ATTR_RW(loopback)
+DP83867_ATTR_RW(mirror)
+DP83867_ATTR_RO(reg_bmsr)
+DP83867_ATTR_RO(reg_physts)
+DP83867_ATTR_RO(reg_cfg2)
+DP83867_ATTR_RO(reg_cfg3)
+DP83867_ATTR_RO(reg_tdr_cfg)
+DP83867_ATTR_RO(reg_phyctrl)
+DP83867_ATTR_WO(tdr_start)
+DP83867_ATTR_RO(tdr_status)
+DP83867_ATTR_RO(tdr_results)
+
+static const struct attribute *dp83867_attrs[] = {
+	&dp83867_attr_loopback.attr,
+	&dp83867_attr_mirror.attr,
+	&dp83867_attr_reg_bmsr.attr,
+	&dp83867_attr_reg_physts.attr,
+	&dp83867_attr_reg_cfg2.attr,
+	&dp83867_attr_reg_cfg3.attr,
+	&dp83867_attr_reg_phyctrl.attr,
+	&dp83867_attr_reg_tdr_cfg.attr,
+	&dp83867_attr_tdr_start.attr,
+	&dp83867_attr_tdr_status.attr,
+	&dp83867_attr_tdr_results.attr,
+	NULL
+};
+
 static int dp83867_probe(struct phy_device *phydev)
 {
 	struct dp83867_private *priv;
@@ -169,7 +493,11 @@ static int dp83867_probe(struct phy_devi
 		return -ENOMEM;
 
 	phydev->priv = priv;
-	return dp83867_of_init(phydev);
+	ret = dp83867_of_init(phydev);
+	if (ret)
+		return ret;
+
+	return sysfs_create_files(&phydev->mdio.dev.kobj, dp83867_attrs);
 }
 
 static int dp83867_config_init(struct phy_device *phydev)
@@ -246,6 +574,11 @@ static int dp83867_config_init(struct ph
 		phy_write(phydev, DP83867_CFG3, val);
 	}
 
+	/* Enable automatic TDR on link fail */
+	val = phy_read(phydev, MII_DP83867_CFG1);
+	val |= MII_DP83867_CFG1_TDR_AUTORUN;
+	phy_write(phydev, MII_DP83867_CFG1, val);
+
 	/* Enable speed downshift after a single link failure */
 	val = phy_read(phydev, MII_DP83867_CFG2);
 	val &= ~MII_DP83867_CFG2_SPEED_OPT_ATTEMPT_CNT_MASK;
@@ -298,6 +631,11 @@ static int dp83867_read_status(struct ph
 	return 0;
 }
 
+static void dp83867_remove(struct phy_device *phydev)
+{
+	sysfs_remove_files(&phydev->mdio.dev.kobj, dp83867_attrs);
+}
+
 static struct phy_driver dp83867_driver[] = {
 	{
 		.phy_id		= DP83867_PHY_ID,
@@ -318,6 +656,7 @@ static struct phy_driver dp83867_driver[
 		.read_status	= dp83867_read_status,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
+		.remove		= dp83867_remove,
 	},
 };
 module_phy_driver(dp83867_driver);

The TDR patch exposes a few additional registers to assist in debugging, which is above and beyond the TDR functionality itself.

Best regards,

Roger