Re: Re: [PATCH net v1 2/2] net: stmmac: eic7700: fix delay step calculation and ensure safe register initialization

[李志 <lizhi2@eswincomputing.com>]

> -----原始邮件-----
> 发件人: sashiko-bot@kernel.org
> 发送时间:2026-05-09 01:14:32 (星期六)
> 收件人: lizhi2@eswincomputing.com
> 抄送: devicetree@vger.kernel.org, conor+dt@kernel.org, robh@kernel.org, krzk+dt@kernel.org
> 主题: Re: [PATCH net v1 2/2] net: stmmac: eic7700: fix delay step calculation and ensure safe register initialization
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Unconditional ABI breakage with older Device Trees
> --
> 
> commit 8a3e405f7bc380e02d2d33cd362d35a040182e99
> Author: Zhi Li <lizhi2@eswincomputing.com>
> 
> net: stmmac: eic7700: fix delay step calculation and ensure safe register initialization
> 
> This commit updates the EIC7700 DWMAC glue driver to match hardware delay
> granularity and moves HSP CSR configuration to the initialization path. It
> also introduces new offsets and clears the TXD/RXD delay control registers
> during setup to ensure deterministic RGMII timing.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
> [ ... ]
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 3, &dwc_priv->eth_clk_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't get eth_clk_offset\n");
> >  
> > +	ret = of_property_read_u32_index(pdev->dev.of_node,
> > +					 "eswin,hsp-sp-csr",
> > +					 4, &dwc_priv->eth_txd_offset);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "can't get eth_txd_offset\n");
> 
> Will this break compatibility with older device trees that only provide 3
> indices for the eswin,hsp-sp-csr property?
> 
> If the driver boots with an older device tree, of_property_read_u32_index
> will return an error when attempting to read index 4, causing the probe to
> fail entirely and resulting in a loss of network functionality.
> 
> Could the new offsets at index 4 and 5 be made optional, or handled with a
> backward-compatible fallback, to preserve the device tree ABI?
> 

Good point, thanks for catching this.

You're right that requiring the additional offsets would break
compatibility with older device trees that only provide the original
three offsets in the eswin,hsp-sp-csr property.

To preserve the Devicetree ABI (“new kernels must continue to work with
old device trees”), I plan to make the TXD and RXD offsets optional in
both the binding and the driver.

In the binding, I plan to update eswin,hsp-sp-csr to support both the
legacy representation and the extended representation (up to 6 cells)
by adding:

- minItems: 4

and marking the last two entries as optional:

- Optional offset of register controlling TXD delay
- Optional offset of register controlling RXD delay

In the driver, if indices 4 and 5 are not present, I will fall back to
the existing shared TX/RX clock delay register offset. This ensures
older device trees remain fully functional with newer kernels.

Conor, does this look acceptable to you, and would this backward-
compatible clarification affect your previous Acked-by?

Please let me know if you see any other issues with this approach.