[PATCH net-next 0/4] net: phy: broadcom: cable tester support

[PATCH net-next 0/4] net: phy: broadcom: cable tester support

[Michael Walle]

Add cable tester support for the Broadcom PHYs. Support for it was
developed on a BCM54140 Quad PHY which RDB register access.

If there is a link partner the results are not as good as with an open
cable. I guess we could retry if the measurement until all pairs had at
least one valid result.

Please note that this patch depends on Andrew's latest cable testing
series:
https://lore.kernel.org/netdev/20200509162851.362346-1-andrew@lunn.ch/

Michael Walle (4):
  net: phy: broadcom: add exp register access methods without buslock
  net: phy: broadcom: add bcm_phy_modify_exp()
  net: phy: broadcom: add cable test support
  net: phy: bcm54140: add cable diagnostics support

 drivers/net/phy/bcm-phy-lib.c | 264 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h |  10 ++
 drivers/net/phy/bcm54140.c    |   2 +
 include/linux/brcmphy.h       |  52 +++++++
 4 files changed, 321 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH net-next 1/4] net: phy: broadcom: add exp register access methods without buslock

[Michael Walle]

Add helper to read and write expansion registers without taking the mdio
lock.

Please note, that this changes the semantics of the read and write.
Before there was no lock between selecting the expansion register and
the actual read/write. This may lead to access failures if there are
parallel accesses. Instead take the bus lock during the whole access
cycle.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm-phy-lib.c | 38 ++++++++++++++++++++++++++++-------
 drivers/net/phy/bcm-phy-lib.h |  2 ++
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index d5f9a2701989..a390812714ed 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -14,33 +14,57 @@
 #define MII_BCM_CHANNEL_WIDTH     0x2000
 #define BCM_CL45VEN_EEE_ADV       0x3c
 
-int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val)
+int __bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val)
 {
 	int rc;
 
-	rc = phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
+	rc = __phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
 	if (rc < 0)
 		return rc;
 
-	return phy_write(phydev, MII_BCM54XX_EXP_DATA, val);
+	return __phy_write(phydev, MII_BCM54XX_EXP_DATA, val);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_write_exp);
+
+int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val)
+{
+	int rc;
+
+	phy_lock_mdio_bus(phydev);
+	rc = __bcm_phy_write_exp(phydev, reg, val);
+	phy_unlock_mdio_bus(phydev);
+
+	return rc;
 }
 EXPORT_SYMBOL_GPL(bcm_phy_write_exp);
 
-int bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
+int __bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
 {
 	int val;
 
-	val = phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
+	val = __phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
 	if (val < 0)
 		return val;
 
-	val = phy_read(phydev, MII_BCM54XX_EXP_DATA);
+	val = __phy_read(phydev, MII_BCM54XX_EXP_DATA);
 
 	/* Restore default value.  It's O.K. if this write fails. */
-	phy_write(phydev, MII_BCM54XX_EXP_SEL, 0);
+	__phy_write(phydev, MII_BCM54XX_EXP_SEL, 0);
 
 	return val;
 }
+EXPORT_SYMBOL_GPL(__bcm_phy_read_exp);
+
+int bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
+{
+	int rc;
+
+	phy_lock_mdio_bus(phydev);
+	rc = __bcm_phy_read_exp(phydev, reg);
+	phy_unlock_mdio_bus(phydev);
+
+	return rc;
+}
 EXPORT_SYMBOL_GPL(bcm_phy_read_exp);
 
 int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum)
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 4d3de91cda6c..0eb5333cda39 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -27,6 +27,8 @@
 #define AFE_HPF_TRIM_OTHERS		MISC_ADDR(0x3a, 0)
 
 
+int __bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
+int __bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
 int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
 int bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
 
-- 
2.20.1

Re: [PATCH net-next 1/4] net: phy: broadcom: add exp register access methods without buslock

[Florian Fainelli]

On 5/9/2020 3:37 PM, Michael Walle wrote:
> Add helper to read and write expansion registers without taking the mdio
> lock.
> 
> Please note, that this changes the semantics of the read and write.
> Before there was no lock between selecting the expansion register and
> the actual read/write. This may lead to access failures if there are
> parallel accesses. Instead take the bus lock during the whole access
> cycle.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 1/4] net: phy: broadcom: add exp register access methods without buslock

[Andrew Lunn]

On Sun, May 10, 2020 at 12:37:11AM +0200, Michael Walle wrote:
> Add helper to read and write expansion registers without taking the mdio
> lock.
> 
> Please note, that this changes the semantics of the read and write.
> Before there was no lock between selecting the expansion register and
> the actual read/write. This may lead to access failures if there are
> parallel accesses. Instead take the bus lock during the whole access
> cycle.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 2/4] net: phy: broadcom: add bcm_phy_modify_exp()

[Michael Walle]

Add the convenience function to do a read-modify-write. This has the
additional benefit of saving one write to the selection register.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm-phy-lib.c | 32 ++++++++++++++++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index a390812714ed..41c728fbcfb2 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -67,6 +67,38 @@ int bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_read_exp);
 
