| Message ID | 20221201003109.448693-1-tharvey@gateworks.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ARM: dts: imx6qdl-gw5904: add internal mdio nodes | expand |
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 >
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>
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
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
> > > > 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
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
> 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
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; + }; }; }; };
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(+)