Re: [PATCH net-next] net: phy: qca83xx: read resolved QCA8337 link status

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Sep 29, 2026 at 12:07:49AM +0200, Yongzhao Chen wrote:
> Read QCA8337's vendor status on each autonegotiated link check so the
> reported speed matches the PHY after SmartSpeed changes negotiation.
> Keep link down while that status is unresolved or encodes a reserved
> speed; PHYLIB then notifies its consumers when valid status returns.
> Forced mode continues to use the generic BMCR result.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
> Assisted-by: LLM
> ---
> Andrew suggested this in his review of RFC v3 4/5, where he noted that
> read_status should report the real speed from the vendor register.
> Thanks, Andrew. I mentioned this change in the same thread:
> https://lore.kernel.org/netdev/20260924234814.1734-1-yongzhao.derek@gmail.com/
> 
> It does not depend on the qca8k series and applies to net-next on its own.
> 
> Testing: a model test compiles this function with the real
> genphy_read_status(), phy_resolve_aneg_pause(), phy_read_status() and
> phy_check_link_status() and covers 10/100/1000 in both duplexes,
> unresolved and reserved speed codes, a speed change while the link
> stays up, forced mode, pause resolution and MDIO read errors. W=1
> builds for arm64 are clean. The same function is in my OpenWrt Linux
> 6.18.52 build for a Redmi AX5400 (QCA8337), including the build used
> to test the at803x IPQ5018 fixes. I have not injected a real SmartSpeed
> downshift on a user port.
> 
>  drivers/net/phy/qcom/qca83xx.c | 49 ++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
> index bc70ed8efd8..b3f3183d4a4 100644
> --- a/drivers/net/phy/qcom/qca83xx.c
> +++ b/drivers/net/phy/qcom/qca83xx.c
> @@ -92,6 +92,54 @@ static int qca83xx_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int qca8337_read_status(struct phy_device *phydev)
> +{
> +	int ret, ss;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return 0;
> +
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = false;
> +	phydev->asym_pause = false;
> +	if (!phydev->link)
> +		return 0;

If there is no link, genphy_read_status() will of already set these to
_UNKNOWN etc. There is no need to clear them.

> +	/* SmartSpeed can make the actual speed differ from the advertised modes. */
> +	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
> +	if (ss < 0)
> +		return ss;
> +	if (!(ss & AT803X_SS_SPEED_DUPLEX_RESOLVED)) {
> +		phydev->link = 0;

Can this really happen? BCMR says there is link, but this says there
is no link? What does the data sheet say? It seems unlikely to me.

	Andrew