+int __bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set)
+{
+	int new, ret;
+
+	ret = __phy_write(phydev, MII_BCM54XX_EXP_SEL, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = __phy_read(phydev, MII_BCM54XX_EXP_DATA);
+	if (ret < 0)
+		return ret;
+
+	new = (ret & ~mask) | set;
+	if (new == ret)
+		return 0;
+
+	return __phy_write(phydev, MII_BCM54XX_EXP_DATA, new);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_modify_exp);
+
+int bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set)
+{
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+	ret = __bcm_phy_modify_exp(phydev, reg, mask, set);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_modify_exp);
+
 int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum)
 {
 	/* The register must be written to both the Shadow Register Select and
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 0eb5333cda39..b35d880220b9 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -29,8 +29,10 @@
 
 int __bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
 int __bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
+int __bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set);
 int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
 int bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
+int bcm_phy_modify_exp(struct phy_device *phydev, u16 reg, u16 mask, u16 set);
 
 static inline int bcm_phy_write_exp_sel(struct phy_device *phydev,
 					u16 reg, u16 val)
-- 
2.20.1

Re: [PATCH net-next 2/4] net: phy: broadcom: add bcm_phy_modify_exp()

[Florian Fainelli]

On 5/9/2020 3:37 PM, Michael Walle wrote:
> Add the convenience function to do a read-modify-write. This has the
> additional benefit of saving one write to the selection register.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/4] net: phy: broadcom: add bcm_phy_modify_exp()

[Andrew Lunn]

On Sun, May 10, 2020 at 12:37:12AM +0200, Michael Walle wrote:
> Add the convenience function to do a read-modify-write. This has the
> additional benefit of saving one write to the selection register.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 3/4] net: phy: broadcom: add cable test support

[Michael Walle]

Most modern broadcom PHYs support ECD (enhanced cable diagnostics). Add
support for it in the bcm-phy-lib so they can easily be used in the PHY
driver.

There are two access methods for ECD: legacy by expansion registers and
via the new RDB registers which are exclusive. Provide functions in two
variants where the PHY driver can from. To keep things simple for now,
we just switch the register access to expansion registers in the RDB
variant for now. On the flipside, we have to keep a bus lock to prevent
any other non-legacy access on the PHY.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm-phy-lib.c | 194 ++++++++++++++++++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |   6 ++
 include/linux/brcmphy.h       |  52 +++++++++
 3 files changed, 252 insertions(+)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 41c728fbcfb2..4c4ee72a2e52 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -4,12 +4,14 @@
  */
 
 #include "bcm-phy-lib.h"
+#include <linux/bitfield.h>
 #include <linux/brcmphy.h>
 #include <linux/export.h>
 #include <linux/mdio.h>
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 
 #define MII_BCM_CHANNEL_WIDTH     0x2000
 #define BCM_CL45VEN_EEE_ADV       0x3c
@@ -581,6 +583,198 @@ int bcm_phy_enable_jumbo(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_enable_jumbo);
 
+int __bcm_phy_enable_rdb_access(struct phy_device *phydev)
+{
+	return __bcm_phy_write_exp(phydev, BCM54XX_EXP_REG7E, 0);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_enable_rdb_access);
+
+int __bcm_phy_enable_legacy_access(struct phy_device *phydev)
+{
+	return __bcm_phy_write_rdb(phydev, BCM54XX_RDB_REG0087,
+				   BCM54XX_ACCESS_MODE_LEGACY_EN);
+}
+EXPORT_SYMBOL_GPL(__bcm_phy_enable_legacy_access);
+
+static int _bcm_phy_cable_test_start(struct phy_device *phydev, bool is_rdb)
+{
+	u16 mask, set;
+	int ret;
+
+	/* Auto-negotiation must be enabled for cable diagnostics to work, but
+	 * don't advertise any capabilities.
+	 */
+	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
+	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
+	phy_write(phydev, MII_CTRL1000, 0);
+
+	phy_lock_mdio_bus(phydev);
+	if (is_rdb) {
+		ret = __bcm_phy_enable_legacy_access(phydev);
+		if (ret)
+			goto out;
+	}
+
+	mask = BCM54XX_ECD_CTRL_CROSS_SHORT_DIS | BCM54XX_ECD_CTRL_UNIT_MASK;
+	set = BCM54XX_ECD_CTRL_RUN | BCM54XX_ECD_CTRL_BREAK_LINK |
+	      FIELD_PREP(BCM54XX_ECD_CTRL_UNIT_MASK,
+			 BCM54XX_ECD_CTRL_UNIT_CM);
+
+	ret = __bcm_phy_modify_exp(phydev, BCM54XX_EXP_ECD_CTRL, mask, set);
+
+out:
+	/* re-enable the RDB access even if there was an error */
+	if (is_rdb)
+		ret = __bcm_phy_enable_rdb_access(phydev) ? : ret;
+
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+static int bcm_phy_cable_test_report_trans(int result)
+{
+	switch (result) {
+	case BCM54XX_ECD_FAULT_TYPE_OK:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case BCM54XX_ECD_FAULT_TYPE_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case BCM54XX_ECD_FAULT_TYPE_SAME_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+	case BCM54XX_ECD_FAULT_TYPE_INVALID:
+	case BCM54XX_ECD_FAULT_TYPE_BUSY:
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static bool bcm_phy_distance_valid(int result)
+{
+	switch (result) {
+	case BCM54XX_ECD_FAULT_TYPE_OPEN:
+	case BCM54XX_ECD_FAULT_TYPE_SAME_SHORT:
+	case BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int bcm_phy_report_length(struct phy_device *phydev, int result,
+				 int pair)
+{
+	int val;
+
+	val = __bcm_phy_read_exp(phydev,
+				 BCM54XX_EXP_ECD_PAIR_A_LENGTH_RESULTS + pair);
+	if (val < 0)
+		return val;
+
+	if (val == BCM54XX_ECD_LENGTH_RESULTS_INVALID)
+		return 0;
+
+	/* intra-pair shorts report twice the length */
+	if (result == BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT)
+		val >>= 1;
+
+	ethnl_cable_test_fault_length(phydev, pair, val);
+
+	return 0;
+}
+
+static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
+					  bool *finished, bool is_rdb)
+{
+	int pair_a, pair_b, pair_c, pair_d, ret;
+
+	*finished = false;
+
+	phy_lock_mdio_bus(phydev);
+
+	if (is_rdb) {
+		ret = __bcm_phy_enable_legacy_access(phydev);
+		if (ret)
+			goto out;
+	}
+
+	ret = __bcm_phy_read_exp(phydev, BCM54XX_EXP_ECD_CTRL);
+	if (ret < 0)
+		goto out;
+
+	if (ret & BCM54XX_ECD_CTRL_IN_PROGRESS) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = __bcm_phy_read_exp(phydev, BCM54XX_EXP_ECD_FAULT_TYPE);
+	if (ret < 0)
+		goto out;
+
+	pair_a = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_A_MASK, ret);
+	pair_b = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_B_MASK, ret);
+	pair_c = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_C_MASK, ret);
+	pair_d = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_D_MASK, ret);
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+				bcm_phy_cable_test_report_trans(pair_a));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				bcm_phy_cable_test_report_trans(pair_b));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				bcm_phy_cable_test_report_trans(pair_c));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				bcm_phy_cable_test_report_trans(pair_d));
+
+	if (bcm_phy_distance_valid(pair_a))
+		bcm_phy_report_length(phydev, pair_a, 0);
+	if (bcm_phy_distance_valid(pair_b))
+		bcm_phy_report_length(phydev, pair_b, 1);
+	if (bcm_phy_distance_valid(pair_c))
+		bcm_phy_report_length(phydev, pair_c, 2);
+	if (bcm_phy_distance_valid(pair_d))
+		bcm_phy_report_length(phydev, pair_d, 3);
+
+	ret = 0;
+	*finished = true;
+out:
+	/* re-enable the RDB access even if there was an error */
+	if (is_rdb)
+		ret = __bcm_phy_enable_rdb_access(phydev) ? : ret;
+
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+
+int bcm_phy_cable_test_start(struct phy_device *phydev)
+{
+	return _bcm_phy_cable_test_start(phydev, false);
+}
+EXPORT_SYMBOL_GPL(bcm_phy_cable_test_start);
+
+int bcm_phy_cable_test_get_status(struct phy_device *phydev, bool *finished)
+{
+	return _bcm_phy_cable_test_get_status(phydev, finished, false);
+}
+EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status);
+
+/* We assume that all PHYs which support RDB access can be switched to legacy
+ * mode. If, in the future, this is not true anymore, we have to re-implement
+ * this with RDB access.
+ */
+int bcm_phy_cable_test_start_rdb(struct phy_device *phydev)
+{
+	return _bcm_phy_cable_test_start(phydev, true);
+}
+EXPORT_SYMBOL_GPL(bcm_phy_cable_test_start_rdb);
+
+int bcm_phy_cable_test_get_status_rdb(struct phy_device *phydev,
+				      bool *finished)
+{
+	return _bcm_phy_cable_test_get_status(phydev, finished, true);
+}
+EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb);
+
 MODULE_DESCRIPTION("Broadcom PHY Library");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index b35d880220b9..237a8503c9b4 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -80,4 +80,10 @@ void bcm_phy_r_rc_cal_reset(struct phy_device *phydev);
 int bcm_phy_28nm_a0b0_afe_config_init(struct phy_device *phydev);
 int bcm_phy_enable_jumbo(struct phy_device *phydev);
 
