[PATCH net] net: stmmac: enable the MAC on link up at 10G on 10GBASE-R

[PATCH net] net: stmmac: enable the MAC on link up at 10G on 10GBASE-R

[vadik likholetov]

stmmac_mac_link_down() clears the MAC's transmit and receive enable bits.
stmmac_mac_link_up() is expected to set them again through
stmmac_mac_set(..., true), but it first switches on the negotiated speed
and returns early for a speed it does not recognise.

The generic branch of that switch -- taken for every interface that is
neither USXGMII nor XLGMII, which includes PHY_INTERFACE_MODE_10GBASER --
handles only SPEED_2500, SPEED_1000, SPEED_100 and SPEED_10.

MGBE on Tegra234 runs 10GBASE-R into an Aquantia AQR113C. That PHY does
rate matching, so phylink_link_up() replaces the media speed with the
MAC-side interface speed before calling into the MAC:

	case RATE_MATCH_PAUSE:
		speed = phylink_interface_max_speed(link_state.interface);
		duplex = DUPLEX_FULL;

The driver is therefore called as

	stmmac_mac_link_up(interface=10GBASER, speed=10000, duplex=1)

which falls through to "default: return;". The MAC is never re-enabled,
and the interface stops passing traffic after the first link flap.

The failure is easy to misread. The link still comes up, because the PHY
is polled over MDIO and needs no MAC, so the interface reports carrier 1
at the media speed. The DMA is untouched, so its start bits stay set and
descriptors are still consumed. Only the MAC itself is gated off: the
receiver counts nothing (mmc_rx_framecount_gb stops advancing, RE is 0)
and nothing reaches the wire (TE is 0). The interface survives boot only
because stmmac_hw_setup(), called from ndo_open, enables the MAC
unconditionally -- so the problem appears only once the cable has been
unplugged and plugged back in, and "ip link set dev <ethX> down && ip
link set dev <ethX> up" appears to fix it.

Handle SPEED_10000 in the generic branch, as the USXGMII and XLGMII
branches already do. For dwxgmac2, link.xgmii.speed10000 is
XGMAC_CONFIG_SS_10000, which is 0 and is the correct speed selection for
a 10GBASE-R MAC: ctrl then equals old_ctrl, the register write is
skipped, and execution reaches stmmac_mac_set(..., true).

Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
Signed-off-by: vadik likholetov <vadikas@gmail.com>
---
Verified on an AGX Orin devkit (Tegra234 MGBE0 + AQR113C), before and
after, on the same board and cable. MAC registers read with
`ethtool -d`, after a physical unplug and replug:

  stock		MAC_TX_CONFIG 0x00010000 (TE=0)
		MAC_RX_CONFIG 0x3ff022c0 (RE=0)
		rx_packets frozen, DHCP lease lost

  patched	MAC_TX_CONFIG 0x00010001 (TE=1)
		MAC_RX_CONFIG 0x3ff022c1 (RE=1)
		rx_packets keeps climbing, DHCP lease retained

A kprobe trace of the stock kernel across a link flap shows the
mechanism directly -- dwxgmac2_set_mac(enable=1) never follows the
link-up:

  linkdown: (stmmac_mac_link_down+0x0/0xc8) iface=27
  setmac:   (dwxgmac2_set_mac+0x0/0x80) enable=0
  linkup:   (stmmac_mac_link_up+0x0/0x350) iface=27 speed=10000 duplex=1

(iface 27 is PHY_INTERFACE_MODE_10GBASER.)

Note on the Fixes tag: the missing SPEED_10000 case in the generic branch
predates the commit cited above. I picked d8ca113724e7 because MGBE is
the first in-tree user to reach it -- it needs a 10GBASE-R interface
driven by a rate-matching PHY, so that phylink hands the MAC a 10G speed
on a link that negotiated slower. Happy to re-target the tag if you would
rather it point at the stmmac commit that introduced the switch.

