Skip to contentSkip to conversation

mvneta: implement SGMII-based in-band link state signaling

PATCH Superseded Stas Sergeev 3 participants 6 messages Mar 31, 2015 – Apr 3, 2015

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mvneta: implement SGMII-based in-band link state signaling

When MDIO bus is unavailable (common setup for SGMII), the in-band signaling must be used to correctly track link state.
This patch enables the in-band status delivery for links state changes, namely:
- link up/down
- link speed
- duplex full/half
fixed_phy link_update() callback is used to update status.

Note: SGMII setup is so common that I think it deserves a separate API rather than the per-driver handling. There is an alternative implementation that adds such API:
https://lkml.org/lkml/2015/3/27/346

CC Thomas Petazzoni <[email protected]>
CC Florian Fainelli <[email protected]>
---
 drivers/net/ethernet/marvell/mvneta.c |   90 +++++++++++++++++++++++++++++----
 1 file changed, 79 insertions(+), 11 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +79 -11
index 96208f1..13a2aa2 100644
@@ -100,6 +100,8 @@
 #define MVNETA_TXQ_CMD                           0x2448
 #define      MVNETA_TXQ_DISABLE_SHIFT            8
 #define      MVNETA_TXQ_ENABLE_MASK              0x000000ff
+#define MVNETA_GMAC_CLOCK_DIVIDER                0x24f4
+#define      MVNETA_GMAC_1MS_CLOCK_ENABLE        BIT(31)
 #define MVNETA_ACC_MODE                          0x2500
 #define MVNETA_CPU_MAP(cpu)                      (0x2540 + ((cpu) << 2))
 #define      MVNETA_CPU_RXQ_ACCESS_ALL_MASK      0x000000ff
@@ -122,6 +124,7 @@
 #define      MVNETA_TX_INTR_MASK_ALL             (0xff << 0)
 #define      MVNETA_RX_INTR_MASK(nr_rxqs)        (((1 << nr_rxqs) - 1) << 8)
 #define      MVNETA_RX_INTR_MASK_ALL             (0xff << 8)
+#define      MVNETA_MISCINTR_INTR_MASK           BIT(31)
 
 #define MVNETA_INTR_OLD_CAUSE                    0x25a8
 #define MVNETA_INTR_OLD_MASK                     0x25ac
@@ -165,6 +168,7 @@
 #define      MVNETA_GMAC_MAX_RX_SIZE_MASK        0x7ffc
 #define      MVNETA_GMAC0_PORT_ENABLE            BIT(0)
 #define MVNETA_GMAC_CTRL_2                       0x2c08
+#define      MVNETA_GMAC2_INBAND_AN_ENABLE       BIT(0)
 #define      MVNETA_GMAC2_PCS_ENABLE             BIT(3)
 #define      MVNETA_GMAC2_PORT_RGMII             BIT(4)
 #define      MVNETA_GMAC2_PORT_RESET             BIT(6)
@@ -180,9 +184,11 @@
 #define MVNETA_GMAC_AUTONEG_CONFIG               0x2c0c
 #define      MVNETA_GMAC_FORCE_LINK_DOWN         BIT(0)
 #define      MVNETA_GMAC_FORCE_LINK_PASS         BIT(1)
+#define      MVNETA_GMAC_INBAND_AN_ENABLE        BIT(2)
 #define      MVNETA_GMAC_CONFIG_MII_SPEED        BIT(5)
 #define      MVNETA_GMAC_CONFIG_GMII_SPEED       BIT(6)
 #define      MVNETA_GMAC_AN_SPEED_EN             BIT(7)
+#define      MVNETA_GMAC_AN_FLOW_CTRL_EN         BIT(11)
 #define      MVNETA_GMAC_CONFIG_FULL_DUPLEX      BIT(12)
 #define      MVNETA_GMAC_AN_DUPLEX_EN            BIT(13)
 #define MVNETA_MIB_COUNTERS_BASE                 0x3080