+int bcm_phy_cable_test_get_status_rdb(struct phy_device *phydev,
+				      bool *finished);
+int bcm_phy_cable_test_start_rdb(struct phy_device *phydev);
+int bcm_phy_cable_test_start(struct phy_device *phydev);
+int bcm_phy_cable_test_get_status(struct phy_device *phydev, bool *finished);
+
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 58d0150acc3e..d41624db6de2 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -119,6 +119,11 @@
 #define MII_BCM54XX_RDB_ADDR	0x1e
 #define MII_BCM54XX_RDB_DATA	0x1f
 
+/* legacy access control via rdb/expansion register */
+#define BCM54XX_RDB_REG0087		0x0087
+#define BCM54XX_EXP_REG7E		(MII_BCM54XX_EXP_SEL_ER + 0x7E)
+#define BCM54XX_ACCESS_MODE_LEGACY_EN	BIT(15)
+
 /*
  * AUXILIARY CONTROL SHADOW ACCESS REGISTERS.  (PHY REG 0x18)
  */
@@ -294,4 +299,51 @@
 #define MII_BRCM_CORE_EXPB0	0xB0
 #define MII_BRCM_CORE_EXPB1	0xB1
 
+/* Enhanced Cable Diagnostics */
+#define BCM54XX_RDB_ECD_CTRL			0x2a0
+#define BCM54XX_EXP_ECD_CTRL			(MII_BCM54XX_EXP_SEL_ER + 0xc0)
+
+#define BCM54XX_ECD_CTRL_CABLE_TYPE_CAT3	1	/* CAT3 or worse */
+#define BCM54XX_ECD_CTRL_CABLE_TYPE_CAT5	0	/* CAT5 or better */
+#define BCM54XX_ECD_CTRL_CABLE_TYPE_MASK	BIT(0)	/* cable type */
+#define BCM54XX_ECD_CTRL_INVALID		BIT(3)	/* invalid result */
+#define BCM54XX_ECD_CTRL_UNIT_CM		0	/* centimeters */
+#define BCM54XX_ECD_CTRL_UNIT_M			1	/* meters */
+#define BCM54XX_ECD_CTRL_UNIT_MASK		BIT(10)	/* cable length unit */
+#define BCM54XX_ECD_CTRL_IN_PROGRESS		BIT(11)	/* test in progress */
+#define BCM54XX_ECD_CTRL_BREAK_LINK		BIT(12)	/* unconnect link
+							 * during test
+							 */
+#define BCM54XX_ECD_CTRL_CROSS_SHORT_DIS	BIT(13)	/* disable inter-pair
+							 * short check
+							 */
+#define BCM54XX_ECD_CTRL_RUN			BIT(15)	/* run immediate */
+
+#define BCM54XX_RDB_ECD_FAULT_TYPE		0x2a1
+#define BCM54XX_EXP_ECD_FAULT_TYPE		(MII_BCM54XX_EXP_SEL_ER + 0xc1)
+#define BCM54XX_ECD_FAULT_TYPE_INVALID		0x0
+#define BCM54XX_ECD_FAULT_TYPE_OK		0x1
+#define BCM54XX_ECD_FAULT_TYPE_OPEN		0x2
+#define BCM54XX_ECD_FAULT_TYPE_SAME_SHORT	0x3 /* short same pair */
+#define BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT	0x4 /* short different pairs */
+#define BCM54XX_ECD_FAULT_TYPE_BUSY		0x9
+#define BCM54XX_ECD_FAULT_TYPE_PAIR_D_MASK	GENMASK(3, 0)
+#define BCM54XX_ECD_FAULT_TYPE_PAIR_C_MASK	GENMASK(7, 4)
+#define BCM54XX_ECD_FAULT_TYPE_PAIR_B_MASK	GENMASK(11, 8)
+#define BCM54XX_ECD_FAULT_TYPE_PAIR_A_MASK	GENMASK(15, 12)
+#define BCM54XX_ECD_PAIR_A_LENGTH_RESULTS	0x2a2
+#define BCM54XX_ECD_PAIR_B_LENGTH_RESULTS	0x2a3
+#define BCM54XX_ECD_PAIR_C_LENGTH_RESULTS	0x2a4
+#define BCM54XX_ECD_PAIR_D_LENGTH_RESULTS	0x2a5
+
+#define BCM54XX_RDB_ECD_PAIR_A_LENGTH_RESULTS	0x2a2
+#define BCM54XX_EXP_ECD_PAIR_A_LENGTH_RESULTS	(MII_BCM54XX_EXP_SEL_ER + 0xc2)
+#define BCM54XX_RDB_ECD_PAIR_B_LENGTH_RESULTS	0x2a3
+#define BCM54XX_EXP_ECD_PAIR_B_LENGTH_RESULTS	(MII_BCM54XX_EXP_SEL_ER + 0xc3)
+#define BCM54XX_RDB_ECD_PAIR_C_LENGTH_RESULTS	0x2a4
+#define BCM54XX_EXP_ECD_PAIR_C_LENGTH_RESULTS	(MII_BCM54XX_EXP_SEL_ER + 0xc4)
+#define BCM54XX_RDB_ECD_PAIR_D_LENGTH_RESULTS	0x2a5
+#define BCM54XX_EXP_ECD_PAIR_D_LENGTH_RESULTS	(MII_BCM54XX_EXP_SEL_ER + 0xc5)
+#define BCM54XX_ECD_LENGTH_RESULTS_INVALID	0xffff
+
 #endif /* _LINUX_BRCMPHY_H */
