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

[李志 <lizhi2@eswincomputing.com>]

> -----Original Messages-----
> From: "Maxime Chevallier" <maxime.chevallier@bootlin.com>
> Send time:Thursday, 07/05/2026 19:21:41
> To: lizhi2@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org
> Cc: ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com
> Subject: Re: [PATCH net v1 2/2] net: stmmac: eic7700: fix delay step calculation and ensure safe register initialization
> 
> Hi,
> 
> On 07/05/2026 10:32, lizhi2@eswincomputing.com wrote:
> > From: Zhi Li <lizhi2@eswincomputing.com>
> > 
> > Fix several issues in the EIC7700 DWMAC glue driver related to delay
> > configuration and register initialization.
> > 
> > The hardware implements TX/RX delay with a granularity of 20 ps per
> > step, but the driver previously assumed a 100 ps step. Update the
> > definitions to match the actual hardware behaviour and align with
> > the binding constraints.
> > 
> > Introduce explicit definitions for the maximum programmable delay
> > range based on the hardware limits.
> > 
> > Move HSP CSR configuration into the initialization path after clocks
> > are enabled. This ensures that all register accesses occur with the
> > required clocks active, avoiding undefined behaviour.
> > 
> > Clear the TXD and RXD delay control registers during initialization
> > to override any residual configuration left by the bootloader. This
> > ensures deterministic RGMII timing and prevents unintended delay
> > being applied.
> > 
> > The MAC RGMII delay programming is only required for 100Mbps and
> > 1000Mbps modes, where precise clock-to-data alignment is necessary for
> > reliable sampling.
> > 
> > For 10Mbps operation, timing margins are sufficiently relaxed and no
> > additional delay compensation is required. In this case, the driver
> > falls back to a safe default configuration with delay disabled.
> > 
> > For unsupported or unexpected link speeds, the driver avoids
> > programming invalid delay values and falls back to a safe default
> > state by explicitly clearing the delay configuration.
> > 
> > Explicitly programming zero ensures that no residual delay settings
> > from previous configurations or bootloader state remain active.
> > 
> > These changes fix incorrect delay programming and initialization
> > ordering for existing users.
> > 
> > This also aligns the driver implementation with the updated device
> > tree binding.
> 
> There's a lot going on in this patch, can you split this into patches
> that solves each of these individual issues ?
> 
> It's a mix of fixes (the reg access moved after clk config for example)
> and non-fixes (the RGMII timings, you're improving the granularity of
> the delays, is this required to fix existing setups, or is it a generic
> improvement ?), splitting this would make it both easier to review, and
> easier to bisect should problems arise in the future.
> 

Thanks for the detailed review and suggestions.

You're right that the current patch mixes several logically independent
changes, and splitting them will make the series easier to review and
bisect. I will follow your suggestion and split the current patch into
multiple smaller patches within the same series.

All five changes below are correctness fixes addressing hardware or driver
issues, not improvements or new features.

Based on the current change set, the individual fixes are:

1. TX/RX delay granularity correction (100 ps -> 20 ps step)
   This corrects an incorrect hardware capability modeling in the driver.
   The driver previously assumed a 100 ps step, while the hardware actually
   implements 20 ps granularity.
   This fixes incorrect delay programming that could occur when fine-grained
   delay values are used, ensuring correct representation of the hardware
   capability.

2. Introduce explicit maximum delay range definitions
   This fixes missing enforcement of hardware constraints, preventing invalid
   delay values from being accepted or programmed.

3. Move HSP CSR configuration after clock enable
   This fixes a register access ordering issue where accessing HSP CSR before
   clocks are enabled may result in undefined behavior during initialization.

4. Clear TXD/RXD delay control registers during initialization
   This fixes residual configuration left by bootloader state, ensuring
   deterministic behavior across reboot and driver reload.

5. Delay handling for 10Mbps and invalid link speeds
   This fixes incorrect application of RGMII delay programming outside valid
   operating modes, preventing invalid configuration from being applied.

I will split these into separate patches in the next revision, while keeping
them within the same series.

For the DT binding side, would you also recommend splitting the binding
changes to match the driver-level granularity, or would it be better to keep
them consolidated in a single binding patch?

If you have any further suggestions on the split or classification, please
let me know.

Thanks,
Zhi