[PATCH v2 0/3] Add the dwmac driver support for T-HEAD TH1520 SoC

[PATCH v2 0/3] Add the dwmac driver support for T-HEAD TH1520 SoC

[Drew Fustini]

This patch series is based on linux-next which contains the TH1520 clk
dts patches in my v6.12 pull request:

 https://lore.kernel.org/linux-riscv/ZsWs8QiVruMXjzPc@x1/

This patch also depends on this TH1520 pinctrl series:

 https://lore.kernel.org/linux-riscv/20240914-th1520-pinctrl-v2-0-3ba67dde882c@tenstorrent.com/

I have a branch with this series and the dependencies:

 https://github.com/pdp7/linux/tree/b4/th1520-dwmac

Regarding clocks, the gmac nodes in th1520.dtsi have the "stmmac_clk"
clock set to CLK_GMAC_AXI in the AP_SUBSYS clock controller. This
corresponds to the enable bit for the GMAC axi4_clk gate which is
handled by the clk-th1520-ap driver. thead_dwmac_fix_speed() does not
modify anything in the AP_SUBSYS clock controller. It only writes to
GMAC APB registers. It seems unnecessary to create a new clock driver
just for the GMAC APB registers. Refer to section 1.6.2 in the TH1520
Peripheral Interface User Manual [1].

Regarding rx and tx internal delays, that same section in the manual
doesn't specify what unit is represented by the delay_ctrl bit field in
GMAC_RXCLK_DELAY_CTRL and GMAC_TXCLK_DELAY_CTRL. It is only 5 bits and
a max value of 31 seems too small to represent picoseconds. The vendor
kernel [2] uses properties named "rx-clk-delay" and "tx-clk-delay" but
doesn't indicate any units. I see ti,dp83867.yaml adds vendor specific
rx and tx delay properties so that is what I've now done in this series.
Note: the hardware default value of 0 for delay_ctrl works okay for the
TH1520 hardware that I have.

There was a question in my v1 series about if the phy required a delay
after reset in the BeagleV Ahead device tree. The board has the Realtek
RTL8211F-CG phy [3]. According to this datasheet I found [4], the reset
pin must be asserted low for at least 10ms for the internal regulator.
Software must wait least 50ms before accessing the phy registers. I've
now added reset-delay-us and reset-post-delay-us with those values.

[1] https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
[2] https://github.com/revyos/thead-kernel/blob/lpi4a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
[3] https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/BeagleV-Ahead_SCH.pdf
[4] https://www.lcsc.com/datasheet/lcsc_datasheet_1912111437_Realtek-Semicon-RTL8211F-CG_C187932.pdf

Changes since v1:
 - Drop the first patch as it is no longer needed due to upstream commit
   d01e0e98de31 ("dt-bindings: net: dwmac: Validate PBL for all IP-cores")
 - Rename compatible from "thead,th1520-dwmac" to "thead,th1520-gmac"
 - Add thead,rx-internal-delay and thead,tx-internal-delay properties
   and check that it does not exceed the maximum value
 - Convert from stmmac_dvr_probe() to devm_stmmac_pltfr_probe() and
   delete the .remove_new hook as it is no longer needed
 - Handle return value of regmap_write() in case it fails
 - Add phy reset delay properties to the BeagleV Ahead device tree
 - Link: https://lore.kernel.org/all/20240713-thead-dwmac-v1-0-81f04480cd31@tenstorrent.com/

Changes since Jisheng v2:
 - remove thead,gmacapb that references syscon for APB registers
 - add a second memory region to gmac nodes for the APB registers
 - Link: https://lore.kernel.org/all/20230827091710.1483-1-jszhang@kernel.org/

Changes since Jisheng v1:
 - rebase on the lastest net-next
 - collect Reviewed-by tag
 - address Krzysztof's comment of the dt binding
 - fix "div is not initialised" issue pointed out by Simon
 - Link: https://lore.kernel.org/all/20230820120213.2054-1-jszhang@kernel.org/

---
Emil Renner Berthing (1):
      riscv: dts: thead: Add TH1520 ethernet nodes

Jisheng Zhang (2):
      dt-bindings: net: Add T-HEAD dwmac support
      net: stmmac: Add glue layer for T-HEAD TH1520 SoC

 .../devicetree/bindings/net/snps,dwmac.yaml        |   1 +
 .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++
 MAINTAINERS                                        |   2 +
 arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts |  91 ++++++
 .../boot/dts/thead/th1520-lichee-module-4a.dtsi    | 135 +++++++++
 arch/riscv/boot/dts/thead/th1520.dtsi              |  50 ++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig        |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c  | 319 +++++++++++++++++++++
 9 files changed, 719 insertions(+)
---
base-commit: eb9913a02c55913317dcb4797026f958ce2c97d5
change-id: 20240923-th1520-dwmac-55a320ae01a6

Best regards,
-- 
Drew Fustini <dfustini@tenstorrent.com>

[PATCH v2 1/3] dt-bindings: net: Add T-HEAD dwmac support

[Drew Fustini]

From: Jisheng Zhang <jszhang@kernel.org>

Add documentation to describe T-HEAD dwmac.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
[drew: change apb registers from syscon to second reg of gmac node]
[drew: rename compatible, add thead rx/tx internal delay properties]
Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml        |   1 +
 .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++++++++++++++++
 MAINTAINERS                                        |   1 +
 3 files changed, 111 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 4e2ba1bf788c..474ade185033 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -99,6 +99,7 @@ properties:
         - snps,dwxgmac-2.10
         - starfive,jh7100-dwmac
         - starfive,jh7110-dwmac
+        - thead,th1520-gmac
 
   reg:
     minItems: 1
diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
new file mode 100644
index 000000000000..1070e891c025
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: T-HEAD TH1520 GMAC Ethernet controller
+
+maintainers:
+  - Drew Fustini <dfustini@tenstorrent.com>
+
+description: |
+  The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
+  https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
+
+  Features include
+    - Compliant with IEEE802.3 Specification
+    - IEEE 1588-2008 standard for precision networked clock synchronization
+    - Supports 10/100/1000Mbps data transfer rate
+    - Supports RGMII/MII interface
+    - Preamble and start of frame data (SFD) insertion in Transmit path
+    - Preamble and SFD deletion in the Receive path
+    - Automatic CRC and pad generation options for receive frames
+    - MDIO master interface for PHY device configuration and management
+
+  The GMAC Registers consists of two parts
+    - APB registers are used to configure clock frequency/clock enable/clock
+      direction/PHY interface type.
+    - AHB registers are use to configure GMAC core (DesignWare Core part).
+      GMAC core register consists of DMA registers and GMAC registers.
+
+select:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - thead,th1520-gmac
+  required:
+    - compatible
+
+allOf:
+  - $ref: snps,dwmac.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - thead,th1520-gmac
+      - const: snps,dwmac-3.70a
+
+  reg:
+    items:
+      - description: DesignWare GMAC IP core registers
+      - description: GMAC APB registers
+
+  reg-names:
+    items:
+      - const: dwmac
+      - const: apb
+
+  thead,rx-internal-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      RGMII receive clock delay. The value is used for the delay_ctrl
+      field in GMAC_RXCLK_DELAY_CTRL. Units are not specified.
+
+  thead,tx-internal-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      RGMII transmit clock delay. The value is used for the delay_ctrl
+      field in GMAC_TXCLK_DELAY_CTRL. Units are not specified.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - phy-mode
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    gmac0: ethernet@e7070000 {
+        compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
+        reg = <0xe7070000 0x2000>, <0xec003000 0x1000>;
+        reg-names = "dwmac", "apb";
+        clocks = <&clk 1>, <&clk 2>;
+        clock-names = "stmmaceth", "pclk";
+        interrupts = <66>;
+        interrupt-names = "macirq";
+        phy-mode = "rgmii-id";
+        snps,fixed-burst;
+        snps,axi-config = <&stmmac_axi_setup>;
+        snps,pbl = <32>;
+        phy-handle = <&phy0>;
+
+        mdio {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "snps,dwmac-mdio";
+
+            phy0: ethernet-phy@0 {
+                reg = <0>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3e18aefd1222..aaa24189de43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19940,6 +19940,7 @@ L:	linux-riscv@lists.infradead.org
 S:	Maintained
 T:	git https://github.com/pdp7/linux.git
 F:	Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
+F:	Documentation/devicetree/bindings/net/thead,dwmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c

-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: net: Add T-HEAD dwmac support

[Krzysztof Kozlowski]

On Thu, Sep 26, 2024 at 11:15:50AM -0700, Drew Fustini wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> Add documentation to describe T-HEAD dwmac.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> [drew: change apb registers from syscon to second reg of gmac node]
> [drew: rename compatible, add thead rx/tx internal delay properties]
> Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml        |   1 +
>  .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  3 files changed, 111 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index 4e2ba1bf788c..474ade185033 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -99,6 +99,7 @@ properties:
>          - snps,dwxgmac-2.10
>          - starfive,jh7100-dwmac
>          - starfive,jh7110-dwmac
> +        - thead,th1520-gmac
>  
>    reg:
>      minItems: 1
> diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> new file mode 100644
> index 000000000000..1070e891c025
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> @@ -0,0 +1,109 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: T-HEAD TH1520 GMAC Ethernet controller
> +
> +maintainers:
> +  - Drew Fustini <dfustini@tenstorrent.com>
> +
> +description: |
> +  The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
> +  https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
> +
> +  Features include
> +    - Compliant with IEEE802.3 Specification
> +    - IEEE 1588-2008 standard for precision networked clock synchronization
> +    - Supports 10/100/1000Mbps data transfer rate
> +    - Supports RGMII/MII interface
> +    - Preamble and start of frame data (SFD) insertion in Transmit path
> +    - Preamble and SFD deletion in the Receive path
> +    - Automatic CRC and pad generation options for receive frames
> +    - MDIO master interface for PHY device configuration and management
> +
> +  The GMAC Registers consists of two parts
> +    - APB registers are used to configure clock frequency/clock enable/clock
> +      direction/PHY interface type.
> +    - AHB registers are use to configure GMAC core (DesignWare Core part).
> +      GMAC core register consists of DMA registers and GMAC registers.
> +
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - thead,th1520-gmac
> +  required:
> +    - compatible
> +
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - thead,th1520-gmac
> +      - const: snps,dwmac-3.70a
> +
> +  reg:
> +    items:
> +      - description: DesignWare GMAC IP core registers
> +      - description: GMAC APB registers
> +
> +  reg-names:
> +    items:
> +      - const: dwmac
> +      - const: apb
> +
> +  thead,rx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII receive clock delay. The value is used for the delay_ctrl
> +      field in GMAC_RXCLK_DELAY_CTRL. Units are not specified.

What do you mean by "unspecified units"? They are always specified,
hardware does not work randomly, e.g. once uses clock cycles, but next
time you run it will use picoseconds.

You also miss default (property is not required) and some sort of constraints.

> +
> +  thead,tx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII transmit clock delay. The value is used for the delay_ctrl
> +      field in GMAC_TXCLK_DELAY_CTRL. Units are not specified.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: net: Add T-HEAD dwmac support

[Drew Fustini]

On Fri, Sep 27, 2024 at 11:34:48AM +0200, Krzysztof Kozlowski wrote:
> On Thu, Sep 26, 2024 at 11:15:50AM -0700, Drew Fustini wrote:
> > From: Jisheng Zhang <jszhang@kernel.org>
> > 
> > Add documentation to describe T-HEAD dwmac.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > [drew: change apb registers from syscon to second reg of gmac node]
> > [drew: rename compatible, add thead rx/tx internal delay properties]
> > Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
> > ---
> >  .../devicetree/bindings/net/snps,dwmac.yaml        |   1 +
> >  .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  3 files changed, 111 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > index 4e2ba1bf788c..474ade185033 100644
> > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> > @@ -99,6 +99,7 @@ properties:
> >          - snps,dwxgmac-2.10
> >          - starfive,jh7100-dwmac
> >          - starfive,jh7110-dwmac
> > +        - thead,th1520-gmac
> >  
> >    reg:
> >      minItems: 1
> > diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> > new file mode 100644
> > index 000000000000..1070e891c025
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
> > @@ -0,0 +1,109 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: T-HEAD TH1520 GMAC Ethernet controller
> > +
> > +maintainers:
> > +  - Drew Fustini <dfustini@tenstorrent.com>
> > +
> > +description: |
> > +  The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
> > +  https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
> > +
> > +  Features include
> > +    - Compliant with IEEE802.3 Specification
> > +    - IEEE 1588-2008 standard for precision networked clock synchronization
> > +    - Supports 10/100/1000Mbps data transfer rate
> > +    - Supports RGMII/MII interface
> > +    - Preamble and start of frame data (SFD) insertion in Transmit path
> > +    - Preamble and SFD deletion in the Receive path
> > +    - Automatic CRC and pad generation options for receive frames
> > +    - MDIO master interface for PHY device configuration and management
> > +
> > +  The GMAC Registers consists of two parts
> > +    - APB registers are used to configure clock frequency/clock enable/clock
> > +      direction/PHY interface type.
> > +    - AHB registers are use to configure GMAC core (DesignWare Core part).
> > +      GMAC core register consists of DMA registers and GMAC registers.
> > +
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - thead,th1520-gmac
> > +  required:
> > +    - compatible
> > +
> > +allOf:
> > +  - $ref: snps,dwmac.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - thead,th1520-gmac
> > +      - const: snps,dwmac-3.70a
> > +
> > +  reg:
> > +    items:
> > +      - description: DesignWare GMAC IP core registers
> > +      - description: GMAC APB registers
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dwmac
> > +      - const: apb
> > +
> > +  thead,rx-internal-delay:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      RGMII receive clock delay. The value is used for the delay_ctrl
> > +      field in GMAC_RXCLK_DELAY_CTRL. Units are not specified.
> 
> What do you mean by "unspecified units"? They are always specified,
> hardware does not work randomly, e.g. once uses clock cycles, but next
> time you run it will use picoseconds.
> 
> You also miss default (property is not required) and some sort of constraints.

I should have stated that I don't know the units for delay_ctrl. The
5-bit field has a max value of 31 which seems far too small for
picoseconds. Unfortunately, the documentation from the SoC vendor does
not give anymore details about what the value represents.

Andrew Lunn replied [1] to my cover letter that it is best to hard code
the field to 0 (which is the hardware reset value) if I don't know what
the units are for delay_ctrl. The hardware that I have works okay with
delay_ctrl of 0, so it seems these new vendor properties are not needed.

Thanks,
Drew

[1] https://lore.kernel.org/lkml/5e379911-e3de-478c-b785-61dbcc9627b1@lunn.ch/

Re: [PATCH v2 1/3] dt-bindings: net: Add T-HEAD dwmac support

[Krzysztof Kozlowski]

On 27/09/2024 22:51, Drew Fustini wrote:
> On Fri, Sep 27, 2024 at 11:34:48AM +0200, Krzysztof Kozlowski wrote:
>> On Thu, Sep 26, 2024 at 11:15:50AM -0700, Drew Fustini wrote:
>>> From: Jisheng Zhang <jszhang@kernel.org>
>>>
>>> Add documentation to describe T-HEAD dwmac.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>> [drew: change apb registers from syscon to second reg of gmac node]
>>> [drew: rename compatible, add thead rx/tx internal delay properties]
>>> Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
>>> ---
>>>  .../devicetree/bindings/net/snps,dwmac.yaml        |   1 +
>>>  .../devicetree/bindings/net/thead,th1520-gmac.yaml | 109 +++++++++++++++++++++
>>>  MAINTAINERS                                        |   1 +
>>>  3 files changed, 111 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index 4e2ba1bf788c..474ade185033 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -99,6 +99,7 @@ properties:
>>>          - snps,dwxgmac-2.10
>>>          - starfive,jh7100-dwmac
>>>          - starfive,jh7110-dwmac
>>> +        - thead,th1520-gmac
>>>  
>>>    reg:
>>>      minItems: 1
>>> diff --git a/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> new file mode 100644
>>> index 000000000000..1070e891c025
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml
>>> @@ -0,0 +1,109 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/thead,th1520-gmac.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: T-HEAD TH1520 GMAC Ethernet controller
>>> +
>>> +maintainers:
>>> +  - Drew Fustini <dfustini@tenstorrent.com>
>>> +
>>> +description: |
>>> +  The TH1520 GMAC is described in the TH1520 Peripheral Interface User Manual
>>> +  https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/tree/main/docs
>>> +
>>> +  Features include
>>> +    - Compliant with IEEE802.3 Specification
>>> +    - IEEE 1588-2008 standard for precision networked clock synchronization
>>> +    - Supports 10/100/1000Mbps data transfer rate
>>> +    - Supports RGMII/MII interface
>>> +    - Preamble and start of frame data (SFD) insertion in Transmit path
>>> +    - Preamble and SFD deletion in the Receive path
>>> +    - Automatic CRC and pad generation options for receive frames
>>> +    - MDIO master interface for PHY device configuration and management
>>> +
>>> +  The GMAC Registers consists of two parts
>>> +    - APB registers are used to configure clock frequency/clock enable/clock
>>> +      direction/PHY interface type.
>>> +    - AHB registers are use to configure GMAC core (DesignWare Core part).
>>> +      GMAC core register consists of DMA registers and GMAC registers.
>>> +
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        enum:
>>> +          - thead,th1520-gmac
>>> +  required:
>>> +    - compatible
>>> +
>>> +allOf:
>>> +  - $ref: snps,dwmac.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - thead,th1520-gmac
>>> +      - const: snps,dwmac-3.70a
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: DesignWare GMAC IP core registers
>>> +      - description: GMAC APB registers
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: dwmac
>>> +      - const: apb
>>> +
>>> +  thead,rx-internal-delay:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      RGMII receive clock delay. The value is used for the delay_ctrl
>>> +      field in GMAC_RXCLK_DELAY_CTRL. Units are not specified.
>>
>> What do you mean by "unspecified units"? They are always specified,
>> hardware does not work randomly, e.g. once uses clock cycles, but next
>> time you run it will use picoseconds.
>>
>> You also miss default (property is not required) and some sort of constraints.
> 
> I should have stated that I don't know the units for delay_ctrl. The
> 5-bit field has a max value of 31 which seems far too small for
> picoseconds. Unfortunately, the documentation from the SoC vendor does
> not give anymore details about what the value represents.
> 
> Andrew Lunn replied [1] to my cover letter that it is best to hard code
> the field to 0 (which is the hardware reset value) if I don't know what
> the units are for delay_ctrl. The hardware that I have works okay with
> delay_ctrl of 0, so it seems these new vendor properties are not needed.

Then just say that this is using register values directly.

Best regards,
Krzysztof

[PATCH v2 2/3] net: stmmac: Add glue layer for T-HEAD TH1520 SoC

[Drew Fustini]

From: Jisheng Zhang <jszhang@kernel.org>

Add dwmac glue driver to support the dwmac on the T-HEAD TH1520 SoC.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
[esmil: rename plat->interface -> plat->mac_interface,
        use devm_stmmac_probe_config_dt()]
Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
[drew: change apb registers from syscon to second reg of gmac node]
[drew: convert from stmmac_dvr_probe() to devm_stmmac_pltfr_probe()]
Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
---
 MAINTAINERS                                       |   1 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig       |  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile      |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c | 319 ++++++++++++++++++++++
 4 files changed, 332 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index aaa24189de43..54ac52695975 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19944,6 +19944,7 @@ F:	Documentation/devicetree/bindings/net/thead,dwmac.yaml
 F:	Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml
 F:	arch/riscv/boot/dts/thead/
 F:	drivers/clk/thead/clk-th1520-ap.c
+F:	drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
 F:	drivers/pinctrl/pinctrl-th1520.c
 F:	include/dt-bindings/clock/thead,th1520-clk-ap.h
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 05cc07b8f48c..82030adaf16e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -228,6 +228,17 @@ config DWMAC_SUN8I
 	  stmmac device driver. This driver is used for H3/A83T/A64
 	  EMAC ethernet controller.
 
+config DWMAC_THEAD
+	tristate "T-HEAD dwmac support"
+	depends on OF && (ARCH_THEAD || COMPILE_TEST)
+	select MFD_SYSCON
+	help
+	  Support for ethernet controllers on T-HEAD RISC-V SoCs
+
+	  This selects the T-HEAD platform specific glue layer support for
+	  the stmmac device driver. This driver is used for T-HEAD TH1520
+	  ethernet controller.
+
 config DWMAC_IMX8
 	tristate "NXP IMX8 DWMAC support"
 	default ARCH_MXC
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index c2f0e91f6bf8..d065634c6223 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_DWMAC_STI)		+= dwmac-sti.o
 obj-$(CONFIG_DWMAC_STM32)	+= dwmac-stm32.o
 obj-$(CONFIG_DWMAC_SUNXI)	+= dwmac-sunxi.o
 obj-$(CONFIG_DWMAC_SUN8I)	+= dwmac-sun8i.o
+obj-$(CONFIG_DWMAC_THEAD)	+= dwmac-thead.o
 obj-$(CONFIG_DWMAC_DWC_QOS_ETH)	+= dwmac-dwc-qos-eth.o
 obj-$(CONFIG_DWMAC_INTEL_PLAT)	+= dwmac-intel-plat.o
 obj-$(CONFIG_DWMAC_LOONGSON1)	+= dwmac-loongson1.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
new file mode 100644
index 000000000000..9abe8ba323bd
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * T-HEAD DWMAC platform driver
+ *
+ * Copyright (C) 2021 Alibaba Group Holding Limited.
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "stmmac_platform.h"
+
+#define GMAC_CLK_EN			0x00
+#define  GMAC_TX_CLK_EN			BIT(1)
+#define  GMAC_TX_CLK_N_EN		BIT(2)
+#define  GMAC_TX_CLK_OUT_EN		BIT(3)
+#define  GMAC_RX_CLK_EN			BIT(4)
+#define  GMAC_RX_CLK_N_EN		BIT(5)
+#define  GMAC_EPHY_REF_CLK_EN		BIT(6)
+#define GMAC_RXCLK_DELAY_CTRL		0x04
+#define  GMAC_RXCLK_BYPASS		BIT(15)
+#define  GMAC_RXCLK_INVERT		BIT(14)
+#define  GMAC_RXCLK_DELAY_MASK		GENMASK(4, 0)
+#define  GMAC_RXCLK_DELAY_VAL(x)	FIELD_PREP(GMAC_RXCLK_DELAY_MASK, (x))
+#define GMAC_TXCLK_DELAY_CTRL		0x08
+#define  GMAC_TXCLK_BYPASS		BIT(15)
+#define  GMAC_TXCLK_INVERT		BIT(14)
+#define  GMAC_TXCLK_DELAY_MASK		GENMASK(4, 0)
+#define  GMAC_TXCLK_DELAY_VAL(x)	FIELD_PREP(GMAC_RXCLK_DELAY_MASK, (x))
+#define GMAC_PLLCLK_DIV			0x0c
+#define  GMAC_PLLCLK_DIV_EN		BIT(31)
+#define  GMAC_PLLCLK_DIV_MASK		GENMASK(7, 0)
+#define  GMAC_PLLCLK_DIV_NUM(x)		FIELD_PREP(GMAC_PLLCLK_DIV_MASK, (x))
+#define GMAC_GTXCLK_SEL			0x18
+#define  GMAC_GTXCLK_SEL_PLL		BIT(0)
+#define GMAC_INTF_CTRL			0x1c
+#define  PHY_INTF_MASK			BIT(0)
+#define  PHY_INTF_RGMII			FIELD_PREP(PHY_INTF_MASK, 1)
+#define  PHY_INTF_MII_GMII		FIELD_PREP(PHY_INTF_MASK, 0)
+#define GMAC_TXCLK_OEN			0x20
+#define  TXCLK_DIR_MASK			BIT(0)
+#define  TXCLK_DIR_OUTPUT		FIELD_PREP(TXCLK_DIR_MASK, 0)
+#define  TXCLK_DIR_INPUT		FIELD_PREP(TXCLK_DIR_MASK, 1)
+
+#define GMAC_GMII_RGMII_RATE	125000000
+#define GMAC_MII_RATE		25000000
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+struct thead_dwmac {
+	struct plat_stmmacenet_data *plat;
+	struct regmap *apb_regmap;
+	struct device *dev;
+	u32 rx_delay;
+	u32 tx_delay;
+};
+
+static int thead_dwmac_set_phy_if(struct plat_stmmacenet_data *plat)
+{
+	struct thead_dwmac *dwmac = plat->bsp_priv;
+	u32 phyif;
+
+	switch (plat->mac_interface) {
+	case PHY_INTERFACE_MODE_MII:
+		phyif = PHY_INTF_MII_GMII;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		phyif = PHY_INTF_RGMII;
+		break;
+	default:
+		dev_err(dwmac->dev, "unsupported phy interface %d\n",
+			plat->mac_interface);
+		return -EINVAL;
+	};
+
+	return regmap_write(dwmac->apb_regmap, GMAC_INTF_CTRL, phyif);
+}
+
+static int thead_dwmac_set_txclk_dir(struct plat_stmmacenet_data *plat)
+{
+	struct thead_dwmac *dwmac = plat->bsp_priv;
+	u32 txclk_dir;
+
+	switch (plat->mac_interface) {
+	case PHY_INTERFACE_MODE_MII:
+		txclk_dir = TXCLK_DIR_INPUT;
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		txclk_dir = TXCLK_DIR_OUTPUT;
+		break;
+	default:
+		dev_err(dwmac->dev, "unsupported phy interface %d\n",
+			plat->mac_interface);
+		return -EINVAL;
+	};
+
+	return regmap_write(dwmac->apb_regmap, GMAC_TXCLK_OEN, txclk_dir);
+}
+
+static void thead_dwmac_fix_speed(void *priv, unsigned int speed, unsigned int mode)
+{
+	struct plat_stmmacenet_data *plat;
+	struct thead_dwmac *dwmac = priv;
+	unsigned long rate;
+	u32 div;
+
+	plat = dwmac->plat;
+
+	switch (plat->mac_interface) {
+	/* For MII, rxc/txc is provided by phy */
+	case PHY_INTERFACE_MODE_MII:
+		return;
+
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		rate = clk_get_rate(plat->stmmac_clk);
+		if (!rate || rate % GMAC_GMII_RGMII_RATE != 0 ||
+		    rate % GMAC_MII_RATE != 0) {
+			dev_err(dwmac->dev, "invalid gmac rate %ld\n", rate);
+			return;
+		}
+
+		regmap_update_bits(dwmac->apb_regmap, GMAC_PLLCLK_DIV, GMAC_PLLCLK_DIV_EN, 0);
+
+		switch (speed) {
+		case SPEED_1000:
+			div = rate / GMAC_GMII_RGMII_RATE;
+			break;
+		case SPEED_100:
+			div = rate / GMAC_MII_RATE;
+			break;
+		case SPEED_10:
+			div = rate * 10 / GMAC_MII_RATE;
+			break;
+		default:
+			dev_err(dwmac->dev, "invalid speed %u\n", speed);
+			return;
+		}
+		regmap_update_bits(dwmac->apb_regmap, GMAC_PLLCLK_DIV,
+				   GMAC_PLLCLK_DIV_MASK, GMAC_PLLCLK_DIV_NUM(div));
+
+		regmap_update_bits(dwmac->apb_regmap, GMAC_PLLCLK_DIV,
+				   GMAC_PLLCLK_DIV_EN, GMAC_PLLCLK_DIV_EN);
+		break;
+	default:
+		dev_err(dwmac->dev, "unsupported phy interface %d\n",
+			plat->mac_interface);
+		return;
+	}
+}
+
+static int thead_dwmac_enable_clk(struct plat_stmmacenet_data *plat)
+{
+	struct thead_dwmac *dwmac = plat->bsp_priv;
+	int err;
+	u32 reg;
+
+	switch (plat->mac_interface) {
+	case PHY_INTERFACE_MODE_MII:
+		reg = GMAC_RX_CLK_EN | GMAC_TX_CLK_EN;
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		/* use pll */
+		err = regmap_write(dwmac->apb_regmap, GMAC_GTXCLK_SEL, GMAC_GTXCLK_SEL_PLL);
+		if (err)
+			return dev_err_probe(dwmac->dev, err,
+					     "failed to set phy interface\n");
+
+		reg = GMAC_TX_CLK_EN | GMAC_TX_CLK_N_EN | GMAC_TX_CLK_OUT_EN |
+		      GMAC_RX_CLK_EN | GMAC_RX_CLK_N_EN;
+		break;
+
+	default:
+		dev_err(dwmac->dev, "unsupported phy interface %d\n",
+			plat->mac_interface);
+		return -EINVAL;
+	}
+
+	return regmap_write(dwmac->apb_regmap, GMAC_CLK_EN, reg);
+}
+
+static int thead_dwmac_init(struct platform_device *pdev, void *priv)
+{
+	struct thead_dwmac *dwmac = priv;
+	int ret;
+
+	ret = thead_dwmac_set_phy_if(dwmac->plat);
+	if (ret)
+		return ret;
+
+	ret = thead_dwmac_set_txclk_dir(dwmac->plat);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(dwmac->apb_regmap, GMAC_RXCLK_DELAY_CTRL,
+			   GMAC_RXCLK_DELAY_VAL(dwmac->rx_delay));
+	if (ret)
+		return dev_err_probe(dwmac->dev, ret,
+				     "failed to set GMAC RX clock delay\n");
+
+	ret = regmap_write(dwmac->apb_regmap, GMAC_TXCLK_DELAY_CTRL,
+			   GMAC_TXCLK_DELAY_VAL(dwmac->tx_delay));
+	if (ret)
+		return dev_err_probe(dwmac->dev, ret,
+				     "failed to set GMAC TX clock delay\n");
+
+	thead_dwmac_fix_speed(dwmac, SPEED_1000, 0);
+
+	return thead_dwmac_enable_clk(dwmac->plat);
+}
+
+static int thead_dwmac_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct stmmac_resources stmmac_res;
+	struct plat_stmmacenet_data *plat;
+	struct thead_dwmac *dwmac;
+	void __iomem *apb;
+	u32 delay;
+	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 = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat))
+		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
+				     "dt configuration failed\n");
+
+	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
+	if (!dwmac)
+		return -ENOMEM;
+
+	/* hardware default is 0 for the rx and tx internal clock delay */
+	dwmac->rx_delay = 0;
+	dwmac->tx_delay = 0;
+
+	/* rx and tx internal delay properties are optional */
+	if (!of_property_read_u32(np, "thead,rx-internal-delay", &delay)) {
+		if (delay > GMAC_RXCLK_DELAY_MASK)
+			dev_warn(&pdev->dev,
+				 "thead,rx-internal-delay (%u) exceeds max (%lu)\n",
+				 delay, GMAC_RXCLK_DELAY_MASK);
+		else
+			dwmac->rx_delay = delay;
+	}
+
+	if (!of_property_read_u32(np, "thead,tx-internal-delay", &delay)) {
+		if (delay > GMAC_TXCLK_DELAY_MASK)
+			dev_warn(&pdev->dev,
+				 "thead,tx-internal-delay (%u) exceeds max (%lu)\n",
+				 delay, GMAC_TXCLK_DELAY_MASK);
+		else
+			dwmac->tx_delay = delay;
+	}
+
+	apb = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(apb))
+		return dev_err_probe(&pdev->dev, PTR_ERR(apb),
+				     "Failed to remap gmac apb registers\n");
+
+	dwmac->apb_regmap = devm_regmap_init_mmio(&pdev->dev, apb, &regmap_config);
+	if (IS_ERR(dwmac->apb_regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->apb_regmap),
+				     "Failed to access gmac apb registers\n");
+
+	dwmac->dev = &pdev->dev;
+	dwmac->plat = plat;
+	plat->bsp_priv = dwmac;
+	plat->fix_mac_speed = thead_dwmac_fix_speed;
+	plat->init = thead_dwmac_init;
+
+	return devm_stmmac_pltfr_probe(pdev, plat, &stmmac_res);
+}
+
+static const struct of_device_id thead_dwmac_match[] = {
+	{ .compatible = "thead,th1520-gmac" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, thead_dwmac_match);
+
+static struct platform_driver thead_dwmac_driver = {
+	.probe = thead_dwmac_probe,
+	.driver = {
+		.name = "thead-dwmac",
+		.pm = &stmmac_pltfr_pm_ops,
+		.of_match_table = thead_dwmac_match,
+	},
+};
+module_platform_driver(thead_dwmac_driver);
+
+MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
+MODULE_DESCRIPTION("T-HEAD DWMAC platform driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v2 2/3] net: stmmac: Add glue layer for T-HEAD TH1520 SoC

[Andrew Lunn]

> +static int thead_dwmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct thead_dwmac *dwmac = priv;
> +	int ret;
> +
> +	ret = thead_dwmac_set_phy_if(dwmac->plat);
> +	if (ret)
> +		return ret;
> +
> +	ret = thead_dwmac_set_txclk_dir(dwmac->plat);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(dwmac->apb_regmap, GMAC_RXCLK_DELAY_CTRL,
> +			   GMAC_RXCLK_DELAY_VAL(dwmac->rx_delay));
> +	if (ret)
> +		return dev_err_probe(dwmac->dev, ret,
> +				     "failed to set GMAC RX clock delay\n");
> +
> +	ret = regmap_write(dwmac->apb_regmap, GMAC_TXCLK_DELAY_CTRL,
> +			   GMAC_TXCLK_DELAY_VAL(dwmac->tx_delay));
> +	if (ret)
> +		return dev_err_probe(dwmac->dev, ret,
> +				     "failed to set GMAC TX clock delay\n");
> +
> +	thead_dwmac_fix_speed(dwmac, SPEED_1000, 0);

Is this needed? I would expect this to be called when the PHY has link
and you know the link speed. So why set it here?

> +
> +	return thead_dwmac_enable_clk(dwmac->plat);
> +}
> +
> +static int thead_dwmac_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct stmmac_resources stmmac_res;
> +	struct plat_stmmacenet_data *plat;
> +	struct thead_dwmac *dwmac;
> +	void __iomem *apb;
> +	u32 delay;
> +	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 = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> +	if (IS_ERR(plat))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> +				     "dt configuration failed\n");
> +
> +	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> +	if (!dwmac)
> +		return -ENOMEM;
> +
> +	/* hardware default is 0 for the rx and tx internal clock delay */
> +	dwmac->rx_delay = 0;
> +	dwmac->tx_delay = 0;
> +
> +	/* rx and tx internal delay properties are optional */
> +	if (!of_property_read_u32(np, "thead,rx-internal-delay", &delay)) {
> +		if (delay > GMAC_RXCLK_DELAY_MASK)
> +			dev_warn(&pdev->dev,
> +				 "thead,rx-internal-delay (%u) exceeds max (%lu)\n",
> +				 delay, GMAC_RXCLK_DELAY_MASK);
> +		else
> +			dwmac->rx_delay = delay;
> +	}
> +

