[PATCH v3 0/2] Add driver support for Eswin eic7700 SoC ethernet controller

[PATCH v3 0/2] Add driver support for Eswin eic7700 SoC ethernet controller

[weishangjuan]

From: Shangjuan Wei <weishangjuan@eswincomputing.com>

This patch depends on the vendor prefix patch:
https://lore.kernel.org/all/20250616112316.3833343-4-pinkesh.vaghela@einfochips.com/

Updates:

  Changes in v3:
  - Updated eswin,eic7700-eth.yaml
    - Add descriptions of snps,write-questions, snps,read-questions,
      snps,burst-map attributes
    - Remove the description of reg
    - Delete snps,axi-config
  - Updated dwmac-eic7700.c
    - Simplify drivers and remove unnecessary API and DTS attribute configurations
    - Increase the mapping from tx/rx_delay_ps to private dly
  - Link to v2: https://lore.kernel.org/all/aDad+8YHEFdOIs38@mev-dev.igk.intel.com/

  Changes in v2:
  - Updated eswin,eic7700-eth.yaml
    - Add snps,dwmac in binding file
    - Chang the names of reset-names and phy-mode
  - Updated dwmac-eic7700.c
    - Remove the code related to PHY LED configuration from the MAC driver
    - Adjust the code format and driver interfaces, such as replacing kzalloc
      with devm_kzalloc, etc.
    - Use phylib instead of the GPIO API in the driver to implement the PHY
      reset function
  - Link to v1: https://lore.kernel.org/all/20250516010849.784-1-weishangjuan@eswincomputing.com/

Shangjuan Wei (2):
  dt-bindings: ethernet: eswin: Document for EIC7700 SoC
  ethernet: eswin: Add eic7700 ethernet driver

 .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 257 ++++++++++++++++++
 4 files changed, 444 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c

-- 
2.17.1

[PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[weishangjuan]

From: Shangjuan Wei <weishangjuan@eswincomputing.com>

Add ESWIN EIC7700 Ethernet controller, supporting clock
configuration, delay adjustment and speed adaptive functions.

Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
---
 .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++++++++
 1 file changed, 175 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml

diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
new file mode 100644
index 000000000000..04b4c7bfbb5b
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
@@ -0,0 +1,175 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Eswin EIC7700 SOC Eth Controller
+
+maintainers:
+  - Shuang Liang <liangshuang@eswincomputing.com>
+  - Zhi Li <lizhi2@eswincomputing.com>
+  - Shangjuan Wei <weishangjuan@eswincomputing.com>
+
+description:
+  The eth controller registers are part of the syscrg block on
+  the EIC7700 SoC.
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - eswin,eic7700-qos-eth
+  required:
+    - compatible
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: eswin,eic7700-qos-eth
+      - const: snps,dwmac-5.20
+
+  reg:
+    minItems: 1
+
+  interrupt-names:
+    const: macirq
+
+  interrupts:
+    maxItems: 1
+
+  phy-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - rgmii
+      - rgmii-rxid
+      - rgmii-txid
+      - rgmii-id
+
+  phy-handle:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Reference to the PHY device
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    minItems: 2
+    maxItems: 2
+    contains:
+      enum:
+        - stmmaceth
+        - tx
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: stmmaceth
+
+  rx-internal-delay-ps:
+    description:
+      RGMII Receive Clock Delay defined in pico seconds. This is used for
+      controllers that have configurable RX internal delays. If this
+      property is present then the MAC applies the RX delay.
+
+  tx-internal-delay-ps:
+    description:
+      RGMII Transmit Clock Delay defined in pico seconds. This is used for
+      controllers that have configurable TX internal delays. If this
+      property is present then the MAC applies the TX delay.
+
+  eswin,hsp-sp-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description: Phandle to HSP(High-Speed Peripheral) device
+      - description: Control register offset
+      - description: Status register offset
+      - description: Interrupt register offset
+    description: |
+      A phandle to hsp-sp-csr with three arguments that configure
+      HSP(High-Speed Peripheral) device. The argument one is the
+      offset of control register, the argument two is the offset
+      of status register, the argument three is the offset of
+      interrupt register.
+
+  eswin,syscrg-csr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - description:
+          Phandle to system CRG(System Clock and Reset Generator)
+          device
+      - description: Clock control register offset
+      - description: Reset control register offset
+    description: |
+      A phandle to syscrg-csr with two arguments that configure
+      CRG(System Clock and Reset Generator) device. The argument
+      one is the offset of clock control register, the argument
+      two is the offset of reset control register.
+
+  eswin,dly-hsp-reg:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    items:
+      - description: Control the delay of TXD
+      - description: Control the CLK delay of TX and RX
+      - description: Control the delay of RXD
+    description: |
+      An array to dly-hsp-reg with three arguments that
+      configure delay. The argument one is used to control the
+      delay of TXD, the argument two is used to control the
+      CLK delay of TX and RX, the argument three is used to
+      control the delay of RXD.
+
+required:
+  - compatible
+  - reg
+  - interrupt-names
+  - interrupts
+  - phy-mode
+  - rx-internal-delay-ps
+  - tx-internal-delay-ps
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - eswin,hsp-sp-csr
+  - eswin,syscrg-csr
+  - eswin,dly-hsp-reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    ethernet@50400000 {
+        compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
+        reg = <0x50400000 0x10000>;
+        interrupt-parent = <&plic>;
+        interrupt-names = "macirq";
+        interrupts = <61>;
+        phy-mode = "rgmii";
+        phy-handle = <&phy0>;
+        rx-internal-delay-ps = <9000>;
+        tx-internal-delay-ps = <2200>;
+        clocks = <&clock 417>, <&clock 418>;
+        clock-names = "stmmaceth", "tx";
+        resets = <&reset 95>;
+        reset-names = "stmmaceth";
+        eswin,hsp-sp-csr = <&hsp_sp_csr 0x1030 0x100 0x108>;
+        eswin,syscrg-csr = <&sys_crg 0x148 0x14c>;
+        eswin,dly-hsp-reg = <0x114 0x118 0x11c>;
+        snps,axi-config = <&stmmac_axi_setup>;
+        snps,fixed-burst;
+        snps,aal;
+        snps,tso;
+        stmmac_axi_setup: stmmac-axi-config {
+            snps,blen = <0 0 0 0 16 8 4>;
+            snps,rd_osr_lmt = <2>;
+            snps,wr_osr_lmt = <2>;
+        };
+    };
--
2.17.1

Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[Krzysztof Kozlowski]

On 03/07/2025 11:19, weishangjuan@eswincomputing.com wrote:
> From: Shangjuan Wei <weishangjuan@eswincomputing.com>
> 
> Add ESWIN EIC7700 Ethernet controller, supporting clock
> configuration, delay adjustment and speed adaptive functions.
> 
> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
> ---
>  .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> new file mode 100644
> index 000000000000..04b4c7bfbb5b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Eswin EIC7700 SOC Eth Controller
> +
> +maintainers:
> +  - Shuang Liang <liangshuang@eswincomputing.com>
> +  - Zhi Li <lizhi2@eswincomputing.com>
> +  - Shangjuan Wei <weishangjuan@eswincomputing.com>
> +
> +description:
> +  The eth controller registers are part of the syscrg block on
> +  the EIC7700 SoC.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - eswin,eic7700-qos-eth
> +  required:
> +    - compatible
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: eswin,eic7700-qos-eth
> +      - const: snps,dwmac-5.20
> +
> +  reg:
> +    minItems: 1

Nope. Changelog does not explain that, it is not correct and no one ever
requested something like that. See also writing bindings about constraints.

> +
> +  interrupt-names:
> +    const: macirq
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +      - rgmii
> +      - rgmii-rxid
> +      - rgmii-txid
> +      - rgmii-id
> +
> +  phy-handle:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Reference to the PHY device
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2
> +    contains:
> +      enum:
> +        - stmmaceth
> +        - tx

Not much changed, nothing explained in the changelog in cover letter.

You got already feedback that you keep pushing same code without fixing
anything. You don't respond to feedback. You don't address it.

What is left for me? Start treating us seriously. I am not going to
review the rest.

Respond to previous feedback with acknowledging that you understood it
or further questions if you did not understand it, but you made thorough
research on other bindings and example schema how to do it.

NAK

Best regards,
Krzysztof

Re: Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[韦尚娟]

Dear Krzysztof Kozlowski,

I apologize for the inconvenience caused by sending patches multiple times due to my incomplete understanding of your previous version's response. Thank you very much for reviewing our patches multiple times. Regarding your response on V3, I have two questions about YAML files that I would like to confirm with you. Can you take some time out of your busy schedule to reply to me?

1. Regarding "reg: minItems: 1"
I have reviewed the writing method from other YAML files in the source code, and they all use “reg: maxItems: 1 ” instead of
“reg: minItems: 1”. So we also need to use “reg: maxItems: 1 ” in our YAML file.
Is this understanding correct?

2. Regarding clocks and clock-names
For clocks and clock-names, from other YAML files there is no minItems and maxItems mentioned.
We will remove minItems and maxItems from clocks and clock-names and as we have fix 2 clocks,
we will also add description in clocks:items.
Ref yaml: sophgo,sg2044-dwmac.yaml, starfive,jh7110-dwmac.yaml
Let me know if this is correct? We will update in next version based on your suggestions.

3. Do we need to include all changes based on the previous version in the cover letter patch when submitting the patches?
If so, we will cover all the changes in cover letter from next time.

Look forward to your reply！




> -----原始邮件-----
> 发件人: "Krzysztof Kozlowski" <krzk@kernel.org>
> 发送时间:2025-07-03 17:51:47 (星期四)
> 收件人: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org
> 抄送: ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC
> 
> On 03/07/2025 11:19, weishangjuan@eswincomputing.com wrote:
> > From: Shangjuan Wei <weishangjuan@eswincomputing.com>
> > 
> > Add ESWIN EIC7700 Ethernet controller, supporting clock
> > configuration, delay adjustment and speed adaptive functions.
> > 
> > Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> > Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
> > ---
> >  .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++++++++
> >  1 file changed, 175 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > new file mode 100644
> > index 000000000000..04b4c7bfbb5b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > @@ -0,0 +1,175 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Eswin EIC7700 SOC Eth Controller
> > +
> > +maintainers:
> > +  - Shuang Liang <liangshuang@eswincomputing.com>
> > +  - Zhi Li <lizhi2@eswincomputing.com>
> > +  - Shangjuan Wei <weishangjuan@eswincomputing.com>
> > +
> > +description:
> > +  The eth controller registers are part of the syscrg block on
> > +  the EIC7700 SoC.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - eswin,eic7700-qos-eth
> > +  required:
> > +    - compatible
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: eswin,eic7700-qos-eth
> > +      - const: snps,dwmac-5.20
> > +
> > +  reg:
> > +    minItems: 1
> 
> Nope. Changelog does not explain that, it is not correct and no one ever
> requested something like that. See also writing bindings about constraints.
> 
> > +
> > +  interrupt-names:
> > +    const: macirq
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum:
> > +      - rgmii
> > +      - rgmii-rxid
> > +      - rgmii-txid
> > +      - rgmii-id
> > +
> > +  phy-handle:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Reference to the PHY device
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    minItems: 2
> > +    maxItems: 2
> > +    contains:
> > +      enum:
> > +        - stmmaceth
> > +        - tx
> 
> Not much changed, nothing explained in the changelog in cover letter.
> 
> You got already feedback that you keep pushing same code without fixing
> anything. You don't respond to feedback. You don't address it.
> 
> What is left for me? Start treating us seriously. I am not going to
> review the rest.
> 
> Respond to previous feedback with acknowledging that you understood it
> or further questions if you did not understand it, but you made thorough
> research on other bindings and example schema how to do it.
> 
> NAK
> 
> Best regards,
> Krzysztof

Re: Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[韦尚娟]

Hi, Krzysztof Kozlowski,

I apologize for the inconvenience caused by sending patches multiple times due to my incomplete
understanding of your previous version's response. I have two questions about YAML files that I would 
like to confirm with you. The questions are as follows.


> -----原始邮件-----
> 发件人: "Krzysztof Kozlowski" <krzk@kernel.org>
> 发送时间:2025-07-03 17:51:47 (星期四)
> 收件人: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org
> 抄送: ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC
> 
> On 03/07/2025 11:19, weishangjuan@eswincomputing.com wrote:
> > From: Shangjuan Wei <weishangjuan@eswincomputing.com>
> > 
> > Add ESWIN EIC7700 Ethernet controller, supporting clock
> > configuration, delay adjustment and speed adaptive functions.
> > 
> > Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> > Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
> > ---
> >  .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++++++++
> >  1 file changed, 175 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > new file mode 100644
> > index 000000000000..04b4c7bfbb5b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> > @@ -0,0 +1,175 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/eswin,eic7700-eth.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Eswin EIC7700 SOC Eth Controller
> > +
> > +maintainers:
> > +  - Shuang Liang <liangshuang@eswincomputing.com>
> > +  - Zhi Li <lizhi2@eswincomputing.com>
> > +  - Shangjuan Wei <weishangjuan@eswincomputing.com>
> > +
> > +description:
> > +  The eth controller registers are part of the syscrg block on
> > +  the EIC7700 SoC.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - eswin,eic7700-qos-eth
> > +  required:
> > +    - compatible
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: eswin,eic7700-qos-eth
> > +      - const: snps,dwmac-5.20
> > +
> > +  reg:
> > +    minItems: 1
> 
> Nope. Changelog does not explain that, it is not correct and no one ever
> requested something like that. See also writing bindings about constraints.

I have reviewed the writing method from other YAML files in the source code, 
and they all use “reg: maxItems: 1 ” instead of “reg: minItems: 1”. So we also
need to use “reg: maxItems: 1 ” in our YAML file. Is this understanding correct?

> > +
> > +  interrupt-names:
> > +    const: macirq
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  phy-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum:
> > +      - rgmii
> > +      - rgmii-rxid
> > +      - rgmii-txid
> > +      - rgmii-id
> > +
> > +  phy-handle:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Reference to the PHY device
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    minItems: 2
> > +    maxItems: 2
> > +    contains:
> > +      enum:
> > +        - stmmaceth
> > +        - tx
> 
> Not much changed, nothing explained in the changelog in cover letter.
> 

For clocks and clock-names, other YAML files have no minItems
and maxItems. Remove minItems and maxItems from
clocks and clock-names and as we have fix 2 clocks. Add description in clocks:items. 
Ref yaml: sophgo,sg2044-dwmac.yaml, starfive,jh7110-dwmac.yaml

All the changes will be added in cover letter in the next version. Is this understanding correct?

>
> You got already feedback that you keep pushing same code without fixing
> anything. You don't respond to feedback. You don't address it.
> 
> What is left for me? Start treating us seriously. I am not going to
> review the rest.
> 
> Respond to previous feedback with acknowledging that you understood it
> or further questions if you did not understand it, but you made thorough
> research on other bindings and example schema how to do it.
> 
> NAK
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[Krzysztof Kozlowski]

On 15/07/2025 10:54, 韦尚娟 wrote:
>>> +
>>> +allOf:
>>> +  - $ref: snps,dwmac.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: eswin,eic7700-qos-eth
>>> +      - const: snps,dwmac-5.20
>>> +
>>> +  reg:
>>> +    minItems: 1
>>
>> Nope. Changelog does not explain that, it is not correct and no one ever
>> requested something like that. See also writing bindings about constraints.
> 
> I have reviewed the writing method from other YAML files in the source code, 
> and they all use “reg: maxItems: 1 ” instead of “reg: minItems: 1”. So we also
> need to use “reg: maxItems: 1 ” in our YAML file. Is this understanding correct?

Yes, assuming you have here one entry.

> 
>>> +
>>> +  interrupt-names:
>>> +    const: macirq
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  phy-mode:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    enum:
>>> +      - rgmii
>>> +      - rgmii-rxid
>>> +      - rgmii-txid
>>> +      - rgmii-id
>>> +
>>> +  phy-handle:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: Reference to the PHY device
>>> +
>>> +  clocks:
>>> +    minItems: 2
>>> +    maxItems: 2
>>> +
>>> +  clock-names:
>>> +    minItems: 2
>>> +    maxItems: 2
>>> +    contains:
>>> +      enum:
>>> +        - stmmaceth
>>> +        - tx
>>
>> Not much changed, nothing explained in the changelog in cover letter.
>>
> 
> For clocks and clock-names, other YAML files have no minItems
> and maxItems. Remove minItems and maxItems from
> clocks and clock-names and as we have fix 2 clocks. Add description in clocks:items. 
> Ref yaml: sophgo,sg2044-dwmac.yaml, starfive,jh7110-dwmac.yaml
> 
> All the changes will be added in cover letter in the next version. Is this understanding correct?

Yes.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[Rob Herring (Arm)]

On Thu, 03 Jul 2025 17:19:47 +0800, weishangjuan@eswincomputing.com wrote:
> From: Shangjuan Wei <weishangjuan@eswincomputing.com>
> 
> Add ESWIN EIC7700 Ethernet controller, supporting clock
> configuration, delay adjustment and speed adaptive functions.
> 
> Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
> Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
> ---
>  .../bindings/net/eswin,eic7700-eth.yaml       | 175 ++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/eswin,eic7700-eth.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/eswin,eic7700-eth.example.dtb: ethernet@50400000 (eswin,eic7700-qos-eth): 'eswin,dly-hsp-reg', 'eswin,hsp-sp-csr', 'eswin,syscrg-csr' do not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pciclass|pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^GEFanuc,.*', '^IBM,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acbel,.*', '^acelink,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^adieng,.*', '^admatec,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^aldec,.*', '^alfa-network,.*', '^allegro,.*', '^allegromicro,.*', '^alliedvision,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^amphenol,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^anbernic,.*', '^andestech,.*', '^anvo,.*', '^aoly,.*', '^aosong,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arcom,.*', '^arctic,.*', '^arcx,.*', '^argon40,.*', '^ariaboard,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^armsom,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asteralabs,.*', '^asus,.*', '^atheros,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^belling,.*', '^bhf,.*', '^bigtreetech,.*', '^bitmain,.*', '^blaize,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calao,.*', '^calaosystems,.*', '^calxeda,.*', '^cameo,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cct,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^chargebyte,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chongzhou,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^clockwork,.*', '^cloos,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^comvetia,.*', '^congatec,.*', '^coolpi,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csot,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cudy,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^deepcomputing,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dfrobot,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dimonoff,.*', '^diodes,.*', '^dioo,.*', '^djn,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^dream,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^econet,.*', '^edgeble,.*', '^edimax,.*', '^edt,.*', '^ees,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embedfire,.*', '^embest,.*', '^emcraft,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairchild,.*', '^fairphone,.*', '^faraday,.*', '^fascontek,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^freebox,.*', '^freecom,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^galaxycore,.*', '^gameforce,.*', '^gardena,.*', '^gateway,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gehc,.*', '^gemei,.*', '^gemtek,.*', '^genesys,.*', '^genexis,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^glinet,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^gocontroll,.*', '^goldelico,.*', '^goodix,.*', '^google,.*', '^goramo,.*', '^gplus,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperf,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^htc,.*', '^huawei,.*', '^hugsun,.*', '^huiling,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynitron,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^iei,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^inanbo,.*', '^incircuit,.*', '^indiedroid,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innocomm,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^irondevice,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jadard,.*', '^jasonic,.*', '^jdi,.*', '^jedec,.*', '^jenson,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^jide,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^lckfb,.*', '^lctech,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^lincolntech,.*', '^lineartechnology,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liontron,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongmasses,.*', '^loongson,.*', '^lsi,.*', '^luckfox,.*', '^lunzn,.*', '^luxul,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marantec,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^maxlinear,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsensing,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^methode,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^microtips,.*', '^mikroe,.*', '^mikrotik,.*', '^milkv,.*', '^miniand,.*', '^minix,.*', '^mips,.*', '^miramems,.*', '^mitsubishi,.*', '^mitsumi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^mobileye,.*', '^modtronix,.*', '^moortec,.*', '^mosaixtech,.*', '^motorcomm,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^neardi,.*', '^nec,.*', '^neofidelity,.*', '^neonode,.*', '^netcube,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^newvision,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^nothing,.*', '^novatek,.*', '^novtech,.*', '^numonyx,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onie,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^openailab,.*', '^opencores,.*', '^openembed,.*', '^openpandora,.*', '^openrisc,.*', '^openwrt,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pegatron,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pinctrl-[0-9]+$', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^polyhex,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^powkiddy,.*', '^pri,.*', '^primeview,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^puya,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^relfor,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^retronix,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,.*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^rve,.*', '^saef,.*', '^sakurapi,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^schneider,.*', '^sciosense,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shineworld,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^siflower,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartfiber,.*', '^smartlabs,.*', '^smartrg,.*', '^smi,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^sophgo,.*', '^sourceparts,.*', '^spacemit,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^square,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^starterkit,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tcu,.*', '^tdo,.*', '^team-source-display,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^techwell,.*', '^teejet,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^test,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^thwc,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^topland,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^transpeed,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^turing,.*', '^tyan,.*', '^tyhx,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ufispace,.*', '^ugoos,.*', '^ultratronik,.*', '^uni-t,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^usr,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vialab,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^voltafield,.*', '^vot,.*', '^vscom,.*', '^vxt,.*', '^wacom,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^widora,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winsen,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^wolfvision,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^yiming,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^yuridenki,.*', '^yuzukihd,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*'
	from schema $id: http://devicetree.org/schemas/vendor-prefixes.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250703091947.1148-1-weishangjuan@eswincomputing.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 1/2] dt-bindings: ethernet: eswin: Document for EIC7700 SoC

[Andrew Lunn]

> +    ethernet@50400000 {
> +        compatible = "eswin,eic7700-qos-eth", "snps,dwmac-5.20";
> +        reg = <0x50400000 0x10000>;
> +        interrupt-parent = <&plic>;
> +        interrupt-names = "macirq";
> +        interrupts = <61>;
> +        phy-mode = "rgmii";

Please don't user 'rgmii' in examples. It is normally wrong, and we
don't want DT developers copying it into real DT binding, just for me
to tell them it is wrong.

	Andrew

[PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[weishangjuan]

From: Shangjuan Wei <weishangjuan@eswincomputing.com>

Add Ethernet controller support for Eswin's eic7700 SoC. The driver
provides management and control of Ethernet signals for the eiC7700
series chips.

Signed-off-by: Zhi Li <lizhi2@eswincomputing.com>
Signed-off-by: Shangjuan Wei <weishangjuan@eswincomputing.com>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-eic7700.c   | 257 ++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 67fa879b1e52..a13b15ce1abd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -67,6 +67,17 @@ config DWMAC_ANARION

 	  This selects the Anarion SoC glue layer support for the stmmac driver.

+config DWMAC_EIC7700
+	tristate "Support for Eswin eic7700 ethernet driver"
+	select CRC32
+	select MII
+	depends on OF && HAS_DMA && ARCH_ESWIN || COMPILE_TEST
+	help
+	  This driver supports the Eswin EIC7700 Ethernet controller,
+	  which integrates Synopsys DesignWare QoS features. It enables
+	  high-speed networking with DMA acceleration and is optimized
+	  for embedded systems.
+
 config DWMAC_INGENIC
 	tristate "Ingenic MAC support"
 	default MACH_INGENIC
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index b591d93f8503..f4ec5fc16571 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -14,6 +14,7 @@ stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
 # Ordering matters. Generic driver must be last.
 obj-$(CONFIG_STMMAC_PLATFORM)	+= stmmac-platform.o
 obj-$(CONFIG_DWMAC_ANARION)	+= dwmac-anarion.o
+obj-$(CONFIG_DWMAC_EIC7700)	+= dwmac-eic7700.o
 obj-$(CONFIG_DWMAC_INGENIC)	+= dwmac-ingenic.o
 obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
new file mode 100644
index 000000000000..000362e9987d
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-eic7700.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Eswin DWC Ethernet linux driver
+ *
+ * Copyright 2025, Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ * Authors:
+ *   Shuang Liang <liangshuang@eswincomputing.com>
+ *   Shangjuan Wei <weishangjuan@eswincomputing.com>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/stmmac.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+
+#include "stmmac_platform.h"
+
+/* eth_phy_ctrl_offset eth0:0x100; eth1:0x200 */
+#define EIC7700_ETH_TX_CLK_SEL		BIT(16)
+#define EIC7700_ETH_PHY_INTF_SELI	BIT(0)
+
+/* eth_axi_lp_ctrl_offset eth0:0x108; eth1:0x208 */
+#define EIC7700_ETH_CSYSREQ_VAL		BIT(0)
+
+/* hsp_aclk_ctrl_offset (0x148) */
+#define EIC7700_HSP_ACLK_CLKEN		BIT(31)
+#define EIC7700_HSP_ACLK_DIVSOR		(0x2 << 4)
+
+/* hsp_cfg_ctrl_offset (0x14c) */
+#define EIC7700_HSP_CFG_CLKEN		BIT(31)
+#define EIC7700_SCU_HSP_PCLK_EN		BIT(30)
+#define EIC7700_HSP_CFG_CTRL_REGSET	(EIC7700_HSP_CFG_CLKEN | EIC7700_SCU_HSP_PCLK_EN)
+
+/* TX/RX clock delay (unit: 0.1ns per bit) */
+#define EIC7700_ETH_TX_ADJ_DELAY	GENMASK(14, 8)
+#define EIC7700_ETH_RX_ADJ_DELAY	GENMASK(30, 24)
+
+/* Default delay value*/
+#define EIC7700_DELAY_VALUE0 0x20202020
+#define EIC7700_DELAY_VALUE1 0x96205A20
+
+struct eic7700_qos_priv {
+	struct device *dev;
+	struct regmap *crg_regmap;
+	struct regmap *hsp_regmap;
+	u32 tx_delay_ps;
+	u32 rx_delay_ps;
+	u32 dly_hsp_reg[3];
+	u32 dly_param_1000m[3];
+	u32 dly_param_100m[3];
+	u32 dly_param_10m[3];
+};
+
+static inline void eic7700_set_delay(u32 rx_ps, u32 tx_ps, u32 *reg)
+{
+	u32 rx_val = rx_ps / 100;
+
+	if (rx_val > 0x7F)
+		rx_val = 0x7F;
+
+	*reg &= ~EIC7700_ETH_RX_ADJ_DELAY;
+	*reg |= (rx_val << 24) & EIC7700_ETH_RX_ADJ_DELAY;
+
+	u32 tx_val = tx_ps / 100;
+
+	if (tx_val > 0x7F)
+		tx_val = 0x7F;
+
+	*reg &= ~EIC7700_ETH_TX_ADJ_DELAY;
+	*reg |= (tx_val << 8) & EIC7700_ETH_TX_ADJ_DELAY;
+}
+
+static void eic7700_qos_fix_speed(void *priv, int speed, u32 mode)
+{
+	struct eic7700_qos_priv *dwc_priv = priv;
+	int i;
+
+	switch (speed) {
+	case SPEED_1000:
+		for (i = 0; i < 3; i++)
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_1000m[i]);
+		break;
+	case SPEED_100:
+		for (i = 0; i < 3; i++) {
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_100m[i]);
+		}
+		break;
+	case SPEED_10:
+		for (i = 0; i < 3; i++) {
+			regmap_write(dwc_priv->hsp_regmap,
+				     dwc_priv->dly_hsp_reg[i],
+				     dwc_priv->dly_param_10m[i]);
+		}
+		break;
+	default:
+		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
+		break;
+	}
+}
+
+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;
+	u32 hsp_aclk_ctrl_offset;
+	u32 hsp_aclk_ctrl_regset;
+	u32 hsp_cfg_ctrl_offset;
+	u32 eth_axi_lp_ctrl_offset;
+	u32 eth_phy_ctrl_offset;
+	u32 eth_phy_ctrl_regset;
+	bool has_rx_dly = false;
+	bool has_tx_dly = false;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				"failed to get resources\n");
+
+	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
+				"dt configuration failed\n");
+
+	dwc_priv = devm_kzalloc(&pdev->dev, sizeof(*dwc_priv), GFP_KERNEL);
+	if (!dwc_priv)
+		return -ENOMEM;
+
+	dwc_priv->dev = &pdev->dev;
+	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
+	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
+	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
+	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
+	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
+	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
+	dwc_priv->dly_param_10m[0] = 0x0;
+	dwc_priv->dly_param_10m[1] = 0x0;
+	dwc_priv->dly_param_10m[2] = 0x0;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
+				   &dwc_priv->rx_delay_ps);
+	if (ret)
+		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);
+	else
+		has_rx_dly = true;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
+				   &dwc_priv->tx_delay_ps);
+	if (ret)
+		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
+	else
+		has_tx_dly = true;
+	if (has_rx_dly && has_tx_dly) {
+		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
+				  &dwc_priv->dly_param_1000m[1]);
+		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
+				  &dwc_priv->dly_param_100m[1]);
+		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
+				  &dwc_priv->dly_param_10m[1]);
+	} else {
+		dev_dbg(&pdev->dev, " use default dly\n");
+	}
+
+	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "eswin,dly_hsp_reg",
+						  &dwc_priv->dly_hsp_reg[0], 3, 0);
+	if (ret != 3) {
+		dev_err(&pdev->dev, "can't get delay hsp reg.ret(%d)\n", ret);
+		return ret;
+	}
+
+	dwc_priv->crg_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							       "eswin,syscrg_csr");
+	if (IS_ERR(dwc_priv->crg_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->crg_regmap),
+				"Failed to get syscrg_csr regmap\n");
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
+					 &hsp_aclk_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
+
+	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
+	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
+	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
+					 &hsp_cfg_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
+
+	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
+
+	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							       "eswin,hsp_sp_csr");
+	if (IS_ERR(dwc_priv->hsp_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
+				"Failed to get hsp_sp_csr regmap\n");
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,
+					 &eth_phy_ctrl_offset);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
+
+	regmap_read(dwc_priv->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(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
+
+	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
+					 &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(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
+
+	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
+	if (IS_ERR(plat_dat->clk_tx_i))
+		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat->clk_tx_i),
+				"error getting tx clock\n");
+
+	plat_dat->fix_mac_speed = eic7700_qos_fix_speed;
+	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
+	plat_dat->bsp_priv = dwc_priv;
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to driver probe\n");
+
+	return ret;
+}
+
+static const struct of_device_id eic7700_dwmac_match[] = {
+	{ .compatible = "eswin,eic7700-qos-eth" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, eic7700_dwmac_match);
+
+static struct platform_driver eic7700_dwmac_driver = {
+	.probe  = eic7700_dwmac_probe,
+	.remove = stmmac_pltfr_remove,
+	.driver = {
+		.name           = "eic7700-eth-dwmac",
+		.pm             = &stmmac_pltfr_pm_ops,
+		.of_match_table = eic7700_dwmac_match,
+	},
+};
+module_platform_driver(eic7700_dwmac_driver);
+
+MODULE_AUTHOR("Eswin");
+MODULE_AUTHOR("Shuang Liang <liangshuang@eswincomputing.com>");
+MODULE_AUTHOR("Shangjuan Wei <weishangjuan@eswincomputing.com>");
+MODULE_DESCRIPTION("Eswin eic7700 qos ethernet driver");
+MODULE_LICENSE("GPL");
--
2.17.1

Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Krzysztof Kozlowski]

