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

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

[Yongzhao Chen]

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;
+
+	/* 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;
+		return 0;
+	}
+
+	switch ((ss & AT803X_SS_SPEED_MASK) >> 14) {
+	case AT803X_SS_SPEED_10:
+		phydev->speed = SPEED_10;
+		break;
+	case AT803X_SS_SPEED_100:
+		phydev->speed = SPEED_100;
+		break;
+	case AT803X_SS_SPEED_1000:
+		phydev->speed = SPEED_1000;
+		break;
+	default:
+		phydev->link = 0;
+		return 0;
+	}
+
+	phydev->duplex = ss & AT803X_SS_DUPLEX ? DUPLEX_FULL : DUPLEX_HALF;
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
+		phy_resolve_aneg_pause(phydev);
+
+	return 0;
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -220,6 +268,7 @@ static struct phy_driver qca83xx_driver[] = {
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
+	.read_status		= qca8337_read_status,
 	.get_sset_count		= qca83xx_get_sset_count,
 	.get_strings		= qca83xx_get_strings,
 	.get_stats		= qca83xx_get_stats,

base-commit: 014d795c73837ea2339a4ea8e8f82c6e959b845d
-- 
2.43.0

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

[Andrew Lunn]

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

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

[Yongzhao Chen]

Hi Andrew,

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

Right, thanks. In the next revision I have replaced the custom decoding
and state clearing with at803x_read_status(), followed by
genphy_read_master_slave() to keep the master/slave reporting from the
generic path.

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

I have not observed it on hardware. The QCA8337 datasheet (80-Y0619-3
Rev. D, p. 328) describes the resolved bit and the speed/duplex fields
in register 0x11, and says speed/duplex are valid after autonegotiation
completes. I could not find anything there about the ordering of that
bit and the BMSR link status across separate reads. The unresolved case
in my test was synthetic input, not a state captured on hardware.

In that synthetic sequence, at803x_read_status() reports link up with
SPEED_UNKNOWN. If BMSR stays up when 0x11 later resolves, the speed
remains SPEED_UNKNOWN until the next link transition. So switching to
the helper does not by itself answer whether this state can occur on
real hardware. Is there a QCA8337-specific rule about when register 0x11
is updated relative to BMSR that I should check before deciding whether
this needs separate handling?

Separately, I have not yet reproduced on hardware the case this patch
targets, a QCA8337-side SmartSpeed downshift where the old code reports
the wrong speed; so far that difference is only shown by the model test.
With a two-pair cable, the link partner advertised gigabit and then
withdrew it, and the link came up at 100 Mb/s without the QCA8337
downshift status set.

If you know of a link partner or setup that reliably makes the QCA8337
side downshift, I would be glad to test with it.

Thanks,
Yongzhao Chen

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

[Andrew Lunn]

> Separately, I have not yet reproduced on hardware the case this patch
> targets, a QCA8337-side SmartSpeed downshift where the old code reports
> the wrong speed; so far that difference is only shown by the model test.
> With a two-pair cable, the link partner advertised gigabit and then
> withdrew it, and the link came up at 100 Mb/s without the QCA8337
> downshift status set.
> 
> If you know of a link partner or setup that reliably makes the QCA8337
> side downshift, I would be glad to test with it.

Sorry, i've never used this hardware.

One thing you can try is to set the link partner to only advertise 1G,
not the lower speeds. I've no idea what it will do then, but you might
get into the situation you are interested in.

    Andrew

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

[Yongzhao Chen]

Hi Andrew,

> One thing you can try is to set the link partner to only advertise 1G,
> not the lower speeds. I've no idea what it will do then, but you might
> get into the situation you are interested in.

Thanks. I tried that with an Intel igb link partner and a two-pair
cable. With only 1000baseT/Full advertised, the QCA8337 set its
downshift bit at times, but the link did not come up during the
observation window. With the partner's default advertisement, though,
the QCA8337 side did downshift: it was not advertising 1000BASE-T
(CTRL1000 0x0400) while the partner was (STAT1000 0x2800), and register
0x11 showed the downshift bit set and a resolved 100 Mb/s full-duplex
link.

For the subsequent old/new comparison, I kept the partner's default
advertisement and used one OpenWrt Linux 6.18.52 test image. A selector
switched only the tested user PHY between genphy_read_status() and
at803x_read_status() followed by genphy_read_master_slave(), with the
same tracing in both paths.

In both paths, phydev->advertising and lp_advertising still had
1000baseT/Full set. With genphy_read_status(), the driver reported
1000 Mb/s, phylink passed that to the MAC, and the MAC port status
register read back 1000 Mb/s. With the new path, the driver reported
100 Mb/s, phylink passed 100 Mb/s, and the MAC register agreed.
Outside in-band mode, qca8k_phylink_mac_link_up() in net programs the
port speed the same way.

Each path was tested across two warm boots and three port down/up
cycles, and the downshift was present in every stage. Ping was 0/20
in each direction in every old-path stage and 20/20 in every new-path
stage.

This covers the resolved downshift case only. It does not show whether
BMSR can report link up while register 0x11 is still unresolved, and
it does not change the SPEED_UNKNOWN limitation from my previous reply.

Given the traffic failure, I would like to target net for v2. Three
questions:

1. Is net appropriate, or should this stay in net-next?
2. Is 272833b9b3b3 ("net: phy: add support for qca8k switch internal
   PHY in at803x") the right Fixes target? It added the QCA8337 entry
   without .read_status; I have not checked whether the problem
   predates it. Christian, as its author, is now on Cc.
3. Compared with genphy_read_status(), the helper takes the forced-mode
   speed from register 0x11 rather than BMCR and adds MDI-X reporting,
   and the added genphy_read_master_slave() call runs on every poll. Is
   that scope acceptable for net, or would you prefer a narrower fix?

Thanks,
Yongzhao Chen

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

[Andrew Lunn]

> This covers the resolved downshift case only.

It is good testing, thanks.

> It does not show whether
> BMSR can report link up while register 0x11 is still unresolved, and
> it does not change the SPEED_UNKNOWN limitation from my previous reply.

Greed.

> Given the traffic failure, I would like to target net for v2. Three
> questions:
> 
> 1. Is net appropriate, or should this stay in net-next?

The traffic failure was however because of the error with setting the
DACs?

> 2. Is 272833b9b3b3 ("net: phy: add support for qca8k switch internal
>    PHY in at803x") the right Fixes target? It added the QCA8337 entry
>    without .read_status; I have not checked whether the problem
>    predates it. Christian, as its author, is now on Cc.

Reporting the correct downshift has nothing to do with that, as far as
i understand. Correctly reporting downshift i would put to net-next.

> 3. Compared with genphy_read_status(), the helper takes the forced-mode
>    speed from register 0x11 rather than BMCR and adds MDI-X reporting,
>    and the added genphy_read_master_slave() call runs on every poll. Is
>    that scope acceptable for net, or would you prefer a narrower fix?

net is fixing broken things, especially regressions. net-next is new
features.

	Andrew