So you keep going, with an invalid value? It is better to use
dev_err() and return -EINVAL. The DT write will then correct their
error when the device fails to probe.

If you decide to keep this... I'm not sure these properties are
needed.

> +MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");

Please add a second author, if you have taken over this driver.

       Andrew

Re: [PATCH v2 2/3] net: stmmac: Add glue layer for T-HEAD TH1520 SoC

[Drew Fustini]

On Thu, Sep 26, 2024 at 08:32:00PM +0200, Andrew Lunn wrote:
> > +static int thead_dwmac_init(struct platform_device *pdev, void *priv)
> > +{
> > +	struct thead_dwmac *dwmac = priv;
> > +	int ret;
> > +
> > +	ret = thead_dwmac_set_phy_if(dwmac->plat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = thead_dwmac_set_txclk_dir(dwmac->plat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(dwmac->apb_regmap, GMAC_RXCLK_DELAY_CTRL,
> > +			   GMAC_RXCLK_DELAY_VAL(dwmac->rx_delay));
> > +	if (ret)
> > +		return dev_err_probe(dwmac->dev, ret,
> > +				     "failed to set GMAC RX clock delay\n");
> > +
> > +	ret = regmap_write(dwmac->apb_regmap, GMAC_TXCLK_DELAY_CTRL,
> > +			   GMAC_TXCLK_DELAY_VAL(dwmac->tx_delay));
> > +	if (ret)
> > +		return dev_err_probe(dwmac->dev, ret,
> > +				     "failed to set GMAC TX clock delay\n");
> > +
> > +	thead_dwmac_fix_speed(dwmac, SPEED_1000, 0);
> 
> Is this needed? I would expect this to be called when the PHY has link
> and you know the link speed. So why set it here?

Good point.  I've removed this line and the probe still completes okay
and the Ethernet connection is working okay.

> > +
> > +	return thead_dwmac_enable_clk(dwmac->plat);
> > +}
> > +
> > +static int thead_dwmac_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct stmmac_resources stmmac_res;
> > +	struct plat_stmmacenet_data *plat;
> > +	struct thead_dwmac *dwmac;
> > +	void __iomem *apb;
> > +	u32 delay;
> > +	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 = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > +	if (IS_ERR(plat))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(plat),
> > +				     "dt configuration failed\n");
> > +
> > +	dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> > +	if (!dwmac)
> > +		return -ENOMEM;
> > +
> > +	/* hardware default is 0 for the rx and tx internal clock delay */
> > +	dwmac->rx_delay = 0;
> > +	dwmac->tx_delay = 0;
> > +
> > +	/* rx and tx internal delay properties are optional */
> > +	if (!of_property_read_u32(np, "thead,rx-internal-delay", &delay)) {
> > +		if (delay > GMAC_RXCLK_DELAY_MASK)
> > +			dev_warn(&pdev->dev,
> > +				 "thead,rx-internal-delay (%u) exceeds max (%lu)\n",
> > +				 delay, GMAC_RXCLK_DELAY_MASK);
> > +		else
> > +			dwmac->rx_delay = delay;
> > +	}
> > +
> 
> So you keep going, with an invalid value? It is better to use
> dev_err() and return -EINVAL. The DT write will then correct their
> error when the device fails to probe.

My intention was to keep the default of 0 if the dt property exists and
exceeds the max value. I had considered failing the probe but I wasn't
sure that was too severe of a reaction to a bad value for the delay.

> 
> If you decide to keep this... I'm not sure these properties are
> needed.

Given your reply to the cover letter, I think it does make sense for me
to remove handling of these delay properties since the units of the
delay bit field are unknown and the hardware I have is okay with the
default delay.

> 
> > +MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
> 
> Please add a second author, if you have taken over this driver.

Yes, Jisheng is no longer working on it, so I will add myself.

Thanks,
Drew

[PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

From: Emil Renner Berthing <emil.renner.berthing@canonical.com>

Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
[drew: change apb registers from syscon to second reg of gmac node]
[drew: add phy reset delay properties for beaglev ahead]
Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
---
 arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts |  91 ++++++++++++++
 .../boot/dts/thead/th1520-lichee-module-4a.dtsi    | 135 +++++++++++++++++++++
 arch/riscv/boot/dts/thead/th1520.dtsi              |  50 ++++++++
 3 files changed, 276 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
index 5a5888f4eda6..ddcee6298939 100644
--- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
+++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
@@ -15,6 +15,7 @@ / {
 	compatible = "beagle,beaglev-ahead", "thead,th1520";
 
 	aliases {
+		ethernet0 = &gmac0;
 		gpio0 = &gpio0;
 		gpio1 = &gpio1;
 		gpio2 = &gpio2;
@@ -108,6 +109,25 @@ &sdio0 {
 	status = "okay";
 };
 
+&gmac0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac0_pins>;
+	phy-handle = <&phy0>;
+	phy-mode = "rgmii-id";
+	status = "okay";
+};
+
+&mdio0 {
+	phy0: ethernet-phy@1 {
+		reg = <1>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
+		reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
+		reset-delay-us = <10000>;
+		reset-post-delay-us = <50000>;
+	};
+};
+
 &padctrl_aosys {
 	led_pins: led-0 {
 		led-pins {
@@ -127,6 +147,77 @@ led-pins {
 };
 
 &padctrl0_apsys {
+	gmac0_pins: gmac0-0 {
+		tx-pins {
+			pins = "GMAC0_TX_CLK",
+			       "GMAC0_TXEN",
+			       "GMAC0_TXD0",
+			       "GMAC0_TXD1",
+			       "GMAC0_TXD2",
+			       "GMAC0_TXD3";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <25>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		rx-pins {
+			pins = "GMAC0_RX_CLK",
+			       "GMAC0_RXDV",
+			       "GMAC0_RXD0",
+			       "GMAC0_RXD1",
+			       "GMAC0_RXD2",
+			       "GMAC0_RXD3";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <1>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		mdc-pins {
+			pins = "GMAC0_MDC";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <13>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		mdio-pins {
+			pins = "GMAC0_MDIO";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <13>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+
+		phy-reset-pins {
+			pins = "GMAC0_COL"; /* GPIO3_21 */
+			bias-disable;
+			drive-strength = <3>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		phy-interrupt-pins {
+			pins = "GMAC0_CRS"; /* GPIO3_22 */
+			function = "gpio";
+			bias-pull-up;
+			drive-strength = <1>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+	};
+
 	uart0_pins: uart0-0 {
 		tx-pins {
 			pins = "UART0_TXD";
diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
index ca84bc2039ef..d9d2e1f4dc68 100644
--- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
@@ -11,6 +11,11 @@ / {
 	model = "Sipeed Lichee Module 4A";
 	compatible = "sipeed,lichee-module-4a", "thead,th1520";
 
+	aliases {
+		ethernet0 = &gmac0;
+		ethernet1 = &gmac1;
+	};
+
 	memory@0 {
 		device_type = "memory";
 		reg = <0x0 0x00000000 0x2 0x00000000>;
@@ -25,6 +30,16 @@ &osc_32k {
 	clock-frequency = <32768>;
 };
 
+&dmac0 {
+	status = "okay";
+};
+
+&aogpio {
+	gpio-line-names = "", "", "",
+			  "GPIO00",
+			  "GPIO04";
+};
+
 &aonsys_clk {
 	clock-frequency = <73728000>;
 };
@@ -55,6 +70,22 @@ &sdio0 {
 	status = "okay";
 };
 
+&gmac0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac0_pins>, <&mdio0_pins>;
+	phy-handle = <&phy0>;
+	phy-mode = "rgmii-id";
+	status = "okay";
+};
+
+&gmac1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac1_pins>;
+	phy-handle = <&phy1>;
+	phy-mode = "rgmii-id";
+	status = "okay";
+};
+
 &gpio0 {
 	gpio-line-names = "", "", "", "", "", "", "", "", "", "",
 			  "", "", "", "", "", "", "", "", "", "",
@@ -87,3 +118,107 @@ &gpio3 {
 			  "GPIO09",
 			  "GPIO10";
 };
+
+&mdio0 {
+	phy0: ethernet-phy@1 {
+		reg = <1>;
+	};
+
+	phy1: ethernet-phy@2 {
+		reg = <2>;
+	};
+};
+
+&padctrl0_apsys {
+	gmac0_pins: gmac0-0 {
+		tx-pins {
+			pins = "GMAC0_TX_CLK",
+			       "GMAC0_TXEN",
+			       "GMAC0_TXD0",
+			       "GMAC0_TXD1",
+			       "GMAC0_TXD2",
+			       "GMAC0_TXD3";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <25>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		rx-pins {
+			pins = "GMAC0_RX_CLK",
+			       "GMAC0_RXDV",
+			       "GMAC0_RXD0",
+			       "GMAC0_RXD1",
+			       "GMAC0_RXD2",
+			       "GMAC0_RXD3";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <1>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
+	gmac1_pins: gmac1-0 {
+		tx-pins {
+			pins = "GPIO2_18", /* GMAC1_TX_CLK */
+			       "GPIO2_20", /* GMAC1_TXEN */
+			       "GPIO2_21", /* GMAC1_TXD0 */
+			       "GPIO2_22", /* GMAC1_TXD1 */
+			       "GPIO2_23", /* GMAC1_TXD2 */
+			       "GPIO2_24"; /* GMAC1_TXD3 */
+			function = "gmac1";
+			bias-disable;
+			drive-strength = <25>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		rx-pins {
+			pins = "GPIO2_19", /* GMAC1_RX_CLK */
+			       "GPIO2_25", /* GMAC1_RXDV */
+			       "GPIO2_30", /* GMAC1_RXD0 */
+			       "GPIO2_31", /* GMAC1_RXD1 */
+			       "GPIO3_0",  /* GMAC1_RXD2 */
+			       "GPIO3_1";  /* GMAC1_RXD3 */
+			function = "gmac1";
+			bias-disable;
+			drive-strength = <1>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
+	mdio0_pins: mdio0-0 {
+		mdc-pins {
+			pins = "GMAC0_MDC";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <13>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+
+		mdio-pins {
+			pins = "GMAC0_MDIO";
+			function = "gmac0";
+			bias-disable;
+			drive-strength = <13>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+	};
+};
+
+&sdio0 {
+	bus-width = <4>;
+	max-frequency = <198000000>;
+	status = "okay";
+};
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 517a9e2e93a3..f51cc0c465ee 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -222,6 +222,12 @@ aonsys_clk: aonsys-clk {
 		#clock-cells = <0>;
 	};
 
+	stmmac_axi_config: stmmac-axi-config {
+		snps,wr_osr_lmt = <15>;
+		snps,rd_osr_lmt = <15>;
+		snps,blen = <0 0 64 32 0 0 0>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
@@ -273,6 +279,50 @@ uart0: serial@ffe7014000 {
 			status = "disabled";
 		};
 
+		gmac1: ethernet@ffe7060000 {
+			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
+			reg = <0xff 0xe7060000 0x0 0x2000>, <0xff 0xec004000 0x0 0x1000>;
+			reg-names = "dwmac", "apb";
+			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
+			clock-names = "stmmaceth", "pclk";
+			snps,pbl = <32>;
+			snps,fixed-burst;
+			snps,multicast-filter-bins = <64>;
+			snps,perfect-filter-entries = <32>;
+			snps,axi-config = <&stmmac_axi_config>;
+			status = "disabled";
+
+			mdio1: mdio {
+				compatible = "snps,dwmac-mdio";
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
+		gmac0: ethernet@ffe7070000 {
+			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
+			reg = <0xff 0xe7070000 0x0 0x2000>, <0xff 0xec003000 0x0 0x1000>;
+			reg-names = "dwmac", "apb";
+			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "macirq";
+			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
+			clock-names = "stmmaceth", "pclk";
+			snps,pbl = <32>;
+			snps,fixed-burst;
+			snps,multicast-filter-bins = <64>;
+			snps,perfect-filter-entries = <32>;
+			snps,axi-config = <&stmmac_axi_config>;
+			status = "disabled";
+
+			mdio0: mdio {
+				compatible = "snps,dwmac-mdio";
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		emmc: mmc@ffe7080000 {
 			compatible = "thead,th1520-dwcmshc";
 			reg = <0xff 0xe7080000 0x0 0x10000>;

-- 
2.34.1

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Andrew Lunn]

> +&mdio0 {
> +	phy0: ethernet-phy@1 {
> +		reg = <1>;
> +	};
> +
> +	phy1: ethernet-phy@2 {
> +		reg = <2>;
> +	};
> +};

Two PHYs on one bus...

> +		gmac1: ethernet@ffe7060000 {
> +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> +			reg = <0xff 0xe7060000 0x0 0x2000>, <0xff 0xec004000 0x0 0x1000>;
> +			reg-names = "dwmac", "apb";
> +			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "macirq";
> +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> +			clock-names = "stmmaceth", "pclk";
> +			snps,pbl = <32>;
> +			snps,fixed-burst;
> +			snps,multicast-filter-bins = <64>;
> +			snps,perfect-filter-entries = <32>;
> +			snps,axi-config = <&stmmac_axi_config>;
> +			status = "disabled";
> +
> +			mdio1: mdio {
> +				compatible = "snps,dwmac-mdio";
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
> +		gmac0: ethernet@ffe7070000 {
> +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> +			reg = <0xff 0xe7070000 0x0 0x2000>, <0xff 0xec003000 0x0 0x1000>;
> +			reg-names = "dwmac", "apb";
> +			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "macirq";
> +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;

And the MACs are listed in opposite order. Does gmac1 probe first,
find the PHY does not exist, and return -EPROBE_DEFER. Then gmac0
probes successfully, and then sometime later gmac1 then reprobes?

I know it is normal to list nodes in address order, but you might be
able to avoid the EPROBE_DEFER if you reverse the order.

     Andrew

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

On Thu, Sep 26, 2024 at 08:39:29PM +0200, Andrew Lunn wrote:
> > +&mdio0 {
> > +	phy0: ethernet-phy@1 {
> > +		reg = <1>;
> > +	};
> > +
> > +	phy1: ethernet-phy@2 {
> > +		reg = <2>;
> > +	};
> > +};
> 
> Two PHYs on one bus...

Thanks for pointing this out. I will move phy1 to mdio1.

> 
> > +		gmac1: ethernet@ffe7060000 {
> > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > +			reg = <0xff 0xe7060000 0x0 0x2000>, <0xff 0xec004000 0x0 0x1000>;
> > +			reg-names = "dwmac", "apb";
> > +			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "macirq";
> > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> > +			clock-names = "stmmaceth", "pclk";
> > +			snps,pbl = <32>;
> > +			snps,fixed-burst;
> > +			snps,multicast-filter-bins = <64>;
> > +			snps,perfect-filter-entries = <32>;
> > +			snps,axi-config = <&stmmac_axi_config>;
> > +			status = "disabled";
> > +
> > +			mdio1: mdio {
> > +				compatible = "snps,dwmac-mdio";
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +			};
> > +		};
> > +
> > +		gmac0: ethernet@ffe7070000 {
> > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > +			reg = <0xff 0xe7070000 0x0 0x2000>, <0xff 0xec003000 0x0 0x1000>;
> > +			reg-names = "dwmac", "apb";
> > +			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "macirq";
> > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> 
> And the MACs are listed in opposite order. Does gmac1 probe first,
> find the PHY does not exist, and return -EPROBE_DEFER. Then gmac0
> probes successfully, and then sometime later gmac1 then reprobes?
> 
> I know it is normal to list nodes in address order, but you might be
> able to avoid the EPROBE_DEFER if you reverse the order.

The probe order seems to always be the ethernet@ffe7060000 (gmac1) first
and then ethernet@ffe7070000 (gmac0). I do not see any probe deferral
in the boot log [1].

Thanks,
Drew

[1] https://gist.github.com/pdp7/02a44b024bdb6be5fe61ac21303ab29a

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Andrew Lunn]

On Thu, Sep 26, 2024 at 12:13:06PM -0700, Drew Fustini wrote:
> On Thu, Sep 26, 2024 at 08:39:29PM +0200, Andrew Lunn wrote:
> > > +&mdio0 {
> > > +	phy0: ethernet-phy@1 {
> > > +		reg = <1>;
> > > +	};
> > > +
> > > +	phy1: ethernet-phy@2 {
> > > +		reg = <2>;
> > > +	};
> > > +};
> > 
> > Two PHYs on one bus...
> 
> Thanks for pointing this out. I will move phy1 to mdio1.

???

Are you saying the two PHYs are not on the same bus?

> > > +		gmac1: ethernet@ffe7060000 {
> > > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > > +			reg = <0xff 0xe7060000 0x0 0x2000>, <0xff 0xec004000 0x0 0x1000>;
> > > +			reg-names = "dwmac", "apb";
> > > +			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
> > > +			interrupt-names = "macirq";
> > > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> > > +			clock-names = "stmmaceth", "pclk";
> > > +			snps,pbl = <32>;
> > > +			snps,fixed-burst;
> > > +			snps,multicast-filter-bins = <64>;
> > > +			snps,perfect-filter-entries = <32>;
> > > +			snps,axi-config = <&stmmac_axi_config>;
> > > +			status = "disabled";
> > > +
> > > +			mdio1: mdio {
> > > +				compatible = "snps,dwmac-mdio";
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +			};
> > > +		};
> > > +
> > > +		gmac0: ethernet@ffe7070000 {
> > > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > > +			reg = <0xff 0xe7070000 0x0 0x2000>, <0xff 0xec003000 0x0 0x1000>;
> > > +			reg-names = "dwmac", "apb";
> > > +			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
> > > +			interrupt-names = "macirq";
> > > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> > 
> > And the MACs are listed in opposite order. Does gmac1 probe first,
> > find the PHY does not exist, and return -EPROBE_DEFER. Then gmac0
> > probes successfully, and then sometime later gmac1 then reprobes?
> > 
> > I know it is normal to list nodes in address order, but you might be
> > able to avoid the EPROBE_DEFER if you reverse the order.
> 
> The probe order seems to always be the ethernet@ffe7060000 (gmac1) first
> and then ethernet@ffe7070000 (gmac0). I do not see any probe deferral
> in the boot log [1].

> [1] https://gist.github.com/pdp7/02a44b024bdb6be5fe61ac21303ab29a

So two PHYs are found, so they must be on the same bus.

It could well be that this MAC driver does not connect to the PHY
until the interface is opened. That is a good 30 seconds after the
driver probes in this log message. So there has been plenty of time
for the PHYs to be found.

What would be interesting is if you used NFS root. Then the interface
would be opened much faster, and you might see an EPROBE_DEFER. But
i'm just speculating. If it works for you, there is no need to do
more.

      Andrew

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

On Thu, Sep 26, 2024 at 09:30:32PM +0200, Andrew Lunn wrote:
> On Thu, Sep 26, 2024 at 12:13:06PM -0700, Drew Fustini wrote:
> > On Thu, Sep 26, 2024 at 08:39:29PM +0200, Andrew Lunn wrote:
> > > > +&mdio0 {
> > > > +	phy0: ethernet-phy@1 {
> > > > +		reg = <1>;
> > > > +	};
> > > > +
> > > > +	phy1: ethernet-phy@2 {
> > > > +		reg = <2>;
> > > > +	};
> > > > +};
> > > 
> > > Two PHYs on one bus...
> > 
> > Thanks for pointing this out. I will move phy1 to mdio1.
> 
> ???
> 
> Are you saying the two PHYs are not on the same bus?

Sorry, this is my first time working on an Ethernet driver and I
misunderstood.

Sipeed only shares the schematic of the baseboard for the LPi4a [1].
There are pages for GMAC Ethernet0 and GMAC Ethernet1. Each shows 4 MDIO
differential pairs going into a SG4301G transformer.

I believe that RTL8211F-CG phy chips are on the LM4A SoM board which
contains the TH1520 SoC. Unfortunately, Sipeed does not provide a
schematic of the SoM so its hard for me to inspect how the phy chips are
wired up.

Vendor kernel [2] that Sipeed uses has:

	mdio0 {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "snps,dwmac-mdio";

		phy_88E1111_0: ethernet-phy@0 {
			reg = <0x1>;
		};

		phy_88E1111_1: ethernet-phy@1 {
			reg = <0x2>;
		};
	};

so I think that does mean they are on the same MDIO bus.

> 
> > > > +		gmac1: ethernet@ffe7060000 {
> > > > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > > > +			reg = <0xff 0xe7060000 0x0 0x2000>, <0xff 0xec004000 0x0 0x1000>;
> > > > +			reg-names = "dwmac", "apb";
> > > > +			interrupts = <67 IRQ_TYPE_LEVEL_HIGH>;
> > > > +			interrupt-names = "macirq";
> > > > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> > > > +			clock-names = "stmmaceth", "pclk";
> > > > +			snps,pbl = <32>;
> > > > +			snps,fixed-burst;
> > > > +			snps,multicast-filter-bins = <64>;
> > > > +			snps,perfect-filter-entries = <32>;
> > > > +			snps,axi-config = <&stmmac_axi_config>;
> > > > +			status = "disabled";
> > > > +
> > > > +			mdio1: mdio {
> > > > +				compatible = "snps,dwmac-mdio";
> > > > +				#address-cells = <1>;
> > > > +				#size-cells = <0>;
> > > > +			};
> > > > +		};
> > > > +
> > > > +		gmac0: ethernet@ffe7070000 {
> > > > +			compatible = "thead,th1520-gmac", "snps,dwmac-3.70a";
> > > > +			reg = <0xff 0xe7070000 0x0 0x2000>, <0xff 0xec003000 0x0 0x1000>;
> > > > +			reg-names = "dwmac", "apb";
> > > > +			interrupts = <66 IRQ_TYPE_LEVEL_HIGH>;
> > > > +			interrupt-names = "macirq";
> > > > +			clocks = <&clk CLK_GMAC_AXI>, <&clk CLK_GMAC_AXI>;
> > > 
> > > And the MACs are listed in opposite order. Does gmac1 probe first,
> > > find the PHY does not exist, and return -EPROBE_DEFER. Then gmac0
> > > probes successfully, and then sometime later gmac1 then reprobes?
> > > 
> > > I know it is normal to list nodes in address order, but you might be
> > > able to avoid the EPROBE_DEFER if you reverse the order.
> > 
> > The probe order seems to always be the ethernet@ffe7060000 (gmac1) first
> > and then ethernet@ffe7070000 (gmac0). I do not see any probe deferral
> > in the boot log [1].
> 
> > [1] https://gist.github.com/pdp7/02a44b024bdb6be5fe61ac21303ab29a
> 
> So two PHYs are found, so they must be on the same bus.
> 
> It could well be that this MAC driver does not connect to the PHY
> until the interface is opened. That is a good 30 seconds after the
> driver probes in this log message. So there has been plenty of time
> for the PHYs to be found.
> 
> What would be interesting is if you used NFS root. Then the interface
> would be opened much faster, and you might see an EPROBE_DEFER. But
> i'm just speculating. If it works for you, there is no need to do
> more.
> 
>       Andrew

I tried to setup an nfs server with a rootfs on my local network. I can
mount it okay from my laptop so I think it is working okay. However, it
does not seem to work on the lpi4a [3]. It appears the rgmii-id
validation fails and the dwmac driver can not open the phy:

 thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
 thead-dwmac ffe7060000.ethernet eth0: validation of rgmii-id with support \
             00,00000000,00000000,00006280 and advertisementa \
	     00,00000000,00000000,00006280 failed: -EINVAL
 thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -22)

I suppose that this is what you were talking about that NFS will cause
the interface to be opened much faster.

Thanks,
Drew

[1] https://dl.sipeed.com/shareURL/LICHEE/licheepi4a/02_Schematic
[2] https://github.com/revyos/thead-kernel/blob/lpi4a/arch/riscv/boot/dts/thead/th1520-b-product.dts#L758
[3] https://gist.github.com/pdp7/458eb93509548383beabeb21c8ffc43a

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Emil Renner Berthing]

Drew Fustini wrote:
> On Thu, Sep 26, 2024 at 09:30:32PM +0200, Andrew Lunn wrote:
> > On Thu, Sep 26, 2024 at 12:13:06PM -0700, Drew Fustini wrote:
> > > On Thu, Sep 26, 2024 at 08:39:29PM +0200, Andrew Lunn wrote:
> > > > > +&mdio0 {
> > > > > +	phy0: ethernet-phy@1 {
> > > > > +		reg = <1>;
> > > > > +	};
> > > > > +
> > > > > +	phy1: ethernet-phy@2 {
> > > > > +		reg = <2>;
> > > > > +	};
> > > > > +};
> > > >
> > > > Two PHYs on one bus...
> > >
> > > Thanks for pointing this out. I will move phy1 to mdio1.
> >
> > ???
> >
> > Are you saying the two PHYs are not on the same bus?
>
> Sorry, this is my first time working on an Ethernet driver and I
> misunderstood.
>
> Sipeed only shares the schematic of the baseboard for the LPi4a [1].
> There are pages for GMAC Ethernet0 and GMAC Ethernet1. Each shows 4 MDIO
> differential pairs going into a SG4301G transformer.
>
> I believe that RTL8211F-CG phy chips are on the LM4A SoM board which
> contains the TH1520 SoC. Unfortunately, Sipeed does not provide a
> schematic of the SoM so its hard for me to inspect how the phy chips are
> wired up.
>
> Vendor kernel [2] that Sipeed uses has:
>
> 	mdio0 {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		compatible = "snps,dwmac-mdio";
>
> 		phy_88E1111_0: ethernet-phy@0 {
> 			reg = <0x1>;
> 		};
>
> 		phy_88E1111_1: ethernet-phy@1 {
> 			reg = <0x2>;
> 		};
> 	};
>
> so I think that does mean they are on the same MDIO bus.

It depends how you look at it. The SoC has two MACs and they can both
control their own MDIO bus. However MDIO of both MACs are pinmux'ed to
the same pins on the SoC. So the solution above just mux the pins to
GMAC0 and let that control both PHYs. Alternatively I guess one could
let each GMAC control their own phy on their own MDIO bus and then
switch pinmux settings everytime you need to need to talk to one or
the other.

/Emil

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Andrew Lunn]

> > Vendor kernel [2] that Sipeed uses has:
> >
> > 	mdio0 {
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 		compatible = "snps,dwmac-mdio";
> >
> > 		phy_88E1111_0: ethernet-phy@0 {
> > 			reg = <0x1>;
> > 		};
> >
> > 		phy_88E1111_1: ethernet-phy@1 {
> > 			reg = <0x2>;
> > 		};
> > 	};
> >
> > so I think that does mean they are on the same MDIO bus.
> 
> It depends how you look at it. The SoC has two MACs and they can both
> control their own MDIO bus. However MDIO of both MACs are pinmux'ed to
> the same pins on the SoC.

Ah. That is unusual. 

> So the solution above just mux the pins to GMAC0 and let that
> control both PHYs.

That makes sense. Using both MDIO bus controllers and playing with the
pinmux on each transaction would be a lot more complex.

	Andrew

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Andrew Lunn]

> I tried to setup an nfs server with a rootfs on my local network. I can
> mount it okay from my laptop so I think it is working okay. However, it
> does not seem to work on the lpi4a [3]. It appears the rgmii-id
> validation fails and the dwmac driver can not open the phy:
> 
>  thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
>  thead-dwmac ffe7060000.ethernet eth0: validation of rgmii-id with support \
>              00,00000000,00000000,00006280 and advertisementa \
> 	     00,00000000,00000000,00006280 failed: -EINVAL
>  thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -22)

Given what Emil said, i would suggest flipping the MDIO busses
around. Put the PHYs on gmac1's MDIO bus, and set the pinmux so that
its MDIO bus controller is connected to the outside world. Then, when
gmac1 probes first, its MDIO bus will be probed at the same time, and
its PHY found.

	Andrew

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

On Fri, Sep 27, 2024 at 01:58:40PM +0200, Andrew Lunn wrote:
> > I tried to setup an nfs server with a rootfs on my local network. I can
> > mount it okay from my laptop so I think it is working okay. However, it
> > does not seem to work on the lpi4a [3]. It appears the rgmii-id
> > validation fails and the dwmac driver can not open the phy:
> > 
> >  thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
> >  thead-dwmac ffe7060000.ethernet eth0: validation of rgmii-id with support \
> >              00,00000000,00000000,00006280 and advertisementa \
> > 	     00,00000000,00000000,00006280 failed: -EINVAL
> >  thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -22)
> 
> Given what Emil said, i would suggest flipping the MDIO busses
> around. Put the PHYs on gmac1's MDIO bus, and set the pinmux so that
> its MDIO bus controller is connected to the outside world. Then, when
> gmac1 probes first, its MDIO bus will be probed at the same time, and
> its PHY found.
> 
> 	Andrew

I'm trying to configure the pinmux to have gmac1 control the mdio bus
but it seems I've not done so correctly. I changed pins "GMAC0_MDC" and
"GMAC0_MDIO" to function "gmac1" (see the patch below).

I don't see any errors about the dwmac or phy in the boot log [1] but
ultimately there is no carrier detected and the ethernet interface does
not come up.

Section "3.3.4.103 G3_MUXCFG_007" in the TH1520 System User Manual shows
that bits [19:16] control GMAC0_MDIO_MUX_CFG where value of 2 selects
GMAC1_MDIO. Similarly, bits [15:12] control GMAC0_MDC_MUX_CFG where a
value of 2 also selects GMAC1_MDC.

Emil - do you have any suggestion as to what I might be doing wrong with
the pinmux?

Thanks,
Drew

[1] https://gist.github.com/pdp7/1f9fcd76f26acd5715398d54f65a2e27

diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
index ca84bc2039ef..f2f6c9d9b590 100644
--- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
@@ -11,6 +11,11 @@ / {
        model = "Sipeed Lichee Module 4A";
        compatible = "sipeed,lichee-module-4a", "thead,th1520";

+       aliases {
+               ethernet0 = &gmac0;
+               ethernet1 = &gmac1;
+       };
+
        memory@0 {
                device_type = "memory";
                reg = <0x0 0x00000000 0x2 0x00000000>;
@@ -55,6 +60,22 @@ &sdio0 {
        status = "okay";
 };

+&gmac0 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&gmac0_pins>;
+       phy-handle = <&phy0>;
+       phy-mode = "rgmii-id";
+       status = "okay";
+};
+
+&gmac1 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&gmac1_pins>, <&mdio1_pins>;
+       phy-handle = <&phy1>;
+       phy-mode = "rgmii-id";
+       status = "okay";
+};
+
 &gpio0 {
        gpio-line-names = "", "", "", "", "", "", "", "", "", "",
                          "", "", "", "", "", "", "", "", "", "",
@@ -87,3 +108,101 @@ &gpio3 {
                          "GPIO09",
                          "GPIO10";
 };
+
+&mdio1 {
+       phy0: ethernet-phy@1 {
+               reg = <1>;
+       };
+
+       phy1: ethernet-phy@2 {
+               reg = <2>;
+       };
+};
+
+&padctrl0_apsys {
+       gmac0_pins: gmac0-0 {
+               tx-pins {
+                       pins = "GMAC0_TX_CLK",
+                              "GMAC0_TXEN",
+                              "GMAC0_TXD0",
+                              "GMAC0_TXD1",
+                              "GMAC0_TXD2",
+                              "GMAC0_TXD3";
+                       function = "gmac0";
+                       bias-disable;
+                       drive-strength = <25>;
+                       input-disable;
+                       input-schmitt-disable;
+                       slew-rate = <0>;
+               };
+
+               rx-pins {
+                       pins = "GMAC0_RX_CLK",
+                              "GMAC0_RXDV",
+                              "GMAC0_RXD0",
+                              "GMAC0_RXD1",
+                              "GMAC0_RXD2",
+                              "GMAC0_RXD3";
+                       function = "gmac0";
+                       bias-disable;
+                       drive-strength = <1>;
+                       input-enable;
+                       input-schmitt-disable;
+                       slew-rate = <0>;
+               };
+       };
+
+       gmac1_pins: gmac1-0 {
+               tx-pins {
+                       pins = "GPIO2_18", /* GMAC1_TX_CLK */
+                              "GPIO2_20", /* GMAC1_TXEN */
+                              "GPIO2_21", /* GMAC1_TXD0 */
+                              "GPIO2_22", /* GMAC1_TXD1 */
+                              "GPIO2_23", /* GMAC1_TXD2 */
+                              "GPIO2_24"; /* GMAC1_TXD3 */
+                       function = "gmac1";
+                       bias-disable;
+                       drive-strength = <25>;
+                       input-disable;
+                       input-schmitt-disable;
+                       slew-rate = <0>;
+               };
+
+               rx-pins {
+                       pins = "GPIO2_19", /* GMAC1_RX_CLK */
+                              "GPIO2_25", /* GMAC1_RXDV */
+                              "GPIO2_30", /* GMAC1_RXD0 */
+                              "GPIO2_31", /* GMAC1_RXD1 */
+                              "GPIO3_0",  /* GMAC1_RXD2 */
+                              "GPIO3_1";  /* GMAC1_RXD3 */
+                       function = "gmac1";
+                       bias-disable;
+                       drive-strength = <1>;
+                       input-enable;
+                       input-schmitt-disable;
+                       slew-rate = <0>;
+               };
+       };
+
+       mdio1_pins: mdio1-0 {
+               mdc-pins {
+                       pins = "GMAC0_MDC";
+                       function = "gmac1";
+                       bias-disable;
+                       drive-strength = <13>;
+                       input-disable;
+                       input-schmitt-disable;
+                       slew-rate = <0>;
+               };
+
+               mdio-pins {
+                       pins = "GMAC0_MDIO";
+                       function = "gmac1";
+                       bias-disable;
+                       drive-strength = <13>;
+                       input-enable;
+                       input-schmitt-enable;
+                       slew-rate = <0>;
+               };
+       };
+};

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

On Sat, Sep 28, 2024 at 02:05:13PM -0700, Drew Fustini wrote:
> On Fri, Sep 27, 2024 at 01:58:40PM +0200, Andrew Lunn wrote:
> > > I tried to setup an nfs server with a rootfs on my local network. I can
> > > mount it okay from my laptop so I think it is working okay. However, it
> > > does not seem to work on the lpi4a [3]. It appears the rgmii-id
> > > validation fails and the dwmac driver can not open the phy:
> > > 
> > >  thead-dwmac ffe7060000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
> > >  thead-dwmac ffe7060000.ethernet eth0: validation of rgmii-id with support \
> > >              00,00000000,00000000,00006280 and advertisementa \
> > > 	     00,00000000,00000000,00006280 failed: -EINVAL
> > >  thead-dwmac ffe7060000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -22)
> > 
> > Given what Emil said, i would suggest flipping the MDIO busses
> > around. Put the PHYs on gmac1's MDIO bus, and set the pinmux so that
> > its MDIO bus controller is connected to the outside world. Then, when
> > gmac1 probes first, its MDIO bus will be probed at the same time, and
> > its PHY found.
> > 
> > 	Andrew
> 
> I'm trying to configure the pinmux to have gmac1 control the mdio bus
> but it seems I've not done so correctly. I changed pins "GMAC0_MDC" and
> "GMAC0_MDIO" to function "gmac1" (see the patch below).
> 
> I don't see any errors about the dwmac or phy in the boot log [1] but
> ultimately there is no carrier detected and the ethernet interface does
> not come up.
> 
> Section "3.3.4.103 G3_MUXCFG_007" in the TH1520 System User Manual shows
> that bits [19:16] control GMAC0_MDIO_MUX_CFG where value of 2 selects
> GMAC1_MDIO. Similarly, bits [15:12] control GMAC0_MDC_MUX_CFG where a
> value of 2 also selects GMAC1_MDC.
> 
> Emil - do you have any suggestion as to what I might be doing wrong with
> the pinmux?

I've been thinking about this and I don't think there is any problem on
the LPi4a. Both Ethernet jacks work when I have the mdio bus muxed for
gmac0 to control it. It seems to control both phy's okay.

Earlier, I tried to setup nfs root but it didn't work. I believe this is
just due to my ignorance of how to configure it correctly. I've instead
switched to just adding 'ip=dhcp' to the kernel command. This causes
stmmac_open() to happen shortly after the thead-dwmac is probed for both
gmac0 and gmac1. The phy is found for both and there are no errors.

Without 'ip=dhcp', stmmac_open() is not called until around 40 seconds
into boot when NetworkManager tries to open it. Everything works
correctly but the delay of over 30 seconds from thead-dwmac probe to
interface up looks odd in the logs, but I am pretty sure this is just
due the point in time at which NetworkManager decides to bring up
the network interfaces.

Booting with gmac0 connected to Ethernet cable:
https://gist.github.com/pdp7/e1a8e7666706c7d3c99b6b7a3b43f070

Booting with gmac1 connected to Ethernet cable:
https://gist.github.com/pdp7/8a9c2066a2c20377ec3b479213b9be4c

thead-dwmac probe for gmac happens around 6 seconds. stmmac_open()
occurs shortly after that. The interface is up by around 10 seconds
into boot. DHCP request works okay and the interface is up and working
once the shell is ready.

In short, I believe the dts configuration in patch 3/3 of this series
works okay for both Ethernet ports on the LPi4a.

Thanks,
Drew

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Emil Renner Berthing]

Drew Fustini wrote:
> From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>
> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> [drew: change apb registers from syscon to second reg of gmac node]
> [drew: add phy reset delay properties for beaglev ahead]
> Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
> ---
>  arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts |  91 ++++++++++++++
>  .../boot/dts/thead/th1520-lichee-module-4a.dtsi    | 135 +++++++++++++++++++++
>  arch/riscv/boot/dts/thead/th1520.dtsi              |  50 ++++++++
>  3 files changed, 276 insertions(+)

...

> diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> index ca84bc2039ef..d9d2e1f4dc68 100644
> --- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> @@ -11,6 +11,11 @@ / {
>  	model = "Sipeed Lichee Module 4A";
>  	compatible = "sipeed,lichee-module-4a", "thead,th1520";
>
> +	aliases {
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +	};
> +
>  	memory@0 {
>  		device_type = "memory";
>  		reg = <0x0 0x00000000 0x2 0x00000000>;
> @@ -25,6 +30,16 @@ &osc_32k {
>  	clock-frequency = <32768>;
>  };
>
> +&dmac0 {
> +	status = "okay";
> +};
> +
> +&aogpio {
> +	gpio-line-names = "", "", "",
> +			  "GPIO00",
> +			  "GPIO04";
> +};
> +

These GPIO line names does not belong in this patch. They should
already be included in your other patchset adding the names for the
other lines.

/Emil

Re: [PATCH v2 3/3] riscv: dts: thead: Add TH1520 ethernet nodes

[Drew Fustini]

On Fri, Sep 27, 2024 at 05:55:04AM -0700, Emil Renner Berthing wrote:
> Drew Fustini wrote:
> > From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> >
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > [drew: change apb registers from syscon to second reg of gmac node]
> > [drew: add phy reset delay properties for beaglev ahead]
> > Signed-off-by: Drew Fustini <dfustini@tenstorrent.com>
> > ---
> >  arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts |  91 ++++++++++++++
> >  .../boot/dts/thead/th1520-lichee-module-4a.dtsi    | 135 +++++++++++++++++++++
> >  arch/riscv/boot/dts/thead/th1520.dtsi              |  50 ++++++++
> >  3 files changed, 276 insertions(+)
> 
> ...
> 
> > diff --git a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > index ca84bc2039ef..d9d2e1f4dc68 100644
> > --- a/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > +++ b/arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi
> > @@ -11,6 +11,11 @@ / {
> >  	model = "Sipeed Lichee Module 4A";
> >  	compatible = "sipeed,lichee-module-4a", "thead,th1520";
> >
> > +	aliases {
> > +		ethernet0 = &gmac0;
> > +		ethernet1 = &gmac1;
> > +	};
> > +
> >  	memory@0 {
> >  		device_type = "memory";
> >  		reg = <0x0 0x00000000 0x2 0x00000000>;
> > @@ -25,6 +30,16 @@ &osc_32k {
> >  	clock-frequency = <32768>;
> >  };
> >
> > +&dmac0 {
> > +	status = "okay";
> > +};
> > +
> > +&aogpio {
> > +	gpio-line-names = "", "", "",
> > +			  "GPIO00",
> > +			  "GPIO04";
> > +};
> > +
> 
> These GPIO line names does not belong in this patch. They should
> already be included in your other patchset adding the names for the
> other lines.
> 
> /Emil

Thanks, yeah, those sneaked in here.

Drew

Re: [PATCH v2 0/3] Add the dwmac driver support for T-HEAD TH1520 SoC

[Andrew Lunn]

> Regarding rx and tx internal delays, that same section in the manual
> doesn't specify what unit is represented by the delay_ctrl bit field in
> GMAC_RXCLK_DELAY_CTRL and GMAC_TXCLK_DELAY_CTRL. It is only 5 bits and
> a max value of 31 seems too small to represent picoseconds. The vendor
> kernel [2] uses properties named "rx-clk-delay" and "tx-clk-delay" but
> doesn't indicate any units. I see ti,dp83867.yaml adds vendor specific
> rx and tx delay properties so that is what I've now done in this series.
> Note: the hardware default value of 0 for delay_ctrl works okay for the
> TH1520 hardware that I have.

I assume you are talking about RGMII delays here?

Do you have a board which needs to set these delays? In general, linux
has the PHY provide the 2ns delay. You only need the MAC to add the
delays if a PHY is being used which cannot add the needed
delays. Occasionally you need to fine tune the delay, and the MAC
delays can then be interesting. But since you have no idea what the
units are, i would prefer to simply hard code it to 0, unless is it
really needed.

	Andrew

Re: [PATCH v2 0/3] Add the dwmac driver support for T-HEAD TH1520 SoC

[Drew Fustini]

On Thu, Sep 26, 2024 at 08:23:12PM +0200, Andrew Lunn wrote:
> > Regarding rx and tx internal delays, that same section in the manual
> > doesn't specify what unit is represented by the delay_ctrl bit field in
> > GMAC_RXCLK_DELAY_CTRL and GMAC_TXCLK_DELAY_CTRL. It is only 5 bits and
> > a max value of 31 seems too small to represent picoseconds. The vendor
> > kernel [2] uses properties named "rx-clk-delay" and "tx-clk-delay" but
> > doesn't indicate any units. I see ti,dp83867.yaml adds vendor specific
> > rx and tx delay properties so that is what I've now done in this series.
> > Note: the hardware default value of 0 for delay_ctrl works okay for the
> > TH1520 hardware that I have.
> 
> I assume you are talking about RGMII delays here?
> 
> Do you have a board which needs to set these delays? In general, linux
> has the PHY provide the 2ns delay. You only need the MAC to add the
> delays if a PHY is being used which cannot add the needed
> delays. Occasionally you need to fine tune the delay, and the MAC
> delays can then be interesting. But since you have no idea what the
> units are, i would prefer to simply hard code it to 0, unless is it
> really needed.
> 
> 	Andrew

Yes, this is for the RGMII delays. None of the TH1520 boards that I have
seem to need this. The hardware reset value is 0 which seems to work
okay.

I'll remove these custom properties in the next revision.

Thanks,
Drew