On 03/07/2025 11:20, weishangjuan@eswincomputing.com wrote:
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
> +					 &hsp_aclk_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
> +	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
> +	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
> +					 &hsp_cfg_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
> +
> +	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,hsp_sp_csr");

There is no such property. I already said at v2 you cannot have
undocumented ABI.

> +	if (IS_ERR(dwc_priv->hsp_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
> +				"Failed to get hsp_sp_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,

NAK

> +					 &eth_phy_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->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(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
> +					 &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(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
> +
> +	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
> +	if (IS_ERR(plat_dat->clk_tx_i))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat->clk_tx_i),
> +				"error getting tx clock\n");
> +
> +	plat_dat->fix_mac_speed = eic7700_qos_fix_speed;
> +	plat_dat->set_clk_tx_rate = stmmac_set_clk_tx_rate;
> +	plat_dat->bsp_priv = dwc_priv;
> +
> +	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to driver probe\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id eic7700_dwmac_match[] = {
> +	{ .compatible = "eswin,eic7700-qos-eth" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, eic7700_dwmac_match);
> +
> +static struct platform_driver eic7700_dwmac_driver = {
> +	.probe  = eic7700_dwmac_probe,
> +	.remove = stmmac_pltfr_remove,
> +	.driver = {
> +		.name           = "eic7700-eth-dwmac",
> +		.pm             = &stmmac_pltfr_pm_ops,
> +		.of_match_table = eic7700_dwmac_match,
> +	},
> +};
> +module_platform_driver(eic7700_dwmac_driver);
> +
> +MODULE_AUTHOR("Eswin");

Drop, that's not a person.


Best regards,
Krzysztof

Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Russell King (Oracle)]

On Thu, Jul 03, 2025 at 05:20:15PM +0800, weishangjuan@eswincomputing.com wrote:
> +static void eic7700_qos_fix_speed(void *priv, int speed, u32 mode)
> +{
> +	struct eic7700_qos_priv *dwc_priv = priv;
> +	int i;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		for (i = 0; i < 3; i++)
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_1000m[i]);
> +		break;
> +	case SPEED_100:
> +		for (i = 0; i < 3; i++) {
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_100m[i]);
> +		}

The other two instances don't have the curley braces, why does this need
it?

> +		break;
> +	case SPEED_10:
> +		for (i = 0; i < 3; i++) {
> +			regmap_write(dwc_priv->hsp_regmap,
> +				     dwc_priv->dly_hsp_reg[i],
> +				     dwc_priv->dly_param_10m[i]);
> +		}
> +		break;
> +	default:
> +		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
> +		break;
> +	}

