[PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

Complete the switch definition by adding the internal mdio nodes.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/boot/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
index 612b6e068e28..08463e65dca3 100644
--- a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
@@ -212,6 +212,27 @@ switch@0 {
 			compatible = "marvell,mv88e6085";
 			reg = <0>;
 
+			mdio {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				sw_phy0: ethernet-phy@0 {
+					reg = <0x0>;
+				};
+
+				sw_phy1: ethernet-phy@1 {
+					reg = <0x1>;
+				};
+
+				sw_phy2: ethernet-phy@2 {
+					reg = <0x2>;
+				};
+
+				sw_phy3: ethernet-phy@3 {
+					reg = <0x3>;
+				};
+			};
+
 			ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -219,27 +240,41 @@ ports {
 				port@0 {
 					reg = <0>;
 					label = "lan4";
+					phy-handle = <&sw_phy0>;
+					phy-mode = "internal";
 				};
 
 				port@1 {
 					reg = <1>;
 					label = "lan3";
+					phy-handle = <&sw_phy1>;
+					phy-mode = "internal";
 				};
 
 				port@2 {
 					reg = <2>;
 					label = "lan2";
+					phy-handle = <&sw_phy2>;
+					phy-mode = "internal";
 				};
 
 				port@3 {
 					reg = <3>;
 					label = "lan1";
+					phy-handle = <&sw_phy3>;
+					phy-mode = "internal";
 				};
 
 				port@5 {
 					reg = <5>;
 					label = "cpu";
 					ethernet = <&fec>;
+					phy-mode = "rgmii-id";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+					};
 				};
 			};
 		};
-- 
2.25.1

[PATCH 2/2] ARM: dts: imx6qdl-gw5904: add dt props for populating eth MAC addrs

[Tim Harvey]

