[RFC net-next] net: phy: at803x: add cable diagnostics support

[RFC net-next] net: phy: at803x: add cable diagnostics support

[Michael Walle]

The AR8031/AR8033 and the AR8035 support cable diagnostics. Adding
driver support is straightforward, so lets add it.

The PHY just do one pair at a time, so we have to start the test four
times. The cable_test_get_status() can block and therefore we can just
busy polling the test completion and continue with the next pair until
we are done.
The time delta counter seems to run with 125MHz which just gives us a
resolution of about 82.4cm per tick.

This was tested with a 100m ethernet cable, a 1m cable and a broken
1m cable where pair 3 was shorted and pair 4 were open. Leaving the
100m cable unconnected resulted in a measured fault distance of about
114m. For the short cable the result the measured distance is either
82cm or 1.64m.

If the cable is connected to an active peer, the results might be
inconclusive if there are broken cables. I guess this is because
the remote PHY will disturb the measurement. Using an intact ethernet
cable the restart of the auto negotiation seem to cause the remote PHY
to stop sending distrubance signals.

Signed-off-by: Michael Walle <michael@walle.cc>
---
This is an RFC because the patch requires Andrew's cable test support:
https://lore.kernel.org/netdev/20200425180621.1140452-1-andrew@lunn.ch/

 drivers/net/phy/at803x.c | 167 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 167 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 76eb5a77fc5c..978f1b2fb931 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -18,6 +18,8 @@
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/consumer.h>
+#include <linux/ethtool_netlink.h>
+#include <uapi/linux/ethtool_netlink.h>
 #include <dt-bindings/net/qca-ar803x.h>
 
 #define AT803X_SPECIFIC_STATUS			0x11
@@ -46,6 +48,16 @@
 #define AT803X_SMART_SPEED_ENABLE		BIT(5)
 #define AT803X_SMART_SPEED_RETRY_LIMIT_MASK	GENMASK(4, 2)
 #define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
+#define AT803X_CDT				0x16
+#define AT803X_CDT_MDI_PAIR_MASK		GENMASK(9, 8)
+#define AT803X_CDT_ENABLE_TEST			BIT(0)
+#define AT803X_CDT_STATUS			0x1c
+#define AT803X_CDT_STATUS_STAT_NORMAL		0
+#define AT803X_CDT_STATUS_STAT_SHORT		1
+#define AT803X_CDT_STATUS_STAT_OPEN		2
+#define AT803X_CDT_STATUS_STAT_FAIL		3
+#define AT803X_CDT_STATUS_STAT_MASK		GENMASK(9, 8)
+#define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
 #define AT803X_LED_CONTROL			0x18
 
 #define AT803X_DEVICE_ADDR			0x03
@@ -110,6 +122,8 @@
 #define AT803X_MIN_DOWNSHIFT 2
 #define AT803X_MAX_DOWNSHIFT 9
 
+#define AT803X_CDT_DELAY_MS 1500
+
 #define ATH9331_PHY_ID 0x004dd041
 #define ATH8030_PHY_ID 0x004dd076
 #define ATH8031_PHY_ID 0x004dd074
@@ -129,6 +143,7 @@ struct at803x_priv {
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
+	unsigned long cdt_start;
 };
 
 struct at803x_context {
@@ -794,11 +809,158 @@ static int at803x_set_tunable(struct phy_device *phydev,
 	}
 }
 
