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

[李志]

> -----原始邮件-----
> 发件人: "Simon Horman" <horms@xxxxxxxxxx>
> 发送时间:2026-03-16 00:27:35 (星期一)
> 收件人: lizhi2@xxxxxxxxxxxxxxxxxx
> 抄送: "Simon Horman" <horms@xxxxxxxxxx>, alexandre.torgue@xxxxxxxxxxx, devicetree@xxxxxxxxxxxxxxx, alex@xxxxxxxx, linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx, ningyu@xxxxxxxxxxxxxxxxxx, linux-riscv@xxxxxxxxxxxxxxxxxxx, krzk+dt@xxxxxxxxxx, davem@xxxxxxxxxxxxx, andrew+netdev@xxxxxxx, conor+dt@xxxxxxxxxx, weishangjuan@xxxxxxxxxxxxxxxxxx, kuba@xxxxxxxxxx, robh@xxxxxxxxxx, edumazet@xxxxxxxxxx, pjw@xxxxxxxxxx, rmk+kernel@xxxxxxxxxxxxxxx, palmer@xxxxxxxxxxx, mcoquelin.stm32@xxxxxxxxx, pinkesh.vaghela@xxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, pritesh.patel@xxxxxxxxxxxxxx, pabeni@xxxxxxxxxx, aou@xxxxxxxxxxxxxxxxx, wens@xxxxxxxxxx, netdev@xxxxxxxxxxxxxxx, linmin@xxxxxxxxxxxxxxxxxx
> 主题: 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.