@@ -304,6 +310,7 @@ struct mvneta_port {
 	unsigned int link;
 	unsigned int duplex;
 	unsigned int speed;
+	int use_inband_status:1;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -994,6 +1001,20 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
 	val &= ~MVNETA_PHY_POLLING_ENABLE;
 	mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
 
+	if (pp->use_inband_status) {
+		val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
+		val &= ~(MVNETA_GMAC_FORCE_LINK_PASS |
+			 MVNETA_GMAC_FORCE_LINK_DOWN |
+			 MVNETA_GMAC_AN_FLOW_CTRL_EN);
+		val |= MVNETA_GMAC_INBAND_AN_ENABLE |
+		       MVNETA_GMAC_AN_SPEED_EN |
+		       MVNETA_GMAC_AN_DUPLEX_EN;
+		mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
+		val = mvreg_read(pp, MVNETA_GMAC_CLOCK_DIVIDER);
+		val |= MVNETA_GMAC_1MS_CLOCK_ENABLE;
+		mvreg_write(pp, MVNETA_GMAC_CLOCK_DIVIDER, val);
+	}
+
 	mvneta_set_ucast_table(pp, -1);
 	mvneta_set_special_mcast_table(pp, -1);
 	mvneta_set_other_mcast_table(pp, -1);
@@ -2043,6 +2064,23 @@ static irqreturn_t mvneta_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int mvneta_fixed_link_update(struct net_device *dev,
+				  struct fixed_phy_status *status)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
+
+	status->link = !!(gmac_stat & MVNETA_GMAC_LINK_UP);
+	if (gmac_stat & MVNETA_GMAC_SPEED_1000)
+		status->speed = SPEED_1000;
+	else if (gmac_stat & MVNETA_GMAC_SPEED_100)
+		status->speed = SPEED_100;
+	else
+		status->speed = SPEED_10;
+	status->duplex = !!(gmac_stat & MVNETA_GMAC_FULL_DUPLEX);
+	return 0;
+}
+
 /* NAPI handler
  * Bits 0 - 7 of the causeRxTx register indicate that are transmitted
  * packets on the corresponding TXQ (Bit 0 is for TX queue 1).
@@ -2063,8 +2101,12 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
 	}
 
 	/* Read cause register */
-	cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE) &
-		(MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
+	cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE);
+	if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
+		/* since phylib uses polling, not much to do here...
+		 * Any way to tell phylib to do the poll NOW? */
+		mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
+	}
 
 	/* Release Tx descriptors */
 	if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {
@@ -2109,7 +2151,9 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
 		napi_complete(napi);
 		local_irq_save(flags);
 		mvreg_write(pp, MVNETA_INTR_NEW_MASK,
-			    MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
+			    MVNETA_RX_INTR_MASK(rxq_number) |
+			    MVNETA_TX_INTR_MASK(txq_number) |
+			    MVNETA_MISCINTR_INTR_MASK);
 		local_irq_restore(flags);
 	}
 
@@ -2373,7 +2417,13 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	/* Unmask interrupts */
 	mvreg_write(pp, MVNETA_INTR_NEW_MASK,
-		    MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
+		    MVNETA_RX_INTR_MASK(rxq_number) |
+		    MVNETA_TX_INTR_MASK(txq_number) |
+		    MVNETA_MISCINTR_INTR_MASK);
+	mvreg_write(pp, MVNETA_INTR_MISC_MASK,
+		    MVNETA_CAUSE_PHY_STATUS_CHANGE |
+		    MVNETA_CAUSE_LINK_CHANGE |
+		    MVNETA_CAUSE_PSC_SYNC_CHANGE);
 
 	phy_start(pp->phy_dev);
 	netif_tx_start_all_queues(pp->dev);
@@ -2523,9 +2573,7 @@ static void mvneta_adjust_link(struct net_device *ndev)
 			val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
 			val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
 				 MVNETA_GMAC_CONFIG_GMII_SPEED |
-				 MVNETA_GMAC_CONFIG_FULL_DUPLEX |
-				 MVNETA_GMAC_AN_SPEED_EN |
-				 MVNETA_GMAC_AN_DUPLEX_EN);
+				 MVNETA_GMAC_CONFIG_FULL_DUPLEX);
 
 			if (phydev->duplex)
 				val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
