[PATCHv4 0/5] GEHC Bx50 Switch Support

[PATCHv4 0/5] GEHC Bx50 Switch Support

[Sebastian Reichel]

Hi,

This adds support for the internal switch found in GE Healthcare
B450v3, B650v3 and B850v3. All devices use a GPIO bitbanged MDIO
bus to communicate with the switch and a PCIe based network card
for exchanging network data. The cpu network data link requires,
that the switch's internal phy interface is enabled, so support
for that is added by the first patch in this series.

The patch series is based on v4.15-rc8.

Changes since PATCHv3:
 * Enable the phy in dsa_port_setup() instead of abusing the
   fixed link setup function
Changes since PATCHv2:
 * Add phy nodes to switch in bx50.dtsi and reference them
   from switch ports
 * Enable cpu-port's phy based on 'phy-handle' instead of 'phy-mode'
Changes since PATCHv1:
 * Use 'marvell,mv88e6085' instead of introducing compatible
   string for mv88e6240.
 * Fix indention of DT nodes
 * Only enable 'cpu' phy, if explicitly set to "internal".

-- Sebastian

Sebastian Reichel (5):
  net: dsa: Support internal phy on 'cpu' port
  ARM: dts: imx6q-bx50v3: Add internal switch
  ARM: dts: imx6q-b850v3: Add switch port configuration
  ARM: dts: imx6q-b650v3: Add switch port configuration
  ARM: dts: imx6q-b450v3: Add switch port configuration

 arch/arm/boot/dts/imx6q-b450v3.dts  | 52 +++++++++++++++++++++++++
 arch/arm/boot/dts/imx6q-b650v3.dts  | 52 +++++++++++++++++++++++++
 arch/arm/boot/dts/imx6q-b850v3.dts  | 75 +++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx6q-bx50v3.dtsi | 62 ++++++++++++++++++++++++++++++
 net/dsa/dsa2.c                      | 25 ++++++++++---
 net/dsa/dsa_priv.h                  |  1 +
 net/dsa/port.c                      | 49 ++++++++++++++++++++++++
 7 files changed, 310 insertions(+), 6 deletions(-)

-- 
2.15.1

[PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[Sebastian Reichel]

This adds support for enabling the internal PHY for a 'cpu' port.
It has been tested on GE B850v3,  B650v3 and B450v3, which have a
built-in MV88E6240 switch hardwired to a PCIe based network card
making use of the internal PHY. Since mv88e6xxx driver resets the
chip during probe, the PHY is disabled without this patch resulting
in missing link and non-functional switch device.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 net/dsa/dsa2.c     | 25 +++++++++++++++++++------
 net/dsa/dsa_priv.h |  1 +
 net/dsa/port.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 1e287420ff49..f65938d10b6d 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -18,6 +18,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 
 #include "dsa_priv.h"
 
@@ -271,11 +272,20 @@ static int dsa_port_setup(struct dsa_port *dp)
 		break;
 	case DSA_PORT_TYPE_CPU:
 	case DSA_PORT_TYPE_DSA:
-		err = dsa_port_fixed_link_register_of(dp);
-		if (err) {
-			dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
-				ds->index, dp->index);
-			return err;
+		if (of_phy_is_fixed_link(dp->dn)) {
+			err = dsa_port_fixed_link_register_of(dp);
+			if (err) {
+				dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
+					ds->index, dp->index);
+				return err;
+			}
+		} else {
+			err = dsa_port_setup_phy_of(dp, true);
+			if (err) {
+				dev_err(ds->dev, "failed to enable phy for port %d.%d\n",
+					ds->index, dp->index);
+				return err;
+			}
 		}
 
 		break;
@@ -301,7 +311,10 @@ static void dsa_port_teardown(struct dsa_port *dp)
 		break;
 	case DSA_PORT_TYPE_CPU:
 	case DSA_PORT_TYPE_DSA:
