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

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 <redacted>

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