[PATCH v1] net: phy: at803x: add cable test support

Subsystems: ethernet phy library, networking drivers, the rest

STALE2273d

9 messages, 5 authors, 2020-05-13 · open the first message on its own page

[PATCH v1] net: phy: at803x: add cable test support

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2020-05-13 12:06:56

The cable test seems to be support by all of currently support Atherso
PHYs, so add support for all of them. This patch was tested only on
AR9331 PHY with following results:
- No cable is detected as short
- A 15m long cable connected only on one side is detected as 9m open.
- A cable test with active link partner will provide no usable results.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/at803x.c | 141 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f4fec5f644e91..03ec500defb34 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -7,11 +7,13 @@
  * Author: Matus Ujhelyi <ujhelyi.m@gmail.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/of_gpio.h>
 #include <linux/bitfield.h>
 #include <linux/gpio/consumer.h>
@@ -48,6 +50,20 @@
 #define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
 #define AT803X_LED_CONTROL			0x18
 
+/* Cable Tester Contol Register */
+#define AT803X_CABLE_DIAG_CTRL			0x16
+#define AT803X_CABLE_DIAG_MDI_PAIR		GENMASK(9, 8)
+#define AT803X_CABLE_DIAG_EN			BIT(0)
+
+/* Cable Tester Status Register */
+#define AT803X_CABLE_DIAG_STATUS		0x1c
+#define AT803X_CABLE_DIAG_RESULT		GENMASK(9, 8)
+#define AT803X_CABLE_DIAG_RESULT_OK		0
+#define AT803X_CABLE_DIAG_RESULT_SHORT		1
+#define AT803X_CABLE_DIAG_RESULT_OPEN		2
+#define AT803X_CABLE_DIAG_RESULT_FAIL		3
+#define AT803X_CABLE_DIAG_DTIME			GENMASK(7, 0)
+
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
 #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
