Skip to contentSkip to conversation

mvneta: SGMII-based in-band link state signaling

PATCH In review Stas Sergeev 3 participants 6 messages Apr 1, 2015 – Apr 3, 2015

Subsystems (4)
Networking Network drivers Ethernet PHY Marvell mvneta Ethernet driver
lore
Share & tools
lore

Stas Sergeev

author
#0 lore raw

v3 cover mvneta: SGMII-based in-band link state signaling

Hello.

Currently the fixed-link DT binding is pre-configured and cannot be changed in run-time. This means the cable unplug events are not being detected, and the link parameters can't be negotiated.

The following patches are needed when mvneta is used in fixed-link mode (without MDIO).
They add an API to fixed_phy that allows to update status, and use that API in the mvneta driver when parsing the SGMII in-band status.

There is also another implementation that doesn't add any API and does everything in mvneta driver locally:
https://lkml.org/lkml/2015/3/31/327
I'll let people decide which approach is better.
No strong opinion on my side.

Signed-off-by Stas Sergeev <[email protected]>
author
#1 lore raw

PATCH v3 1/2 add fixed_phy_update_state() - update state of fixed_phy

Currently fixed_phy uses a callback to periodically poll the link state. This patch adds the fixed_phy_update_state() API.
It solves the following problems:
- On link state interrupt, MAC driver can't update status. Instead it needs to provide the callback to periodically query the HW about the link state. It is more efficient to update status after interrupt.
- The callback needs to be unregistered before phy_disconnect(), or otherwise it will be called with net_dev==NULL. phy_disconnect() does not have enough info to unregister the callback automatically.
- The callback needs to be registered before of_phy_connect() to avoid running with outdated state, but of_phy_connect() returns the phy_device pointer, which is needed to register the callback. Registering it before of_phy_connect() will therefore require a hack to get the pointer earlier.

Overall, this addition makes the subsequent patch that implements SGMII link status for mvneta, much cleaner.

CC Florian Fainelli <[email protected]>
CC [email protected]
CC [email protected]
Signed-off-by Stas Sergeev <[email protected]>
---
 drivers/net/phy/fixed_phy.c |   29 +++++++++++++++++++++++++++++
 include/linux/phy_fixed.h   |    9 +++++++++
 2 files changed, 38 insertions(+)
drivers/net/phy/fixed_phy.c +29 -0
index a08a3c7..1960b46 100644
@@ -183,6 +183,35 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(fixed_phy_set_link_update);
 
