[PATCH v2 1/2] net: phy: adin: disable diag clock & disable standby mode in config_aneg

[PATCH v2 1/2] net: phy: adin: disable diag clock & disable standby mode in config_aneg

[Alexandru Ardelean]

When the PHY powers up, the diagnostics clock isn't enabled (bit 2 in
register PHY_CTRL_1 (0x0012)).
Also, the PHY is not in standby mode, so bit 13 in PHY_CTRL_3 (0x0017) is
always set at power up.

The standby mode and the diagnostics clock are both meant to be for the
cable diagnostics feature of the PHY (in phylib this would be equivalent to
the cable-test support), and for the frame-generator feature of the PHY.

In standby mode, the PHY doesn't negotiate links or manage links.

To use the cable diagnostics/test (or frame-generator), the PHY must be
first set in standby mode, so that the link operation doesn't interfere.
Then, the diagnostics clock must be enabled.

For the cable-test feature, when the operation finishes, the PHY goes into
PHY_UP state, and the config_aneg hook is called.

For the ADIN PHY, we need to make sure that during autonegotiation
configuration/setup the PHY is removed from standby mode and the
diagnostics clock is disabled, so that normal operation is resumed.

This change does that by moving the set of the ADIN1300_LINKING_EN bit (2)
in the config_aneg (to disable standby mode).
Previously, this was set in the downshift setup, because the downshift
retry value and the ADIN1300_LINKING_EN are in the same register.

And the ADIN1300_DIAG_CLK_EN bit (13) is cleared, to disable the
diagnostics clock.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v1 -> v2:
* updated title; updated description to better describe the diagnostics
  clock & standby mode, and what they mean for the PHY


 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.25.1

[PATCH v2 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.25.1

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

[Jakub Kicinski]

On Thu, 22 Oct 2020 10:45:51 +0300 Alexandru Ardelean wrote:
> 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>

# Form letter - net-next is closed

We have already sent a pull request for 5.10 and therefore net-next 
is closed for new drivers, features, and code refactoring.

Please repost when net-next reopens after 5.10-rc1 is cut.

(http://vger.kernel.org/~davem/net-next.html will not be up to date 
 this time around, sorry about that).

RFC patches sent for review only are obviously welcome at any time.

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

[Alexandru Ardelean]

On Fri, Oct 23, 2020 at 3:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 22 Oct 2020 10:45:51 +0300 Alexandru Ardelean wrote:
> > 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>
>
> # Form letter - net-next is closed
>
> We have already sent a pull request for 5.10 and therefore net-next
> is closed for new drivers, features, and code refactoring.
>
> Please repost when net-next reopens after 5.10-rc1 is cut.
>

Ack.
No hurry from my side.

Thanks
Alex

> (http://vger.kernel.org/~davem/net-next.html will not be up to date
>  this time around, sorry about that).
>
> RFC patches sent for review only are obviously welcome at any time.

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

[Andrew Lunn]

On Thu, Oct 22, 2020 at 10:45:51AM +0300, Alexandru Ardelean wrote:
> 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>

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

For a patch series, it is normal to include a cover letter explaining
what the series as a whole does.

Also the subject should indicate which tree the patchset is for.

See the netdev FAQ.

    Andrew

Re: [PATCH v2 1/2] net: phy: adin: disable diag clock & disable standby mode in config_aneg

[Andrew Lunn]

On Thu, Oct 22, 2020 at 10:45:50AM +0300, Alexandru Ardelean wrote:
> When the PHY powers up, the diagnostics clock isn't enabled (bit 2 in
> register PHY_CTRL_1 (0x0012)).
> Also, the PHY is not in standby mode, so bit 13 in PHY_CTRL_3 (0x0017) is
> always set at power up.
> 
> The standby mode and the diagnostics clock are both meant to be for the
> cable diagnostics feature of the PHY (in phylib this would be equivalent to
> the cable-test support), and for the frame-generator feature of the PHY.
> 
> In standby mode, the PHY doesn't negotiate links or manage links.
> 
> To use the cable diagnostics/test (or frame-generator), the PHY must be
> first set in standby mode, so that the link operation doesn't interfere.
> Then, the diagnostics clock must be enabled.
> 
> For the cable-test feature, when the operation finishes, the PHY goes into
> PHY_UP state, and the config_aneg hook is called.
> 
> For the ADIN PHY, we need to make sure that during autonegotiation
> configuration/setup the PHY is removed from standby mode and the
> diagnostics clock is disabled, so that normal operation is resumed.
> 
> This change does that by moving the set of the ADIN1300_LINKING_EN bit (2)
> in the config_aneg (to disable standby mode).
> Previously, this was set in the downshift setup, because the downshift
> retry value and the ADIN1300_LINKING_EN are in the same register.
> 
> And the ADIN1300_DIAG_CLK_EN bit (13) is cleared, to disable the
> diagnostics clock.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

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

    Andrew

Re: [PATCH v2 1/2] net: phy: adin: disable diag clock & disable standby mode in config_aneg

[Alexandru Ardelean]

On Sat, Oct 24, 2020 at 1:43 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Oct 22, 2020 at 10:45:50AM +0300, Alexandru Ardelean wrote:
> > When the PHY powers up, the diagnostics clock isn't enabled (bit 2 in
> > register PHY_CTRL_1 (0x0012)).
> > Also, the PHY is not in standby mode, so bit 13 in PHY_CTRL_3 (0x0017) is
> > always set at power up.
> >
> > The standby mode and the diagnostics clock are both meant to be for the
> > cable diagnostics feature of the PHY (in phylib this would be equivalent to
> > the cable-test support), and for the frame-generator feature of the PHY.
> >
> > In standby mode, the PHY doesn't negotiate links or manage links.
> >
> > To use the cable diagnostics/test (or frame-generator), the PHY must be
> > first set in standby mode, so that the link operation doesn't interfere.
> > Then, the diagnostics clock must be enabled.
> >
> > For the cable-test feature, when the operation finishes, the PHY goes into
> > PHY_UP state, and the config_aneg hook is called.
> >
> > For the ADIN PHY, we need to make sure that during autonegotiation
> > configuration/setup the PHY is removed from standby mode and the
> > diagnostics clock is disabled, so that normal operation is resumed.
> >
> > This change does that by moving the set of the ADIN1300_LINKING_EN bit (2)
> > in the config_aneg (to disable standby mode).
> > Previously, this was set in the downshift setup, because the downshift
> > retry value and the ADIN1300_LINKING_EN are in the same register.
> >
> > And the ADIN1300_DIAG_CLK_EN bit (13) is cleared, to disable the
> > diagnostics clock.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

So, then re-send for this or just this patch ping?
Naturally, this is for net-next.
I don't mind doing either way.

Thanks
Alex

>
>     Andrew

Re: [PATCH v2 1/2] net: phy: adin: disable diag clock & disable standby mode in config_aneg

[Andrew Lunn]

> So, then re-send for this or just this patch ping?
> Naturally, this is for net-next.
> I don't mind doing either way.

Please resend, with all the acked-by, reviewed-by added, now that
net-next is open.

	 Andrew