@@ -122,6 +138,7 @@ MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
 struct at803x_priv {
+	struct phy_device *phydev;
 	int flags;
 #define AT803X_KEEP_PLL_ENABLED	BIT(0)	/* don't turn off internal PLL */
 	u16 clk_25m_reg;
@@ -129,6 +146,9 @@ struct at803x_priv {
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
+	struct work_struct cable_test_work;
+	bool cable_test_finished;
+	int cable_test_ret;
 };
 
 struct at803x_context {
@@ -168,6 +188,113 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
 	return phy_write(phydev, AT803X_DEBUG_DATA, val);
 }
 
+
+static bool at803x_distance_valid(int result)
+{
+	switch (result) {
+	case AT803X_CABLE_DIAG_RESULT_OPEN:
+	case AT803X_CABLE_DIAG_RESULT_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int at803x_cable_test_report_trans(int result)
+{
+	switch (result) {
+	case AT803X_CABLE_DIAG_RESULT_OK:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case AT803X_CABLE_DIAG_RESULT_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case AT803X_CABLE_DIAG_RESULT_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static int at803x_cable_test_pair(struct phy_device *phydev, int pair)
+{
+	int result, dtime;
+	int ret, val;
+
+	val = FIELD_PREP(AT803X_CABLE_DIAG_MDI_PAIR, pair)
+		| AT803X_CABLE_DIAG_EN;
+
+	phy_write(phydev, AT803X_CABLE_DIAG_CTRL, val);
+
+	ret = phy_read_poll_timeout(phydev, AT803X_CABLE_DIAG_CTRL, val,
+				    !(val & AT803X_CABLE_DIAG_EN),
+				    50000, 600000, true);
+	if (ret) {
+		phydev_err(phydev, "waiting for cable test results filed\n");
+		return ret;
+	}
+
+	ret = phy_read(phydev, AT803X_CABLE_DIAG_STATUS);
+	if (ret < 0)
+		return ret;
+
+	result = FIELD_GET(AT803X_CABLE_DIAG_RESULT, ret);
+
+	ethnl_cable_test_result(phydev, pair,
+				at803x_cable_test_report_trans(result));
+
+	if (at803x_distance_valid(result)) {
+		dtime = FIELD_GET(AT803X_CABLE_DIAG_DTIME, ret) * 824 / 10;
+		ethnl_cable_test_fault_length(phydev, pair, dtime);
+	}
+
+	return 0;
+}
+
+static int at803x_cable_test_get_status(struct phy_device *phydev,
+					      bool *finished)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	*finished = priv->cable_test_finished;
+
+	return 0;
+}
+
+static void at803x_cable_test_work(struct work_struct *work)
+{
+	struct at803x_priv *priv = container_of(work, struct at803x_priv,
+						cable_test_work);
+	struct phy_device *phydev = priv->phydev;
+	int i, ret = 0, pairs = 4;
+
+	if (phydev->phy_id == ATH9331_PHY_ID)
+		pairs = 2;
+
+	for (i = 0; i < pairs; i++) {
+		ret = at803x_cable_test_pair(phydev, i);
+		if (ret)
+			break;
+	}
+
+	priv->cable_test_ret = ret;
+	priv->cable_test_finished = true;
+
+	phy_queue_state_machine(phydev, 0);
+}
+
+static int at803x_cable_test_start(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	if (!priv->cable_test_finished) {
+		phydev_err(phydev, "cable test is already running\n");
+		return -EIO;
+	}
+
+	priv->cable_test_finished = false;
+	schedule_work(&priv->cable_test_work);
+
+	return 0;
+}
+
 static int at803x_enable_rx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
@@ -492,7 +619,10 @@ static int at803x_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->phydev = phydev;
 	phydev->priv = priv;
+	priv->cable_test_finished = true;
+	INIT_WORK(&priv->cable_test_work, at803x_cable_test_work);
 
 	return at803x_parse_dt(phydev);
 }
@@ -814,6 +944,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,
@@ -830,6 +962,8 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.cable_test_start	= at803x_cable_test_start,
+	.cable_test_get_status	= at803x_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8031/AR8033 */
 	.phy_id			= ATH8031_PHY_ID,
@@ -850,6 +984,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),
@@ -865,15 +1001,20 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= at803x_ack_interrupt,
 	.config_intr		= at803x_config_intr,
+	.cable_test_start	= at803x_cable_test_start,
+	.cable_test_get_status	= at803x_cable_test_get_status,
 }, {
 	/* ATHEROS AR9331 */
 	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
 	.name			= "Qualcomm Atheros AR9331 built-in PHY",
+	.probe			= at803x_probe,
 	.suspend		= at803x_suspend,
 	.resume			= at803x_resume,
 	/* PHY_BASIC_FEATURES */
 	.ack_interrupt		= &at803x_ack_interrupt,
 	.config_intr		= &at803x_config_intr,
+	.cable_test_start	= at803x_cable_test_start,
+	.cable_test_get_status	= at803x_cable_test_get_status,
 } };
 
 module_phy_driver(at803x_driver);
-- 
2.26.2

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Andrew Lunn <andrew@lunn.ch>
Date: 2020-05-13 13:32:15

On Wed, May 13, 2020 at 02:06:48PM +0200, Oleksij Rempel wrote:
The cable test seems to be support by all of currently support Atherso
PHYs, so add support for all of them. This patch was tested only on
AR9331 PHY with following results:
- No cable is detected as short
- A 15m long cable connected only on one side is detected as 9m open.
That sounds wrong. What about a shorted 15m cable? Is it also 9m?  Do
you have any other long cables you can test with? Is it always 1/2 the
cable length?
quoted hunk
- A cable test with active link partner will provide no usable results.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/at803x.c | 141 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f4fec5f644e91..03ec500defb34 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -7,11 +7,13 @@
  * Author: Matus Ujhelyi <ujhelyi.m@gmail.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/of_gpio.h>
 #include <linux/bitfield.h>
 #include <linux/gpio/consumer.h>
@@ -48,6 +50,20 @@
 #define AT803X_SMART_SPEED_BYPASS_TIMER		BIT(1)
 #define AT803X_LED_CONTROL			0x18
 
