Skip to contentSkip to conversation

Introduce EN7581 ethernet support

PATCH Superseded Lorenzo Bianconi 4 participants 14 messages Jul 4, 2024 – Jul 8, 2024

Subsystems (5)

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Lorenzo Bianconi

author maintainer
#0 lore raw

v5 cover Introduce EN7581 ethernet support

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board. EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

Changes since v4:
- fix compilation warnings
- use airoha_qdma_rr() and not airoha_rr() in airoha_qdma_set_irqmask()
- add missing descriptions in dt-binding
- remove mdio node in binding example
Changes since v3:
- rework architecture to allow future gdm{1,4} support
- read REG_INT_ENABLE() register in airoha_qdma_set_irqmask() to guarantee airoha_qdma_wr() complete in the spinlock critical section - thx Arnd for the clarification
- remove unnecessary wmb()
- remove debugfs
- move register definitions in .c and remove .h
- fix warnings
- enable NAPI thread by default
Changes since v2:
- rename airoha,en7581.yaml in airoha,en7581-eth.yaml
- remove reset dependency in airoha,en7581-eth.yaml
- remove airoha_dev_change_mtu() callback
Changes since v1:
- drop patch 2/3
- remove queue lock for rx queues
- add bql support
- add ethtool stats support
- fix possible infinite loop in airoha_qdma_rx_process routine
- always destroy page_pool in case of error during initialization
- cosmetics

Lorenzo Bianconi (2):
dt-bindings: net: airoha: Add EN7581 ethernet controller
net: airoha: Introduce ethernet support for EN7581 SoC

 .../bindings/net/airoha,en7581-eth.yaml       |  146 +
 MAINTAINERS                                   |    9 +
 drivers/net/ethernet/mediatek/Kconfig         |   11 +-
 drivers/net/ethernet/mediatek/Makefile        |    1 +
 drivers/net/ethernet/mediatek/airoha_eth.c    | 2705 +++++++++++++++++
 5 files changed, 2871 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
 create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.c
Signature · 1 line
2.45.2
author maintainer
#1 lore raw

PATCH v5 1/2 dt-bindings: net: airoha: Add EN7581 ethernet controller

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +146 -0
new file mode 100644
index 000000000000..f4b1f8afddd0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#
+
+maintainers:
+  - Lorenzo Bianconi <[email protected]>
+
+description:
+  The frame engine ethernet controller can be found on Airoha SoCs.
+  These SoCs have multi-GMAC ports.
+
+properties:
+  compatible:
+    enum:
+      - airoha,en7581-eth
+
+  reg:
+    items:
+      - description: Frame engine base address
+      - description: QDMA0 base address
+      - description: QDMA1 base address
+
+  reg-names:
+    items:
+      - const: fe
+      - const: qdma0
+      - const: qdma1
+
+  interrupts:
+    items:
+      - description: QDMA lan irq0
+      - description: QDMA lan irq1
+      - description: QDMA lan irq2
+      - description: QDMA lan irq3
+      - description: QDMA wan irq0
+      - description: QDMA wan irq1
+      - description: QDMA wan irq2
+      - description: QDMA wan irq3
+      - description: FE error irq
+      - description: PDMA irq
+
+  resets:
+    maxItems: 8
+
+  reset-names:
+    items:
+      - const: fe
+      - const: pdma
+      - const: qdma
+      - const: xsi-mac
+      - const: hsi0-mac
+      - const: hsi1-mac
+      - const: hsi-mac
+      - const: xfp-mac
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^mac@[1-4]$":
+    type: object
+    unevaluatedProperties: false
+    $ref: ethernet-controller.yaml#
+    description:
+      Ethernet GMAC port associated to the MAC controller
+    properties:
+      compatible:
+        const: airoha,eth-mac
+
+      reg:
+        minimum: 1
+        maximum: 4
+        description: GMAC port identifier
+
+    required:
+      - reg
+      - compatible
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/en7523-clk.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      eth0: ethernet@1fb50000 {
+        compatible = "airoha,en7581-eth";
+        reg = <0 0x1fb50000 0 0x2600>,
+              <0 0x1fb54000 0 0x2000>,
+              <0 0x1fb56000 0 0x2000>;
+        reg-names = "fe", "qdma0", "qdma1";
+
+        resets = <&scuclk 44>,
+                 <&scuclk 30>,
+                 <&scuclk 31>,
+                 <&scuclk 6>,
+                 <&scuclk 15>,
+                 <&scuclk 16>,
+                 <&scuclk 17>,
+                 <&scuclk 26>;
+        reset-names = "fe", "pdma", "qdma", "xsi-mac",
+                      "hsi0-mac", "hsi1-mac", "hsi-mac",
+                      "xfp-mac";
+
+        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac1: mac@1 {
+          compatible = "airoha,eth-mac";
+          reg = <1>;
+        };
+      };
+    };
Signature · 1 line
2.45.2
maintainer
#2 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 15 lines · Lorenzo Bianconi

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +0 -0
new file mode 100644
index 000000000000..f4b1f8afddd0

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 10

Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +10 -0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#

Again, to rephrase, what are you using from this binding? It does not make sense for the parent and child both to use it.

Quoted 50 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +50 -0
+
+maintainers:
+  - Lorenzo Bianconi <[email protected]>
+
+description:
+  The frame engine ethernet controller can be found on Airoha SoCs.
+  These SoCs have multi-GMAC ports.
+
+properties:
+  compatible:
+    enum:
+      - airoha,en7581-eth
+
+  reg:
+    items:
+      - description: Frame engine base address
+      - description: QDMA0 base address
+      - description: QDMA1 base address
+
+  reg-names:
+    items:
+      - const: fe
+      - const: qdma0
+      - const: qdma1
+
+  interrupts:
+    items:
+      - description: QDMA lan irq0
+      - description: QDMA lan irq1
+      - description: QDMA lan irq2
+      - description: QDMA lan irq3
+      - description: QDMA wan irq0
+      - description: QDMA wan irq1
+      - description: QDMA wan irq2
+      - description: QDMA wan irq3
+      - description: FE error irq
+      - description: PDMA irq
+
+  resets:
+    maxItems: 8
+
+  reset-names:
+    items:
+      - const: fe
+      - const: pdma
+      - const: qdma
+      - const: xsi-mac
+      - const: hsi0-mac
+      - const: hsi1-mac
+      - const: hsi-mac

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 70

Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +10 -0
+      - const: xfp-mac
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^mac@[1-4]$":

