Re: [PATCH net v2 3/3] net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

Re: [PATCH net v2 3/3] net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] Misattributed Fixes: tag and inconsistent stable range.
- [Medium] Setting STMMAC_FLAG_SERDES_SUPPORTS_2500M makes
  stmmac_mac_select_pcs() (stmmac_main.c:1031-1033) return the dwmac…

--

net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

This sets STMMAC_FLAG_SERDES_SUPPORTS_2500M in qcom_ethqos_probe() when the
optional SerDes phy is present, so that stmmac_integrated_pcs_init() adds
PHY_INTERFACE_MODE_2500BASEX to the integrated PCS supported_interfaces
bitmap.

A couple of questions about this one.

> Fixes: 61e9be0efbe8 ("net: stmmac: qcom-ethqos: add support for 2.5G BASEX mode")
> Cc: stable@vger.kernel.org # 7.0+

[Severity: Medium]
Is this the right commit to blame?  The behaviour being corrected is the
flag gate in stmmac_integrated_pcs_init():

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:stmmac_integrated_pcs_init() {
	...
	/* Only allow 2500BASE-X if the SerDes has support. */
	if (priv->plat->flags & STMMAC_FLAG_SERDES_SUPPORTS_2500M)
		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
			  spcs->pcs.supported_interfaces);
	...
}

git show 61e9be0efbe8 only touches dwmac-qcom-ethqos.c (it adds
qcom_ethqos_speed_mode_2500() and the PHY_INTERFACE_MODE_2500BASEX switch
case); it does not touch stmmac_pcs.c or include/linux/stmmac.h.

git blame points both the gate above and the

	#define STMMAC_FLAG_SERDES_SUPPORTS_2500M	BIT(16)

