|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

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

The commit corrects programming of PHY auto-negotiation advertisement
registers to advertise only supported full-duplex capabilities for
10/100/1000BASE-T operation. The MAC does not support half-duplex CSMA/CD
operation, so half-duplex capabilities are intentionally omitted from the
advertisement.

> 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

Does this code properly account for endianness? The value 0x0200
corresponds to bit 9 in little-endian representation. Does the underlying
hardware register access layer handle byte-order conversion, or is this
constant platform-dependent?

>  
>  /* 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;
> +
> +	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;

In axgbe_phy_config_advert(), does this code fully clear previous MII_CTRL1000
contents before writing the new value? If the register was previously
programmed with different bits set, can those old bits remain active after
this write operation?

The original code in axgbe_phy_config_advert() read MII_ADVERTISE, modified
it, then wrote it back. Does the removal of the read-modify-write sequence
introduce a correctness issue where reserved or implementation-specific bits
in MII_ADVERTISE might be unintentionally cleared?