'ethernet' is the defined node name for users of
ethernet-controller.yaml.

Quoted 79 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +76 -0
+    type: object
+    unevaluatedProperties: false
+    $ref: ethernet-controller.yaml#
+    description:
+      Ethernet GMAC port associated to the MAC controller
+    properties:
+      compatible:
+        const: airoha,eth-mac
+
+      reg:
+        minimum: 1
+        maximum: 4
+        description: GMAC port identifier
+
+    required:
+      - reg
+      - compatible
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/en7523-clk.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      eth0: ethernet@1fb50000 {
+        compatible = "airoha,en7581-eth";
+        reg = <0 0x1fb50000 0 0x2600>,
+              <0 0x1fb54000 0 0x2000>,
+              <0 0x1fb56000 0 0x2000>;
+        reg-names = "fe", "qdma0", "qdma1";
+
+        resets = <&scuclk 44>,
+                 <&scuclk 30>,
+                 <&scuclk 31>,
+                 <&scuclk 6>,
+                 <&scuclk 15>,
+                 <&scuclk 16>,
+                 <&scuclk 17>,
+                 <&scuclk 26>;
+        reset-names = "fe", "pdma", "qdma", "xsi-mac",
+                      "hsi0-mac", "hsi1-mac", "hsi-mac",
+                      "xfp-mac";
+
+        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac1: mac@1 {
+          compatible = "airoha,eth-mac";
+          reg = <1>;
+        };
+      };
+    };
Signature · 1 line
2.45.2
author maintainer
#3 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 21 lines · Lorenzo Bianconi
Quoted 20 lines

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +5 -0
new file mode 100644
index 000000000000..f4b1f8afddd0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
Quoted 8 lines

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 10

Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +5 -0
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#

Again, to rephrase, what are you using from this binding? It does not make sense for the parent and child both to use it.

Below I reported the ethernet dts node I am using (I have not posted the dts changes yet):

eth0: ethernet@1fb50000 {
	compatible = "airoha,en7581-eth";
	reg = <0 0x1fb50000 0 0x2600>,
	      <0 0x1fb54000 0 0x2000>,
	      <0 0x1fb56000 0 0x2000>;
	reg-names = "fe", "qdma0", "qdma1";

	resets = <&scuclk EN7581_FE_RST>,
		 <&scuclk EN7581_FE_PDMA_RST>,
		 <&scuclk EN7581_FE_QDMA_RST>,
		 <&scuclk EN7581_XSI_MAC_RST>,
		 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
		 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
		 <&scuclk EN7581_HSI_MAC_RST>,
		 <&scuclk EN7581_XFP_MAC_RST>;
	reset-names = "fe", "pdma", "qdma", "xsi-mac",
		      "hsi0-mac", "hsi1-mac", "hsi-mac",
		      "xfp-mac";

	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

	status = "disabled";

	#address-cells = <1>;
	#size-cells = <0>;

	gdm1: mac@1 {
		compatible = "airoha,eth-mac";
		reg = <1>;
		phy-mode = "internal";
		status = "disabled";

		fixed-link {
			speed = <1000>;
			full-duplex;
			pause;
		};
	};
};

I am using phy related binding for gdm1:mac@1 node. gdm1 is the GMAC port used as cpu port by the mt7530 dsa switch

switch: switch@1fb58000 {
	compatible = "airoha,en7581-switch";
	reg = <0 0x1fb58000 0 0x8000>;
	resets = <&scuclk EN7581_GSW_RST>;

	interrupt-controller;
	#interrupt-cells = <1>;
	interrupt-parent = <&gic>;
	interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;

	status = "disabled";

	#address-cells = <1>;
	#size-cells = <1>;

	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		gsw_port1: port@1 {
			reg = <1>;
			label = "lan1";
			phy-mode = "internal";
			phy-handle = <&gsw_phy1>;
		};

		gsw_port2: port@2 {
			reg = <2>;
			label = "lan2";
			phy-mode = "internal";
			phy-handle = <&gsw_phy2>;
		};

		gsw_port3: port@3 {
			reg = <3>;
			label = "lan3";
			phy-mode = "internal";
			phy-handle = <&gsw_phy3>;
		};

		gsw_port4: port@4 {
			reg = <4>;
			label = "lan4";
			phy-mode = "internal";
			phy-handle = <&gsw_phy4>;
		};

		port@6 {
			reg = <6>;
			label = "cpu";
			ethernet = <&gdm1>;
			phy-mode = "internal";

			fixed-link {
				speed = <1000>;
				full-duplex;
				pause;
			};
		};
	};

	mdio {
		#address-cells = <1>;
		#size-cells = <0>;

		gsw_phy1: ethernet-phy@1 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <9>;
			phy-mode = "internal";
		};

		gsw_phy2: ethernet-phy@2 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <10>;
			phy-mode = "internal";
		};

		gsw_phy3: ethernet-phy@3 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <11>;
			phy-mode = "internal";
		};

		gsw_phy4: ethernet-phy@4 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <12>;
			phy-mode = "internal";
		};
	};
};
Quoted 56 lines
Quoted 56 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +55 -0
+
+maintainers:
+  - Lorenzo Bianconi <[email protected]>
+
+description:
+  The frame engine ethernet controller can be found on Airoha SoCs.
+  These SoCs have multi-GMAC ports.
+
+properties:
+  compatible:
+    enum:
+      - airoha,en7581-eth
+
+  reg:
+    items:
+      - description: Frame engine base address
+      - description: QDMA0 base address
+      - description: QDMA1 base address
+
+  reg-names:
+    items:
+      - const: fe
+      - const: qdma0
+      - const: qdma1
+
+  interrupts:
+    items:
+      - description: QDMA lan irq0
+      - description: QDMA lan irq1
+      - description: QDMA lan irq2
+      - description: QDMA lan irq3
+      - description: QDMA wan irq0
+      - description: QDMA wan irq1
+      - description: QDMA wan irq2
+      - description: QDMA wan irq3
+      - description: FE error irq
+      - description: PDMA irq
+
+  resets:
+    maxItems: 8
+
+  reset-names:
+    items:
+      - const: fe
+      - const: pdma
+      - const: qdma
+      - const: xsi-mac
+      - const: hsi0-mac
+      - const: hsi1-mac
+      - const: hsi-mac
+      - const: xfp-mac
+
+  "#address-cells":
+    const: 1
+
Quoted 8 lines

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 70

Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +5 -0
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^mac@[1-4]$":