-		dsa_port_fixed_link_unregister_of(dp);
+		if (of_phy_is_fixed_link(dp->dn))
+			dsa_port_fixed_link_unregister_of(dp);
+		else
+			dsa_port_setup_phy_of(dp, false);
 		break;
 	case DSA_PORT_TYPE_USER:
 		if (dp->slave) {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 7d036696e8c4..6c14079e6cc8 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -159,6 +159,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
 int dsa_port_fixed_link_register_of(struct dsa_port *dp);
 void dsa_port_fixed_link_unregister_of(struct dsa_port *dp);
+int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable);
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index bb4be2679904..a1518024bba3 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -273,6 +273,55 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	return 0;
 }
 
+int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
+{
+	struct device_node *port_dn = dp->dn;
+	struct device_node *phy_dn;
+	struct dsa_switch *ds = dp->ds;
+	struct phy_device *phydev;
+	int port = dp->index;
+	int err = 0;
+
+	phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
+	if (!phy_dn)
+		return 0;
+
+	phydev = of_phy_find_device(phy_dn);
+	if (!phydev) {
+		err = -EPROBE_DEFER;
+		goto err_put_of;
+	}
+
+	if (enable) {
+		err = genphy_config_init(phydev);
+		if (err < 0)
+			goto err_put_dev;
+
+		err = genphy_resume(phydev);
+		if (err < 0)
+			goto err_put_dev;
+
+		err = genphy_read_status(phydev);
+		if (err < 0)
+			goto err_put_dev;
+	} else {
+		err = genphy_suspend(phydev);
+		if (err < 0)
+			goto err_put_dev;
+	}
+
+	if (ds->ops->adjust_link)
+		ds->ops->adjust_link(ds, port, phydev);
+
+	dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev));
+
+err_put_dev:
+	put_device(&phydev->mdio.dev);
+err_put_of:
+	of_node_put(phy_dn);
+	return err;
+}
+
 int dsa_port_fixed_link_register_of(struct dsa_port *dp)
 {
 	struct device_node *dn = dp->dn;
-- 
2.15.1

[PATCHv4 2/5] ARM: dts: imx6q-bx50v3: Add internal switch

[Sebastian Reichel]

B850v3, B650v3 and B450v3 all have a GPIO bit banged MDIO bus to
communicate with a Marvell switch. On all devices the switch is
connected to a PCI based network card, which needs to be referenced
by DT, so this also adds the common PCI root node.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/imx6q-bx50v3.dtsi | 62 +++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-bx50v3.dtsi b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
index 0808dffc9a48..09b13ac967ec 100644
--- a/arch/arm/boot/dts/imx6q-bx50v3.dtsi
+++ b/arch/arm/boot/dts/imx6q-bx50v3.dtsi
@@ -99,6 +99,56 @@
 		mux-int-port = <1>;
 		mux-ext-port = <4>;
 	};
+
+	aliases {
+		mdio-gpio0 = &mdio0;
+	};
+
+	mdio0: mdio-gpio {
+		compatible = "virtual,mdio-gpio";
+		gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>, /* mdc */
+			<&gpio2 7 GPIO_ACTIVE_HIGH>; /* mdio */
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		switch@0 {
+			compatible = "marvell,mv88e6085"; /* 88e6240*/
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+
+			switch_ports: ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			mdio {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				switchphy0: switchphy@0 {
+					reg = <0>;
+				};
+
+				switchphy1: switchphy@1 {
+					reg = <1>;
+				};
+
+				switchphy2: switchphy@2 {
+					reg = <2>;
+				};
+
+				switchphy3: switchphy@3 {
+					reg = <3>;
+				};
+
+				switchphy4: switchphy@4 {
+					reg = <4>;
+				};
+			};
+		};
+	};
 };
 
 &ecspi5 {
@@ -337,3 +387,15 @@
 		tcxo-clock-frequency = <26000000>;
 	};
 };
+
+&pcie {
+	/* Synopsys, Inc. Device */
+	pci_root: root@0,0 {
+		compatible = "pci16c3,abcd";
+		reg = <0x00000000 0 0 0 0>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+	};
+};
-- 
2.15.1