+static int at803x_cdt_test_result(u16 status)
+{
+	switch (FIELD_GET(AT803X_CDT_STATUS_STAT_MASK, status)) {
+	case AT803X_CDT_STATUS_STAT_NORMAL:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case AT803X_CDT_STATUS_STAT_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case AT803X_CDT_STATUS_STAT_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case AT803X_CDT_STATUS_STAT_FAIL:
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static bool at803x_cdt_fault_length_valid(u16 status)
+{
+	switch (FIELD_GET(AT803X_CDT_STATUS_STAT_MASK, status)) {
+	case AT803X_CDT_STATUS_STAT_OPEN:
+	case AT803X_CDT_STATUS_STAT_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int at803x_cdt_fault_length(u16 status)
+{
+	int dt;
+
+	/* According to the datasheet the distance to the fault is
+	 * DELTA_TIME * 0.824 meters.
+	 *
+	 * The author suspect the correct formula is:
+	 *
+	 *   fault_distance = DELTA_TIME * (c * VF) / 125MHz / 2
+	 *
+	 * where c is the speed of light, VF is the velocity factor of
+	 * the twisted pair cable, 125MHz the counter frequency and
+	 * we need to divide by 2 because the hardware will measure the
+	 * round trip time to the fault and back to the PHY.
+	 *
+	 * With a VF of 0.69 we get the factor 0.824 mentioned in the
+	 * datasheet.
+	 */
+	dt = FIELD_GET(AT803X_CDT_STATUS_DELTA_TIME_MASK, status);
+
+	return (dt * 824) / 10;
+}
+
+static int at803x_cdt_start(struct phy_device *phydev, int pair)
+{
+	u16 cdt;
+
+	cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
+	      AT803X_CDT_ENABLE_TEST;
+
+	return phy_write(phydev, AT803X_CDT, cdt);
+}
+
+static int at803x_cdt_wait_for_completion(struct phy_device *phydev)
+{
+	int val, ret;
+
+	/* One test run takes about 25ms */
+	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
+				    !(val & AT803X_CDT_ENABLE_TEST),
+				    30000, 100000, true);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int at803x_cable_test_get_status(struct phy_device *phydev,
+					bool *finished)
+{
+	struct at803x_priv *priv = phydev->priv;
+	static const int ethtool_pair[] = {
+		ETHTOOL_A_CABLE_PAIR_0, ETHTOOL_A_CABLE_PAIR_1,
+		ETHTOOL_A_CABLE_PAIR_2, ETHTOOL_A_CABLE_PAIR_3};
+	int pair, val, ret;
+	unsigned int delay_ms;
+
+	*finished = false;
+
+	if (priv->cdt_start) {
+		delay_ms = AT803X_CDT_DELAY_MS;
+		delay_ms -= jiffies_delta_to_msecs(jiffies - priv->cdt_start);
+		if (delay_ms > 0)
+			msleep(delay_ms);
+	}
+
+	for (pair = 0; pair < 4; pair++) {
+		ret = at803x_cdt_start(phydev, pair);
+		if (ret)
+			return ret;
+
+		ret = at803x_cdt_wait_for_completion(phydev);
+		if (ret)
+			return ret;
+
+		val = phy_read(phydev, AT803X_CDT_STATUS);
+		if (val < 0)
+			return val;
+
+		ethnl_cable_test_result(phydev, ethtool_pair[pair],
+					at803x_cdt_test_result(val));
+
+		if (at803x_cdt_fault_length_valid(val))
+			continue;
+
+		ethnl_cable_test_fault_length(phydev, ethtool_pair[pair],
+					      at803x_cdt_fault_length(val));
+	}
+
+	*finished = true;
+
+	return 0;
+}
+
+static int at803x_cable_test_start(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+	int bmsr, ret;
+
+	/* According to the datasheet the CDT can be performed when
+	 * there is no link partner or when the link partner is
+	 * auto-negotiating. Thus restart the AN before starting the
+	 * test. Manual tests have shown, that the we have to wait
+	 * between 1s and 3s after restarting the AN to have reliable
+	 * results.
+	 */
+	bmsr = phy_read(phydev, MII_BMSR);
+	if (bmsr < 0)
+		return bmsr;
+
+	if (bmsr & BMSR_LSTATUS) {
+		ret = genphy_restart_aneg(phydev);
+		if (ret)
+			return ret;
+		priv->cdt_start = jiffies;
+	} else {
+		priv->cdt_start = 0;
+	}
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
 	PHY_ID_MATCH_EXACT(ATH8035_PHY_ID),
 	.name			= "Qualcomm Atheros AR8035",
+	.flags			= PHY_POLL_CABLE_TEST,
 	.probe			= at803x_probe,
 	.remove			= at803x_remove,
 	.config_init		= at803x_config_init,
@@ -813,6 +975,8 @@ static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
+	.cable_test_start	= at803x_cable_test_start,
+	.cable_test_get_status	= at803x_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,
@@ -833,6 +997,7 @@ static struct phy_driver at803x_driver[] = {
 	/* Qualcomm Atheros AR8031/AR8033 */
 	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
 	.name			= "Qualcomm Atheros AR8031/AR8033",
+	.flags			= PHY_POLL_CABLE_TEST,
 	.probe			= at803x_probe,
 	.remove			= at803x_remove,
 	.config_init		= at803x_config_init,
@@ -848,6 +1013,8 @@ static struct phy_driver at803x_driver[] = {
 	.config_intr		= &at803x_config_intr,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
+	.cable_test_start	= at803x_cable_test_start,
+	.cable_test_get_status	= at803x_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
-- 
2.20.1

Re: [RFC net-next] net: phy: at803x: add cable diagnostics support

[Andrew Lunn]

> +static int at803x_cable_test_get_status(struct phy_device *phydev,
> +					bool *finished)
> +{
> +	struct at803x_priv *priv = phydev->priv;
> +	static const int ethtool_pair[] = {
> +		ETHTOOL_A_CABLE_PAIR_0, ETHTOOL_A_CABLE_PAIR_1,
> +		ETHTOOL_A_CABLE_PAIR_2, ETHTOOL_A_CABLE_PAIR_3};

If you put one per line, you will keep the reverse christmas tree, and
David will be happy.

> +	int pair, val, ret;
> +	unsigned int delay_ms;

Well, David will be happy if you move this as well.

> +	*finished = false;
> +
> +	if (priv->cdt_start) {
> +		delay_ms = AT803X_CDT_DELAY_MS;
> +		delay_ms -= jiffies_delta_to_msecs(jiffies - priv->cdt_start);
> +		if (delay_ms > 0)
> +			msleep(delay_ms);
> +	}
> +
> +	for (pair = 0; pair < 4; pair++) {
> +		ret = at803x_cdt_start(phydev, pair);
> +		if (ret)
> +			return ret;
> +
> +		ret = at803x_cdt_wait_for_completion(phydev);
> +		if (ret)
> +			return ret;
> +
> +		val = phy_read(phydev, AT803X_CDT_STATUS);
> +		if (val < 0)
> +			return val;
> +
> +		ethnl_cable_test_result(phydev, ethtool_pair[pair],
> +					at803x_cdt_test_result(val));
> +
> +		if (at803x_cdt_fault_length_valid(val))
> +			continue;

The name is not very intuitive. It return false if it is valid?

Otherwise, this looks good.

    Andrew

Re: [RFC net-next] net: phy: at803x: add cable diagnostics support

[Michael Walle]

Am 2020-05-05 15:07, schrieb Andrew Lunn:
>> +static int at803x_cable_test_get_status(struct phy_device *phydev,
>> +					bool *finished)
>> +{
>> +	struct at803x_priv *priv = phydev->priv;
>> +	static const int ethtool_pair[] = {
>> +		ETHTOOL_A_CABLE_PAIR_0, ETHTOOL_A_CABLE_PAIR_1,
>> +		ETHTOOL_A_CABLE_PAIR_2, ETHTOOL_A_CABLE_PAIR_3};
> 
> If you put one per line, you will keep the reverse christmas tree, and
> David will be happy.
> 
>> +	int pair, val, ret;
>> +	unsigned int delay_ms;
> 
> Well, David will be happy if you move this as well.

Damn, this should really be a checkpatch.pl check ;) It was "int 
delay_ms;"
before, then it was changed to "unsigned int delay_ms;"..

> 
>> +	*finished = false;
>> +
>> +	if (priv->cdt_start) {
>> +		delay_ms = AT803X_CDT_DELAY_MS;
>> +		delay_ms -= jiffies_delta_to_msecs(jiffies - priv->cdt_start);
>> +		if (delay_ms > 0)
>> +			msleep(delay_ms);
>> +	}
>> +
>> +	for (pair = 0; pair < 4; pair++) {
>> +		ret = at803x_cdt_start(phydev, pair);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = at803x_cdt_wait_for_completion(phydev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		val = phy_read(phydev, AT803X_CDT_STATUS);
>> +		if (val < 0)
>> +			return val;
>> +
>> +		ethnl_cable_test_result(phydev, ethtool_pair[pair],
>> +					at803x_cdt_test_result(val));
>> +
>> +		if (at803x_cdt_fault_length_valid(val))
>> +			continue;
> 
> The name is not very intuitive. It return false if it is valid?

Mhh, this is actually wrong, it returns true if the length is
valid. I need to double check that. what about
at803x_cdt_fault_length_is_valid()

> Otherwise, this looks good.

I'll wait for your v3 and then I'll rebase on that.

-michael

Re: [RFC net-next] net: phy: at803x: add cable diagnostics support

[Matthias May]

On 03/05/2020 20:15, Michael Walle wrote:
> The AR8031/AR8033 and the AR8035 support cable diagnostics. Adding
> driver support is straightforward, so lets add it.
>
> The PHY just do one pair at a time, so we have to start the test four
> times. The cable_test_get_status() can block and therefore we can just
> busy polling the test completion and continue with the next pair until
> we are done.
> The time delta counter seems to run with 125MHz which just gives us a
> resolution of about 82.4cm per tick.
>
> This was tested with a 100m ethernet cable, a 1m cable and a broken
> 1m cable where pair 3 was shorted and pair 4 were open. Leaving the
> 100m cable unconnected resulted in a measured fault distance of about
> 114m. For the short cable the result the measured distance is either
> 82cm or 1.64m.
>
> If the cable is connected to an active peer, the results might be
> inconclusive if there are broken cables. I guess this is because
> the remote PHY will disturb the measurement. Using an intact ethernet
> cable the restart of the auto negotiation seem to cause the remote PHY
> to stop sending distrubance signals.
>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> This is an RFC because the patch requires Andrew's cable test support:
> https://lore.kernel.org/netdev/20200425180621.1140452-1-andrew@lunn.ch/
>
>  drivers/net/phy/at803x.c | 167 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 167 insertions(+)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 76eb5a77fc5c..978f1b2fb931 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -18,6 +18,8 @@
>  #include <linux/regulator/of_regulator.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/ethtool_netlink.h>
> +#include <uapi/linux/ethtool_netlink.h>
>  #include <dt-bindings/net/qca-ar803x.h>
>  
>  #define AT803X_SPECIFIC_STATUS			0x11
> @@ -46,6 +48,16 @@
>  #define AT803X_SMART_SPEED_ENABLE		BIT(5)
>  #define AT803X_SMART_SPEED_RETRY_LIMIT_MASK	GENMASK(4, 2)
>  #define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
> +#define AT803X_CDT				0x16
> +#define AT803X_CDT_MDI_PAIR_MASK		GENMASK(9, 8)
> +#define AT803X_CDT_ENABLE_TEST			BIT(0)
> +#define AT803X_CDT_STATUS			0x1c
> +#define AT803X_CDT_STATUS_STAT_NORMAL		0
> +#define AT803X_CDT_STATUS_STAT_SHORT		1
> +#define AT803X_CDT_STATUS_STAT_OPEN		2
> +#define AT803X_CDT_STATUS_STAT_FAIL		3
> +#define AT803X_CDT_STATUS_STAT_MASK		GENMASK(9, 8)
> +#define AT803X_CDT_STATUS_DELTA_TIME_MASK	GENMASK(7, 0)
>  #define AT803X_LED_CONTROL			0x18
>  
>  #define AT803X_DEVICE_ADDR			0x03
> @@ -110,6 +122,8 @@
>  #define AT803X_MIN_DOWNSHIFT 2
>  #define AT803X_MAX_DOWNSHIFT 9
>  
> +#define AT803X_CDT_DELAY_MS 1500
> +
>  #define ATH9331_PHY_ID 0x004dd041
>  #define ATH8030_PHY_ID 0x004dd076
>  #define ATH8031_PHY_ID 0x004dd074
> @@ -129,6 +143,7 @@ struct at803x_priv {
>  	struct regulator_dev *vddio_rdev;
>  	struct regulator_dev *vddh_rdev;
>  	struct regulator *vddio;
> +	unsigned long cdt_start;
>  };
>  
>  struct at803x_context {
> @@ -794,11 +809,158 @@ static int at803x_set_tunable(struct phy_device *phydev,
>  	}
>  }
>  
> +static int at803x_cdt_test_result(u16 status)
> +{
> +	switch (FIELD_GET(AT803X_CDT_STATUS_STAT_MASK, status)) {
> +	case AT803X_CDT_STATUS_STAT_NORMAL:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> +	case AT803X_CDT_STATUS_STAT_SHORT:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
> +	case AT803X_CDT_STATUS_STAT_OPEN:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> +	case AT803X_CDT_STATUS_STAT_FAIL:
> +	default:
> +		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
> +	}
> +}
> +
> +static bool at803x_cdt_fault_length_valid(u16 status)
> +{
> +	switch (FIELD_GET(AT803X_CDT_STATUS_STAT_MASK, status)) {
> +	case AT803X_CDT_STATUS_STAT_OPEN:
> +	case AT803X_CDT_STATUS_STAT_SHORT:
> +		return true;
> +	}
> +	return false;
> +}
> +
> +static int at803x_cdt_fault_length(u16 status)
> +{
> +	int dt;
> +
> +	/* According to the datasheet the distance to the fault is
> +	 * DELTA_TIME * 0.824 meters.
> +	 *
> +	 * The author suspect the correct formula is:
> +	 *
> +	 *   fault_distance = DELTA_TIME * (c * VF) / 125MHz / 2
> +	 *
> +	 * where c is the speed of light, VF is the velocity factor of
> +	 * the twisted pair cable, 125MHz the counter frequency and
> +	 * we need to divide by 2 because the hardware will measure the
> +	 * round trip time to the fault and back to the PHY.
> +	 *
> +	 * With a VF of 0.69 we get the factor 0.824 mentioned in the
> +	 * datasheet.
> +	 */
> +	dt = FIELD_GET(AT803X_CDT_STATUS_DELTA_TIME_MASK, status);
> +
> +	return (dt * 824) / 10;
> +}
> +
> +static int at803x_cdt_start(struct phy_device *phydev, int pair)
> +{
> +	u16 cdt;
> +
> +	cdt = FIELD_PREP(AT803X_CDT_MDI_PAIR_MASK, pair) |
> +	      AT803X_CDT_ENABLE_TEST;
> +
> +	return phy_write(phydev, AT803X_CDT, cdt);
> +}
> +
> +static int at803x_cdt_wait_for_completion(struct phy_device *phydev)
> +{
> +	int val, ret;
> +
> +	/* One test run takes about 25ms */
> +	ret = phy_read_poll_timeout(phydev, AT803X_CDT, val,
> +				    !(val & AT803X_CDT_ENABLE_TEST),
> +				    30000, 100000, true);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int at803x_cable_test_get_status(struct phy_device *phydev,
> +					bool *finished)
> +{
> +	struct at803x_priv *priv = phydev->priv;
> +	static const int ethtool_pair[] = {
> +		ETHTOOL_A_CABLE_PAIR_0, ETHTOOL_A_CABLE_PAIR_1,
> +		ETHTOOL_A_CABLE_PAIR_2, ETHTOOL_A_CABLE_PAIR_3};
> +	int pair, val, ret;
> +	unsigned int delay_ms;
> +
> +	*finished = false;
> +
> +	if (priv->cdt_start) {
> +		delay_ms = AT803X_CDT_DELAY_MS;
> +		delay_ms -= jiffies_delta_to_msecs(jiffies - priv->cdt_start);
> +		if (delay_ms > 0)
> +			msleep(delay_ms);
> +	}
> +
> +	for (pair = 0; pair < 4; pair++) {
> +		ret = at803x_cdt_start(phydev, pair);
> +		if (ret)
> +			return ret;
> +
> +		ret = at803x_cdt_wait_for_completion(phydev);
> +		if (ret)
> +			return ret;
> +
> +		val = phy_read(phydev, AT803X_CDT_STATUS);
> +		if (val < 0)
> +			return val;
> +
> +		ethnl_cable_test_result(phydev, ethtool_pair[pair],
> +					at803x_cdt_test_result(val));
> +
> +		if (at803x_cdt_fault_length_valid(val))
> +			continue;
> +
> +		ethnl_cable_test_fault_length(phydev, ethtool_pair[pair],
> +					      at803x_cdt_fault_length(val));
> +	}
> +
> +	*finished = true;
> +
> +	return 0;
> +}
> +
> +static int at803x_cable_test_start(struct phy_device *phydev)
> +{
> +	struct at803x_priv *priv = phydev->priv;
> +	int bmsr, ret;
> +
> +	/* According to the datasheet the CDT can be performed when
> +	 * there is no link partner or when the link partner is
> +	 * auto-negotiating. Thus restart the AN before starting the
> +	 * test. Manual tests have shown, that the we have to wait
> +	 * between 1s and 3s after restarting the AN to have reliable
> +	 * results.
> +	 */
> +	bmsr = phy_read(phydev, MII_BMSR);
> +	if (bmsr < 0)
> +		return bmsr;
> +
> +	if (bmsr & BMSR_LSTATUS) {
> +		ret = genphy_restart_aneg(phydev);
> +		if (ret)
> +			return ret;
> +		priv->cdt_start = jiffies;
> +	} else {
> +		priv->cdt_start = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct phy_driver at803x_driver[] = {
>  {
>  	/* Qualcomm Atheros AR8035 */
>  	PHY_ID_MATCH_EXACT(ATH8035_PHY_ID),
>  	.name			= "Qualcomm Atheros AR8035",
> +	.flags			= PHY_POLL_CABLE_TEST,
>  	.probe			= at803x_probe,
>  	.remove			= at803x_remove,
>  	.config_init		= at803x_config_init,
> @@ -813,6 +975,8 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> +	.cable_test_start	= at803x_cable_test_start,
> +	.cable_test_get_status	= at803x_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8030 */
>  	.phy_id			= ATH8030_PHY_ID,
> @@ -833,6 +997,7 @@ static struct phy_driver at803x_driver[] = {
>  	/* Qualcomm Atheros AR8031/AR8033 */
>  	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
>  	.name			= "Qualcomm Atheros AR8031/AR8033",
> +	.flags			= PHY_POLL_CABLE_TEST,
>  	.probe			= at803x_probe,
>  	.remove			= at803x_remove,
>  	.config_init		= at803x_config_init,
> @@ -848,6 +1013,8 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= &at803x_config_intr,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> +	.cable_test_start	= at803x_cable_test_start,
> +	.cable_test_get_status	= at803x_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8032 */
>  	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),

Hi
I've worked with this PHY quite often and we've hacked together some support for the CDT in uboot.

Have you done any tests with the cable on the other side being plugged in?

With the cable being plugged in, we something (1 out of 10 or so) observed that the test returns with a failure.
--> AT803X_CDT_STATUS_STAT_FAIL in AT803X_CDT_STATUS

Often you get the correct result if you simply try again. Sometimes however it gets into a "stuck" state where it just
returns FAIL for ~3~10 seconds. After some that it seems to recover and gets the correct result again.

BR
Matthias

Re: [RFC net-next] net: phy: at803x: add cable diagnostics support

[Michael Walle]

Am 2020-05-06 11:01, schrieb Matthias May:
> I've worked with this PHY quite often and we've hacked together some
> support for the CDT in uboot.
> 
> Have you done any tests with the cable on the other side being plugged 
> in?

yes

> 
> With the cable being plugged in, we something (1 out of 10 or so)
> observed that the test returns with a failure.
> --> AT803X_CDT_STATUS_STAT_FAIL in AT803X_CDT_STATUS
> 
> Often you get the correct result if you simply try again. Sometimes
> however it gets into a "stuck" state where it just
> returns FAIL for ~3~10 seconds. After some that it seems to recover
> and gets the correct result again.

its actually pretty stable for the following sequence (see also code
above):

restart AN -> wait 1.5s -> start test

Only thing I've noticed is that if you perform the test without
waiting for the AN to complete beforehand there might be some
failed states. Seems like the "restart an" doesn't work while
AN is still running. But that seems to be the link partner
who disturbs the measurement.
And it seems that AN is a requirement to do successful testing
(or to silence the link partner I guess).

-michael