[PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Russell King (Oracle)]

Hi,

I've found yet more potential for cleanups in the XPCS driver.

The first patch switches to using generic register definitions.

Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
which can be simplified down to a simple if() statement.

Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
from the functional bit.

Next, realising that the functional bit is just the helper function we
already have and are using in the SGMII version of this function,
switch over to that.

We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
are basically functionally identical except for the warnings, so merge
the two functions.

Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
established pattern.

Lastly, "return foo();" where foo is a void function and the function
being returned from is also void is a weird programming pattern.
Replace this with something more conventional.

With these changes, we see yet another reduction in the amount of
code in this driver.

 drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
 drivers/net/pcs/pcs-xpcs.h |  12 ----
 2 files changed, 65 insertions(+), 81 deletions(-)

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

[PATCH net-next 1/7] net: pcs: xpcs: use generic register definitions

[Russell King (Oracle)]

As a general policy, we refer our generic register definitions over
vendor specific definitions. In XPCS, it appears that the register
layout follows a BMCR, BMSR and ADVERTISE register definition. We
already refer to this BMCR register using several different macros
which is confusing.

Convert the following register definitions to generic versions:

DW_VR_MII_MMD_CTRL => MII_BMCR
MDIO_CTRL1 => MII_BMCR
AN_CL37_EN => BMCR_ANENABLE
SGMII_SPEED_SS6 => BMCR_SPEED1000
SGMII_SPEED_SS13 => BMCR_SPEED100
MDIO_CTRL1_RESET => BMCR_RESET

DW_VR_MII_MMD_STS => MII_BMSR
DW_VR_MII_MMD_STS_LINK_STS => BMSR_LSTATUS

DW_FULL_DUPLEX => ADVERTISE_1000XFULL
iDW_HALF_DUPLEX => ADVERTISE_1000XHALF

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 64 ++++++++++++++++++++------------------
 drivers/net/pcs/pcs-xpcs.h | 12 -------
 2 files changed, 34 insertions(+), 42 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index c69421e80d19..a5e2d93db285 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -223,8 +223,8 @@ static int xpcs_poll_reset(struct dw_xpcs *xpcs, int dev)
 	int ret, val;
 
 	ret = read_poll_timeout(xpcs_read, val,
-				val < 0 || !(val & MDIO_CTRL1_RESET),
-				50000, 600000, true, xpcs, dev, MDIO_CTRL1);
+				val < 0 || !(val & BMCR_RESET),
+				50000, 600000, true, xpcs, dev, MII_BMCR);
 	if (val < 0)
 		ret = val;
 
@@ -250,7 +250,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
 		return -EINVAL;
 	}
 
-	ret = xpcs_write(xpcs, dev, MDIO_CTRL1, MDIO_CTRL1_RESET);
+	ret = xpcs_write(xpcs, dev, MII_BMCR, BMCR_RESET);
 	if (ret < 0)
 		return ret;
 
@@ -343,7 +343,7 @@ static void xpcs_config_usxgmii(struct dw_xpcs *xpcs, int speed)
 	if (ret < 0)
 		goto out;
 
-	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, DW_USXGMII_SS_MASK,
+	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, DW_USXGMII_SS_MASK,
 			  speed_sel | DW_USXGMII_FULL);
 	if (ret < 0)
 		goto out;
@@ -646,19 +646,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	 *    speed/duplex mode change by HW after SGMII AN complete)
 	 * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
 	 *
+	 * Note that VR_MII_MMD_CTRL is MII_BMCR.
+	 *
 	 * Note: Since it is MAC side SGMII, there is no need to set
 	 *	 SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
 	 *	 PHY about the link state change after C28 AN is completed
 	 *	 between PHY and Link Partner. There is also no need to
 	 *	 trigger AN restart for MAC-side SGMII.
 	 */
-	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
+	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
 	if (mdio_ctrl < 0)
 		return mdio_ctrl;
 
-	if (mdio_ctrl & AN_CL37_EN) {
-		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
-				 mdio_ctrl & ~AN_CL37_EN);
+	if (mdio_ctrl & BMCR_ANENABLE) {
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+				 mdio_ctrl & ~BMCR_ANENABLE);
 		if (ret < 0)
 			return ret;
 	}