'ethernet' is the defined node name for users of
ethernet-controller.yaml.

Looking at the dts above, ethernet is already used by the parent node. This approach has been already used here [0],[1],[2]. Is it fine to reuse it?

Regards,
Lorenzo

[0] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622.dtsi#L964
[1] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts#L136
[2] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/mediatek%2Cnet.yaml#L370

Quoted 80 lines
Quoted 80 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +76 -0
+    type: object
+    unevaluatedProperties: false
+    $ref: ethernet-controller.yaml#
+    description:
+      Ethernet GMAC port associated to the MAC controller
+    properties:
+      compatible:
+        const: airoha,eth-mac
+
+      reg:
+        minimum: 1
+        maximum: 4
+        description: GMAC port identifier
+
+    required:
+      - reg
+      - compatible
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/clock/en7523-clk.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      eth0: ethernet@1fb50000 {
+        compatible = "airoha,en7581-eth";
+        reg = <0 0x1fb50000 0 0x2600>,
+              <0 0x1fb54000 0 0x2000>,
+              <0 0x1fb56000 0 0x2000>;
+        reg-names = "fe", "qdma0", "qdma1";
+
+        resets = <&scuclk 44>,
+                 <&scuclk 30>,
+                 <&scuclk 31>,
+                 <&scuclk 6>,
+                 <&scuclk 15>,
+                 <&scuclk 16>,
+                 <&scuclk 17>,
+                 <&scuclk 26>;
+        reset-names = "fe", "pdma", "qdma", "xsi-mac",
+                      "hsi0-mac", "hsi1-mac", "hsi-mac",
+                      "xfp-mac";
+
+        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mac1: mac@1 {
+          compatible = "airoha,eth-mac";
+          reg = <1>;
+        };
+      };
+    };
Signature · 1 line
2.45.2
maintainer
#4 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 31 lines · Lorenzo Bianconi
Quoted 22 lines · Lorenzo Bianconi
Quoted 20 lines

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +5 -0
new file mode 100644
index 000000000000..f4b1f8afddd0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
Quoted 8 lines

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 10

Quoted 5 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +5 -0
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#

Again, to rephrase, what are you using from this binding? It does not make sense for the parent and child both to use it.

Quoted 2 lines

Below I reported the ethernet dts node I am using (I have not posted the dts changes yet):

What happens when you remove this $ref? Nothing, because you use 0 properties from it. If none of the properties apply, then don't reference it. It is that simple.

Quoted 50 lines
eth0: ethernet@1fb50000 {
        compatible = "airoha,en7581-eth";
        reg = <0 0x1fb50000 0 0x2600>,
              <0 0x1fb54000 0 0x2000>,
              <0 0x1fb56000 0 0x2000>;
        reg-names = "fe", "qdma0", "qdma1";

        resets = <&scuclk EN7581_FE_RST>,
                 <&scuclk EN7581_FE_PDMA_RST>,
                 <&scuclk EN7581_FE_QDMA_RST>,
                 <&scuclk EN7581_XSI_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
                 <&scuclk EN7581_HSI_MAC_RST>,
                 <&scuclk EN7581_XFP_MAC_RST>;
        reset-names = "fe", "pdma", "qdma", "xsi-mac",
                      "hsi0-mac", "hsi1-mac", "hsi-mac",
                      "xfp-mac";

        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <0>;

        gdm1: mac@1 {
                compatible = "airoha,eth-mac";
                reg = <1>;
                phy-mode = "internal";
                status = "disabled";

                fixed-link {
                        speed = <1000>;
                        full-duplex;
                        pause;
                };
        };
};
Quoted 1 line

I am using phy related binding for gdm1:mac@1 node.

Right, so you should reference ethernet-controller.yaml for the mac node because you use properties from the schema.

Quoted 2 lines

gdm1 is the GMAC port used
as cpu port by the mt7530 dsa switch

That has nothing to do with *this* binding...

Quoted 84 lines
switch: switch@1fb58000 {
        compatible = "airoha,en7581-switch";
        reg = <0 0x1fb58000 0 0x8000>;
        resets = <&scuclk EN7581_GSW_RST>;

        interrupt-controller;
        #interrupt-cells = <1>;
        interrupt-parent = <&gic>;
        interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <1>;

        ports {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_port1: port@1 {
                        reg = <1>;
                        label = "lan1";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy1>;
                };

                gsw_port2: port@2 {
                        reg = <2>;
                        label = "lan2";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy2>;
                };

                gsw_port3: port@3 {
                        reg = <3>;
                        label = "lan3";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy3>;
                };

                gsw_port4: port@4 {
                        reg = <4>;
                        label = "lan4";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy4>;
                };

                port@6 {
                        reg = <6>;
                        label = "cpu";
                        ethernet = <&gdm1>;
                        phy-mode = "internal";

                        fixed-link {
                                speed = <1000>;
                                full-duplex;
                                pause;
                        };
                };
        };

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_phy1: ethernet-phy@1 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <9>;
                        phy-mode = "internal";
                };

                gsw_phy2: ethernet-phy@2 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <10>;
                        phy-mode = "internal";
                };

                gsw_phy3: ethernet-phy@3 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <11>;
                        phy-mode = "internal";
                };
Quoted 7 lines
                gsw_phy4: ethernet-phy@4 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <12>;
                        phy-mode = "internal";
                };
        };
};

None of this is relevant.

Quoted 7 lines
Quoted 5 lines

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 70

Quoted 2 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +2 -0
+patternProperties:
+  "^mac@[1-4]$":

'ethernet' is the defined node name for users of
ethernet-controller.yaml.

Looking at the dts above, ethernet is already used by the parent node.

So? Not really any reason a node named foo can't have a child named foo, too.

An 'ethernet' node should implement an ethernet interface. It is the child nodes that implement the ethernet interface(s). Whether you use 'ethernet' on the parent or not, I don't care too much.

Quoted 1 line

This approach has been already used here [0],[1],[2]. Is it fine to reuse it?

That one appears to be wrong too with the parent referencing ethernet-controller.yaml.

Rob

Quoted 6 lines

Regards,
Lorenzo

[0] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622.dtsi#L964
[1] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts#L136
[2] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/mediatek%2Cnet.yaml#L370