-- 
2.20.1

Re: [PATCH net-next 3/4] net: phy: broadcom: add cable test support

[Florian Fainelli]

On 5/9/2020 3:37 PM, Michael Walle wrote:
> Most modern broadcom PHYs support ECD (enhanced cable diagnostics). Add
> support for it in the bcm-phy-lib so they can easily be used in the PHY
> driver.
> 
> There are two access methods for ECD: legacy by expansion registers and
> via the new RDB registers which are exclusive. Provide functions in two
> variants where the PHY driver can from. To keep things simple for now,
> we just switch the register access to expansion registers in the RDB
> variant for now. On the flipside, we have to keep a bus lock to prevent
> any other non-legacy access on the PHY.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks for dealing with the legacy expansion vs. RDB access method, this
looks really nice, now I just need to test it on a variety of devices :)
-- 
Florian

Re: [PATCH net-next 3/4] net: phy: broadcom: add cable test support

[kbuild test robot]

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

Hi Michael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on next-20200508]
[cannot apply to net/master linus/master ipvs/master v5.7-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/net-phy-broadcom-cable-tester-support/20200510-063955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 2c674bec76d35b75c7c730f863424387c9e9633a
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/phy/bcm-phy-lib.c: In function 'bcm_phy_cable_test_report_trans':
>> drivers/net/phy/bcm-phy-lib.c:639:10: error: 'ETHTOOL_A_CABLE_RESULT_CODE_OK' undeclared (first use in this function); did you mean 'ETHTOOL_A_COALESCE_PKT_RATE_LOW'?
      return ETHTOOL_A_CABLE_RESULT_CODE_OK;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ETHTOOL_A_COALESCE_PKT_RATE_LOW
   drivers/net/phy/bcm-phy-lib.c:639:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/net/phy/bcm-phy-lib.c:641:10: error: 'ETHTOOL_A_CABLE_RESULT_CODE_OPEN' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_RESULT_CODE_OK'?
      return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ETHTOOL_A_CABLE_RESULT_CODE_OK
>> drivers/net/phy/bcm-phy-lib.c:643:10: error: 'ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_RESULT_CODE_OPEN'?
      return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ETHTOOL_A_CABLE_RESULT_CODE_OPEN
>> drivers/net/phy/bcm-phy-lib.c:645:10: error: 'ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT'?
      return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT
>> drivers/net/phy/bcm-phy-lib.c:649:10: error: 'ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_RESULT_CODE_OPEN'?
      return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             ETHTOOL_A_CABLE_RESULT_CODE_OPEN
   drivers/net/phy/bcm-phy-lib.c: In function 'bcm_phy_report_length':
>> drivers/net/phy/bcm-phy-lib.c:681:2: error: implicit declaration of function 'ethnl_cable_test_fault_length' [-Werror=implicit-function-declaration]
     ethnl_cable_test_fault_length(phydev, pair, val);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm-phy-lib.c: In function '_bcm_phy_cable_test_get_status':
>> drivers/net/phy/bcm-phy-lib.c:719:2: error: implicit declaration of function 'ethnl_cable_test_result'; did you mean 'bcm_phy_cable_test_start'? [-Werror=implicit-function-declaration]
     ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
     ^~~~~~~~~~~~~~~~~~~~~~~
     bcm_phy_cable_test_start
>> drivers/net/phy/bcm-phy-lib.c:719:34: error: 'ETHTOOL_A_CABLE_PAIR_A' undeclared (first use in this function); did you mean 'ETHTOOL_A_PAUSE_MAX'?
     ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
                                     ^~~~~~~~~~~~~~~~~~~~~~
                                     ETHTOOL_A_PAUSE_MAX
>> drivers/net/phy/bcm-phy-lib.c:721:34: error: 'ETHTOOL_A_CABLE_PAIR_B' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_PAIR_A'?
     ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
                                     ^~~~~~~~~~~~~~~~~~~~~~
                                     ETHTOOL_A_CABLE_PAIR_A
>> drivers/net/phy/bcm-phy-lib.c:723:34: error: 'ETHTOOL_A_CABLE_PAIR_C' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_PAIR_B'?
     ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
                                     ^~~~~~~~~~~~~~~~~~~~~~
                                     ETHTOOL_A_CABLE_PAIR_B
>> drivers/net/phy/bcm-phy-lib.c:725:34: error: 'ETHTOOL_A_CABLE_PAIR_D' undeclared (first use in this function); did you mean 'ETHTOOL_A_CABLE_PAIR_C'?
     ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
                                     ^~~~~~~~~~~~~~~~~~~~~~
                                     ETHTOOL_A_CABLE_PAIR_C
   cc1: some warnings being treated as errors

vim +639 drivers/net/phy/bcm-phy-lib.c

   634	
   635	static int bcm_phy_cable_test_report_trans(int result)
   636	{
   637		switch (result) {
   638		case BCM54XX_ECD_FAULT_TYPE_OK:
 > 639			return ETHTOOL_A_CABLE_RESULT_CODE_OK;
   640		case BCM54XX_ECD_FAULT_TYPE_OPEN:
 > 641			return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
   642		case BCM54XX_ECD_FAULT_TYPE_SAME_SHORT:
 > 643			return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
   644		case BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT:
 > 645			return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
   646		case BCM54XX_ECD_FAULT_TYPE_INVALID:
   647		case BCM54XX_ECD_FAULT_TYPE_BUSY:
   648		default:
 > 649			return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
   650		}
   651	}
   652	
   653	static bool bcm_phy_distance_valid(int result)
   654	{
   655		switch (result) {
   656		case BCM54XX_ECD_FAULT_TYPE_OPEN:
   657		case BCM54XX_ECD_FAULT_TYPE_SAME_SHORT:
   658		case BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT:
   659			return true;
   660		}
   661		return false;
   662	}
   663	
   664	static int bcm_phy_report_length(struct phy_device *phydev, int result,
   665					 int pair)
   666	{
   667		int val;
   668	
   669		val = __bcm_phy_read_exp(phydev,
   670					 BCM54XX_EXP_ECD_PAIR_A_LENGTH_RESULTS + pair);
   671		if (val < 0)
   672			return val;
   673	
   674		if (val == BCM54XX_ECD_LENGTH_RESULTS_INVALID)
   675			return 0;
   676	
   677		/* intra-pair shorts report twice the length */
   678		if (result == BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT)
   679			val >>= 1;
   680	
 > 681		ethnl_cable_test_fault_length(phydev, pair, val);
   682	
   683		return 0;
   684	}
   685	
   686	static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
   687						  bool *finished, bool is_rdb)
   688	{
   689		int pair_a, pair_b, pair_c, pair_d, ret;
   690	
   691		*finished = false;
   692	
   693		phy_lock_mdio_bus(phydev);
   694	
   695		if (is_rdb) {
   696			ret = __bcm_phy_enable_legacy_access(phydev);
   697			if (ret)
   698				goto out;
   699		}
   700	
   701		ret = __bcm_phy_read_exp(phydev, BCM54XX_EXP_ECD_CTRL);
   702		if (ret < 0)
   703			goto out;
   704	
   705		if (ret & BCM54XX_ECD_CTRL_IN_PROGRESS) {
   706			ret = 0;
   707			goto out;
   708		}
   709	
   710		ret = __bcm_phy_read_exp(phydev, BCM54XX_EXP_ECD_FAULT_TYPE);
   711		if (ret < 0)
   712			goto out;
   713	
   714		pair_a = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_A_MASK, ret);
   715		pair_b = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_B_MASK, ret);
   716		pair_c = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_C_MASK, ret);
   717		pair_d = FIELD_GET(BCM54XX_ECD_FAULT_TYPE_PAIR_D_MASK, ret);
   718	
 > 719		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
   720					bcm_phy_cable_test_report_trans(pair_a));
 > 721		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
   722					bcm_phy_cable_test_report_trans(pair_b));
 > 723		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
   724					bcm_phy_cable_test_report_trans(pair_c));
 > 725		ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
   726					bcm_phy_cable_test_report_trans(pair_d));
   727	
   728		if (bcm_phy_distance_valid(pair_a))
   729			bcm_phy_report_length(phydev, pair_a, 0);
   730		if (bcm_phy_distance_valid(pair_b))
   731			bcm_phy_report_length(phydev, pair_b, 1);
   732		if (bcm_phy_distance_valid(pair_c))
   733			bcm_phy_report_length(phydev, pair_c, 2);
   734		if (bcm_phy_distance_valid(pair_d))
   735			bcm_phy_report_length(phydev, pair_d, 3);
   736	
   737		ret = 0;
   738		*finished = true;
   739	out:
   740		/* re-enable the RDB access even if there was an error */
   741		if (is_rdb)
   742			ret = __bcm_phy_enable_rdb_access(phydev) ? : ret;
   743	
   744		phy_unlock_mdio_bus(phydev);
   745	
   746		return ret;
   747	}
   748	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72353 bytes --]