@@ -696,8 +698,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 		return ret;
 
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
-		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
-				 mdio_ctrl | AN_CL37_EN);
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+				 mdio_ctrl | BMCR_ANENABLE);
 
 	return ret;
 }
@@ -715,14 +717,16 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
 	 * be disabled first:-
 	 * 1) VR_MII_MMD_CTRL Bit(12)[AN_ENABLE] = 0b
 	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 00b (1000BASE-X C37)
+	 *
+	 * Note that VR_MII_MMD_CTRL is MII_BMCR.
 	 */
-	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
+	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
 	if (mdio_ctrl < 0)
 		return mdio_ctrl;
 
-	if (mdio_ctrl & AN_CL37_EN) {
-		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
-				 mdio_ctrl & ~AN_CL37_EN);
+	if (mdio_ctrl & BMCR_ANENABLE) {
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+				 mdio_ctrl & ~BMCR_ANENABLE);
 		if (ret < 0)
 			return ret;
 	}
@@ -760,8 +764,8 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
 		return ret;
 
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
-		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
-				 mdio_ctrl | AN_CL37_EN);
+		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+				 mdio_ctrl | BMCR_ANENABLE);
 		if (ret < 0)
 			return ret;
 	}
@@ -780,9 +784,9 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 	if (ret < 0)
 		return ret;
 
-	return xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
-			   AN_CL37_EN | SGMII_SPEED_SS6 | SGMII_SPEED_SS13,
-			   SGMII_SPEED_SS6);
+	return xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+			   BMCR_ANENABLE | BMCR_SPEED1000 | BMCR_SPEED100,
+			   BMCR_SPEED1000);
 }
 
 static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
@@ -972,14 +976,14 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 
 		state->link = true;
 
-		speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
+		speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
 		if (speed < 0)
 			return speed;
 
-		speed &= SGMII_SPEED_SS13 | SGMII_SPEED_SS6;
-		if (speed == SGMII_SPEED_SS6)
+		speed &= BMCR_SPEED100 | BMCR_SPEED1000;
+		if (speed == BMCR_SPEED1000)
 			state->speed = SPEED_1000;
-		else if (speed == SGMII_SPEED_SS13)
+		else if (speed == BMCR_SPEED100)
 			state->speed = SPEED_100;
 		else if (speed == 0)
 			state->speed = SPEED_10;
@@ -988,9 +992,9 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
 		if (duplex < 0)
 			return duplex;
 
-		if (duplex & DW_FULL_DUPLEX)
+		if (duplex & ADVERTISE_1000XFULL)
 			state->duplex = DUPLEX_FULL;
-		else if (duplex & DW_HALF_DUPLEX)
+		else if (duplex & ADVERTISE_1000XHALF)
 			state->duplex = DUPLEX_HALF;
 
 		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
@@ -1039,13 +1043,13 @@ static int xpcs_get_state_2500basex(struct dw_xpcs *xpcs,
 {
 	int ret;
 
-	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_STS);
+	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMSR);
 	if (ret < 0) {
 		state->link = 0;
 		return ret;
 	}
 
-	state->link = !!(ret & DW_VR_MII_MMD_STS_LINK_STS);
+	state->link = !!(ret & BMSR_LSTATUS);
 	if (!state->link)
 		return 0;
 
@@ -1109,7 +1113,7 @@ static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int neg_mode,
 		return;
 
 	val = mii_bmcr_encode_fixed(speed, duplex);
-	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
 	if (ret)
 		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
 			__func__, ERR_PTR(ret));
@@ -1141,7 +1145,7 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 		dev_err(&xpcs->mdiodev->dev, "%s: half duplex not supported\n",
 			__func__);
 
-	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
 	if (ret)
 		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
 			__func__, ERR_PTR(ret));
@@ -1164,7 +1168,7 @@ static void xpcs_an_restart(struct phylink_pcs *pcs)
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
-	xpcs_modify(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, BMCR_ANRESTART,
+	xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
 		    BMCR_ANRESTART);
 }
 
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 9a22eed4404d..adc5a0b3c883 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -54,9 +54,6 @@
 
 /* Clause 37 Defines */
 /* VR MII MMD registers offsets */