#5 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 49 lines
Quoted 49 lines
eth0: ethernet@1fb50000 {
        compatible = "airoha,en7581-eth";
        reg = <0 0x1fb50000 0 0x2600>,
              <0 0x1fb54000 0 0x2000>,
              <0 0x1fb56000 0 0x2000>;
        reg-names = "fe", "qdma0", "qdma1";

        resets = <&scuclk EN7581_FE_RST>,
                 <&scuclk EN7581_FE_PDMA_RST>,
                 <&scuclk EN7581_FE_QDMA_RST>,
                 <&scuclk EN7581_XSI_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
                 <&scuclk EN7581_HSI_MAC_RST>,
                 <&scuclk EN7581_XFP_MAC_RST>;
        reset-names = "fe", "pdma", "qdma", "xsi-mac",
                      "hsi0-mac", "hsi1-mac", "hsi-mac",
                      "xfp-mac";

        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <0>;

        gdm1: mac@1 {
                compatible = "airoha,eth-mac";
                reg = <1>;
                phy-mode = "internal";
                status = "disabled";

                fixed-link {
                        speed = <1000>;
                        full-duplex;
                        pause;
                };
        };
};
Quoted 1 line
Quoted 1 line

I am using phy related binding for gdm1:mac@1 node.

Hi Lorenzo

phy-mode is a MAC property, not a PHY property. Same for fixed-link. These are in ethernet-controller.yaml.

You sometimes have an network controller IP which has multiple MACs and some shared infrastructure. You would typically describe the shared infrastructure at the top level. The MACs are then listed as children, and they make use of ethernet-controller.yaml, and that is where all the network specific properties are placed. Is that what you are trying to do here?

Andrew

author maintainer
#6 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 49 lines
Quoted 49 lines
Quoted 49 lines
eth0: ethernet@1fb50000 {
        compatible = "airoha,en7581-eth";
        reg = <0 0x1fb50000 0 0x2600>,
              <0 0x1fb54000 0 0x2000>,
              <0 0x1fb56000 0 0x2000>;
        reg-names = "fe", "qdma0", "qdma1";

        resets = <&scuclk EN7581_FE_RST>,
                 <&scuclk EN7581_FE_PDMA_RST>,
                 <&scuclk EN7581_FE_QDMA_RST>,
                 <&scuclk EN7581_XSI_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
                 <&scuclk EN7581_HSI_MAC_RST>,
                 <&scuclk EN7581_XFP_MAC_RST>;
        reset-names = "fe", "pdma", "qdma", "xsi-mac",
                      "hsi0-mac", "hsi1-mac", "hsi-mac",
                      "xfp-mac";

        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <0>;

        gdm1: mac@1 {
                compatible = "airoha,eth-mac";
                reg = <1>;
                phy-mode = "internal";
                status = "disabled";

                fixed-link {
                        speed = <1000>;
                        full-duplex;
                        pause;
                };
        };
};
Quoted 3 lines
Quoted 1 line
Quoted 1 line

I am using phy related binding for gdm1:mac@1 node.

Hi Lorenzo

Hi Andrew,

Quoted 3 lines

phy-mode is a MAC property, not a PHY property. Same for fixed-link. These are in ethernet-controller.yaml.

ack

Quoted 7 lines

You sometimes have an network controller IP which has multiple MACs and some shared infrastructure. You would typically describe the shared infrastructure at the top level. The MACs are then listed as children, and they make use of ethernet-controller.yaml, and that is where all the network specific properties are placed. Is that what you are trying to do here?

yep, exactly. Here we have multiple mac nodes that represent the real ethernet controllers (e.g. used as DSA cpu port). I will use the ethernet-controller.yaml just for the mac childs.

Regards,
Lorenzo

Quoted 2 lines

Andrew

author maintainer
#7 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

In reply to Rob Herring
Quoted 32 lines · Lorenzo Bianconi
Quoted 31 lines
Quoted 30 lines · Lorenzo Bianconi
Quoted 15 lines

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +0 -0
new file mode 100644
index 000000000000..f4b1f8afddd0

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 10

Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +10 -0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#

Again, to rephrase, what are you using from this binding? It does not make sense for the parent and child both to use it.

Quoted 6 lines
Quoted 2 lines

Below I reported the ethernet dts node I am using (I have not posted the dts changes yet):

What happens when you remove this $ref? Nothing, because you use 0 properties from it. If none of the properties apply, then don't reference it. It is that simple.

if I get rid of "$ref: ethernet-controller.yaml#" here I get the following error using en7581-evb.dts (not posted upstream yet):

$make CHECK_DTBS=y DT_SCHEMA_FILES=airoha airoha/en7581-evb.dtb
UPD include/config/kernel.release
DTC_CHK arch/arm64/boot/dts/airoha/en7581-evb.dtb
/home/lorenzo/workspace/linux-mediatek/arch/arm64/boot/dts/airoha/en7581-evb.dtb: ethernet@1fb50000: mac@1: Unevaluated properties are not allowed ('fixed-link', 'phy-mode' were unexpected) from schema $id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#

Quoted 51 lines
Quoted 51 lines

eth0: ethernet@1fb50000 {
        compatible = "airoha,en7581-eth";
        reg = <0 0x1fb50000 0 0x2600>,
              <0 0x1fb54000 0 0x2000>,
              <0 0x1fb56000 0 0x2000>;
        reg-names = "fe", "qdma0", "qdma1";

        resets = <&scuclk EN7581_FE_RST>,
                 <&scuclk EN7581_FE_PDMA_RST>,
                 <&scuclk EN7581_FE_QDMA_RST>,
                 <&scuclk EN7581_XSI_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
                 <&scuclk EN7581_HSI_MAC_RST>,
                 <&scuclk EN7581_XFP_MAC_RST>;
        reset-names = "fe", "pdma", "qdma", "xsi-mac",
                      "hsi0-mac", "hsi1-mac", "hsi-mac",
                      "xfp-mac";

        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <0>;

        gdm1: mac@1 {
                compatible = "airoha,eth-mac";
                reg = <1>;
                phy-mode = "internal";
                status = "disabled";

                fixed-link {
                        speed = <1000>;
                        full-duplex;
                        pause;
                };
        };
};
Quoted 4 lines
Quoted 1 line

I am using phy related binding for gdm1:mac@1 node.

Right, so you should reference ethernet-controller.yaml for the mac node because you use properties from the schema.