Add device-tree props to allow boot firmware to populate MAC addresses.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/boot/dts/imx6qdl-gw5904.dtsi | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
index 08463e65dca3..6596e8bd7e60 100644
--- a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
@@ -52,6 +52,11 @@
 / {
 	/* these are used by bootloader for disabling nodes */
 	aliases {
+		ethernet0 = &fec;
+		ethernet1 = &lan1;
+		ethernet2 = &lan2;
+		ethernet3 = &lan3;
+		ethernet4 = &lan4;
 		led0 = &led0;
 		led1 = &led1;
 		led2 = &led2;
@@ -237,32 +242,36 @@ ports {
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-				port@0 {
+				lan4: port@0 {
 					reg = <0>;
 					label = "lan4";
 					phy-handle = <&sw_phy0>;
 					phy-mode = "internal";
+					local-mac-address = [00 00 00 00 00 00];
 				};
 
-				port@1 {
+				lan3: port@1 {
 					reg = <1>;
 					label = "lan3";
 					phy-handle = <&sw_phy1>;
 					phy-mode = "internal";
+					local-mac-address = [00 00 00 00 00 00];
 				};
 
-				port@2 {
+				lan2: port@2 {
 					reg = <2>;
 					label = "lan2";
 					phy-handle = <&sw_phy2>;
 					phy-mode = "internal";
+					local-mac-address = [00 00 00 00 00 00];
 				};
 
-				port@3 {
+				lan1: port@3 {
 					reg = <3>;
 					label = "lan1";
 					phy-handle = <&sw_phy3>;
 					phy-mode = "internal";
+					local-mac-address = [00 00 00 00 00 00];
 				};
 
 				port@5 {
-- 
2.25.1

Re: [PATCH 2/2] ARM: dts: imx6qdl-gw5904: add dt props for populating eth MAC addrs

[Fabio Estevam]

Hi Tim,

On Wed, Nov 30, 2022 at 9:31 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Add device-tree props to allow boot firmware to populate MAC addresses.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Fabio Estevam]

Hi Tim,

[Adding Andrew]

On Wed, Nov 30, 2022 at 9:31 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> Complete the switch definition by adding the internal mdio nodes.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

> ---
>  arch/arm/boot/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> index 612b6e068e28..08463e65dca3 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> @@ -212,6 +212,27 @@ switch@0 {
>                         compatible = "marvell,mv88e6085";
>                         reg = <0>;
>
> +                       mdio {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               sw_phy0: ethernet-phy@0 {
> +                                       reg = <0x0>;
> +                               };
> +
> +                               sw_phy1: ethernet-phy@1 {
> +                                       reg = <0x1>;
> +                               };
> +
> +                               sw_phy2: ethernet-phy@2 {
> +                                       reg = <0x2>;
> +                               };
> +
> +                               sw_phy3: ethernet-phy@3 {
> +                                       reg = <0x3>;
> +                               };
> +                       };
> +
>                         ports {
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
> @@ -219,27 +240,41 @@ ports {
>                                 port@0 {
>                                         reg = <0>;
>                                         label = "lan4";
> +                                       phy-handle = <&sw_phy0>;
> +                                       phy-mode = "internal";
>                                 };
>
>                                 port@1 {
>                                         reg = <1>;
>                                         label = "lan3";
> +                                       phy-handle = <&sw_phy1>;
> +                                       phy-mode = "internal";
>                                 };
>
>                                 port@2 {
>                                         reg = <2>;
>                                         label = "lan2";
> +                                       phy-handle = <&sw_phy2>;
> +                                       phy-mode = "internal";
>                                 };
>
>                                 port@3 {
>                                         reg = <3>;
>                                         label = "lan1";
> +                                       phy-handle = <&sw_phy3>;
> +                                       phy-mode = "internal";
>                                 };
>
>                                 port@5 {
>                                         reg = <5>;
>                                         label = "cpu";
>                                         ethernet = <&fec>;
> +                                       phy-mode = "rgmii-id";
> +
> +                                       fixed-link {
> +                                               speed = <1000>;
> +                                               full-duplex;
> +                                       };
>                                 };
>                         };
>                 };
> --
> 2.25.1
>

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Andrew Lunn]

On Thu, Dec 01, 2022 at 10:02:08PM -0300, Fabio Estevam wrote:
> Hi Tim,
> 
> [Adding Andrew]

It is not wrong, but it should also mostly not be needed. The switch
driver can link internal PHYs to ports.

> >                                 port@5 {
> >                                         reg = <5>;
> >                                         label = "cpu";
> >                                         ethernet = <&fec>;
> > +                                       phy-mode = "rgmii-id";
> > +
> > +                                       fixed-link {
> > +                                               speed = <1000>;
> > +                                               full-duplex;
> > +                                       };
> >                                 };

This part is needed to make a warning go away. Does the SoC network interface 
have phy-mode = "rgmii"; ?

     Andrew

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

On Fri, Dec 2, 2022 at 5:08 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Dec 01, 2022 at 10:02:08PM -0300, Fabio Estevam wrote:
> > Hi Tim,
> >
> > [Adding Andrew]
>
> It is not wrong, but it should also mostly not be needed. The switch
> driver can link internal PHYs to ports.

Andrew,

I should have mentioned in the commit log that this does not change
behavior on Linux but is required for boot firmware. Specifically
U-Boot requires the internal PHY ports to be defined for its DSA
architecture and they share dt's as much as possible.

>
> > >                                 port@5 {
> > >                                         reg = <5>;
> > >                                         label = "cpu";
> > >                                         ethernet = <&fec>;
> > > +                                       phy-mode = "rgmii-id";
> > > +
> > > +                                       fixed-link {
> > > +                                               speed = <1000>;
> > > +                                               full-duplex;
> > > +                                       };
> > >                                 };
>
> This part is needed to make a warning go away. Does the SoC network interface
> have phy-mode = "rgmii"; ?

No, it looks like this:

&fec {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_enet>;
        phy-mode = "rgmii-id";
        status = "okay";

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

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

                switch@0 {
                        compatible = "marvell,mv88e6085";
                        reg = <0>;
...

Is something here wrong?

Best Regards,

Tim

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Andrew Lunn]

> > > >                                 port@5 {
> > > >                                         reg = <5>;
> > > >                                         label = "cpu";
> > > >                                         ethernet = <&fec>;
> > > > +                                       phy-mode = "rgmii-id";
> > > > +
> > > > +                                       fixed-link {
> > > > +                                               speed = <1000>;
> > > > +                                               full-duplex;
> > > > +                                       };
> > > >                                 };
> >
> > This part is needed to make a warning go away. Does the SoC network interface
> > have phy-mode = "rgmii"; ?
> 
> No, it looks like this:
> 
> &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
>         phy-mode = "rgmii-id";
 
> Is something here wrong?

It suggests both ends should insert RGMII delays. So you will end up
with double delays. Have one end say plain "rgmii" and the other
"rgmii-id". I would probably go with the FEC being "rgmii".

    Andrew

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

On Fri, Dec 2, 2022 at 9:31 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > >                                 port@5 {
> > > > >                                         reg = <5>;
> > > > >                                         label = "cpu";
> > > > >                                         ethernet = <&fec>;
> > > > > +                                       phy-mode = "rgmii-id";
> > > > > +
> > > > > +                                       fixed-link {
> > > > > +                                               speed = <1000>;
> > > > > +                                               full-duplex;
> > > > > +                                       };
> > > > >                                 };
> > >
> > > This part is needed to make a warning go away. Does the SoC network interface
> > > have phy-mode = "rgmii"; ?
> >
> > No, it looks like this:
> >
> > &fec {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_enet>;
> >         phy-mode = "rgmii-id";
>
> > Is something here wrong?
>
> It suggests both ends should insert RGMII delays. So you will end up
> with double delays. Have one end say plain "rgmii" and the other
> "rgmii-id". I would probably go with the FEC being "rgmii".
>
>     Andrew

Andrew,

That makes sense - I will change the fec node to rgmii.

Upon further testing I find there is something else wrong with this
patch however that I don't yet understand.

Without it the switch works fine (due to RGMII delay being configured
via boot firmware) but I do get the warning you had mentioned due to
the phy-mode/phy-handle props missing:
mv88e6085 2188000.ethernet-1:00: switch 0x1760 detected: Marvell
88E6176, revision 1
mv88e6085 2188000.ethernet-1:00: OF node
/soc/bus@2100000/ethernet@2188000/mdio/switch@0/ports/port@5 of CPU
port 5 lacks the required "phy-mode" property
mv88e6085 2188000.ethernet-1:00: OF node
/soc/bus@2100000/ethernet@2188000/mdio/switch@0/ports/port@5 of CPU
port 5 lacks the required "phy-handle", "fixed-link" or "managed"
properties
mv88e6085 2188000.ethernet-1:00: Skipping phylink registration for CPU port 5
mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): PHY
[mv88e6xxx-1:00] driver [Generic PHY] (irq=POLL)
mv88e6085 2188000.ethernet-1:00 lan3 (uninitialized): PHY
[mv88e6xxx-1:01] driver [Generic PHY] (irq=POLL)
mv88e6085 2188000.ethernet-1:00 lan2 (uninitialized): PHY
[mv88e6xxx-1:02] driver [Generic PHY] (irq=POLL)
mv88e6085 2188000.ethernet-1:00 lan1 (uninitialized): PHY
[mv88e6xxx-1:03] driver [Generic PHY] (irq=POLL)

When I add the phy-mode/phy-handle props with this patch I get the
following failure:
mv88e6085 2188000.ethernet-1:00: switch 0x1760 detected: Marvell
88E6176, revision 1
mv88e6085 2188000.ethernet-1:00: switch 0x1760 detected: Marvell
88E6176, revision 1
mv88e6085 2188000.ethernet-1:00: configuring for fixed/rgmii-id link mode
mv88e6085 2188000.ethernet-1:00: p5: delay RXCLK yes, TXCLK yes
mv88e6085 2188000.ethernet-1:00: p5: delay RXCLK yes, TXCLK yes
mv88e6085 2188000.ethernet-1:00: Link is Up - 1Gbps/Full - flow control off
mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): validation of
internal with support 00000000,00000000,000062ff and advertisement
00000000,00000000,000062ff failed: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): failed to
connect to PHY: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 0
mv88e6085 2188000.ethernet-1:00 lan3 (uninitialized): validation of
internal with support 00000000,00000000,000062ff and advertisement
00000000,00000000,000062ff failed: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan3 (uninitialized): failed to
connect to PHY: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan3 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 1
mv88e6085 2188000.ethernet-1:00 lan2 (uninitialized): validation of
internal with support 00000000,00000000,000062ff and advertisement
00000000,00000000,000062ff failed: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan2 (uninitialized): failed to
connect to PHY: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan2 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 2
mv88e6085 2188000.ethernet-1:00 lan1 (uninitialized): validation of
internal with support 00000000,00000000,000062ff and advertisement
00000000,00000000,000062ff failed: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan1 (uninitialized): failed to
connect to PHY: -EINVAL
mv88e6085 2188000.ethernet-1:00 lan1 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 3

I've run into this message before and had a hard time understanding
the issue from the message - it seems to indicate the phy status
matches advertisement but that its an invalid mode?

Thanks,

Tim

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Andrew Lunn]

> mv88e6085 2188000.ethernet-1:00: Skipping phylink registration for CPU port 5
> mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): PHY
> [mv88e6xxx-1:00] driver [Generic PHY] (irq=POLL)
> mv88e6085 2188000.ethernet-1:00 lan3 (uninitialized): PHY
> [mv88e6xxx-1:01] driver [Generic PHY] (irq=POLL)
> mv88e6085 2188000.ethernet-1:00 lan2 (uninitialized): PHY
> [mv88e6xxx-1:02] driver [Generic PHY] (irq=POLL)
> mv88e6085 2188000.ethernet-1:00 lan1 (uninitialized): PHY
> [mv88e6xxx-1:03] driver [Generic PHY] (irq=POLL)

Hi Tim

You are using the generic PHY driver, not the Marvell driver.
I suggest you fix that first. See if that causes the problem to go
away.

	Andrew

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Vladimir Oltean]

Hi Tim,

On Fri, Dec 02, 2022 at 02:29:20PM -0800, Tim Harvey wrote:
> When I add the phy-mode/phy-handle props with this patch I get the
> following failure:
> mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): validation of internal with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL
> mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): failed to connect to PHY: -EINVAL
> 
> I've run into this message before and had a hard time understanding
> the issue from the message - it seems to indicate the phy status
> matches advertisement but that its an invalid mode?

Does this patch help?

From 6bd79d9b9994fcb7762301fe297f963c272d9210 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 5 Dec 2022 19:05:24 +0200
Subject: [PATCH] net: dsa: mv88e6xxx: accept phy-mode = "internal" for
 internal PHY ports

The ethernet-controller dt-schema, mostly evolved by Linux, has the
"internal" PHY mode for connections between a MAC and internal PHY.

U-Boot may provide device tree blobs where this phy-mode is specified,
so make the Linux driver accept them.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index ccfa4751d3b7..ba4fff8690aa 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -833,10 +833,13 @@ static void mv88e6xxx_get_caps(struct dsa_switch *ds, int port,
 
 	chip->info->ops->phylink_get_caps(chip, port, config);
 
-	/* Internal ports need GMII for PHYLIB */
-	if (mv88e6xxx_phy_is_internal(ds, port))
+	if (mv88e6xxx_phy_is_internal(ds, port)) {
+		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
+			  config->supported_interfaces);
+		/* Internal ports with no phy-mode need GMII for PHYLIB */
 		__set_bit(PHY_INTERFACE_MODE_GMII,
 			  config->supported_interfaces);
+	}
 }
 
 static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
