Re: [PATCH net-next 5/9] net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

[netdev-bot+sashiko]

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

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

- [High] ethqos_set_clk_tx_rate() applies the new DLL-bypass (no-MAC-
  delay) link clock rate to every RGMII variant except plain "rgmii",…
- [Low] The patch is titled "fix RGMII_ID mode to use DLL bypass" and the
  changelog states the driver "currently falls through to the generic…

Pre-existing issues:
- [Medium] For PHY_INTERFACE_MODE_RGMII_RXID the PHY supplies the RX
  delay, yet that mode still takes the legacy path in…

--

net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass

For rgmii-id the PHY supplies both delays, so this powers down the DLL,
sets DDR bypass and programs the IO_MACRO through a new
ethqos_rgmii_id_macro_init() helper.  It also changes the link clock rate
calculation in ethqos_set_clk_tx_rate() and drops RGMII_ID from the
phase-shift suppression in ethqos_rgmii_macro_init().

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 44270c25d874f..67738c1598c50 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c

[ ... ]

> @@ -183,7 +186,15 @@ static int ethqos_set_clk_tx_rate(void *bsp_priv, struct clk *clk_tx_i,
>  	if (rate < 0)
>  		return rate;
>  
> -	return clk_set_rate(ethqos->link_clk, rate * 2);
> +	/* 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;
> +
> +	return clk_set_rate(ethqos->link_clk, rate);
>  }

[Severity: High]
Should this predicate match the one that actually selects the bypass
hardware configuration?

Here the un-doubled (bypass) rate is applied to every RGMII variant
except plain "rgmii", so rgmii-txid and rgmii-rxid also get the
un-doubled rate at 100M and 10M:

  ethqos_set_clk_tx_rate()
     if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII ||
         speed == SPEED_1000)
             rate *= 2;

But in ethqos_fix_mac_speed_rgmii() only PHY_INTERFACE_MODE_RGMII_ID
takes the new early return into the bypass configuration:

  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-txid and rgmii-rxid still run the full DLL init and
ethqos_rgmii_macro_init(), which sets RGMII_CONFIG_DDR_MODE and programs
PRG_RCLK_DLY, i.e. the "MAC added delay" configuration that the new
comment says needs 50 MHz at 100M and 5 MHz at 10M.  Those modes now get
25 MHz and 2.5 MHz instead, where before the patch the rate was always
doubled.

arch/arm64/boot/dts/qcom/sa8540p-ride.dts uses phy-mode = "rgmii-txid"
on both EMAC nodes, so is this a regression for that board whenever the
link comes up at 100 Mbps or 10 Mbps?

Would something like the following keep the two halves consistent?

  if (ethqos->phy_mode != PHY_INTERFACE_MODE_RGMII_ID ||
      speed == SPEED_1000)
          rate *= 2;

The condition is still unchanged at the tip of the series
(70ec5f4a19736338086794f26222dcb9ec15b24c); the later commit
"net: stmmac: qcom-ethqos: warn about legacy RGMII PHY modes" only adds a
dev_warn() on that path and does not restore the doubled rate.

>  
>  static void
> @@ -405,8 +416,7 @@ static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  		      RGMII_IO_MACRO_CONFIG2);
>  
>  	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>  		phase_shift = 0;
>  	else
>  		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;

[Severity: Medium]
This isn't a problem introduced by this patch, but was rgmii-rxid
considered while reworking this?

qcom_ethqos_probe() accepts it and routes it to the same handler:

  case PHY_INTERFACE_MODE_RGMII_RXID:
  case PHY_INTERFACE_MODE_RGMII_TXID:
          plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;

and ethqos_rgmii_macro_init() programs the MAC RX delay regardless of
phy-mode:

  rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
                FIELD_PREP(SDCC_DDR_CONFIG_PRG_RCLK_DLY,
                           prg_rclk_dly), SDCC_HC_REG_DDR_CONFIG);

  rgmii_setmask(ethqos, SDCC_DDR_CONFIG_PRG_DLY_EN,
                SDCC_HC_REG_DDR_CONFIG);

(and SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE = 5 plus the EXT_PRG_RCLK_DLY
bits at 100M/10M).  For rgmii-rxid the PHY already supplies the RX delay,
which is the same class of issue the patch fixes on the rgmii-id side.
No in-tree qcom DTS uses "rgmii-rxid", so only out-of-tree boards would
see it.  Could the changelog say whether rgmii-rxid was evaluated and
left alone deliberately?

[ ... ]

> @@ -493,6 +539,21 @@ static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  
>  	ethqos_set_func_clk_en(ethqos);
>  
> +	/* For rgmii-id mode, the PHY should add the required delays.
> +	 * Therefore, power down the DLL and program it in bypass mode.
> +	 * Program the IO_MACRO as per the settings recommended by the
> +	 * programming guide for bypass mode. This will ensure that the
> +	 * MAC core doesn't add any additional delays.
> +	 */
> +	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;
> +	}
> +

[Severity: Low]
The subject says "fix RGMII_ID mode to use DLL bypass" and the changelog
describes a user-visible timing misconfiguration, but the commit carries
only a Signed-off-by, with no Fixes: tag and no stable Cc.

Would this one be appropriate?

  Fixes: 164a9ebe9742 ("net: stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay")

That commit introduced the per-phy-mode delay handling for rgmii-id and
rgmii-txid while leaving the DLL/RX delay path enabled for rgmii-id.

>  	/* Initialize the DLL first */
>  
>  	/* Set DLL_RST */

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