diff mbox

ARM: dts: turris-omnia: add support for ethernet switch

Message ID 20161221102047.28036-1-uwe@kleine-koenig.org
State New
Headers show

Commit Message

Uwe Kleine-König Dec. 21, 2016, 10:20 a.m. UTC
The Turris Omnia features a Marvell MV88E7176 ethernet switch. Add it to
the dts.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

when the MAC is operated in rgmii mode and the switch in rgmii-id
communication between them works fine. The other way round it doesn't work.
The fixed-link nodes in the cpu port description is necessary to let the
mv88e6xxx driver use the phy-mode description. Is this a bug?

Regarding the binding, I think the label in the port nodes is strange.

	label = "lan2"

for an external port is fine. But

	label = "cpu"

for a port facing a CPU MAC looks wrong, still more as the Turris Omnia has
two ports connected to the SoC and so there are two ports with the same
label. I would suggest to use a separate property for that, e.g.

	cpu-port;

and no label.

Best regards
Uwe

 arch/arm/boot/dts/armada-385-turris-omnia.dts | 101 +++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Dec. 21, 2016, 3:18 p.m. UTC | #1
On Wed, Dec 21, 2016 at 11:20:47AM +0100, Uwe Kleine-König wrote:
> The Turris Omnia features a Marvell MV88E7176 ethernet switch. Add it to
> the dts.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
> 
> when the MAC is operated in rgmii mode and the switch in rgmii-id
> communication between them works fine. The other way round it doesn't work.
> The fixed-link nodes in the cpu port description is necessary to let the
> mv88e6xxx driver use the phy-mode description. Is this a bug?

It is somewhat deliberate. Normally, there is no phy at all for a cpu
port or a dsa port. If there is no phy, it makes no sense to set the
phy-mode. With fixed-link, there is a phy connected to the MAC, in
terms of how the network stack sees the devices. It is a somewhat
special phy, in fact its bandwidth is fixed, but it does support
phy-handle, and a few other phy properties and callbacks.

> Regarding the binding, I think the label in the port nodes is strange.
> 
> 	label = "lan2"
> 
> for an external port is fine. But
> 
> 	label = "cpu"
> 
> for a port facing a CPU MAC looks wrong, still more as the Turris Omnia has
> two ports connected to the SoC and so there are two ports with the same
> label.

This is somewhat historical. When DSA was designed, only one CPU port
was expected. There are in fact quite a few devices with two CPU
ports. I have some old proof of concept patches to support this, with
some basic load balancing. In the last few weeks, John Crispin has
been working on them, and has duel CPU ports working for the qca8k.
So i expect early next year we can make the Marvell chips support dual
CPU ports as well.

> I would suggest to use a separate property for that, e.g.
> 
> 	cpu-port;
> 
> and no label.

I would suggest keeping this idea for DSA v3, otherwise we end up with
messy code for little gain.

      Andrew
Andrew Lunn Dec. 21, 2016, 4:49 p.m. UTC | #2
>  	/* Switch MV88E7176 at address 0x10 */
> +	switch@10 {
> +		compatible = "marvell,mv88e6085";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		dsa,member = <0 0>;
> +
> +		reg = <0x10>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			ports@0 {
> +				reg = <0>;
> +				label = "lan0";
> +				phy-handle = <&lan0phy>;

snip

> +		mdio {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			lan0phy: ethernet-phy@0 {
> +				compatible = "ethernet-phy-ieee802.3-c22";
> +				reg = <0>;
> +			};

You probably don't need the phy-handle's and the whole mdio node. So
long as there is a simple mapping, port 0 uses the phy at address 0,
it should just work. You only need the mdio node when you want to
support PHY interrupts, or there is an odd mapping between port and
PHY, like the new switch chip added recently.

   Andrew
Uwe Kleine-König Dec. 21, 2016, 4:50 p.m. UTC | #3
Hello Andrew,

On Wed, Dec 21, 2016 at 04:18:14PM +0100, Andrew Lunn wrote:
> On Wed, Dec 21, 2016 at 11:20:47AM +0100, Uwe Kleine-König wrote:
> > when the MAC is operated in rgmii mode and the switch in rgmii-id
> > communication between them works fine. The other way round it doesn't work.
> > The fixed-link nodes in the cpu port description is necessary to let the
> > mv88e6xxx driver use the phy-mode description. Is this a bug?
> 
> It is somewhat deliberate. Normally, there is no phy at all for a cpu
> port or a dsa port. If there is no phy, it makes no sense to set the
> phy-mode. With fixed-link, there is a phy connected to the MAC, in
> terms of how the network stack sees the devices. It is a somewhat
> special phy, in fact its bandwidth is fixed, but it does support
> phy-handle, and a few other phy properties and callbacks.

Here the switch is the phy, and in this case it is necessary to
correctly configure it's physical properties. fixed-link is conceptual
wrong here because (IIUC) this is for MACs only. So (maybe also for DSA
v3 or a general phy cleanup) a standard binding to specify this which
doesn't formalise the switch (or general: phy) as MAC.

Best regards
Uwe
diff mbox

Patch

diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
index ab49acb2d452..6722d3243f9b 100644
--- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
+++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
@@ -122,7 +122,7 @@ 
 	pinctrl-names = "default";
 	pinctrl-0 = <&ge0_rgmii_pins>;
 	status = "okay";
-	phy-mode = "rgmii-id";
+	phy-mode = "rgmii";
 
 	fixed-link {
 		speed = <1000>;
@@ -135,7 +135,7 @@ 
 	pinctrl-names = "default";
 	pinctrl-0 = <&ge1_rgmii_pins>;
 	status = "okay";
-	phy-mode = "rgmii-id";
+	phy-mode = "rgmii";
 
 	fixed-link {
 		speed = <1000>;
@@ -274,6 +274,103 @@ 
 	};
 
 	/* Switch MV88E7176 at address 0x10 */
+	switch@10 {
+		compatible = "marvell,mv88e6085";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		dsa,member = <0 0>;
+
+		reg = <0x10>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			ports@0 {
+				reg = <0>;
+				label = "lan0";
+				phy-handle = <&lan0phy>;
+			};
+
+			ports@1 {
+				reg = <1>;
+				label = "lan1";
+				phy-handle = <&lan1phy>;
+			};
+
+			ports@2 {
+				reg = <2>;
+				label = "lan2";
+				phy-handle = <&lan2phy>;
+			};
+
+			ports@3 {
+				reg = <3>;
+				label = "lan3";
+				phy-handle = <&lan3phy>;
+			};
+
+			ports@4 {
+				reg = <4>;
+				label = "lan4";
+				phy-handle = <&lan4phy>;
+			};
+
+			ports@5 {
+				reg = <5>;
+				label = "cpu";
+				ethernet = <&eth1>;
+				phy-mode = "rgmii-id";
+
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+
+			ports@6 {
+				reg = <6>;
+				label = "cpu";
+				ethernet = <&eth0>;
+				phy-mode = "rgmii-id";
+
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+		};
+
+		mdio {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			lan0phy: ethernet-phy@0 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <0>;
+			};
+
+			lan1phy: ethernet-phy@1 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <1>;
+			};
+
+			lan2phy: ethernet-phy@2 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <2>;
+			};
+
+			lan3phy: ethernet-phy@3 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <3>;
+			};
+
+			lan4phy: ethernet-phy@4 {
+				compatible = "ethernet-phy-ieee802.3-c22";
+				reg = <4>;
+			};
+		};
+	};
 };
 
 &pinctrl {