Re: [PATCH net-next v3 6/9] net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] qcom-ethqos: 1G DLL configured at SPEED_10 in ethqos_fix_mac_speed_rgmii()
--

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

net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed

This commit changes the initial RGMII link clock rate in qcom_ethqos_probe()
from SPEED_1000 to SPEED_10 to reduce power consumption before a link is
established.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a92bf4a6a2dbd..2471871562ed9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -814,7 +814,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  				     "Failed to get serdes phy\n");
>  
>  	ethqos_set_clk_tx_rate(ethqos, NULL, plat_dat->phy_interface,
> -			       SPEED_1000);
> +			       SPEED_10);

[Severity: High]
Does initializing ethqos->link_clk to SPEED_10 cause the SDCC DLL to be
configured and calibrated at 5 MHz instead of 250 MHz on the initial
1000 Mbps link-up in non-RGMII_ID modes (PHY_INTERFACE_MODE_RGMII,
PHY_INTERFACE_MODE_RGMII_RXID, and PHY_INTERFACE_MODE_RGMII_TXID)?

When a link comes up, stmmac_mac_link_up() in
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c calls fix_mac_speed()
before set_clk_tx_rate():

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_link_up() {
	...
	if (priv->plat->fix_mac_speed)
		priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
					  speed, mode);
	...
	if (priv->plat->set_clk_tx_rate) {
		ret = priv->plat->set_clk_tx_rate(priv->plat->bsp_priv,
						priv->plat->clk_tx_i,
						interface, speed);
	...
}

In ethqos_fix_mac_speed_rgmii(), for non-RGMII_ID modes at SPEED_1000, the
driver enables the SDCC DLL, polls SDC4_STATUS for SDC4_STATUS_DLL_LOCK, and
calls ethqos_dll_configure() (which programs MCLK_FREQ_CALC for 250 MHz and
waits for DDR calibration):

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_fix_mac_speed_rgmii() {
	...
	if (speed != SPEED_100 && speed != SPEED_10) {
		/* Set DLL_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
			      SDCC_HC_REG_DLL_CONFIG);

		/* Set CK_OUT_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_CK_OUT_EN,
			      SDCC_HC_REG_DLL_CONFIG);

		/* Set USR_CTL bit 26 with mask of 3 bits */
		if (!ethqos->has_emac_ge_3)
			rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26),
				      SDCC_USR_CTL);

		/* wait for DLL LOCK */
		if (read_poll_timeout_atomic(rgmii_readl, val,
					     val & SDC4_STATUS_DLL_LOCK,
					     1000, 1000000, true,
					     ethqos, SDC4_STATUS))
			dev_err(dev, "Timeout while waiting for DLL lock\n");
	}

	if (speed == SPEED_1000)
		ethqos_dll_configure(ethqos);
	...
}

Prior to commit 9192320a65b7 ("net: stmmac: qcom-ethqos: convert to
set_clk_tx_rate() method"), ethqos_fix_mac_speed() updated ethqos->link_clk
before configuring the DLL and RGMII macro. After that commit moved the clock
rate update to set_clk_tx_rate(), the initial 1000 Mbps link-up after probe
only had a 250 MHz clock during ethqos_fix_mac_speed_rgmii() because
qcom_ethqos_probe() initialized ethqos->link_clk to SPEED_1000.

With qcom_ethqos_probe() now setting ethqos->link_clk to SPEED_10, won't the
first 1000 Mbps link-up run ethqos_fix_mac_speed_rgmii() and
ethqos_dll_configure() while ethqos->link_clk is still at 5 MHz, and only
switch ethqos->link_clk to 250 MHz afterwards in ethqos_set_clk_tx_rate()
without re-locking or recalibrating the DLL?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-shikra_ethernet-v3-0-26a8beb44b8f@oss.qualcomm.com?part=6