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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518021919.404-1-lizhi2@eswincomputing.com?part=2