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

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

[Yongzhao Chen]

After a QCA8337 SmartSpeed downshift, genphy_read_status() can report
1000 Mbit/s from the advertised modes when the PHY is running at
100 Mbit/s. On a Redmi AX5400 running downstream OpenWrt Linux 6.18.52,
this caused qca8k to program the user-port MAC for 1000 Mbit/s and
traffic failed. An earlier prototype that read the resolved speed
from the PHY restored traffic.

Wrap genphy_read_status() and use at803x_read_specific_status() to
replace its speed and duplex result when an autonegotiated link comes
up. Resolve pause for the resulting duplex. Until the PHY reports a
resolved speed and duplex, report the link as down so that the next
poll reads them again, as marvell_read_status_page_an() does.
QCA8337's register layout matches the helper (data sheet 80-Y0619-3
Rev. D, pages 327-328).

Keep the generic master/slave read order and the early return for a
steady autonegotiated link. This avoids additional reads of the
clear-on-read idle error count in MII_STAT1000. Forced mode continues
to use BMCR. Report the MDI-X state read at link-up and invalidate it
when the link goes down or autonegotiation is disabled. Clear
mdix_ctrl because this driver does not implement MDI-X configuration.

Fixes: b3591c2a3661 ("net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB")
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
v4:
 - Report the link as down while the PHY-specific status is unresolved
   or holds the reserved speed value, as marvell_read_status_page_an()
   does. v3 reported the link up with SPEED_UNKNOWN, and the early
   return then kept it until the link dropped.
 - Invalidate mdix when the link goes down or autonegotiation is
   disabled, instead of keeping the value read at the last link-up.
 - Rebased onto net 37f12441f557. The files involved are unchanged.
 Both issues were raised in the Sashiko review of v3, which Jakub
 forwarded.

v3: https://lore.kernel.org/netdev/20261006160316.1356-1-yongzhao.derek@gmail.com/
v2: https://lore.kernel.org/netdev/20261003173210.1235-1-yongzhao.derek@gmail.com/
v1: https://lore.kernel.org/netdev/20260928220749.857-1-yongzhao.derek@gmail.com/

Testing: the source-extraction model passes 27 checks covering speed
and duplex at the phylib notification boundary, latch-low transitions,
pause, forced mode, MDIO errors, the idle-error and MSFAIL read order
from v3, and the two issues above. v3 fails exactly the four checks
for those two issues: an unresolved or reserved status at link-up,
and mdix after a link drop or with autonegotiation disabled. An arm64
W=1 build of qca83xx.o, at803x.o and qcom-phy-lib.o is clean.

v4 has not been tested on hardware. The hardware result above used
v2's at803x_read_status() plus genphy_read_master_slave() on one board,
with an Intel igb peer, a two-pair cable and default autonegotiation.
Ping changed from 0/20 to 20/20 in each direction at resolved 100 Full.
Whether QCA8337 can report link up before the resolved bit is set is
unknown.

 drivers/net/phy/qcom/qca83xx.c | 53 ++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
index bc70ed8efd8..be00bc7cda0 100644
--- a/drivers/net/phy/qcom/qca83xx.c
+++ b/drivers/net/phy/qcom/qca83xx.c
@@ -92,6 +92,58 @@ static int qca83xx_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca8337_read_status(struct phy_device *phydev)
+{
+	struct at803x_ss_mask ss_mask = {
+		.speed_mask = AT803X_SS_SPEED_MASK,
+		.speed_shift = __bf_shf(AT803X_SS_SPEED_MASK),
+	};
+	int ret, old_link = phydev->link;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	/* Keep the state resolved at link-up while the link stays up */
+	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
+		goto out;
+
+	phydev->mdix = ETH_TP_MDI_INVALID;
+
+	/* genphy_read_status() derives speed and duplex from the
+	 * advertisements, which is wrong after a SmartSpeed downshift.
+	 * Use what the PHY resolved instead.
+	 */
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
+	    phydev->link) {
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+		phydev->pause = false;
+		phydev->asym_pause = false;
+
+		ret = at803x_read_specific_status(phydev, ss_mask);
+		if (ret)
+			return ret;
+
+		/* Report the link down until speed and duplex are resolved,
+		 * so that the next poll reads them again.
+		 */
+		if (phydev->speed == SPEED_UNKNOWN ||
+		    phydev->duplex == DUPLEX_UNKNOWN) {
+			phydev->link = 0;
+			phydev->mdix = ETH_TP_MDI_INVALID;
+		} else {
+			phy_resolve_aneg_pause(phydev);
+		}
+	}
+
+out:
+	/* MDI-X configuration is not implemented */
+	phydev->mdix_ctrl = ETH_TP_MDI_INVALID;
+
+	return 0;
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -220,6 +272,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: 37f12441f557468a56c1e27790413aa78c82afa2
-- 
2.43.0

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The fix covers only the QCA8337 entry of qca83xx_driver[] in
  drivers/net/phy/qcom/qca83xx.c.
- [Low] In qca8337_read_status() (drivers/net/phy/qcom/qca83xx.c), the
  link is forced down when at803x_read_specific_status() leaves speed or…

--

net: phy: qca83xx: read resolved QCA8337 link status

This adds a read_status callback for the QCA8337 internal PHY. It wraps
genphy_read_status() and, when an autonegotiated link comes up, replaces
speed and duplex with the values from at803x_read_specific_status(). It
also reports the link as down until the PHY-specific status says it is
resolved.

> diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
> index bc70ed8efd86a..be00bc7cda0b9 100644
> --- a/drivers/net/phy/qcom/qca83xx.c
> +++ b/drivers/net/phy/qcom/qca83xx.c
> @@ -92,6 +92,58 @@ static int qca83xx_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int qca8337_read_status(struct phy_device *phydev)
> +{

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
> +	    phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		phydev->pause = false;
> +		phydev->asym_pause = false;
> +
> +		ret = at803x_read_specific_status(phydev, ss_mask);
> +		if (ret)
> +			return ret;
> +
> +		/* Report the link down until speed and duplex are resolved,
> +		 * so that the next poll reads them again.
> +		 */
> +		if (phydev->speed == SPEED_UNKNOWN ||
> +		    phydev->duplex == DUPLEX_UNKNOWN) {
> +			phydev->link = 0;
> +			phydev->mdix = ETH_TP_MDI_INVALID;

[Severity: Low]
Should more of the link-up state be cleared here?

By the time qca8337_read_status() forces link to 0, genphy_read_status()
has already run with link=1 and autoneg_complete=1. That means:

  genphy_read_master_slave() has set master_slave_state to MASTER or SLAVE
  genphy_read_lpa() has filled lp_advertising

There is also the case where the 2-bit speed field holds the reserved
value 3. There, at803x_read_specific_status() leaves speed as
SPEED_UNKNOWN but still sets duplex to DUPLEX_FULL or DUPLEX_HALF.

Only link and mdix are reset. Until the next poll, could ethtool report
"Link detected: no" together with a master/slave state, a link partner
advertisement and possibly a concrete duplex?

The commit message names marvell_read_status_page_an() as the model. Its
caller, marvell_read_status_page(), clears lp_advertising first:

drivers/net/phy/marvell.c:marvell_read_status_page() {
    ...
	linkmode_zero(phydev->lp_advertising);
	phydev->pause = 0;
	phydev->asym_pause = 0;
	phydev->speed = SPEED_UNKNOWN;
	phydev->duplex = DUPLEX_UNKNOWN;
    ...
}

The PHY_HALTED path in _phy_state_machine() also resets
master_slave_state, mdix and lp_advertising together when it forces the
link down.

> +		} else {
> +			phy_resolve_aneg_pause(phydev);
> +		}
> +	}

[ ... ]

> @@ -220,6 +272,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,

[Severity: Medium]
Does the QCA8327 internal PHY need the same fix?

In the same qca83xx_driver[] table, the QCA8327-A and QCA8327-B entries
still have no .read_status, so phylib uses genphy_read_status() for them.

If those PHYs can also do a SmartSpeed downshift, the same path would
report 1000 Mbit/s while the PHY runs at 100 Mbit/s:

genphy_read_status()
  phy_resolve_aneg_linkmode()     picks 1000
phylink
  qca8k_phylink_mac_link_up()
    case SPEED_1000:
        reg = QCA8K_PORT_STATUS_SPEED_1000;

The Fixes: commit (the qca8k switch to phylink) covers every qca8k
switch, not only QCA8337.

On QCA8327 the wrong speed has a second user.
qca83xx_link_change_notify() only applies the 100M DAC amplitude
adjustment when phydev->speed is SPEED_100:

drivers/net/phy/qcom/qca83xx.c:qca83xx_link_change_notify() {
	if (phydev->state == PHY_RUNNING) {
		if (phydev->speed == SPEED_100)
			at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
					      QCA8327_DEBUG_MANU_CTRL_EN,
					      QCA8327_DEBUG_MANU_CTRL_EN);
    ...
}

If the link is misreported as 1000, would that adjustment be skipped?

The commit message does not mention QCA8327. Could it say whether these
PHYs can downshift, or why they were left out?

>  	.get_sset_count		= qca83xx_get_sset_count,
>  	.get_strings		= qca83xx_get_strings,
>  	.get_stats		= qca83xx_get_stats,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008193515.14880-1-yongzhao.derek%40gmail.com

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

[Yongzhao Chen]

On Mon, 12 Oct 2026 19:35:48 +0000 netdev-bot+sashiko@kernel.org wrote:
> Should more of the link-up state be cleared here?

Yes. genphy_read_status() has already filled master_slave_state and
lp_advertising for an up link by then, and a reserved speed value
leaves a concrete duplex. phylink is not told about the link in this
case, so only ethtool sees the stale values until the next poll, but
v5 clears them anyway: duplex, master_slave_state and lp_advertising,
as the PHY_HALTED path does.

> Does the QCA8327 internal PHY need the same fix?

Possibly. The AR8327 data sheet (MKG-15791 Rev. 1.0) gives PHY ID 2
as 0xd033, which matches QCA8327_A_PHY_ID. It documents the same
PHY-specific status layout, and SmartSpeed (register 0x14, bit 5) is
enabled after a hardware reset. I found nothing for the QCA8327-B
PHY ID.

I have no QCA8327 hardware, and the failure and the tests were on
QCA8337 only, so v5 stays limited to QCA8337 and says why in the
commit message. Using the same function for the QCA8327 entries would
also give qca83xx_link_change_notify() the resolved speed. I think
that should be a separate patch, tested by someone with such a board.

The model test now has a check for the state left after an unresolved
or reserved status, and v4 fails it. v5 has not been tested on
hardware. I will send it separately.

Thanks,
Yongzhao Chen

pw-bot: cr