[PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Alexandru Ardelean]

The LINKING_EN bit is always cleared during reset. Initially it was set
during the downshift setup, because it's in the same register as the
downshift retry count (PHY_CTRL1).

This change moves the handling of LINKING_EN from the downshift handler to
the autonegotiation handler. Also, during autonegotiation setup, the
diagnostics clock is cleared.

This is being done as a prequel to the cable-diagnostics patch. When the
cable diagnostics finishes, the PHY state machine goes back into the PHY_UP
state and the autonegotiation is restarted (or better said, the
autonegotiation handler is called).
During this call, the diagnostics clock should be disabled, and the
LINKING_EN bit set in the PHY_CTRL1 register.

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

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 5bc3926c52f0..619d36685b5d 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -23,6 +23,7 @@
 #define ADIN1300_PHY_CTRL1			0x0012
 #define   ADIN1300_AUTO_MDI_EN			BIT(10)
 #define   ADIN1300_MAN_MDIX_EN			BIT(9)
+#define   ADIN1300_DIAG_CLK_EN			BIT(2)
 
 #define ADIN1300_RX_ERR_CNT			0x0014
 
@@ -326,10 +327,9 @@ static int adin_set_downshift(struct phy_device *phydev, u8 cnt)
 		return -E2BIG;
 
 	val = FIELD_PREP(ADIN1300_DOWNSPEED_RETRIES_MSK, cnt);
-	val |= ADIN1300_LINKING_EN;
 
 	rc = phy_modify(phydev, ADIN1300_PHY_CTRL3,
-			ADIN1300_LINKING_EN | ADIN1300_DOWNSPEED_RETRIES_MSK,
+			ADIN1300_DOWNSPEED_RETRIES_MSK,
 			val);
 	if (rc < 0)
 		return rc;
@@ -560,6 +560,14 @@ static int adin_config_aneg(struct phy_device *phydev)
 {
 	int ret;
 
+	ret = phy_clear_bits(phydev, ADIN1300_PHY_CTRL1, ADIN1300_DIAG_CLK_EN);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_set_bits(phydev, ADIN1300_PHY_CTRL3, ADIN1300_LINKING_EN);
+	if (ret < 0)
+		return ret;
+
 	ret = adin_config_mdix(phydev);
 	if (ret)
 		return ret;
-- 
2.17.1

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

[Alexandru Ardelean]

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(+)

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,
 	},
 };
 
-- 
2.17.1

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

[Andrew Lunn]

> 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

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

[Alexandru Ardelean]

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

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

[Andrew Lunn]

> 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

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

[Alexandru Ardelean]

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

Re: [PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Andrew Lunn]

On Wed, Oct 21, 2020 at 04:51:39PM +0300, Alexandru Ardelean wrote:
> The LINKING_EN bit is always cleared during reset. Initially it was set
> during the downshift setup, because it's in the same register as the
> downshift retry count (PHY_CTRL1).

Hi Alexandru

For those of us how have not read the datasheet, could you give a
brief explanation what LINKING_EN does?
 
> This change moves the handling of LINKING_EN from the downshift handler to
> the autonegotiation handler. Also, during autonegotiation setup, the
> diagnostics clock is cleared.

And what is the diagnostics clock used for?

    Andrew

Re: [PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Alexandru Ardelean]

On Wed, Oct 21, 2020 at 4:58 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Oct 21, 2020 at 04:51:39PM +0300, Alexandru Ardelean wrote:
> > The LINKING_EN bit is always cleared during reset. Initially it was set
> > during the downshift setup, because it's in the same register as the
> > downshift retry count (PHY_CTRL1).
>
> Hi Alexandru
>
> For those of us how have not read the datasheet, could you give a
> brief explanation what LINKING_EN does?

So, clearing this bit puts the PHY in a standby-state.
The PHY doesn't do any autonegotiation or link handling.

>
> > This change moves the handling of LINKING_EN from the downshift handler to
> > the autonegotiation handler. Also, during autonegotiation setup, the
> > diagnostics clock is cleared.
>
> And what is the diagnostics clock used for?

The clock diagnostics is used for 2 things: the diagnostics block
[mostly for stuff like cable-diagnostics] and the frame-generator.
The frame-generator is an interesting feature of the PHY, that's not
useful for the current phylib; the PHY can send packages [like a
signal generator], and then these can be looped back, or sent over the
wire.
Maybe it's being used mostly internally by the group that created the PHY

Having said this, I'll include some comments for these in a V2 of this patchset.

>
>     Andrew

Re: [PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Andrew Lunn]

> The frame-generator is an interesting feature of the PHY, that's not
> useful for the current phylib; the PHY can send packages [like a
> signal generator], and then these can be looped back, or sent over the
> wire.

Many PHYs that that. I posted some patches to the list a few years ago
adding basic support for the Marvell PHY frame generator. They got
NACKed. The netlink API, and some of the infrastructure i added for
cable testing would make it possible to fix the issues that caused the
NACK.

> Having said this, I'll include some comments for these in a V2 of this patchset.

Thanks.

	Andrew

P.S.

Your mail is broken somehow:

Delivery has failed to these recipients or groups:

alexaundru.ardelean@analog.com
The email address you entered couldn't be found. Please check the recipient's
email address and try to resend the message. If the problem continues, please
contact your email admin.

Re: [PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Alexandru Ardelean]

On Wed, Oct 21, 2020 at 5:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > The frame-generator is an interesting feature of the PHY, that's not
> > useful for the current phylib; the PHY can send packages [like a
> > signal generator], and then these can be looped back, or sent over the
> > wire.
>

removed my typo-ed [work] email
i use gmail as a mirror-email for my work email, because.... reasons
and i added my work-email to the --cc list with a typo, because the
universe seems to have wanted that [in a manner of saying it]

> Many PHYs that that. I posted some patches to the list a few years ago
> adding basic support for the Marvell PHY frame generator. They got
> NACKed. The netlink API, and some of the infrastructure i added for
> cable testing would make it possible to fix the issues that caused the
> NACK.

i'll think about the frame-generator;

i was super-happy when the cable-test support was added;
when i first wrote the PHY, i actually wrote this logic for
cable-testing, then scrapped it because the code [without any
framework around it] just looked bad, and like it was asking to cause
trouble;

with this minimal framework in place, cable-testing looks like a neat
feature [and neatly implemented];
and it took me less than a day to write and test it;
so, thank you for this :)

>
> > Having said this, I'll include some comments for these in a V2 of this patchset.
>
> Thanks.
>
>         Andrew
>
> P.S.
>
> Your mail is broken somehow:
>
> Delivery has failed to these recipients or groups:
>
> alexaundru.ardelean@analog.com
> The email address you entered couldn't be found. Please check the recipient's
> email address and try to resend the message. If the problem continues, please
> contact your email admin.

Re: [PATCH 1/2] net: phy: adin: clear the diag clock and set LINKING_EN during autoneg

[Andrew Lunn]

> i'll think about the frame-generator;

Here were the two main problems i can remember with my first version:

How do you discover what is can actually do? You probably need to
collect up all the open PHY datasheets and get an idea what the
different vendors provide, what is common, what could be shared
extensions etc, and think about how you can describe the
capabilities. Probably a netlink call will be needed to return what
the hardware is capable of doing.

At the time, it was necessary to hold RTNL while performing packet
generation. That is bad, because it means most of the control plane
stops for all devices. We will need to copy some of the ideas from the
cable test to avoid this, adding a state to the state machine, etc.

      Andrew