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

[Mohd Ayaan Anwar]

On Fri, Sep 11, 2026 at 11:25:14AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] The patch is worded as a bug fix ("fix RGMII_ID mode to use
>   DLL bypass", "Also fix ethqos_set_clk_tx_rate()") for two user-visible…
> 
> --
> 
> net: stmmac: qcom-ethqos: fix RGMII_ID mode to use DLL bypass
> 
> For phy-mode = "rgmii-id" the driver now powers down the DLL, sets DDR
> bypass and programs the IO_MACRO through a new
> ethqos_rgmii_id_macro_init() helper instead of running the generic DLL
> init.  It also stops doubling the link clock rate at 100M/10M in that
> mode, and drops RGMII_ID from the TX phase-shift suppression in
> ethqos_rgmii_macro_init().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > index 44270c25d874f..53fb9de4d92c7 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_ID ||
> > +	    speed == SPEED_1000)
> > +		rate *= 2;
> > +
> > +	return clk_set_rate(ethqos->link_clk, rate);
> >  }
> 
> [ ... ]
> 
> > @@ -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: Medium]
> The changelog describes this as two functional corrections, "fix RGMII_ID
> mode to use DLL bypass" and "Also fix ethqos_set_clk_tx_rate() to not
> double the clock rate in bypass mode at 100M/10M", but there is no Fixes:
> tag.  Should one be added?
> 
> Both defects look reachable before this patch.  qcom_ethqos_probe() accepts
> PHY_INTERFACE_MODE_RGMII_ID and installs the affected callback:
> 
> 	case PHY_INTERFACE_MODE_RGMII_ID:
> 	case PHY_INTERFACE_MODE_RGMII_RXID:
> 	case PHY_INTERFACE_MODE_RGMII_TXID:
> 		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_rgmii;
> 
> so an rgmii-id link previously ran the full DLL init and had the RX delay
> programmed by ethqos_rgmii_macro_init() through
> SDCC_DDR_CONFIG_PRG_RCLK_DLY / SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY, and also
> got rate * 2 from ethqos_set_clk_tx_rate().
> 
> The partial mitigation for the TX side came in with 164a9ebe9742 ("net:
> stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay"), which only
> suppressed RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN and left the RX delay and
> the doubled rate in place.  Would that commit, or the original driver
> commit, be the right Fixes: target here?
> 
> Related question: which platform reproduces the misprogramming, and on
> which hardware was the new bypass register sequence in
> ethqos_rgmii_id_macro_init() validated?  Grepping arch/arm64/boot/dts/qcom
> turns up no ethqos node using phy-mode = "rgmii-id" (only sa8540p-ride.dts,
> which uses "rgmii-txid"), so it is hard to tell from the changelog who is
> affected and what the backport risk is.  Could that be stated in the commit
> message?
> 

Since there are no current ethqos users of "rgmii-id", nothing is really
broken in the current tree, so a Fixes: tag might not be required.

	Ayaan