[PATCHv4 3/5] ARM: dts: imx6q-b850v3: Add switch port configuration

[Sebastian Reichel]

This adds support for the Marvell switch and names the network
ports according to the labels, that can be found next to the
connectors ("ID", "IX", "ePort 1", "ePort 2"). The switch is
connected to the host system using a PCI based network card.

The PCI bus configuration has been written using the following
information:

root@b850v3# lspci -tv
-[0000:00]---00.0-[01]----00.0-[02-05]--+-01.0-[03]----00.0  Intel Corporation I210 Gigabit Network Connection
                                        +-02.0-[04]----00.0  Intel Corporation I210 Gigabit Network Connection
                                        \-03.0-[05]--
root@b850v3# lspci -nn
00:00.0 PCI bridge [0604]: Synopsys, Inc. Device [16c3:abcd] (rev 01)
01:00.0 PCI bridge [0604]: PLX Technology, Inc. PEX 8605 PCI Express 4-port Gen2 Switch [10b5:8605] (rev ab)
02:01.0 PCI bridge [0604]: PLX Technology, Inc. PEX 8605 PCI Express 4-port Gen2 Switch [10b5:8605] (rev ab)
02:02.0 PCI bridge [0604]: PLX Technology, Inc. PEX 8605 PCI Express 4-port Gen2 Switch [10b5:8605] (rev ab)
02:03.0 PCI bridge [0604]: PLX Technology, Inc. PEX 8605 PCI Express 4-port Gen2 Switch [10b5:8605] (rev ab)
03:00.0 Ethernet controller [0200]: Intel Corporation I210 Gigabit Network Connection [8086:1533] (rev 03)
04:00.0 Ethernet controller [0200]: Intel Corporation I210 Gigabit Network Connection [8086:1533] (rev 03)

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/imx6q-b850v3.dts | 75 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-b850v3.dts b/arch/arm/boot/dts/imx6q-b850v3.dts
index 46bdc6722715..35edbdc7bcd1 100644
--- a/arch/arm/boot/dts/imx6q-b850v3.dts
+++ b/arch/arm/boot/dts/imx6q-b850v3.dts
@@ -212,3 +212,78 @@
 		};
 	};
 };
+
+&pci_root {
+	/* PLX Technology, Inc. PEX 8605 PCI Express 4-port Gen2 Switch */
+	bridge@1,0 {
+		compatible = "pci10b5,8605";
+		reg = <0x00010000 0 0 0 0>;
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+
+		bridge@2,1 {
+			compatible = "pci10b5,8605";
+			reg = <0x00020800 0 0 0 0>;
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+
+			/* Intel Corporation I210 Gigabit Network Connection */
+			ethernet@3,0 {
+				compatible = "pci8086,1533";
+				reg = <0x00030000 0 0 0 0>;
+			};
+		};
+
+		bridge@2,2 {
+			compatible = "pci10b5,8605";
+			reg = <0x00021000 0 0 0 0>;
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+
+			/* Intel Corporation I210 Gigabit Network Connection */
+			switch_nic: ethernet@4,0 {
+				compatible = "pci8086,1533";
+				reg = <0x00040000 0 0 0 0>;
+			};
+		};
+	};
+};
+
+&switch_ports {
+	port@0 {
+		reg = <0>;
+		label = "eneport1";
+		phy-handle = <&switchphy0>;
+	};
+
+	port@1 {
+		reg = <1>;
+		label = "eneport2";
+		phy-handle = <&switchphy1>;
+	};
+
+	port@2 {
+		reg = <2>;
+		label = "enix";
+		phy-handle = <&switchphy2>;
+	};
+
+	port@3 {
+		reg = <3>;
+		label = "enid";
+		phy-handle = <&switchphy3>;
+	};
+
+	port@4 {
+		reg = <4>;
+		label = "cpu";
+		ethernet = <&switch_nic>;
+		phy-handle = <&switchphy4>;
+	};
+};
-- 
2.15.1

