Re: [PATCH net-next 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 3 potential
issue(s) to consider:

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

- [High] Lowering the probe-time link clock to the SPEED_10 rate makes
  the first link-up's RGMII IO-macro/DLL programming run with a…
- [High] The RGMII source-clock rate no longer matches the programmed IO-
  macro/DLL delay mode for phy-mode "rgmii-txid" and "rgmii-rxid" at…
- [Medium] The commit message's safety argument, "fix_mac_speed updates
  the rate once a link is established", describes a mechanism that no…

--

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

The probe-time initialisation of the RGMII link clock is changed from
SPEED_1000 to SPEED_10, so the source clock starts at the 10 Mbps rate
instead of 250 MHz while no link is present.

> 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: Medium]
Is the sentence "fix_mac_speed updates the rate once a link is established"
accurate for this driver?

Neither ethqos_fix_mac_speed_rgmii() nor ethqos_fix_mac_speed_sgmii()
contains a clk_set_rate() call.  The only place the link clock rate is
programmed is ethqos_set_clk_tx_rate():

	return clk_set_rate(ethqos->link_clk, rate);

which is installed as plat_dat->set_clk_tx_rate in qcom_ethqos_probe().

stmmac_mac_link_up() calls the two callbacks in this order:

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

So the rate is updated by set_clk_tx_rate(), and it happens after
fix_mac_speed() rather than before it.  Could the changelog be reworded to
match the callback that actually applies the rate, and the ordering it runs
in?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 2ca2f03de3017..4f9bef662c579 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 this leave the first link-up's DLL programming running at the 10 Mbps
reference rate on the legacy RGMII phy-modes?

Because stmmac_mac_link_up() runs fix_mac_speed() before set_clk_tx_rate(),
ethqos_fix_mac_speed_rgmii() always executes with the previously programmed
link_clk rate.  After this change that rate is, on the first link event,
whatever ethqos_set_clk_tx_rate() computed for SPEED_10: 5 MHz for
phy-mode "rgmii" and 2.5 MHz for "rgmii-txid"/"rgmii-rxid", instead of the
250 MHz it was before.

Only rgmii-id takes the bypass early return in
ethqos_fix_mac_speed_rgmii():

	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID) {
		rgmii_setmask(ethqos, SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
		rgmii_setmask(ethqos, SDCC_USR_CTL_DDR_BYPASS, SDCC_USR_CTL);

		ethqos_rgmii_id_macro_init(ethqos, speed);

		return;
	}

rgmii, rgmii-txid and rgmii-rxid fall through to the DLL enable and lock
poll, and at 1G also to ethqos_dll_configure():

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

ethqos_dll_configure() programs a calibration constant that is explicitly a
function of the MCLK frequency:

	rgmii_updatel(ethqos, SDCC_DLL_CONFIG2_MCLK_FREQ_CALC,
		      FIELD_PREP(SDCC_DLL_CONFIG2_MCLK_FREQ_CALC, 26),
		      SDCC_HC_REG_DLL_CONFIG2);

Can the DLL lock and calibrate at 2.5/5 MHz?  A lock timeout is only logged
with dev_err() and the link is brought up anyway, and nothing re-runs the
DLL sequence after set_clk_tx_rate() raises the rate, so the calibration
performed at the stale rate is the one that stays in effect.  There is also
up to ~3 seconds of busy-wait from the three 1 s read_poll_timeout_atomic()
budgets in this path.

arch/arm64/boot/dts/qcom/sa8540p-ride.dts uses phy-mode = "rgmii-txid" on
both ethqos ports (ethernet1 with a fixed-link at 1000), so the very first
link-up on that board takes this path.  Would raising the rate before
calling fix_mac_speed(), or restricting this initialisation change to the
DLL-bypass mode, avoid that?

[Severity: High]
Separately, does the rate computed here match the delay mode that
ethqos_fix_mac_speed_rgmii() programs for rgmii-txid and rgmii-rxid?

ethqos_set_clk_tx_rate() doubles the rate only for plain RGMII or at 1G:

	/* 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 (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
	    speed == SPEED_1000)
		rate *= 2;

But DDR bypass is only programmed for PHY_INTERFACE_MODE_RGMII_ID, so
rgmii-txid and rgmii-rxid stay in MAC-added-delay mode and, per the comment
above, need 5 MHz at 10M and 50 MHz at 100M rather than 2.5/25 MHz.

Before this series the driver did clk_set_rate(link_clk, rate * 2)
unconditionally, which matched every mode reaching the legacy path; the
condition was narrowed by "net: stmmac: qcom-ethqos: fix RGMII_ID mode to
use DLL bypass" and is unchanged at the end of the series.  Can 10/100 Mbps
links pass traffic on sa8540p-ride (phy-mode "rgmii-txid") with a
half-frequency RGMII TXC?  This patch routes the probe-time initialisation
through the same calculation, so such boards are left at the bypass 10M
rate instead of the MAC-delay 10M rate.

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

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