Re: Re: [net-next,v4,2/3] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing

[李志 <lizhi2@eswincomputing.com>]

> -----原始邮件-----
> 发件人: "Simon Horman" <horms@kernel.org>
> 发送时间:2026-03-16 00:27:35 (星期一)
> 收件人: lizhi2@eswincomputing.com
> 抄送: "Simon Horman" <horms@kernel.org>, alexandre.torgue@foss.st.com, devicetree@vger.kernel.org, alex@ghiti.fr, linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, ningyu@eswincomputing.com, linux-riscv@lists.infradead.org, krzk+dt@kernel.org, davem@davemloft.net, andrew+netdev@lunn.ch, conor+dt@kernel.org, weishangjuan@eswincomputing.com, kuba@kernel.org, robh@kernel.org, edumazet@google.com, pjw@kernel.org, rmk+kernel@armlinux.org.uk, palmer@dabbelt.com, mcoquelin.stm32@gmail.com, pinkesh.vaghela@einfochips.com, linux-kernel@vger.kernel.org, pritesh.patel@einfochips.com, pabeni@redhat.com, aou@eecs.berkeley.edu, wens@kernel.org, netdev@vger.kernel.org, linmin@eswincomputing.com
> 主题: Re: [net-next,v4,2/3] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing
> 
> The second Ethernet controller (eth1) on the Eswin EIC7700 SoC may fail
> to sample RX data correctly at Gigabit speed due to EIC7700-specific
> receive clock to data skew at the MAC input in the silicon. The existing
> internal delay configuration does not provide sufficient adjustment range
> to compensate for this condition at 1000Mbps. Update the EIC7700 DWMAC
> glue driver to apply EIC7700-specific clock sampling inversion only
> during Gigabit operation on MAC instances that require it.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > index bcb8e000e720..b230bc5b902a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> 
> [ ... ]
> 
> > @@ -61,8 +81,26 @@ static int eic7700_clks_config(void *priv, bool enabled)
> >  static int eic7700_dwmac_init(struct device *dev, void *priv)
> >  {
> >  	struct eic7700_qos_priv *dwc = priv;
> > +	int ret;
> > +
> > +	ret = eic7700_clks_config(dwc, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_set_bits(dwc->eic7700_hsp_regmap,
> > +			      dwc->eth_phy_ctrl_offset,
> > +			      EIC7700_ETH_TX_CLK_SEL |
> > +			      EIC7700_ETH_PHY_INTF_SELI);
> > +	if (ret)
> > +		return ret;
> 
> Does this code leak the enabled clocks? The function calls
> eic7700_clks_config(dwc, true) to enable the clocks, then later calls
> regmap_set_bits(). If regmap_set_bits() fails, the function returns
> without disabling the clocks.
> 
> Looking at the caller in stmmac_main.c:stmmac_dvr_probe():
> 
>     if (priv->plat->init) {
>         ret = priv->plat->init(priv->device, priv->plat->bsp_priv);
>         if (ret)
>             return ret;
>     }
> 
>     ret = __stmmac_dvr_probe(priv->device, priv->plat, &res);
>     if (ret)
>         goto error_exit_init;
> 
> The exit handler is only called when __stmmac_dvr_probe() fails, not
> when init() fails. This leaves the bulk clocks in the prepared-and-enabled
> state on this error path.
> 
Thanks for the review and for catching this.

You're right, the error path in eic7700_dwmac_init() would leak the enabled
clocks if regmap_set_bits() fails. This was an oversight during my own review.

I'll fix the error handling and include the update in the next version (v5)
of the patch series.

Thanks again.