Re: [PATCH net-next 3/4] net: phy: broadcom: add cable test support

[Andrew Lunn]

On Sun, May 10, 2020 at 12:37:13AM +0200, Michael Walle wrote:
> Most modern broadcom PHYs support ECD (enhanced cable diagnostics). Add
> support for it in the bcm-phy-lib so they can easily be used in the PHY
> driver.
> 
> There are two access methods for ECD: legacy by expansion registers and
> via the new RDB registers which are exclusive. Provide functions in two
> variants where the PHY driver can from. To keep things simple for now,

can from ?

> +static int bcm_phy_report_length(struct phy_device *phydev, int result,
> +				 int pair)
> +{
> +	int val;
> +
> +	val = __bcm_phy_read_exp(phydev,
> +				 BCM54XX_EXP_ECD_PAIR_A_LENGTH_RESULTS + pair);
> +	if (val < 0)
> +		return val;
> +
> +	if (val == BCM54XX_ECD_LENGTH_RESULTS_INVALID)
> +		return 0;
> +
> +	/* intra-pair shorts report twice the length */
> +	if (result == BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT)
> +		val >>= 1;

You mentioned this before. This seems odd. The pulse travelled the
same distance as for an open or shorted cable. The whole of time
domain reflectrometry is based on some sort of echo and you always
need to device by two. So why this special case?

Florian, do you have access to any erratas? Is this maybe fixed in
other revisions/family members?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/4] net: phy: broadcom: add cable test support

