[PATCH net v1 0/2] net: stmmac: eic7700: fix delay calculation and initialization ordering

[PATCH net v1 0/2] net: stmmac: eic7700: fix delay calculation and initialization ordering

[lizhi2]

From: Zhi Li <lizhi2@eswincomputing.com>

This series fixes several issues in the EIC7700 DWMAC glue driver
affecting existing eth0 functionality due to incorrect delay programming
and initialization ordering.

The previous implementation used an incorrect delay step (100 ps),
while the hardware operates with 20 ps granularity. This resulted in
incorrect programming of RX/TX delay values relative to the actual
hardware timing model.

In addition, the driver did not guarantee that clocks were enabled
before accessing HSP CSR registers, and did not explicitly clear
TXD/RXD delay registers, which may leave residual configuration from
the bootloader and affect RGMII timing determinism.

The device tree binding is updated to reflect the actual hardware delay
model and to clarify the semantics of MAC-side delay configuration,
aligning it with the real programming model without changing the
intended semantic meaning of the properties.

Changes in this series:
  - Correct delay step from 100 ps to 20 ps and validate input range
  - Ensure clocks are enabled before CSR access
  - Clear TXD/RXD delay registers during initialization
  - Update dt-binding to use range-based constraints (0-2540 ps, 20 ps step)
  - Make delay properties optional depending on RGMII mode
  - Clarify MAC-side delay semantics in binding documentation

These changes correct eth0 behavior and hardware programming correctness
for existing usage.

The previous revisions (v1-v7) mixed bug fixes and new functionality.
Based on review feedback, the changes are now split, and this series
contains only fixes targeting the net tree. Eth1 enablement will be
submitted separately to net-next.

Previous discussion:
  https://lore.kernel.org/lkml/20260427072353.1114-1-lizhi2@eswincomputing.com/

This binding update is safe as there are currently no in-tree users
relying on the previous enum-based representation.

Zhi Li (2):
  dt-bindings: ethernet: eswin: refine delay model and HSP register
    description
  net: stmmac: eic7700: fix delay step calculation and ensure safe
    register initialization

 .../bindings/net/eswin,eic7700-eth.yaml       |  50 ++++--
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 154 +++++++++++++-----
 2 files changed, 148 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[lizhi2]

From: Zhi Li <lizhi2@eswincomputing.com>

Refine the EIC7700 Ethernet dt-binding based on observed hardware behavior
and clarify the original delay model for eth0.

The previous binding used an enum-based definition for
rx-internal-delay-ps and tx-internal-delay-ps. Replace it with a
range-based model using:

  - minimum: 0
  - maximum: 2540
  - multipleOf: 20

This better reflects the actual hardware implementation, which
supports 20ps granularity delay steps in the MAC RGMII interface.

The tx/rx internal delay values are clarified as MAC-side programmable
delay components applied on the RGMII clock/data path, representing
the effective delay seen at the MAC interface.

This does not change the intended hardware semantics, but aligns the
binding with the actual hardware implementation.

These properties are optional and only required when MAC-side fine
tuning is needed; otherwise delay alignment is provided by PHY or
board design.

Depending on the selected RGMII timing mode, delay alignment may be
provided by the PHY (e.g. rgmii-id) or by board/MAC-side configuration.
When PHY or board design already provides the required delay, these
MAC-side properties may be omitted. When MAC-side fine tuning is
required, they should be provided to describe the internal RGMII
timing adjustment.

Additionally, extend the description of the HSP subsystem register
layout used by the MAC glue logic. This includes explicit TXD and RXD
delay control registers to ensure deterministic initialization and
to override any residual configuration potentially left by bootloaders.

Add reference to the EIC7700X SoC Technical Reference Manual,
Chapter 10 ("High-Speed Interface"), Part 4 for background of the
HSP CSR block:
https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases

There are no in-tree users of this binding, so no ABI impact is
expected.

Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
---
 .../bindings/net/eswin,eic7700-eth.yaml       | 50 +++++++++++++------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
index 91e8cd1db67b..fab95603bd82 100644
--- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
+++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
@@ -63,16 +63,39 @@ properties:
       - const: stmmaceth
 
   rx-internal-delay-ps:
-    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+    minimum: 0
+    maximum: 2540
+    multipleOf: 20
+    description:
+      RX internal delay in picoseconds applied on the RGMII clock at the MAC
+      side. The hardware supports 20 ps steps.
+      This property is optional and only needed when MAC-side delay tuning
+      is required.
 
   tx-internal-delay-ps:
-    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
+    minimum: 0
+    maximum: 2540
+    multipleOf: 20
+    description:
+      TX internal delay in picoseconds applied on the RGMII clock at the MAC
+      side. The hardware supports 20 ps steps.
+      This property is optional and only needed when MAC-side delay tuning
+      is required.
 
   eswin,hsp-sp-csr:
     description:
       HSP CSR is to control and get status of different high-speed peripherals
       (such as Ethernet, USB, SATA, etc.) via register, which can tune
       board-level's parameters of PHY, etc.
+
+      Additional background information about the High-Speed Subsystem
+      and the HSP CSR block is available in Chapter 10 ("High-Speed Interface")
+      of the EIC7700X SoC Technical Reference Manual, Part 4
+      (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf). The manual is
+      publicly available at
+      https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
+
+      This reference is provided for background information only.
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
       - items:
@@ -82,6 +105,8 @@ properties:
           - description: Offset of AXI clock controller Low-Power request
                          register
           - description: Offset of register controlling TX/RX clock delay
+          - description: Offset of register controlling TXD delay
+          - description: Offset of register controlling RXD delay
 
 required:
   - compatible
@@ -93,8 +118,6 @@ required:
   - phy-mode
   - resets
   - reset-names
-  - rx-internal-delay-ps
-  - tx-internal-delay-ps
   - eswin,hsp-sp-csr
 
 unevaluatedProperties: false