Overall, wouldn't:

	const u32 *dly_param;

	switch (speed) {
	case SPEED_1000:
		dly_param = dwc_priv->dly_param_1000m;
		break;
	... etc ...
	default:
		dly_param = NULL;
		dev_err(dwc_priv->dev, "invalid speed %u\n", speed);
		break;
	}

	if (dly_param)
		for (i = 0; i < 3; i++)
			regmap_write(dwc_priv->hsp_regmap,
				     dwc_priv->dly_hsp_reg[i],
				     dly_param[i]);

be more concise and easier to read?

> +}
> +
> +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;
> +	u32 hsp_aclk_ctrl_offset;
> +	u32 hsp_aclk_ctrl_regset;
> +	u32 hsp_cfg_ctrl_offset;
> +	u32 eth_axi_lp_ctrl_offset;
> +	u32 eth_phy_ctrl_offset;
> +	u32 eth_phy_ctrl_regset;
> +	bool has_rx_dly = false;
> +	bool has_tx_dly = false;
> +	int ret;
> +
> +	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				"failed to get resources\n");
> +
> +	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat_dat))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
> +				"dt configuration failed\n");
> +
> +	dwc_priv = devm_kzalloc(&pdev->dev, sizeof(*dwc_priv), GFP_KERNEL);
> +	if (!dwc_priv)
> +		return -ENOMEM;
> +
> +	dwc_priv->dev = &pdev->dev;
> +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_10m[0] = 0x0;
> +	dwc_priv->dly_param_10m[1] = 0x0;
> +	dwc_priv->dly_param_10m[2] = 0x0;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
> +				   &dwc_priv->rx_delay_ps);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);

