diff mbox series

[2/2] net: phy: adin: implement cable-test support

Message ID 20201021135140.51300-2-alexandru.ardelean@analog.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg | expand

Commit Message

Alexandru Ardelean Oct. 21, 2020, 1:51 p.m. UTC
The ADIN1300/ADIN1200 support cable diagnostics using TDR.

The cable fault detection is automatically run on all four pairs looking at
all combinations of pair faults by first putting the PHY in standby (clear
the LINK_EN bit, PHY_CTRL_3 register, Address 0x0017) and then enabling the
diagnostic clock (set the DIAG_CLK_EN bit, PHY_CTRL_1 register, Address
0x0012).

Cable diagnostics can then be run (set the CDIAG_RUN bit in the
CDIAG_RUN register, Address 0xBA1B). The results are reported for each pair
in the cable diagnostics results registers, CDIAG_DTLD_RSLTS_0,
CDIAG_DTLD_RSLTS_1, CDIAG_DTLD_RSLTS_2, and CDIAG_DTLD_RSLTS_3, Address
0xBA1D to Address 0xBA20).

The distance to the first fault for each pair is reported in the cable
fault distance registers, CDIAG_FLT_DIST_0, CDIAG_FLT_DIST_1,
CDIAG_FLT_DIST_2, and CDIAG_FLT_DIST_3, Address 0xBA21 to Address 0xBA24).

This change implements support for this using phylib's cable-test support.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 138 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

Comments

Andrew Lunn Oct. 21, 2020, 2:08 p.m. UTC | #1
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hi Alexandru

Overall, this looks good.