-#define DW_VR_MII_MMD_CTRL		0x0000
-#define DW_VR_MII_MMD_STS		0x0001
-#define DW_VR_MII_MMD_STS_LINK_STS	BIT(2)
 #define DW_VR_MII_DIG_CTRL1		0x8000
 #define DW_VR_MII_AN_CTRL		0x8001
 #define DW_VR_MII_AN_INTR_STS		0x8002
@@ -93,15 +90,6 @@
 #define DW_VR_MII_C37_ANSGM_SP_1000		0x2
 #define DW_VR_MII_C37_ANSGM_SP_LNKSTS		BIT(4)
 
-/* SR MII MMD Control defines */
-#define AN_CL37_EN			BIT(12)	/* Enable Clause 37 auto-nego */
-#define SGMII_SPEED_SS13		BIT(13)	/* SGMII speed along with SS6 */
-#define SGMII_SPEED_SS6			BIT(6)	/* SGMII speed along with SS13 */
-
-/* SR MII MMD AN Advertisement defines */
-#define DW_HALF_DUPLEX			BIT(6)
-#define DW_FULL_DUPLEX			BIT(5)
-
 /* VR MII EEE Control 0 defines */
 #define DW_VR_MII_EEE_LTX_EN			BIT(0)  /* LPI Tx Enable */
 #define DW_VR_MII_EEE_LRX_EN			BIT(1)  /* LPI Rx Enable */
-- 
2.30.2

[PATCH net-next 2/7] net: pcs: xpcs: remove switch() in xpcs_link_up_1000basex()

[Russell King (Oracle)]

Remove an unnecessary switch() statement in xpcs_link_up_1000basex().
The only value this switch statement is interested in is SPEED_1000,
all other values lead to an error. Replace this with a simple if()
statement.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index a5e2d93db285..183df8f8c50f 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1127,18 +1127,13 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		return;
 
-	switch (speed) {
-	case SPEED_1000:
-		val = BMCR_SPEED1000;
-		break;
-	case SPEED_100:
-	case SPEED_10:
-	default:
-		dev_err(&xpcs->mdiodev->dev, "%s: speed = %d\n",
+	if (speed != SPEED_1000) {
+		dev_err(&xpcs->mdiodev->dev, "%s: speed %dMbps not supported\n",
 			__func__, speed);
 		return;
 	}
 
+	val = BMCR_SPEED1000;
 	if (duplex == DUPLEX_FULL)
 		val |= BMCR_FULLDPLX;
 	else
-- 
2.30.2

[PATCH net-next 3/7] net: pcs: xpcs: rearrange xpcs_link_up_1000basex()

[Russell King (Oracle)]

Rearrange xpcs_link_up_1000basex() to make it more obvious what will
happen in the following commit.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 183df8f8c50f..3222b8851bff 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1133,12 +1133,13 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 		return;
 	}
 
+	if (duplex != DUPLEX_FULL)
+		dev_err(&xpcs->mdiodev->dev, "%s: half duplex not supported\n",
+			__func__);
+
 	val = BMCR_SPEED1000;
 	if (duplex == DUPLEX_FULL)
 		val |= BMCR_FULLDPLX;
-	else
-		dev_err(&xpcs->mdiodev->dev, "%s: half duplex not supported\n",
-			__func__);
 
 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
 	if (ret)
-- 
2.30.2

[PATCH net-next 4/7] net: pcs: xpcs: replace open-coded mii_bmcr_encode_fixed()

[Russell King (Oracle)]

We can now see that we have an open-coded version of
mii_bmcr_encode_fixed() when this is called with SPEED_1000:

        val = BMCR_SPEED1000;
        if (duplex == DUPLEX_FULL)
                val |= BMCR_FULLDPLX;

Replace this with a call to mii_bmcr_encode_fixed().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 3222b8851bff..5b38f9019f83 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1137,10 +1137,7 @@ static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
 		dev_err(&xpcs->mdiodev->dev, "%s: half duplex not supported\n",
 			__func__);
 
-	val = BMCR_SPEED1000;
-	if (duplex == DUPLEX_FULL)
-		val |= BMCR_FULLDPLX;
-
+	val = mii_bmcr_encode_fixed(speed, duplex);
 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
 	if (ret)
 		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
-- 
2.30.2

[PATCH net-next 5/7] net: pcs: xpcs: combine xpcs_link_up_{1000basex,sgmii}()

[Russell King (Oracle)]

xpcs_link_up_sgmii() and xpcs_link_up_1000basex() are almost identical
with the exception of checking the speed and duplex for 1000BASE-X.
Combine the two functions.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 54 ++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 5b38f9019f83..6cc658f8366c 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1104,41 +1104,32 @@ static void xpcs_get_state(struct phylink_pcs *pcs,
 	}
 }
 