[Michael Walle]

Am 2020-05-10 16:44, schrieb Andrew Lunn:
> On Sun, May 10, 2020 at 12:37:13AM +0200, Michael Walle wrote:
>> Most modern broadcom PHYs support ECD (enhanced cable diagnostics). 
>> Add
>> support for it in the bcm-phy-lib so they can easily be used in the 
>> PHY
>> driver.
>> 
>> There are two access methods for ECD: legacy by expansion registers 
>> and
>> via the new RDB registers which are exclusive. Provide functions in 
>> two
>> variants where the PHY driver can from. To keep things simple for now,
> 
> can from ?

can choose from. Should I send a new patch? Will DaveM fix minor typos, 
if
he commits it?

> 
>> +static int bcm_phy_report_length(struct phy_device *phydev, int 
>> result,
>> +				 int pair)
>> +{
>> +	int val;
>> +
>> +	val = __bcm_phy_read_exp(phydev,
>> +				 BCM54XX_EXP_ECD_PAIR_A_LENGTH_RESULTS + pair);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	if (val == BCM54XX_ECD_LENGTH_RESULTS_INVALID)
>> +		return 0;
>> +
>> +	/* intra-pair shorts report twice the length */
>> +	if (result == BCM54XX_ECD_FAULT_TYPE_CROSS_SHORT)
>> +		val >>= 1;
> 
> You mentioned this before. This seems odd. The pulse travelled the
> same distance as for an open or shorted cable. The whole of time
> domain reflectrometry is based on some sort of echo and you always
> need to device by two. So why this special case?

Well, I don't know why this is special. But one thing which is
different is that you listen on all pairs for the pulse instead of
just the one where you've sent it (which seems to be a bit trickier
otherwise the cheapo AT8031 would support it, too). Maybe they
screwed that. In any case, I can try it with a 100m cable just
to be sure.

-michael

[PATCH net-next 4/4] net: phy: bcm54140: add cable diagnostics support

[Michael Walle]

Use the generic cable tester functions from bcm-phy-lib to add cable
tester support.

100m cable, A/B/C/D open:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair: Pair A, result: Open Circuit
  Pair: Pair B, result: Open Circuit
  Pair: Pair C, result: Open Circuit
  Pair: Pair D, result: Open Circuit
  Pair: Pair A, fault length: 106.60m
  Pair: Pair B, fault length: 103.32m
  Pair: Pair C, fault length: 104.96m
  Pair: Pair D, fault length: 106.60m

