diff mbox series

[net-next,v15,5/9] net: stmmac: qcom-ethqos: set serdes mode before powerup

Message ID 20260923-qcom-sa8255p-emac-v15-5-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 
Call phy_set_mode_ext() before phy_power_on() in
qcom_ethqos_serdes_powerup(). This is harmless for existing users but on
SCMI systems this is required for the PHY driver to select the right
performance level - which translates to the link speed. This is done
ahead of adding support for the firmware-managed EMAC on Qualcomm sa8255p.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 11 ++++++++++-
 1 file changed, 10 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 1 potential
issue(s) to consider.

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

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] In qcom_ethqos_serdes_powerup()
  (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c), the patch…

--

net: stmmac: qcom-ethqos: set serdes mode before powerup

This patch adds a call to phy_set_mode_ext() before phy_power_on() in
qcom_ethqos_serdes_powerup(). On SCMI systems the PHY driver needs it to
pick the right performance level, which sets the link speed. It prepares
for adding support for the firmware-managed EMAC on Qualcomm sa8255p.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a1..47b70b5e706f2 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -601,10 +601,19 @@ static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
> +			       ethqos->phy_mode);
> +	if (ret)
> +		goto err_out;

[Severity: Medium]
What happens here if ethqos->phy_mode is one of the RGMII variants?

qcom_ethqos_probe() accepts RGMII, RGMII_ID, RGMII_RXID and RGMII_TXID.
It installs serdes_powerup whenever a "serdes" PHY is found, whatever the
phy_mode is:

qcom_ethqos_probe() {
    ...
	if (ethqos->serdes_phy) {
		plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
		plat_dat->serdes_powerdown  = qcom_ethqos_serdes_powerdown;
	}
    ...
}

The qcom,ethqos binding allows this combination too. It has "phys: true"
and "phy-names: const: serdes", with no condition on phy-mode.

Both SerDes PHY drivers reject any submode other than SGMII, 1000BASEX
and 2500BASEX. In phy-qcom-sgmii-eth.c, qcom_dwmac_sgmii_phy_validate()
is called from qcom_dwmac_sgmii_phy_set_mode():

	if (submode == PHY_INTERFACE_MODE_SGMII ||
	    submode == PHY_INTERFACE_MODE_1000BASEX ||
	    submode == PHY_INTERFACE_MODE_2500BASEX)
		return 0;

	return -EINVAL;

The new qcom_dwmac_sgmii_phy_scmi_validate() in
phy-qcom-sgmii-eth-scmi.c does the same.

Take a DT that sets phy-mode = "rgmii*" and also references an enabled
serdes PHY. With this patch, powerup would fail like this:

stmmac_open()
  stmmac_legacy_serdes_power_up()
    qcom_ethqos_serdes_powerup()
      phy_set_mode_ext()
        qcom_dwmac_sgmii_phy_set_mode()
          qcom_dwmac_sgmii_phy_validate() returns -EINVAL
      goto err_out -> phy_exit()

"SerDes powerup failed" is logged, and both stmmac_open() and
stmmac_resume() fail. Before this patch the same DT powered the SerDes
on without an error.

The other caller of this API in the same file already limits the call to
SerDes modes:

ethqos_mac_finish_serdes() {
    ...
	if (interface == PHY_INTERFACE_MODE_SGMII ||
	    interface == PHY_INTERFACE_MODE_2500BASEX)
		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
				       interface);
    ...
}

Later patches in the series don't change serdes_powerup. The patch that
updates phy_mode to the resolved interface in mac_finish() only touches
phy_mode inside mac_finish. mac_finish is only installed for SGMII and
2500BASEX, so an RGMII phy_mode would still reach this call at the end
of the series.

I checked the in-tree DTs. The boards that reference a serdes PHY
(lemans.dtsi and monaco.dtsi based) all use sgmii or 2500base-x. The
RGMII boards have no serdes PHY. So "harmless for existing users" holds
for in-tree users, and this only affects DTs the binding allows but that
aren't in the tree.

Should the call be guarded the same way as in ethqos_mac_finish_serdes()
(SGMII, 1000BASEX and 2500BASEX only)? Another option would be to
install serdes_powerup only for SerDes phy-modes in qcom_ethqos_probe(),
or to restrict the binding.

> +
>  	ret = phy_power_on(ethqos->serdes_phy);
>  	if (ret)
> -		phy_exit(ethqos->serdes_phy);
> +		goto err_out;
>  
> +	return 0;
> +
> +err_out:
> +	phy_exit(ethqos->serdes_phy);
>  	return ret;
>  }
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 ac7d6d3e205a1ab5b391def879d6f1033a0961b6..47b70b5e706f221c01f1c0ae3b1acafae6641165 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -601,10 +601,19 @@  static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv)
 	if (ret)
 		return ret;
 
+	ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
+			       ethqos->phy_mode);
+	if (ret)
+		goto err_out;
+
 	ret = phy_power_on(ethqos->serdes_phy);
 	if (ret)
-		phy_exit(ethqos->serdes_phy);
+		goto err_out;
 
+	return 0;
+
+err_out:
+	phy_exit(ethqos->serdes_phy);
 	return ret;
 }