Consider using %pe and ERR_PTR(ret) so that error codes can be
translated to human readable strings. Ditto elsewhere.

> +	else
> +		has_rx_dly = true;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
> +				   &dwc_priv->tx_delay_ps);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
> +	else
> +		has_tx_dly = true;
> +	if (has_rx_dly && has_tx_dly) {
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_1000m[1]);
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_100m[1]);
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_10m[1]);
> +	} else {
> +		dev_dbg(&pdev->dev, " use default dly\n");
> +	}
> +
> +	ret = of_property_read_variable_u32_array(pdev->dev.of_node, "eswin,dly_hsp_reg",
> +						  &dwc_priv->dly_hsp_reg[0], 3, 0);
> +	if (ret != 3) {
> +		dev_err(&pdev->dev, "can't get delay hsp reg.ret(%d)\n", ret);
> +		return ret;
> +	}
> +
> +	dwc_priv->crg_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,syscrg_csr");
> +	if (IS_ERR(dwc_priv->crg_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->crg_regmap),
> +				"Failed to get syscrg_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 1,
> +					 &hsp_aclk_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_aclk_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, &hsp_aclk_ctrl_regset);
> +	hsp_aclk_ctrl_regset |= (EIC7700_HSP_ACLK_CLKEN | EIC7700_HSP_ACLK_DIVSOR);
> +	regmap_write(dwc_priv->crg_regmap, hsp_aclk_ctrl_offset, hsp_aclk_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,syscrg_csr", 2,
> +					 &hsp_cfg_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get hsp_cfg_ctrl_offset\n");
> +
> +	regmap_write(dwc_priv->crg_regmap, hsp_cfg_ctrl_offset, EIC7700_HSP_CFG_CTRL_REGSET);
> +
> +	dwc_priv->hsp_regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							       "eswin,hsp_sp_csr");
> +	if (IS_ERR(dwc_priv->hsp_regmap))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwc_priv->hsp_regmap),
> +				"Failed to get hsp_sp_csr regmap\n");
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 2,
> +					 &eth_phy_ctrl_offset);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "can't get eth_phy_ctrl_offset\n");
> +
> +	regmap_read(dwc_priv->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(dwc_priv->hsp_regmap, eth_phy_ctrl_offset, eth_phy_ctrl_regset);
> +
> +	ret = of_property_read_u32_index(pdev->dev.of_node, "eswin,hsp_sp_csr", 3,
> +					 &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(dwc_priv->hsp_regmap, eth_axi_lp_ctrl_offset, EIC7700_ETH_CSYSREQ_VAL);
> +

Consider more sensible wrapping of this (netdev frowns at >80
characters per line, except for message strings that should remain
greppable.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> +/* Default delay value*/
> +#define EIC7700_DELAY_VALUE0 0x20202020
> +#define EIC7700_DELAY_VALUE1 0x96205A20

We need a better explanation of what is going on here. What do these
numbers mean?

> +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> +	dwc_priv->dly_param_10m[0] = 0x0;
> +	dwc_priv->dly_param_10m[1] = 0x0;
> +	dwc_priv->dly_param_10m[2] = 0x0;

What are the three different values for?

> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
> +				   &dwc_priv->rx_delay_ps);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);
> +	else
> +		has_rx_dly = true;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
> +				   &dwc_priv->tx_delay_ps);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
> +	else
> +		has_tx_dly = true;
> +	if (has_rx_dly && has_tx_dly)

What if i only to set a TX delay? I want the RX delay to default to
0ps.