> +static int adin_cable_test_report_trans(int result)
> +{
> +	int mask;
> +
> +	if (result & ADIN1300_CDIAG_RSLT_GOOD)
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> +	if (result & ADIN1300_CDIAG_RSLT_OPEN)
> +		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> +
> +	/* short with other pairs */
> +	mask = ADIN1300_CDIAG_RSLT_XSHRT3 |
> +	       ADIN1300_CDIAG_RSLT_XSHRT2 |
> +	       ADIN1300_CDIAG_RSLT_XSHRT1;
> +	if (result & mask)
> +		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;

The nice thing about the netlink API is that it is extendable without
breaking backwards compatibility. You could if you want add another
attribute, indicating what pair it is shorted to.

	Andrew
Alexandru Ardelean Oct. 21, 2020, 2:16 p.m. UTC | #2
On Wed, Oct 21, 2020 at 5:09 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>

removed my typo-ed email

> Hi Alexandru
>
> Overall, this looks good.
>
> > +static int adin_cable_test_report_trans(int result)
> > +{
> > +     int mask;
> > +
> > +     if (result & ADIN1300_CDIAG_RSLT_GOOD)
> > +             return ETHTOOL_A_CABLE_RESULT_CODE_OK;
> > +     if (result & ADIN1300_CDIAG_RSLT_OPEN)
> > +             return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
> > +
> > +     /* short with other pairs */
> > +     mask = ADIN1300_CDIAG_RSLT_XSHRT3 |
> > +            ADIN1300_CDIAG_RSLT_XSHRT2 |
> > +            ADIN1300_CDIAG_RSLT_XSHRT1;
> > +     if (result & mask)
> > +             return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
>
> The nice thing about the netlink API is that it is extendable without
> breaking backwards compatibility. You could if you want add another
> attribute, indicating what pair it is shorted to.

That would be an idea.

Actually, I'd also be interested [for this PHY], to report a
"significance impedance" detection, which is similar to the
short-detection that is already done.
At first, this report would sound like it could be interesting; but
feel free to disagree with me.

And there's also some "busy" indicator; as-in "unknown activity during
diagnostics"; to-be-honest, I don't know what this is yet.
I'd need to check, but odds are that I'd need to also ask about it.
So, I don't think I'd implement this.

>
>         Andrew
Andrew Lunn Oct. 21, 2020, 2:28 p.m. UTC | #3
> Actually, I'd also be interested [for this PHY], to report a
> "significance impedance" detection, which is similar to the
> short-detection that is already done.

You can add that as just another element of the enum.

> At first, this report would sound like it could be interesting; but
> feel free to disagree with me.
> 
> And there's also some "busy" indicator; as-in "unknown activity during
> diagnostics"; to-be-honest, I don't know what this is yet.

The link partner did not go quiet. You can only do cable tests if the
partner is not sending frames or pulses. You will find most PHYs have
some sort of error status for this. For the Marvell driver, this is 
MII_VCT7_RESULTS_INVALID. In that case, the Marvell driver returns
ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC.

	Andrew
Alexandru Ardelean Oct. 21, 2020, 2:46 p.m. UTC | #4
On Wed, Oct 21, 2020 at 5:28 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Actually, I'd also be interested [for this PHY], to report a
> > "significance impedance" detection, which is similar to the
> > short-detection that is already done.
>
> You can add that as just another element of the enum.
>
> > At first, this report would sound like it could be interesting; but
> > feel free to disagree with me.
> >
> > And there's also some "busy" indicator; as-in "unknown activity during
> > diagnostics"; to-be-honest, I don't know what this is yet.
>
> The link partner did not go quiet. You can only do cable tests if the
> partner is not sending frames or pulses. You will find most PHYs have
> some sort of error status for this. For the Marvell driver, this is
> MII_VCT7_RESULTS_INVALID. In that case, the Marvell driver returns
> ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC.

Good to know.
So, then a quick question: would this patchset [well, a V2 of this] be
ok in this form, for the initial cable-test support of this PHY?

For other enhancements on the PHY's cable-test [that also require some
new netlink attributes, I can do other patches, in the form of
"new-netlink-attr, then driver change, and then ethtool update".

>
>         Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 619d36685b5d..3e66f97c7611 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -8,6 +8,7 @@ 
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/mii.h>
@@ -70,6 +71,31 @@ 
 #define ADIN1300_CLOCK_STOP_REG			0x9400
 #define ADIN1300_LPI_WAKE_ERR_CNT_REG		0xa000
 
+#define ADIN1300_CDIAG_RUN			0xba1b
+#define   ADIN1300_CDIAG_RUN_EN			BIT(0)
+
+/*
+ * The XSIM3/2/1 and XSHRT3/2/1 are actually relative.
+ * For CDIAG_DTLD_RSLTS(0) it's ADIN1300_CDIAG_RSLT_XSIM3/2/1
+ * For CDIAG_DTLD_RSLTS(1) it's ADIN1300_CDIAG_RSLT_XSIM3/2/0
+ * For CDIAG_DTLD_RSLTS(2) it's ADIN1300_CDIAG_RSLT_XSIM3/1/0
+ * For CDIAG_DTLD_RSLTS(3) it's ADIN1300_CDIAG_RSLT_XSIM2/1/0
+ */
+#define ADIN1300_CDIAG_DTLD_RSLTS(x)		(0xba1d + (x))
+#define   ADIN1300_CDIAG_RSLT_BUSY		BIT(10)
+#define   ADIN1300_CDIAG_RSLT_XSIM3		BIT(9)
+#define   ADIN1300_CDIAG_RSLT_XSIM2		BIT(8)
+#define   ADIN1300_CDIAG_RSLT_XSIM1		BIT(7)
+#define   ADIN1300_CDIAG_RSLT_SIM		BIT(6)
+#define   ADIN1300_CDIAG_RSLT_XSHRT3		BIT(5)
+#define   ADIN1300_CDIAG_RSLT_XSHRT2		BIT(4)
+#define   ADIN1300_CDIAG_RSLT_XSHRT1		BIT(3)
+#define   ADIN1300_CDIAG_RSLT_SHRT		BIT(2)
+#define   ADIN1300_CDIAG_RSLT_OPEN		BIT(1)
+#define   ADIN1300_CDIAG_RSLT_GOOD		BIT(0)
+
+#define ADIN1300_CDIAG_FLT_DIST(x)		(0xba21 + (x))
+
 #define ADIN1300_GE_SOFT_RESET_REG		0xff0c
 #define   ADIN1300_GE_SOFT_RESET		BIT(0)
 
@@ -741,10 +767,117 @@  static int adin_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int adin_cable_test_start(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_clear_bits(phydev, ADIN1300_PHY_CTRL3, ADIN1300_LINKING_EN);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_clear_bits(phydev, ADIN1300_PHY_CTRL1, ADIN1300_DIAG_CLK_EN);
+	if (ret < 0)
+		return ret;
+
+	/* wait a bit for the clock to stabilize */
+	msleep(50);
+
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_CDIAG_RUN,
+				ADIN1300_CDIAG_RUN_EN);
+}
+
+static int adin_cable_test_report_trans(int result)
+{
+	int mask;
+
+	if (result & ADIN1300_CDIAG_RSLT_GOOD)
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	if (result & ADIN1300_CDIAG_RSLT_OPEN)
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+
+	/* short with other pairs */
+	mask = ADIN1300_CDIAG_RSLT_XSHRT3 |
+	       ADIN1300_CDIAG_RSLT_XSHRT2 |
+	       ADIN1300_CDIAG_RSLT_XSHRT1;
+	if (result & mask)
+		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+
+	if (result & ADIN1300_CDIAG_RSLT_SHRT)
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+
+	return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+}
+
+static int adin_cable_test_report_pair(struct phy_device *phydev,
+				       unsigned int pair)
+{
+	int fault_rslt;
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+			   ADIN1300_CDIAG_DTLD_RSLTS(pair));
+	if (ret < 0)
+		return ret;
+
+	fault_rslt = adin_cable_test_report_trans(ret);
+
+	ret = ethnl_cable_test_result(phydev, pair, fault_rslt);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+			   ADIN1300_CDIAG_FLT_DIST(pair));
+	if (ret < 0)
+		return ret;
+
+	switch (fault_rslt) {
+	case ETHTOOL_A_CABLE_RESULT_CODE_OPEN:
+	case ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT:
+	case ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT:
+		return ethnl_cable_test_fault_length(phydev, pair, ret * 100);
+	default:
+		return  0;
+	}
+}
+
+static int adin_cable_test_report(struct phy_device *phydev)
+{
+	unsigned int pair;
+	int ret;
+
+	for (pair = ETHTOOL_A_CABLE_PAIR_A; pair <= ETHTOOL_A_CABLE_PAIR_D; pair++) {
+		ret = adin_cable_test_report_pair(phydev, pair);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int adin_cable_test_get_status(struct phy_device *phydev,
+				      bool *finished)
+{
+	int ret;
+
+	*finished = false;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_CDIAG_RUN);
+	if (ret < 0)
+		return ret;
+
+	if (ret & ADIN1300_CDIAG_RUN_EN)
+		return 0;
+
+	*finished = true;
+
+	return adin_cable_test_report(phydev);
+}
+
 static struct phy_driver adin_driver[] = {
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_ADIN1200),
 		.name		= "ADIN1200",
+		.flags		= PHY_POLL_CABLE_TEST,
 		.probe		= adin_probe,
 		.config_init	= adin_config_init,
 		.soft_reset	= adin_soft_reset,
@@ -761,10 +894,13 @@  static struct phy_driver adin_driver[] = {
 		.suspend	= genphy_suspend,
 		.read_mmd	= adin_read_mmd,
 		.write_mmd	= adin_write_mmd,
+		.cable_test_start	= adin_cable_test_start,
+		.cable_test_get_status	= adin_cable_test_get_status,
 	},
 	{
 		PHY_ID_MATCH_MODEL(PHY_ID_ADIN1300),
 		.name		= "ADIN1300",
+		.flags		= PHY_POLL_CABLE_TEST,
 		.probe		= adin_probe,
 		.config_init	= adin_config_init,
 		.soft_reset	= adin_soft_reset,
@@ -781,6 +917,8 @@  static struct phy_driver adin_driver[] = {
 		.suspend	= genphy_suspend,
 		.read_mmd	= adin_read_mmd,
 		.write_mmd	= adin_write_mmd,
+		.cable_test_start	= adin_cable_test_start,
+		.cable_test_get_status	= adin_cable_test_get_status,
 	},
 };