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@xxxxxxxxx>

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