Re: Re: [PATCH net v2 2/5] net: stmmac: eswin: fix HSP CSR init ordering after clock enable

[李志 <lizhi2@eswincomputing.com>]

> -----Original Messages-----
> From: sashiko-bot@kernel.org
> Send time:Tuesday, 19/05/2026 10:23:35
> To: lizhi2@eswincomputing.com
> Cc: conor+dt@kernel.org, devicetree@vger.kernel.org, robh@kernel.org
> Subject: Re: [PATCH net v2 2/5] net: stmmac: eswin: fix HSP CSR init ordering after clock enable
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Hardware State Loss on Resume: The HSP CSR registers are not restored during system resume, contradicting the commit message.
> --
> 
> commit 627f510ee556ff4322e0f4ed0b60fdd27337080f
> Author: Zhi Li <lizhi2@eswincomputing.com>
> 
> net: stmmac: eswin: fix HSP CSR init ordering after clock enable
> 
> This commit moves the HSP CSR configuration in the EIC7700 DWMAC glue driver
> into the post-clock-enable initialization path to ensure proper ordering.
> The intent is to prevent register access before clock enablement and ensure
> deterministic initialization during probe or resume.
> 
> >     This change ensures deterministic initialization and prevents
> >     clock-dependent register access failures during probe or resume.
> 
> Are the HSP CSR registers actually restored during system resume?
> 
> The driver overrides the core resume callback by setting
> plat_dat->resume = eic7700_dwmac_resume. Because a custom resume callback is
> provided, the STMMAC core invokes this instead of the default
> stmmac_plat_resume() (which would have called plat->init()).
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > index bcb8e000e720b..63001c4acdb7a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> 
> [ ... ]
> 
> > @@ -61,8 +66,28 @@ 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) {
> > +		eic7700_clks_config(dwc, false);
> > +		return ret;
> > +	}
> > +
> > +	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_axi_lp_ctrl_offset,
> > +		     EIC7700_ETH_CSYSREQ_VAL);
> >  
> > -	return eic7700_clks_config(dwc, true);
> > +	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_clk_offset,
> > +		     dwc->eth_clk_dly_param);
> > +
> > +	return 0;
> >  }
> 
> Since eic7700_dwmac_resume() only calls pm_runtime_force_resume(dev) and
> doesn't invoke eic7700_dwmac_init(), the HSP CSR configuration won't be
> re-applied upon system resume. If the hardware loses state during suspend,
> will the RGMII delays be permanently lost, leading to a MAC hang or silent
> network failure?
> 
> Also, if we try to fix this by naively calling init() from resume(), would
> it cause a clock reference leak? pm_runtime_force_resume() already
> re-enables the clocks via the runtime PM callbacks, so the CSR configuration
> might need to be factored out into a separate helper function.
> 

You're right, this was simply a mistake in the commit message.

There is no resume path involved — only probe calls
eic7700_dwmac_init(), where the HSP CSR configuration is applied
after clock enablement.

I'll fix the commit message in v3 accordingly.

Thanks for the review.