-static void xpcs_link_up_sgmii(struct dw_xpcs *xpcs, unsigned int neg_mode,
-			       int speed, int duplex)
+static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
+					 unsigned int neg_mode,
+					 phy_interface_t interface,
+					 int speed, int duplex)
 {
-	int val, ret;
+	int ret;
 
 	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 		return;
 
-	val = mii_bmcr_encode_fixed(speed, duplex);
-	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
-	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
-			__func__, ERR_PTR(ret));
-}
-
-static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, unsigned int neg_mode,
-				   int speed, int duplex)
-{
-	int val, ret;
-
-	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
-		return;
+	if (interface == PHY_INTERFACE_MODE_1000BASEX) {
+		if (speed != SPEED_1000) {
+			dev_err(&xpcs->mdiodev->dev,
+				"%s: speed %dMbps not supported\n",
+				__func__, speed);
+			return;
+		}
 
-	if (speed != SPEED_1000) {
-		dev_err(&xpcs->mdiodev->dev, "%s: speed %dMbps not supported\n",
-			__func__, speed);
-		return;
+		if (duplex != DUPLEX_FULL)
+			dev_err(&xpcs->mdiodev->dev,
+				"%s: half duplex not supported\n",
+				__func__);
 	}
 
-	if (duplex != DUPLEX_FULL)
-		dev_err(&xpcs->mdiodev->dev, "%s: half duplex not supported\n",
-			__func__);
-
-	val = mii_bmcr_encode_fixed(speed, duplex);
-	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR, val);
+	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+			 mii_bmcr_encode_fixed(speed, duplex));
 	if (ret)
 		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
 			__func__, ERR_PTR(ret));
@@ -1151,10 +1142,11 @@ static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 
 	if (interface == PHY_INTERFACE_MODE_USXGMII)
 		return xpcs_config_usxgmii(xpcs, speed);
-	if (interface == PHY_INTERFACE_MODE_SGMII)
-		return xpcs_link_up_sgmii(xpcs, neg_mode, speed, duplex);
-	if (interface == PHY_INTERFACE_MODE_1000BASEX)
-		return xpcs_link_up_1000basex(xpcs, neg_mode, speed, duplex);
+
+	if (interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_1000BASEX)
+		return xpcs_link_up_sgmii_1000basex(xpcs, neg_mode, interface,
+						    speed, duplex);
 }
 
 static void xpcs_an_restart(struct phylink_pcs *pcs)
-- 
2.30.2

[PATCH net-next 6/7] net: pcs: xpcs: rename xpcs_config_usxgmii()

[Russell King (Oracle)]

xpcs_config_usxgmii() is only called from the xpcs_link_up() method, so
let's name it similarly to the SGMII and 1000BASEX functions.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 6cc658f8366c..89ceedc0f18b 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -311,7 +311,7 @@ static int xpcs_read_fault_c73(struct dw_xpcs *xpcs,
 	return 0;
 }
 