{
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_1000m[1]);
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_100m[1]);
> +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> +				  &dwc_priv->dly_param_10m[1]);
> +	} else {
> +		dev_dbg(&pdev->dev, " use default dly\n");

What is the default? It should be 0ps. So there is no point printing
this message.

	Andrew

Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
We have carefully reviewed your comments and made the corresponding changes. Could you please help us evaluate whether the updates we mentioned in our previous email properly address your concerns and meet the expected standards?
At the same time, we still have some questions that need clarification. We have included these in the original email — we would appreciate it if you could also take a moment to review those points.


@Krzysztof Kozlowski
We noticed your review comment on the following page, but we did not receive it via email:
🔗 https://lore.kernel.org/lkml/f096afa1-260e-4f8c-8595-3b41425b2964@kernel.org/
Thank you for your professional feedback on our Ethernet driver.
Regarding the issue you raised:
"There is no such property. I already said at v2 you cannot have undocumented ABI."
Upon rechecking, we realized that during verification, we mistakenly used underscore-separated property names in the driver, while dash-separated names were used in the YAML bindings. We have now synchronized the property naming.
Could you please confirm if this mismatch was the root cause of your concern?


Best regards,

Li Zhi
Eswin Computing

> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-04 00:12:29 (星期五)
> 收件人: weishangjuan@eswincomputing.com
> 抄送: andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > +/* Default delay value*/
> > +#define EIC7700_DELAY_VALUE0 0x20202020
> > +#define EIC7700_DELAY_VALUE1 0x96205A20
> 
> We need a better explanation of what is going on here. What do these
> numbers mean?
> 

In response to your suggestion, we added the following more detailed comments to the code. Is this appropriate?
+/*
+ * Default delay register values for different signals:
+ *
+ * EIC7700_DELAY_VALUE0: Used for TXD and RXD signals delay configuration.
+ * Bits layout:
+ *   Byte 0 (bits 0-7)   : TXD0 / RXD0 delay (0x20 = 3.2 ns)
+ *   Byte 1 (bits 8-15)  : TXD1 / RXD1 delay (0x20 = 3.2 ns)
+ *   Byte 2 (bits 16-23) : TXD2 / RXD2 delay (0x20 = 3.2 ns)
+ *   Byte 3 (bits 24-31) : TXD3 / RXD3 delay (0x20 = 3.2 ns)
+ *
+ * EIC7700_DELAY_VALUE1: Used for control signals delay configuration.
+ * Bits layout:
+ *   Bits 0-6     : TXEN delay
+ *   Bits 8-14    : TXCLK delay
+ *   Bit  15      : TXCLK invert (1 = invert)
+ *   Bits 16-22   : RXDV delay
+ *   Bits 24-30   : RXCLK delay
+ *   Bit  31      : RXCLK invert (1 = invert)
+ */
+#define EIC7700_DELAY_VALUE0 0x20202020
+#define EIC7700_DELAY_VALUE1 0x96205A20

> > +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> > +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> > +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_10m[0] = 0x0;
> > +	dwc_priv->dly_param_10m[1] = 0x0;
> > +	dwc_priv->dly_param_10m[2] = 0x0;
> 
> What are the three different values for?

In response to your question, we have added the following more detailed comments to the code. Is this appropriate?
+        /* Initialize default delay parameters for 1000Mbps and 100Mbps speeds */
+        dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0; /* TXD delay */
+        dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1; /* Control signals delay */
+        dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0; /* RXD delay */
+        dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
+        dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
+        dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
+        /* For 10Mbps, no delay by default */
+        dwc_priv->dly_param_10m[0] = 0x0;
+        dwc_priv->dly_param_10m[1] = 0x0;
+        dwc_priv->dly_param_10m[2] = 0x0;

> 
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
> > +				   &dwc_priv->rx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_rx_dly = true;
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
> > +				   &dwc_priv->tx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_tx_dly = true;
> > +	if (has_rx_dly && has_tx_dly)
> 
> What if i only to set a TX delay? I want the RX delay to default to
> 0ps.
> 

Regarding your question, we have added default values ​​for tx delay and rx delay in the code, and as long as one of the two delays is configured in DTS, the original configuration can be overwritten. Does this process meet your suggestion?
+        /* Default delays in picoseconds */
+        dwc_priv->rx_delay_ps = 0;
+        dwc_priv->tx_delay_ps = 0;
+
+        if (has_rx_dly || has_tx_dly) {

> {
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_1000m[1]);
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_100m[1]);
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_10m[1]);
> > +	} else {
> > +		dev_dbg(&pdev->dev, " use default dly\n");
> 
> What is the default? It should be 0ps. So there is no point printing
> this message.
> 

Our original strategy was to use the value used when initializing dly_param_*m as the default value, so should we continue to follow your suggestion and use 0ps as the default value?

> 	Andrew

Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the original email below.


Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-04 00:12:29 (星期五)
> 收件人: weishangjuan@eswincomputing.com
> 抄送: andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, lizhi2@eswincomputing.com
> 主题: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > +/* Default delay value*/
> > +#define EIC7700_DELAY_VALUE0 0x20202020
> > +#define EIC7700_DELAY_VALUE1 0x96205A20
> 
> We need a better explanation of what is going on here. What do these
> numbers mean?
> 

Let me clarify:
  EIC7700_DELAY_VALUE0 (0x20202020) is used to configure delay taps for TXD[3:0] signals. Each byte represents the delay value for one data line.
  EIC7700_DELAY_VALUE1 (0x96205A20) configures control signal delays, such as TX_EN, RX_DV, and others. Again, each byte corresponds to a specific signal line.
More detailed inline comments will be added in the next patch to explain the bit layout and purpose of each byte in these default values. Is this understanding correct?

> > +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> > +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> > +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> > +	dwc_priv->dly_param_10m[0] = 0x0;
> > +	dwc_priv->dly_param_10m[1] = 0x0;
> > +	dwc_priv->dly_param_10m[2] = 0x0;
> 
> What are the three different values for?
> 

Let me clarify the purpose of the three elements in each dly_param_* array:
  dly_param_[x][0]: Delay configuration for TXD signals
  dly_param_[x][1]: Delay configuration for control signals (e.g., TX_EN, RX_DV, RX_CLK)
  dly_param_[x][2]: Delay configuration for RXD signals
These values are defined separately for different link speeds: 1000 Mbps, 100 Mbps, and 10 Mbps. During PHY initialization or when the link speed changes, the corresponding delay parameters are selected and applied to the hardware registers.
Inline comments will be added in the next patch to clarify the meaning and usage of each element. Is this understanding correct?

> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "rx-internal-delay-ps",
> > +				   &dwc_priv->rx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get rx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_rx_dly = true;
> > +
> > +	ret = of_property_read_u32(pdev->dev.of_node, "tx-internal-delay-ps",
> > +				   &dwc_priv->tx_delay_ps);
> > +	if (ret)
> > +		dev_dbg(&pdev->dev, "can't get tx-internal-delay-ps, ret(%d).", ret);
> > +	else
> > +		has_tx_dly = true;
> > +	if (has_rx_dly && has_tx_dly)
> 
> What if i only to set a TX delay? I want the RX delay to default to
> 0ps.
> 

Yes, this can be handled separately by calling eic7700_set_rgmii_rx_dly() and eic7700_set_rgmii_tx_dly() in the next patch. Is this correct?

> {
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_1000m[1]);
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_100m[1]);
> > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > +				  &dwc_priv->dly_param_10m[1]);
> > +	} else {
> > +		dev_dbg(&pdev->dev, " use default dly\n");
> 
> What is the default? It should be 0ps. So there is no point printing
> this message.
> 

The default value is EIC7700_DELAY_VALUE1, which is used in the absence of the DTS attribute. The print message will be removed in the next patch. Is this correct?

> 	Andrew

Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> > > +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> > > +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> > > +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> > > +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> > > +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> > > +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> > > +	dwc_priv->dly_param_10m[0] = 0x0;
> > > +	dwc_priv->dly_param_10m[1] = 0x0;
> > > +	dwc_priv->dly_param_10m[2] = 0x0;
> > 
> > What are the three different values for?
> > 
> 
> Let me clarify the purpose of the three elements in each dly_param_* array:
>   dly_param_[x][0]: Delay configuration for TXD signals
>   dly_param_[x][1]: Delay configuration for control signals (e.g., TX_EN, RX_DV, RX_CLK)
>   dly_param_[x][2]: Delay configuration for RXD signals

Maybe add a #define or an enum for the index.

Do these delays represent the RGMII 2ns delay?

> > {
> > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > +				  &dwc_priv->dly_param_1000m[1]);
> > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > +				  &dwc_priv->dly_param_100m[1]);
> > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > +				  &dwc_priv->dly_param_10m[1]);
> > > +	} else {
> > > +		dev_dbg(&pdev->dev, " use default dly\n");
> > 
> > What is the default? It should be 0ps. So there is no point printing
> > this message.
> > 
> 
> The default value is EIC7700_DELAY_VALUE1

But what does EIC7700_DELAY_VALUE1 mean? It should mean 0ps? But i'm
not sure it does.

	Andrew

Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the original email below.


Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-15 21:09:17 (星期二)
> 收件人: 李志 <lizhi2@eswincomputing.com>
> 抄送: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > > > +	dwc_priv->dly_param_1000m[0] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_1000m[1] = EIC7700_DELAY_VALUE1;
> > > > +	dwc_priv->dly_param_1000m[2] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_100m[0] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_100m[1] = EIC7700_DELAY_VALUE1;
> > > > +	dwc_priv->dly_param_100m[2] = EIC7700_DELAY_VALUE0;
> > > > +	dwc_priv->dly_param_10m[0] = 0x0;
> > > > +	dwc_priv->dly_param_10m[1] = 0x0;
> > > > +	dwc_priv->dly_param_10m[2] = 0x0;
> > > 
> > > What are the three different values for?
> > > 
> > 
> > Let me clarify the purpose of the three elements in each dly_param_* array:
> >   dly_param_[x][0]: Delay configuration for TXD signals
> >   dly_param_[x][1]: Delay configuration for control signals (e.g., TX_EN, RX_DV, RX_CLK)
> >   dly_param_[x][2]: Delay configuration for RXD signals
> 
> Maybe add a #define or an enum for the index.
> 
> Do these delays represent the RGMII 2ns delay?
> 

Yes, these delays refer to the RGMII delay, but they are not strictly 2ns. There are a few points that require further clarification:
1. Regarding delay configuration logic:
   As you mentioned in version V2, rx-internal-delay-ps and tx-internal-delay-ps will be mapped to and overwrite the corresponding bits in the EIC7700_DELAY_VALUE1 register, which controls the rx_clk and tx_clk delays. Is this understanding and approach correct and feasible?
2. About the phy-mode setting:
   In our platform, the internal delays are provided by the MAC. When configuring rx-internal-delay-ps and tx-internal-delay-ps in the device tree, is it appropriate to set phy-mode = "rgmii-id" in this case?
3. Delay values being greater than 2ns:
   In our platform, the optimal delay values for rx_clk and tx_clk are determined based on the board-level timing adjustment, and both are greater than 2ns. Given this, is it reasonable and compliant with the RGMII specification to set both rx-internal-delay-ps and tx-internal-delay-ps to values greater than 2ns in the Device Tree?

> > > {
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_1000m[1]);
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_100m[1]);
> > > > +		eic7700_set_delay(dwc_priv->rx_delay_ps, dwc_priv->tx_delay_ps,
> > > > +				  &dwc_priv->dly_param_10m[1]);
> > > > +	} else {
> > > > +		dev_dbg(&pdev->dev, " use default dly\n");
> > > 
> > > What is the default? It should be 0ps. So there is no point printing
> > > this message.
> > > 
> > 
> > The default value is EIC7700_DELAY_VALUE1
> 
> But what does EIC7700_DELAY_VALUE1 mean? It should mean 0ps? But i'm
> not sure it does.
> 

There is a question that needs clarification:
The EIC7700_DELAY_VALUE0 and EIC7700_DELAY_VALUE1 registers contain the optimal delay configurations determined through board-level phase adjustment. Therefore, they are also used as the default values in our platform. If the default delay is set to 0ps, the Ethernet interface may fail to function correctly in our platform.

> 	Andrew

Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> > > Let me clarify the purpose of the three elements in each dly_param_* array:
> > >   dly_param_[x][0]: Delay configuration for TXD signals
> > >   dly_param_[x][1]: Delay configuration for control signals (e.g., TX_EN, RX_DV, RX_CLK)
> > >   dly_param_[x][2]: Delay configuration for RXD signals
> > 
> > Maybe add a #define or an enum for the index.
> > 
> > Do these delays represent the RGMII 2ns delay?
> > 
> 
> Yes, these delays refer to the RGMII delay, but they are not strictly 2ns. There are a few points that require further clarification:
> 1. Regarding delay configuration logic:
>    As you mentioned in version V2, rx-internal-delay-ps and tx-internal-delay-ps will be mapped to and overwrite the corresponding bits in the EIC7700_DELAY_VALUE1 register, which controls the rx_clk and tx_clk delays. Is this understanding and approach correct and feasible?

Please configure your email client to wrap at about 78
characters. Standard network etiquette.

Yes, if rx-internal-delay-ps or/and tx-internal-delay-ps are in DT,
they should configure the delay the MAC applies.


> 2. About the phy-mode setting:
>    In our platform, the internal delays are provided by the MAC. When configuring rx-internal-delay-ps and tx-internal-delay-ps in the device tree, is it appropriate to set phy-mode = "rgmii-id" in this case?

Please read:

https://elixir.bootlin.com/linux/v6.15.7/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

It gives a detailed description of what phy-mode = "rmgii-id" means. 

> 3. Delay values being greater than 2ns:
>    In our platform, the optimal delay values for rx_clk and tx_clk are determined based on the board-level timing adjustment, and both are greater than 2ns. Given this, is it reasonable and compliant with the RGMII specification to set both rx-internal-delay-ps and tx-internal-delay-ps to values greater than 2ns in the Device Tree?

It is O.K. when the total delay is > 2ns. However, please note what is
said, the normal way to implement delays in Linux. The PHY does the
2ns delay. The MAC can then do fine tuning, adding additional small
delays.

> There is a question that needs clarification:
> The EIC7700_DELAY_VALUE0 and EIC7700_DELAY_VALUE1 registers contain the optimal delay configurations determined through board-level phase adjustment. Therefore, they are also used as the default values in our platform. If the default delay is set to 0ps, the Ethernet interface may fail to function correctly in our platform.

So there is only every going to be one board? There will never produce
a cost optimised version with a different, cheaper PHY? You will never
support connecting the MAC directly an Ethernet switch? You will never
make use of a PHY which can translate to SGMII/1000BaseX, and then
have an SFP cage?

DT properties are there to make your hardware more flexible. You can
use it to describe such setups, and handle the timing needed for each.

By default, when phy-mode is rgmii-id, the MAC adds 0ns, the PHY 2ns,
and most systems will just work. That 2ns is what the RGMII standard
requires. You can then fine tune it with rx-internal-delay-ps and
tx-internal-delay-ps if your design does not correctly follow the
RGMII standard.

	Andrew

Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 5867 bytes --]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the
original email below.


