|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-03-31

I will review this patch carefully according to the DPDK coding standards and contribution guidelines.

commit 7ba7d89890ab (reference in Fixes tag)
Author: Ashok Kumar Natarajan <ashokkumar.natarajan at amd.com>
Subject: [PATCH v1] net/axgbe: fix PHY auto-negotiation advertisement

This patch corrects the programming of PHY auto-negotiation advertisement
registers by properly configuring Clause 22 PHY registers according to
IEEE 802.3 standards. It moves from a read-modify-write approach to
directly constructing the advertisement values based on the supported
capabilities.

> 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

According to the DPDK coding style guide, macro definitions should have
aligned values. These new macros use spaces for alignment, but based on the
existing style in the file (using tabs like AXGBE_M88E1512_MODE_RGMII_SGMII),
shouldn't these definitions also be tab-aligned?

> 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;

Does this code intentionally omit advertising half-duplex modes for
10/100baseT? The original code set ADVERTISE_FULL which might have been
masking an issue, but does the hardware support 10baseT_Half and
100baseT_Half? If so, shouldn't those be advertised when present in the
adv mask?

> +	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;
> +	ctrl1000 = 0;
> +	if (adv & ADVERTISED_1000baseT_Full)
> +		ctrl1000 |= AXGBE_PHY_MII_CTRL1000_1000T_FULL;

Similarly, should 1000baseT_Half be advertised if present in the adv mask?
The AXGBE_PHY_MII_CTRL1000_1000T_HALF macro is defined but never used.

Additionally, are the newly defined AXGBE_PHY_MII_CTRL1000_MS_VALUE and
AXGBE_PHY_MII_CTRL1000_MS_MANUAL macros needed? They appear unused in this
patch. If they're not needed yet, should they be removed to avoid adding
dead code?

>  
> -	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;