1m cable, A/B connected, pair C shorted, D open:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair: Pair A, result: OK
  Pair: Pair B, result: OK
  Pair: Pair C, result: Short within Pair
  Pair: Pair D, result: Open Circuit
  Pair: Pair C, fault length: 0.82m
  Pair: Pair D, fault length: 1.64m

1m cable, A/B connected, pair C shorted with D:
  Cable test started for device eth0.
  Cable test completed for device eth0.
  Pair: Pair A, result: OK
  Pair: Pair B, result: OK
  Pair: Pair C, result: Short to another pair
  Pair: Pair D, result: Short to another pair
  Pair: Pair C, fault length: 1.64m
  Pair: Pair D, fault length: 1.64m

The granularity of the length measurement seems to be 82cm.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm54140.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 9ef37a3bc2bb..8998e68bb26b 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -831,6 +831,7 @@ static struct phy_driver bcm54140_drivers[] = {
 		.phy_id         = PHY_ID_BCM54140,
 		.phy_id_mask    = BCM54140_PHY_ID_MASK,
 		.name           = "Broadcom BCM54140",
+		.flags		= PHY_POLL_CABLE_TEST,
 		.features       = PHY_GBIT_FEATURES,
 		.config_init    = bcm54140_config_init,
 		.did_interrupt	= bcm54140_did_interrupt,
@@ -842,6 +843,8 @@ static struct phy_driver bcm54140_drivers[] = {
 		.soft_reset	= genphy_soft_reset,
 		.get_tunable	= bcm54140_get_tunable,
 		.set_tunable	= bcm54140_set_tunable,
+		.cable_test_start = bcm_phy_cable_test_start_rdb,
+		.cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
 	},
 };
 module_phy_driver(bcm54140_drivers);
-- 
2.20.1

Re: [PATCH net-next 4/4] net: phy: bcm54140: add cable diagnostics support

[kbuild test robot]

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

Hi Michael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on next-20200508]
[cannot apply to net/master linus/master v5.7-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/net-phy-broadcom-cable-tester-support/20200510-063955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 2c674bec76d35b75c7c730f863424387c9e9633a
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/phy/bcm54140.c:834:13: error: 'PHY_POLL_CABLE_TEST' undeclared here (not in a function)
     834 |   .flags  = PHY_POLL_CABLE_TEST,
         |             ^~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:846:4: error: 'struct phy_driver' has no member named 'cable_test_start'
     846 |   .cable_test_start = bcm_phy_cable_test_start_rdb,
         |    ^~~~~~~~~~~~~~~~
>> drivers/net/phy/bcm54140.c:846:23: error: initialization of 'int (*)(struct phy_device *, bool)' {aka 'int (*)(struct phy_device *, _Bool)'} from incompatible pointer type 'int (*)(struct phy_device *)' [-Werror=incompatible-pointer-types]
     846 |   .cable_test_start = bcm_phy_cable_test_start_rdb,
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:846:23: note: (near initialization for 'bcm54140_drivers[0].set_loopback')
   drivers/net/phy/bcm54140.c:847:4: error: 'struct phy_driver' has no member named 'cable_test_get_status'
     847 |   .cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
         |    ^~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:847:28: warning: excess elements in struct initializer
     847 |   .cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:847:28: note: (near initialization for 'bcm54140_drivers[0]')
   cc1: some warnings being treated as errors

vim +846 drivers/net/phy/bcm54140.c

   828	
   829	static struct phy_driver bcm54140_drivers[] = {
   830		{
   831			.phy_id         = PHY_ID_BCM54140,
   832			.phy_id_mask    = BCM54140_PHY_ID_MASK,
   833			.name           = "Broadcom BCM54140",
   834			.flags		= PHY_POLL_CABLE_TEST,
   835			.features       = PHY_GBIT_FEATURES,
   836			.config_init    = bcm54140_config_init,
   837			.did_interrupt	= bcm54140_did_interrupt,
   838			.ack_interrupt  = bcm54140_ack_intr,
   839			.config_intr    = bcm54140_config_intr,
   840			.probe		= bcm54140_probe,
   841			.suspend	= genphy_suspend,
   842			.resume		= genphy_resume,
   843			.soft_reset	= genphy_soft_reset,
   844			.get_tunable	= bcm54140_get_tunable,
   845			.set_tunable	= bcm54140_set_tunable,
 > 846			.cable_test_start = bcm_phy_cable_test_start_rdb,
   847			.cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
   848		},
   849	};
   850	module_phy_driver(bcm54140_drivers);
   851	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61434 bytes --]

Re: [PATCH net-next 4/4] net: phy: bcm54140: add cable diagnostics support

[Florian Fainelli]

On 5/9/2020 3:37 PM, Michael Walle wrote:
> Use the generic cable tester functions from bcm-phy-lib to add cable
> tester support.
> 
> 100m cable, A/B/C/D open:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: Open Circuit
>   Pair: Pair B, result: Open Circuit
>   Pair: Pair C, result: Open Circuit
>   Pair: Pair D, result: Open Circuit
>   Pair: Pair A, fault length: 106.60m
>   Pair: Pair B, fault length: 103.32m
>   Pair: Pair C, fault length: 104.96m
>   Pair: Pair D, fault length: 106.60m
> 
> 1m cable, A/B connected, pair C shorted, D open:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: OK
>   Pair: Pair B, result: OK
>   Pair: Pair C, result: Short within Pair
>   Pair: Pair D, result: Open Circuit
>   Pair: Pair C, fault length: 0.82m
>   Pair: Pair D, fault length: 1.64m
> 
> 1m cable, A/B connected, pair C shorted with D:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: OK
>   Pair: Pair B, result: OK
>   Pair: Pair C, result: Short to another pair
>   Pair: Pair D, result: Short to another pair
>   Pair: Pair C, fault length: 1.64m
>   Pair: Pair D, fault length: 1.64m
> 
> The granularity of the length measurement seems to be 82cm.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 4/4] net: phy: bcm54140: add cable diagnostics support

