diff mbox series

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

Message ID 48dde2595c6ff497a846183b117ac9704537b78c.1720079772.git.lorenzo@kernel.org (mailing list archive)
State New, archived
Headers show
Series Introduce EN7581 ethernet support | expand

Commit Message

Lorenzo Bianconi July 4, 2024, 8:08 a.m. UTC
Introduce device-tree binding documentation for Airoha EN7581 ethernet
mac controller.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml

Comments

Rob Herring (Arm) July 8, 2024, 4:37 p.m. UTC | #1
On Thu, Jul 04, 2024 at 10:08:10AM +0200, Lorenzo Bianconi wrote:
> Introduce device-tree binding documentation for Airoha EN7581 ethernet
> mac controller.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> new file mode 100644
> index 000000000000..f4b1f8afddd0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> @@ -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.

> +
> +maintainers:
> +  - Lorenzo Bianconi <lorenzo@kernel.org>
> +
> +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]$":

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

> +    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>;
> +        };
> +      };
> +    };
> -- 
> 2.45.2
>
Lorenzo Bianconi July 8, 2024, 5:03 p.m. UTC | #2
> On Thu, Jul 04, 2024 at 10:08:10AM +0200, Lorenzo Bianconi wrote:
> > Introduce device-tree binding documentation for Airoha EN7581 ethernet
> > mac controller.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
> >  1 file changed, 146 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > new file mode 100644
> > index 000000000000..f4b1f8afddd0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > @@ -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.

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";
		};
	};
};

> 
> > +
> > +maintainers:
> > +  - Lorenzo Bianconi <lorenzo@kernel.org>
> > +
> > +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]$":
> 
> '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

> 
> > +    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>;
> > +        };
> > +      };
> > +    };
> > -- 
> > 2.45.2
> >
Rob Herring (Arm) July 8, 2024, 8:19 p.m. UTC | #3
On Mon, Jul 8, 2024 at 11:03 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On Thu, Jul 04, 2024 at 10:08:10AM +0200, Lorenzo Bianconi wrote:
> > > Introduce device-tree binding documentation for Airoha EN7581 ethernet
> > > mac controller.
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > new file mode 100644
> > > index 000000000000..f4b1f8afddd0
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > @@ -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.
>
> 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.

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

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

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

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

>
> 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";
>                 };
>         };
> };

None of this is relevant.

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

> 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

> 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
Andrew Lunn July 8, 2024, 8:48 p.m. UTC | #4
> > 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.

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
Lorenzo Bianconi July 8, 2024, 9:10 p.m. UTC | #5
> On Mon, Jul 8, 2024 at 11:03 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > > On Thu, Jul 04, 2024 at 10:08:10AM +0200, Lorenzo Bianconi wrote:
> > > > Introduce device-tree binding documentation for Airoha EN7581 ethernet
> > > > mac controller.
> > > >
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
> > > >  1 file changed, 146 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > new file mode 100644
> > > > index 000000000000..f4b1f8afddd0
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > @@ -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.
> >
> > 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#

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

> 
> > gdm1 is the GMAC port used
> > as cpu port by the mt7530 dsa switch
> 
> That has nothing to do with *this* binding...
> 
> >
> > 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";
> >                 };
> >         };
> > };
> 
> None of this is relevant.
> 
> > > > +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.

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

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

> 
> > 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
> 
> > 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
Lorenzo Bianconi July 8, 2024, 9:16 p.m. UTC | #6
> > > 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.
> 
> Hi Lorenzo

Hi Andrew,

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

ack

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

> 
>     Andrew
Lorenzo Bianconi July 8, 2024, 10:02 p.m. UTC | #7
> > On Mon, Jul 8, 2024 at 11:03 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > >
> > > > On Thu, Jul 04, 2024 at 10:08:10AM +0200, Lorenzo Bianconi wrote:
> > > > > Introduce device-tree binding documentation for Airoha EN7581 ethernet
> > > > > mac controller.
> > > > >
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > >  .../bindings/net/airoha,en7581-eth.yaml       | 146 ++++++++++++++++++
> > > > >  1 file changed, 146 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..f4b1f8afddd0
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
> > > > > @@ -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.
> > >
> > > 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#

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

Regards
Lorenzo

> 
> > 
> > >
> > > 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.
> > 
> > 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?
> 
> > 
> > > gdm1 is the GMAC port used
> > > as cpu port by the mt7530 dsa switch
> > 
> > That has nothing to do with *this* binding...
> > 
> > >
> > > 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";
> > >                 };
> > >         };
> > > };
> > 
> > None of this is relevant.
> > 
> > > > > +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.
> 
> ack, fine. I will fix it in the next revision.
> 
> > 
> > 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
> 
> > 
> > > 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
> > 
> > > 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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
new file mode 100644
index 000000000000..f4b1f8afddd0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/airoha,en7581-eth.yaml
@@ -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 <lorenzo@kernel.org>
+
+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>;
+        };
+      };
+    };