Re: [PATCH RFC net-next 9/9] net: stmmac: qcom-ethqos: add Qualcomm Nord support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: stmmac: qcom-ethqos: USXGMII clock remains disabled due to copy-paste error
- [High] net: stmmac: qcom-ethqos: SGMII loopback permanently enabled for non-SerDes PHY modes on Nord
- [High] net: stmmac: qcom-ethqos: missing USXGMII SerDes PHY mode configuration
- [Low] net: stmmac: qcom-ethqos: build failure due to undeclared DWMAC_CORE_25GMAC
--

commit f5df70773c629bcedc94b2086fbc5776c1a49506
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

net: stmmac: qcom-ethqos: add Qualcomm Nord support

This commit adds support for the Qualcomm Nord ETHQOS controllers based on
the Synopsys DesignWare 25GMAC IP. It configures the Nord platform data,
DW25GMAC core register layout, USXGMII wrapper programming, and an EEE clock
vote.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a99a78880c0fb..a03c8e203920a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
[ ... ]
> @@ -201,6 +229,15 @@ qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
>  		      EMAC_WRAPPER_SGMII_PHY_CNTRL1);
>  }
>  
> +static void
> +qcom_ethqos_set_sgmii_loopback_nord(struct qcom_ethqos *ethqos, bool enable)
> +{
> +	rgmii_updatel(ethqos,
> +		      SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN,
> +		      enable ? SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN : 0,
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4);
> +}

[Severity: High]
Does this leave the SGMII loopback permanently enabled for non-SerDes PHY
modes on Nord?

Looking at ethqos_clks_config(), .set_sgmii_loopback(..., true) is called
unconditionally. However, for non-SerDes modes, .mac_finish is NULL, so
.set_sgmii_loopback(..., false) is never invoked.

The generic qcom_ethqos_set_sgmii_loopback() has a safety check for
ethqos->needs_sgmii_loopback and ethqos->phy_mode to prevent this, but the
new Nord-specific function lacks it.

[ ... ]
> @@ -485,6 +527,47 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  	return 0;
>  }
>  
> +static void ethqos_configure_usxgmii(struct qcom_ethqos *ethqos)
> +{
[ ... ]
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_GMII_CLK_BLK_SEL, 0,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_CLK_EN, 0,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_GMII_CLK_BLK_SEL,
> +		      USXGMII_CLK_BLK_GMII_CLK_BLK_SEL,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +}

[Severity: High]
Will this leave the USXGMII clock permanently disabled?

The code clears USXGMII_CLK_BLK_CLK_EN to disable the clock, but then
incorrectly writes to USXGMII_CLK_BLK_GMII_CLK_BLK_SEL again instead of
setting USXGMII_CLK_BLK_CLK_EN to re-enable it.

This appears to be a copy-paste error that will prevent the network interface
from functioning in USXGMII mode.

[ ... ]
> @@ -625,7 +708,8 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	struct qcom_ethqos *ethqos = priv;
>  	int ret = 0;
>  
> -	qcom_ethqos_set_sgmii_loopback(ethqos, false);
> +	if (ethqos->set_sgmii_loopback)
> +		ethqos->set_sgmii_loopback(ethqos, false);
>  
>  	if (interface == PHY_INTERFACE_MODE_SGMII ||
>  	    interface == PHY_INTERFACE_MODE_2500BASEX)
>  		ret = phy_set_mode_ext(ethqos->serdes_phy, PHY_MODE_ETHERNET,
>  				       interface);
>  
>  	return ret;
>  }

[Severity: High]
Is the PHY_INTERFACE_MODE_USXGMII check missing here?

Since this function is now configured as the .mac_finish callback for the
newly added PHY_INTERFACE_MODE_USXGMII, omitting it from this condition
causes the SerDes PHY configuration to be silently bypassed, which will
prevent link establishment.

[ ... ]
> @@ -817,8 +924,33 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
>  }
>  
> +static const struct ethqos_emac_por emac_nord_por[] = {
> +	{ .offset = RGMII_IO_MACRO_CONFIG,    .value = 0x00C04D03 },
> +	{ .offset = SDCC_HC_REG_DLL_CONFIG,   .value = 0x2004642C },
> +	{ .offset = RGMII_IO_MACRO_CONFIG2,   .value = 0x00222060 },
> +	{ .offset = RGMII_IO_MACRO_SCRATCH_2, .value = 0x4c },
> +};
> +
> +static const struct ethqos_emac_driver_data emac_nord_data = {
> +	.rgmii_por = emac_nord_por,
> +	.num_rgmii_por = ARRAY_SIZE(emac_nord_por),
> +	.dma_addr_width = 40,
> +	.link_clk_name = "phyaux",
> +	.core_type = DWMAC_CORE_25GMAC,

[Severity: Low]
Does this cause a build failure?

The patch assigns DWMAC_CORE_25GMAC to .core_type, but this enum value is
not defined in enum dwmac_core_type in include/linux/stmmac.h.

> +	.set_sgmii_loopback = qcom_ethqos_set_sgmii_loopback_nord,
> +	.dwxgmac_addrs = {
> +		.dma_even_chan_base = 0x00008500,
> +		.dma_odd_chan_base  = 0x00008580,
> +		.dma_chan_offset    = 0x00001000,
> +		.mtl_chan_base      = 0x00008000,
> +		.mtl_chan_offset    = 0x00001000,
> +		.timestamp_base     = 0x00007000,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-qcom_xpcs_nord_emac-v1-0-4b1c682af70f@oss.qualcomm.com?part=9