Testing was done with MGBE0 handed to a VM via vfio-platform, so the
driver ran in a guest; the MAC register evidence above is read from the
device itself and the code path is not virtualisation-specific. I do not
have a bare-metal AGX Orin running mainline dwmac-tegra to confirm on,
but nothing in the analysis depends on the passthrough.

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1125,6 +1125,9 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 		}
 	} else {
 		switch (speed) {
+		case SPEED_10000:
+			ctrl |= priv->hw->link.xgmii.speed10000;
+			break;
 		case SPEED_2500:
 			ctrl |= priv->hw->link.speed2500;
 			break;
--
2.51.0

Re: [PATCH net] net: stmmac: enable the MAC on link up at 10G on 10GBASE-R

[Maxime Chevallier]

Hi,

On 7/10/26 14:09, vadik likholetov wrote:
> stmmac_mac_link_down() clears the MAC's transmit and receive enable bits.
> stmmac_mac_link_up() is expected to set them again through
> stmmac_mac_set(..., true), but it first switches on the negotiated speed
> and returns early for a speed it does not recognise.
> 
> The generic branch of that switch -- taken for every interface that is
> neither USXGMII nor XLGMII, which includes PHY_INTERFACE_MODE_10GBASER --
> handles only SPEED_2500, SPEED_1000, SPEED_100 and SPEED_10.
> 
> MGBE on Tegra234 runs 10GBASE-R into an Aquantia AQR113C. That PHY does
> rate matching, so phylink_link_up() replaces the media speed with the
> MAC-side interface speed before calling into the MAC:
> 
> 	case RATE_MATCH_PAUSE:
> 		speed = phylink_interface_max_speed(link_state.interface);
> 		duplex = DUPLEX_FULL;
> 
> The driver is therefore called as
> 
> 	stmmac_mac_link_up(interface=10GBASER, speed=10000, duplex=1)
> 
> which falls through to "default: return;". The MAC is never re-enabled,
> and the interface stops passing traffic after the first link flap.
> 
> The failure is easy to misread. The link still comes up, because the PHY
> is polled over MDIO and needs no MAC, so the interface reports carrier 1
> at the media speed. The DMA is untouched, so its start bits stay set and
> descriptors are still consumed. Only the MAC itself is gated off: the
> receiver counts nothing (mmc_rx_framecount_gb stops advancing, RE is 0)
> and nothing reaches the wire (TE is 0). The interface survives boot only
> because stmmac_hw_setup(), called from ndo_open, enables the MAC
> unconditionally -- so the problem appears only once the cable has been
> unplugged and plugged back in, and "ip link set dev <ethX> down && ip
> link set dev <ethX> up" appears to fix it.
> 
> Handle SPEED_10000 in the generic branch, as the USXGMII and XLGMII
> branches already do. For dwxgmac2, link.xgmii.speed10000 is
> XGMAC_CONFIG_SS_10000, which is 0 and is the correct speed selection for
> a 10GBASE-R MAC: ctrl then equals old_ctrl, the register write is
> skipped, and execution reaches stmmac_mac_set(..., true).
> 
> Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
> Signed-off-by: vadik likholetov <vadikas@gmail.com>

Looking at this, it seems the only important thing in that speed selection
logic is how 2.5G is handled : 

 - either through the XGMII block when using USXGMII

   ctrl |= priv->hw->link.xgmii.speed2500;

 - or through the regular speed selection bits :

   ctrl |= priv->hw->link.speed2500;

The rest is speed validation, and phylink should already be doing that for us.

I suggest that instead of this fix (that is ommiting the 5G case btw), we rewrite
the whole ctrl assignment as :

	switch (speed) {
	case SPEED_100000:
		ctrl |= priv->hw->link.xlgmii.speed100000;
		break;
	case SPEED_50000:
		ctrl |= priv->hw->link.xlgmii.speed50000;
		break;
	case SPEED_40000:
		ctrl |= priv->hw->link.xlgmii.speed40000;
		break;
	case SPEED_25000:
		ctrl |= priv->hw->link.xlgmii.speed25000;
		break;
	case SPEED_10000:
		ctrl |= priv->hw->link.xgmii.speed10000;
		break;
	case SPEED_5000:
		ctrl |= priv->hw->link.xgmii.speed5000;
		break;
	case SPEED_2500:
		if (interface == PHY_INTERFACE_MODE_USXGMII)
			ctrl |= priv->hw->link.xgmii.speed2500;
		else
			ctrl |= priv->hw->link.speed2500;
		break;
	case SPEED_1000:
		ctrl |= priv->hw->link.speed1000;
	case SPEED_100:
		ctrl |= priv->hw->link.speed100;
		break;
	case SPEED_10:
		ctrl |= priv->hw->link.speed10;
		break;
	default:
		return;
	}

(I haven't tested that, I don't have any stmmac boards here that can do more
than 1Gbps :( )

Thanks,

Maxime

Re: [PATCH net] net: stmmac: enable the MAC on link up at 10G on 10GBASE-R

[Andrew Lunn]

> I suggest that instead of this fix (that is ommiting the 5G case btw), we rewrite
> the whole ctrl assignment as :
> 
> 	switch (speed) {
> 	case SPEED_100000:
> 		ctrl |= priv->hw->link.xlgmii.speed100000;
> 		break;
> 	case SPEED_50000:
> 		ctrl |= priv->hw->link.xlgmii.speed50000;
> 		break;
> 	case SPEED_40000:
> 		ctrl |= priv->hw->link.xlgmii.speed40000;
> 		break;
> 	case SPEED_25000:
> 		ctrl |= priv->hw->link.xlgmii.speed25000;
> 		break;
> 	case SPEED_10000:
> 		ctrl |= priv->hw->link.xgmii.speed10000;
> 		break;
> 	case SPEED_5000:
> 		ctrl |= priv->hw->link.xgmii.speed5000;
> 		break;
> 	case SPEED_2500:
> 		if (interface == PHY_INTERFACE_MODE_USXGMII)
> 			ctrl |= priv->hw->link.xgmii.speed2500;
> 		else
> 			ctrl |= priv->hw->link.speed2500;
> 		break;
> 	case SPEED_1000:
> 		ctrl |= priv->hw->link.speed1000;
> 	case SPEED_100:
> 		ctrl |= priv->hw->link.speed100;
> 		break;
> 	case SPEED_10:
> 		ctrl |= priv->hw->link.speed10;
> 		break;
> 	default:
> 		return;
> 	}

The bug description makes it sound like it was not obvious to
debug. So maybe put an error message in the default: to aid the next
developer who hits it with SPEED_14000, or whatever comes next.

	  Andrew

[PATCH net v2] net: stmmac: enable the MAC on link up for all supported speeds

[vadik likholetov]

stmmac_mac_link_down() clears the MAC's transmit and receive enable bits.
stmmac_mac_link_up() is expected to set them again through
stmmac_mac_set(..., true), but it first switches on the negotiated speed
and returns early for a speed the switch does not list. The MAC is then
left gated off.

The speed selection is split into three switches, keyed on the interface.
The generic branch -- taken for everything that is neither USXGMII nor
XLGMII, so including PHY_INTERFACE_MODE_10GBASER -- lists only SPEED_2500,
SPEED_1000, SPEED_100 and SPEED_10.

MGBE on Tegra234 runs 10GBASE-R into an Aquantia AQR113C. That PHY does
rate matching, so phylink_link_up() replaces the media speed with the
MAC-side interface speed before calling into the MAC:

	case RATE_MATCH_PAUSE:
		speed = phylink_interface_max_speed(link_state.interface);
		duplex = DUPLEX_FULL;

The driver is therefore called as

	stmmac_mac_link_up(interface=10GBASER, speed=10000, duplex=1)

which falls through to "default: return;". The interface stops passing
traffic after the first link flap.

The failure is easy to misread. The link still comes up, because the PHY
is polled over MDIO and needs no MAC, so the interface reports carrier 1
at the media speed. The DMA is untouched, so its start bits stay set and
descriptors are still consumed. Only the MAC itself is gated off: the
receiver counts nothing (mmc_rx_framecount_gb stops advancing, RE is 0)
and nothing reaches the wire (TE is 0). The interface survives boot only
because stmmac_hw_setup(), called from ndo_open, enables the MAC
unconditionally -- so the problem appears only once the cable has been
unplugged and plugged back in, and "ip link set dev <ethX> down && ip
link set dev <ethX> up" appears to fix it.

The interface is not what the speed bits depend on: with the single
exception of 2.5G, which is selected through the XGMII block on USXGMII
and through the regular speed bits otherwise, each speed maps to one
field of struct mac_link. The per-interface switches are speed
validation, and phylink already validates the speed against
priv->hw->link.caps. So collapse the three switches into one keyed on the
speed alone, keeping the interface test only for the 2.5G case. This
covers 10G on 10GBASE-R, and equally 5G, and 1G/100/10 on USXGMII, all of
which hit "default: return;" today.

A core that does not support a speed leaves the corresponding mac_link
field at 0, and phylink will not offer it that speed in the first place.
For dwxgmac2 at 10G, link.xgmii.speed10000 is XGMAC_CONFIG_SS_10000,
which is 0 and is the correct speed selection for a 10GBASE-R MAC: ctrl
then equals old_ctrl, the register write is skipped, and execution
reaches stmmac_mac_set(..., true).

Log an error in the default case, since a speed with no entry here leaves
the MAC disabled and the symptom does not point at the cause.

Fixes: d8ca113724e7 ("net: stmmac: tegra: Add MGBE support")
Suggested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: vadik likholetov <vadikas@gmail.com>
---
v2:
 - Collapse the three per-interface switches into a single switch on the
   speed, rather than adding SPEED_10000 to the generic branch, which
   left SPEED_5000 and the USXGMII sub-1G speeds broken (Maxime).
 - netdev_err() in the default case (Andrew).

Fixes tag: the missing speeds predate the commit cited above. I picked
d8ca113724e7 because MGBE is the first in-tree user to reach it -- it
needs a 10GBASE-R interface driven by a rate-matching PHY, so that
phylink hands the MAC a 10G speed. Happy to re-target it.

Verified on an AGX Orin devkit (Tegra234 MGBE0 + AQR113C), before and
after, on the same board and cable. MAC registers read with `ethtool -d`,
after a physical unplug and replug:

  stock		MAC_TX_CONFIG 0x00010000 (TE=0)
		MAC_RX_CONFIG 0x3ff022c0 (RE=0)
		rx_packets frozen, DHCP lease lost

  patched	MAC_TX_CONFIG 0x00010001 (TE=1)
		MAC_RX_CONFIG 0x3ff022c1 (RE=1)
		rx_packets keeps climbing, DHCP lease retained

Only the 10GBASE-R path is covered by hardware here; the other speeds are
by inspection. Testing was done with MGBE0 handed to a VM via
vfio-platform, so the driver ran in a guest; the MAC register evidence
above is read from the device itself and the code path is not
virtualisation-specific.

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 92 ++++++++-----------
 1 file changed, 37 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2a0d7eff8..fa9f88e51 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1083,63 +1083,45 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 	old_ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
 	ctrl = old_ctrl & ~priv->hw->link.speed_mask;
 
-	if (interface == PHY_INTERFACE_MODE_USXGMII) {
-		switch (speed) {
-		case SPEED_10000:
-			ctrl |= priv->hw->link.xgmii.speed10000;
-			break;
-		case SPEED_5000:
-			ctrl |= priv->hw->link.xgmii.speed5000;
-			break;
-		case SPEED_2500:
+	switch (speed) {
+	case SPEED_100000:
+		ctrl |= priv->hw->link.xlgmii.speed100000;
+		break;
+	case SPEED_50000:
+		ctrl |= priv->hw->link.xlgmii.speed50000;
+		break;
+	case SPEED_40000:
+		ctrl |= priv->hw->link.xlgmii.speed40000;
+		break;
+	case SPEED_25000:
+		ctrl |= priv->hw->link.xlgmii.speed25000;
+		break;
+	case SPEED_10000:
+		ctrl |= priv->hw->link.xgmii.speed10000;
+		break;
+	case SPEED_5000:
+		ctrl |= priv->hw->link.xgmii.speed5000;
+		break;
+	case SPEED_2500:
+		if (interface == PHY_INTERFACE_MODE_USXGMII)
 			ctrl |= priv->hw->link.xgmii.speed2500;
-			break;
-		default:
-			return;
-		}
-	} else if (interface == PHY_INTERFACE_MODE_XLGMII) {
-		switch (speed) {
-		case SPEED_100000:
-			ctrl |= priv->hw->link.xlgmii.speed100000;
-			break;
-		case SPEED_50000:
-			ctrl |= priv->hw->link.xlgmii.speed50000;
-			break;
-		case SPEED_40000:
-			ctrl |= priv->hw->link.xlgmii.speed40000;
-			break;
-		case SPEED_25000:
-			ctrl |= priv->hw->link.xlgmii.speed25000;
-			break;
-		case SPEED_10000:
-			ctrl |= priv->hw->link.xgmii.speed10000;
-			break;
-		case SPEED_2500:
-			ctrl |= priv->hw->link.speed2500;
-			break;
-		case SPEED_1000:
-			ctrl |= priv->hw->link.speed1000;
-			break;
-		default:
-			return;
-		}
-	} else {
-		switch (speed) {
-		case SPEED_2500:
+		else
 			ctrl |= priv->hw->link.speed2500;
-			break;
-		case SPEED_1000:
-			ctrl |= priv->hw->link.speed1000;
-			break;
-		case SPEED_100:
-			ctrl |= priv->hw->link.speed100;
-			break;
-		case SPEED_10:
-			ctrl |= priv->hw->link.speed10;
-			break;
-		default:
-			return;
-		}
+		break;
+	case SPEED_1000:
+		ctrl |= priv->hw->link.speed1000;
+		break;
+	case SPEED_100:
+		ctrl |= priv->hw->link.speed100;
+		break;
+	case SPEED_10:
+		ctrl |= priv->hw->link.speed10;
+		break;
+	default:
+		netdev_err(priv->dev,
+			   "unsupported speed %d on %s, leaving the MAC disabled\n",
+			   speed, phy_modes(interface));
+		return;
 	}
 
 	if (priv->plat->fix_mac_speed)
-- 
2.53.0