[PATCH net-next v3 0/9] provide cable test support for the ksz886x switch

[PATCH net-next v3 0/9] provide cable test support for the ksz886x switch

[Oleksij Rempel]

Since we already have 5.13-rc3, I assume http://vger.kernel.org/~davem/net-next.html
is out of date.

changes v3:
- remove RFC tag

changes v2:
- use generic MII_* defines where possible
- rework phylink validate
- remove phylink get state function
- reorder cabletest patches to make PHY flag patch in the right order
- fix MDI-X detection

This patches provide support for cable testing on the ksz886x switches.
Since it has one special port, we needed to add phylink with validation
and extra quirk for the PHY to signal, that one port will not provide
valid cable testing reports.

Michael Grzeschik (2):
  net: phy: micrel: move phy reg offsets to common header
  net: dsa: microchip: ksz8795: add phylink support

Oleksij Rempel (7):
  net: phy: micrel: use consistent indention after define
  net: phy: micrel: apply resume errata workaround for ksz8873 and
    ksz8863
  net: phy/dsa micrel/ksz886x add MDI-X support
  net: phy: micrel: ksz8081 add MDI-X support
  net: dsa: microchip: ksz8795: add LINK_MD register support
  net: dsa: dsa_slave_phy_connect(): extend phy's flags with port
    specific phy flags
  net: phy: micrel: ksz886x/ksz8081: add cabletest support

 drivers/net/dsa/microchip/ksz8795.c     | 218 +++++++++----
 drivers/net/dsa/microchip/ksz8795_reg.h |  67 +---
 drivers/net/ethernet/micrel/ksz884x.c   | 105 +-----
 drivers/net/phy/micrel.c                | 403 +++++++++++++++++++++++-
 drivers/net/phy/phylink.c               |   2 +-
 include/linux/micrel_phy.h              |  16 +
 net/dsa/slave.c                         |   4 +
 7 files changed, 588 insertions(+), 227 deletions(-)

-- 
2.29.2

[PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header

[Oleksij Rempel]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

Some micrel devices share the same PHY register defines. This patch
moves them to one common header so other drivers can reuse them.
And reuse generic MII_* defines where possible.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 119 ++++++++++++------------
 drivers/net/dsa/microchip/ksz8795_reg.h |  62 ------------
 drivers/net/ethernet/micrel/ksz884x.c   | 105 +++------------------
 include/linux/micrel_phy.h              |  13 +++
 4 files changed, 88 insertions(+), 211 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index ad509a57a945..ba065003623f 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -15,6 +15,7 @@
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
+#include <linux/micrel_phy.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
 
@@ -731,88 +732,88 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 	u8 p = phy;
 
 	switch (reg) {
-	case PHY_REG_CTRL:
+	case MII_BMCR:
 		ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
 		ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
 		ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
 		if (restart & PORT_PHY_LOOPBACK)
-			data |= PHY_LOOPBACK;
+			data |= BMCR_LOOPBACK;
 		if (ctrl & PORT_FORCE_100_MBIT)
-			data |= PHY_SPEED_100MBIT;
+			data |= BMCR_SPEED100;
 		if (ksz_is_ksz88x3(dev)) {
 			if ((ctrl & PORT_AUTO_NEG_ENABLE))
-				data |= PHY_AUTO_NEG_ENABLE;
+				data |= BMCR_ANENABLE;
 		} else {
 			if (!(ctrl & PORT_AUTO_NEG_DISABLE))
-				data |= PHY_AUTO_NEG_ENABLE;
+				data |= BMCR_ANENABLE;
 		}
 		if (restart & PORT_POWER_DOWN)
-			data |= PHY_POWER_DOWN;
+			data |= BMCR_PDOWN;
 		if (restart & PORT_AUTO_NEG_RESTART)
-			data |= PHY_AUTO_NEG_RESTART;
+			data |= BMCR_ANRESTART;
 		if (ctrl & PORT_FORCE_FULL_DUPLEX)
-			data |= PHY_FULL_DUPLEX;
+			data |= BMCR_FULLDPLX;
 		if (speed & PORT_HP_MDIX)
-			data |= PHY_HP_MDIX;
+			data |= KSZ886X_BMCR_HP_MDIX;
 		if (restart & PORT_FORCE_MDIX)
-			data |= PHY_FORCE_MDIX;
+			data |= KSZ886X_BMCR_FORCE_MDI;
 		if (restart & PORT_AUTO_MDIX_DISABLE)
-			data |= PHY_AUTO_MDIX_DISABLE;
+			data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
 		if (restart & PORT_TX_DISABLE)
-			data |= PHY_TRANSMIT_DISABLE;
+			data |= KSZ886X_BMCR_DISABLE_TRANSMIT;
 		if (restart & PORT_LED_OFF)
-			data |= PHY_LED_DISABLE;
+			data |= KSZ886X_BMCR_DISABLE_LED;
 		break;
-	case PHY_REG_STATUS:
+	case MII_BMSR:
 		ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
-		data = PHY_100BTX_FD_CAPABLE |
-		       PHY_100BTX_CAPABLE |
-		       PHY_10BT_FD_CAPABLE |
-		       PHY_10BT_CAPABLE |
-		       PHY_AUTO_NEG_CAPABLE;
+		data = BMSR_100FULL |
+		       BMSR_100HALF |
+		       BMSR_10FULL |
+		       BMSR_10HALF |
+		       BMSR_ANEGCAPABLE;
 		if (link & PORT_AUTO_NEG_COMPLETE)
-			data |= PHY_AUTO_NEG_ACKNOWLEDGE;
+			data |= BMSR_ANEGCOMPLETE;
 		if (link & PORT_STAT_LINK_GOOD)
-			data |= PHY_LINK_STATUS;
+			data |= BMSR_LSTATUS;
 		break;
-	case PHY_REG_ID_1:
+	case MII_PHYSID1:
 		data = KSZ8795_ID_HI;
 		break;
-	case PHY_REG_ID_2:
+	case MII_PHYSID2:
 		if (ksz_is_ksz88x3(dev))
 			data = KSZ8863_ID_LO;
 		else
 			data = KSZ8795_ID_LO;
 		break;
-	case PHY_REG_AUTO_NEGOTIATION:
+	case MII_ADVERTISE:
 		ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
-		data = PHY_AUTO_NEG_802_3;
+		data = ADVERTISE_CSMA;
 		if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
-			data |= PHY_AUTO_NEG_SYM_PAUSE;
+			data |= ADVERTISE_PAUSE_CAP;
 		if (ctrl & PORT_AUTO_NEG_100BTX_FD)
-			data |= PHY_AUTO_NEG_100BTX_FD;
+			data |= ADVERTISE_100FULL;
 		if (ctrl & PORT_AUTO_NEG_100BTX)
-			data |= PHY_AUTO_NEG_100BTX;
+			data |= ADVERTISE_100HALF;
 		if (ctrl & PORT_AUTO_NEG_10BT_FD)
-			data |= PHY_AUTO_NEG_10BT_FD;
+			data |= ADVERTISE_10FULL;
 		if (ctrl & PORT_AUTO_NEG_10BT)
-			data |= PHY_AUTO_NEG_10BT;
+			data |= ADVERTISE_10HALF;
 		break;
-	case PHY_REG_REMOTE_CAPABILITY:
+	case MII_LPA:
 		ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link);
-		data = PHY_AUTO_NEG_802_3;
+		data = LPA_SLCT;
 		if (link & PORT_REMOTE_SYM_PAUSE)
-			data |= PHY_AUTO_NEG_SYM_PAUSE;
+			data |= LPA_PAUSE_CAP;
 		if (link & PORT_REMOTE_100BTX_FD)
-			data |= PHY_AUTO_NEG_100BTX_FD;
+			data |= LPA_100FULL;
 		if (link & PORT_REMOTE_100BTX)
-			data |= PHY_AUTO_NEG_100BTX;
+			data |= LPA_100HALF;
 		if (link & PORT_REMOTE_10BT_FD)
-			data |= PHY_AUTO_NEG_10BT_FD;
+			data |= LPA_10FULL;
 		if (link & PORT_REMOTE_10BT)
-			data |= PHY_AUTO_NEG_10BT;
-		if (data & ~PHY_AUTO_NEG_802_3)
-			data |= PHY_REMOTE_ACKNOWLEDGE_NOT;
+			data |= LPA_10HALF;
+		if (data & ~LPA_SLCT)
+			data |= LPA_LPACK;
 		break;
 	default:
 		processed = false;
@@ -830,14 +831,14 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 	u8 p = phy;
 
 	switch (reg) {
-	case PHY_REG_CTRL:
+	case MII_BMCR:
 
 		/* Do not support PHY reset function. */
-		if (val & PHY_RESET)
+		if (val & BMCR_RESET)
 			break;
 		ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
 		data = speed;
-		if (val & PHY_HP_MDIX)
+		if (val & KSZ886X_BMCR_HP_MDIX)
 			data |= PORT_HP_MDIX;
 		else
 			data &= ~PORT_HP_MDIX;
@@ -846,12 +847,12 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 		ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
 		data = ctrl;
 		if (ksz_is_ksz88x3(dev)) {
-			if ((val & PHY_AUTO_NEG_ENABLE))
+			if ((val & BMCR_ANENABLE))
 				data |= PORT_AUTO_NEG_ENABLE;
 			else
 				data &= ~PORT_AUTO_NEG_ENABLE;
 		} else {
-			if (!(val & PHY_AUTO_NEG_ENABLE))
+			if (!(val & BMCR_ANENABLE))
 				data |= PORT_AUTO_NEG_DISABLE;
 			else
 				data &= ~PORT_AUTO_NEG_DISABLE;
@@ -861,11 +862,11 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 				data |= PORT_AUTO_NEG_DISABLE;
 		}
 
-		if (val & PHY_SPEED_100MBIT)
+		if (val & BMCR_SPEED100)
 			data |= PORT_FORCE_100_MBIT;
 		else
 			data &= ~PORT_FORCE_100_MBIT;
-		if (val & PHY_FULL_DUPLEX)
+		if (val & BMCR_FULLDPLX)
 			data |= PORT_FORCE_FULL_DUPLEX;
 		else
 			data &= ~PORT_FORCE_FULL_DUPLEX;
@@ -873,38 +874,38 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			ksz_pwrite8(dev, p, regs[P_FORCE_CTRL], data);
 		ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
 		data = restart;
-		if (val & PHY_LED_DISABLE)
+		if (val & KSZ886X_BMCR_DISABLE_LED)
 			data |= PORT_LED_OFF;
 		else
 			data &= ~PORT_LED_OFF;
-		if (val & PHY_TRANSMIT_DISABLE)
+		if (val & KSZ886X_BMCR_DISABLE_TRANSMIT)
 			data |= PORT_TX_DISABLE;
 		else
 			data &= ~PORT_TX_DISABLE;
-		if (val & PHY_AUTO_NEG_RESTART)
+		if (val & BMCR_ANRESTART)
 			data |= PORT_AUTO_NEG_RESTART;
 		else
 			data &= ~(PORT_AUTO_NEG_RESTART);
-		if (val & PHY_POWER_DOWN)
+		if (val & BMCR_PDOWN)
 			data |= PORT_POWER_DOWN;
 		else
 			data &= ~PORT_POWER_DOWN;
-		if (val & PHY_AUTO_MDIX_DISABLE)
+		if (val & KSZ886X_BMCR_DISABLE_AUTO_MDIX)
 			data |= PORT_AUTO_MDIX_DISABLE;
 		else
 			data &= ~PORT_AUTO_MDIX_DISABLE;
-		if (val & PHY_FORCE_MDIX)
+		if (val & KSZ886X_BMCR_FORCE_MDI)
 			data |= PORT_FORCE_MDIX;
 		else
 			data &= ~PORT_FORCE_MDIX;
-		if (val & PHY_LOOPBACK)
+		if (val & BMCR_LOOPBACK)
 			data |= PORT_PHY_LOOPBACK;
 		else
 			data &= ~PORT_PHY_LOOPBACK;
 		if (data != restart)
 			ksz_pwrite8(dev, p, regs[P_NEG_RESTART_CTRL], data);
 		break;
-	case PHY_REG_AUTO_NEGOTIATION:
+	case MII_ADVERTISE:
 		ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
 		data = ctrl;
 		data &= ~(PORT_AUTO_NEG_SYM_PAUSE |
@@ -912,15 +913,15 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 			  PORT_AUTO_NEG_100BTX |
 			  PORT_AUTO_NEG_10BT_FD |
 			  PORT_AUTO_NEG_10BT);
-		if (val & PHY_AUTO_NEG_SYM_PAUSE)
+		if (val & ADVERTISE_PAUSE_CAP)
 			data |= PORT_AUTO_NEG_SYM_PAUSE;
-		if (val & PHY_AUTO_NEG_100BTX_FD)
+		if (val & ADVERTISE_100FULL)
 			data |= PORT_AUTO_NEG_100BTX_FD;
-		if (val & PHY_AUTO_NEG_100BTX)
+		if (val & ADVERTISE_100HALF)
 			data |= PORT_AUTO_NEG_100BTX;
-		if (val & PHY_AUTO_NEG_10BT_FD)
+		if (val & ADVERTISE_10FULL)
 			data |= PORT_AUTO_NEG_10BT_FD;
-		if (val & PHY_AUTO_NEG_10BT)
+		if (val & ADVERTISE_10HALF)
 			data |= PORT_AUTO_NEG_10BT;
 		if (data != ctrl)
 			ksz_pwrite8(dev, p, regs[P_LOCAL_CTRL], data);
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index c2e52c40a54c..f925ddee5238 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -744,68 +744,6 @@
 
 #define PORT_ACL_FORCE_DLR_MISS		BIT(0)
 
-#ifndef PHY_REG_CTRL
-#define PHY_REG_CTRL			0
-
-#define PHY_RESET			BIT(15)
-#define PHY_LOOPBACK			BIT(14)
-#define PHY_SPEED_100MBIT		BIT(13)
-#define PHY_AUTO_NEG_ENABLE		BIT(12)
-#define PHY_POWER_DOWN			BIT(11)
-#define PHY_MII_DISABLE			BIT(10)
-#define PHY_AUTO_NEG_RESTART		BIT(9)
-#define PHY_FULL_DUPLEX			BIT(8)
-#define PHY_COLLISION_TEST_NOT		BIT(7)
-#define PHY_HP_MDIX			BIT(5)
-#define PHY_FORCE_MDIX			BIT(4)
-#define PHY_AUTO_MDIX_DISABLE		BIT(3)
-#define PHY_REMOTE_FAULT_DISABLE	BIT(2)
-#define PHY_TRANSMIT_DISABLE		BIT(1)
-#define PHY_LED_DISABLE			BIT(0)
-
-#define PHY_REG_STATUS			1
-
-#define PHY_100BT4_CAPABLE		BIT(15)
-#define PHY_100BTX_FD_CAPABLE		BIT(14)
-#define PHY_100BTX_CAPABLE		BIT(13)
-#define PHY_10BT_FD_CAPABLE		BIT(12)
-#define PHY_10BT_CAPABLE		BIT(11)
-#define PHY_MII_SUPPRESS_CAPABLE_NOT	BIT(6)
-#define PHY_AUTO_NEG_ACKNOWLEDGE	BIT(5)
-#define PHY_REMOTE_FAULT		BIT(4)
-#define PHY_AUTO_NEG_CAPABLE		BIT(3)
-#define PHY_LINK_STATUS			BIT(2)
-#define PHY_JABBER_DETECT_NOT		BIT(1)
-#define PHY_EXTENDED_CAPABILITY		BIT(0)
-
-#define PHY_REG_ID_1			2
-#define PHY_REG_ID_2			3
-
-#define PHY_REG_AUTO_NEGOTIATION	4
-
-#define PHY_AUTO_NEG_NEXT_PAGE_NOT	BIT(15)
-#define PHY_AUTO_NEG_REMOTE_FAULT_NOT	BIT(13)
-#define PHY_AUTO_NEG_SYM_PAUSE		BIT(10)
-#define PHY_AUTO_NEG_100BT4		BIT(9)
-#define PHY_AUTO_NEG_100BTX_FD		BIT(8)
-#define PHY_AUTO_NEG_100BTX		BIT(7)
-#define PHY_AUTO_NEG_10BT_FD		BIT(6)
-#define PHY_AUTO_NEG_10BT		BIT(5)
-#define PHY_AUTO_NEG_SELECTOR		0x001F
-#define PHY_AUTO_NEG_802_3		0x0001
-
-#define PHY_REG_REMOTE_CAPABILITY	5
-
-#define PHY_REMOTE_NEXT_PAGE_NOT	BIT(15)
-#define PHY_REMOTE_ACKNOWLEDGE_NOT	BIT(14)
-#define PHY_REMOTE_REMOTE_FAULT_NOT	BIT(13)
-#define PHY_REMOTE_SYM_PAUSE		BIT(10)
-#define PHY_REMOTE_100BTX_FD		BIT(8)
-#define PHY_REMOTE_100BTX		BIT(7)
-#define PHY_REMOTE_10BT_FD		BIT(6)
-#define PHY_REMOTE_10BT			BIT(5)
-#endif
-
 #define KSZ8795_ID_HI			0x0022
 #define KSZ8795_ID_LO			0x1550
 #define KSZ8863_ID_LO			0x1430
diff --git a/drivers/net/ethernet/micrel/ksz884x.c b/drivers/net/ethernet/micrel/ksz884x.c
index 3532bfe936f6..7945eb5e2fe8 100644
--- a/drivers/net/ethernet/micrel/ksz884x.c
+++ b/drivers/net/ethernet/micrel/ksz884x.c
@@ -25,6 +25,7 @@
 #include <linux/crc32.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/micrel_phy.h>
 
 
 /* DMA Registers */
@@ -271,84 +272,15 @@
 
 #define KS884X_PHY_CTRL_OFFSET		0x00
 
-/* Mode Control Register */
-#define PHY_REG_CTRL			0
-
-#define PHY_RESET			0x8000
-#define PHY_LOOPBACK			0x4000
-#define PHY_SPEED_100MBIT		0x2000
-#define PHY_AUTO_NEG_ENABLE		0x1000
-#define PHY_POWER_DOWN			0x0800
-#define PHY_MII_DISABLE			0x0400
-#define PHY_AUTO_NEG_RESTART		0x0200
-#define PHY_FULL_DUPLEX			0x0100
-#define PHY_COLLISION_TEST		0x0080
-#define PHY_HP_MDIX			0x0020
-#define PHY_FORCE_MDIX			0x0010
-#define PHY_AUTO_MDIX_DISABLE		0x0008
-#define PHY_REMOTE_FAULT_DISABLE	0x0004
-#define PHY_TRANSMIT_DISABLE		0x0002
-#define PHY_LED_DISABLE			0x0001
-
 #define KS884X_PHY_STATUS_OFFSET	0x02
 
-/* Mode Status Register */
-#define PHY_REG_STATUS			1
-
-#define PHY_100BT4_CAPABLE		0x8000
-#define PHY_100BTX_FD_CAPABLE		0x4000
-#define PHY_100BTX_CAPABLE		0x2000
-#define PHY_10BT_FD_CAPABLE		0x1000
-#define PHY_10BT_CAPABLE		0x0800
-#define PHY_MII_SUPPRESS_CAPABLE	0x0040
-#define PHY_AUTO_NEG_ACKNOWLEDGE	0x0020
-#define PHY_REMOTE_FAULT		0x0010
-#define PHY_AUTO_NEG_CAPABLE		0x0008
-#define PHY_LINK_STATUS			0x0004
-#define PHY_JABBER_DETECT		0x0002
-#define PHY_EXTENDED_CAPABILITY		0x0001
-
 #define KS884X_PHY_ID_1_OFFSET		0x04
 #define KS884X_PHY_ID_2_OFFSET		0x06
 
-/* PHY Identifier Registers */
-#define PHY_REG_ID_1			2
-#define PHY_REG_ID_2			3
-
 #define KS884X_PHY_AUTO_NEG_OFFSET	0x08
 
-/* Auto-Negotiation Advertisement Register */
-#define PHY_REG_AUTO_NEGOTIATION	4
-
-#define PHY_AUTO_NEG_NEXT_PAGE		0x8000
-#define PHY_AUTO_NEG_REMOTE_FAULT	0x2000
-/* Not supported. */
-#define PHY_AUTO_NEG_ASYM_PAUSE		0x0800
-#define PHY_AUTO_NEG_SYM_PAUSE		0x0400
-#define PHY_AUTO_NEG_100BT4		0x0200
-#define PHY_AUTO_NEG_100BTX_FD		0x0100
-#define PHY_AUTO_NEG_100BTX		0x0080
-#define PHY_AUTO_NEG_10BT_FD		0x0040
-#define PHY_AUTO_NEG_10BT		0x0020
-#define PHY_AUTO_NEG_SELECTOR		0x001F
-#define PHY_AUTO_NEG_802_3		0x0001
-
-#define PHY_AUTO_NEG_PAUSE  (PHY_AUTO_NEG_SYM_PAUSE | PHY_AUTO_NEG_ASYM_PAUSE)
-
 #define KS884X_PHY_REMOTE_CAP_OFFSET	0x0A
 
-/* Auto-Negotiation Link Partner Ability Register */
-#define PHY_REG_REMOTE_CAPABILITY	5
-
-#define PHY_REMOTE_NEXT_PAGE		0x8000
-#define PHY_REMOTE_ACKNOWLEDGE		0x4000
-#define PHY_REMOTE_REMOTE_FAULT		0x2000
-#define PHY_REMOTE_SYM_PAUSE		0x0400
-#define PHY_REMOTE_100BTX_FD		0x0100
-#define PHY_REMOTE_100BTX		0x0080
-#define PHY_REMOTE_10BT_FD		0x0040
-#define PHY_REMOTE_10BT			0x0020
-
 /* P1VCT */
 #define KS884X_P1VCT_P			0x04F0
 #define KS884X_P1PHYCTRL_P		0x04F2
@@ -2886,15 +2818,6 @@ static void sw_block_addr(struct ksz_hw *hw)
 	}
 }
 