@@ -104,24 +127,23 @@ examples:
     ethernet@50400000 {
         compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
         reg = <0x50400000 0x10000>;
-        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
-                <&d0_clock 193>;
-        clock-names = "axi", "cfg", "stmmaceth", "tx";
         interrupt-parent = <&plic>;
         interrupts = <61>;
         interrupt-names = "macirq";
-        phy-mode = "rgmii-id";
-        phy-handle = <&phy0>;
+        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
+                <&d0_clock 193>;
+        clock-names = "axi", "cfg", "stmmaceth", "tx";
         resets = <&reset 95>;
         reset-names = "stmmaceth";
-        rx-internal-delay-ps = <200>;
-        tx-internal-delay-ps = <200>;
-        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
-        snps,axi-config = <&stmmac_axi_setup>;
+        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118 0x114 0x11c>;
+        phy-handle = <&phy0>;
+        phy-mode = "rgmii-id";
         snps,aal;
         snps,fixed-burst;
         snps,tso;
-        stmmac_axi_setup: stmmac-axi-config {
+        snps,axi-config = <&stmmac_axi_setup_gmac0>;
+
+        stmmac_axi_setup_gmac0: stmmac-axi-config {
             snps,blen = <0 0 0 0 16 8 4>;
             snps,rd_osr_lmt = <2>;
             snps,wr_osr_lmt = <2>;
-- 
2.25.1

Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[Andrew Lunn]

>      ethernet@50400000 {
>          compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
>          reg = <0x50400000 0x10000>;
> -        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> -                <&d0_clock 193>;
> -        clock-names = "axi", "cfg", "stmmaceth", "tx";
>          interrupt-parent = <&plic>;
>          interrupts = <61>;
>          interrupt-names = "macirq";
> -        phy-mode = "rgmii-id";
> -        phy-handle = <&phy0>;
> +        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> +                <&d0_clock 193>;
> +        clock-names = "axi", "cfg", "stmmaceth", "tx";

Please don't move the clocks around, since they have nothing to do
with RGMII delays.


>          resets = <&reset 95>;
>          reset-names = "stmmaceth";
> -        rx-internal-delay-ps = <200>;
> -        tx-internal-delay-ps = <200>;
> -        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
> -        snps,axi-config = <&stmmac_axi_setup>;
> +        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118 0x114 0x11c>;
> +        phy-handle = <&phy0>;
> +        phy-mode = "rgmii-id";
>          snps,aal;
>          snps,fixed-burst;
>          snps,tso;
> -        stmmac_axi_setup: stmmac-axi-config {
> +        snps,axi-config = <&stmmac_axi_setup_gmac0>;
> +
> +        stmmac_axi_setup_gmac0: stmmac-axi-config {

And what do these changes have to do with RGMII delays?

    Andrew

---
pw-bot: cr

Re: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[李志]

> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2026-05-07 20:29:10 (星期四)
> 收件人: 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, maxime.chevallier@bootlin.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com
> 主题: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description
> 
> >      ethernet@50400000 {
> >          compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
> >          reg = <0x50400000 0x10000>;
> > -        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> > -                <&d0_clock 193>;
> > -        clock-names = "axi", "cfg", "stmmaceth", "tx";
> >          interrupt-parent = <&plic>;
> >          interrupts = <61>;
> >          interrupt-names = "macirq";
> > -        phy-mode = "rgmii-id";
> > -        phy-handle = <&phy0>;
> > +        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> > +                <&d0_clock 193>;
> > +        clock-names = "axi", "cfg", "stmmaceth", "tx";
> 
> Please don't move the clocks around, since they have nothing to do
> with RGMII delays.
> 
> 
> >          resets = <&reset 95>;
> >          reset-names = "stmmaceth";
> > -        rx-internal-delay-ps = <200>;
> > -        tx-internal-delay-ps = <200>;
> > -        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
> > -        snps,axi-config = <&stmmac_axi_setup>;
> > +        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118 0x114 0x11c>;
> > +        phy-handle = <&phy0>;
> > +        phy-mode = "rgmii-id";
> >          snps,aal;
> >          snps,fixed-burst;
> >          snps,tso;
> > -        stmmac_axi_setup: stmmac-axi-config {
> > +        snps,axi-config = <&stmmac_axi_setup_gmac0>;
> > +
> > +        stmmac_axi_setup_gmac0: stmmac-axi-config {
> 
> And what do these changes have to do with RGMII delays?
> 

You're right, those unrelated example changes should not be mixed into the
fix-related binding update.

I will limit the binding changes to only what is required for the fixes,
such as the additional HSP CSR offsets needed for explicit TXD/RXD delay
register initialization, and drop the unrelated DTS example reordering or
cleanup changes from this series.

Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6710 bytes --]

On Thu, May 07, 2026 at 04:31:36PM +0800, lizhi2@eswincomputing.com wrote:
> From: Zhi Li <lizhi2@eswincomputing.com>
> 
> Refine the EIC7700 Ethernet dt-binding based on observed hardware behavior
> and clarify the original delay model for eth0.
> 
> The previous binding used an enum-based definition for
> rx-internal-delay-ps and tx-internal-delay-ps. Replace it with a
> range-based model using:
> 
>   - minimum: 0
>   - maximum: 2540
>   - multipleOf: 20
> 
> This better reflects the actual hardware implementation, which
> supports 20ps granularity delay steps in the MAC RGMII interface.
> 
> The tx/rx internal delay values are clarified as MAC-side programmable
> delay components applied on the RGMII clock/data path, representing
> the effective delay seen at the MAC interface.
> 
> This does not change the intended hardware semantics, but aligns the
> binding with the actual hardware implementation.
> 
> These properties are optional and only required when MAC-side fine
> tuning is needed; otherwise delay alignment is provided by PHY or
> board design.
> 
> Depending on the selected RGMII timing mode, delay alignment may be
> provided by the PHY (e.g. rgmii-id) or by board/MAC-side configuration.
> When PHY or board design already provides the required delay, these
> MAC-side properties may be omitted. When MAC-side fine tuning is
> required, they should be provided to describe the internal RGMII
> timing adjustment.
> 
> Additionally, extend the description of the HSP subsystem register
> layout used by the MAC glue logic. This includes explicit TXD and RXD
> delay control registers to ensure deterministic initialization and
> to override any residual configuration potentially left by bootloaders.
> 
> Add reference to the EIC7700X SoC Technical Reference Manual,
> Chapter 10 ("High-Speed Interface"), Part 4 for background of the
> HSP CSR block:
> https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
> 
> There are no in-tree users of this binding, so no ABI impact is
> expected.
> 
> Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> ---

While this is v1, it's really v8 and there should therefore be a
changelog that explains where my ack and the new compatible went.

Cheers,
Conor.

>  .../bindings/net/eswin,eic7700-eth.yaml       | 50 +++++++++++++------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> index 91e8cd1db67b..fab95603bd82 100644
> --- a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> @@ -63,16 +63,39 @@ properties:
>        - const: stmmaceth
>  
>    rx-internal-delay-ps:
> -    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> +    minimum: 0
> +    maximum: 2540
> +    multipleOf: 20
> +    description:
> +      RX internal delay in picoseconds applied on the RGMII clock at the MAC
> +      side. The hardware supports 20 ps steps.
> +      This property is optional and only needed when MAC-side delay tuning
> +      is required.
>  
>    tx-internal-delay-ps:
> -    enum: [0, 200, 600, 1200, 1600, 1800, 2000, 2200, 2400]
> +    minimum: 0
> +    maximum: 2540
> +    multipleOf: 20
> +    description:
> +      TX internal delay in picoseconds applied on the RGMII clock at the MAC
> +      side. The hardware supports 20 ps steps.
> +      This property is optional and only needed when MAC-side delay tuning
> +      is required.
>  
>    eswin,hsp-sp-csr:
>      description:
>        HSP CSR is to control and get status of different high-speed peripherals
>        (such as Ethernet, USB, SATA, etc.) via register, which can tune
>        board-level's parameters of PHY, etc.
> +
> +      Additional background information about the High-Speed Subsystem
> +      and the HSP CSR block is available in Chapter 10 ("High-Speed Interface")
> +      of the EIC7700X SoC Technical Reference Manual, Part 4
> +      (EIC7700X_SoC_Technical_Reference_Manual_Part4.pdf). The manual is
> +      publicly available at
> +      https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
> +
> +      This reference is provided for background information only.
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      items:
>        - items:
> @@ -82,6 +105,8 @@ properties:
>            - description: Offset of AXI clock controller Low-Power request
>                           register
>            - description: Offset of register controlling TX/RX clock delay
> +          - description: Offset of register controlling TXD delay
> +          - description: Offset of register controlling RXD delay
>  
>  required:
>    - compatible
> @@ -93,8 +118,6 @@ required:
>    - phy-mode
>    - resets
>    - reset-names
> -  - rx-internal-delay-ps
> -  - tx-internal-delay-ps
>    - eswin,hsp-sp-csr
>  
>  unevaluatedProperties: false
> @@ -104,24 +127,23 @@ examples:
>      ethernet@50400000 {
>          compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
>          reg = <0x50400000 0x10000>;
> -        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> -                <&d0_clock 193>;
> -        clock-names = "axi", "cfg", "stmmaceth", "tx";
>          interrupt-parent = <&plic>;
>          interrupts = <61>;
>          interrupt-names = "macirq";
> -        phy-mode = "rgmii-id";
> -        phy-handle = <&phy0>;
> +        clocks = <&d0_clock 186>, <&d0_clock 171>, <&d0_clock 40>,
> +                <&d0_clock 193>;
> +        clock-names = "axi", "cfg", "stmmaceth", "tx";
>          resets = <&reset 95>;
>          reset-names = "stmmaceth";
> -        rx-internal-delay-ps = <200>;
> -        tx-internal-delay-ps = <200>;
> -        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118>;
> -        snps,axi-config = <&stmmac_axi_setup>;
> +        eswin,hsp-sp-csr = <&hsp_sp_csr 0x100 0x108 0x118 0x114 0x11c>;
> +        phy-handle = <&phy0>;
> +        phy-mode = "rgmii-id";
>          snps,aal;
>          snps,fixed-burst;
>          snps,tso;
> -        stmmac_axi_setup: stmmac-axi-config {
> +        snps,axi-config = <&stmmac_axi_setup_gmac0>;
> +
> +        stmmac_axi_setup_gmac0: stmmac-axi-config {
>              snps,blen = <0 0 0 0 16 8 4>;
>              snps,rd_osr_lmt = <2>;
>              snps,wr_osr_lmt = <2>;
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[李志]

> -----原始邮件-----
> 发件人: "Conor Dooley" <conor@kernel.org>
> 发送时间:2026-05-08 01:24:02 (星期五)
> 收件人: 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, maxime.chevallier@bootlin.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com
> 主题: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description
> 
> On Thu, May 07, 2026 at 04:31:36PM +0800, lizhi2@eswincomputing.com wrote:
> > From: Zhi Li <lizhi2@eswincomputing.com>
> > 
> > Refine the EIC7700 Ethernet dt-binding based on observed hardware behavior
> > and clarify the original delay model for eth0.
> > 
> > The previous binding used an enum-based definition for
> > rx-internal-delay-ps and tx-internal-delay-ps. Replace it with a
> > range-based model using:
> > 
> >   - minimum: 0
> >   - maximum: 2540
> >   - multipleOf: 20
> > 
> > This better reflects the actual hardware implementation, which
> > supports 20ps granularity delay steps in the MAC RGMII interface.
> > 
> > The tx/rx internal delay values are clarified as MAC-side programmable
> > delay components applied on the RGMII clock/data path, representing
> > the effective delay seen at the MAC interface.
> > 
> > This does not change the intended hardware semantics, but aligns the
> > binding with the actual hardware implementation.
> > 
> > These properties are optional and only required when MAC-side fine
> > tuning is needed; otherwise delay alignment is provided by PHY or
> > board design.
> > 
> > Depending on the selected RGMII timing mode, delay alignment may be
> > provided by the PHY (e.g. rgmii-id) or by board/MAC-side configuration.
> > When PHY or board design already provides the required delay, these
> > MAC-side properties may be omitted. When MAC-side fine tuning is
> > required, they should be provided to describe the internal RGMII
> > timing adjustment.
> > 
> > Additionally, extend the description of the HSP subsystem register
> > layout used by the MAC glue logic. This includes explicit TXD and RXD
> > delay control registers to ensure deterministic initialization and
> > to override any residual configuration potentially left by bootloaders.
> > 
> > Add reference to the EIC7700X SoC Technical Reference Manual,
> > Chapter 10 ("High-Speed Interface"), Part 4 for background of the
> > HSP CSR block:
> > https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
> > 
> > There are no in-tree users of this binding, so no ABI impact is
> > expected.
> > 
> > Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
> > Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> > ---
> 
> While this is v1, it's really v8 and there should therefore be a
> changelog that explains where my ack and the new compatible went.
> 

Thanks for the review.

Based on Jakub's feedback on the previous v7 series, I plan to split the
changes into two separate series:

- a smaller fix series intended for net,
- and a separate eth1 feature series intended for net-next.

After the split, the scope and target trees of the two series will differ
from the original combined series, so I plan to restart the revision
numbering from v1 for both series.

The additional compatible string and the eth1-specific DT binding
extensions will be moved into the separate feature series, and I will
reflect this in the v2 cover letter.

The DT binding changes in this fix series v1 are simply extracted from the
previous v7 series as part of the split.

Since the series has been restructured, I will drop the previous
Acked-by tags.

I will also document the reason for doing so and the impact of the split
in the v2 cover letter.

If you think the binding changes are still effectively unchanged and the
previous Acked-by can still apply, I am happy to retain them or re-apply
them as appropriate. Otherwise I will assume a fresh review is preferred.

Please let me know your preference.

Thanks,
Zhi

Re: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]

On Fri, May 08, 2026 at 01:43:23PM +0800, 李志 wrote:
> 
> 
> 
> > -----原始邮件-----
> > 发件人: "Conor Dooley" <conor@kernel.org>
> > 发送时间:2026-05-08 01:24:02 (星期五)
> > 收件人: 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, maxime.chevallier@bootlin.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com
> > 主题: Re: [PATCH net v1 1/2] dt-bindings: ethernet: eswin: refine delay model and HSP register description
> > 
> > On Thu, May 07, 2026 at 04:31:36PM +0800, lizhi2@eswincomputing.com wrote:
> > > From: Zhi Li <lizhi2@eswincomputing.com>
> > > 
> > > Refine the EIC7700 Ethernet dt-binding based on observed hardware behavior
> > > and clarify the original delay model for eth0.
> > > 
> > > The previous binding used an enum-based definition for
> > > rx-internal-delay-ps and tx-internal-delay-ps. Replace it with a
> > > range-based model using:
> > > 
> > >   - minimum: 0
> > >   - maximum: 2540
> > >   - multipleOf: 20
> > > 
> > > This better reflects the actual hardware implementation, which
> > > supports 20ps granularity delay steps in the MAC RGMII interface.
> > > 
> > > The tx/rx internal delay values are clarified as MAC-side programmable
> > > delay components applied on the RGMII clock/data path, representing
> > > the effective delay seen at the MAC interface.
> > > 
> > > This does not change the intended hardware semantics, but aligns the
> > > binding with the actual hardware implementation.
> > > 
> > > These properties are optional and only required when MAC-side fine
> > > tuning is needed; otherwise delay alignment is provided by PHY or
> > > board design.
> > > 
> > > Depending on the selected RGMII timing mode, delay alignment may be
> > > provided by the PHY (e.g. rgmii-id) or by board/MAC-side configuration.
> > > When PHY or board design already provides the required delay, these
> > > MAC-side properties may be omitted. When MAC-side fine tuning is
> > > required, they should be provided to describe the internal RGMII
> > > timing adjustment.
> > > 
> > > Additionally, extend the description of the HSP subsystem register
> > > layout used by the MAC glue logic. This includes explicit TXD and RXD
> > > delay control registers to ensure deterministic initialization and
> > > to override any residual configuration potentially left by bootloaders.
> > > 
> > > Add reference to the EIC7700X SoC Technical Reference Manual,
> > > Chapter 10 ("High-Speed Interface"), Part 4 for background of the
> > > HSP CSR block:
> > > https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual/releases
> > > 
> > > There are no in-tree users of this binding, so no ABI impact is
> > > expected.
> > > 
> > > Fixes: 888bd0eca93c ("dt-bindings: ethernet: eswin: Document for EIC7700 SoC")
> > > Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> > > ---
> > 
> > While this is v1, it's really v8 and there should therefore be a
> > changelog that explains where my ack and the new compatible went.
> > 
> 
> Thanks for the review.
> 
> Based on Jakub's feedback on the previous v7 series, I plan to split the
> changes into two separate series:
> 
> - a smaller fix series intended for net,
> - and a separate eth1 feature series intended for net-next.
> 
> After the split, the scope and target trees of the two series will differ
> from the original combined series, so I plan to restart the revision
> numbering from v1 for both series.
> 
> The additional compatible string and the eth1-specific DT binding
> extensions will be moved into the separate feature series, and I will
> reflect this in the v2 cover letter.
> 
> The DT binding changes in this fix series v1 are simply extracted from the
> previous v7 series as part of the split.
> 
> Since the series has been restructured, I will drop the previous
> Acked-by tags.
> 
> I will also document the reason for doing so and the impact of the split
> in the v2 cover letter.

This isn't what I am looking for. I just want/wanted an explanation for
why the new compatible has been removed from the patch.

> 
> If you think the binding changes are still effectively unchanged and the
> previous Acked-by can still apply, I am happy to retain them or re-apply
> them as appropriate. Otherwise I will assume a fresh review is preferred.

If the removed compatible is the one for eth1, then
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> 
> Please let me know your preference.
> 
> Thanks,
> Zhi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[lizhi2]

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.

Fixes: ea77dbbdbc4e ("net: stmmac: add Eswin EIC7700 glue driver")
Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
---
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 154 +++++++++++++-----
 1 file changed, 112 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
index bcb8e000e720..0f1c62062797 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
@@ -28,20 +28,31 @@
 
 /*
  * TX/RX Clock Delay Bit Masks:
- * - TX Delay: bits [14:8] — TX_CLK delay (unit: 0.1ns per bit)
- * - RX Delay: bits [30:24] — RX_CLK delay (unit: 0.1ns per bit)
+ * - TX Delay: bits [14:8] — TX_CLK delay (unit: 0.02ns per bit)
+ * - RX Delay: bits [30:24] — RX_CLK delay (unit: 0.02ns per bit)
  */
 #define EIC7700_ETH_TX_ADJ_DELAY	GENMASK(14, 8)
 #define EIC7700_ETH_RX_ADJ_DELAY	GENMASK(30, 24)
 
-#define EIC7700_MAX_DELAY_UNIT 0x7F
+#define EIC7700_MAX_DELAY_STEPS		0x7F
+#define EIC7700_DELAY_STEP_PS		20
+#define EIC7700_MAX_DELAY_PS	\
+	(EIC7700_MAX_DELAY_STEPS * EIC7700_DELAY_STEP_PS)
 
 static const char * const eic7700_clk_names[] = {
 	"tx", "axi", "cfg",
 };
 
 struct eic7700_qos_priv {
+	struct device *dev;
 	struct plat_stmmacenet_data *plat_dat;
+	struct regmap *eic7700_hsp_regmap;
+	u32 eth_axi_lp_ctrl_offset;
+	u32 eth_phy_ctrl_offset;
+	u32 eth_txd_offset;
+	u32 eth_clk_offset;
+	u32 eth_rxd_offset;
+	u32 eth_clk_dly_param;
 };
 
 static int eic7700_clks_config(void *priv, bool enabled)
@@ -61,8 +72,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);
+
+	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_txd_offset, 0);
+	regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_rxd_offset, 0);
 
-	return eic7700_clks_config(dwc, true);
+	return 0;
 }
 
 static void eic7700_dwmac_exit(struct device *dev, void *priv)
@@ -88,18 +119,38 @@ static int eic7700_dwmac_resume(struct device *dev, void *priv)
 	return ret;
 }
 
+static void eic7700_dwmac_fix_speed(void *priv, phy_interface_t interface,
+				    int speed, unsigned int mode)
+{
+	struct eic7700_qos_priv *dwc = (struct eic7700_qos_priv *)priv;
+	bool needs_calibration = false;
+
+	switch (speed) {
+	case SPEED_1000:
+	case SPEED_100:
+		needs_calibration = true;
+		fallthrough;
+	case SPEED_10:
+		break;
+	default:
+		dev_err(dwc->dev, "invalid speed %u\n", speed);
+		break;
+	}
+
+	if (needs_calibration) {
+		regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_clk_offset,
+			     dwc->eth_clk_dly_param);
+	} else {
+		regmap_write(dwc->eic7700_hsp_regmap, dwc->eth_clk_offset, 0);
+	}
+}
+
 static int eic7700_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
 	struct stmmac_resources stmmac_res;
 	struct eic7700_qos_priv *dwc_priv;
-	struct regmap *eic7700_hsp_regmap;
-	u32 eth_axi_lp_ctrl_offset;
-	u32 eth_phy_ctrl_offset;
-	u32 eth_phy_ctrl_regset;
-	u32 eth_rxd_dly_offset;
-	u32 eth_dly_param = 0;
-	u32 delay_ps;
+	u32 delay_ps, val;
 	int i, ret;
 
 	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
@@ -116,70 +167,88 @@ static int eic7700_dwmac_probe(struct platform_device *pdev)
 	if (!dwc_priv)
 		return -ENOMEM;
 
+	dwc_priv->dev = &pdev->dev;
+
 	/* Read rx-internal-delay-ps and update rx_clk delay */
 	if (!of_property_read_u32(pdev->dev.of_node,
 				  "rx-internal-delay-ps", &delay_ps)) {
-		u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
+		if (delay_ps % EIC7700_DELAY_STEP_PS)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+				"rx delay must be multiple of %dps\n",
+				EIC7700_DELAY_STEP_PS);
 
-		eth_dly_param &= ~EIC7700_ETH_RX_ADJ_DELAY;
-		eth_dly_param |= FIELD_PREP(EIC7700_ETH_RX_ADJ_DELAY, val);
-	} else {
-		return dev_err_probe(&pdev->dev, -EINVAL,
-			"missing required property rx-internal-delay-ps\n");
+		if (delay_ps > EIC7700_MAX_DELAY_PS)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+				"rx delay out of range\n");
+
+		val = delay_ps / EIC7700_DELAY_STEP_PS;
+
+		dwc_priv->eth_clk_dly_param &= ~EIC7700_ETH_RX_ADJ_DELAY;
+		dwc_priv->eth_clk_dly_param |=
+				 FIELD_PREP(EIC7700_ETH_RX_ADJ_DELAY, val);
 	}
 
 	/* Read tx-internal-delay-ps and update tx_clk delay */
 	if (!of_property_read_u32(pdev->dev.of_node,
 				  "tx-internal-delay-ps", &delay_ps)) {
-		u32 val = min(delay_ps / 100, EIC7700_MAX_DELAY_UNIT);
+		if (delay_ps % EIC7700_DELAY_STEP_PS)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+				"tx delay must be multiple of %dps\n",
+				EIC7700_DELAY_STEP_PS);
 
-		eth_dly_param &= ~EIC7700_ETH_TX_ADJ_DELAY;
-		eth_dly_param |= FIELD_PREP(EIC7700_ETH_TX_ADJ_DELAY, val);
-	} else {
-		return dev_err_probe(&pdev->dev, -EINVAL,
-			"missing required property tx-internal-delay-ps\n");
+		if (delay_ps > EIC7700_MAX_DELAY_PS)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+				"tx delay out of range\n");
+
+		val = delay_ps / EIC7700_DELAY_STEP_PS;
+
+		dwc_priv->eth_clk_dly_param &= ~EIC7700_ETH_TX_ADJ_DELAY;
+		dwc_priv->eth_clk_dly_param |=
+				 FIELD_PREP(EIC7700_ETH_TX_ADJ_DELAY, val);
 	}
 
