[RFC PATCH 0/2] unify full duplex flow control resolution

[RFC PATCH 0/2] unify full duplex flow control resolution

[Steve Glendinning]

4 drivers (tg3, smsc911x, smsc9420 & smsc9500) have identical full 
duplex flow control resolution functions (primarily because I re-used
tg3's implementation for the 3 SMSC drivers).  This patchset moves this 
to an inline function in mii.h and changes the 4 drivers to use it.

It's a small enough function, and it's only used once in each driver,
but I'm still not completely sure inline is right.  These drivers 
don't all have a dependency on mii.c though, and I wanted to avoid 
adding this to the SMSC drivers that have been converted to phylib.

--
Steve Glendinning (2):
  move flow control definitions to mii.h
  refactor full duplex flow control resolution

 drivers/net/smsc911x.c     |   24 +-------------
 drivers/net/smsc911x.h     |    3 --
 drivers/net/smsc9420.c     |   24 +-------------
 drivers/net/smsc9420.h     |    3 --
 drivers/net/tg3.c          |   74 +++++++++++++++----------------------------
 drivers/net/tg3.h          |    2 -
 drivers/net/usb/smsc95xx.c |   26 +---------------
 include/linux/mii.h        |   33 +++++++++++++++++++
 8 files changed, 62 insertions(+), 127 deletions(-)

[RFC PATCH 1/2] move flow control definitions to mii.h

[Steve Glendinning]

flags used within drivers for indicating tx and rx flow control are
defined in 4 drivers (and probably more), move these constants to mii.h.

The 3 SMSC drivers use the same constants (FLOW_CTRL_TX), but TG3 uses
TG3_FLOW_CTRL_TX, so this patch also renames the constants within TG3.

Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>
---
 drivers/net/smsc911x.h     |    3 --
 drivers/net/smsc9420.h     |    3 --
 drivers/net/tg3.c          |   50 ++++++++++++++++++++++----------------------
 drivers/net/tg3.h          |    2 -
 drivers/net/usb/smsc95xx.c |    2 -
 include/linux/mii.h        |    4 +++
 6 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/drivers/net/smsc911x.h b/drivers/net/smsc911x.h
index f818cf0..2b76654 100644
--- a/drivers/net/smsc911x.h
+++ b/drivers/net/smsc911x.h
@@ -61,9 +61,6 @@
 #define SMSC_ASSERT_MAC_LOCK(pdata) do {} while (0)
 #endif				/* CONFIG_DEBUG_SPINLOCK */
 
-#define FLOW_CTRL_TX		(1)
-#define FLOW_CTRL_RX		(2)
-
 /* SMSC911x registers and bitfields */
 #define RX_DATA_FIFO			0x00
 
diff --git a/drivers/net/smsc9420.h b/drivers/net/smsc9420.h
index 80c426d..69c351f 100644
--- a/drivers/net/smsc9420.h
+++ b/drivers/net/smsc9420.h
@@ -45,9 +45,6 @@
 
 #define SMSC9420_EEPROM_SIZE		((u32)11)
 
-#define FLOW_CTRL_TX			(1)
-#define FLOW_CTRL_RX			(2)
-
 #define PKT_BUF_SZ			(VLAN_ETH_FRAME_LEN + NET_IP_ALIGN + 4)
 
 /***********************************************/
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 6e40d52..f353f69 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -1187,9 +1187,9 @@ static void tg3_link_report(struct tg3 *tp)
 		printk(KERN_INFO PFX
 		       "%s: Flow control is %s for TX and %s for RX.\n",
 		       tp->dev->name,
-		       (tp->link_config.active_flowctrl & TG3_FLOW_CTRL_TX) ?
+		       (tp->link_config.active_flowctrl & FLOW_CTRL_TX) ?
 		       "on" : "off",
-		       (tp->link_config.active_flowctrl & TG3_FLOW_CTRL_RX) ?
+		       (tp->link_config.active_flowctrl & FLOW_CTRL_RX) ?
 		       "on" : "off");
 		tg3_ump_link_report(tp);
 	}