[PATCHv4 4/5] ARM: dts: imx6q-b650v3: Add switch port configuration

[Sebastian Reichel]

This adds support for the Marvell switch and names the network
ports according to the labels, that can be found next to the
connectors. The switch is connected to the host system using a
PCI based network card.

The PCI bus configuration has been written using the following
information:

root@b650v3# lspci -tv
-[0000:00]---00.0-[01]----00.0  Intel Corporation I210 Gigabit Network Connection
root@b650v3# lspci -nn
00:00.0 PCI bridge [0604]: Synopsys, Inc. Device [16c3:abcd] (rev 01)
01:00.0 Ethernet controller [0200]: Intel Corporation I210 Gigabit Network Connection [8086:1533] (rev 03)

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/imx6q-b650v3.dts | 52 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-b650v3.dts b/arch/arm/boot/dts/imx6q-b650v3.dts
index 7f9f176901d4..5650a9b11091 100644
--- a/arch/arm/boot/dts/imx6q-b650v3.dts
+++ b/arch/arm/boot/dts/imx6q-b650v3.dts
@@ -111,3 +111,55 @@
 	fsl,tx-cal-45-dp-ohms = <55>;
 	fsl,tx-d-cal = <100>;
 };
+
+&pci_root {
+	/* Intel Corporation I210 Gigabit Network Connection */
+	switch_nic: ethernet@3,0 {
+		compatible = "pci8086,1533";
+		reg = <0x00010000 0 0 0 0>;
+	};
+};
+
+&switch_ports {
+	port@0 {
+		reg = <0>;
+		label = "enacq";
+		phy-handle = <&switchphy0>;
+	};
+
+	port@1 {
+		reg = <1>;
+		label = "eneport1";
+		phy-handle = <&switchphy1>;
+	};
+
+	port@2 {
+		reg = <2>;
+		label = "enix";
+		phy-handle = <&switchphy2>;
+	};
+
+	port@3 {
+		reg = <3>;
+		label = "enid";
+		phy-handle = <&switchphy3>;
+	};
+
+	port@4 {
+		reg = <4>;
+		label = "cpu";
+		ethernet = <&switch_nic>;
+		phy-handle = <&switchphy4>;
+	};
+
+	port@5 {
+		reg = <5>;
+		label = "enembc";
+
+		/* connected to Ethernet MAC of AT91RM9200 in MII mode */
+		fixed-link {
+			speed = <100>;
+			full-duplex;
+		};
+	};
+};
-- 
2.15.1

[PATCHv4 5/5] ARM: dts: imx6q-b450v3: Add switch port configuration

[Sebastian Reichel]

This adds support for the Marvell switch and names the network
ports according to the labels, that can be found next to the
connectors. The switch is connected to the host system using a
PCI based network card.

The PCI bus configuration has been written using the following
information:

root@b450v3# lspci -tv
-[0000:00]---00.0-[01]----00.0  Intel Corporation I210 Gigabit Network Connection
root@b450v3# lspci -nn
00:00.0 PCI bridge [0604]: Synopsys, Inc. Device [16c3:abcd] (rev 01)
01:00.0 Ethernet controller [0200]: Intel Corporation I210 Gigabit Network Connection [8086:1533] (rev 03)

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 arch/arm/boot/dts/imx6q-b450v3.dts | 52 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-b450v3.dts b/arch/arm/boot/dts/imx6q-b450v3.dts
index 404a93d9596b..3ec58500e9c2 100644
--- a/arch/arm/boot/dts/imx6q-b450v3.dts
+++ b/arch/arm/boot/dts/imx6q-b450v3.dts
@@ -112,3 +112,55 @@
                 line-name = "PCA9539-P07";
         };
 };
