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

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?

[ ... ]

[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,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-qcom-sa8255p-emac-v15-0-e82f33720737%40oss.qualcomm.com