@@ -1199,11 +1199,11 @@ static u16 tg3_advert_flowctrl_1000T(u8 flow_ctrl)
 {
 	u16 miireg;
 
-	if ((flow_ctrl & TG3_FLOW_CTRL_TX) && (flow_ctrl & TG3_FLOW_CTRL_RX))
+	if ((flow_ctrl & FLOW_CTRL_TX) && (flow_ctrl & FLOW_CTRL_RX))
 		miireg = ADVERTISE_PAUSE_CAP;
-	else if (flow_ctrl & TG3_FLOW_CTRL_TX)
+	else if (flow_ctrl & FLOW_CTRL_TX)
 		miireg = ADVERTISE_PAUSE_ASYM;
-	else if (flow_ctrl & TG3_FLOW_CTRL_RX)
+	else if (flow_ctrl & FLOW_CTRL_RX)
 		miireg = ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
 	else
 		miireg = 0;
@@ -1215,11 +1215,11 @@ static u16 tg3_advert_flowctrl_1000X(u8 flow_ctrl)
 {
 	u16 miireg;
 
-	if ((flow_ctrl & TG3_FLOW_CTRL_TX) && (flow_ctrl & TG3_FLOW_CTRL_RX))
+	if ((flow_ctrl & FLOW_CTRL_TX) && (flow_ctrl & FLOW_CTRL_RX))
 		miireg = ADVERTISE_1000XPAUSE;
-	else if (flow_ctrl & TG3_FLOW_CTRL_TX)
+	else if (flow_ctrl & FLOW_CTRL_TX)
 		miireg = ADVERTISE_1000XPSE_ASYM;
-	else if (flow_ctrl & TG3_FLOW_CTRL_RX)
+	else if (flow_ctrl & FLOW_CTRL_RX)
 		miireg = ADVERTISE_1000XPAUSE | ADVERTISE_1000XPSE_ASYM;
 	else
 		miireg = 0;
@@ -1256,16 +1256,16 @@ static u8 tg3_resolve_flowctrl_1000X(u16 lcladv, u16 rmtadv)
 	if (lcladv & ADVERTISE_1000XPAUSE) {
 		if (lcladv & ADVERTISE_1000XPSE_ASYM) {
 			if (rmtadv & LPA_1000XPAUSE)
-				cap = TG3_FLOW_CTRL_TX | TG3_FLOW_CTRL_RX;
+				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
 			else if (rmtadv & LPA_1000XPAUSE_ASYM)
-				cap = TG3_FLOW_CTRL_RX;
+				cap = FLOW_CTRL_RX;
 		} else {
 			if (rmtadv & LPA_1000XPAUSE)
-				cap = TG3_FLOW_CTRL_TX | TG3_FLOW_CTRL_RX;
+				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
 		}
 	} else if (lcladv & ADVERTISE_1000XPSE_ASYM) {
 		if ((rmtadv & LPA_1000XPAUSE) && (rmtadv & LPA_1000XPAUSE_ASYM))
-			cap = TG3_FLOW_CTRL_TX;
+			cap = FLOW_CTRL_TX;
 	}
 
 	return cap;
@@ -1294,7 +1294,7 @@ static void tg3_setup_flow_control(struct tg3 *tp, u32 lcladv, u32 rmtadv)
 
 	tp->link_config.active_flowctrl = flowctrl;
 
-	if (flowctrl & TG3_FLOW_CTRL_RX)
+	if (flowctrl & FLOW_CTRL_RX)
 		tp->rx_mode |= RX_MODE_FLOW_CTRL_ENABLE;
 	else
 		tp->rx_mode &= ~RX_MODE_FLOW_CTRL_ENABLE;
@@ -1302,7 +1302,7 @@ static void tg3_setup_flow_control(struct tg3 *tp, u32 lcladv, u32 rmtadv)
 	if (old_rx_mode != tp->rx_mode)
 		tw32_f(MAC_RX_MODE, tp->rx_mode);
 
-	if (flowctrl & TG3_FLOW_CTRL_TX)
+	if (flowctrl & FLOW_CTRL_TX)
 		tp->tx_mode |= TX_MODE_FLOW_CTRL_ENABLE;
 	else
 		tp->tx_mode &= ~TX_MODE_FLOW_CTRL_ENABLE;
@@ -9419,12 +9419,12 @@ static void tg3_get_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 
 	epause->autoneg = (tp->tg3_flags & TG3_FLAG_PAUSE_AUTONEG) != 0;
 
-	if (tp->link_config.active_flowctrl & TG3_FLOW_CTRL_RX)
+	if (tp->link_config.active_flowctrl & FLOW_CTRL_RX)
 		epause->rx_pause = 1;
 	else
 		epause->rx_pause = 0;
 
-	if (tp->link_config.active_flowctrl & TG3_FLOW_CTRL_TX)
+	if (tp->link_config.active_flowctrl & FLOW_CTRL_TX)
 		epause->tx_pause = 1;
 	else
 		epause->tx_pause = 0;
@@ -9475,14 +9475,14 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 			}
 		} else {
 			if (epause->rx_pause)
-				tp->link_config.flowctrl |= TG3_FLOW_CTRL_RX;
+				tp->link_config.flowctrl |= FLOW_CTRL_RX;
 			else
-				tp->link_config.flowctrl &= ~TG3_FLOW_CTRL_RX;
+				tp->link_config.flowctrl &= ~FLOW_CTRL_RX;
 
 			if (epause->tx_pause)
-				tp->link_config.flowctrl |= TG3_FLOW_CTRL_TX;
+				tp->link_config.flowctrl |= FLOW_CTRL_TX;
 			else
-				tp->link_config.flowctrl &= ~TG3_FLOW_CTRL_TX;
+				tp->link_config.flowctrl &= ~FLOW_CTRL_TX;
 
 			if (netif_running(dev))
 				tg3_setup_flow_control(tp, 0, 0);
@@ -9502,13 +9502,13 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 		else
 			tp->tg3_flags &= ~TG3_FLAG_PAUSE_AUTONEG;
 		if (epause->rx_pause)
-			tp->link_config.flowctrl |= TG3_FLOW_CTRL_RX;
+			tp->link_config.flowctrl |= FLOW_CTRL_RX;
 		else
-			tp->link_config.flowctrl &= ~TG3_FLOW_CTRL_RX;
+			tp->link_config.flowctrl &= ~FLOW_CTRL_RX;
 		if (epause->tx_pause)
-			tp->link_config.flowctrl |= TG3_FLOW_CTRL_TX;
+			tp->link_config.flowctrl |= FLOW_CTRL_TX;
 		else
-			tp->link_config.flowctrl &= ~TG3_FLOW_CTRL_TX;
+			tp->link_config.flowctrl &= ~FLOW_CTRL_TX;
 
 		if (netif_running(dev)) {
 			tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
@@ -13849,7 +13849,7 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 
 	/* flow control autonegotiation is default behavior */
 	tp->tg3_flags |= TG3_FLAG_PAUSE_AUTONEG;
-	tp->link_config.flowctrl = TG3_FLOW_CTRL_TX | TG3_FLOW_CTRL_RX;
+	tp->link_config.flowctrl = FLOW_CTRL_TX | FLOW_CTRL_RX;
 
 	tg3_init_coal(tp);
 
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 6155676..0880cfa 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2338,8 +2338,6 @@ struct tg3_link_config {
 	u8				duplex;
 	u8				autoneg;
 	u8				flowctrl;
-#define TG3_FLOW_CTRL_TX		0x01
-#define TG3_FLOW_CTRL_RX		0x02
 
 	/* Describes what we actually have. */
 	u8				active_flowctrl;
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 4638a7b..ee2eac3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -45,8 +45,6 @@
 #define SMSC95XX_INTERNAL_PHY_ID	(1)
 #define SMSC95XX_TX_OVERHEAD		(8)
 #define SMSC95XX_TX_OVERHEAD_CSUM	(12)
-#define FLOW_CTRL_TX			(1)
-#define FLOW_CTRL_RX			(2)
 
 struct smsc95xx_priv {
 	u32 mac_cr;
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 151b7e0..4a376e0 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -135,6 +135,10 @@
 #define LPA_1000FULL            0x0800  /* Link partner 1000BASE-T full duplex */
 #define LPA_1000HALF            0x0400  /* Link partner 1000BASE-T half duplex */
 
+/* Flow control flags */
+#define FLOW_CTRL_TX		0x01
+#define FLOW_CTRL_RX		0x02
+
 /* This structure is used in all SIOCxMIIxxx ioctl calls */
 struct mii_ioctl_data {
 	__u16		phy_id;
-- 
1.6.0.4

[RFC PATCH 2/2] refactor full duplex flow control resolution

[Steve Glendinning]

These 4 drivers have identical full duplex flow control resolution
functions.  This patch changes them all to use one common function.

The function in question decides whether a device should enable TX and
RX flow control in a standard way (IEEE 802.3-2005 table 28B-3), so this
should also be useful for other drivers.

Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>
---
 drivers/net/smsc911x.c     |   24 +-----------------------
 drivers/net/smsc9420.c     |   24 +-----------------------
 drivers/net/tg3.c          |   24 +-----------------------
 drivers/net/usb/smsc95xx.c |   24 +-----------------------
 include/linux/mii.h        |   29 +++++++++++++++++++++++++++++
 5 files changed, 33 insertions(+), 92 deletions(-)

diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
index ae32716..fa28542 100644
--- a/drivers/net/smsc911x.c
+++ b/drivers/net/smsc911x.c
@@ -642,28 +642,6 @@ static int smsc911x_phy_loopbacktest(struct net_device *dev)
 }
 #endif				/* USE_PHY_WORK_AROUND */
 
-static u8 smsc95xx_resolve_flowctrl_fulldplx(u16 lcladv, u16 rmtadv)
-{
-	u8 cap = 0;
-
-	if (lcladv & ADVERTISE_PAUSE_CAP) {
-		if (lcladv & ADVERTISE_PAUSE_ASYM) {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-			else if (rmtadv & LPA_PAUSE_ASYM)
-				cap = FLOW_CTRL_RX;
-		} else {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-		}
-	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
-		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
-			cap = FLOW_CTRL_TX;
-	}
-
-	return cap;
-}
-
 static void smsc911x_phy_update_flowcontrol(struct smsc911x_data *pdata)
 {
 	struct phy_device *phy_dev = pdata->phy_dev;
@@ -674,7 +652,7 @@ static void smsc911x_phy_update_flowcontrol(struct smsc911x_data *pdata)
 	if (phy_dev->duplex == DUPLEX_FULL) {
 		u16 lcladv = phy_read(phy_dev, MII_ADVERTISE);
 		u16 rmtadv = phy_read(phy_dev, MII_LPA);
-		u8 cap = smsc95xx_resolve_flowctrl_fulldplx(lcladv, rmtadv);
+		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
 
 		if (cap & FLOW_CTRL_RX)
 			flow = 0xFFFF0002;
diff --git a/drivers/net/smsc9420.c b/drivers/net/smsc9420.c
index 2a8e9b7..47450d4 100644
--- a/drivers/net/smsc9420.c
+++ b/drivers/net/smsc9420.c
@@ -1055,28 +1055,6 @@ static void smsc9420_set_multicast_list(struct net_device *dev)
 	smsc9420_pci_flush_write(pd);
 }
 
-static u8 smsc9420_resolve_flowctrl_fulldplx(u16 lcladv, u16 rmtadv)
-{
-	u8 cap = 0;
-
-	if (lcladv & ADVERTISE_PAUSE_CAP) {
-		if (lcladv & ADVERTISE_PAUSE_ASYM) {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-			else if (rmtadv & LPA_PAUSE_ASYM)
-				cap = FLOW_CTRL_RX;
-		} else {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-		}
-	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
-		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
-			cap = FLOW_CTRL_TX;
-	}
-
-	return cap;
-}
-
 static void smsc9420_phy_update_flowcontrol(struct smsc9420_pdata *pd)
 {
 	struct phy_device *phy_dev = pd->phy_dev;
@@ -1085,7 +1063,7 @@ static void smsc9420_phy_update_flowcontrol(struct smsc9420_pdata *pd)
 	if (phy_dev->duplex == DUPLEX_FULL) {
 		u16 lcladv = phy_read(phy_dev, MII_ADVERTISE);
 		u16 rmtadv = phy_read(phy_dev, MII_LPA);
-		u8 cap = smsc9420_resolve_flowctrl_fulldplx(lcladv, rmtadv);
+		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
 
 		if (cap & FLOW_CTRL_RX)
 			flow = 0xFFFF0002;
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index f353f69..06bd2f4 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -1227,28 +1227,6 @@ static u16 tg3_advert_flowctrl_1000X(u8 flow_ctrl)
 	return miireg;
 }
 
-static u8 tg3_resolve_flowctrl_1000T(u16 lcladv, u16 rmtadv)
-{
-	u8 cap = 0;
-
-	if (lcladv & ADVERTISE_PAUSE_CAP) {
-		if (lcladv & ADVERTISE_PAUSE_ASYM) {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = TG3_FLOW_CTRL_TX | TG3_FLOW_CTRL_RX;
-			else if (rmtadv & LPA_PAUSE_ASYM)
-				cap = TG3_FLOW_CTRL_RX;
-		} else {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = TG3_FLOW_CTRL_TX | TG3_FLOW_CTRL_RX;
-		}
-	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
-		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
-			cap = TG3_FLOW_CTRL_TX;
-	}
-
-	return cap;
-}
-
 static u8 tg3_resolve_flowctrl_1000X(u16 lcladv, u16 rmtadv)
 {
 	u8 cap = 0;
@@ -1288,7 +1266,7 @@ static void tg3_setup_flow_control(struct tg3 *tp, u32 lcladv, u32 rmtadv)
 		if (tp->tg3_flags2 & TG3_FLG2_ANY_SERDES)
 			flowctrl = tg3_resolve_flowctrl_1000X(lcladv, rmtadv);
 		else
-			flowctrl = tg3_resolve_flowctrl_1000T(lcladv, rmtadv);
+			flowctrl = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
 	} else
 		flowctrl = tp->link_config.flowctrl;
 
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index ee2eac3..fed22ff 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -435,28 +435,6 @@ static void smsc95xx_set_multicast(struct net_device *netdev)
 	smsc95xx_write_reg_async(dev, MAC_CR, &pdata->mac_cr);
 }
 
-static u8 smsc95xx_resolve_flowctrl_fulldplx(u16 lcladv, u16 rmtadv)
-{
-	u8 cap = 0;
-
-	if (lcladv & ADVERTISE_PAUSE_CAP) {
-		if (lcladv & ADVERTISE_PAUSE_ASYM) {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-			else if (rmtadv & LPA_PAUSE_ASYM)
-				cap = FLOW_CTRL_RX;
-		} else {
-			if (rmtadv & LPA_PAUSE_CAP)
-				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
-		}
-	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
-		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
-			cap = FLOW_CTRL_TX;
-	}
-
-	return cap;
-}
-
 static void smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
 					    u16 lcladv, u16 rmtadv)
 {
@@ -469,7 +447,7 @@ static void smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
 	}
 
 	if (duplex == DUPLEX_FULL) {
-		u8 cap = smsc95xx_resolve_flowctrl_fulldplx(lcladv, rmtadv);
+		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
 
 		if (cap & FLOW_CTRL_RX)
 			flow = 0xFFFF0002;
diff --git a/include/linux/mii.h b/include/linux/mii.h
index 4a376e0..ad74858 100644
--- a/include/linux/mii.h
+++ b/include/linux/mii.h
@@ -239,5 +239,34 @@ static inline unsigned int mii_duplex (unsigned int duplex_lock,
 	return 0;
 }
 
+/**
+ * mii_resolve_flowctrl_fdx
+ * @lcladv: value of MII ADVERTISE register
+ * @rmtadv: value of MII LPA register
+ *
+ * Resolve full duplex flow control as per IEEE 802.3-2005 table 28B-3
+ */
+static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
+{
+	u8 cap = 0;
+
+	if (lcladv & ADVERTISE_PAUSE_CAP) {
+		if (lcladv & ADVERTISE_PAUSE_ASYM) {
+			if (rmtadv & LPA_PAUSE_CAP)
+				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
+			else if (rmtadv & LPA_PAUSE_ASYM)
+				cap = FLOW_CTRL_RX;
+		} else {
+			if (rmtadv & LPA_PAUSE_CAP)
+				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
+		}
+	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
+		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
+			cap = FLOW_CTRL_TX;
+	}
+
+	return cap;
+}
+
 #endif /* __KERNEL__ */
 #endif /* __LINUX_MII_H__ */
-- 
1.6.0.4

Re: [RFC PATCH 1/2] move flow control definitions to mii.h

[David Miller]

From: Steve Glendinning <steve.glendinning@smsc.com>
Date: Sun, 14 Dec 2008 12:38:20 +0000

> flags used within drivers for indicating tx and rx flow control are
> defined in 4 drivers (and probably more), move these constants to mii.h.
> 
> The 3 SMSC drivers use the same constants (FLOW_CTRL_TX), but TG3 uses
> TG3_FLOW_CTRL_TX, so this patch also renames the constants within TG3.
> 
> Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>

Applied.

Re: [RFC PATCH 2/2] refactor full duplex flow control resolution

[David Miller]

From: Steve Glendinning <steve.glendinning@smsc.com>
Date: Sun, 14 Dec 2008 12:38:21 +0000

> These 4 drivers have identical full duplex flow control resolution
> functions.  This patch changes them all to use one common function.
> 
> The function in question decides whether a device should enable TX and
> RX flow control in a standard way (IEEE 802.3-2005 table 28B-3), so this
> should also be useful for other drivers.
> 
> Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>

Looks fine, applied.

Thanks Steve.

Re: [RFC PATCH 1/2] move flow control definitions to mii.h

[Andrew Morton]

On Sun, 14 Dec 2008 12:38:20 +0000
Steve Glendinning <steve.glendinning@smsc.com> wrote:

> flags used within drivers for indicating tx and rx flow control are
> defined in 4 drivers (and probably more), move these constants to mii.h.
> 
> The 3 SMSC drivers use the same constants (FLOW_CTRL_TX), but TG3 uses
> TG3_FLOW_CTRL_TX, so this patch also renames the constants within TG3.
> 
> Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>
> ---
>  drivers/net/smsc911x.h     |    3 --
>  drivers/net/smsc9420.h     |    3 --
>  drivers/net/tg3.c          |   50 ++++++++++++++++++++++----------------------
>  drivers/net/tg3.h          |    2 -
>  drivers/net/usb/smsc95xx.c |    2 -
>  include/linux/mii.h        |    4 +++

In file included from drivers/net/bnx2.c:52:
drivers/net/bnx2.h:6851:1: warning: "FLOW_CTRL_TX" redefined
In file included from drivers/net/bnx2.c:37:
include/linux/mii.h:139:1: warning: this is the location of the previous definition
In file included from drivers/net/bnx2.c:52:
drivers/net/bnx2.h:6852:1: warning: "FLOW_CTRL_RX" redefined

There are unfixed definitions of FLOW_CTRL_TX in drivers/net/bnx2.h and
drivers/net/bnx2x_link.h.

Re: [RFC PATCH 1/2] move flow control definitions to mii.h

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 17 Dec 2008 16:17:08 -0800

> On Sun, 14 Dec 2008 12:38:20 +0000
> Steve Glendinning <steve.glendinning@smsc.com> wrote:
> 
> > flags used within drivers for indicating tx and rx flow control are
> > defined in 4 drivers (and probably more), move these constants to mii.h.
> > 
> > The 3 SMSC drivers use the same constants (FLOW_CTRL_TX), but TG3 uses
> > TG3_FLOW_CTRL_TX, so this patch also renames the constants within TG3.
> > 
> > Signed-off-by: Steve Glendinning <steve.glendinning@smsc.com>
> > ---
> >  drivers/net/smsc911x.h     |    3 --
> >  drivers/net/smsc9420.h     |    3 --
> >  drivers/net/tg3.c          |   50 ++++++++++++++++++++++----------------------
> >  drivers/net/tg3.h          |    2 -
> >  drivers/net/usb/smsc95xx.c |    2 -
> >  include/linux/mii.h        |    4 +++
> 
> In file included from drivers/net/bnx2.c:52:
> drivers/net/bnx2.h:6851:1: warning: "FLOW_CTRL_TX" redefined
> In file included from drivers/net/bnx2.c:37:
> include/linux/mii.h:139:1: warning: this is the location of the previous definition
> In file included from drivers/net/bnx2.c:52:
> drivers/net/bnx2.h:6852:1: warning: "FLOW_CTRL_RX" redefined
> 
> There are unfixed definitions of FLOW_CTRL_TX in drivers/net/bnx2.h and
> drivers/net/bnx2x_link.h.

Already pointed out by Stephen Rothwell yesterday and fixed in
my tree 10 minutes later.

Re: [RFC PATCH 2/2] refactor full duplex flow control resolution

[Ben Hutchings]

On Sun, 2008-12-14 at 12:38 +0000, Steve Glendinning wrote:
[...]
> +/**
> + * mii_resolve_flowctrl_fdx
> + * @lcladv: value of MII ADVERTISE register
> + * @rmtadv: value of MII LPA register
> + *
> + * Resolve full duplex flow control as per IEEE 802.3-2005 table 28B-3
> + */
> +static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
> +{
> +	u8 cap = 0;
> +
> +	if (lcladv & ADVERTISE_PAUSE_CAP) {
> +		if (lcladv & ADVERTISE_PAUSE_ASYM) {
> +			if (rmtadv & LPA_PAUSE_CAP)
> +				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> +			else if (rmtadv & LPA_PAUSE_ASYM)
> +				cap = FLOW_CTRL_RX;
> +		} else {
> +			if (rmtadv & LPA_PAUSE_CAP)
> +				cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> +		}
> +	} else if (lcladv & ADVERTISE_PAUSE_ASYM) {
> +		if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
> +			cap = FLOW_CTRL_TX;
> +	}
> +
> +	return cap;
> +}

There's a rather more elegant way to write this, which is:

	if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
		cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
	} else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
		if (lcladv & ADVERTISE_PAUSE_CAP)
			cap = FLOW_CTRL_RX;
		else if (rmtadv & ADVERTISE_PAUSE_CAP)
			cap = FLOW_CTRL_TX;
	}

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 1/2] move flow control definitions to mii.h

[Ben Hutchings]

On Sun, 2008-12-14 at 12:38 +0000, Steve Glendinning wrote:
[...]
> diff --git a/include/linux/mii.h b/include/linux/mii.h
> index 151b7e0..4a376e0 100644
> --- a/include/linux/mii.h
> +++ b/include/linux/mii.h
> @@ -135,6 +135,10 @@
>  #define LPA_1000FULL            0x0800  /* Link partner 1000BASE-T full duplex */
>  #define LPA_1000HALF            0x0400  /* Link partner 1000BASE-T half duplex */
>  
> +/* Flow control flags */
> +#define FLOW_CTRL_TX		0x01
> +#define FLOW_CTRL_RX		0x02
> +
>  /* This structure is used in all SIOCxMIIxxx ioctl calls */
>  struct mii_ioctl_data {
>  	__u16		phy_id;

How about a FLOW_CTRL_AUTO flag to represent whether auto-negotiation of
FC is enabled (or inversely FLOW_CTRL_FORCE)?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 2/2] refactor full duplex flow control resolution

[Steve.Glendinning]

Thanks Ben, this looks good.

Do you want to submit a patch for this or should I?

Regards,
--
Steve Glendinning
SMSC GmbH
m: +44 777 933 9124
e: steve.glendinning@smsc.com



Ben Hutchings <bhutchings@solarflare.com> wrote on 23/12/2008 17:45:35:

> On Sun, 2008-12-14 at 12:38 +0000, Steve Glendinning wrote:
> [...]
> > +/**
> > + * mii_resolve_flowctrl_fdx
> > + * @lcladv: value of MII ADVERTISE register
> > + * @rmtadv: value of MII LPA register
> > + *
> > + * Resolve full duplex flow control as per IEEE 802.3-2005 table 
28B-3
> > + */
> > +static inline u8 mii_resolve_flowctrl_fdx(u16 lcladv, u16 rmtadv)
> > +{
> > +   u8 cap = 0;
> > +
> > +   if (lcladv & ADVERTISE_PAUSE_CAP) {
> > +      if (lcladv & ADVERTISE_PAUSE_ASYM) {
> > +         if (rmtadv & LPA_PAUSE_CAP)
> > +            cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> > +         else if (rmtadv & LPA_PAUSE_ASYM)
> > +            cap = FLOW_CTRL_RX;
> > +      } else {
> > +         if (rmtadv & LPA_PAUSE_CAP)
> > +            cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
> > +      }
> > +   } else if (lcladv & ADVERTISE_PAUSE_ASYM) {
> > +      if ((rmtadv & LPA_PAUSE_CAP) && (rmtadv & LPA_PAUSE_ASYM))
> > +         cap = FLOW_CTRL_TX;
> > +   }
> > +
> > +   return cap;
> > +}
> 
> There's a rather more elegant way to write this, which is:
> 
>    if (lcladv & rmtadv & ADVERTISE_PAUSE_CAP) {
>       cap = FLOW_CTRL_TX | FLOW_CTRL_RX;
>    } else if (lcladv & rmtadv & ADVERTISE_PAUSE_ASYM) {
>       if (lcladv & ADVERTISE_PAUSE_CAP)
>          cap = FLOW_CTRL_RX;
>       else if (rmtadv & ADVERTISE_PAUSE_CAP)
>          cap = FLOW_CTRL_TX;
>    }
> 
> Ben.
> 
> -- 
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.