+int fixed_phy_update_state(struct phy_device *phydev,
+			   const struct fixed_phy_status *status,
+			   const struct fixed_phy_status *changed)
+{
+	struct fixed_mdio_bus *fmb = &platform_fmb;
+	struct fixed_phy *fp;
+
+	if (!phydev || !phydev->bus)
+		return -EINVAL;
+
+	list_for_each_entry(fp, &fmb->phys, node) {
+		if (fp->addr == phydev->addr) {
+#define _UPD(x) if (changed->x) \
+	fp->status.x = status->x
+			_UPD(link);
+			_UPD(speed);
+			_UPD(duplex);
+			_UPD(pause);
+			_UPD(asym_pause);
+#undef _UPD
+			fixed_phy_update_regs(fp);
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL(fixed_phy_update_state);
+
 int fixed_phy_add(unsigned int irq, int phy_addr,
 		  struct fixed_phy_status *status)
 {
include/linux/phy_fixed.h +9 -0
index 7e75bfe..fe5732d 100644
@@ -21,6 +21,9 @@ extern void fixed_phy_del(int phy_addr);
 extern int fixed_phy_set_link_update(struct phy_device *phydev,
 			int (*link_update)(struct net_device *,
 					   struct fixed_phy_status *));
+extern int fixed_phy_update_state(struct phy_device *phydev,
+			   const struct fixed_phy_status *status,
+			   const struct fixed_phy_status *changed);
 #else
 static inline int fixed_phy_add(unsigned int irq, int phy_id,
 				struct fixed_phy_status *status)

@@ -43,6 +46,12 @@ static inline int fixed_phy_set_link_update(struct phy_device *phydev,
 {
 	return -ENODEV;
 }
+static inline int fixed_phy_update_state(struct phy_device *phydev,
+			   const struct fixed_phy_status *status,
+			   const struct fixed_phy_status *changed)
+{
+	return -ENODEV;
+}
 #endif /* CONFIG_FIXED_PHY */
 
 #endif /* __PHY_FIXED_H */
Signature · 1 line
1.7.9.5
#2 lore raw

Re: [PATCH 1/2] add fixed_phy_update_state() - update state of fixed_phy

Quoted 17 lines · Stas Sergeev

Currently fixed_phy uses a callback to periodically poll the link state. This patch adds the fixed_phy_update_state() API.
It solves the following problems:
- On link state interrupt, MAC driver can't update status. Instead it needs to provide the callback to periodically query the HW about the link state. It is more efficient to update status after interrupt.
- The callback needs to be unregistered before phy_disconnect(), or otherwise it will be called with net_dev==NULL. phy_disconnect() does not have enough info to unregister the callback automatically.
- The callback needs to be registered before of_phy_connect() to avoid running with outdated state, but of_phy_connect() returns the phy_device pointer, which is needed to register the callback. Registering it before of_phy_connect() will therefore require a hack to get the pointer earlier.

Quoted 2 lines

Overall, this addition makes the subsequent patch that implements SGMII link status for mvneta, much cleaner.

Agreed, now that we have that, we should probably just remove the ability to have a fixed link update callback since it suffers from all the deficiencies you outlined above, and is creating some overhead by polling the hardware.

Thanks!

Signature · 1 line
Florian
author
#3 lore raw

Re: [PATCH 1/2] add fixed_phy_update_state() - update state of fixed_phy

03.04.2015 22:25, Florian Fainelli пишет:

Quoted 6 lines · Stas Sergeev
Quoted 5 lines

- The callback needs to be registered before of_phy_connect() to avoid running with outdated state, but of_phy_connect() returns the phy_device pointer, which is needed to register the callback. Registering it before of_phy_connect() will therefore require a hack to get the pointer earlier.

In fact, this can't even be done: registering it before of_phy_connect() is an asking for troubles with NULL deref.

Quoted 6 lines
Quoted 2 lines

Overall, this addition makes the subsequent patch that implements SGMII link status for mvneta, much cleaner.

Agreed, now that we have that, we should probably just remove the ability to have a fixed link update callback since it suffers from all the deficiencies you outlined above, and is creating some overhead by polling the hardware.

In fact, there is still a bit of a problem.
You certainly want to set the initial state, so that on of_phy_connect() time it is already valid. For this I still use of_phy_find_device()... This, however, differs from registering the callback:
- initial state is set only once; callback should be registered before _every_ of_phy_connect() (but it can't be registered before because of the NULL, so only after)
- initial state can be set anywhere anytime, not necessary any near of_phy_connect().

So the problem is mostly solved, but of_phy_find_device() is still there. To get rid of this, I guess some addition to of_mdio may be needed as well. Maybe optionally passing the status directly to of_phy_connect()? Not sure.

author
#4 lore raw

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

In reply to Stas Sergeev

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 link state changes, namely:
- link up/down
- link speed
- duplex full/half
fixed_phy_update_state() is used to update phy status.

CC Thomas Petazzoni <[email protected]>
CC Florian Fainelli <[email protected]>
CC [email protected]
CC [email protected]
Signed-off-by Stas Sergeev <[email protected]>
---
 drivers/net/ethernet/marvell/mvneta.c |  106 +++++++++++++++++++++++++++++----
 1 file changed, 95 insertions(+), 11 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +95 -11
index 96208f1..a7a9271 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,28 @@ static irqreturn_t mvneta_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int mvneta_fixed_link_update(struct mvneta_port *pp,
+				    struct phy_device *phy)
+{
+	struct fixed_phy_status status;
+	struct fixed_phy_status changed = {};
+	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);
+	changed.link = 1;
+	changed.speed = 1;
+	changed.duplex = 1;
+	fixed_phy_update_state(phy, &status, &changed);
+	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 +2106,18 @@ 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) {
+		u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
+
+		mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
+		if (pp->use_inband_status && (cause_misc &
+				(MVNETA_CAUSE_PHY_STATUS_CHANGE |
+				 MVNETA_CAUSE_LINK_CHANGE |
+				 MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
+			mvneta_fixed_link_update(pp, pp->phy_dev);
+		}
+	}
 
 	/* Release Tx descriptors */
 	if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {

@@ -2109,7 +2162,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 +2428,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 +2584,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 +2613,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);

@@ -2910,6 +2981,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 +3008,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 +3042,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 +3066,8 @@ 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)) {

@@ -3067,6 +3145,12 @@ static int mvneta_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pp->dev);
 
+	if (pp->use_inband_status) {
+		struct phy_device *phy = of_phy_find_device(dn);
+
+		mvneta_fixed_link_update(pp, phy);
+	}
+
 	return 0;
 
 err_free_stats:
Signature · 1 line
1.7.9.5
maintainer
#5 lore raw
In reply to Stas Sergeev

From: Stas Sergeev <[email protected]>
Date: Wed, 01 Apr 2015 20:28:25 +0300

Quoted 11 lines

Currently the fixed-link DT binding is pre-configured and cannot be changed in run-time. This means the cable unplug events are not being detected, and the link parameters can't be negotiated.

The following patches are needed when mvneta is used in fixed-link mode (without MDIO).
They add an API to fixed_phy that allows to update status, and use that API in the mvneta driver when parsing the SGMII in-band status.

Quoted 7 lines

There is also another implementation that doesn't add any API and does everything in mvneta driver locally:
https://lkml.org/lkml/2015/3/31/327
I'll let people decide which approach is better.
No strong opinion on my side.

Signed-off-by Stas Sergeev <[email protected]>

Series applied, thanks.