-- 
2.34.1

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

On Mon, Dec 5, 2022 at 9:10 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Hi Tim,
>
> On Fri, Dec 02, 2022 at 02:29:20PM -0800, Tim Harvey wrote:
> > When I add the phy-mode/phy-handle props with this patch I get the
> > following failure:
> > mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): validation of internal with support 00000000,00000000,000062ff and advertisement 00000000,00000000,000062ff failed: -EINVAL
> > mv88e6085 2188000.ethernet-1:00 lan4 (uninitialized): failed to connect to PHY: -EINVAL
> >
> > I've run into this message before and had a hard time understanding
> > the issue from the message - it seems to indicate the phy status
> > matches advertisement but that its an invalid mode?
>
> Does this patch help?
>
> From 6bd79d9b9994fcb7762301fe297f963c272d9210 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Mon, 5 Dec 2022 19:05:24 +0200
> Subject: [PATCH] net: dsa: mv88e6xxx: accept phy-mode = "internal" for
>  internal PHY ports
>
> The ethernet-controller dt-schema, mostly evolved by Linux, has the
> "internal" PHY mode for connections between a MAC and internal PHY.
>
> U-Boot may provide device tree blobs where this phy-mode is specified,
> so make the Linux driver accept them.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index ccfa4751d3b7..ba4fff8690aa 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -833,10 +833,13 @@ static void mv88e6xxx_get_caps(struct dsa_switch *ds, int port,
>
>         chip->info->ops->phylink_get_caps(chip, port, config);
>
> -       /* Internal ports need GMII for PHYLIB */
> -       if (mv88e6xxx_phy_is_internal(ds, port))
> +       if (mv88e6xxx_phy_is_internal(ds, port)) {
> +               __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +                         config->supported_interfaces);
> +               /* Internal ports with no phy-mode need GMII for PHYLIB */
>                 __set_bit(PHY_INTERFACE_MODE_GMII,
>                           config->supported_interfaces);
> +       }
>  }
>
>  static void mv88e6xxx_mac_config(struct dsa_switch *ds, int port,
> --
> 2.34.1
>

Vladimir,

Yes, this patch resolves the issue. Enabling CONFIG_MARVELL_PHY
without this patch still shows the issue.

Thanks - please cc me on that and I'll respond (unless you want me to send it).

I'll submit a v2 of my dt patch with fec phy-mode = 'rgmii' after this
patch lands.

Best Regards,

Tim

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Vladimir Oltean]

