diff mbox series

[net-next,v14,07/10] net: stmmac: qcom-ethqos: fix SGMII loopback not set on resume after speed change

Message ID 20260908-qcom-sa8255p-emac-v14-7-ec212b747a7d@oss.qualcomm.com (mailing list archive)
State New
Headers
Series net: stmmac: qcom-ethqos: add support for SCMI power domains |

Commit Message

Bartosz Golaszewski Sept. 8, 2026, 2:57 p.m. UTC 
The loopback in the MAC wrapper feeds the TX clock back into the RX path,
while the SerDes is being powered up. Without it, dwmac4_dma_reset()
polls a clock that never toggles and times out.

qcom_ethqos_set_sgmii_loopback() gates this on phy_mode being 2500BASEX.
mac_finish() now updates phy_mode to the runtime-resolved interface, any
subsequent resume with a 1G SGMII link skips the loopback setup and the
DMA reset hangs.

Extend the guard to include PHY_INTERFACE_MODE_SGMII.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Sept. 14, 2026, 11:16 p.m. UTC | #1 
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: qcom-ethqos: fix SGMII loopback not set on resume after speed change

This extends the guard in qcom_ethqos_set_sgmii_loopback() so that the MAC
wrapper TX-to-RX loopback is also armed when ethqos->phy_mode holds
PHY_INTERFACE_MODE_SGMII, not only PHY_INTERFACE_MODE_2500BASEX. Without
the loopback, dwmac4_dma_reset() polls an RX clock that never toggles and
times out.

No Link: tags were present in the commit header.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index fd7414a50eb4e..e067ceee01621 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -190,7 +190,8 @@ static void
>  qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
>  {
>  	if (!ethqos->needs_sgmii_loopback ||
> -	    ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
> +	    (ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX &&
> +	     ethqos->phy_mode != PHY_INTERFACE_MODE_SGMII))
>  		return;

[Severity: Medium]
Should this hunk be squashed into the preceding patch of the series, or at
least carry a Fixes: tag naming it?

The commit message says "mac_finish() now updates phy_mode to the
runtime-resolved interface", which is the change made one patch earlier in
this same series:

  f40e72d57bcb38 ("net: stmmac: qcom-ethqos: update phy_mode to the
  resolved interface in mac_finish()")

git blame at this commit shows the assignment in ethqos_mac_finish_serdes():

	ethqos->phy_mode = interface;

came from f40e72d57bcb38. Before that, ethqos->phy_mode was written only
once, in qcom_ethqos_probe() from the DT phy-mode property, so the
2500BASEX-only guard could not go stale at runtime and the DMA reset
timeout described here did not exist.

Does that leave f40e72d57bcb38 broken on its own, e.g. as a bisect point or
a stable backport? On a needs_sgmii_loopback platform (emac_v4_0_0_data)
with DT phy-mode "2500base-x" whose link resolves to 1G SGMII:

  ethqos_mac_finish_serdes()      -> ethqos->phy_mode = PHY_INTERFACE_MODE_SGMII
  suspend / ifdown
  resume / ifup
  ethqos_clks_config(ethqos, true)
    qcom_ethqos_set_sgmii_loopback(ethqos, true)
      if (... ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
              return;                 /* loopback never enabled */
  dwmac4_dma_reset()                  /* polls a non-toggling clock, times out */

Also, the subject line reads as a standalone fix for a pre-existing
problem, which hides the dependency on the previous patch. Would either
squashing the guard change into f40e72d57bcb38, or adding

  Fixes: f40e72d57bcb38 ("net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()")

make the ordering requirement explicit?

One more note on the commit message wording: the second paragraph reads
"qcom_ethqos_set_sgmii_loopback() gates this on phy_mode being 2500BASEX.
mac_finish() now updates phy_mode to the runtime-resolved interface, any
subsequent resume with a 1G SGMII link skips the loopback setup" — the
sentence joining is missing a connector such as "so that" or "and since",
which makes the causal relationship hard to follow.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index fd7414a50eb4e7d4d5adad7e93d49a55500c7f32..e067ceee016217c49d3b3d572fd9f33e562d1a75 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -190,7 +190,8 @@  static void
 qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
 {
 	if (!ethqos->needs_sgmii_loopback ||
-	    ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
+	    (ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX &&
+	     ethqos->phy_mode != PHY_INTERFACE_MODE_SGMII))
 		return;
 
 	rgmii_updatel(ethqos,