[kbuild test robot]

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

Hi Michael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on next-20200508]
[cannot apply to net/master linus/master ipvs/master v5.7-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/net-phy-broadcom-cable-tester-support/20200510-063955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 2c674bec76d35b75c7c730f863424387c9e9633a
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/net/phy/bcm54140.c:834:13: error: 'PHY_POLL_CABLE_TEST' undeclared here (not in a function)
      .flags  = PHY_POLL_CABLE_TEST,
                ^~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/bcm54140.c:846:4: error: 'struct phy_driver' has no member named 'cable_test_start'
      .cable_test_start = bcm_phy_cable_test_start_rdb,
       ^~~~~~~~~~~~~~~~
>> drivers/net/phy/bcm54140.c:846:23: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
      .cable_test_start = bcm_phy_cable_test_start_rdb,
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:846:23: note: (near initialization for 'bcm54140_drivers[0].set_loopback')
>> drivers/net/phy/bcm54140.c:847:4: error: 'struct phy_driver' has no member named 'cable_test_get_status'
      .cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
       ^~~~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/bcm54140.c:847:28: warning: excess elements in struct initializer
      .cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/bcm54140.c:847:28: note: (near initialization for 'bcm54140_drivers[0]')
   cc1: some warnings being treated as errors

vim +/PHY_POLL_CABLE_TEST +834 drivers/net/phy/bcm54140.c

   828	
   829	static struct phy_driver bcm54140_drivers[] = {
   830		{
   831			.phy_id         = PHY_ID_BCM54140,
   832			.phy_id_mask    = BCM54140_PHY_ID_MASK,
   833			.name           = "Broadcom BCM54140",
 > 834			.flags		= PHY_POLL_CABLE_TEST,
   835			.features       = PHY_GBIT_FEATURES,
   836			.config_init    = bcm54140_config_init,
   837			.did_interrupt	= bcm54140_did_interrupt,
   838			.ack_interrupt  = bcm54140_ack_intr,
   839			.config_intr    = bcm54140_config_intr,
   840			.probe		= bcm54140_probe,
   841			.suspend	= genphy_suspend,
   842			.resume		= genphy_resume,
   843			.soft_reset	= genphy_soft_reset,
   844			.get_tunable	= bcm54140_get_tunable,
   845			.set_tunable	= bcm54140_set_tunable,
 > 846			.cable_test_start = bcm_phy_cable_test_start_rdb,
 > 847			.cable_test_get_status = bcm_phy_cable_test_get_status_rdb,
   848		},
   849	};
   850	module_phy_driver(bcm54140_drivers);
   851	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 72353 bytes --]

Re: [PATCH net-next 4/4] net: phy: bcm54140: add cable diagnostics support

[Andrew Lunn]

On Sun, May 10, 2020 at 12:37:14AM +0200, Michael Walle wrote:
> Use the generic cable tester functions from bcm-phy-lib to add cable
> tester support.
> 
> 100m cable, A/B/C/D open:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: Open Circuit
>   Pair: Pair B, result: Open Circuit
>   Pair: Pair C, result: Open Circuit
>   Pair: Pair D, result: Open Circuit
>   Pair: Pair A, fault length: 106.60m
>   Pair: Pair B, fault length: 103.32m
>   Pair: Pair C, fault length: 104.96m
>   Pair: Pair D, fault length: 106.60m
> 
> 1m cable, A/B connected, pair C shorted, D open:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: OK
>   Pair: Pair B, result: OK
>   Pair: Pair C, result: Short within Pair
>   Pair: Pair D, result: Open Circuit
>   Pair: Pair C, fault length: 0.82m
>   Pair: Pair D, fault length: 1.64m
> 
> 1m cable, A/B connected, pair C shorted with D:
>   Cable test started for device eth0.
>   Cable test completed for device eth0.
>   Pair: Pair A, result: OK
>   Pair: Pair B, result: OK
>   Pair: Pair C, result: Short to another pair
>   Pair: Pair D, result: Short to another pair
>   Pair: Pair C, fault length: 1.64m
>   Pair: Pair D, fault length: 1.64m
> 
> The granularity of the length measurement seems to be 82cm.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 0/4] net: phy: broadcom: cable tester support

[Jakub Kicinski]

On Sun, 10 May 2020 00:37:10 +0200 Michael Walle wrote:
> Add cable tester support for the Broadcom PHYs. Support for it was
> developed on a BCM54140 Quad PHY which RDB register access.
> 
> If there is a link partner the results are not as good as with an open
> cable. I guess we could retry if the measurement until all pairs had at
> least one valid result.
> 
> Please note that this patch depends on Andrew's latest cable testing
> series:
> https://lore.kernel.org/netdev/20200509162851.362346-1-andrew@lunn.ch/

Thanks for posting the patches early Michael, it's definitely useful,
but I'd advise to post as RFC before the dependency is in the tree.
CI bots won't understand English well enough to take care of this
dependency, so we're loosing whatever test coverage they provide..

Please repost once Andrew's patches get merged, I'm marking these as
RFC in patchwork for now.