ack. So, IIUC what you mean here, I need to get rid of "$ref: ethernet-controller.yaml#" in the parent node and just use in the mac node. Correct?

Quoted 103 lines
Quoted 3 lines

gdm1 is the GMAC port used
as cpu port by the mt7530 dsa switch

That has nothing to do with *this* binding...

Quoted 84 lines
switch: switch@1fb58000 {
        compatible = "airoha,en7581-switch";
        reg = <0 0x1fb58000 0 0x8000>;
        resets = <&scuclk EN7581_GSW_RST>;

        interrupt-controller;
        #interrupt-cells = <1>;
        interrupt-parent = <&gic>;
        interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <1>;

        ports {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_port1: port@1 {
                        reg = <1>;
                        label = "lan1";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy1>;
                };

                gsw_port2: port@2 {
                        reg = <2>;
                        label = "lan2";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy2>;
                };

                gsw_port3: port@3 {
                        reg = <3>;
                        label = "lan3";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy3>;
                };

                gsw_port4: port@4 {
                        reg = <4>;
                        label = "lan4";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy4>;
                };

                port@6 {
                        reg = <6>;
                        label = "cpu";
                        ethernet = <&gdm1>;
                        phy-mode = "internal";

                        fixed-link {
                                speed = <1000>;
                                full-duplex;
                                pause;
                        };
                };
        };

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_phy1: ethernet-phy@1 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <9>;
                        phy-mode = "internal";
                };

                gsw_phy2: ethernet-phy@2 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <10>;
                        phy-mode = "internal";
                };

                gsw_phy3: ethernet-phy@3 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <11>;
                        phy-mode = "internal";
                };
Quoted 7 lines
                gsw_phy4: ethernet-phy@4 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <12>;
                        phy-mode = "internal";
                };
        };
};

None of this is relevant.

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +2 -0
+patternProperties:
+  "^mac@[1-4]$":
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

'ethernet' is the defined node name for users of
ethernet-controller.yaml.

Looking at the dts above, ethernet is already used by the parent node.

So? Not really any reason a node named foo can't have a child named foo, too.

ack, fine. I will fix it in the next revision.

Quoted 4 lines

An 'ethernet' node should implement an ethernet interface. It is the child nodes that implement the ethernet interface(s). Whether you use 'ethernet' on the parent or not, I don't care too much.

ack, I will use "$ref: ethernet-controller.yaml#" just for the child in this case.

Regards,
Lorenzo

Quoted 14 lines
Quoted 2 lines

This approach has been already used here [0],[1],[2]. Is it fine to reuse it?

That one appears to be wrong too with the parent referencing ethernet-controller.yaml.

Rob

Quoted 6 lines

Regards,
Lorenzo

[0] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622.dtsi#L964
[1] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts#L136
[2] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/mediatek%2Cnet.yaml#L370

author maintainer
#8 lore raw

Re: [PATCH v5 net-next 1/2] dt-bindings: net: airoha: Add EN7581 ethernet controller

Quoted 39 lines
Quoted 32 lines · Lorenzo Bianconi
Quoted 31 lines
Quoted 30 lines · Lorenzo Bianconi
Quoted 15 lines

Introduce device-tree binding documentation for Airoha EN7581 ethernet mac controller.

---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +0 -0
new file mode 100644
index 000000000000..f4b1f8afddd0

Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml line 10

Quoted 10 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +10 -0
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Airoha EN7581 Frame Engine Ethernet controller
+
+allOf:
+  - $ref: ethernet-controller.yaml#

Again, to rephrase, what are you using from this binding? It does not make sense for the parent and child both to use it.

Quoted 6 lines
Quoted 2 lines

Below I reported the ethernet dts node I am using (I have not posted the dts changes yet):

What happens when you remove this $ref? Nothing, because you use 0 properties from it. If none of the properties apply, then don't reference it. It is that simple.

Quoted 8 lines

if I get rid of "$ref: ethernet-controller.yaml#" here I get the following error using en7581-evb.dts (not posted upstream yet):

$make CHECK_DTBS=y DT_SCHEMA_FILES=airoha airoha/en7581-evb.dtb
UPD include/config/kernel.release
DTC_CHK arch/arm64/boot/dts/airoha/en7581-evb.dtb
/home/lorenzo/workspace/linux-mediatek/arch/arm64/boot/dts/airoha/en7581-evb.dtb: ethernet@1fb50000: mac@1: Unevaluated properties are not allowed ('fixed-link', 'phy-mode' were unexpected) from schema $id: http://devicetree.org/schemas/net/airoha,en7581-eth.yaml#

actually I confused the parent "$ref: ethernet-controller.yaml#" with the child one. Please ignore the error above.

Regards
Lorenzo

Quoted 196 lines
Quoted 52 lines
Quoted 52 lines


eth0: ethernet@1fb50000 {
        compatible = "airoha,en7581-eth";
        reg = <0 0x1fb50000 0 0x2600>,
              <0 0x1fb54000 0 0x2000>,
              <0 0x1fb56000 0 0x2000>;
        reg-names = "fe", "qdma0", "qdma1";

        resets = <&scuclk EN7581_FE_RST>,
                 <&scuclk EN7581_FE_PDMA_RST>,
                 <&scuclk EN7581_FE_QDMA_RST>,
                 <&scuclk EN7581_XSI_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI0_MAC_RST>,
                 <&scuclk EN7581_DUAL_HSI1_MAC_RST>,
                 <&scuclk EN7581_HSI_MAC_RST>,
                 <&scuclk EN7581_XFP_MAC_RST>;
        reset-names = "fe", "pdma", "qdma", "xsi-mac",
                      "hsi0-mac", "hsi1-mac", "hsi-mac",
                      "xfp-mac";

        interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <0>;

        gdm1: mac@1 {
                compatible = "airoha,eth-mac";
                reg = <1>;
                phy-mode = "internal";
                status = "disabled";

                fixed-link {
                        speed = <1000>;
                        full-duplex;
                        pause;
                };
        };
};
Quoted 4 lines
Quoted 1 line

I am using phy related binding for gdm1:mac@1 node.

Right, so you should reference ethernet-controller.yaml for the mac node because you use properties from the schema.

ack. So, IIUC what you mean here, I need to get rid of "$ref: ethernet-controller.yaml#" in the parent node and just use in the mac node. Correct?

Quoted 103 lines
Quoted 3 lines

gdm1 is the GMAC port used
as cpu port by the mt7530 dsa switch

