[PATCH net-next v1 0/5] add cable test support for ksz8081 and ksz8873

[PATCH net-next v1 0/5] add cable test support for ksz8081 and ksz8873

[Oleksij Rempel]

This patch series provide support for cable testing on some of micrel PHYs.
Since this PHYs do not allow to switch between cable pairs within the
test register, I used MDI-X functionality to make it possible.

Oleksij Rempel (5):
  net: phy: micrel: use consistent indention after define
  net: phy: micrel: apply resume errata workaround for ksz8873 and
    ksz8863
  net: phy: micrel: ksz886x add MDI-X support
  net: phy: micrel: ksz8081 add MDI-X support
  net: phy: micrel: ksz886x/ksz8081: add cabletest support

 drivers/net/phy/micrel.c | 411 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 398 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH net-next v1 1/5] net: phy: micrel: use consistent indention after define

[Oleksij Rempel]

This patch changes the indention to one space between "#define" and the
macro.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 3fe552675dd2..caf56b9b51db 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -38,23 +38,23 @@
 
 /* general Interrupt control/status reg in vendor specific block. */
 #define MII_KSZPHY_INTCS			0x1B
-#define	KSZPHY_INTCS_JABBER			BIT(15)
-#define	KSZPHY_INTCS_RECEIVE_ERR		BIT(14)
-#define	KSZPHY_INTCS_PAGE_RECEIVE		BIT(13)
-#define	KSZPHY_INTCS_PARELLEL			BIT(12)
-#define	KSZPHY_INTCS_LINK_PARTNER_ACK		BIT(11)
-#define	KSZPHY_INTCS_LINK_DOWN			BIT(10)
-#define	KSZPHY_INTCS_REMOTE_FAULT		BIT(9)
-#define	KSZPHY_INTCS_LINK_UP			BIT(8)
-#define	KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
+#define KSZPHY_INTCS_JABBER			BIT(15)
+#define KSZPHY_INTCS_RECEIVE_ERR		BIT(14)
+#define KSZPHY_INTCS_PAGE_RECEIVE		BIT(13)
+#define KSZPHY_INTCS_PARELLEL			BIT(12)
+#define KSZPHY_INTCS_LINK_PARTNER_ACK		BIT(11)
+#define KSZPHY_INTCS_LINK_DOWN			BIT(10)
+#define KSZPHY_INTCS_REMOTE_FAULT		BIT(9)
+#define KSZPHY_INTCS_LINK_UP			BIT(8)
+#define KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
 						KSZPHY_INTCS_LINK_DOWN)
 
 /* PHY Control 1 */
-#define	MII_KSZPHY_CTRL_1			0x1e
+#define MII_KSZPHY_CTRL_1			0x1e
 
 /* PHY Control 2 / PHY Control (if no PHY Control 1) */
-#define	MII_KSZPHY_CTRL_2			0x1f
-#define	MII_KSZPHY_CTRL				MII_KSZPHY_CTRL_2
+#define MII_KSZPHY_CTRL_2			0x1f
+#define MII_KSZPHY_CTRL				MII_KSZPHY_CTRL_2
 /* bitmap of PHY register to set interrupt mode */
 #define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
 #define KSZPHY_RMII_REF_CLK_SEL			BIT(7)
-- 
2.27.0

Re: [PATCH net-next v1 1/5] net: phy: micrel: use consistent indention after define

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:47PM +0200, Oleksij Rempel wrote:
> This patch changes the indention to one space between "#define" and the
> macro.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew

[PATCH net-next v1 2/5] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Oleksij Rempel]

The ksz8873 and ksz8863 switches are affected by following errata:

