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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] For legacy RGMII phy-modes (rgmii, rgmii-rxid, rgmii-txid) the
  RGMII IO-macro DLL is programmed and its lock polled inside…
- [Low] Commit message names the wrong callback: it states "fix_mac_speed
  updates the rate once a link is established", but neither…

--

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

The probe-time call to ethqos_set_clk_tx_rate() is changed from SPEED_1000
to SPEED_10 so the RGMII link clock is not left at 250 MHz while no link is
up.  Two questions below, one about the changelog wording and one about the
first link-up path.

> net: stmmac: qcom-ethqos: set initial RGMII link clock to lowest speed
>
> On probe the RGMII link clock is initialised at SPEED_1000, which
> translates to a 250 MHz source clock even when no PHY link is present,
> drawing unnecessary power.
>
> Initialise at SPEED_10 instead; fix_mac_speed updates the rate once
> a link is established.

[Severity: Low]
Is "fix_mac_speed updates the rate once a link is established" the right
callback name here?  Neither ethqos_fix_mac_speed_rgmii() nor
ethqos_fix_mac_speed_sgmii() calls clk_set_rate() on ethqos->link_clk; the
only clk_set_rate(ethqos->link_clk, ...) in the file is in
ethqos_set_clk_tx_rate(), which probe installs as
plat_dat->set_clk_tx_rate.

stmmac_mac_link_up() invokes the two as separate callbacks, fix_mac_speed()
first and set_clk_tx_rate() later:

	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);

Could the changelog name set_clk_tx_rate instead, so the ordering is not
hidden?

> 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: Medium]
Does the first 1000 Mbps link-up now run the SDCC DLL bring-up with the
probe-time 10 Mbps clock rate?

For the legacy rgmii / rgmii-rxid / rgmii-txid modes,
ethqos_fix_mac_speed_rgmii() enables the DLL and polls for lock:

	if (speed != SPEED_100 && speed != SPEED_10) {
		/* Set DLL_EN */
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,
			      SDCC_HC_REG_DLL_CONFIG);
		...
		/* 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");
	}

and stmmac_mac_link_up() calls fix_mac_speed() before set_clk_tx_rate(), so
at that point ethqos->link_clk is still at the rate programmed at probe.
With SPEED_10 that is rgmii_clock(SPEED_10) = 2500000, doubled to 5 MHz for
the non-rgmii-id modes by ethqos_set_clk_tx_rate(), where it used to be
250 MHz:

	/* Clock Rate Requirements:
	 * MAC added delay: 250/50/5 Mhz for 1G/100M/10M
	 * No MAC delay (DLL bypass): 250/25/2.5 Mhz for 1G/100M/10M
	 */

If the DLL cannot lock at that input rate, the poll busy-waits its full 1 s
budget in the link-up path and then continues with an unlocked DLL, since
fix_mac_speed() is not re-run after set_clk_tx_rate() raises the rate to
250 MHz.

Note the driver itself skips the DLL enable/lock for SPEED_10 and
SPEED_100, which suggests the block is expected to see the rate matching
the negotiated speed.  Is a minimum DLL input frequency involved here, and
if so should the probe-time rate stay high, or should the clock be raised
before fix_mac_speed() runs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-shikra_ethernet-v2-0-bbe3389d0652%40oss.qualcomm.com