Re: Re: [PATCH net-next v7 2/4] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing

[李志 <lizhi2@eswincomputing.com>]

> -----原始邮件-----
> 发件人: "Jakub Kicinski" <kuba@kernel.org>
> 发送时间:2026-04-29 09:06:25 (星期三)
> 收件人: lizhi2@eswincomputing.com
> 抄送: devicetree@vger.kernel.org, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, linux-riscv@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, maxime.chevallier@bootlin.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com, horms@kernel.org
> 主题: Re: [PATCH net-next v7 2/4] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing
> 
> 
> Why Fixes? If eth1 never worked this is not a fix but new functionality
> If you want to make this a fix to prevent incompatibility - cut it down
> just to the eth0 changes.
> 
Thank you for the suggestion.

You're right that eth1 never worked at Gigabit speed, so this should
not be treated as a fix.

In v8, I will split the changes into two patches within the same series:
- Patch 1 will contain only the fixes affecting the existing eth0
functionality, and will keep the Fixes tag.
- Patch 2 will add the eth1 support (RX clock inversion workaround)
as new functionality, without a Fixes tag.

Please let me know if you would prefer a different split or ordering.

> 
> AI says:
> 
>   The step unit is being silently changed from 0.1 ns (delay_ps / 100)
>   to 0.02 ns (delay_ps / 20).  The same DT value now programs 5x the number
>   of delay steps into the hardware.
> 

This change is intentional. The previous driver used an incorrect
conversion (100ps per step), while the actual hardware step is 20ps.

This patch corrects the delay calculation to match the hardware.

The DT binding is updated accordingly to reflect the correct
20ps granularity and enforce valid values.

This binding has not yet been used in any upstream DTS, so there
are no in-tree users relying on the previous behavior.

I will clarify this explicitly in the v8 commit message.

> 
> AI says
> 
>   In the default case this logs "invalid speed %u" but then falls
>   through and still executes the regmap_write() with the base dly_param.  An
>   unsupported speed reports an error and reprograms the hardware anyway.
> 
>   Should the default path return without writing, or should the write be
>   moved into the valid cases only?
> 

The default case should not reprogram any hardware registers.

I will change the implementation so that register writes are only
performed for valid speeds (SPEED_10/100/1000). For unsupported
speeds, the function will only log the error and return without
any side effects.

> 
> AI says:
> 
>   First, rx-internal-delay-ps and tx-internal-delay-ps are silently
>   demoted from required to optional.  The previous driver returned
>   -EINVAL with "missing required property ..." when either was absent;
>   the else branches have been removed, so missing properties now leave
>   eth_clk_dly_param at zero and fix_mac_speed() programs zero delay
>   without diagnostic.  Is this intentional, and if so should the commit
>   message and the DT binding reflect it?
>  
>   Second, the previous code silently clamped out-of-range values via
>   min() and only required non-zero step units by integer truncation.
>   The new code rejects non-multiples of 20 and values greater than 2540
>   ps with -EINVAL at probe time.  DTBs that previously probed with
>   clamped delays will now fail probe.  Should this stricter validation
>   also be called out in the commit message?
> 

Yes, this change is intentional.

The previous implementation used incorrect delay semantics
(100ps step and implicit clamping), which does not match the
actual hardware behavior.

The new implementation corrects the hardware programming to use
the real 20ps step granularity and enforces valid ranges.

Regarding the DT binding change (required → optional) and the
stricter validation, this is also intentional. Zero delay is a
valid hardware configuration, and the stricter checks are added
to avoid silently masking invalid values.

Importantly, this binding has not been used by any upstream DTS
yet, so there are no in-tree users affected by this change.

I will explicitly document both the semantic change and the
compatibility notes in the v8 commit message and update the
binding description accordingly.

> 
> AI says:
> 
>   The eswin,hsp-sp-csr phandle-args layout is expanded from 4 cells
>   (phandle + 3 offsets) to 6 cells (phandle + 5 offsets), with new
>   indices 3/4/5 now required.  Any DTB produced against the original
>   ea77dbbdbc4e binding will fail probe here with -EINVAL.

The previous phandle-args layout (4 cells) was based on an
incomplete initial design and did not fully describe all required
hardware register offsets.

The updated 6-cell format reflects the complete hardware CSR layout
and allows explicit configuration of additional control registers.

Importantly, this binding has not been used by any upstream DTS,
so there are no in-tree users affected by this change.

I agree that this is a DT ABI change, and I will explicitly document
the compatibility impact and updated binding description in the v8
series to make this clear.