-	eic7700_hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
-							     "eswin,hsp-sp-csr");
-	if (IS_ERR(eic7700_hsp_regmap))
+	dwc_priv->eic7700_hsp_regmap =
+			syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							"eswin,hsp-sp-csr");
+	if (IS_ERR(dwc_priv->eic7700_hsp_regmap))
 		return dev_err_probe(&pdev->dev,
-				PTR_ERR(eic7700_hsp_regmap),
+				PTR_ERR(dwc_priv->eic7700_hsp_regmap),
 				"Failed to get hsp-sp-csr regmap\n");
 
 	ret = of_property_read_u32_index(pdev->dev.of_node,
 					 "eswin,hsp-sp-csr",
-					 1, &eth_phy_ctrl_offset);
+					 1, &dwc_priv->eth_phy_ctrl_offset);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
 				     "can't get eth_phy_ctrl_offset\n");
 
-	regmap_read(eic7700_hsp_regmap, eth_phy_ctrl_offset,
-		    &eth_phy_ctrl_regset);
-	eth_phy_ctrl_regset |=
-		(EIC7700_ETH_TX_CLK_SEL | EIC7700_ETH_PHY_INTF_SELI);
-	regmap_write(eic7700_hsp_regmap, eth_phy_ctrl_offset,
-		     eth_phy_ctrl_regset);
-
 	ret = of_property_read_u32_index(pdev->dev.of_node,
 					 "eswin,hsp-sp-csr",
-					 2, &eth_axi_lp_ctrl_offset);
+					 2, &dwc_priv->eth_axi_lp_ctrl_offset);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
 				     "can't get eth_axi_lp_ctrl_offset\n");
 