+
+&pci_root {
+	/* Intel Corporation I210 Gigabit Network Connection */
+	switch_nic: ethernet@3,0 {
+		compatible = "pci8086,1533";
+		reg = <0x00010000 0 0 0 0>;
+	};
+};
+
+&switch_ports {
+	port@0 {
+		reg = <0>;
+		label = "enacq";
+		phy-handle = <&switchphy0>;
+	};
+
+	port@1 {
+		reg = <1>;
+		label = "eneport1";
+		phy-handle = <&switchphy1>;
+	};
+
+	port@2 {
+		reg = <2>;
+		label = "enix";
+		phy-handle = <&switchphy2>;
+	};
+
+	port@3 {
+		reg = <3>;
+		label = "enid";
+		phy-handle = <&switchphy3>;
+	};
+
+	port@4 {
+		reg = <4>;
+		label = "cpu";
+		ethernet = <&switch_nic>;
+		phy-handle = <&switchphy4>;
+	};
+
+	port@5 {
+		reg = <5>;
+		label = "enembc";
+
+		/* connected to Ethernet MAC of AT91RM9200 in MII mode */
+		fixed-link {
+			speed = <100>;
+			full-duplex;
+		};
+	};
+};
-- 
2.15.1

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[David Miller]

From: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Date: Tue, 16 Jan 2018 11:19:54 +0100

> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3,  B650v3 and B450v3, which have a
> built-in MV88E6240 switch hardwired to a PCIe based network card
> making use of the internal PHY. Since mv88e6xxx driver resets the
> chip during probe, the PHY is disabled without this patch resulting
> in missing link and non-functional switch device.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Andrew, Florian, Vivien, please review.

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[David Miller]

From: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Date: Tue, 16 Jan 2018 11:19:54 +0100

> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3,  B650v3 and B450v3, which have a
> built-in MV88E6240 switch hardwired to a PCIe based network card
> making use of the internal PHY. Since mv88e6xxx driver resets the
> chip during probe, the PHY is disabled without this patch resulting
> in missing link and non-functional switch device.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

Andrew, Vivien, Florian, ping.

Please review this.

Thank you.