-static void xpcs_config_usxgmii(struct dw_xpcs *xpcs, int speed)
+static void xpcs_link_up_usxgmii(struct dw_xpcs *xpcs, int speed)
 {
 	int ret, speed_sel;
 
@@ -1141,7 +1141,7 @@ static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
 	if (interface == PHY_INTERFACE_MODE_USXGMII)
-		return xpcs_config_usxgmii(xpcs, speed);
+		return xpcs_link_up_usxgmii(xpcs, speed);
 
 	if (interface == PHY_INTERFACE_MODE_SGMII ||
 	    interface == PHY_INTERFACE_MODE_1000BASEX)
-- 
2.30.2

[PATCH net-next 7/7] net: pcs: xpcs: remove return statements in void function

[Russell King (Oracle)]

While using "return" when calling a void returning function inside a
function that returns void doesn't cause a compiler warning, it looks
weird. Convert the bunch of if() statements to a switch() and remove
these return statements.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 89ceedc0f18b..7246a910728d 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1140,13 +1140,20 @@ static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
 
-	if (interface == PHY_INTERFACE_MODE_USXGMII)
-		return xpcs_link_up_usxgmii(xpcs, speed);
+	switch (interface) {
+	case PHY_INTERFACE_MODE_USXGMII:
+		xpcs_link_up_usxgmii(xpcs, speed);
+		break;
+
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		xpcs_link_up_sgmii_1000basex(xpcs, neg_mode, interface, speed,
+					     duplex);
+		break;
 
-	if (interface == PHY_INTERFACE_MODE_SGMII ||
-	    interface == PHY_INTERFACE_MODE_1000BASEX)
-		return xpcs_link_up_sgmii_1000basex(xpcs, neg_mode, interface,
-						    speed, duplex);
+	default:
+		break;
+	}
 }
 
 static void xpcs_an_restart(struct phylink_pcs *pcs)
-- 
2.30.2

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Serge Semin]

Hi

On Thu, Oct 17, 2024 at 12:52:17PM GMT, Russell King (Oracle) wrote:
> Hi,
> 
> I've found yet more potential for cleanups in the XPCS driver.
> 
> The first patch switches to using generic register definitions.
> 
> Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
> which can be simplified down to a simple if() statement.
> 
> Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
> from the functional bit.
> 
> Next, realising that the functional bit is just the helper function we
> already have and are using in the SGMII version of this function,
> switch over to that.
> 
> We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
> are basically functionally identical except for the warnings, so merge
> the two functions.
> 
> Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
> established pattern.
> 
> Lastly, "return foo();" where foo is a void function and the function
> being returned from is also void is a weird programming pattern.
> Replace this with something more conventional.
> 
> With these changes, we see yet another reduction in the amount of
> code in this driver.

If you wish this to be tested before merging in, I'll be able to do
that tomorrow or on Wednesday. In anyway I'll get back with the
results after testing the series out.

-Serge(y)

> 
>  drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
>  drivers/net/pcs/pcs-xpcs.h |  12 ----
>  2 files changed, 65 insertions(+), 81 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Russell King (Oracle)]

Hi netdev maintainers,

I see patchwork has failed again. It claims this series does not have a
cover letter, but it does, and lore has it:

https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/

vs

https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/

I guess the kernel.org infrastructure has failed in some way to deliver
the cover message to patchwork.

On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> I've found yet more potential for cleanups in the XPCS driver.
> 
> The first patch switches to using generic register definitions.
> 
> Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
> which can be simplified down to a simple if() statement.
> 
> Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
> from the functional bit.
> 
> Next, realising that the functional bit is just the helper function we
> already have and are using in the SGMII version of this function,
> switch over to that.
> 
> We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
> are basically functionally identical except for the warnings, so merge
> the two functions.
> 
> Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
> established pattern.
> 
> Lastly, "return foo();" where foo is a void function and the function
> being returned from is also void is a weird programming pattern.
> Replace this with something more conventional.
> 
> With these changes, we see yet another reduction in the amount of
> code in this driver.
> 
>  drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
>  drivers/net/pcs/pcs-xpcs.h |  12 ----
>  2 files changed, 65 insertions(+), 81 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

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

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Paolo Abeni]

On 10/22/24 10:59, Russell King (Oracle) wrote:
> I see patchwork has failed again. It claims this series does not have a
> cover letter, but it does, and lore has it:
> 
> https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/
> 
> vs
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/
> 
> I guess the kernel.org infrastructure has failed in some way to deliver
> the cover message to patchwork.

Thanks for the head-up!

I can't investigate the issue any deeper than you, lacking permissions
on the relevant hosts, but I verified that the merged script fetch the
cover letter correctly, so no need to repost just for that.

Cheers,

Paolo

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Russell King (Oracle)]