-	regmap_write(eic7700_hsp_regmap, eth_axi_lp_ctrl_offset,
-		     EIC7700_ETH_CSYSREQ_VAL);
+	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",
-					 3, &eth_rxd_dly_offset);
+					 4, &dwc_priv->eth_txd_offset);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret,
-				     "can't get eth_rxd_dly_offset\n");
+				     "can't get eth_txd_offset\n");
 
-	regmap_write(eic7700_hsp_regmap, eth_rxd_dly_offset,
-		     eth_dly_param);
+	ret = of_property_read_u32_index(pdev->dev.of_node,
+					 "eswin,hsp-sp-csr",
+					 5, &dwc_priv->eth_rxd_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "can't get eth_rxd_offset\n");
 
 	plat_dat->num_clks = ARRAY_SIZE(eic7700_clk_names);
 	plat_dat->clks = devm_kcalloc(&pdev->dev,
@@ -208,6 +277,7 @@ static int eic7700_dwmac_probe(struct platform_device *pdev)
 	plat_dat->exit = eic7700_dwmac_exit;
 	plat_dat->suspend = eic7700_dwmac_suspend;
 	plat_dat->resume = eic7700_dwmac_resume;
+	plat_dat->fix_mac_speed = eic7700_dwmac_fix_speed;
 
 	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
 }
-- 
2.25.1

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

[Maxime Chevallier]

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,

Maxime

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

[李志]

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

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

[sashiko-bot]

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?

> +	ret = of_property_read_u32_index(pdev->dev.of_node,
> +					 "eswin,hsp-sp-csr",
> +					 5, &dwc_priv->eth_rxd_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "can't get eth_rxd_offset\n");

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

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

[李志]

> -----原始邮件-----
> 发件人: 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.