+/* Cable Tester Contol Register */
+#define AT803X_CABLE_DIAG_CTRL			0x16
+#define AT803X_CABLE_DIAG_MDI_PAIR		GENMASK(9, 8)
+#define AT803X_CABLE_DIAG_EN			BIT(0)
+
+/* Cable Tester Status Register */
+#define AT803X_CABLE_DIAG_STATUS		0x1c
+#define AT803X_CABLE_DIAG_RESULT		GENMASK(9, 8)
+#define AT803X_CABLE_DIAG_RESULT_OK		0
+#define AT803X_CABLE_DIAG_RESULT_SHORT		1
+#define AT803X_CABLE_DIAG_RESULT_OPEN		2
+#define AT803X_CABLE_DIAG_RESULT_FAIL		3
+#define AT803X_CABLE_DIAG_DTIME			GENMASK(7, 0)
+
 #define AT803X_DEVICE_ADDR			0x03
 #define AT803X_LOC_MAC_ADDR_0_15_OFFSET		0x804C
 #define AT803X_LOC_MAC_ADDR_16_31_OFFSET	0x804B
@@ -122,6 +138,7 @@ MODULE_AUTHOR("Matus Ujhelyi");
 MODULE_LICENSE("GPL");
 
 struct at803x_priv {
+	struct phy_device *phydev;
 	int flags;
 #define AT803X_KEEP_PLL_ENABLED	BIT(0)	/* don't turn off internal PLL */
 	u16 clk_25m_reg;
@@ -129,6 +146,9 @@ struct at803x_priv {
 	struct regulator_dev *vddio_rdev;
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
+	struct work_struct cable_test_work;
+	bool cable_test_finished;
+	int cable_test_ret;
 };
 
 struct at803x_context {
@@ -168,6 +188,113 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
 	return phy_write(phydev, AT803X_DEBUG_DATA, val);
 }
  
+static void at803x_cable_test_work(struct work_struct *work)
+{
+	struct at803x_priv *priv = container_of(work, struct at803x_priv,
+						cable_test_work);
+	struct phy_device *phydev = priv->phydev;
+	int i, ret = 0, pairs = 4;
+
+	if (phydev->phy_id == ATH9331_PHY_ID)
+		pairs = 2;
+
+	for (i = 0; i < pairs; i++) {
+		ret = at803x_cable_test_pair(phydev, i);
+		if (ret)
+			break;
+	}
+
+	priv->cable_test_ret = ret;
+	priv->cable_test_finished = true;
+
+	phy_queue_state_machine(phydev, 0);
+}
+
+static int at803x_cable_test_start(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	if (!priv->cable_test_finished) {
+		phydev_err(phydev, "cable test is already running\n");
+		return -EIO;
+	}
+
+	priv->cable_test_finished = false;
+	schedule_work(&priv->cable_test_work);
You don't need the work queue. You cannot spend a long time in
at803x_cable_test_start(), but you can in
at803x_cable_test_get_status(). So start the first the measurement of
the first pair in at803x_cable_test_start(), and do the rest in
of the work in at803x_cable_test_get_status().

If we see there is a common pattern of wanting
at803x_cable_test_get_status() to run immediately, not one second
later, we can change to core to support that.

       Andrew

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: 2020-05-13 13:50:16

On Wed, May 13, 2020 at 03:32:09PM +0200, Andrew Lunn wrote:
On Wed, May 13, 2020 at 02:06:48PM +0200, Oleksij Rempel wrote:
quoted
The cable test seems to be support by all of currently support Atherso
PHYs, so add support for all of them. This patch was tested only on
AR9331 PHY with following results:
- No cable is detected as short
- A 15m long cable connected only on one side is detected as 9m open.
That sounds wrong. What about a shorted 15m cable? Is it also 9m?  Do
you have any other long cables you can test with? Is it always 1/2 the
cable length?
I had similar inaccuracies with my recent faulty cable when testing
with a Marvell PHY as I mentioned.

"Using the VCT in the Marvell PHY points to it being pair 3, at a
distance of 0x190 or 0x50 depending on which way round the cable is
connected.  That's in cm.  The cable isn't 480cm long, it's 278cm
long, and the problem is up by one of the connectors."

0x190 = 400cm, 0x50 = 80cm.

Given that the issue was at one of the connectors on the cable, and
I tried VCT with it plugged into the same port, you can't even say
"well, if we define the start of the cable at 80cm, then that works
for the cable connected the other way around" - it gets us closer
but it's still about 30cm wrong.

It doesn't even work if you think maybe the figures have forgotten
to take into account the fact that the TDR pulse has to go out and
then return (so travel twice the distance, so maybe the figures are
doubled.)

So, it seems we have more than one PHY that produces only wildly
inaccurate guesses at the distance to the fault.

I'd say this technology is a "it would be nice if we could" but the
results can not be relied upon.  It may be grounded in hard physics,
but there's clearly something causing incorrect results.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Florian Fainelli <f.fainelli@gmail.com>
Date: 2020-05-13 15:23:49


On 5/13/2020 5:06 AM, Oleksij Rempel wrote:
The cable test seems to be support by all of currently support Atherso
PHYs, so add support for all of them. This patch was tested only on
AR9331 PHY with following results:
- No cable is detected as short
- A 15m long cable connected only on one side is detected as 9m open.
- A cable test with active link partner will provide no usable results.
How does this relate to Michael's recent work here:

https://www.spinics.net/lists/kernel/msg3509304.html
-- 
Florian

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2020-05-13 15:45:57

On Wed, May 13, 2020 at 08:23:42AM -0700, Florian Fainelli wrote:

On 5/13/2020 5:06 AM, Oleksij Rempel wrote:
quoted
The cable test seems to be support by all of currently support Atherso
PHYs, so add support for all of them. This patch was tested only on
AR9331 PHY with following results:
- No cable is detected as short
- A 15m long cable connected only on one side is detected as 9m open.
- A cable test with active link partner will provide no usable results.
How does this relate to Michael's recent work here:

https://www.spinics.net/lists/kernel/msg3509304.html
Uff.. i missed this. Then I'll need only to add some changes on top of
his patch.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Andrew Lunn <andrew@lunn.ch>
Date: 2020-05-13 15:50:01

Uff.. i missed this. Then I'll need only to add some changes on top of
his patch.
I've been chatting with mwalle on IRC today. There should be a repost
of the patches soon.

   Andrew

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2020-05-13 16:00:33

On Wed, May 13, 2020 at 05:49:53PM +0200, Andrew Lunn wrote:
quoted
Uff.. i missed this. Then I'll need only to add some changes on top of
his patch.
I've been chatting with mwalle on IRC today. There should be a repost
of the patches soon.
Cool!
@Michael, please CC me.

you can include support for AR9331 and AR8032 in your patch (if you
like)
http://www.jhongtech.com/DOWN/ATHEROS--AR8032.pdf

They have same register, but only 2 pairs.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Andrew Lunn <andrew@lunn.ch>
Date: 2020-05-13 16:24:00

On Wed, May 13, 2020 at 06:00:26PM +0200, Oleksij Rempel wrote:
On Wed, May 13, 2020 at 05:49:53PM +0200, Andrew Lunn wrote:
quoted
quoted
Uff.. i missed this. Then I'll need only to add some changes on top of
his patch.
I've been chatting with mwalle on IRC today. There should be a repost
of the patches soon.
Cool!
@Michael, please CC me.

you can include support for AR9331 and AR8032 in your patch (if you
like)
http://www.jhongtech.com/DOWN/ATHEROS--AR8032.pdf

They have same register, but only 2 pairs.
Hi Oleksij 

Michael just reposted. Please send a follow up patch adding these two
PHYs.

       Andrew

Re: [PATCH v1] net: phy: at803x: add cable test support

From: Michael Walle <hidden>
Date: 2020-05-13 16:26:41

Am 2020-05-13 18:00, schrieb Oleksij Rempel:
On Wed, May 13, 2020 at 05:49:53PM +0200, Andrew Lunn wrote:
quoted
quoted
Uff.. i missed this. Then I'll need only to add some changes on top of
his patch.
I've been chatting with mwalle on IRC today. There should be a repost
of the patches soon.
Cool!
@Michael, please CC me.
sure no problem.
you can include support for AR9331 and AR8032 in your patch (if you
like)
I guess merging doesn't take too long for my patch. So better you just 
post
a patch on top of that, before I screw something up.

-michael
http://www.jhongtech.com/DOWN/ATHEROS--AR8032.pdf

They have same register, but only 2 pairs.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help