> ---
>  net/dsa/dsa2.c     | 25 +++++++++++++++++++------
>  net/dsa/dsa_priv.h |  1 +
>  net/dsa/port.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 1e287420ff49..f65938d10b6d 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -18,6 +18,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/of.h>
>  #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -271,11 +272,20 @@ static int dsa_port_setup(struct dsa_port *dp)
>  		break;
>  	case DSA_PORT_TYPE_CPU:
>  	case DSA_PORT_TYPE_DSA:
> -		err = dsa_port_fixed_link_register_of(dp);
> -		if (err) {
> -			dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
> -				ds->index, dp->index);
> -			return err;
> +		if (of_phy_is_fixed_link(dp->dn)) {
> +			err = dsa_port_fixed_link_register_of(dp);
> +			if (err) {
> +				dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
> +					ds->index, dp->index);
> +				return err;
> +			}
> +		} else {
> +			err = dsa_port_setup_phy_of(dp, true);
> +			if (err) {
> +				dev_err(ds->dev, "failed to enable phy for port %d.%d\n",
> +					ds->index, dp->index);
> +				return err;
> +			}
>  		}
>  
>  		break;
> @@ -301,7 +311,10 @@ static void dsa_port_teardown(struct dsa_port *dp)
>  		break;
>  	case DSA_PORT_TYPE_CPU:
>  	case DSA_PORT_TYPE_DSA:
> -		dsa_port_fixed_link_unregister_of(dp);
> +		if (of_phy_is_fixed_link(dp->dn))
> +			dsa_port_fixed_link_unregister_of(dp);
> +		else
> +			dsa_port_setup_phy_of(dp, false);
>  		break;
>  	case DSA_PORT_TYPE_USER:
>  		if (dp->slave) {
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 7d036696e8c4..6c14079e6cc8 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -159,6 +159,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
>  		      const struct switchdev_obj_port_vlan *vlan);
>  int dsa_port_fixed_link_register_of(struct dsa_port *dp);
>  void dsa_port_fixed_link_unregister_of(struct dsa_port *dp);
> +int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable);
>  
>  /* slave.c */
>  extern const struct dsa_device_ops notag_netdev_ops;
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index bb4be2679904..a1518024bba3 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -273,6 +273,55 @@ int dsa_port_vlan_del(struct dsa_port *dp,
>  	return 0;
>  }
>  
> +int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable)
> +{
> +	struct device_node *port_dn = dp->dn;
> +	struct device_node *phy_dn;
> +	struct dsa_switch *ds = dp->ds;
> +	struct phy_device *phydev;
> +	int port = dp->index;
> +	int err = 0;
> +
> +	phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
> +	if (!phy_dn)
> +		return 0;
> +
> +	phydev = of_phy_find_device(phy_dn);
> +	if (!phydev) {
> +		err = -EPROBE_DEFER;
> +		goto err_put_of;
> +	}
> +
> +	if (enable) {
> +		err = genphy_config_init(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +
> +		err = genphy_resume(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +
> +		err = genphy_read_status(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +	} else {
> +		err = genphy_suspend(phydev);
> +		if (err < 0)
> +			goto err_put_dev;
> +	}
> +
> +	if (ds->ops->adjust_link)
> +		ds->ops->adjust_link(ds, port, phydev);
> +
> +	dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev));
> +
> +err_put_dev:
> +	put_device(&phydev->mdio.dev);
> +err_put_of:
> +	of_node_put(phy_dn);
> +	return err;
> +}
> +
>  int dsa_port_fixed_link_register_of(struct dsa_port *dp)
>  {
>  	struct device_node *dn = dp->dn;
> -- 
> 2.15.1
> 

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[Florian Fainelli]

On 01/16/2018 02:19 AM, Sebastian Reichel wrote:
> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3,  B650v3 and B450v3, which have a
> built-in MV88E6240 switch hardwired to a PCIe based network card
> making use of the internal PHY. Since mv88e6xxx driver resets the
> chip during probe, the PHY is disabled without this patch resulting
> in missing link and non-functional switch device.

Apologies for the late review, the code is fine, but there is room for
improvement, see below:

> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>> ---
>  net/dsa/dsa2.c     | 25 +++++++++++++++++++------
>  net/dsa/dsa_priv.h |  1 +
>  net/dsa/port.c     | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 1e287420ff49..f65938d10b6d 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -18,6 +18,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/of.h>
>  #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -271,11 +272,20 @@ static int dsa_port_setup(struct dsa_port *dp)
>  		break;
>  	case DSA_PORT_TYPE_CPU:
>  	case DSA_PORT_TYPE_DSA:
> -		err = dsa_port_fixed_link_register_of(dp);
> -		if (err) {
> -			dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
> -				ds->index, dp->index);
> -			return err;
> +		if (of_phy_is_fixed_link(dp->dn)) {
> +			err = dsa_port_fixed_link_register_of(dp);

This function does exactly the same check you are adding, which
indicates that you should create a common function, e.g:
dsa_port_setup_link_of() which internally does check whether the PHY is
fixed or not and does the registration.

> +			if (err) {
> +				dev_err(ds->dev, "failed to register fixed link for port %d.%d\n",
> +					ds->index, dp->index);
> +				return err;
> +			}
> +		} else {
> +			err = dsa_port_setup_phy_of(dp, true);
> +			if (err) {
> +				dev_err(ds->dev, "failed to enable phy for port %d.%d\n",
> +					ds->index, dp->index);
> +				return err;
> +			}
>  		}
>  
>  		break;
> @@ -301,7 +311,10 @@ static void dsa_port_teardown(struct dsa_port *dp)
>  		break;
>  	case DSA_PORT_TYPE_CPU:
>  	case DSA_PORT_TYPE_DSA:
> -		dsa_port_fixed_link_unregister_of(dp);
> +		if (of_phy_is_fixed_link(dp->dn))
> +			dsa_port_fixed_link_unregister_of(dp);
> +		else
> +			dsa_port_setup_phy_of(dp, false);

Likewise, please rename dsa_port_fixed_link_unregister_of() into e.g:
dsa_port_teardown_link_of() and take care of the two cases.

The rest of the changes do look okay to me.

Note: there is still technically a misreprentation of how the PHY is
"attached" to the network device. In your DTSes, you have to have the
CPU port have a "phy-handle" to the internal PHY, while technically it
should be the i210 which has a "phy-handle" property to that PHY, and
even better, if the e1000e/idb drivers were PHYLIB capable, they could
manage it directly.

Since this is a link, which has two ends, it is probably acceptable to
make that shortcut with lack of a better solution.
-- 
Florian

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[Andrew Lunn]

> Note: there is still technically a misreprentation of how the PHY is
> "attached" to the network device. In your DTSes, you have to have the
> CPU port have a "phy-handle" to the internal PHY, while technically it
> should be the i210 which has a "phy-handle" property to that PHY, and
> even better, if the e1000e/idb drivers were PHYLIB capable, they could
> manage it directly.

Hi Florian

Err, i don't think i agree. But maybe i'm missunderstanding.

We have two back-to-back PHYs. I would expect the i210 MAC to have a
phy-handle pointing it its PHY. The CPU port would then point to the
internal switch PHY.

Or are you suggesting the i210 has two phy-handles, its own and the
switches?

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[Florian Fainelli]

On 01/22/2018 12:54 PM, Andrew Lunn wrote:
>> Note: there is still technically a misreprentation of how the PHY is
>> "attached" to the network device. In your DTSes, you have to have the
>> CPU port have a "phy-handle" to the internal PHY, while technically it
>> should be the i210 which has a "phy-handle" property to that PHY, and
>> even better, if the e1000e/idb drivers were PHYLIB capable, they could
>> manage it directly.
> 
> Hi Florian
> 
> Err, i don't think i agree. But maybe i'm missunderstanding.
> 
> We have two back-to-back PHYs. I would expect the i210 MAC to have a
> phy-handle pointing it its PHY. The CPU port would then point to the
> internal switch PHY.

Is it really a back-to-back PHY? If that is the case, ok, that can
indeed work without magnetics, but this is really an inefficient way to
connect a MAC to a switch, especially when you can do direct (R)GMII
without any PHY in between... If that is the case, then disregard my
comment.

> 
> Or are you suggesting the i210 has two phy-handles, its own and the
> switches?

Not suggesting that, that would be weird.

> 
> 	Andrew
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/5] net: dsa: Support internal phy on 'cpu' port

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

Hi,

On Mon, Jan 22, 2018 at 12:56:44PM -0800, Florian Fainelli wrote:
> On 01/22/2018 12:54 PM, Andrew Lunn wrote:
> >> Note: there is still technically a misreprentation of how the PHY is
> >> "attached" to the network device. In your DTSes, you have to have the
> >> CPU port have a "phy-handle" to the internal PHY, while technically it
> >> should be the i210 which has a "phy-handle" property to that PHY, and
> >> even better, if the e1000e/idb drivers were PHYLIB capable, they could
> >> manage it directly.
> > 
> > Hi Florian
> > 
> > Err, i don't think i agree. But maybe i'm missunderstanding.
> > 
> > We have two back-to-back PHYs. I would expect the i210 MAC to have a
> > phy-handle pointing it its PHY. The CPU port would then point to the
> > internal switch PHY.

Right. For the i210 the internal phy is used, so there is no
phy-handle. It's basically a normal network card. It looks
like this:

i210.internal-phy <--- pcb lanes ---> switch.port4.internal-phy

> Is it really a back-to-back PHY? If that is the case, ok, that can
> indeed work without magnetics, but this is really an inefficient way to
> connect a MAC to a switch, especially when you can do direct (R)GMII
> without any PHY in between... If that is the case, then disregard my
> comment.
>
> > Or are you suggesting the i210 has two phy-handles, its own and the
> > switches?
> 
> Not suggesting that, that would be weird.
> 
> > 
> > 	Andrew
> > 
> 
> 
> -- 
> Florian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]