| "Receiver error in 100BASE-TX mode following Soft Power Down"
|
| Some KSZ8873 devices may exhibit receiver errors after transitioning
| from Soft Power Down mode to Normal mode, as controlled by register 195
| (0xC3) bits [1:0]. When exiting Soft Power Down mode, the receiver
| blocks may not start up properly, causing the PHY to miss data and
| exhibit erratic behavior. The problem may appear on either port 1 or
| port 2, or both ports. The problem occurs only for 100BASE-TX, not
| 10BASE-T.
|
| END USER IMPLICATIONS
| When the failure occurs, the following symptoms are seen on the affected
| port(s):
| - The port is able to link
| - LED0 blinks, even when there is no traffic
| - The MIB counters indicate receive errors (Rx Fragments, Rx Symbol
|   Errors, Rx CRC Errors, Rx Alignment Errors)
| - Only a small fraction of packets is correctly received and forwarded
|   through the switch. Most packets are dropped due to receive errors.
|
| The failing condition cannot be corrected by the following:
| - Removing and reconnecting the cable
| - Hardware reset
| - Software Reset and PCS Reset bits in register 67 (0x43)
|
| Work around:
| The problem can be corrected by setting and then clearing the Port Power
| Down bits (registers 29 (0x1D) and 45 (0x2D), bit 3). This must be done
| separately for each affected port after returning from Soft Power Down
| Mode to Normal Mode. The following procedure will ensure no further
| issues due to this erratum. To enter Soft Power Down Mode, set register
| 195 (0xC3), bits [1:0] = 10.
|
| To exit Soft Power Down Mode, follow these steps:
| 1. Set register 195 (0xC3), bits [1:0] = 00 // Exit soft power down mode
| 2. Wait 1ms minimum
| 3. Set register 29 (0x1D), bit [3] = 1 // Enter PHY port 1 power down mode
| 4. Set register 29 (0x1D), bit [3] = 0 // Exit PHY port 1 power down mode
| 5. Set register 45 (0x2D), bit [3] = 1 // Enter PHY port 2 power down mode
| 6. Set register 45 (0x2D), bit [3] = 0 // Exit PHY port 2 power down mode

This patch implements steps 2...6 of the suggested workaround. The first
step needs to be implemented in the switch driver.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index caf56b9b51db..12106fbea565 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1010,6 +1010,26 @@ static int ksz8873mll_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz886x_resume(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Apply errata workaround for KSZ8863 and KSZ8873:
+	 * Receiver error in 100BASE-TX mode following Soft Power Down
+	 *
+	 * When exiting Soft Power Down mode, the receiver blocks may not start
+	 * up properly, causing the PHY to miss data and exhibit erratic
+	 * behavior.
+	 */
+	usleep_range(1000, 2000);
+
+	ret = phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN);
+	if (ret)
+		return ret;
+
+	return phy_clear_bits(phydev, MII_BMCR, BMCR_PDOWN);
+}
+
 static int kszphy_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(kszphy_hw_stats);
@@ -1347,7 +1367,7 @@ static struct phy_driver ksphy_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_init	= kszphy_config_init,
 	.suspend	= genphy_suspend,