definition in include/linux/stmmac.h at 68cff4fff61f ("net: stmmac: add
BASE-X support to integrated PCS"), which is also where the flag first
appeared, and no in-tree platform ever set it.  Should the Fixes tag name
68cff4fff61f instead?

There is also a mismatch between the two tags: 61e9be0efbe8 shipped in
v6.11, so the Fixes tag nominates trees that predate the code being fixed,
while the Cc annotation says 7.0+.  Which range is intended?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a..0e07225a70ca 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -786,6 +786,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		plat_dat->host_dma_width = data->dma_addr_width;
>  
>  	if (ethqos->serdes_phy) {
> +		plat_dat->flags |= STMMAC_FLAG_SERDES_SUPPORTS_2500M;

[Severity: Medium]
Does setting this flag leave auto-negotiation enabled for 2500BASE-X on
ethqos?

With 2500BASEX now in the integrated PCS supported_interfaces,
stmmac_mac_select_pcs() starts returning the integrated PCS for that
interface instead of NULL (ethqos installs no plat->select_pcs):

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_select_pcs() {
	...
	if (priv->integrated_pcs &&
	    test_bit(interface, priv->integrated_pcs->pcs.supported_interfaces))
		return &priv->integrated_pcs->pcs;
	...
}

That newly routes ethqos through dwmac_integrated_pcs_config():

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:dwmac_integrated_pcs_config() {
	bool changed = false, ane = true;

	/* Only configure the advertisement and allow AN in BASE-X mode if
	 * the core supports TBI/RTBI. ...
	 */
	if (phy_interface_mode_is_8023z(interface) &&
	    spcs->support_tbi_rtbi) {
		ane = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
	...
	dwmac_ctrl_ane(spcs->base, 0, ane,
		       spcs->priv->hw->reverse_sgmii_enable);
}

ethqos is the non-TBI case, per the comment in
stmmac_integrated_pcs_init():

	/* If the PCS supports extended status, then it supports BASE-X AN
	 * with a TBI interface to the SerDes. Otherwise, we can support
	 * BASE-X without AN using SGMII, which is required for qcom-ethqos.
	 */

So support_tbi_rtbi is false, the branch is skipped, ane stays true, and
dwmac_ctrl_ane() programs GMAC_AN_CTRL_ANE|GMAC_AN_CTRL_RAN even though
dwmac_integrated_pcs_inband_caps() reported only LINK_INBAND_DISABLE for
802.3z on this core and phylink therefore passed an out-of-band neg_mode.

That also leaves two layers writing GMAC_AN_CTRL in opposite directions:
pcs_config() sets ANE, while the glue clears it later at link up:

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_fix_mac_speed_sgmii() {
	...
	ethqos_pcs_set_inband(ethqos, interface == PHY_INTERFACE_MODE_SGMII);
	...
}

Is correctness then dependent on a mac_link_up() always following
pcs_config()?  If the interface is configured while the PHY link is down,
or on a repeated major_config without a link-up, the PCS looks like it is
left with AN enabled and restarted on a 2500BASE-X link whose SerDes does
no AN.

Should the ane initialisation in dwmac_integrated_pcs_config() be fixed
first, and should the now-redundant ethqos_pcs_set_inband() write be
mentioned in the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SJ2PR11MB8401F5C3A075DE2A0013E063E4872%40SJ2PR11MB8401.namprd11.prod.outlook.com

[PATCH net v2 0/3] stmmac: fix ETHQOS SerDes interface handling

[Thomas Karpiniec]

From: Thomas Karpiniec <tom@wendy.sh>

With a QCA8081 PHY and phy-mode = "2500base-x", stmmac reports only
2500BASE-X in phylink's supported interface bitmap. Phylink consequently
removes the PHY's lower-speed copper modes, preventing a link with a
gigabit switch.

The QCA8081 uses SGMII for lower copper speeds and 2500BASE-X for 2.5Gbps.
ETHQOS already switches the SerDes accordingly. Add the integrated PCS
interface bitmap to phylink's supported interfaces and mark the Qualcomm
SerDes as supporting 2500BASE-X. Also propagate platform mac_finish()
errors instead of reporting a successfully configured link.

The existing Qualcomm PCS ANE handling is unchanged. Physical cable swaps
produced transient PCS events during the unplug interval, but carrier and
PCS counters remained unchanged for 60 seconds after each final link.

This exact series was tested on an IQ-8275 EVK with cold boots and
physical cable swaps at 1Gbps and 2.5Gbps. Two 100-packet tests at 1Gbps
had no loss. Injecting -ETIMEDOUT from mac_finish() held carrier down at
both speeds, and both recovered after disabling injection and restarting
the interface. The test did not cover a speed transition with the cable
remaining connected or traffic at 2.5Gbps.

The tests used regulator_ignore_unused: without it, the upstream board
tree loses PCS link because it lacks the vendor REFGEN always-on
workaround. No other kernel or device-tree patches were applied.

Changes in v2:
- Replace the qcom-ethqos get_interfaces() callback with integrated PCS
  interface propagation and the 2500BASE-X capability flag.

Thomas Karpiniec (3):
  net: stmmac: propagate platform mac_finish errors
  net: stmmac: advertise integrated PCS interfaces to phylink
  net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

 .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c      | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)


base-commit: 46bc52d13594848023e681860df8700c8db14354
-- 
2.54.0 (Apple Git-157)

[PATCH net v2 3/3] net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

[Thomas Karpiniec]

From: Thomas Karpiniec <tom@wendy.sh>

The Qualcomm ETHQOS SerDes supports SGMII, 1000BASE-X and
2500BASE-X. However, the integrated PCS only includes 2500BASE-X in its
supported interface bitmap when the platform sets
STMMAC_FLAG_SERDES_SUPPORTS_2500M.

Set the flag when the optional SerDes is present. This allows stmmac to
select and configure the integrated PCS for 2500BASE-X and, once its
interface bitmap is propagated to phylink, preserves both the
lower-speed SGMII modes and 2500BASE-T.

Fixes: 61e9be0efbe8 ("net: stmmac: qcom-ethqos: add support for 2.5G BASEX mode")
Cc: stable@vger.kernel.org # 7.0+
Assisted-by: LLM sparse
Signed-off-by: Thomas Karpiniec <tom@wendy.sh>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index ac7d6d3e205a..0e07225a70ca 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -786,6 +786,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 		plat_dat->host_dma_width = data->dma_addr_width;
 
 	if (ethqos->serdes_phy) {
+		plat_dat->flags |= STMMAC_FLAG_SERDES_SUPPORTS_2500M;
 		plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
 		plat_dat->serdes_powerdown  = qcom_ethqos_serdes_powerdown;
 	}
-- 
2.54.0 (Apple Git-157)