-#define PHY_LINK_SUPPORT		\
-	(PHY_AUTO_NEG_ASYM_PAUSE |	\
-	PHY_AUTO_NEG_SYM_PAUSE |	\
-	PHY_AUTO_NEG_100BT4 |		\
-	PHY_AUTO_NEG_100BTX_FD |	\
-	PHY_AUTO_NEG_100BTX |		\
-	PHY_AUTO_NEG_10BT_FD |		\
-	PHY_AUTO_NEG_10BT)
-
 static inline void hw_r_phy_ctrl(struct ksz_hw *hw, int phy, u16 *data)
 {
 	*data = readw(hw->io + phy + KS884X_PHY_CTRL_OFFSET);
@@ -3238,16 +3161,18 @@ static void determine_flow_ctrl(struct ksz_hw *hw, struct ksz_port *port,
 	rx = tx = 0;
 	if (port->force_link)
 		rx = tx = 1;
-	if (remote & PHY_AUTO_NEG_SYM_PAUSE) {
-		if (local & PHY_AUTO_NEG_SYM_PAUSE) {
+	if (remote & LPA_PAUSE_CAP) {
+		if (local & ADVERTISE_PAUSE_CAP) {
 			rx = tx = 1;
-		} else if ((remote & PHY_AUTO_NEG_ASYM_PAUSE) &&
-				(local & PHY_AUTO_NEG_PAUSE) ==
-				PHY_AUTO_NEG_ASYM_PAUSE) {
+		} else if ((remote & LPA_PAUSE_ASYM) &&
+			   (local &
+			    (ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM)) ==
+			   ADVERTISE_PAUSE_ASYM) {
 			tx = 1;
 		}
-	} else if (remote & PHY_AUTO_NEG_ASYM_PAUSE) {
-		if ((local & PHY_AUTO_NEG_PAUSE) == PHY_AUTO_NEG_PAUSE)
+	} else if (remote & LPA_PAUSE_ASYM) {
+		if ((local & (ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM))
+		    == (ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM))
 			rx = 1;
 	}
 	if (!hw->ksz_switch)
@@ -3428,16 +3353,16 @@ static void port_force_link_speed(struct ksz_port *port)
 		phy = KS884X_PHY_1_CTRL_OFFSET + p * PHY_CTRL_INTERVAL;
 		hw_r_phy_ctrl(hw, phy, &data);
 
-		data &= ~PHY_AUTO_NEG_ENABLE;
+		data &= ~BMCR_ANENABLE;
 
 		if (10 == port->speed)
-			data &= ~PHY_SPEED_100MBIT;
+			data &= ~BMCR_SPEED100;
 		else if (100 == port->speed)
-			data |= PHY_SPEED_100MBIT;
+			data |= BMCR_SPEED100;
 		if (1 == port->duplex)
-			data &= ~PHY_FULL_DUPLEX;
+			data &= ~BMCR_FULLDPLX;
 		else if (2 == port->duplex)
-			data |= PHY_FULL_DUPLEX;
+			data |= BMCR_FULLDPLX;
 		hw_w_phy_ctrl(hw, phy, data);
 	}
 }
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 416ee6dd2574..b03e2afcb53f 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -45,4 +45,17 @@
 #define MICREL_KSZ9021_RGMII_CLK_CTRL_PAD_SCEW	0x104
 #define MICREL_KSZ9021_RGMII_RX_DATA_PAD_SCEW	0x105
 
+/* 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)
+
 #endif /* _MICREL_PHY_H */
-- 
2.29.2

Re: [PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header

[Vladimir Oltean]

On Wed, May 26, 2021 at 06:30:29AM +0200, Oleksij Rempel wrote:
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> Some micrel devices share the same PHY register defines. This patch
> moves them to one common header so other drivers can reuse them.
> And reuse generic MII_* defines where possible.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c     | 119 ++++++++++++------------
>  drivers/net/dsa/microchip/ksz8795_reg.h |  62 ------------
>  drivers/net/ethernet/micrel/ksz884x.c   | 105 +++------------------
>  include/linux/micrel_phy.h              |  13 +++
>  4 files changed, 88 insertions(+), 211 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index ad509a57a945..ba065003623f 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -15,6 +15,7 @@
>  #include <linux/phy.h>
>  #include <linux/etherdevice.h>
>  #include <linux/if_bridge.h>
> +#include <linux/micrel_phy.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
>  
> @@ -731,88 +732,88 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
>  	u8 p = phy;
>  
>  	switch (reg) {
> -	case PHY_REG_CTRL:
> +	case MII_BMCR:
>  		ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
>  		ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
>  		ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
>  		if (restart & PORT_PHY_LOOPBACK)
> -			data |= PHY_LOOPBACK;
> +			data |= BMCR_LOOPBACK;
>  		if (ctrl & PORT_FORCE_100_MBIT)
> -			data |= PHY_SPEED_100MBIT;
> +			data |= BMCR_SPEED100;
>  		if (ksz_is_ksz88x3(dev)) {
>  			if ((ctrl & PORT_AUTO_NEG_ENABLE))
> -				data |= PHY_AUTO_NEG_ENABLE;
> +				data |= BMCR_ANENABLE;
>  		} else {
>  			if (!(ctrl & PORT_AUTO_NEG_DISABLE))
> -				data |= PHY_AUTO_NEG_ENABLE;
> +				data |= BMCR_ANENABLE;
>  		}
>  		if (restart & PORT_POWER_DOWN)
> -			data |= PHY_POWER_DOWN;
> +			data |= BMCR_PDOWN;
>  		if (restart & PORT_AUTO_NEG_RESTART)
> -			data |= PHY_AUTO_NEG_RESTART;
> +			data |= BMCR_ANRESTART;
>  		if (ctrl & PORT_FORCE_FULL_DUPLEX)
> -			data |= PHY_FULL_DUPLEX;
> +			data |= BMCR_FULLDPLX;
>  		if (speed & PORT_HP_MDIX)
> -			data |= PHY_HP_MDIX;
> +			data |= KSZ886X_BMCR_HP_MDIX;
>  		if (restart & PORT_FORCE_MDIX)
> -			data |= PHY_FORCE_MDIX;
> +			data |= KSZ886X_BMCR_FORCE_MDI;
>  		if (restart & PORT_AUTO_MDIX_DISABLE)
> -			data |= PHY_AUTO_MDIX_DISABLE;
> +			data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
>  		if (restart & PORT_TX_DISABLE)
> -			data |= PHY_TRANSMIT_DISABLE;
> +			data |= KSZ886X_BMCR_DISABLE_TRANSMIT;
>  		if (restart & PORT_LED_OFF)
> -			data |= PHY_LED_DISABLE;
> +			data |= KSZ886X_BMCR_DISABLE_LED;
>  		break;

I am deeply confused as to what this function is doing. It is reading
the 8-bit port registers P_NEG_RESTART_CTRL, P_SPEED_STATUS and
P_FORCE_CTRL and stitching them into a 16-bit "MII_BMCR"?

What layout does this control register even have? Seeing as this is the
implementation of ksz_phy_read16(), I expect that MII_BMCR has the
layout specified in clause 22.2.4.1?

But clause 22 says register 0.5 is "Unidirectional enable", not
"PHY_HP_MDIX" (whatever that might be), and bits 0.4:0 are reserved and
must be written as zero and ignored on read.

> -	case PHY_REG_STATUS:
> +	case MII_BMSR:
>  		ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
> -		data = PHY_100BTX_FD_CAPABLE |
> -		       PHY_100BTX_CAPABLE |
> -		       PHY_10BT_FD_CAPABLE |
> -		       PHY_10BT_CAPABLE |
> -		       PHY_AUTO_NEG_CAPABLE;
> +		data = BMSR_100FULL |
> +		       BMSR_100HALF |
> +		       BMSR_10FULL |
> +		       BMSR_10HALF |
> +		       BMSR_ANEGCAPABLE;
>  		if (link & PORT_AUTO_NEG_COMPLETE)
> -			data |= PHY_AUTO_NEG_ACKNOWLEDGE;
> +			data |= BMSR_ANEGCOMPLETE;
>  		if (link & PORT_STAT_LINK_GOOD)
> -			data |= PHY_LINK_STATUS;
> +			data |= BMSR_LSTATUS;
>  		break;
> -	case PHY_REG_ID_1:
> +	case MII_PHYSID1:
>  		data = KSZ8795_ID_HI;
>  		break;
> -	case PHY_REG_ID_2:
> +	case MII_PHYSID2:
>  		if (ksz_is_ksz88x3(dev))
>  			data = KSZ8863_ID_LO;
>  		else
>  			data = KSZ8795_ID_LO;
>  		break;
> -	case PHY_REG_AUTO_NEGOTIATION:
> +	case MII_ADVERTISE:
>  		ksz_pread8(dev, p, regs[P_LOCAL_CTRL], &ctrl);
> -		data = PHY_AUTO_NEG_802_3;
> +		data = ADVERTISE_CSMA;
>  		if (ctrl & PORT_AUTO_NEG_SYM_PAUSE)
> -			data |= PHY_AUTO_NEG_SYM_PAUSE;
> +			data |= ADVERTISE_PAUSE_CAP;
>  		if (ctrl & PORT_AUTO_NEG_100BTX_FD)
> -			data |= PHY_AUTO_NEG_100BTX_FD;
> +			data |= ADVERTISE_100FULL;
>  		if (ctrl & PORT_AUTO_NEG_100BTX)
> -			data |= PHY_AUTO_NEG_100BTX;
> +			data |= ADVERTISE_100HALF;
>  		if (ctrl & PORT_AUTO_NEG_10BT_FD)
> -			data |= PHY_AUTO_NEG_10BT_FD;
> +			data |= ADVERTISE_10FULL;
>  		if (ctrl & PORT_AUTO_NEG_10BT)
> -			data |= PHY_AUTO_NEG_10BT;
> +			data |= ADVERTISE_10HALF;
>  		break;
> -	case PHY_REG_REMOTE_CAPABILITY:
> +	case MII_LPA:
>  		ksz_pread8(dev, p, regs[P_REMOTE_STATUS], &link);
> -		data = PHY_AUTO_NEG_802_3;
> +		data = LPA_SLCT;
>  		if (link & PORT_REMOTE_SYM_PAUSE)
> -			data |= PHY_AUTO_NEG_SYM_PAUSE;
> +			data |= LPA_PAUSE_CAP;
>  		if (link & PORT_REMOTE_100BTX_FD)
> -			data |= PHY_AUTO_NEG_100BTX_FD;
> +			data |= LPA_100FULL;
>  		if (link & PORT_REMOTE_100BTX)
> -			data |= PHY_AUTO_NEG_100BTX;
> +			data |= LPA_100HALF;
>  		if (link & PORT_REMOTE_10BT_FD)
> -			data |= PHY_AUTO_NEG_10BT_FD;
> +			data |= LPA_10FULL;
>  		if (link & PORT_REMOTE_10BT)
> -			data |= PHY_AUTO_NEG_10BT;
> -		if (data & ~PHY_AUTO_NEG_802_3)
> -			data |= PHY_REMOTE_ACKNOWLEDGE_NOT;
> +			data |= LPA_10HALF;
> +		if (data & ~LPA_SLCT)
> +			data |= LPA_LPACK;
>  		break;
>  	default:
>  		processed = false;

Re: [PATCH net-next v3 1/9] net: phy: micrel: move phy reg offsets to common header

[Andrew Lunn]

> >  	switch (reg) {
> > -	case PHY_REG_CTRL:
> > +	case MII_BMCR:
> >  		ksz_pread8(dev, p, regs[P_NEG_RESTART_CTRL], &restart);
> >  		ksz_pread8(dev, p, regs[P_SPEED_STATUS], &speed);
> >  		ksz_pread8(dev, p, regs[P_FORCE_CTRL], &ctrl);
> >  		if (restart & PORT_PHY_LOOPBACK)
> > -			data |= PHY_LOOPBACK;
> > +			data |= BMCR_LOOPBACK;
> >  		if (ctrl & PORT_FORCE_100_MBIT)
> > -			data |= PHY_SPEED_100MBIT;
> > +			data |= BMCR_SPEED100;
> >  		if (ksz_is_ksz88x3(dev)) {
> >  			if ((ctrl & PORT_AUTO_NEG_ENABLE))
> > -				data |= PHY_AUTO_NEG_ENABLE;
> > +				data |= BMCR_ANENABLE;
> >  		} else {
> >  			if (!(ctrl & PORT_AUTO_NEG_DISABLE))
> > -				data |= PHY_AUTO_NEG_ENABLE;
> > +				data |= BMCR_ANENABLE;
> >  		}
> >  		if (restart & PORT_POWER_DOWN)
> > -			data |= PHY_POWER_DOWN;
> > +			data |= BMCR_PDOWN;
> >  		if (restart & PORT_AUTO_NEG_RESTART)
> > -			data |= PHY_AUTO_NEG_RESTART;
> > +			data |= BMCR_ANRESTART;
> >  		if (ctrl & PORT_FORCE_FULL_DUPLEX)
> > -			data |= PHY_FULL_DUPLEX;
> > +			data |= BMCR_FULLDPLX;
> >  		if (speed & PORT_HP_MDIX)
> > -			data |= PHY_HP_MDIX;
> > +			data |= KSZ886X_BMCR_HP_MDIX;
> >  		if (restart & PORT_FORCE_MDIX)
> > -			data |= PHY_FORCE_MDIX;
> > +			data |= KSZ886X_BMCR_FORCE_MDI;
> >  		if (restart & PORT_AUTO_MDIX_DISABLE)
> > -			data |= PHY_AUTO_MDIX_DISABLE;
> > +			data |= KSZ886X_BMCR_DISABLE_AUTO_MDIX;
> >  		if (restart & PORT_TX_DISABLE)
> > -			data |= PHY_TRANSMIT_DISABLE;
> > +			data |= KSZ886X_BMCR_DISABLE_TRANSMIT;
> >  		if (restart & PORT_LED_OFF)
> > -			data |= PHY_LED_DISABLE;
> > +			data |= KSZ886X_BMCR_DISABLE_LED;
> >  		break;
> 
> I am deeply confused as to what this function is doing. It is reading
> the 8-bit port registers P_NEG_RESTART_CTRL, P_SPEED_STATUS and
> P_FORCE_CTRL and stitching them into a 16-bit "MII_BMCR"?

Sort of. Take a look at the datasheet for the ksz8841. It has clause
22 like registers which it exports to a PHY driver. It puts MDIX
control into the bottom of the BMCR. So this DSA driver is emulating
the ksz8841 so it can share the PHY driver.

    Andrew

[PATCH net-next v3 2/9] net: dsa: microchip: ksz8795: add phylink support

[Oleksij Rempel]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch adds the phylink support to the ksz8795 driver to provide
configuration exceptions on quirky KSZ8863 and KSZ8873 ports.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index ba065003623f..cf81ae87544d 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -18,6 +18,7 @@
 #include <linux/micrel_phy.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
+#include <linux/phylink.h>
 
 #include "ksz_common.h"
 #include "ksz8795_reg.h"
@@ -1420,11 +1421,69 @@ static int ksz8_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+static void ksz8_validate(struct dsa_switch *ds, int port,
+			  unsigned long *supported,
+			  struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+	struct ksz_device *dev = ds->priv;
+
+	if (port == dev->cpu_port) {
+		if (state->interface != PHY_INTERFACE_MODE_RMII &&
+		    state->interface != PHY_INTERFACE_MODE_MII &&
+		    state->interface != PHY_INTERFACE_MODE_NA)
+			goto unsupported;
+	} else if (port > dev->port_cnt) {
+		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+		dev_err(ds->dev, "Unsupported port: %i\n", port);
+		return;
+	} else {
+		if (state->interface != PHY_INTERFACE_MODE_INTERNAL &&
+		    state->interface != PHY_INTERFACE_MODE_NA)
+			goto unsupported;
+	}
+
+	/* Allow all the expected bits */
+	phylink_set_port_modes(mask);
+	phylink_set(mask, Autoneg);
+
+	/* Silicon Errata Sheet (DS80000830A):
+	 * "Port 1 does not respond to received flow control PAUSE frames"
+	 * So, disable Pause support on "Port 1" (port == 0) for all ksz88x3
+	 * switches.
+	 */
+	if (!ksz_is_ksz88x3(dev) || port)
+		phylink_set(mask, Pause);
+
+	/* Asym pause is not supported on KSZ8863 and KSZ8873 */
+	if (!ksz_is_ksz88x3(dev))
+		phylink_set(mask, Asym_Pause);
+
+	/* 10M and 100M are only supported */
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+
+	bitmap_and(supported, supported, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+	bitmap_and(state->advertising, state->advertising, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+	return;
+
+unsupported:
+	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+	dev_err(ds->dev, "Unsupported interface: %s, port: %d\n",
+		phy_modes(state->interface), port);
+}
+
 static const struct dsa_switch_ops ksz8_switch_ops = {
 	.get_tag_protocol	= ksz8_get_tag_protocol,
 	.setup			= ksz8_setup,
 	.phy_read		= ksz_phy_read16,
 	.phy_write		= ksz_phy_write16,
+	.phylink_validate	= ksz8_validate,
 	.phylink_mac_link_down	= ksz_mac_link_down,
 	.port_enable		= ksz_enable_port,
 	.get_strings		= ksz8_get_strings,
-- 
2.29.2

Re: [PATCH net-next v3 2/9] net: dsa: microchip: ksz8795: add phylink support

[Vladimir Oltean]

On Wed, May 26, 2021 at 06:30:30AM +0200, Oleksij Rempel wrote:
> From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> This patch adds the phylink support to the ksz8795 driver to provide
> configuration exceptions on quirky KSZ8863 and KSZ8873 ports.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index ba065003623f..cf81ae87544d 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -18,6 +18,7 @@
>  #include <linux/micrel_phy.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
> +#include <linux/phylink.h>
>  
>  #include "ksz_common.h"
>  #include "ksz8795_reg.h"
> @@ -1420,11 +1421,69 @@ static int ksz8_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static void ksz8_validate(struct dsa_switch *ds, int port,
> +			  unsigned long *supported,
> +			  struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +	struct ksz_device *dev = ds->priv;
> +
> +	if (port == dev->cpu_port) {
> +		if (state->interface != PHY_INTERFACE_MODE_RMII &&
> +		    state->interface != PHY_INTERFACE_MODE_MII &&
> +		    state->interface != PHY_INTERFACE_MODE_NA)
> +			goto unsupported;
> +	} else if (port > dev->port_cnt) {
> +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +		dev_err(ds->dev, "Unsupported port: %i\n", port);
> +		return;

Is this possible or do we just like to invent things to check?
Unless I'm missing something, ksz8_switch_init() does:

	dev->ds->num_ports = dev->port_cnt;

and dsa_port_phylink_validate() does:

	ds->ops->phylink_validate(ds, dp->index, supported, state);

where dp->index is set to @port by dsa_port_touch() in this loop:

	for (port = 0; port < ds->num_ports; port++) {
		dp = dsa_port_touch(ds, port);
		if (!dp)
			return -ENOMEM;
	}

So, if 0 <= dp->index < ds->num_ports == dev->port_cnt, what is the point?

> +	} else {
> +		if (state->interface != PHY_INTERFACE_MODE_INTERNAL &&
> +		    state->interface != PHY_INTERFACE_MODE_NA)
> +			goto unsupported;
> +	}
> +
> +	/* Allow all the expected bits */
> +	phylink_set_port_modes(mask);
> +	phylink_set(mask, Autoneg);
> +
> +	/* Silicon Errata Sheet (DS80000830A):
> +	 * "Port 1 does not respond to received flow control PAUSE frames"
> +	 * So, disable Pause support on "Port 1" (port == 0) for all ksz88x3
> +	 * switches.
> +	 */
> +	if (!ksz_is_ksz88x3(dev) || port)
> +		phylink_set(mask, Pause);
> +
> +	/* Asym pause is not supported on KSZ8863 and KSZ8873 */
> +	if (!ksz_is_ksz88x3(dev))
> +		phylink_set(mask, Asym_Pause);
> +
> +	/* 10M and 100M are only supported */
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +
> +	bitmap_and(supported, supported, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_and(state->advertising, state->advertising, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +
> +	return;
> +
> +unsupported:
> +	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	dev_err(ds->dev, "Unsupported interface: %s, port: %d\n",
> +		phy_modes(state->interface), port);
> +}
> +
>  static const struct dsa_switch_ops ksz8_switch_ops = {
>  	.get_tag_protocol	= ksz8_get_tag_protocol,
>  	.setup			= ksz8_setup,
>  	.phy_read		= ksz_phy_read16,
>  	.phy_write		= ksz_phy_write16,
> +	.phylink_validate	= ksz8_validate,
>  	.phylink_mac_link_down	= ksz_mac_link_down,
>  	.port_enable		= ksz_enable_port,
>  	.get_strings		= ksz8_get_strings,
> -- 
> 2.29.2
> 

Re: [PATCH net-next v3 2/9] net: dsa: microchip: ksz8795: add phylink support

[Oleksij Rempel]

On Thu, May 27, 2021 at 01:13:04AM +0300, Vladimir Oltean wrote:
> On Wed, May 26, 2021 at 06:30:30AM +0200, Oleksij Rempel wrote:
> > From: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > 
> > This patch adds the phylink support to the ksz8795 driver to provide
> > configuration exceptions on quirky KSZ8863 and KSZ8873 ports.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> > index ba065003623f..cf81ae87544d 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/micrel_phy.h>
> >  #include <net/dsa.h>
> >  #include <net/switchdev.h>
> > +#include <linux/phylink.h>
> >  
> >  #include "ksz_common.h"
> >  #include "ksz8795_reg.h"
> > @@ -1420,11 +1421,69 @@ static int ksz8_setup(struct dsa_switch *ds)
> >  	return 0;
> >  }
> >  
> > +static void ksz8_validate(struct dsa_switch *ds, int port,
> > +			  unsigned long *supported,
> > +			  struct phylink_link_state *state)
> > +{
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +	struct ksz_device *dev = ds->priv;
> > +
> > +	if (port == dev->cpu_port) {
> > +		if (state->interface != PHY_INTERFACE_MODE_RMII &&
> > +		    state->interface != PHY_INTERFACE_MODE_MII &&
> > +		    state->interface != PHY_INTERFACE_MODE_NA)
> > +			goto unsupported;
> > +	} else if (port > dev->port_cnt) {
> > +		bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +		dev_err(ds->dev, "Unsupported port: %i\n", port);
> > +		return;
> 
> Is this possible or do we just like to invent things to check?
> Unless I'm missing something, ksz8_switch_init() does:
> 
> 	dev->ds->num_ports = dev->port_cnt;
> 
> and dsa_port_phylink_validate() does:
> 
> 	ds->ops->phylink_validate(ds, dp->index, supported, state);
> 
> where dp->index is set to @port by dsa_port_touch() in this loop:
> 
> 	for (port = 0; port < ds->num_ports; port++) {
> 		dp = dsa_port_touch(ds, port);
> 		if (!dp)
> 			return -ENOMEM;
> 	}
> 
> So, if 0 <= dp->index < ds->num_ports == dev->port_cnt, what is the point?

good point

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 |

[PATCH net-next v3 3/9] 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 a14a00328fa3..227d88db7d27 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -38,15 +38,15 @@
 
 /* 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)
 #define	KSZPHY_INTCS_LINK_DOWN_STATUS		BIT(2)
 #define	KSZPHY_INTCS_LINK_UP_STATUS		BIT(0)
@@ -54,11 +54,11 @@
 						 KSZPHY_INTCS_LINK_UP_STATUS)
 
 /* 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.29.2

Re: [PATCH net-next v3 3/9] net: phy: micrel: use consistent indention after define

[Vladimir Oltean]

On Wed, May 26, 2021 at 06:30:31AM +0200, Oleksij Rempel wrote:
> This patch changes the indention to one space between "#define" and the

indention
/ɪnˈdɛnʃ(ə)n/
noun
noun: indention; plural noun: indentions

    archaic term for indentation.

Interesting, I learned something new.

Also, technically it's alignment not indentation.

> 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 a14a00328fa3..227d88db7d27 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -38,15 +38,15 @@
>  
>  /* 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)
>  #define	KSZPHY_INTCS_LINK_DOWN_STATUS		BIT(2)
>  #define	KSZPHY_INTCS_LINK_UP_STATUS		BIT(0)

You left these aligned using tabs.

> @@ -54,11 +54,11 @@
>  						 KSZPHY_INTCS_LINK_UP_STATUS)
>  
>  /* 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.29.2
> 

And the last column of these macros at the end is aligned with spaces
unlike everything else:

/* Write/read to/from extended registers */
#define MII_KSZPHY_EXTREG                       0x0b
#define KSZPHY_EXTREG_WRITE                     0x8000

#define MII_KSZPHY_EXTREG_WRITE                 0x0c
#define MII_KSZPHY_EXTREG_READ                  0x0d

/* Extended registers */
#define MII_KSZPHY_CLK_CONTROL_PAD_SKEW         0x104
#define MII_KSZPHY_RX_DATA_PAD_SKEW             0x105
#define MII_KSZPHY_TX_DATA_PAD_SKEW             0x106

I guess if you're going to send this patch you might as well refactor it all.

Re: [PATCH net-next v3 3/9] net: phy: micrel: use consistent indention after define

[Oleksij Rempel]

On Thu, May 27, 2021 at 01:24:48AM +0300, Vladimir Oltean wrote:
> On Wed, May 26, 2021 at 06:30:31AM +0200, Oleksij Rempel wrote:
> > This patch changes the indention to one space between "#define" and the
> 
> indention
> /ɪnˈdɛnʃ(ə)n/
> noun
> noun: indention; plural noun: indentions
> 
>     archaic term for indentation.
> 
> Interesting, I learned something new.
> 
> Also, technically it's alignment not indentation.

ok, changed :)

> > 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 a14a00328fa3..227d88db7d27 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -38,15 +38,15 @@
> >  
> >  /* 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)
> >  #define	KSZPHY_INTCS_LINK_DOWN_STATUS		BIT(2)
> >  #define	KSZPHY_INTCS_LINK_UP_STATUS		BIT(0)
> 
> You left these aligned using tabs.

done.

> > @@ -54,11 +54,11 @@
> >  						 KSZPHY_INTCS_LINK_UP_STATUS)
> >  
> >  /* 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.29.2
> > 
> 
> And the last column of these macros at the end is aligned with spaces
> unlike everything else:
> 
> /* Write/read to/from extended registers */
> #define MII_KSZPHY_EXTREG                       0x0b
> #define KSZPHY_EXTREG_WRITE                     0x8000
> 
> #define MII_KSZPHY_EXTREG_WRITE                 0x0c
> #define MII_KSZPHY_EXTREG_READ                  0x0d
> 
> /* Extended registers */
> #define MII_KSZPHY_CLK_CONTROL_PAD_SKEW         0x104
> #define MII_KSZPHY_RX_DATA_PAD_SKEW             0x105
> #define MII_KSZPHY_TX_DATA_PAD_SKEW             0x106
> 
> I guess if you're going to send this patch you might as well refactor it all.

Ok, done.

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 |

[PATCH net-next v3 4/9] 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 227d88db7d27..f03188ed953a 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1048,6 +1048,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);
@@ -1401,7 +1421,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.29.2

Re: [PATCH net-next v3 4/9] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Vladimir Oltean]

On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
> 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.

Am I right in understanding that register 195 (0xc3) is not a port register?

To hit the erratum, you have to enter Soft Power Down in the first place,
presumably by writing register 0xc3 from somewhere, right?

Where does Linux write this register from?

Once we find that place that enters/exits Soft Power Down mode, can't we
just toggle the Port Power Down bits for each port, exactly like the ERR
workaround says, instead of fooling around with a PHY 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 227d88db7d27..f03188ed953a 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1048,6 +1048,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);
> @@ -1401,7 +1421,7 @@ static struct phy_driver ksphy_driver[] = {
>  	/* PHY_BASIC_FEATURES */
>  	.config_init	= kszphy_config_init,
>  	.suspend	= genphy_suspend,
> -	.resume		= genphy_resume,
> +	.resume		= ksz886x_resume,

Are you able to explain the relation between the call paths of
phy_resume() and the lifetime of the Soft Power Down setting of the
switch? How do we know that the PHYs are resumed after the switch has
exited Soft Power Down mode?

>  }, {
>  	.name		= "Micrel KSZ87XX Switch",
>  	/* PHY_BASIC_FEATURES */
> -- 
> 2.29.2
> 

Re: [PATCH net-next v3 4/9] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Oleksij Rempel]

On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
> On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
> > 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.
> 
> Am I right in understanding that register 195 (0xc3) is not a port register?
> 
> To hit the erratum, you have to enter Soft Power Down in the first place,
> presumably by writing register 0xc3 from somewhere, right?
> 
> Where does Linux write this register from?
> 
> Once we find that place that enters/exits Soft Power Down mode, can't we
> just toggle the Port Power Down bits for each port, exactly like the ERR
> workaround says, instead of fooling around with a PHY driver?

The KSZ8873 switch is using multiple register mappings.
https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
Page 38:
"The MIIM interface is used to access the MII PHY registers defined in
this section. The SPI, I2C, and SMI interfaces can also be used to access
some of these registers. The latter three interfaces use a different
mapping mechanism than the MIIM interface."

This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
integrated register translation mapping.

> > 
> > 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 227d88db7d27..f03188ed953a 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -1048,6 +1048,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);
> > @@ -1401,7 +1421,7 @@ static struct phy_driver ksphy_driver[] = {
> >  	/* PHY_BASIC_FEATURES */
> >  	.config_init	= kszphy_config_init,
> >  	.suspend	= genphy_suspend,
> > -	.resume		= genphy_resume,
> > +	.resume		= ksz886x_resume,
> 
> Are you able to explain the relation between the call paths of
> phy_resume() and the lifetime of the Soft Power Down setting of the
> switch? How do we know that the PHYs are resumed after the switch has
> exited Soft Power Down mode?

The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
[3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.

I assume, I'll need to add this comments to the commit message. Or do
you have other suggestions on how this should be implemented?

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 v3 4/9] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Vladimir Oltean]

On Thu, Jun 10, 2021 at 01:49:20PM +0200, Oleksij Rempel wrote:
> On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
> > On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
> > > 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.
> > 
> > Am I right in understanding that register 195 (0xc3) is not a port register?
> > 
> > To hit the erratum, you have to enter Soft Power Down in the first place,
> > presumably by writing register 0xc3 from somewhere, right?
> > 
> > Where does Linux write this register from?
> > 
> > Once we find that place that enters/exits Soft Power Down mode, can't we
> > just toggle the Port Power Down bits for each port, exactly like the ERR
> > workaround says, instead of fooling around with a PHY driver?
> 
> The KSZ8873 switch is using multiple register mappings.
> https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
> Page 38:
> "The MIIM interface is used to access the MII PHY registers defined in
> this section. The SPI, I2C, and SMI interfaces can also be used to access
> some of these registers. The latter three interfaces use a different
> mapping mechanism than the MIIM interface."
> 
> This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
> integrated register translation mapping.

This doesn't answer my question of where is the Soft Power Down mode enabled.

> > > 
> > > 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 227d88db7d27..f03188ed953a 100644
> > > --- a/drivers/net/phy/micrel.c
> > > +++ b/drivers/net/phy/micrel.c
> > > @@ -1048,6 +1048,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);
> > > @@ -1401,7 +1421,7 @@ static struct phy_driver ksphy_driver[] = {
> > >  	/* PHY_BASIC_FEATURES */
> > >  	.config_init	= kszphy_config_init,
> > >  	.suspend	= genphy_suspend,
> > > -	.resume		= genphy_resume,
> > > +	.resume		= ksz886x_resume,
> > 
> > Are you able to explain the relation between the call paths of
> > phy_resume() and the lifetime of the Soft Power Down setting of the
> > switch? How do we know that the PHYs are resumed after the switch has
> > exited Soft Power Down mode?
> 
> The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
> [3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.
> 
> I assume, I'll need to add this comments to the commit message. Or do
> you have other suggestions on how this should be implemented?

According to "3.2 Power Management" in the datasheet you shared:

There are 5 (five) operation modes under the power management function,
which is controlled by two bits in Register 195 (0xC3) and one bit in
Register 29 (0x1D), 45 (0x2D) as shown below:

Register 195 bit[1:0] = 00 Normal Operation Mode
Register 195 bit[1:0] = 01 Energy Detect Mode
Register 195 bit[1:0] = 10 Soft Power Down Mode
Register 195 bit[1:0] = 11 Power Saving Mode
Register 29, 45 bit 3 = 1 Port Based Power Down Mode

3.2.4 SOFT POWER DOWN MODE

The soft power down mode is entered by setting bit[1:0]=10 in register
195. When KSZ8873MLL/FLL/RLL is in this mode, all PLL clocks are
disabled, the PHY and the MAC are off, all internal registers values
will not change. When the host set bit[1:0]=00 in register 195, this
device will be back from current soft power down mode to normal
operation mode.

3.2.5 PORT-BASED POWER DOWN MODE

In addition, the KSZ8873MLL/FLL/RLL features a per-port power down mode.
To save power, a PHY port that is not in use can be powered down via
port control register 29 or 45 bit 3, or MIIM PHY register. It saves
about 15 mA per port.



From the above I understand that the first 4 power management modes are
global, and the 5th isn't.

You've explained how the PHY driver enters port-based power down mode.
But the ERR describes an issue being triggered by a global power down
mode. What you are describing is not what the ERR text is describing.

Excuse my perhaps stupid question, but have you triggered the issue
described by the erratum? Does this patch fix that? Where is the disconnect?

Re: [PATCH net-next v3 4/9] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Oleksij Rempel]

On Thu, Jun 10, 2021 at 04:04:45PM +0300, Vladimir Oltean wrote:
> On Thu, Jun 10, 2021 at 01:49:20PM +0200, Oleksij Rempel wrote:
> > On Thu, May 27, 2021 at 01:43:29AM +0300, Vladimir Oltean wrote:
> > > On Wed, May 26, 2021 at 06:30:32AM +0200, Oleksij Rempel wrote:
> > > > 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.
> > > 
> > > Am I right in understanding that register 195 (0xc3) is not a port register?
> > > 
> > > To hit the erratum, you have to enter Soft Power Down in the first place,
> > > presumably by writing register 0xc3 from somewhere, right?
> > > 
> > > Where does Linux write this register from?
> > > 
> > > Once we find that place that enters/exits Soft Power Down mode, can't we
> > > just toggle the Port Power Down bits for each port, exactly like the ERR
> > > workaround says, instead of fooling around with a PHY driver?
> > 
> > The KSZ8873 switch is using multiple register mappings.
> > https://ww1.microchip.com/downloads/en/DeviceDoc/00002348A.pdf
> > Page 38:
> > "The MIIM interface is used to access the MII PHY registers defined in
> > this section. The SPI, I2C, and SMI interfaces can also be used to access
> > some of these registers. The latter three interfaces use a different
> > mapping mechanism than the MIIM interface."
> > 
> > This PHY driver is able to work directly over MIIM (MDIO). Or work with DSA over
> > integrated register translation mapping.
> 
> This doesn't answer my question of where is the Soft Power Down mode enabled.
> 
> > > > 
> > > > 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 227d88db7d27..f03188ed953a 100644
> > > > --- a/drivers/net/phy/micrel.c
> > > > +++ b/drivers/net/phy/micrel.c
> > > > @@ -1048,6 +1048,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);
> > > > @@ -1401,7 +1421,7 @@ static struct phy_driver ksphy_driver[] = {
> > > >  	/* PHY_BASIC_FEATURES */
> > > >  	.config_init	= kszphy_config_init,
> > > >  	.suspend	= genphy_suspend,
> > > > -	.resume		= genphy_resume,
> > > > +	.resume		= ksz886x_resume,
> > > 
> > > Are you able to explain the relation between the call paths of
> > > phy_resume() and the lifetime of the Soft Power Down setting of the
> > > switch? How do we know that the PHYs are resumed after the switch has
> > > exited Soft Power Down mode?
> > 
> > The MII_BMCRs BMCR_PDOWN bit is mapped to the "register 29 (0x1D), bit
> > [3]" for the PHY0 and to "register 45 (0x2D), bit [3]" for the PHY1.
> > 
> > I assume, I'll need to add this comments to the commit message. Or do
> > you have other suggestions on how this should be implemented?
> 
> According to "3.2 Power Management" in the datasheet you shared:
> 
> There are 5 (five) operation modes under the power management function,
> which is controlled by two bits in Register 195 (0xC3) and one bit in
> Register 29 (0x1D), 45 (0x2D) as shown below:
> 
> Register 195 bit[1:0] = 00 Normal Operation Mode
> Register 195 bit[1:0] = 01 Energy Detect Mode
> Register 195 bit[1:0] = 10 Soft Power Down Mode
> Register 195 bit[1:0] = 11 Power Saving Mode
> Register 29, 45 bit 3 = 1 Port Based Power Down Mode
> 
> 3.2.4 SOFT POWER DOWN MODE
> 
> The soft power down mode is entered by setting bit[1:0]=10 in register
> 195. When KSZ8873MLL/FLL/RLL is in this mode, all PLL clocks are
> disabled, the PHY and the MAC are off, all internal registers values
> will not change. When the host set bit[1:0]=00 in register 195, this
> device will be back from current soft power down mode to normal
> operation mode.
> 
> 3.2.5 PORT-BASED POWER DOWN MODE
> 
> In addition, the KSZ8873MLL/FLL/RLL features a per-port power down mode.
> To save power, a PHY port that is not in use can be powered down via
> port control register 29 or 45 bit 3, or MIIM PHY register. It saves
> about 15 mA per port.
> 
> 
> 
> From the above I understand that the first 4 power management modes are
> global, and the 5th isn't.
> 
> You've explained how the PHY driver enters port-based power down mode.
> But the ERR describes an issue being triggered by a global power down
> mode. What you are describing is not what the ERR text is describing.
> 
> Excuse my perhaps stupid question, but have you triggered the issue
> described by the erratum? Does this patch fix that? Where is the disconnect?

Yes, this issue was seen  at some early point of development (back in 2019)
reproducible on system start. Where switch was in some default state or
on a state configured by the bootloader. I didn't tried to reproduce it
now. With other words, there is no need to provide global power
management by the DSA driver to trigger 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 v3 4/9] net: phy: micrel: apply resume errata workaround for ksz8873 and ksz8863

[Vladimir Oltean]

On Thu, Jun 10, 2021 at 03:25:05PM +0200, Oleksij Rempel wrote:
> Yes, this issue was seen  at some early point of development (back in 2019)
> reproducible on system start. Where switch was in some default state or
> on a state configured by the bootloader. I didn't tried to reproduce it
> now. With other words, there is no need to provide global power
> management by the DSA driver to trigger it.

If you're sure about that then add it to the commit message or comments,
since this is not what the ERR description says.

[PATCH net-next v3 5/9] net: phy/dsa 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/dsa/microchip/ksz8795.c |  5 ++
 drivers/net/phy/micrel.c            | 88 +++++++++++++++++++++++++++++
 include/linux/micrel_phy.h          |  2 +
 3 files changed, 95 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index cf81ae87544d..55da8ec175da 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -816,6 +816,11 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 		if (data & ~LPA_SLCT)
 			data |= LPA_LPACK;
 		break;
+	case PHY_REG_PHY_CTRL:
+		ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
+		if (link & PORT_MDIX_STATUS)
+			data |= KSZ886X_CTRL_MDIX_STAT;
+		break;
 	default:
 		processed = false;
 		break;
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index f03188ed953a..ac56e8159712 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1048,6 +1048,92 @@ 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;
+
+	/* Same reverse logic as KSZ886X_BMCR_FORCE_MDI */
+	if (ret & KSZ886X_CTRL_MDIX_STAT)
+		phydev->mdix = ETH_TP_MDI_X;
+	else
+		phydev->mdix = ETH_TP_MDI;
+
+	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;
@@ -1420,6 +1506,8 @@ static struct phy_driver ksphy_driver[] = {
 	.name		= "Micrel KSZ8851 Ethernet MAC or 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,
 }, {
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index b03e2afcb53f..58370abd9f4f 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -58,4 +58,6 @@
 #define KSZ886X_BMCR_DISABLE_TRANSMIT		BIT(1)
 #define KSZ886X_BMCR_DISABLE_LED		BIT(0)
 
+#define KSZ886X_CTRL_MDIX_STAT			BIT(4)
+
 #endif /* _MICREL_PHY_H */
-- 
2.29.2

[PATCH net-next v3 6/9] 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 ac56e8159712..b6ce7bd66738 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -55,11 +55,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)
 
@@ -422,6 +428,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;
@@ -1410,6 +1497,8 @@ static struct phy_driver ksphy_driver[] = {
 	.probe		= kszphy_probe,
 	.config_init	= ksz8081_config_init,
 	.soft_reset	= genphy_soft_reset,
+	.config_aneg	= ksz8081_config_aneg,
+	.read_status	= ksz8081_read_status,
 	.config_intr	= kszphy_config_intr,
 	.handle_interrupt = kszphy_handle_interrupt,
 	.get_sset_count = kszphy_get_sset_count,
-- 
2.29.2

[PATCH net-next v3 7/9] net: dsa: microchip: ksz8795: add LINK_MD register support

[Oleksij Rempel]

From: Oleksij Rempel <linux@rempel-privat.de>

Add mapping for LINK_MD register to enable cable testing functionality.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 22 ++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz8795_reg.h |  5 +++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 55da8ec175da..ae5fe9c829da 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -6,6 +6,7 @@
  *	Tristram Ha <Tristram.Ha@microchip.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/gpio.h>
@@ -728,6 +729,7 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 	struct ksz8 *ksz8 = dev->priv;
 	u8 restart, speed, ctrl, link;
 	const u8 *regs = ksz8->regs;
+	u8 val1, val2;
 	int processed = true;
 	u16 data = 0;
 	u8 p = phy;
@@ -816,6 +818,22 @@ static void ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
 		if (data & ~LPA_SLCT)
 			data |= LPA_LPACK;
 		break;
+	case PHY_REG_LINK_MD:
+		ksz_pread8(dev, p, REG_PORT_LINK_MD_CTRL, &val1);
+		ksz_pread8(dev, p, REG_PORT_LINK_MD_RESULT, &val2);
+		if (val1 & PORT_START_CABLE_DIAG)
+			data |= PHY_START_CABLE_DIAG;
+
+		if (val1 & PORT_CABLE_10M_SHORT)
+			data |= PHY_CABLE_10M_SHORT;
+
+		data |= FIELD_PREP(PHY_CABLE_DIAG_RESULT_M,
+				FIELD_GET(PORT_CABLE_DIAG_RESULT_M, val1));
+
+		data |= FIELD_PREP(PHY_CABLE_FAULT_COUNTER_M,
+				(FIELD_GET(PORT_CABLE_FAULT_COUNTER_H, val1) << 8) |
+				FIELD_GET(PORT_CABLE_FAULT_COUNTER_L, val2));
+		break;
 	case PHY_REG_PHY_CTRL:
 		ksz_pread8(dev, p, regs[P_LINK_STATUS], &link);
 		if (link & PORT_MDIX_STATUS)
@@ -932,6 +950,10 @@ static void ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
 		if (data != ctrl)
 			ksz_pwrite8(dev, p, regs[P_LOCAL_CTRL], data);
 		break;
+	case PHY_REG_LINK_MD:
+		if (val & PHY_START_CABLE_DIAG)
+			ksz_port_cfg(dev, p, REG_PORT_LINK_MD_CTRL, PORT_START_CABLE_DIAG, true);
+		break;
 	default:
 		break;
 	}
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index f925ddee5238..a32355624f31 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -249,7 +249,7 @@
 #define REG_PORT_4_LINK_MD_CTRL		0x4A
 
 #define PORT_CABLE_10M_SHORT		BIT(7)
-#define PORT_CABLE_DIAG_RESULT_M	0x3
+#define PORT_CABLE_DIAG_RESULT_M	GENMASK(6, 5)
 #define PORT_CABLE_DIAG_RESULT_S	5
 #define PORT_CABLE_STAT_NORMAL		0
 #define PORT_CABLE_STAT_OPEN		1
@@ -753,13 +753,14 @@
 #define PHY_REG_LINK_MD			0x1D
 
 #define PHY_START_CABLE_DIAG		BIT(15)
+#define PHY_CABLE_DIAG_RESULT_M		GENMASK(14, 13)
 #define PHY_CABLE_DIAG_RESULT		0x6000
 #define PHY_CABLE_STAT_NORMAL		0x0000
 #define PHY_CABLE_STAT_OPEN		0x2000
 #define PHY_CABLE_STAT_SHORT		0x4000
 #define PHY_CABLE_STAT_FAILED		0x6000
 #define PHY_CABLE_10M_SHORT		BIT(12)
-#define PHY_CABLE_FAULT_COUNTER		0x01FF
+#define PHY_CABLE_FAULT_COUNTER_M	GENMASK(8, 0)
 
 #define PHY_REG_PHY_CTRL		0x1F
 
-- 
2.29.2

[PATCH net-next v3 8/9] net: dsa: dsa_slave_phy_connect(): extend phy's flags with port specific phy flags

[Oleksij Rempel]

This patch extends the flags of the phy that's being connected with the
port specific flags of the switch port.

This is needed to handle a port specific erratum of the KSZ8873 switch,
which is added in a later patch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phylink.c | 2 +-
 net/dsa/slave.c           | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 96d8e88b4e46..167c2277814f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1029,7 +1029,7 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	return phy_attach_direct(pl->netdev, phy, phy->dev_flags, interface);
 }
 
 /**
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8c0f3c6ab365..7e208f16f006 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1758,6 +1758,10 @@ static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
 		return -ENODEV;
 	}
 
+	if (ds->ops->get_phy_flags)
+		slave_dev->phydev->dev_flags |=
+			ds->ops->get_phy_flags(ds, dp->index);
+
 	return phylink_connect_phy(dp->pl, slave_dev->phydev);
 }
 
-- 
2.29.2

Re: [PATCH net-next v3 8/9] net: dsa: dsa_slave_phy_connect(): extend phy's flags with port specific phy flags

[Russell King (Oracle)]

On Wed, May 26, 2021 at 06:30:36AM +0200, Oleksij Rempel wrote:
> This patch extends the flags of the phy that's being connected with the
> port specific flags of the switch port.
> 
> This is needed to handle a port specific erratum of the KSZ8873 switch,
> which is added in a later patch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/phylink.c | 2 +-
>  net/dsa/slave.c           | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 96d8e88b4e46..167c2277814f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1029,7 +1029,7 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
>  	if (pl->phydev)
>  		return -EBUSY;
>  
> -	return phy_attach_direct(pl->netdev, phy, 0, interface);
> +	return phy_attach_direct(pl->netdev, phy, phy->dev_flags, interface);

I don't think this has any benefit. phy_attach_direct() does this
internally:

        phydev->dev_flags |= flags;

which means the above change is effectively doing:

        phydev->dev_flags |= phydev->dev_flags;

So, are you sure you need this?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 8/9] net: dsa: dsa_slave_phy_connect(): extend phy's flags with port specific phy flags

[Oleksij Rempel]

On Wed, May 26, 2021 at 04:08:11PM +0100, Russell King (Oracle) wrote:
> On Wed, May 26, 2021 at 06:30:36AM +0200, Oleksij Rempel wrote:
> > This patch extends the flags of the phy that's being connected with the
> > port specific flags of the switch port.
> > 
> > This is needed to handle a port specific erratum of the KSZ8873 switch,
> > which is added in a later patch.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/phy/phylink.c | 2 +-
> >  net/dsa/slave.c           | 4 ++++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 96d8e88b4e46..167c2277814f 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -1029,7 +1029,7 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
> >  	if (pl->phydev)
> >  		return -EBUSY;
> >  
> > -	return phy_attach_direct(pl->netdev, phy, 0, interface);
> > +	return phy_attach_direct(pl->netdev, phy, phy->dev_flags, interface);
> 
> I don't think this has any benefit. phy_attach_direct() does this
> internally:
> 
>         phydev->dev_flags |= flags;
> 
> which means the above change is effectively doing:
> 
>         phydev->dev_flags |= phydev->dev_flags;
> 
> So, are you sure you need this?

Ah, good point. Back for two years, phy_attach_direct() was doing
	phydev->dev_flags = flags;

I didn't noticed this change.

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 |

[PATCH net-next v3 9/9] 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:
  - provides invalid values, thus return -ENOTSUPP
    (Errata: DS80000830A: "LinkMD does not work on Port 1",
     http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ8873-Errata-DS80000830A.pdf)

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

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

---

- added PHY_POLL_CABLE_TEST to make it work in interrupt mode
---
 drivers/net/dsa/microchip/ksz8795.c |  13 ++
 drivers/net/phy/micrel.c            | 180 ++++++++++++++++++++++++++++
 include/linux/micrel_phy.h          |   1 +
 3 files changed, 194 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index ae5fe9c829da..1881adb19c85 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -970,6 +970,18 @@ static enum dsa_tag_protocol ksz8_get_tag_protocol(struct dsa_switch *ds,
 		DSA_TAG_PROTO_KSZ9893 : DSA_TAG_PROTO_KSZ8795;
 }
 
+static u32 ksz8_sw_get_phy_flags(struct dsa_switch *ds, int port)
+{
+	/* Silicon Errata Sheet (DS80000830A):
+	 * Port 1 does not work with LinkMD Cable-Testing.
+	 * Port 1 does not respond to received PAUSE control frames.
+	 */
+	if (!port)
+		return MICREL_KSZ8_P1_ERRATA;
+
+	return 0;
+}
+
 static void ksz8_get_strings(struct dsa_switch *ds, int port,
 			     u32 stringset, uint8_t *buf)
 {
@@ -1507,6 +1519,7 @@ static void ksz8_validate(struct dsa_switch *ds, int port,
 
 static const struct dsa_switch_ops ksz8_switch_ops = {
 	.get_tag_protocol	= ksz8_get_tag_protocol,
+	.get_phy_flags		= ksz8_sw_get_phy_flags,
 	.setup			= ksz8_setup,
 	.phy_read		= ksz_phy_read16,
 	.phy_write		= ksz_phy_write16,
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index b6ce7bd66738..6b744e68ce97 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>
@@ -53,6 +54,18 @@
 #define	KSZPHY_INTCS_STATUS			(KSZPHY_INTCS_LINK_DOWN_STATUS |\
 						 KSZPHY_INTCS_LINK_UP_STATUS)
 
+/* 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)
@@ -1386,6 +1399,167 @@ static int kszphy_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz886x_cable_test_start(struct phy_device *phydev)
+{
+	if (phydev->dev_flags & MICREL_KSZ8_P1_ERRATA)
+		return -ENOTSUPP;
+
+	/* 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,
@@ -1492,6 +1666,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id		= PHY_ID_KSZ8081,
 	.name		= "Micrel KSZ8081 or KSZ8091",
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
+	.flags		= PHY_POLL_CABLE_TEST,
 	/* PHY_BASIC_FEATURES */
 	.driver_data	= &ksz8081_type,
 	.probe		= kszphy_probe,
@@ -1506,6 +1681,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",
@@ -1594,11 +1771,14 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8851 Ethernet MAC or KSZ886X Switch",
 	/* PHY_BASIC_FEATURES */
+	.flags		= PHY_POLL_CABLE_TEST,
 	.config_init	= kszphy_config_init,
 	.config_aneg	= ksz886x_config_aneg,
 	.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 */
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 58370abd9f4f..3d43c60b49fa 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -39,6 +39,7 @@
 /* struct phy_device dev_flags definitions */
 #define MICREL_PHY_50MHZ_CLK	0x00000001
 #define MICREL_PHY_FXEN		0x00000002
+#define MICREL_KSZ8_P1_ERRATA	0x00000003
 
 #define MICREL_KSZ9021_EXTREG_CTRL	0xB
 #define MICREL_KSZ9021_EXTREG_DATA_WRITE	0xC
-- 
2.29.2

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

[Jakub Kicinski]

On Wed, 26 May 2021 06:30:37 +0200 Oleksij Rempel wrote:
> +	if (phydev->dev_flags & MICREL_KSZ8_P1_ERRATA)
> +		return -ENOTSUPP;

EOPNOTSUPP