|SUCCESS| [v1] net/axgbe: fix PHY auto-negotiation advertisement

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162793

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-01

Commit: 7ba7d89890ab ("net/axgbe: support Marvell M88E1512 PHY")
Author: Ashok Kumar Natarajan <ashokkumar.natarajan at amd.com>
Subject: net/axgbe: fix PHY auto-negotiation advertisement

This patch corrects the programming of PHY auto-negotiation advertisement
registers by configuring Clause 22 PHY registers according to IEEE 802.3.

> diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
> index e5568cce5f..12b1d47244 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -156,6 +156,11 @@
>  #define AXGBE_M88E1512_MODE_RGMII_SGMII		4
>  #define AXGBE_M88E1512_MODE_SW_RESET		0x8000
>  
> +#define AXGBE_PHY_MII_CTRL1000_1000T_HALF   0x0100
> +#define AXGBE_PHY_MII_CTRL1000_1000T_FULL   0x0200
> +#define AXGBE_PHY_MII_CTRL1000_MS_VALUE     0x0800
> +#define AXGBE_PHY_MII_CTRL1000_MS_MANUAL    0x1000

Are these new macro definitions necessary for this fix?  Looking at the
code changes, only AXGBE_PHY_MII_CTRL1000_1000T_FULL is actually used in
axgbe_phy_config_advert().  The other three macros
(AXGBE_PHY_MII_CTRL1000_1000T_HALF, AXGBE_PHY_MII_CTRL1000_MS_VALUE,
AXGBE_PHY_MII_CTRL1000_MS_MANUAL) are defined but never referenced.

> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
> index 369d766884..b340ce02dd 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
> @@ -303,21 +303,38 @@ static int axgbe_phy_write(struct axgbe_port *pdata, u16 reg, u16 value)
>  
>  static int axgbe_phy_config_advert(struct axgbe_port *pdata)
>  {
> +	u32 adv = pdata->phy.advertising;
>  	u16 advert;
> +	u16 ctrl1000;
>  	int ret;
>  
> -	ret = pdata->phy_if.phy_impl.read(pdata, MII_ADVERTISE, &advert);
> +	advert = ADVERTISE_CSMA;
> +
> +	if (adv & ADVERTISED_10baseT_Full)
> +		advert |= ADVERTISE_10FULL;
> +
> +	if (adv & ADVERTISED_100baseT_Full)
> +		advert |= ADVERTISE_100FULL;
> +
> +	if (adv & ADVERTISED_Pause)
> +		advert |= ADVERTISE_PAUSE_CAP;
> +
> +	if (adv & ADVERTISED_Asym_Pause)
> +		advert |= ADVERTISE_PAUSE_ASYM;
> +
> +	ret = pdata->phy_if.phy_impl.write(pdata, MII_ADVERTISE, advert);

Does this code fail to advertise 10BASE-T half-duplex and 100BASE-TX
half-duplex capabilities?  The logic only checks for ADVERTISED_10baseT_Full
and ADVERTISED_100baseT_Full, but does not check for the half-duplex
variants.  If pdata->phy.advertising contains ADVERTISED_10baseT_Half or
ADVERTISED_100baseT_Half, they would not be written to the ADVERTISE
register.

>  	if (ret) {
> -		PMD_DRV_LOG_LINE(ERR, "Failed to read ADVERTISE register");
> +		PMD_DRV_LOG_LINE(ERR, "Failed to write ADVERTISE register");
>  		return ret;
>  	}
>  
> -	advert |= ADVERTISE_FULL;
> -	advert |= ADVERTISE_PAUSE_CAP;
> +	ctrl1000 = 0;
> +	if (adv & ADVERTISED_1000baseT_Full)
> +		ctrl1000 |= AXGBE_PHY_MII_CTRL1000_1000T_FULL;
>  
> -	ret = pdata->phy_if.phy_impl.write(pdata, MII_ADVERTISE, advert);
> +	ret = pdata->phy_if.phy_impl.write(pdata, MII_CTRL1000, ctrl1000);

Similarly, does this code fail to advertise 1000BASE-T half-duplex
capability?  If pdata->phy.advertising contains ADVERTISED_1000baseT_Half,
it would not be written to MII_CTRL1000.

>  	if (ret) {
> -		PMD_DRV_LOG_LINE(ERR, "Failed to write ADVERTISE register");
> +		PMD_DRV_LOG_LINE(ERR, "Failed to write MII_CTRL1000 register");
>  		return ret;
>  	}
>  	return 0;