-	.resume		= genphy_resume,
+	.resume		= ksz886x_resume,
 }, {
 	.name		= "Micrel KSZ87XX Switch",
 	/* PHY_BASIC_FEATURES */
-- 
2.27.0

Re: [PATCH net-next v1 2/5] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:48PM +0200, Oleksij Rempel wrote:
> The ksz8873 and ksz8863 switches are affected by following errata:

This should really be a patch on its own, aimed at net, so it gets
back ported to stable.

     Andrew

[PATCH net-next v1 3/5] net: phy: micrel: ksz886x add MDI-X support

[Oleksij Rempel]

Add support for MDI-X status and configuration

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 101 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 12106fbea565..ec409b2cb984 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -28,6 +28,19 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 
+/* Device specific MII_BMCR (Reg 0) bits */
+/* 1 = HP Auto MDI/MDI-X mode, 0 = Microchip Auto MDI/MDI-X mode */
+#define KSZ886X_BMCR_HP_MDIX			BIT(5)
+/* 1 = Force MDI (transmit on RXP/RXM pins), 0 = Normal operation
+ * (transmit on TXP/TXM pins)
+ */
+#define KSZ886X_BMCR_FORCE_MDI			BIT(4)
+/* 1 = Disable auto MDI-X */
+#define KSZ886X_BMCR_DISABLE_AUTO_MDIX		BIT(3)
+#define KSZ886X_BMCR_DISABLE_FAR_END_FAULT	BIT(2)
+#define KSZ886X_BMCR_DISABLE_TRANSMIT		BIT(1)
+#define KSZ886X_BMCR_DISABLE_LED		BIT(0)
+
 /* Operation Mode Strap Override */
 #define MII_KSZPHY_OMSO				0x16
 #define KSZPHY_OMSO_FACTORY_TEST		BIT(15)
@@ -58,6 +71,7 @@
 /* bitmap of PHY register to set interrupt mode */
 #define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
 #define KSZPHY_RMII_REF_CLK_SEL			BIT(7)
+#define KSZ886X_CTRL_MDIX_STAT			BIT(4)
 
 /* Write/read to/from extended registers */
 #define MII_KSZPHY_EXTREG                       0x0b
@@ -1010,6 +1024,91 @@ static int ksz8873mll_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz886x_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI:
+		val = KSZ886X_BMCR_DISABLE_AUTO_MDIX;
+		break;
+	case ETH_TP_MDI_X:
+		/* Note: The naming of the bit KSZ886X_BMCR_FORCE_MDI is bit
+		 * counter intuitive, the "-X" in "1 = Force MDI" in the data
+		 * sheet seems to be missing:
+		 * 1 = Force MDI (sic!) (transmit on RX+/RX- pins)
+		 * 0 = Normal operation (transmit on TX+/TX- pins)
+		 */
+		val = KSZ886X_BMCR_DISABLE_AUTO_MDIX | KSZ886X_BMCR_FORCE_MDI;
+		break;
+	case ETH_TP_MDI_AUTO:
+		val = 0;
+		break;
+	default:
+		return 0;
+	}
+
+	return phy_modify(phydev, MII_BMCR,
+			  KSZ886X_BMCR_HP_MDIX | KSZ886X_BMCR_FORCE_MDI |
+			  KSZ886X_BMCR_DISABLE_AUTO_MDIX,
+			  KSZ886X_BMCR_HP_MDIX | val);
+}
+
+static int ksz886x_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	/* The MDI-X configuration is automatically changed by the PHY after
+	 * switching from autoneg off to on. So, take MDI-X configuration under
+	 * own control and set it after autoneg configuration was done.
+	 */
+	return ksz886x_config_mdix(phydev, phydev->mdix_ctrl);
+}
+
+static int ksz886x_mdix_update(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_BMCR);
+	if (ret < 0)
+		return ret;
+
+	if (ret & KSZ886X_BMCR_DISABLE_AUTO_MDIX) {
+		if (ret & KSZ886X_BMCR_FORCE_MDI)
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+		else
+			phydev->mdix_ctrl = ETH_TP_MDI;
+	} else {
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	}
+
+	ret = phy_read(phydev, MII_KSZPHY_CTRL);
+	if (ret < 0)
+		return ret;
+
+	if (ret & KSZ886X_CTRL_MDIX_STAT)
+		phydev->mdix = ETH_TP_MDI;
+	else
+		phydev->mdix = ETH_TP_MDI_X;
+
+	return 0;
+}
+
+static int ksz886x_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = ksz886x_mdix_update(phydev);
+	if (ret < 0)
+		return ret;
+
+	return genphy_read_status(phydev);
+}
+
 static int ksz886x_resume(struct phy_device *phydev)
 {
 	int ret;
@@ -1366,6 +1465,8 @@ static struct phy_driver ksphy_driver[] = {
 	.name		= "Micrel KSZ886X Switch",
 	/* PHY_BASIC_FEATURES */
 	.config_init	= kszphy_config_init,
+	.config_aneg	= ksz886x_config_aneg,
+	.read_status	= ksz886x_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= ksz886x_resume,
 }, {
-- 
2.27.0

Re: [PATCH net-next v1 3/5] net: phy: micrel: ksz886x add MDI-X support

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:49PM +0200, Oleksij Rempel wrote:
> Add support for MDI-X status and configuration
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew

[PATCH net-next v1 4/5] net: phy: micrel: ksz8081 add MDI-X support

[Oleksij Rempel]

Add support for MDI-X status and configuration

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 89 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index ec409b2cb984..221ba661645f 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -64,11 +64,17 @@
 
 /* PHY Control 1 */
 #define MII_KSZPHY_CTRL_1			0x1e
+#define KSZ8081_CTRL1_MDIX_STAT			BIT(4)
 
 /* PHY Control 2 / PHY Control (if no PHY Control 1) */
 #define MII_KSZPHY_CTRL_2			0x1f
 #define MII_KSZPHY_CTRL				MII_KSZPHY_CTRL_2
 /* bitmap of PHY register to set interrupt mode */
+#define KSZ8081_CTRL2_HP_MDIX			BIT(15)
+#define KSZ8081_CTRL2_MDI_MDI_X_SELECT		BIT(14)
+#define KSZ8081_CTRL2_DISABLE_AUTO_MDIX		BIT(13)
+#define KSZ8081_CTRL2_FORCE_LINK		BIT(11)
+#define KSZ8081_CTRL2_POWER_SAVING		BIT(10)
 #define KSZPHY_CTRL_INT_ACTIVE_HIGH		BIT(9)
 #define KSZPHY_RMII_REF_CLK_SEL			BIT(7)
 #define KSZ886X_CTRL_MDIX_STAT			BIT(4)
@@ -398,6 +404,87 @@ static int ksz8081_config_init(struct phy_device *phydev)
 	return kszphy_config_init(phydev);
 }
 
+static int ksz8081_config_mdix(struct phy_device *phydev, u8 ctrl)
+{
+	u16 val;
+
+	switch (ctrl) {
+	case ETH_TP_MDI:
+		val = KSZ8081_CTRL2_DISABLE_AUTO_MDIX;
+		break;
+	case ETH_TP_MDI_X:
+		val = KSZ8081_CTRL2_DISABLE_AUTO_MDIX |
+			KSZ8081_CTRL2_MDI_MDI_X_SELECT;
+		break;
+	case ETH_TP_MDI_AUTO:
+		val = 0;
+		break;
+	default:
+		return 0;
+	}
+
+	return phy_modify(phydev, MII_KSZPHY_CTRL_2,
+			  KSZ8081_CTRL2_HP_MDIX |
+			  KSZ8081_CTRL2_MDI_MDI_X_SELECT |
+			  KSZ8081_CTRL2_DISABLE_AUTO_MDIX,
+			  KSZ8081_CTRL2_HP_MDIX | val);
+}
+
+static int ksz8081_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_config_aneg(phydev);
+	if (ret)
+		return ret;
+
+	/* The MDI-X configuration is automatically changed by the PHY after
+	 * switching from autoneg off to on. So, take MDI-X configuration under
+	 * own control and set it after autoneg configuration was done.
+	 */
+	return ksz8081_config_mdix(phydev, phydev->mdix_ctrl);
+}
+
+static int ksz8081_mdix_update(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_KSZPHY_CTRL_2);
+	if (ret < 0)
+		return ret;
+
+	if (ret & KSZ8081_CTRL2_DISABLE_AUTO_MDIX) {
+		if (ret & KSZ8081_CTRL2_MDI_MDI_X_SELECT)
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+		else
+			phydev->mdix_ctrl = ETH_TP_MDI;
+	} else {
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	}
+
+	ret = phy_read(phydev, MII_KSZPHY_CTRL_1);
+	if (ret < 0)
+		return ret;
+
+	if (ret & KSZ8081_CTRL1_MDIX_STAT)
+		phydev->mdix = ETH_TP_MDI;
+	else
+		phydev->mdix = ETH_TP_MDI_X;
+
+	return 0;
+}
+
+static int ksz8081_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = ksz8081_mdix_update(phydev);
+	if (ret < 0)
+		return ret;
+
+	return genphy_read_status(phydev);
+}
+
 static int ksz8061_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -1381,6 +1468,8 @@ static struct phy_driver ksphy_driver[] = {
 	.driver_data	= &ksz8081_type,
 	.probe		= kszphy_probe,
 	.config_init	= ksz8081_config_init,
+	.config_aneg	= ksz8081_config_aneg,
+	.read_status	= ksz8081_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.get_sset_count = kszphy_get_sset_count,
-- 
2.27.0

Re: [PATCH net-next v1 4/5] net: phy: micrel: ksz8081 add MDI-X support

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:50PM +0200, Oleksij Rempel wrote:
> Add support for MDI-X status and configuration
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew

[PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Oleksij Rempel]

This patch support for cable test for the ksz886x switches and the
ksz8081 PHY.

The patch was tested on a KSZ8873RLL switch with following results:

- port 1:
  - cannot detect any distance
  - provides inverted values
    (Errata: DS80000830A: "LinkMD does not work on Port 1",
     http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
    - Reports "short" on open or ok.
    - Reports "ok" on short.

- port 2:
  - can detect distance
  - can detect open on each wire of pair A (wire 1 and 2)
  - can detect open only on one wire of pair B (only wire 3)
  - can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
  - short between pairs is detected as open.
    For example short between wires 2 + 3 is detected as open.

In order to work around the errata for port 1, the ksz8795 switch driver
should be extended to provide proper device tree support for the related
PHY nodes. So we can set a DT property to mark the port 1 as affected by
the errata.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 175 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 221ba661645f..0c86ab2e483e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -20,6 +20,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/phy.h>
@@ -62,6 +63,18 @@
 #define KSZPHY_INTCS_ALL			(KSZPHY_INTCS_LINK_UP |\
 						KSZPHY_INTCS_LINK_DOWN)
 
+/* LinkMD Control/Status */
+#define KSZ8081_LMD				0x1d
+#define KSZ8081_LMD_ENABLE_TEST			BIT(15)
+#define KSZ8081_LMD_STAT_NORMAL			0
+#define KSZ8081_LMD_STAT_OPEN			1
+#define KSZ8081_LMD_STAT_SHORT			2
+#define KSZ8081_LMD_STAT_FAIL			3
+#define KSZ8081_LMD_STAT_MASK			GENMASK(14, 13)
+/* Short cable (<10 meter) has been detected by LinkMD */
+#define KSZ8081_LMD_SHORT_INDICATOR		BIT(12)
+#define KSZ8081_LMD_DELTA_TIME_MASK		GENMASK(8, 0)
+
 /* PHY Control 1 */
 #define MII_KSZPHY_CTRL_1			0x1e
 #define KSZ8081_CTRL1_MDIX_STAT			BIT(4)
@@ -1358,6 +1371,164 @@ static int kszphy_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz886x_cable_test_start(struct phy_device *phydev)
+{
+	/* If autoneg is enabled, we won't be able to test cross pair
+	 * short. In this case, the PHY will "detect" a link and
+	 * confuse the internal state machine - disable auto neg here.
+	 * If autoneg is disabled, we should set the speed to 10mbit.
+	 */
+	return phy_clear_bits(phydev, MII_BMCR, BMCR_ANENABLE | BMCR_SPEED100);
+}
+
+static int ksz886x_cable_test_result_trans(u16 status)
+{
+	switch (FIELD_GET(KSZ8081_LMD_STAT_MASK, status)) {
+	case KSZ8081_LMD_STAT_NORMAL:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case KSZ8081_LMD_STAT_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case KSZ8081_LMD_STAT_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case KSZ8081_LMD_STAT_FAIL:
+		/* fall through */
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static bool ksz886x_cable_test_failed(u16 status)
+{
+	return FIELD_GET(KSZ8081_LMD_STAT_MASK, status) ==
+		KSZ8081_LMD_STAT_FAIL;
+}
+
+static bool ksz886x_cable_test_fault_length_valid(u16 status)
+{
+	switch (FIELD_GET(KSZ8081_LMD_STAT_MASK, status)) {
+	case KSZ8081_LMD_STAT_OPEN:
+		/* fall through */
+	case KSZ8081_LMD_STAT_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int ksz886x_cable_test_fault_length(u16 status)
+{
+	int dt;
+
+	/* According to the data sheet the distance to the fault is
+	 * DELTA_TIME * 0.4 meters.
+	 */
+	dt = FIELD_GET(KSZ8081_LMD_DELTA_TIME_MASK, status);
+
+	return (dt * 400) / 10;
+}
+
+static int ksz886x_cable_test_wait_for_completion(struct phy_device *phydev)
+{
+	int val, ret;
+
+	ret = phy_read_poll_timeout(phydev, KSZ8081_LMD, val,
+				    !(val & KSZ8081_LMD_ENABLE_TEST),
+				    30000, 100000, true);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ksz886x_cable_test_one_pair(struct phy_device *phydev, int pair)
+{
+	static const int ethtool_pair[] = {
+		ETHTOOL_A_CABLE_PAIR_A,
+		ETHTOOL_A_CABLE_PAIR_B,
+	};
+	int ret, val, mdix;
+
+	/* There is no way to choice the pair, like we do one ksz9031.
+	 * We can workaround this limitation by using the MDI-X functionality.
+	 */
+	if (pair == 0)
+		mdix = ETH_TP_MDI;
+	else
+		mdix = ETH_TP_MDI_X;
+
+	switch (phydev->phy_id & MICREL_PHY_ID_MASK) {
+	case PHY_ID_KSZ8081:
+		ret = ksz8081_config_mdix(phydev, mdix);
+		break;
+	case PHY_ID_KSZ886X:
+		ret = ksz886x_config_mdix(phydev, mdix);
+		break;
+	default:
+		ret = -ENODEV;
+	}
+
+	if (ret)
+		return ret;
+
+	/* Now we are ready to fire. This command will send a 100ns pulse
+	 * to the pair.
+	 */
+	ret = phy_write(phydev, KSZ8081_LMD, KSZ8081_LMD_ENABLE_TEST);
+	if (ret)
+		return ret;
+
+	ret = ksz886x_cable_test_wait_for_completion(phydev);
+	if (ret)
+		return ret;
+
+	val = phy_read(phydev, KSZ8081_LMD);
+	if (val < 0)
+		return val;
+
+	if (ksz886x_cable_test_failed(val))
+		return -EAGAIN;
+
+	ret = ethnl_cable_test_result(phydev, ethtool_pair[pair],
+				      ksz886x_cable_test_result_trans(val));
+	if (ret)
+		return ret;
+
+	if (!ksz886x_cable_test_fault_length_valid(val))
+		return 0;
+
+	return ethnl_cable_test_fault_length(phydev, ethtool_pair[pair],
+					     ksz886x_cable_test_fault_length(val));
+}
+
+static int ksz886x_cable_test_get_status(struct phy_device *phydev,
+					 bool *finished)
+{
+	unsigned long pair_mask = 0x3;
+	int retries = 20;
+	int pair, ret;
+
+	*finished = false;
+
+	/* Try harder if link partner is active */
+	while (pair_mask && retries--) {
+		for_each_set_bit(pair, &pair_mask, 4) {
+			ret = ksz886x_cable_test_one_pair(phydev, pair);
+			if (ret == -EAGAIN)
+				continue;
+			if (ret < 0)
+				return ret;
+			clear_bit(pair, &pair_mask);
+		}
+		/* If link partner is in autonegotiation mode it will send 2ms
+		 * of FLPs with at least 6ms of silence.
+		 * Add 2ms sleep to have better chances to hit this silence.
+		 */
+		if (pair_mask)
+			msleep(2);
+	}
+
+	*finished = true;
+
+	return 0;
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,
@@ -1477,6 +1648,8 @@ static struct phy_driver ksphy_driver[] = {
 	.get_stats	= kszphy_get_stats,
 	.suspend	= kszphy_suspend,
 	.resume		= kszphy_resume,
+	.cable_test_start	= ksz886x_cable_test_start,
+	.cable_test_get_status	= ksz886x_cable_test_get_status,
 }, {
 	.phy_id		= PHY_ID_KSZ8061,
 	.name		= "Micrel KSZ8061",
@@ -1558,6 +1731,8 @@ static struct phy_driver ksphy_driver[] = {
 	.read_status	= ksz886x_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= ksz886x_resume,
+	.cable_test_start	= ksz886x_cable_test_start,
+	.cable_test_get_status	= ksz886x_cable_test_get_status,
 }, {
 	.name		= "Micrel KSZ87XX Switch",
 	/* PHY_BASIC_FEATURES */
-- 
2.27.0

Re: [PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote:
> This patch support for cable test for the ksz886x switches and the
> ksz8081 PHY.
> 
> The patch was tested on a KSZ8873RLL switch with following results:
> 
> - port 1:
>   - cannot detect any distance
>   - provides inverted values
>     (Errata: DS80000830A: "LinkMD does not work on Port 1",
>      http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
>     - Reports "short" on open or ok.
>     - Reports "ok" on short.
> 
> - port 2:
>   - can detect distance
>   - can detect open on each wire of pair A (wire 1 and 2)
>   - can detect open only on one wire of pair B (only wire 3)
>   - can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
>   - short between pairs is detected as open.
>     For example short between wires 2 + 3 is detected as open.
> 
> In order to work around the errata for port 1, the ksz8795 switch driver
> should be extended to provide proper device tree support for the related
> PHY nodes. So we can set a DT property to mark the port 1 as affected by
> the errata.

Hi Oleksij

Do the PHY register read/writes pass through the DSA driver for the
8873?  I was wondering if the switch could intercept reads/writes on
port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more
robust solution than DT properties, which are going to get forgotten.

      Andrew

Re: [PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Oleksij Rempel]

On Sat, Jul 11, 2020 at 08:29:12PM +0200, Andrew Lunn wrote:
> On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote:
> > This patch support for cable test for the ksz886x switches and the
> > ksz8081 PHY.
> > 
> > The patch was tested on a KSZ8873RLL switch with following results:
> > 
> > - port 1:
> >   - cannot detect any distance
> >   - provides inverted values
> >     (Errata: DS80000830A: "LinkMD does not work on Port 1",
> >      http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
> >     - Reports "short" on open or ok.
> >     - Reports "ok" on short.
> > 
> > - port 2:
> >   - can detect distance
> >   - can detect open on each wire of pair A (wire 1 and 2)
> >   - can detect open only on one wire of pair B (only wire 3)
> >   - can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
> >   - short between pairs is detected as open.
> >     For example short between wires 2 + 3 is detected as open.
> > 
> > In order to work around the errata for port 1, the ksz8795 switch driver
> > should be extended to provide proper device tree support for the related
> > PHY nodes. So we can set a DT property to mark the port 1 as affected by
> > the errata.

Hi Andrew,
 
> Hi Oleksij
> 
> Do the PHY register read/writes pass through the DSA driver for the
> 8873?  I was wondering if the switch could intercept reads/writes on
> port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more
> robust solution than DT properties, which are going to get forgotten.

Yes, it was my first idea as well. But this switch allows direct MDIO
access to the PHYs and this PHY driver could be used without DSA driver.
Not sure if we should support both variants?

Beside, the Port 1 need at least one more quirk. The pause souport is
announced but is not working. Should we some how clear Puase bit in the PHY and
tell PHY framework to not use it? What is the best way to do it?

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 net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Oleksij Rempel]

On Mon, Jul 13, 2020 at 06:11:30AM +0200, Oleksij Rempel wrote:
> On Sat, Jul 11, 2020 at 08:29:12PM +0200, Andrew Lunn wrote:
> > On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote:
> > > This patch support for cable test for the ksz886x switches and the
> > > ksz8081 PHY.
> > > 
> > > The patch was tested on a KSZ8873RLL switch with following results:
> > > 
> > > - port 1:
> > >   - cannot detect any distance
> > >   - provides inverted values
> > >     (Errata: DS80000830A: "LinkMD does not work on Port 1",
> > >      http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
> > >     - Reports "short" on open or ok.
> > >     - Reports "ok" on short.
> > > 
> > > - port 2:
> > >   - can detect distance
> > >   - can detect open on each wire of pair A (wire 1 and 2)
> > >   - can detect open only on one wire of pair B (only wire 3)
> > >   - can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
> > >   - short between pairs is detected as open.
> > >     For example short between wires 2 + 3 is detected as open.
> > > 
> > > In order to work around the errata for port 1, the ksz8795 switch driver
> > > should be extended to provide proper device tree support for the related
> > > PHY nodes. So we can set a DT property to mark the port 1 as affected by
> > > the errata.
> 
> Hi Andrew,
>  
> > Hi Oleksij
> > 
> > Do the PHY register read/writes pass through the DSA driver for the
> > 8873?  I was wondering if the switch could intercept reads/writes on
> > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more
> > robust solution than DT properties, which are going to get forgotten.
> 
> Yes, it was my first idea as well. But this switch allows direct MDIO
> access to the PHYs and this PHY driver could be used without DSA driver.
> Not sure if we should support both variants?
> 
> Beside, the Port 1 need at least one more quirk. The pause souport is
> announced but is not working. Should we some how clear Puase bit in the PHY and
> tell PHY framework to not use it? What is the best way to do it?

On other hand, if adding this quirks in to switch driver is acceptable
way, i'll be happy with this as well.

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 net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Andrew Lunn]

> > Hi Oleksij
> > 
> > Do the PHY register read/writes pass through the DSA driver for the
> > 8873?  I was wondering if the switch could intercept reads/writes on
> > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more
> > robust solution than DT properties, which are going to get forgotten.
> 
> Yes, it was my first idea as well. But this switch allows direct MDIO
> access to the PHYs and this PHY driver could be used without DSA driver.
> Not sure if we should support both variants?
> 
> Beside, the Port 1 need at least one more quirk. The pause souport is
> announced but is not working. Should we some how clear Puase bit in the PHY and
> tell PHY framework to not use it? What is the best way to do it?

It all seems rather odd, the way one PHY is messed up, but the other
works. Does this PHY exist as a standalone device, not integrated into
the switch? Do the same erratas apply to such a standalone device?

If the issues are just limited to integrated PHYs, there is maybe
something you can do via DSA:

in slave.c:

static int dsa_slave_phy_setup(struct net_device *slave_dev)
{
...
        if (ds->ops->get_phy_flags)
                phy_flags = ds->ops->get_phy_flags(ds, dp->index);

        ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);

It is either B53 or SF2 which uses this, i forget which. flags get
or'ed into phydev->dev_flags. These are device specific flags. So you
could define a bit to represent this errata. And then in the PHY
driver do whatever needs to be done when you see the flag set for a
specific PHY.

If Pause is broken, then yes, it would be good to remove the Pause
from the available features, and return an error if requested to use
it.

       Andrew

Re: [PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Oleksij Rempel]

On Mon, Jul 13, 2020 at 05:17:19PM +0200, Andrew Lunn wrote:
> > > Hi Oleksij
> > > 
> > > Do the PHY register read/writes pass through the DSA driver for the
> > > 8873?  I was wondering if the switch could intercept reads/writes on
> > > port1 for KSZ8081_LMD and return EOPNOTSUPP? That would be a more
> > > robust solution than DT properties, which are going to get forgotten.
> > 
> > Yes, it was my first idea as well. But this switch allows direct MDIO
> > access to the PHYs and this PHY driver could be used without DSA driver.
> > Not sure if we should support both variants?
> > 
> > Beside, the Port 1 need at least one more quirk. The pause souport is
> > announced but is not working. Should we some how clear Puase bit in the PHY and
> > tell PHY framework to not use it? What is the best way to do it?
> 
> It all seems rather odd, the way one PHY is messed up, but the other
> works. Does this PHY exist as a standalone device, not integrated into
> the switch? Do the same erratas apply to such a standalone device?

I found multiple microchip devices with same PHYid: KSZ8463, KSZ8851
KSZ8463 - switch. Would be covered by DSA driver
KSZ8851 - single MAC device with PHY. Supported by ethernet/micrel
driver.

This erratum is not documented for other devices. So it may exist or
not.

> If the issues are just limited to integrated PHYs, there is maybe
> something you can do via DSA:
> 
> in slave.c:
> 
> static int dsa_slave_phy_setup(struct net_device *slave_dev)
> {
> ...
>         if (ds->ops->get_phy_flags)
>                 phy_flags = ds->ops->get_phy_flags(ds, dp->index);
> 
>         ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
> 
> It is either B53 or SF2 which uses this, i forget which. flags get
> or'ed into phydev->dev_flags. These are device specific flags. So you
> could define a bit to represent this errata. And then in the PHY
> driver do whatever needs to be done when you see the flag set for a
> specific PHY.
> 
> If Pause is broken, then yes, it would be good to remove the Pause
> from the available features, and return an error if requested to use
> it.

OK. So, i'll cover both errata with separate flags? Set flags in the DSA
driver and apply workarounds in the PHY. ACK?

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 net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Andrew Lunn]

> OK. So, i'll cover both errata with separate flags? Set flags in the DSA
> driver and apply workarounds in the PHY. ACK?

Yes. Assume the issues are limited to just the first PHY in this
switch. If there are discrete PHYs with the same issue, we can come up
with a different way to identify them.

     Andrew

Re: [PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Tue, Jul 14, 2020 at 03:12:40PM +0200, Andrew Lunn wrote:
> > OK. So, i'll cover both errata with separate flags? Set flags in the DSA
> > driver and apply workarounds in the PHY. ACK?
> 
> Yes. Assume the issues are limited to just the first PHY in this
> switch. If there are discrete PHYs with the same issue, we can come up
> with a different way to identify them.

I assume, it is not the blocker for the current patch set?

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v1 5/5] net: phy: micrel: ksz886x/ksz8081: add cabletest support

[Andrew Lunn]

On Fri, Jul 10, 2020 at 02:08:51PM +0200, Oleksij Rempel wrote:
> This patch support for cable test for the ksz886x switches and the
> ksz8081 PHY.
> 
> The patch was tested on a KSZ8873RLL switch with following results:
> 
> - port 1:
>   - cannot detect any distance
>   - provides inverted values
>     (Errata: DS80000830A: "LinkMD does not work on Port 1",
>      http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)
>     - Reports "short" on open or ok.
>     - Reports "ok" on short.
> 
> - port 2:
>   - can detect distance
>   - can detect open on each wire of pair A (wire 1 and 2)
>   - can detect open only on one wire of pair B (only wire 3)
>   - can detect short between wires of a pair (wires 1 + 2 or 3 + 6)
>   - short between pairs is detected as open.
>     For example short between wires 2 + 3 is detected as open.
> 
> In order to work around the errata for port 1, the ksz8795 switch driver
> should be extended to provide proper device tree support for the related
> PHY nodes. So we can set a DT property to mark the port 1 as affected by
> the errata.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

    Andrew