That has nothing to do with *this* binding...

Quoted 84 lines
switch: switch@1fb58000 {
        compatible = "airoha,en7581-switch";
        reg = <0 0x1fb58000 0 0x8000>;
        resets = <&scuclk EN7581_GSW_RST>;

        interrupt-controller;
        #interrupt-cells = <1>;
        interrupt-parent = <&gic>;
        interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;

        status = "disabled";

        #address-cells = <1>;
        #size-cells = <1>;

        ports {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_port1: port@1 {
                        reg = <1>;
                        label = "lan1";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy1>;
                };

                gsw_port2: port@2 {
                        reg = <2>;
                        label = "lan2";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy2>;
                };

                gsw_port3: port@3 {
                        reg = <3>;
                        label = "lan3";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy3>;
                };

                gsw_port4: port@4 {
                        reg = <4>;
                        label = "lan4";
                        phy-mode = "internal";
                        phy-handle = <&gsw_phy4>;
                };

                port@6 {
                        reg = <6>;
                        label = "cpu";
                        ethernet = <&gdm1>;
                        phy-mode = "internal";

                        fixed-link {
                                speed = <1000>;
                                full-duplex;
                                pause;
                        };
                };
        };

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                gsw_phy1: ethernet-phy@1 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <9>;
                        phy-mode = "internal";
                };

                gsw_phy2: ethernet-phy@2 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <10>;
                        phy-mode = "internal";
                };

                gsw_phy3: ethernet-phy@3 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <11>;
                        phy-mode = "internal";
                };
Quoted 7 lines
                gsw_phy4: ethernet-phy@4 {
                        compatible = "ethernet-phy-ieee802.3-c22";
                        reg = <12>;
                        phy-mode = "internal";
                };
        };
};

None of this is relevant.

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml new file +2 -0
+patternProperties:
+  "^mac@[1-4]$":
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

'ethernet' is the defined node name for users of
ethernet-controller.yaml.

Looking at the dts above, ethernet is already used by the parent node.

So? Not really any reason a node named foo can't have a child named foo, too.

ack, fine. I will fix it in the next revision.

Quoted 4 lines

An 'ethernet' node should implement an ethernet interface. It is the child nodes that implement the ethernet interface(s). Whether you use 'ethernet' on the parent or not, I don't care too much.

ack, I will use "$ref: ethernet-controller.yaml#" just for the child in this case.

Regards,
Lorenzo

Quoted 14 lines
Quoted 2 lines

This approach has been already used here [0],[1],[2]. Is it fine to reuse it?

That one appears to be wrong too with the parent referencing ethernet-controller.yaml.

Rob

Quoted 6 lines

Regards,
Lorenzo

[0] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622.dtsi#L964
[1] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts#L136
[2] https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/mediatek%2Cnet.yaml#L370

author maintainer
#9 lore raw

PATCH v5 2/2 net: airoha: Introduce ethernet support for EN7581 SoC

In reply to Lorenzo Bianconi

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

---
 MAINTAINERS                                |    9 +
 drivers/net/ethernet/mediatek/Kconfig      |   11 +-
 drivers/net/ethernet/mediatek/Makefile     |    1 +
 drivers/net/ethernet/mediatek/airoha_eth.c | 2705 ++++++++++++++++++++
 4 files changed, 2725 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mediatek/airoha_eth.c