On Mon, Dec 05, 2022 at 10:01:35AM -0800, Tim Harvey wrote:
> Vladimir,
> 
> Yes, this patch resolves the issue. Enabling CONFIG_MARVELL_PHY
> without this patch still shows the issue.

Yeah, that makes sense considering what the issue is.

> Thanks - please cc me on that and I'll respond (unless you want me to send it).
> 
> I'll submit a v2 of my dt patch with fec phy-mode = 'rgmii' after this
> patch lands.

The question is how far to backport this change.

I see that the GW5904 board was introduced in 2017. Which kernels do you
need to support the complete/unambiguous dt-binding?

I'm tempted to submit the patch to the "net" tree (making it a candidate
for stable releases) but without a Fixes: tag, making it go as far as it
will. That would be down to kernel 5.18, where the mv88e6xxx_get_caps()
function was introduced. Further down, and we'd need different patches
for older stable trees.

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

On Mon, Dec 5, 2022 at 11:02 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Mon, Dec 05, 2022 at 10:01:35AM -0800, Tim Harvey wrote:
> > Vladimir,
> >
> > Yes, this patch resolves the issue. Enabling CONFIG_MARVELL_PHY
> > without this patch still shows the issue.
>
> Yeah, that makes sense considering what the issue is.
>
> > Thanks - please cc me on that and I'll respond (unless you want me to send it).
> >
> > I'll submit a v2 of my dt patch with fec phy-mode = 'rgmii' after this
> > patch lands.
>
> The question is how far to backport this change.
>
> I see that the GW5904 board was introduced in 2017. Which kernels do you
> need to support the complete/unambiguous dt-binding?

