Re: [PATCH net 2/3] net: phy: as21xxx: fix read_status speed handling

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

On 6/1/26 12:53, Christian Marangi wrote:
> With further test with 2.5G NIC it was discovered that
> phy_resolve_aneg_linkmode is not enough to detect speed higher that 1G
> when autoneg is enabled.
> 
> Also in the switch case there is a typo where the speed mask is AND with
> VEND1_SPEED_STATUS instead of the correct mask VEND1_SPEED_MASK.
> 
> Rework the read_status code to always read the speed from the vendor
> register and parse the generic bit only for the pause frame.
> 
> Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  drivers/net/phy/as21xxx.c | 96 +++++++++++++++++++++------------------
>  1 file changed, 53 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index 0db82da8dbdf..97ca37c6929f 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c
> @@ -671,7 +671,7 @@ static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
>  
>  static int as21xxx_read_c22_lpa(struct phy_device *phydev)
>  {
> -	int lpagb;
> +	int lpagb, lpa;
>  
>  	/* MII_STAT1000 are only filled in the mapped C22
>  	 * in C45, use that to fill lpagb values and check.
> @@ -698,12 +698,20 @@ static int as21xxx_read_c22_lpa(struct phy_device *phydev)
>  	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
>  					lpagb);
>  
> +	lpa = phy_read_mmd(phydev, MDIO_MMD_AN,
> +			   AS21XXX_MDIO_AN_C22 + MII_LPA);
> +	if (lpa < 0)
> +		return lpa;
> +
> +	mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
> +
>  	return 0;
>  }
>  
>  static int as21xxx_read_status(struct phy_device *phydev)
>  {
>  	int bmcr, old_link = phydev->link;
> +	int speed;
>  	int ret;
>  
>  	ret = as21xxx_read_link(phydev, &bmcr);
> @@ -720,58 +728,60 @@ static int as21xxx_read_status(struct phy_device *phydev)
>  	phydev->asym_pause = 0;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE) {
> -		ret = genphy_c45_read_lpa(phydev);
> -		if (ret)
> -			return ret;
> +		if (!phydev->autoneg_complete) {
> +			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
> +							0);
> +			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +			return 0;
> +		}
>  
>  		ret = as21xxx_read_c22_lpa(phydev);
>  		if (ret)
>  			return ret;
> -
> -		phy_resolve_aneg_linkmode(phydev);
>  	} else {
> -		int speed;
> -
>  		linkmode_zero(phydev->lp_advertising);
> +	}
>  
> -		speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> -				     VEND1_SPEED_STATUS);
> -		if (speed < 0)
> -			return speed;
> -
> -		switch (speed & VEND1_SPEED_STATUS) {
> -		case VEND1_SPEED_10000:
> -			phydev->speed = SPEED_10000;
> -			phydev->duplex = DUPLEX_FULL;
> -			break;
> -		case VEND1_SPEED_5000:
> -			phydev->speed = SPEED_5000;
> -			phydev->duplex = DUPLEX_FULL;
> -			break;
> -		case VEND1_SPEED_2500:
> -			phydev->speed = SPEED_2500;
> -			phydev->duplex = DUPLEX_FULL;
> -			break;
> -		case VEND1_SPEED_1000:
> -			phydev->speed = SPEED_1000;
> -			if (bmcr & BMCR_FULLDPLX)
> -				phydev->duplex = DUPLEX_FULL;
> -			else
> -				phydev->duplex = DUPLEX_HALF;
> -			break;
> -		case VEND1_SPEED_100:
> -			phydev->speed = SPEED_100;
> +	speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +			     VEND1_SPEED_STATUS);
> +	if (speed < 0)
> +		return speed;
> +
> +	switch (speed & VEND1_SPEED_MASK) {
> +	case VEND1_SPEED_10000:
> +		phydev->speed = SPEED_10000;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;
> +	case VEND1_SPEED_5000:
> +		phydev->speed = SPEED_5000;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;
> +	case VEND1_SPEED_2500:
> +		phydev->speed = SPEED_2500;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;
> +	case VEND1_SPEED_1000:
> +		phydev->speed = SPEED_1000;
> +		if (bmcr & BMCR_FULLDPLX)
>  			phydev->duplex = DUPLEX_FULL;
> -			break;
> -		case VEND1_SPEED_10:
> -			phydev->speed = SPEED_10;
> -			phydev->duplex = DUPLEX_FULL;
> -			break;
> -		default:
> -			return -EINVAL;
> -		}
> +		else
> +			phydev->duplex = DUPLEX_HALF;
> +		break;
> +	case VEND1_SPEED_100:
> +		phydev->speed = SPEED_100;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;
> +	case VEND1_SPEED_10:
> +		phydev->speed = SPEED_10;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> +	if (phydev->autoneg == AUTONEG_ENABLE)
> +		phy_resolve_aneg_pause(phydev);
> +
>  	return 0;
>  }
>