@@ -2554,12 +2602,24 @@ static void mvneta_adjust_link(struct net_device *ndev)
 
 	if (status_change) {
 		if (phydev->link) {
-			u32 val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
-			val |= (MVNETA_GMAC_FORCE_LINK_PASS |
-				MVNETA_GMAC_FORCE_LINK_DOWN);
-			mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG, val);
+			if (!pp->use_inband_status) {
+				u32 val = mvreg_read(pp,
+						  MVNETA_GMAC_AUTONEG_CONFIG);
+				val &= ~MVNETA_GMAC_FORCE_LINK_DOWN;
+				val |= MVNETA_GMAC_FORCE_LINK_PASS;
+				mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG,
+					    val);
+			}
 			mvneta_port_up(pp);
 		} else {
+			if (!pp->use_inband_status) {
+				u32 val = mvreg_read(pp,
+						  MVNETA_GMAC_AUTONEG_CONFIG);
+				val &= ~MVNETA_GMAC_FORCE_LINK_PASS;
+				val |= MVNETA_GMAC_FORCE_LINK_DOWN;
+				mvreg_write(pp, MVNETA_GMAC_AUTONEG_CONFIG,
+					    val);
+			}
 			mvneta_port_down(pp);
 		}
 		phy_print_status(phydev);
@@ -2576,6 +2636,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 		netdev_err(pp->dev, "could not find the PHY\n");
 		return -ENODEV;
 	}
+	fixed_phy_set_link_update(phy_dev, mvneta_fixed_link_update);
 
 	phy_dev->supported &= PHY_GBIT_FEATURES;
 	phy_dev->advertising = phy_dev->supported;
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 
 static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
+	fixed_phy_set_link_update(pp->phy_dev, NULL);
 	phy_disconnect(pp->phy_dev);
 	pp->phy_dev = NULL;
 }
@@ -2910,6 +2972,9 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)
 		return -EINVAL;
 	}
 
+	if (pp->use_inband_status)
+		ctrl |= MVNETA_GMAC2_INBAND_AN_ENABLE;
+
 	/* Cancel Port Reset */
 	ctrl &= ~MVNETA_GMAC2_PORT_RESET;
 	mvreg_write(pp, MVNETA_GMAC_CTRL_2, ctrl);
@@ -2934,6 +2999,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	char hw_mac_addr[ETH_ALEN];
 	const char *mac_from;
 	int phy_mode;