Best regards,

Li Zhi
Eswin Computing


> -----ԭʼÓʼþ-----
> ·¢¼þÈË: "Andrew Lunn" <andrew@lunn.ch>
> ·¢ËÍʱ¼ä:2025-07-21 21:10:55 (ÐÇÆÚÒ»)
> ÊÕ¼þÈË: ÀîÖ¾ <lizhi2@eswincomputing.com>
> ³­ËÍ: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com,
vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com,
prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com,
boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com,
linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> Ö÷Ìâ: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet
driver
> 
> > > > Let me clarify the purpose of the three elements in each dly_param_*
array:
> > > >   dly_param_[x][0]: Delay configuration for TXD signals
> > > >   dly_param_[x][1]: Delay configuration for control signals (e.g.,
TX_EN, RX_DV, RX_CLK)
> > > >   dly_param_[x][2]: Delay configuration for RXD signals
> > > 
> > > Maybe add a #define or an enum for the index.
> > > 
> > > Do these delays represent the RGMII 2ns delay?
> > > 
> > 
> > Yes, these delays refer to the RGMII delay, but they are not strictly
2ns. There are a few points that require further clarification:
> > 1. Regarding delay configuration logic:
> >    As you mentioned in version V2, rx-internal-delay-ps and
tx-internal-delay-ps will be mapped to and overwrite the corresponding bits
in the EIC7700_DELAY_VALUE1 register, which controls the rx_clk and tx_clk
delays. Is this understanding and approach correct and feasible?
> 
> Please configure your email client to wrap at about 78
> characters. Standard network etiquette.
> 
> Yes, if rx-internal-delay-ps or/and tx-internal-delay-ps are in DT,
> they should configure the delay the MAC applies.
> 
> 
> > 2. About the phy-mode setting:
> >    In our platform, the internal delays are provided by the MAC. When
configuring rx-internal-delay-ps and tx-internal-delay-ps in the device
tree, is it appropriate to set phy-mode = "rgmii-id" in this case?
> 
> Please read:
> 
>
https://elixir.bootlin.com/linux/v6.15.7/source/Documentation/devicetree/bin
dings/net/ethernet-controller.yaml#L287
> 
> It gives a detailed description of what phy-mode = "rmgii-id" means. 
> 
> > 3. Delay values being greater than 2ns:
> >    In our platform, the optimal delay values for rx_clk and tx_clk are
determined based on the board-level timing adjustment, and both are greater
than 2ns. Given this, is it reasonable and compliant with the RGMII
specification to set both rx-internal-delay-ps and tx-internal-delay-ps to
values greater than 2ns in the Device Tree?
> 
> It is O.K. when the total delay is > 2ns. However, please note what is
> said, the normal way to implement delays in Linux. The PHY does the
> 2ns delay. The MAC can then do fine tuning, adding additional small
> delays.
> 
> > There is a question that needs clarification:
> > The EIC7700_DELAY_VALUE0 and EIC7700_DELAY_VALUE1 registers contain the
optimal delay configurations determined through board-level phase
adjustment. Therefore, they are also used as the default values in our
platform. If the default delay is set to 0ps, the Ethernet interface may
fail to function correctly in our platform.
> 
> So there is only every going to be one board? There will never produce
> a cost optimised version with a different, cheaper PHY? You will never
> support connecting the MAC directly an Ethernet switch? You will never
> make use of a PHY which can translate to SGMII/1000BaseX, and then
> have an SFP cage?
> 
> DT properties are there to make your hardware more flexible. You can
> use it to describe such setups, and handle the timing needed for each.
> 
> By default, when phy-mode is rgmii-id, the MAC adds 0ns, the PHY 2ns,
> and most systems will just work. That 2ns is what the RGMII standard
> requires. You can then fine tune it with rx-internal-delay-ps and
> tx-internal-delay-ps if your design does not correctly follow the
> RGMII standard.
> 

Yes, DT properties are there to make our hardware more flexible.

Our platform uses three dedicated registers to configure RGMII signal
delays, due to differences in board-level designs. These registers control
delays for signals including RXD0¨C3, TXD0¨C3, RXDV, RXCLK, and TXCLK.
Among these, RXCLK and TXCLK are directly related to the standard DT
properties `rx-internal-delay-ps` and `tx-internal-delay-ps`, respectively.
The remaining signals (such as RXD0-4, TXD0-4, RXDV, etc.) require
additional configuration that cannot be expressed using standard
properties.

In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
device tree, including RXCLK and TXCLK. Based on the latest discussion,
this approach in the next version:
- The delay configuration for RXCLK and TXCLK will be handled using the
 standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
- The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
 continue to use the vendor-specific `eswin,dly-param-xxx` properties.
- If the standard delay properties are not specified in DT, a default of 0
ps
 will be assumed.

Is this understanding and approach correct and feasible?

> 	Andrew

Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
> device tree, including RXCLK and TXCLK. Based on the latest discussion,
> this approach in the next version:
> - The delay configuration for RXCLK and TXCLK will be handled using the
>  standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
> - The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
>  continue to use the vendor-specific `eswin,dly-param-xxx` properties.
> - If the standard delay properties are not specified in DT, a default of 0
> ps
>  will be assumed.

Please keep the RGMII standard in mind. All it says is that there
should be a 2ns delay between the data and the clock signal. It is
also quite generous on the range of delays which should actually
work. It says nothing about being able to configure that delay. And it
definitely says nothing about being able to configure each individual
single.

You hardware has a lot of flexibility, but none of if should actually
be needed, if you follow the standard.

So phy-mode = "rgmii-id"; should be all you need for most boards.
Everything else should be optional, with sensible defaults.

	Andrew

Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your professional and valuable suggestions.
Our questions are embedded below your comments in the original email below.


Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-22 22:07:23 (星期二)
> 收件人: 李志 <lizhi2@eswincomputing.com>
> 抄送: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > In v2, `eswin,dly-param-xxx` is used to configure all delay registers via
> > device tree, including RXCLK and TXCLK. Based on the latest discussion,
> > this approach in the next version:
> > - The delay configuration for RXCLK and TXCLK will be handled using the
> >  standard DT properties `rx-internal-delay-ps` and `tx-internal-delay-ps`.
> > - The remaining delay configuration (e.g., for RXD0-4, TXD0-4, RXDV) will
> >  continue to use the vendor-specific `eswin,dly-param-xxx` properties.
> > - If the standard delay properties are not specified in DT, a default of 0
> > ps
> >  will be assumed.
> 
> Please keep the RGMII standard in mind. All it says is that there
> should be a 2ns delay between the data and the clock signal. It is
> also quite generous on the range of delays which should actually
> work. It says nothing about being able to configure that delay. And it
> definitely says nothing about being able to configure each individual
> single.
> 
> You hardware has a lot of flexibility, but none of if should actually
> be needed, if you follow the standard.
> 
> So phy-mode = "rgmii-id"; should be all you need for most boards.
> Everything else should be optional, with sensible defaults.
> 