before adding the internal PHY nodes it works fine so no backporting
needed. I just don't want to add the complete/unambiguous dt-bindings
until your patch lands as at that point it would otherwise be broken.

Best Regards,

Tim

>
> I'm tempted to submit the patch to the "net" tree (making it a candidate
> for stable releases) but without a Fixes: tag, making it go as far as it
> will. That would be down to kernel 5.18, where the mv88e6xxx_get_caps()
> function was introduced. Further down, and we'd need different patches
> for older stable trees.

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Vladimir Oltean]

On Mon, Dec 05, 2022 at 11:10:45AM -0800, Tim Harvey wrote:
> before adding the internal PHY nodes it works fine so no backporting
> needed. I just don't want to add the complete/unambiguous dt-bindings
> until your patch lands as at that point it would otherwise be broken.

Right, but there is some debate on whether device tree updates should
require new kernel or not. Since in this case, kernels 5.18 and newer
can trivially support the updated device trees with this change, I think
it would be silly not to let it be backported. As for older kernels, it
really depends on what the board ships with.

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Tim Harvey]

On Mon, Dec 5, 2022 at 11:15 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Mon, Dec 05, 2022 at 11:10:45AM -0800, Tim Harvey wrote:
> > before adding the internal PHY nodes it works fine so no backporting
> > needed. I just don't want to add the complete/unambiguous dt-bindings
> > until your patch lands as at that point it would otherwise be broken.
>
> Right, but there is some debate on whether device tree updates should
> require new kernel or not. Since in this case, kernels 5.18 and newer
> can trivially support the updated device trees with this change, I think
> it would be silly not to let it be backported. As for older kernels, it
> really depends on what the board ships with.