+	int fixed_phy = 0;
 	int err;
 
 	/* Our multiqueue support is not complete, so for now, only
@@ -2967,6 +3033,7 @@ static int mvneta_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "cannot register fixed PHY\n");
 			goto err_free_irq;
 		}
+		fixed_phy = 1;
 
 		/* In the case of a fixed PHY, the DT node associated
 		 * to the PHY is the Ethernet MAC DT node.
@@ -2990,6 +3057,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	pp = netdev_priv(dev);
 	pp->phy_node = phy_node;
 	pp->phy_interface = phy_mode;
+	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && fixed_phy;
 
 	pp->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pp->clk)) {
Signature · 1 line
1.7.9.5
maintainer
#1 lore raw

From: Stas Sergeev <[email protected]>
Date: Tue, 31 Mar 2015 16:24:59 +0300

drivers/net/ethernet/marvell/mvneta.c static int mvneta_mdio_probe(struct mvneta_port *pp) · line 2654

Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1 -0
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 
 static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
+	fixed_phy_set_link_update(pp->phy_dev, NULL);

I do not see any other driver doing this on shutdown. Please show me why it is necessary.

And if it is, all other drivers registering a fixed phy link update function need to be adjusted to do the same thing.

#2 lore raw
Quoted 11 lines · David Miller

From: Stas Sergeev <[email protected]>
Date: Tue, 31 Mar 2015 16:24:59 +0300

drivers/net/ethernet/marvell/mvneta.c static int mvneta_mdio_probe(struct mvneta_port *pp) · line 2654

Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1 -0
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 
 static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
+	fixed_phy_set_link_update(pp->phy_dev, NULL);

I do not see any other driver doing this on shutdown. Please show me why it is necessary.

The primary reason is that if you do not do that, past the point where you call phy_disconnect(), we stop the PHY state machine, detach from the net_device, such that it won't invoke the adjust_link callback anymore. The fixed PHY driver, though will still keep calling the fixed_link_update callback asking the driver whether the link parameters need to be updated, and that will just cause a NULL pointer de-reference phydev->attached_dev, since we are now in detached state.

I guess another way to fix that is to look for the PHY state in fixed_mdio_read() and do nothing if it is PHY_HALTED.

Quoted 4 lines

And if it is, all other drivers registering a fixed phy link update function need to be adjusted to do the same thing.

I think the bcmgenet driver is now doing this as a result of Petri's latest changes, and I meant to comment on that before the patch got in. drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke phy_disconnect() nor can be rmmod'd, so a lesser issue.

Signature · 1 line
Florian
maintainer
#3 lore raw

From: Florian Fainelli <[email protected]>
Date: Thu, 02 Apr 2015 18:03:53 -0700

Quoted 21 lines · David Miller
Quoted 11 lines

From: Stas Sergeev <[email protected]>
Date: Tue, 31 Mar 2015 16:24:59 +0300

drivers/net/ethernet/marvell/mvneta.c static int mvneta_mdio_probe(struct mvneta_port *pp) · line 2654

Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1 -0
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 
 static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
+	fixed_phy_set_link_update(pp->phy_dev, NULL);

I do not see any other driver doing this on shutdown. Please show me why it is necessary.

The primary reason is that if you do not do that, past the point where you call phy_disconnect(), we stop the PHY state machine, detach from the net_device, such that it won't invoke the adjust_link callback anymore. The fixed PHY driver, though will still keep calling the fixed_link_update callback asking the driver whether the link parameters need to be updated, and that will just cause a NULL pointer de-reference phydev->attached_dev, since we are now in detached state.

Quoted 2 lines

I guess another way to fix that is to look for the PHY state in fixed_mdio_read() and do nothing if it is PHY_HALTED.

Ok I'll apply this mvneta patch to net-next then, thanks.

Quoted 7 lines
Quoted 2 lines

And if it is, all other drivers registering a fixed phy link update function need to be adjusted to do the same thing.

I think the bcmgenet driver is now doing this as a result of Petri's latest changes, and I meant to comment on that before the patch got in. drivers/net/dsa/bcm_sf2.c has a similar construct but does not invoke phy_disconnect() nor can be rmmod'd, so a lesser issue.

I just seems insane to me that phy_disconnect() doesn't stop the callbacks from running.

Fixed PHY seems to me to suffer from a lack of proper integration into the PHY layer.

maintainer
#4 lore raw

From: David Miller <[email protected]>
Date: Fri, 03 Apr 2015 15:04:10 -0400 (EDT)

Quoted 1 line

Ok I'll apply this mvneta patch to net-next then, thanks.

Nevermind I just saw in my patchwork queue that there is a v3 of this change set that adds some adjustments to the fixed PHY interfaces.

I think I'll apply that series instead of this patch.

#5 lore raw
In reply to David Miller

03.04.2015 03:51, David Miller пишет:

Quoted 11 lines

From: Stas Sergeev <[email protected]>
Date: Tue, 31 Mar 2015 16:24:59 +0300

drivers/net/ethernet/marvell/mvneta.c static int mvneta_mdio_probe(struct mvneta_port *pp) · line 2654

Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1 -0
@@ -2590,6 +2651,7 @@ static int mvneta_mdio_probe(struct mvneta_port *pp)
 
 static void mvneta_mdio_remove(struct mvneta_port *pp)
 {
+	fixed_phy_set_link_update(pp->phy_dev, NULL);

I do not see any other driver doing this on shutdown. Please show me why it is necessary.

Hello David, sorry for this being discussed in a different thread, so here you have a few pointers to why is this needed:
https://lkml.org/lkml/2015/3/30/367

Quoted 2 lines

And if it is, all other drivers registering a fixed phy link update function need to be adjusted to do the same thing.

Or, for example, get rid of the callback and add an API:
http://www.spinics.net/lists/netdev/msg323517.html

Then you get the patch that avoids all this call-back singing and dancing:
http://www.spinics.net/lists/netdev/msg323518.html

In any case, I posted 3 different implementations and hope people will choose the best one (any choice is fine with me).