On our platform, the vendor-specific attributes eswin,dly-param-* were
initially introduced to compensate for board-specific variations in RGMII
signal timing, primarily due to differences in PCB trace lengths. These
attributes allow fine-grained, per-signal delay control for RXD, TXD,
TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
settings.
In our experience, setting phy-mode = "rgmii-id" alone, along with only
the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
has proven insufficient to meet our hardware's timing requirements.
Therefore these standard properties are treated as controlling only RXCLK
and TXCLK, while continuing to use the eswin,dly-param-* attributes for
other signals.
Additionally, if rx-internal-delay-ps and tx-internal-delay-ps are
omitted, their values default to 0ps due to the use of devm_kzalloc().
This behavior reinforces the need for explicit delay values in certain
configurations. For reference, TI platforms use a dedicated IODELAY
hardware module to program per-signal RGMII delays in a similar fashion.

As per your suggestion, we will set mode="rgmii-id".
We have questions on setting delay parameters from dts we have two
approches. Could you please let us know which approach is appropriate?

1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
   using vendor-specific attributes eswin,dly-param-*.
   e.g.
   eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
   attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
   TXCLK using tx-internal-delay-ps.
   e.g
   eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
   rx-internal-delay-ps = <9000>;
   tx-internal-delay-ps = <2200>;

> 	Andrew

Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> > You hardware has a lot of flexibility, but none of if should actually
> > be needed, if you follow the standard.
> > 
> > So phy-mode = "rgmii-id"; should be all you need for most boards.
> > Everything else should be optional, with sensible defaults.
> > 
> 
> On our platform, the vendor-specific attributes eswin,dly-param-* were
> initially introduced to compensate for board-specific variations in RGMII
> signal timing, primarily due to differences in PCB trace lengths.

So it seems like, because you have the flexibility in the hardware,
you designed your PCB poorly, breaking the standard, so now must have
these properties.  It would of been much better if you had stuck to
the standard...

Please ensure your default values, when nothing is specified in DT,
correspond to a board which actually fulfils the standard. The next
board which is made using this device can then avoid having anything
special in there DT blob.

> These attributes allow fine-grained, per-signal delay control for RXD, TXD,
> TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
> settings.
> In our experience, setting phy-mode = "rgmii-id" alone, along with only
> the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
> has proven insufficient to meet our hardware's timing requirements.

You don't need vendor properties for RXCLK and TXCLK, that is what
tx-internal-delay-ps and rx-internal-delay-ps do. They change the
clock signal relative to TX and RX data. So you only need properties
for TXEN and RXDV. You should probably call these
eswin,txen-internal-delay-ps and eswin,rxdv-internal-delay-ps.  In the
binding you need to clearly define what these mean, for your hardware,
i.e.  what is the delay relative to?

> 1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
>    using vendor-specific attributes eswin,dly-param-*.
>    e.g.
>    eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
> 2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
>    attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
>    TXCLK using tx-internal-delay-ps.
>    e.g
>    eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
>    rx-internal-delay-ps = <9000>;
>    tx-internal-delay-ps = <2200>;

Neither. DT should not contain HW values you poke into registers. They
should be SI using, in this case, pico seconds. From these delays in
picoseconds, have the driver calculate what values should be written
into the registers.

And these delay values are unlikely to be correct. You are using
rgmii-id, so the PHY is adding 2ns. You want the MAC to make small
tuning adjustments, so 200 could be reasonable, but 9000ps is way too
big.

	Andrew

Re: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your valuable and professional suggestions.
Please find our questions and explanations embedded below your comments
in the original email.

Best regards,

Li Zhi
Eswin Computing





> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-07-31 21:31:52 (星期四)
> 收件人: 李志 <lizhi2@eswincomputing.com>
> 抄送: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > > You hardware has a lot of flexibility, but none of if should actually
> > > be needed, if you follow the standard.
> > > 
> > > So phy-mode = "rgmii-id"; should be all you need for most boards.
> > > Everything else should be optional, with sensible defaults.
> > > 
> > 
> > On our platform, the vendor-specific attributes eswin,dly-param-* were
> > initially introduced to compensate for board-specific variations in RGMII
> > signal timing, primarily due to differences in PCB trace lengths.
> 
> So it seems like, because you have the flexibility in the hardware,
> you designed your PCB poorly, breaking the standard, so now must have
> these properties.  It would of been much better if you had stuck to
> the standard...
> 
> Please ensure your default values, when nothing is specified in DT,
> correspond to a board which actually fulfils the standard. The next
> board which is made using this device can then avoid having anything
> special in there DT blob.
> 
> > These attributes allow fine-grained, per-signal delay control for RXD, TXD,
> > TXEN, RXDV, RXCLK, and TXCLK, based on empirically derived optimal phase
> > settings.
> > In our experience, setting phy-mode = "rgmii-id" alone, along with only
> > the standard properties rx-internal-delay-ps and tx-internal-delay-ps,
> > has proven insufficient to meet our hardware's timing requirements.
> 
> You don't need vendor properties for RXCLK and TXCLK, that is what
> tx-internal-delay-ps and rx-internal-delay-ps do. They change the
> clock signal relative to TX and RX data. So you only need properties
> for TXEN and RXDV. You should probably call these
> eswin,txen-internal-delay-ps and eswin,rxdv-internal-delay-ps.  In the
> binding you need to clearly define what these mean, for your hardware,
> i.e.  what is the delay relative to?
> 
> > 1. Setting all delay parameters (RXD, TXD, TXEN, RXDV, RXCLK, and TXCLK)
> >    using vendor-specific attributes eswin,dly-param-*.
> >    e.g.
> >    eswin,dly-param-1000m = <0x20202020 0x96205A20 0x20202020>;
> > 2. Setting delay parameters (RXD, TXD, TXEN, RXDV) using vendor-specific
> >    attributes eswin,dly-param-* , RXCLK using rx-internal-delay-ps and
> >    TXCLK using tx-internal-delay-ps.
> >    e.g
> >    eswin,dly-param-1000m = <0x20202020 0x80200020 0x20202020>;
> >    rx-internal-delay-ps = <9000>;
> >    tx-internal-delay-ps = <2200>;
> 
> Neither. DT should not contain HW values you poke into registers. They
> should be SI using, in this case, pico seconds. From these delays in
> picoseconds, have the driver calculate what values should be written
> into the registers.
> 
> And these delay values are unlikely to be correct. You are using
> rgmii-id, so the PHY is adding 2ns. You want the MAC to make small
> tuning adjustments, so 200 could be reasonable, but 9000ps is way too
> big.
> 

We re-tuned and verified that setting the TXD and RXD delays to 0 and
configuring TXEN and RXDV to 0 yielded the same hardware performance as
long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.
Therefore, in the next patch, we will drop the vendor-specific properties
(e.g. eswin,dly-param-*) and keep only the standard attributes, namely
rx-internal-delay-ps and tx-internal-delay-ps.
Is this correct?

> 	Andrew

Re: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[Andrew Lunn]

> We re-tuned and verified that setting the TXD and RXD delays to 0 and
> configuring TXEN and RXDV to 0 yielded the same hardware performance as
> long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.

This is in addition to phy-mode = 'rgmii-id'?

> Therefore, in the next patch, we will drop the vendor-specific properties
> (e.g. eswin,dly-param-*) and keep only the standard attributes, namely
> rx-internal-delay-ps and tx-internal-delay-ps.
> Is this correct?

Yes, 200ps is a small tuning value, when the PHY adds the 2ns. This is
O.K.

	Andrew

Re: Re: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver

[李志]

Dear Andrew Lunn,
Thank you for your valuable and professional suggestions.
Please find our explanations embedded below your comments in the
original email.

Best regards,

Li Zhi
Eswin Computing


> -----原始邮件-----
> 发件人: "Andrew Lunn" <andrew@lunn.ch>
> 发送时间:2025-08-22 11:17:37 (星期五)
> 收件人: 李志 <lizhi2@eswincomputing.com>
> 抄送: weishangjuan@eswincomputing.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, 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, yong.liang.choong@linux.intel.com, vladimir.oltean@nxp.com, jszhang@kernel.org, jan.petrous@oss.nxp.com, prabhakar.mahadev-lad.rj@bp.renesas.com, inochiama@gmail.com, boon.khai.ng@altera.com, dfustini@tenstorrent.com, 0x1207@gmail.com, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com
> 主题: Re: Re: Re: Re: Re: Re: [PATCH v3 2/2] ethernet: eswin: Add eic7700 ethernet driver
> 
> > We re-tuned and verified that setting the TXD and RXD delays to 0 and
> > configuring TXEN and RXDV to 0 yielded the same hardware performance as
> > long as we only applied delays (e.g. 200ps) to TXCLK and RXCLK.
> 
> This is in addition to phy-mode = 'rgmii-id'?
> 

Yes, our re-tuning and verification were performed with phy-mode set to
rgmii-id.

> > Therefore, in the next patch, we will drop the vendor-specific properties
> > (e.g. eswin,dly-param-*) and keep only the standard attributes, namely
> > rx-internal-delay-ps and tx-internal-delay-ps.
> > Is this correct?
> 
> Yes, 200ps is a small tuning value, when the PHY adds the 2ns. This is
> O.K.
> 
> 	Andrew