On Tue, Oct 22, 2024 at 11:23:30AM +0200, Paolo Abeni wrote:
> On 10/22/24 10:59, Russell King (Oracle) wrote:
> > I see patchwork has failed again. It claims this series does not have a
> > cover letter, but it does, and lore has it:
> > 
> > https://lore.kernel.org/all/ZxD6cVFajwBlC9eN@shell.armlinux.org.uk/
> > 
> > vs
> > 
> > https://patchwork.kernel.org/project/netdevbpf/patch/E1t1P3X-000EJx-ES@rmk-PC.armlinux.org.uk/
> > 
> > I guess the kernel.org infrastructure has failed in some way to deliver
> > the cover message to patchwork.
> 
> Thanks for the head-up!
> 
> I can't investigate the issue any deeper than you, lacking permissions
> on the relevant hosts, but I verified that the merged script fetch the
> cover letter correctly, so no need to repost just for that.

Thanks for checking. Johannes explained that the checks are done by
Jakub's nipa bot and uploaded to patchwork.

https://github.com/linux-netdev/nipa/blob/main/tests/series/series_format/test.py

Seems nipa (or Jakub?) missed out on the cover message.

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

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Serge Semin]

On Mon, Oct 21, 2024 at 02:09:02PM GMT, Serge Semin wrote:
> Hi
> 
> On Thu, Oct 17, 2024 at 12:52:17PM GMT, Russell King (Oracle) wrote:
> > Hi,
> > 
> > I've found yet more potential for cleanups in the XPCS driver.
> > 
> > The first patch switches to using generic register definitions.
> > 
> > Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
> > which can be simplified down to a simple if() statement.
> > 
> > Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
> > from the functional bit.
> > 
> > Next, realising that the functional bit is just the helper function we
> > already have and are using in the SGMII version of this function,
> > switch over to that.
> > 
> > We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
> > are basically functionally identical except for the warnings, so merge
> > the two functions.
> > 
> > Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
> > established pattern.
> > 
> > Lastly, "return foo();" where foo is a void function and the function
> > being returned from is also void is a weird programming pattern.
> > Replace this with something more conventional.
> > 
> > With these changes, we see yet another reduction in the amount of
> > code in this driver.
> 
> If you wish this to be tested before merging in, I'll be able to do
> that tomorrow or on Wednesday. In anyway I'll get back with the
> results after testing the series out.

As I promised, the series has been tested on the hardware with the
next setup:

DW XGMAC <-(XGMII)-> DW XPCS <-(10Gbase-R)-> Marvell 88x2222
<-(10gbase-R)->
SFP+ JT-DAC-SFP-05 SFP+
<-(10gbase-R)->
Marvell 88x2222 <-(10gbase-R)-> DW XPCS <-(XGMII)-> DW XGMAC

No problem has been spotted for both STMMAC and DW XPCS drivers:

Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> 
> -Serge(y)
> 
> > 
> >  drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
> >  drivers/net/pcs/pcs-xpcs.h |  12 ----
> >  2 files changed, 65 insertions(+), 81 deletions(-)
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> > 

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Russell King (Oracle)]

On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> I've found yet more potential for cleanups in the XPCS driver.
> 
> The first patch switches to using generic register definitions.
> 
> Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
> which can be simplified down to a simple if() statement.
> 
> Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
> from the functional bit.
> 
> Next, realising that the functional bit is just the helper function we
> already have and are using in the SGMII version of this function,
> switch over to that.
> 
> We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
> are basically functionally identical except for the warnings, so merge
> the two functions.
> 
> Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
> established pattern.
> 
> Lastly, "return foo();" where foo is a void function and the function
> being returned from is also void is a weird programming pattern.
> Replace this with something more conventional.
> 
> With these changes, we see yet another reduction in the amount of
> code in this driver.
> 
>  drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
>  drivers/net/pcs/pcs-xpcs.h |  12 ----
>  2 files changed, 65 insertions(+), 81 deletions(-)

It's been almost a week, and this series has not been applied. First,
Jakub's NIPA bot failed to spot the cover message that patchwork picked
up - not my problem.

Now, I find that patchwork says "changes requested". What changes? No
one has replied asking for any changes to this series. Serge did
reply saying he would test it, and he has now done so, and replied
with his tested-by.

So, marking this series as "changes requested" is entirely incorrect.

If you think changes are required, please say what they are, or if no
changes are required, please apply this series.

Thanks.

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

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Paolo Abeni]

