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

[Maxime Chevallier]

Hi,

When sending a new iteration, it should be a standalone thread, and not
a reply to the previous version. Patchwork seems to have picked it up, so
I _think_ it's ok for this one though :)

On 7/12/26 08:55, 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 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@xxxxxxxxxxx>
> Signed-off-by: vadik likholetov <vadikas@xxxxxxxxx>
> ---
> 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));

You should probably be using phy_speed_to_str() to print the speed.

Thanks,

Maxime