MAINTAINERS +9 -0
index e0b6b419318f..2025634e838a 100644
@@ -682,6 +682,15 @@ S:	Supported
 F:	fs/aio.c
 F:	include/linux/*aio*.h
 
+AIROHA ETHERNET DRIVER
+M:	Lorenzo Bianconi <[email protected]>
+L:	[email protected] (moderated for non-subscribers)
+L:	[email protected] (moderated for non-subscribers)
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
+F:	drivers/net/ethernet/mediatek/airoha_eth.c
+
 AIROHA SPI SNFI DRIVER
 M:	Lorenzo Bianconi <[email protected]>
 M:	Ray Liu <[email protected]>
drivers/net/ethernet/mediatek/Kconfig +10 -1
index da0db417ab69..e16d0ea5d418 100644
@@ -1,12 +1,21 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config NET_VENDOR_MEDIATEK
 	bool "MediaTek devices"
-	depends on ARCH_MEDIATEK || SOC_MT7621 || SOC_MT7620 || COMPILE_TEST
+	depends on ARCH_MEDIATEK || ARCH_AIROHA || SOC_MT7621 || SOC_MT7620 || COMPILE_TEST
 	help
 	  If you have a Mediatek SoC with ethernet, say Y.
 
 if NET_VENDOR_MEDIATEK
 
+config NET_AIROHA
+	tristate "Airoha SoC Gigabit Ethernet support"
+	depends on NET_DSA || !NET_DSA
+	select PAGE_POOL
+	select PAGE_POOL_STATS
+	help
+	  This driver supports the gigabit ethernet MACs in the
+	  Airoha SoC family.
+
 config NET_MEDIATEK_SOC_WED
 	depends on ARCH_MEDIATEK || COMPILE_TEST
 	def_bool NET_MEDIATEK_SOC != n
drivers/net/ethernet/mediatek/Makefile +1 -0
index 03e008fbc859..ddbb7f4a516c 100644
@@ -11,3 +11,4 @@ mtk_eth-$(CONFIG_NET_MEDIATEK_SOC_WED) += mtk_wed_debugfs.o
 endif
 obj-$(CONFIG_NET_MEDIATEK_SOC_WED) += mtk_wed_ops.o
 obj-$(CONFIG_NET_MEDIATEK_STAR_EMAC) += mtk_star_emac.o
+obj-$(CONFIG_NET_AIROHA) += airoha_eth.o
drivers/net/ethernet/mediatek/airoha_eth.c new file +2705 -0

2,705 lines not on this page · raw message

Signature · 1 line
2.45.2
reviewer
#10 lore raw

Re: [PATCH v5 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

Quoted 14 lines · Lorenzo Bianconi

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

...

Quoted 25 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +25 -0
+static const char * const airoha_ethtool_stats_name[] = {
+	"tx_eth_pkt_cnt",
+	"tx_eth_byte_cnt",
+	"tx_ok_pkt_cnt",
+	"tx_ok_byte_cnt",
+	"tx_eth_drop_cnt",
+	"tx_eth_bc_cnt",
+	"tx_eth_mc_cnt",
+	"tx_eth_lt64_cnt",
+	"tx_eth_eq64_cnt",
+	"tx_eth_65_127_cnt",
+	"tx_eth_128_255_cnt",
+	"tx_eth_256_511_cnt",
+	"tx_eth_512_1023_cnt",
+	"tx_eth_1024_1518_cnt",
+	"tx_eth_gt1518_cnt",
+	"rx_eth_pkt_cnt",
+	"rx_eth_byte_cnt",
+	"rx_ok_pkt_cnt",
+	"rx_ok_byte_cnt",
+	"rx_eth_drop_cnt",
+	"rx_eth_bc_cnt",
+	"rx_eth_mc_cnt",
+	"rx_eth_crc_drop_cnt",
+	"rx_eth_frag_cnt",

drivers/net/ethernet/mediatek/airoha_eth.c line 2283

Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10 -0
+	"rx_eth_jabber_cnt",
+	"rx_eth_lt64_cnt",
+	"rx_eth_eq64_cnt",
+	"rx_eth_65_127_cnt",
+	"rx_eth_128_255_cnt",
+	"rx_eth_256_511_cnt",
+	"rx_eth_512_1023_cnt",
+	"rx_eth_1024_1518_cnt",
+	"rx_eth_gt1518_cnt",
+};

Hi Lorenzo,

Sorry for not noticing this earlier.
It seems to me that some of the stats above could
use standard stats, which is preferred.

Basically, my understanding is that one should:
1. Implement .ndo_get_stats64
(that seems relevant here)
2. As appropriate implement ethtool_stats non-extended stats operations (perhaps not relevant here)
3. Then implement get_ethtool_stats for what is left over

...

author maintainer
#11 lore raw

Re: [PATCH v5 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

Quoted 56 lines · Lorenzo Bianconi
Quoted 14 lines

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

...

Quoted 25 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +25 -0
+static const char * const airoha_ethtool_stats_name[] = {
+	"tx_eth_pkt_cnt",
+	"tx_eth_byte_cnt",
+	"tx_ok_pkt_cnt",
+	"tx_ok_byte_cnt",
+	"tx_eth_drop_cnt",
+	"tx_eth_bc_cnt",
+	"tx_eth_mc_cnt",
+	"tx_eth_lt64_cnt",
+	"tx_eth_eq64_cnt",
+	"tx_eth_65_127_cnt",
+	"tx_eth_128_255_cnt",
+	"tx_eth_256_511_cnt",
+	"tx_eth_512_1023_cnt",
+	"tx_eth_1024_1518_cnt",
+	"tx_eth_gt1518_cnt",
+	"rx_eth_pkt_cnt",
+	"rx_eth_byte_cnt",
+	"rx_ok_pkt_cnt",
+	"rx_ok_byte_cnt",
+	"rx_eth_drop_cnt",
+	"rx_eth_bc_cnt",
+	"rx_eth_mc_cnt",
+	"rx_eth_crc_drop_cnt",
+	"rx_eth_frag_cnt",

drivers/net/ethernet/mediatek/airoha_eth.c line 2283

Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10 -0
+	"rx_eth_jabber_cnt",
+	"rx_eth_lt64_cnt",
+	"rx_eth_eq64_cnt",
+	"rx_eth_65_127_cnt",
+	"rx_eth_128_255_cnt",
+	"rx_eth_256_511_cnt",
+	"rx_eth_512_1023_cnt",
+	"rx_eth_1024_1518_cnt",
+	"rx_eth_gt1518_cnt",
+};

Hi Lorenzo,

Quoted 1 line

Sorry for not noticing this earlier.

Hi Simon,

no worries :)

Quoted 2 lines

It seems to me that some of the stats above could
use standard stats, which is preferred.

Please correct me if I am wrong but it seems quite a common approach to have
same stats in both .ndo_get_stats64() and .get_ethtool_stats():
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L212
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/marvell/mvneta.c#L435
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/i40e/i40e_ethtool.c#L243
- https://github.com/torvalds/linux/blob/master/net/mac80211/ethtool.c#L52
- ...

Do you mean I should just report common stats (e.g. tx_packets or tx_bytes) in .ndo_get_stats64()? Or it is fine to just add .ndo_get_stats64() callback (not supported at the moment)?

Quoted 6 lines

Basically, my understanding is that one should:
1. Implement .ndo_get_stats64
(that seems relevant here)
2. As appropriate implement ethtool_stats non-extended stats operations (perhaps not relevant here)

Can you please provide me a pointer for it?

Regards,
Lorenzo

Quoted 3 lines

3. Then implement get_ethtool_stats for what is left over

...

reviewer
#12 lore raw

Re: [PATCH v5 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

Quoted 73 lines · Lorenzo Bianconi
Quoted 56 lines · Lorenzo Bianconi
Quoted 14 lines

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

...

Quoted 25 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +25 -0
+static const char * const airoha_ethtool_stats_name[] = {
+	"tx_eth_pkt_cnt",
+	"tx_eth_byte_cnt",
+	"tx_ok_pkt_cnt",
+	"tx_ok_byte_cnt",
+	"tx_eth_drop_cnt",
+	"tx_eth_bc_cnt",
+	"tx_eth_mc_cnt",
+	"tx_eth_lt64_cnt",
+	"tx_eth_eq64_cnt",
+	"tx_eth_65_127_cnt",
+	"tx_eth_128_255_cnt",
+	"tx_eth_256_511_cnt",
+	"tx_eth_512_1023_cnt",
+	"tx_eth_1024_1518_cnt",
+	"tx_eth_gt1518_cnt",
+	"rx_eth_pkt_cnt",
+	"rx_eth_byte_cnt",
+	"rx_ok_pkt_cnt",
+	"rx_ok_byte_cnt",
+	"rx_eth_drop_cnt",
+	"rx_eth_bc_cnt",
+	"rx_eth_mc_cnt",
+	"rx_eth_crc_drop_cnt",
+	"rx_eth_frag_cnt",

drivers/net/ethernet/mediatek/airoha_eth.c line 2283

Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10 -0
+	"rx_eth_jabber_cnt",
+	"rx_eth_lt64_cnt",
+	"rx_eth_eq64_cnt",
+	"rx_eth_65_127_cnt",
+	"rx_eth_128_255_cnt",
+	"rx_eth_256_511_cnt",
+	"rx_eth_512_1023_cnt",
+	"rx_eth_1024_1518_cnt",
+	"rx_eth_gt1518_cnt",
+};

Hi Lorenzo,

Quoted 1 line

Sorry for not noticing this earlier.

Hi Simon,

no worries :)

Quoted 2 lines

It seems to me that some of the stats above could
use standard stats, which is preferred.

Please correct me if I am wrong but it seems quite a common approach to have
same stats in both .ndo_get_stats64() and .get_ethtool_stats():
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L212
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/marvell/mvneta.c#L435
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/i40e/i40e_ethtool.c#L243
- https://github.com/torvalds/linux/blob/master/net/mac80211/ethtool.c#L52
- ...

Quoted 3 lines

Do you mean I should just report common stats (e.g. tx_packets or tx_bytes) in .ndo_get_stats64()? Or it is fine to just add .ndo_get_stats64() callback (not supported at the moment)?

The first option: It is my understanding that it preferred to only report common stats via ndo_get_stats64.

"Please limit stats you report in ethtool -S to just the stats for which
proper interfaces don't exist. Don't duplicate what's already reported via rtase_get_stats64(), also take a look at what can be reported via various *_stats members of struct ethtool_ops."

- Re: [PATCH net-next v18 10/13] rtase: Implement ethtool function by Jakub Kicinski
https://lore.kernel.org/netdev/[email protected]/

Quoted 7 lines
Quoted 5 lines

Basically, my understanding is that one should:
1. Implement .ndo_get_stats64
(that seems relevant here)
2. As appropriate implement ethtool_stats non-extended stats operations (perhaps not relevant here)

Can you please provide me a pointer for it?

Sorry, that was not at all clear.
I meant, as per the quote above, *_stats members of struct ethtool_ops, other than those that implement extended ops.
e.g. get_rmon_stats.

Quoted 7 lines

Regards,
Lorenzo

Quoted 3 lines

3. Then implement get_ethtool_stats for what is left over

...

author maintainer
#13 lore raw

Re: [PATCH v5 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

Quoted 81 lines · Lorenzo Bianconi
Quoted 73 lines
Quoted 56 lines · Lorenzo Bianconi
Quoted 14 lines

Add airoha_eth driver in order to introduce ethernet support for Airoha EN7581 SoC available on EN7581 development board (en7581-evb). en7581-evb networking architecture is composed by airoha_eth as mac controller (cpu port) and a mt7530 dsa based switch.
EN7581 mac controller is mainly composed by Frame Engine (FE) and QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic functionalities are supported now) while QDMA is used for DMA operation and QOS functionalities between mac layer and the dsa switch (hw QoS is not available yet and it will be added in the future). Currently only hw lan features are available, hw wan will be added with subsequent patches.

...

Quoted 25 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +25 -0
+static const char * const airoha_ethtool_stats_name[] = {
+	"tx_eth_pkt_cnt",
+	"tx_eth_byte_cnt",
+	"tx_ok_pkt_cnt",
+	"tx_ok_byte_cnt",
+	"tx_eth_drop_cnt",
+	"tx_eth_bc_cnt",
+	"tx_eth_mc_cnt",
+	"tx_eth_lt64_cnt",
+	"tx_eth_eq64_cnt",
+	"tx_eth_65_127_cnt",
+	"tx_eth_128_255_cnt",
+	"tx_eth_256_511_cnt",
+	"tx_eth_512_1023_cnt",
+	"tx_eth_1024_1518_cnt",
+	"tx_eth_gt1518_cnt",
+	"rx_eth_pkt_cnt",
+	"rx_eth_byte_cnt",
+	"rx_ok_pkt_cnt",
+	"rx_ok_byte_cnt",
+	"rx_eth_drop_cnt",
+	"rx_eth_bc_cnt",
+	"rx_eth_mc_cnt",
+	"rx_eth_crc_drop_cnt",
+	"rx_eth_frag_cnt",

drivers/net/ethernet/mediatek/airoha_eth.c line 2283

Quoted 10 lines
drivers/net/ethernet/mediatek/airoha_eth.c new file +10 -0
+	"rx_eth_jabber_cnt",
+	"rx_eth_lt64_cnt",
+	"rx_eth_eq64_cnt",
+	"rx_eth_65_127_cnt",
+	"rx_eth_128_255_cnt",
+	"rx_eth_256_511_cnt",
+	"rx_eth_512_1023_cnt",
+	"rx_eth_1024_1518_cnt",
+	"rx_eth_gt1518_cnt",
+};

Hi Lorenzo,

Quoted 1 line

Sorry for not noticing this earlier.

Hi Simon,

no worries :)

Quoted 2 lines

It seems to me that some of the stats above could
use standard stats, which is preferred.

Please correct me if I am wrong but it seems quite a common approach to have
same stats in both .ndo_get_stats64() and .get_ethtool_stats():
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/mediatek/mtk_eth_soc.c#L212
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/marvell/mvneta.c#L435
- https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/intel/i40e/i40e_ethtool.c#L243
- https://github.com/torvalds/linux/blob/master/net/mac80211/ethtool.c#L52
- ...

Quoted 3 lines

Do you mean I should just report common stats (e.g. tx_packets or tx_bytes) in .ndo_get_stats64()? Or it is fine to just add .ndo_get_stats64() callback (not supported at the moment)?

The first option: It is my understanding that it preferred to only report common stats via ndo_get_stats64.

Quoted 8 lines

"Please limit stats you report in ethtool -S to just the stats for which
proper interfaces don't exist. Don't duplicate what's already reported via rtase_get_stats64(), also take a look at what can be reported via various *_stats members of struct ethtool_ops."

- Re: [PATCH net-next v18 10/13] rtase: Implement ethtool function by Jakub Kicinski
https://lore.kernel.org/netdev/[email protected]/

ack, thx for the clarification. I will move common stats in ndo_get_stats64() in v6.

Regards,
Lorenzo

Quoted 23 lines
Quoted 8 lines
Quoted 6 lines

Basically, my understanding is that one should:
1. Implement .ndo_get_stats64
(that seems relevant here)
2. As appropriate implement ethtool_stats non-extended stats operations (perhaps not relevant here)

Can you please provide me a pointer for it?

Sorry, that was not at all clear.
I meant, as per the quote above, *_stats members of struct ethtool_ops, other than those that implement extended ops.
e.g. get_rmon_stats.

Quoted 7 lines

Regards,
Lorenzo

Quoted 3 lines

3. Then implement get_ethtool_stats for what is left over

...