On 10/23/24 13:17, Russell King (Oracle) wrote:
> On Thu, Oct 17, 2024 at 12:52:17PM +0100, Russell King (Oracle) wrote:
>> I've found yet more potential for cleanups in the XPCS driver.
>>
>> The first patch switches to using generic register definitions.
>>
>> Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
>> which can be simplified down to a simple if() statement.
>>
>> Then, rearrange xpcs_link_up_1000basex() to separate out the warnings
>> from the functional bit.
>>
>> Next, realising that the functional bit is just the helper function we
>> already have and are using in the SGMII version of this function,
>> switch over to that.
>>
>> We can now see that xpcs_link_up_1000basex() and xpcs_link_up_sgmii()
>> are basically functionally identical except for the warnings, so merge
>> the two functions.
>>
>> Next, xpcs_config_usxgmii() seems misnamed, so rename it to follow the
>> established pattern.
>>
>> Lastly, "return foo();" where foo is a void function and the function
>> being returned from is also void is a weird programming pattern.
>> Replace this with something more conventional.
>>
>> With these changes, we see yet another reduction in the amount of
>> code in this driver.
>>
>>  drivers/net/pcs/pcs-xpcs.c | 134 ++++++++++++++++++++++-----------------------
>>  drivers/net/pcs/pcs-xpcs.h |  12 ----
>>  2 files changed, 65 insertions(+), 81 deletions(-)
> 
> It's been almost a week, and this series has not been applied.

Yep, we are lagging a little behind our PW queue due to limited capacity.

> First,
> Jakub's NIPA bot failed to spot the cover message that patchwork picked
> up - not my problem.
> 
> Now, I find that patchwork says "changes requested". What changes? No
> one has replied asking for any changes to this series. Serge did
> reply saying he would test it, and he has now done so, and replied
> with his tested-by.

I agree there is no reason for the 'changes requested' status, I guess
such change happened due to some miscommunication between me and Andrew.
Let me resurrect the series in PW.

I'll go over that soon.

Thanks,

Paolo

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 17 Oct 2024 12:52:17 +0100 you wrote:
> Hi,
> 
> I've found yet more potential for cleanups in the XPCS driver.
> 
> The first patch switches to using generic register definitions.
> 
> Next, there's an overly complex bit of code in xpcs_link_up_1000basex()
> which can be simplified down to a simple if() statement.
> 
> [...]

Here is the summary with links:
  - [net-next,1/7] net: pcs: xpcs: use generic register definitions
    https://git.kernel.org/netdev/net-next/c/1d2709d6d390
  - [net-next,2/7] net: pcs: xpcs: remove switch() in xpcs_link_up_1000basex()
    https://git.kernel.org/netdev/net-next/c/8d2aeab4ce78
  - [net-next,3/7] net: pcs: xpcs: rearrange xpcs_link_up_1000basex()
    https://git.kernel.org/netdev/net-next/c/b61a465a7619
  - [net-next,4/7] net: pcs: xpcs: replace open-coded mii_bmcr_encode_fixed()
    https://git.kernel.org/netdev/net-next/c/1c17f9d3fe17
  - [net-next,5/7] net: pcs: xpcs: combine xpcs_link_up_{1000basex,sgmii}()
    https://git.kernel.org/netdev/net-next/c/4145921c3055
  - [net-next,6/7] net: pcs: xpcs: rename xpcs_config_usxgmii()
    https://git.kernel.org/netdev/net-next/c/11afdf3b2ece
  - [net-next,7/7] net: pcs: xpcs: remove return statements in void function
    https://git.kernel.org/netdev/net-next/c/fd4056db7aee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 0/7] net: pcs: xpcs: yet more cleanups

[Jakub Kicinski]

On Wed, 23 Oct 2024 12:17:18 +0100 Russell King (Oracle) wrote:
> It's been almost a week, and this series has not been applied. First,
> Jakub's NIPA bot failed to spot the cover message that patchwork picked
> up - not my problem.

FWIW NIPA gets the patch <> series metadata from patchwork.
The only possible explanation I have for cover letter being
in patchwork but getting flagged as not present by NIPA is
that it arrived late.

I keep repeating ad nauseam that the patchwork checks have false
positives so they are for maintainers to look at, not submitters.
I think the real failure here is that someone marked this series
as CR silently and incorrectly :(