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

[李志 <lizhi2@eswincomputing.com>]

> -----Original Messages-----
> From: 李志 <lizhi2@eswincomputing.com>
> Send time:Tuesday, 19/05/2026 17:58:19
> To: sashiko-reviews@lists.linux.dev
> Cc: conor+dt@kernel.org, devicetree@vger.kernel.org, robh@kernel.org
> Subject: Re: Re: [PATCH net v2 2/5] net: stmmac: eswin: fix HSP CSR init ordering after clock enable
> 
> 
> 
> 
> > -----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?

No. This was simply a mistake in the commit message. The patch only
affects the probe path where eic7700_dwmac_init() is called.

> > 
> > 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?
> > 

No. The HSP CSR registers reside in a retention domain and retain their
values across suspend/resume.

> > 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.
> > 

There is no need to call eic7700_dwmac_init() from the resume path,
since the patch only changes the probe-time initialization ordering.

Thanks for the review.