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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Below is a plain text review of the patch:

---

commit [git sha from email headers]
Author: Ashok Kumar Natarajan <ashokkumar.natarajan at amd.com>

net/axgbe: fix PHY auto-negotiation advertisement

The patch modifies axgbe_phy_config_advert() to properly program PHY
auto-negotiation advertisement registers for 10/100/1000BASE-T operation
in full-duplex mode only. Half-duplex capabilities are intentionally not
advertised since CSMA/CD is not supported by the MAC.

> diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
> index e5568cce5f..ab9c692d47 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -156,6 +156,7 @@
>  #define AXGBE_M88E1512_MODE_RGMII_SGMII		4
>  #define AXGBE_M88E1512_MODE_SW_RESET		0x8000
>  
> +#define AXGBE_PHY_MII_CTRL1000_1000T_FULL	0x0200
>  
>  /* Control register 1. */
>  /* Enable extended speed selection */
> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
> index 369d766884..aa245c1b44 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
> @@ -303,21 +303,52 @@ 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);
> +	/*
> +	 * AXGBE supports 10BASE-T and 100BASE-TX in full-duplex mode only.
> +	 * Half-duplex operation (CSMA/CD) is not supported by the MAC,
> +	 * so 10/100 half-duplex capabilities are intentionally not advertised.
> +	 * Any ADVERTISED_10baseT_Half or ADVERTISED_100baseT_Half bits
> +	 * present in the software advertising mask are therefore ignored.
> +	 */
> +	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;

Looking at the previous code, it performed a read-modify-write on the
MII_ADVERTISE register, preserving any existing bits. Now the code
unconditionally starts with ADVERTISE_CSMA and builds up the register
value from scratch. Does this change break any existing configuration
that may have had other bits set in MII_ADVERTISE? Could there be
hardware or PHY-specific state that depended on those preserved bits?

> +
> +	ret = pdata->phy_if.phy_impl.write(pdata, MII_ADVERTISE, advert);
>  	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;
> +	/*
> +	 * AXGBE supports 1000BASE-T in full-duplex mode only.
> +	 * Half-duplex operation (CSMA/CD) is not supported by the MAC,
> +	 * so 1000BASE-T half-duplex is intentionally not advertised.
> +	 * Any ADVERTISED_1000baseT_Half bit present in the software
> +	 * advertising mask is therefore ignored here.
> +	 */
> +	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);
>  	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;

The old code never wrote to MII_CTRL1000 at all. This patch now
unconditionally writes to MII_CTRL1000 with either 0 or
AXGBE_PHY_MII_CTRL1000_1000T_FULL based on ADVERTISED_1000baseT_Full.
Does this change impact existing deployments where MII_CTRL1000 may have
been configured externally or by hardware defaults? Could writing 0 to
MII_CTRL1000 disable previously advertised 1000BASE-T capabilities?

Looking at the macro definition, AXGBE_PHY_MII_CTRL1000_1000T_FULL is
0x0200. IEEE 802.3 Clause 40 defines MII_CTRL1000 bit 9 (0x0200) as
the 1000BASE-T full-duplex capability advertisement. Does this macro
correctly match the IEEE specification?

The commit message states this fixes Clause 22 PHY register programming
according to IEEE 802.3. However, the old code path used ADVERTISE_FULL
which is typically defined as (ADVERTISE_10FULL | ADVERTISE_100FULL).
Does the removal of this in favor of explicit conditional setting of
ADVERTISE_10FULL and ADVERTISE_100FULL based on the advertising mask
maintain backward compatibility for cases where advertising was not
explicitly configured?