I see. So then your patch should be backported to when the phy
validation was added which would be d4ebf12bcec4 ("net: dsa:
mv88e6xxx: populate supported_interfaces and mac_capabilities") then
right?

Best Regards,

Tim

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Vladimir Oltean]

On Mon, Dec 05, 2022 at 11:24:39AM -0800, Tim Harvey wrote:
> I see. So then your patch should be backported to when the phy
> validation was added which would be d4ebf12bcec4 ("net: dsa:
> mv88e6xxx: populate supported_interfaces and mac_capabilities") then
> right?

Yup. Actually I suspect that before the commit you're indicating, the
mv88e6xxx driver was actually accepting the "internal" phy-mode.

Re: [PATCH 1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes

[Vladimir Oltean]

On Wed, Nov 30, 2022 at 04:31:08PM -0800, Tim Harvey wrote:
> Complete the switch definition by adding the internal mdio nodes.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  arch/arm/boot/dts/imx6qdl-gw5904.dtsi | 35 +++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> index 612b6e068e28..08463e65dca3 100644
> --- a/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-gw5904.dtsi
> @@ -212,6 +212,27 @@ switch@0 {
>  			compatible = "marvell,mv88e6085";
>  			reg = <0>;
>  
> +			mdio {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				sw_phy0: ethernet-phy@0 {
> +					reg = <0x0>;
> +				};
> +
> +				sw_phy1: ethernet-phy@1 {
> +					reg = <0x1>;
> +				};
> +
> +				sw_phy2: ethernet-phy@2 {
> +					reg = <0x2>;
> +				};
> +
> +				sw_phy3: ethernet-phy@3 {
> +					reg = <0x3>;
> +				};
> +			};
> +
>  			ports {
>  				#address-cells = <1>;
>  				#size-cells = <0>;
> @@ -219,27 +240,41 @@ ports {
>  				port@0 {
>  					reg = <0>;
>  					label = "lan4";
> +					phy-handle = <&sw_phy0>;
> +					phy-mode = "internal";
>  				};
>  
>  				port@1 {
>  					reg = <1>;
>  					label = "lan3";
> +					phy-handle = <&sw_phy1>;
> +					phy-mode = "internal";
>  				};
>  
>  				port@2 {
>  					reg = <2>;
>  					label = "lan2";
> +					phy-handle = <&sw_phy2>;
> +					phy-mode = "internal";
>  				};
>  
>  				port@3 {
>  					reg = <3>;
>  					label = "lan1";
> +					phy-handle = <&sw_phy3>;
> +					phy-mode = "internal";
>  				};
>  
>  				port@5 {
>  					reg = <5>;
>  					label = "cpu";

The patch context here might conflict with Arınç Ünal's effort to remove
label = "cpu" from everywhere.
https://patchwork.kernel.org/project/netdevbpf/cover/20221130141040.32447-1-arinc.unal@arinc9.com/

>  					ethernet = <&fec>;
> +					phy-mode = "rgmii-id";
> +
> +					fixed-link {
> +						speed = <1000>;
> +						full-duplex;
> +					};
>  				};
>  			};
>  		};
> -- 
> 2.25.1
>