diff mbox series

[net-next,v15,6/9] net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

Message ID 20260923-qcom-sa8255p-emac-v15-6-e82f33720737@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. 23, 2026, 3:14 p.m. UTC 
ethqos->phy_mode is read by serdes_powerup() and
qcom_ethqos_set_sgmii_loopback() to configure the SerDes and its
wrapper for the correct interface. It is set once at probe from the DT
phy-mode property and never updated.

phylink resolves the actual runtime interface and passes it to the
mac_finish() callback. On platforms that only ever operate at one speed
these two values are always the same, but when the interface changes
at runtime - for example switching between SGMII and 2500BASE-X - the
stored value becomes stale. The next serdes_powerup() call, on
a subsequent link cycle, then reinitializes the SerDes to the wrong mode.

Update ethqos->phy_mode from the interface argument in
ethqos_mac_finish_serdes() so that it always reflects the last
resolved interface.

With that, we now update the phy_mode to the runtime-resolved interface,
so any subsequent resume with a 1G SGMII link skips the loopback setup
and the DMA reset hangs. To remedy this, 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

netdev-bot+sashiko@kernel.org Sept. 27, 2026, 4:29 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch fixes a regression added by the previous patch in the
  same series, "net: stmmac: qcom-ethqos: set serdes mode before…
- [Low] The new comment in ethqos_mac_finish_serdes() (dwmac-qcom-
  ethqos.c:638-641) says phy_mode is kept in sync "for later…

--

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

This patch updates ethqos->phy_mode in ethqos_mac_finish_serdes() from the
interface that phylink resolved, so later serdes_powerup() calls program
the SerDes to the correct mode. It also widens the guard in
qcom_ethqos_set_sgmii_loopback() to cover PHY_INTERFACE_MODE_SGMII. This
keeps the loopback in place on resume with a 1G SGMII link.

> ethqos->phy_mode is read by serdes_powerup() and
> qcom_ethqos_set_sgmii_loopback() to configure the SerDes and its
> wrapper for the correct interface. It is set once at probe from the DT
> phy-mode property and never updated.

[Severity: Low]
Is this a long-standing problem, or was it added earlier in this series?

As far as I can tell, serdes_powerup() only started reading phy_mode in
the preceding patch, "net: stmmac: qcom-ethqos: set serdes mode before
powerup". That patch added this to qcom_ethqos_serdes_powerup():

	ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
			       ethqos->phy_mode);

Before that patch, power-up never used the stale probe-time value.

With only that patch applied, every open/resume replaces the resolved
interface held by the SerDes PHY driver (data->interface in
phy-qcom-sgmii-eth.c) with the DT mode. This stays in effect until
phylink_mac_initial_config()->ethqos_mac_finish_serdes() programs the
resolved mode again.

The commit message doesn't mention this dependency, so backporters won't
know about it. Bisection can also stop on the intermediate commit.

Both patches are in the same unmerged series, so a Fixes: tag isn't the
right fit here. Could this change be folded into "set serdes mode before
powerup", or placed before it? Could the commit message then say so?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 47b70b5e706f2..e067ceee01621 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -634,6 +635,12 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  
>  	qcom_ethqos_set_sgmii_loopback(ethqos, false);
>  
> +	/* The interface mode is only known once phylink has resolved it and
> +	 * may change at runtime, so keep it in sync for later
> +	 * serdes_powerup()/serdes_powerdown() calls.
> +	 */

[Severity: Low]
This isn't a bug, but is the mention of serdes_powerdown() accurate?
qcom_ethqos_serdes_powerdown() never reads ethqos->phy_mode:

static void qcom_ethqos_serdes_powerdown(struct net_device *ndev, void *priv)
{
	struct qcom_ethqos *ethqos = priv;

	phy_power_off(ethqos->serdes_phy);
	phy_exit(ethqos->serdes_phy);
}

The comment also leaves out qcom_ethqos_set_sgmii_loopback(). That
function is called from ethqos_clks_config(true) on every open/resume:

	qcom_ethqos_set_sgmii_loopback(ethqos, true);
	ethqos_set_func_clk_en(ethqos);

After this patch, its loopback decision depends on this assignment,
which is why the guard had to be widened.

Would it be clearer to list serdes_powerup() and
qcom_ethqos_set_sgmii_loopback() as the readers?

> +	ethqos->phy_mode = interface;
> +
>  	if (interface == PHY_INTERFACE_MODE_SGMII ||
>  	    interface == PHY_INTERFACE_MODE_2500BASEX)
>  		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
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 47b70b5e706f221c01f1c0ae3b1acafae6641165..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,
@@ -634,6 +635,12 @@  static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
 
 	qcom_ethqos_set_sgmii_loopback(ethqos, false);
 
+	/* The interface mode is only known once phylink has resolved it and
+	 * may change at runtime, so keep it in sync for later
+	 * serdes_powerup()/serdes_powerdown() calls.
+	 */
+	ethqos->phy_mode = interface;
+
 	if (interface == PHY_INTERFACE_MODE_SGMII ||
 	    interface == PHY_INTERFACE_MODE_2500BASEX)
 		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,