staging: octeon: Convert to use phylink

Quoted 9 lines

drivers/staging/octeon/ethernet-rx.c +1 -1

-void cvm_oct_rx_initialize(void)
+int cvm_oct_rx_initialize(void)
 {
 	int i;
 	struct net_device *dev_for_napi = NULL;

@@ -460,8 +460,11 @@ void cvm_oct_rx_initialize(void)
 		}
 	}
 

drivers/staging/octeon/ethernet-rx.c void cvm_oct_rx_initialize(void) · line 464

Quoted 4 lines

drivers/staging/octeon/ethernet-rx.c +2 -2

-	if (!dev_for_napi)
-		panic("No net_devices were allocated.");
+	if (!dev_for_napi) {
+		pr_err("No net_devices were allocated.");

It is good practice to use dev_per(dev, ... You then know which device has problem finding its net_devices.

checkpatch is probably warning you about this.

Once you have a registered netdev, you should then use netdev_err(), netdev_dbg() etc.

However, cvm_oct_probe() in 6.3-rc6 seems to be FUBAR. As soon as you call register_netdev(dev), the kernel can start using it, even before that call returns. So the register_netdev(dev) should be the last thing _probe does, once everything is set up. You can call netdev_err() before it is registered, but the name is less informative, something like "(unregistered)".

Andrew

Problem with this code is that it registers netdevices in for loop, so the only device available here is parent device to all that netdevices (which weren't registered).

drivers/staging/octeon/ethernet.c +4 -0

 	num_interfaces = cvmx_helper_get_number_of_interfaces();
 	for (interface = 0; interface < num_interfaces; interface++) {
+		int num_ports, port_index;
+		const struct net_device_ops *ops;
+		const char *name;
+		phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;

Quoted 13 lines

drivers/staging/octeon/ethernet.c +7 -3

 	num_interfaces = cvmx_helper_get_number_of_interfaces();
 	for (interface = 0; interface < num_interfaces; interface++) {
+		int num_ports, port_index;
+		const struct net_device_ops *ops;
+		const char *name;
+		phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
 		cvmx_helper_interface_mode_t imode =
-		    cvmx_helper_interface_get_mode(interface);
-		int num_ports = cvmx_helper_ports_on_interface(interface);
-		int port_index;
+			cvmx_helper_interface_get_mode(interface);
+
+		switch (imode) {

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 812

Quoted 3 lines

drivers/staging/octeon/ethernet.c +3 -0

+		case CVMX_HELPER_INTERFACE_MODE_NPI:
+			ops = &cvm_oct_npi_netdev_ops;
+			name = "npi%d";

In general, the kernel does not give the interface names other than ethX. userspace can rename them, e.g. systemd with its persistent names. So as part of getting this driver out of staging, i would throw this naming code away.

Quoted 5 lines

drivers/staging/octeon/ethernet.c +1 -0

+		num_ports = cvmx_helper_ports_on_interface(interface);
 		for (port_index = 0,
 		     port = cvmx_helper_get_ipd_port(interface, 0);
 		     port < cvmx_helper_get_ipd_port(interface, num_ports);
 		     port_index++, port++) {

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 858

Quoted 4 lines

drivers/staging/octeon/ethernet.c +1 -1

 			struct octeon_ethernet *priv;
 			struct net_device *dev =
-			    alloc_etherdev(sizeof(struct octeon_ethernet));
+				alloc_etherdev(sizeof(struct octeon_ethernet));

Please try to avoid white space changed. Put such white space changes into a patch of their own, with a commit message saying it just contains whitespace cleanup.

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 878

Quoted 9 lines

drivers/staging/octeon/ethernet.c +1 -1

 			if (!dev) {
 				pr_err("Failed to allocate ethernet device for port %d\n",
 				       port);

@@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev)
 			priv->port = port;
 			priv->queue = cvmx_pko_get_base_queue(priv->port);
 			priv->fau = fau - cvmx_pko_get_num_queues(port) * 4;
-			priv->phy_mode = PHY_INTERFACE_MODE_NA;
+			priv->phy_mode = phy_mode;

You should be getting phy_mode from DT.

Ideally, you want lots of small patches which are obviously correct. So i would try to break this up into smaller changes.

I also wounder if you are addresses issues in the correct order. This driver is in staging for a reason. It needs a lot of work. You might be better off first cleaning it up. And then consider moving it to phylink.

Andrew

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you?

Quoted 5 lines

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you?

Is you end goal to get this out of staging? phylib vs phylink is not a reason to keep it in staging.

It just seems odd to be adding new features to a staging driver. As a bit of a "carrot and stick" maybe we should say you cannot add new features until it is ready to move out of staging?

But staging is not my usual domain.

Andrew

Quoted 5 lines

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you?

Is you end goal to get this out of staging? phylib vs phylink is not a reason to keep it in staging.

It just seems odd to be adding new features to a staging driver. As a bit of a "carrot and stick" maybe we should say you cannot add new features until it is ready to move out of staging?

But staging is not my usual domain.

Andrew

Quoted 5 lines

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you?

Is you end goal to get this out of staging? phylib vs phylink is not a reason to keep it in staging.

It just seems odd to be adding new features to a staging driver. As a bit of a "carrot and stick" maybe we should say you cannot add new features until it is ready to move out of staging?

Quoted 9 lines · Andrew Lunn

Quoted 5 lines

I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as this driver is splitted between staging and arch/mips/cavium-octeon/executive/ However I'll provide changes spotted previously as separate preparational patches. Would that work for you?

Is you end goal to get this out of staging? phylib vs phylink is not a reason to keep it in staging.

Quoted 3 lines

It just seems odd to be adding new features to a staging driver. As a bit of a "carrot and stick" maybe we should say you cannot add new features until it is ready to move out of staging?

We already have that rule. But I don't know anything about phy vs phylink...

I'm proposing to leave all that trickery behind and just follow what's written in device tree, so each I/O interface ends up as a driver with its own mac ops. While it is possible to implement that as private mac ops as some other drivers do, I think it is more convenient to use phylink_mac_ops.

In case I'm missing something or I'm wrong with analysis, please let me know.

From: Ladislav Michl <[email protected]>

Signed-off-by Ladislav Michl <[email protected]>

---
 drivers/staging/octeon/Kconfig           |   2 +-
 drivers/staging/octeon/ethernet-mdio.c   | 171 ++++++++++++++---------
 drivers/staging/octeon/ethernet-rgmii.c  |  13 +-
 drivers/staging/octeon/ethernet.c        |  64 +++------
 drivers/staging/octeon/octeon-ethernet.h |   8 +-
 5 files changed, 136 insertions(+), 122 deletions(-)

drivers/staging/octeon/Kconfig +1 -1

index 5319909eb2f6..fda90025710d 100644
@@ -3,7 +3,7 @@ config OCTEON_ETHERNET
 	tristate "Cavium Networks Octeon Ethernet support"
 	depends on CAVIUM_OCTEON_SOC || COMPILE_TEST
 	depends on NETDEVICES
-	select PHYLIB
+	select PHYLINK
 	select MDIO_OCTEON
 	help
 	  This driver supports the builtin ethernet ports on Cavium

drivers/staging/octeon/ethernet-mdio.c +0 -0

index b3049108edc4..a14fb4dbb2fd 100644

drivers/staging/octeon/ethernet-mdio.c +31 -16

+#include <linux/of_net.h>
 #include <generated/utsrelease.h>
 #include <net/dst.h>
 

@@ -26,23 +26,27 @@ static void cvm_oct_get_drvinfo(struct net_device *dev,
 	strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
 }
 
-static int cvm_oct_nway_reset(struct net_device *dev)
+static int cvm_oct_get_link_ksettings(struct net_device *dev,
+				      struct ethtool_link_ksettings *cmd)
 {
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
+	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
+}
 
-	if (dev->phydev)
-		return phy_start_aneg(dev->phydev);
+static int cvm_oct_set_link_ksettings(struct net_device *dev,
+				      const struct ethtool_link_ksettings *cmd)
+{
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
-	return -EINVAL;
+	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
 }
 
 const struct ethtool_ops cvm_oct_ethtool_ops = {
 	.get_drvinfo = cvm_oct_get_drvinfo,
-	.nway_reset = cvm_oct_nway_reset,
 	.get_link = ethtool_op_get_link,
-	.get_link_ksettings = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings = cvm_oct_get_link_ksettings,
+	.set_link_ksettings = cvm_oct_set_link_ksettings,
 };
 
 /**

@@ -55,53 +59,80 @@ const struct ethtool_ops cvm_oct_ethtool_ops = {
  */
 int cvm_oct_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
-	if (!netif_running(dev))
-		return -EINVAL;
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
-	if (!dev->phydev)
-		return -EINVAL;
+	return phylink_mii_ioctl(priv->phylink, rq, cmd);
+}
 
-	return phy_mii_ioctl(dev->phydev, rq, cmd);
+static void cvm_oct_mac_get_state(struct phylink_config *config,
+				  struct phylink_link_state *state)
+{
+	union cvmx_helper_link_info link_info;
+	struct net_device *dev = to_net_dev(config->dev);
+	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	link_info = cvmx_helper_link_get(priv->port);
+	state->link = link_info.s.link_up;
+	state->duplex = link_info.s.full_duplex ? DUPLEX_FULL : DUPLEX_HALF;
+	state->speed = link_info.s.speed;
 }
 
-void cvm_oct_note_carrier(struct octeon_ethernet *priv,
-			  union cvmx_helper_link_info li)
+static void cvm_oct_mac_config(struct phylink_config *config,
+			       unsigned int mode,
+			       const struct phylink_link_state *state)
 {

drivers/staging/octeon/ethernet-mdio.c +65 -38

+
+	cvmx_helper_link_set(priv->port, link_info);
+
+	priv->poll_used = false;
+}
+
+static void cvm_oct_mac_link_up(struct phylink_config *config,
+				struct phy_device *phy,
+				unsigned int mode, phy_interface_t interface,
+				int speed, int duplex,
+				bool tx_pause, bool rx_pause)
+{
 	union cvmx_helper_link_info link_info;
+	struct net_device *dev = to_net_dev(config->dev);
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
 	link_info.u64		= 0;
-	link_info.s.link_up	= dev->phydev->link ? 1 : 0;
-	link_info.s.full_duplex = dev->phydev->duplex ? 1 : 0;
-	link_info.s.speed	= dev->phydev->speed;
+	link_info.s.link_up	= 1;
+	link_info.s.full_duplex = duplex == DUPLEX_FULL ? 1 : 0;
+	link_info.s.speed	= speed;
 	priv->link_info		= link_info.u64;
 
-	/*
-	 * The polling task need to know about link status changes.
-	 */
-	if (priv->poll)
-		priv->poll(dev);
+	cvmx_helper_link_set(priv->port, link_info);
 
-	if (priv->last_link != dev->phydev->link) {
-		priv->last_link = dev->phydev->link;
-		cvmx_helper_link_set(priv->port, link_info);
-		cvm_oct_note_carrier(priv, link_info);
+	if (!phy && priv->poll) {
+		priv->poll_used = true;
+		priv->poll(dev);
 	}
 }
 
+static const struct phylink_mac_ops cvm_oct_phylink_ops = {
+	.validate = phylink_generic_validate,
+	.mac_pcs_get_state = cvm_oct_mac_get_state,
+	.mac_config = cvm_oct_mac_config,
+	.mac_link_down = cvm_oct_mac_link_down,
+	.mac_link_up = cvm_oct_mac_link_up,
+};
+
 int cvm_oct_common_stop(struct net_device *dev)
 {
 	struct octeon_ethernet *priv = netdev_priv(dev);

@@ -116,15 +147,14 @@ int cvm_oct_common_stop(struct net_device *dev)
 
 	priv->poll = NULL;
 
-	if (dev->phydev)
-		phy_disconnect(dev->phydev);
+	phylink_stop(priv->phylink);
+	phylink_disconnect_phy(priv->phylink);
 
 	if (priv->last_link) {
 		link_info.u64 = 0;
 		priv->last_link = 0;
 
 		cvmx_helper_link_set(priv->port, link_info);
-		cvm_oct_note_carrier(priv, link_info);
 	}
 	return 0;
 }

@@ -138,34 +168,45 @@ int cvm_oct_common_stop(struct net_device *dev)
  */
 int cvm_oct_phy_setup_device(struct net_device *dev)
 {
+	phy_interface_t phy_mode;
+	struct phylink *phylink;
 	struct octeon_ethernet *priv = netdev_priv(dev);
-	struct device_node *phy_node;
-	struct phy_device *phydev = NULL;
 
-	if (!priv->of_node)
-		goto no_phy;
-
-	phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
-	if (!phy_node && of_phy_is_fixed_link(priv->of_node))
-		phy_node = of_node_get(priv->of_node);
-	if (!phy_node)
-		goto no_phy;
+	priv->phylink_config.dev = &dev->dev;
+	priv->phylink_config.type = PHYLINK_NETDEV;
+	priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
+						MAC_10 | MAC_100 | MAC_1000;
+	__set_bit(PHY_INTERFACE_MODE_MII,
+		  priv->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_RMII,
+		  priv->phylink_config.supported_interfaces);
+
+	switch (priv->imode) {
+	case CVMX_HELPER_INTERFACE_MODE_RGMII:
+		phy_interface_set_rgmii(priv->phylink_config.supported_interfaces);
+		break;
+	case CVMX_HELPER_INTERFACE_MODE_SGMII:
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  priv->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  priv->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_QSGMII,
+			  priv->phylink_config.supported_interfaces);
+		break;
+	default:
+		break;
+	}
 
-	phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
-				priv->phy_mode);
-	of_node_put(phy_node);
+	if (of_get_phy_mode(priv->of_node, &phy_mode) == 0)
+		priv->phy_mode = phy_mode;
 
-	if (!phydev)
-		return -EPROBE_DEFER;
+	phylink = phylink_create(&priv->phylink_config,
+				 of_fwnode_handle(priv->of_node),
+				 priv->phy_mode, &cvm_oct_phylink_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
 
-	priv->last_link = 0;
-	phy_start(phydev);
+	priv->phylink = phylink;
 
-	return 0;
-no_phy:
-	/* If there is no phy, assume a direct MAC connection and that
-	 * the link is up.
-	 */
-	netif_carrier_on(dev);
 	return 0;
 }

drivers/staging/octeon/ethernet-rgmii.c +0 -0

index 0c4fac31540a..8c6eb0b87254 100644

drivers/staging/octeon/ethernet-rgmii.c +1 -0

+		phylink_mac_change(priv->phylink, link_info.s.link_up);
 }
 
 int cvm_oct_rgmii_open(struct net_device *dev)

drivers/staging/octeon/ethernet.c +1 -1

index 466d43a71d34..21892f805245 100644
@@ -10,7 +10,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/of_mdio.h>

drivers/staging/octeon/ethernet.c +0 -2

 	cvm_oct_device[priv->port]->netdev_ops->ndo_get_stats

@@ -446,23 +446,20 @@ int cvm_oct_common_init(struct net_device *dev)
 
 void cvm_oct_common_uninit(struct net_device *dev)
 {
-	if (dev->phydev)
-		phy_disconnect(dev->phydev);

drivers/staging/octeon/ethernet.c +10 -25

 	union cvmx_gmxx_prtx_cfg gmx_cfg;
 	struct octeon_ethernet *priv = netdev_priv(dev);
 	int interface = INTERFACE(priv->port);
 	int index = INDEX(priv->port);
-	union cvmx_helper_link_info link_info;
-	int rv;
-
-	rv = cvm_oct_phy_setup_device(dev);
-	if (rv)
-		return rv;
 
 	gmx_cfg.u64 = cvmx_read_csr(CVMX_GMXX_PRTX_CFG(index, interface));
 	gmx_cfg.s.en = 1;

@@ -473,20 +470,17 @@ int cvm_oct_common_open(struct net_device *dev,
 	if (octeon_is_simulation())
 		return 0;
 
-	if (dev->phydev) {
-		int r = phy_read_status(dev->phydev);
-
-		if (r == 0 && dev->phydev->link == 0)
-			netif_carrier_off(dev);
-		cvm_oct_adjust_link(dev);
-	} else {
-		link_info = cvmx_helper_link_get(priv->port);
-		if (!link_info.s.link_up)
-			netif_carrier_off(dev);
-		priv->poll = link_poll;
-		link_poll(dev);
+	err = phylink_of_phy_connect(priv->phylink, priv->of_node, 0);
+	if (err) {
+		netdev_err(dev, "Could not attach PHY (%d)\n", err);
+		return err;
 	}
 
+	priv->poll_used = false;
+	priv->poll = link_poll;
+
+	phylink_start(priv->phylink);
+
 	return 0;
 }
 

@@ -504,13 +498,7 @@ void cvm_oct_link_poll(struct net_device *dev)
 	else
 		priv->link_info = link_info.u64;
 
-	if (link_info.s.link_up) {
-		if (!netif_carrier_ok(dev))
-			netif_carrier_on(dev);
-	} else if (netif_carrier_ok(dev)) {
-		netif_carrier_off(dev);
-	}
-	cvm_oct_note_carrier(priv, link_info);
+	phylink_mac_change(priv->phylink, link_info.s.link_up);
 }
 
 static int cvm_oct_xaui_open(struct net_device *dev)

drivers/staging/octeon/ethernet.c +0 -0

 	for (interface = 0; interface < num_interfaces; interface++) {
 		int num_ports, port_index;
 		const struct net_device_ops *ops;

@@ -889,18 +876,15 @@ static int cvm_oct_probe(struct platform_device *pdev)
 			dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead;
 			dev->max_mtu = OCTEON_MAX_MTU - mtu_overhead;
 

Quoted 3 lines · Ladislav Michl

From: Ladislav Michl <[email protected]>

Signed-off-by Ladislav Michl <[email protected]>

We always insist that every commit needs a commit message.

Why are you doing this? Does this have any effect for the user? Imagine you are a power user and something stops working and they are reviewing the git log to see if it's intentional or not.

Quoted 24 lines

---
 drivers/staging/octeon/Kconfig           |   2 +-
 drivers/staging/octeon/ethernet-mdio.c   | 171 ++++++++++++++---------
 drivers/staging/octeon/ethernet-rgmii.c  |  13 +-
 drivers/staging/octeon/ethernet.c        |  64 +++------
 drivers/staging/octeon/octeon-ethernet.h |   8 +-
 5 files changed, 136 insertions(+), 122 deletions(-)

drivers/staging/octeon/Kconfig +1 -1

index 5319909eb2f6..fda90025710d 100644
@@ -3,7 +3,7 @@ config OCTEON_ETHERNET
 	tristate "Cavium Networks Octeon Ethernet support"
 	depends on CAVIUM_OCTEON_SOC || COMPILE_TEST
 	depends on NETDEVICES
-	select PHYLIB
+	select PHYLINK
 	select MDIO_OCTEON
 	help
 	  This driver supports the builtin ethernet ports on Cavium

drivers/staging/octeon/ethernet-mdio.c +0 -1

index b3049108edc4..a14fb4dbb2fd 100644
--- a/drivers/staging/octeon/ethernet-mdio.c

drivers/staging/octeon/ethernet-mdio.c line 12

Quoted 6 lines

+++ b/drivers/staging/octeon/ethernet-mdio.c

drivers/staging/octeon/ethernet-mdio.c +0 -1

@@ -9,7 +9,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/ratelimit.h>
-#include <linux/of_mdio.h>

Removing this include seems unrelated?

Quoted 78 lines

drivers/staging/octeon/ethernet-mdio.c +31 -20

+#include <linux/of_net.h>
 #include <generated/utsrelease.h>
 #include <net/dst.h>
 

@@ -26,23 +26,27 @@ static void cvm_oct_get_drvinfo(struct net_device *dev,
 	strscpy(info->bus_info, "Builtin", sizeof(info->bus_info));
 }
 
-static int cvm_oct_nway_reset(struct net_device *dev)
+static int cvm_oct_get_link_ksettings(struct net_device *dev,
+				      struct ethtool_link_ksettings *cmd)
 {
-	if (!capable(CAP_NET_ADMIN))
-		return -EPERM;
+	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
+}
 
-	if (dev->phydev)
-		return phy_start_aneg(dev->phydev);
+static int cvm_oct_set_link_ksettings(struct net_device *dev,
+				      const struct ethtool_link_ksettings *cmd)
+{
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
-	return -EINVAL;
+	return phylink_ethtool_ksettings_set(priv->phylink, cmd);
 }
 
 const struct ethtool_ops cvm_oct_ethtool_ops = {
 	.get_drvinfo = cvm_oct_get_drvinfo,
-	.nway_reset = cvm_oct_nway_reset,
 	.get_link = ethtool_op_get_link,
-	.get_link_ksettings = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings = cvm_oct_get_link_ksettings,
+	.set_link_ksettings = cvm_oct_set_link_ksettings,
 };
 
 /**

@@ -55,53 +59,80 @@ const struct ethtool_ops cvm_oct_ethtool_ops = {
  */
 int cvm_oct_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
-	if (!netif_running(dev))
-		return -EINVAL;
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
-	if (!dev->phydev)
-		return -EINVAL;
+	return phylink_mii_ioctl(priv->phylink, rq, cmd);
+}
 
-	return phy_mii_ioctl(dev->phydev, rq, cmd);
+static void cvm_oct_mac_get_state(struct phylink_config *config,
+				  struct phylink_link_state *state)
+{
+	union cvmx_helper_link_info link_info;
+	struct net_device *dev = to_net_dev(config->dev);
+	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	link_info = cvmx_helper_link_get(priv->port);
+	state->link = link_info.s.link_up;
+	state->duplex = link_info.s.full_duplex ? DUPLEX_FULL : DUPLEX_HALF;
+	state->speed = link_info.s.speed;
 }
 
-void cvm_oct_note_carrier(struct octeon_ethernet *priv,
-			  union cvmx_helper_link_info li)
+static void cvm_oct_mac_config(struct phylink_config *config,
+			       unsigned int mode,
+			       const struct phylink_link_state *state)
 {
-	if (li.s.link_up) {
-		pr_notice_ratelimited("%s: %u Mbps %s duplex, port %d, queue %d\n",
-				      netdev_name(priv->netdev), li.s.speed,
-				      (li.s.full_duplex) ? "Full" : "Half",

drivers/staging/octeon/ethernet-mdio.c const struct ethtool_ops cvm_oct_ethtool_ops = { · line 84

Quoted 6 lines

drivers/staging/octeon/ethernet-mdio.c +0 -5

-				      priv->port, priv->queue);
-	} else {
-		pr_notice_ratelimited("%s: Link down\n",
-				      netdev_name(priv->netdev));
-	}
 }

Delete this dummy function.

Quoted 6 lines

drivers/staging/octeon/ethernet-mdio.c +4 -1

-void cvm_oct_adjust_link(struct net_device *dev)
+static void cvm_oct_mac_link_down(struct phylink_config *config,
+				  unsigned int mode, phy_interface_t interface)
 {
+	union cvmx_helper_link_info link_info;
+	struct net_device *dev = to_net_dev(config->dev);

drivers/staging/octeon/ethernet-mdio.c const struct ethtool_ops cvm_oct_ethtool_ops = { · line 96

Quoted 6 lines

drivers/staging/octeon/ethernet-mdio.c +5 -0

 	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	link_info.u64		= 0;
+	link_info.s.link_up	= 0;
+	link_info.s.full_duplex = 0;
+	link_info.s.speed	= 0;

Just do this in the initializer:

	union cvmx_helper_link_info link_info = {};

drivers/staging/octeon/ethernet-mdio.c const struct ethtool_ops cvm_oct_ethtool_ops = { · line 97

Quoted 1 line

drivers/staging/octeon/ethernet-mdio.c +1 -0

+	priv->link_info		= link_info.u64;

Best to not align anything in a .c file. Stuff in .c files changes a lot and then when you change it you have to re-indent everything. And it's like, *ugh*, is re-indenting supposed to be done in a follow on patch?

The "link_info.u64" also zeroes everything so the rest was already duplicative...

Quoted 146 lines

drivers/staging/octeon/ethernet-mdio.c +65 -38

+
+	cvmx_helper_link_set(priv->port, link_info);
+
+	priv->poll_used = false;
+}
+
+static void cvm_oct_mac_link_up(struct phylink_config *config,
+				struct phy_device *phy,
+				unsigned int mode, phy_interface_t interface,
+				int speed, int duplex,
+				bool tx_pause, bool rx_pause)
+{
 	union cvmx_helper_link_info link_info;
+	struct net_device *dev = to_net_dev(config->dev);
+	struct octeon_ethernet *priv = netdev_priv(dev);
 
 	link_info.u64		= 0;
-	link_info.s.link_up	= dev->phydev->link ? 1 : 0;
-	link_info.s.full_duplex = dev->phydev->duplex ? 1 : 0;
-	link_info.s.speed	= dev->phydev->speed;
+	link_info.s.link_up	= 1;
+	link_info.s.full_duplex = duplex == DUPLEX_FULL ? 1 : 0;
+	link_info.s.speed	= speed;
 	priv->link_info		= link_info.u64;
 
-	/*
-	 * The polling task need to know about link status changes.
-	 */
-	if (priv->poll)
-		priv->poll(dev);
+	cvmx_helper_link_set(priv->port, link_info);
 
-	if (priv->last_link != dev->phydev->link) {
-		priv->last_link = dev->phydev->link;
-		cvmx_helper_link_set(priv->port, link_info);
-		cvm_oct_note_carrier(priv, link_info);
+	if (!phy && priv->poll) {
+		priv->poll_used = true;
+		priv->poll(dev);
 	}
 }
 
+static const struct phylink_mac_ops cvm_oct_phylink_ops = {
+	.validate = phylink_generic_validate,
+	.mac_pcs_get_state = cvm_oct_mac_get_state,
+	.mac_config = cvm_oct_mac_config,
+	.mac_link_down = cvm_oct_mac_link_down,
+	.mac_link_up = cvm_oct_mac_link_up,
+};
+
 int cvm_oct_common_stop(struct net_device *dev)
 {
 	struct octeon_ethernet *priv = netdev_priv(dev);

@@ -116,15 +147,14 @@ int cvm_oct_common_stop(struct net_device *dev)
 
 	priv->poll = NULL;
 
-	if (dev->phydev)
-		phy_disconnect(dev->phydev);
+	phylink_stop(priv->phylink);
+	phylink_disconnect_phy(priv->phylink);
 
 	if (priv->last_link) {
 		link_info.u64 = 0;
 		priv->last_link = 0;
 
 		cvmx_helper_link_set(priv->port, link_info);
-		cvm_oct_note_carrier(priv, link_info);
 	}
 	return 0;
 }

@@ -138,34 +168,45 @@ int cvm_oct_common_stop(struct net_device *dev)
  */
 int cvm_oct_phy_setup_device(struct net_device *dev)
 {
+	phy_interface_t phy_mode;
+	struct phylink *phylink;
 	struct octeon_ethernet *priv = netdev_priv(dev);
-	struct device_node *phy_node;
-	struct phy_device *phydev = NULL;
 
-	if (!priv->of_node)
-		goto no_phy;
-
-	phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
-	if (!phy_node && of_phy_is_fixed_link(priv->of_node))
-		phy_node = of_node_get(priv->of_node);
-	if (!phy_node)
-		goto no_phy;
+	priv->phylink_config.dev = &dev->dev;
+	priv->phylink_config.type = PHYLINK_NETDEV;
+	priv->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
+						MAC_10 | MAC_100 | MAC_1000;
+	__set_bit(PHY_INTERFACE_MODE_MII,
+		  priv->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_RMII,
+		  priv->phylink_config.supported_interfaces);
+
+	switch (priv->imode) {
+	case CVMX_HELPER_INTERFACE_MODE_RGMII:
+		phy_interface_set_rgmii(priv->phylink_config.supported_interfaces);
+		break;
+	case CVMX_HELPER_INTERFACE_MODE_SGMII:
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  priv->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  priv->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_QSGMII,
+			  priv->phylink_config.supported_interfaces);
+		break;
+	default:
+		break;
+	}
 
-	phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
-				priv->phy_mode);
-	of_node_put(phy_node);
+	if (of_get_phy_mode(priv->of_node, &phy_mode) == 0)
+		priv->phy_mode = phy_mode;
 
-	if (!phydev)
-		return -EPROBE_DEFER;
+	phylink = phylink_create(&priv->phylink_config,
+				 of_fwnode_handle(priv->of_node),
+				 priv->phy_mode, &cvm_oct_phylink_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
 
-	priv->last_link = 0;
-	phy_start(phydev);
+	priv->phylink = phylink;
 
-	return 0;
-no_phy:
-	/* If there is no phy, assume a direct MAC connection and that
-	 * the link is up.
-	 */
-	netif_carrier_on(dev);
 	return 0;
 }

drivers/staging/octeon/ethernet-rgmii.c +0 -0

index 0c4fac31540a..8c6eb0b87254 100644
@@ -115,17 +115,8 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 3 lines

drivers/staging/octeon/ethernet-rgmii.c +0 -1

 	cvm_oct_check_preamble_errors(dev);
 
-	if (likely(!status_change))

                   ^

Negated.

Quoted 5 lines

drivers/staging/octeon/ethernet-rgmii.c +0 -5

-		return;
-
-	/* Tell core. */
-	if (link_info.s.link_up) {
-		if (!netif_carrier_ok(dev))

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 6 lines

drivers/staging/octeon/ethernet-rgmii.c +1 -5

-			netif_carrier_on(dev);
-	} else if (netif_carrier_ok(dev)) {
-		netif_carrier_off(dev);
-	}
-	cvm_oct_note_carrier(priv, link_info);
+	if (likely(status_change))

Originally a status_change was unlikely but now it is likely.

Quoted 21 lines

drivers/staging/octeon/ethernet-rgmii.c +1 -0

+		phylink_mac_change(priv->phylink, link_info.s.link_up);
 }
 
 int cvm_oct_rgmii_open(struct net_device *dev)

drivers/staging/octeon/ethernet.c +1 -1

index 466d43a71d34..21892f805245 100644
@@ -10,7 +10,7 @@
 #include <linux/module.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/of_mdio.h>

@@ -128,7 +128,7 @@ static void cvm_oct_periodic_worker(struct work_struct *work)
 						    struct octeon_ethernet,
 						    port_periodic_work.work);
 

drivers/staging/octeon/ethernet.c static void cvm_oct_periodic_worker(struct work_struct *work) · line 132

Quoted 3 lines

drivers/staging/octeon/ethernet.c +1 -1

-	if (priv->poll)
+	if (priv->poll_used && priv->poll)
 		priv->poll(cvm_oct_device[priv->port]);

The name poll_used was really confusing to me. We set it before we've done any polling so the tense is wrong. I think it means that we should not bother polling if the link is down... Could we change the name to ->link_up? Or maybe add a comment?

Quoted 7 lines

drivers/staging/octeon/ethernet.c +0 -2

 	cvm_oct_device[priv->port]->netdev_ops->ndo_get_stats

@@ -446,23 +446,20 @@ int cvm_oct_common_init(struct net_device *dev)
 
 void cvm_oct_common_uninit(struct net_device *dev)
 {
-	if (dev->phydev)
-		phy_disconnect(dev->phydev);

drivers/staging/octeon/ethernet.c int cvm_oct_common_init(struct net_device *dev) · line 458

Quoted 10 lines

drivers/staging/octeon/ethernet.c +5 -0

+	struct octeon_ethernet *priv = netdev_priv(dev);
+
+	cancel_delayed_work_sync(&priv->port_periodic_work);
+	phylink_destroy(priv->phylink);
 }
 
 int cvm_oct_common_open(struct net_device *dev,
 			void (*link_poll)(struct net_device *))
 {
+	int err;

This is networking code so declarations need to be in reverse Christmas tree order.

	long long_variable_name;
	medium variable_name;
	short name;

Quoted 62 lines

drivers/staging/octeon/ethernet.c +10 -25

 	union cvmx_gmxx_prtx_cfg gmx_cfg;
 	struct octeon_ethernet *priv = netdev_priv(dev);
 	int interface = INTERFACE(priv->port);
 	int index = INDEX(priv->port);
-	union cvmx_helper_link_info link_info;
-	int rv;
-
-	rv = cvm_oct_phy_setup_device(dev);
-	if (rv)
-		return rv;
 
 	gmx_cfg.u64 = cvmx_read_csr(CVMX_GMXX_PRTX_CFG(index, interface));
 	gmx_cfg.s.en = 1;

@@ -473,20 +470,17 @@ int cvm_oct_common_open(struct net_device *dev,
 	if (octeon_is_simulation())
 		return 0;
 
-	if (dev->phydev) {
-		int r = phy_read_status(dev->phydev);
-
-		if (r == 0 && dev->phydev->link == 0)
-			netif_carrier_off(dev);
-		cvm_oct_adjust_link(dev);
-	} else {
-		link_info = cvmx_helper_link_get(priv->port);
-		if (!link_info.s.link_up)
-			netif_carrier_off(dev);
-		priv->poll = link_poll;
-		link_poll(dev);
+	err = phylink_of_phy_connect(priv->phylink, priv->of_node, 0);
+	if (err) {
+		netdev_err(dev, "Could not attach PHY (%d)\n", err);
+		return err;
 	}
 
+	priv->poll_used = false;
+	priv->poll = link_poll;
+
+	phylink_start(priv->phylink);
+
 	return 0;
 }
 

@@ -504,13 +498,7 @@ void cvm_oct_link_poll(struct net_device *dev)
 	else
 		priv->link_info = link_info.u64;
 
-	if (link_info.s.link_up) {
-		if (!netif_carrier_ok(dev))
-			netif_carrier_on(dev);
-	} else if (netif_carrier_ok(dev)) {
-		netif_carrier_off(dev);
-	}
-	cvm_oct_note_carrier(priv, link_info);
+	phylink_mac_change(priv->phylink, link_info.s.link_up);
 }
 
 static int cvm_oct_xaui_open(struct net_device *dev)

@@ -797,7 +785,6 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		}
 	}
 

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 800

Quoted 1 line

drivers/staging/octeon/ethernet.c +0 -1

-	num_interfaces = cvmx_helper_get_number_of_interfaces();

This change is correct, but I wish it were in a different commit. It is unrelated.

Quoted 7 lines

drivers/staging/octeon/ethernet.c +0 -0

 	for (interface = 0; interface < num_interfaces; interface++) {
 		int num_ports, port_index;
 		const struct net_device_ops *ops;

@@ -889,18 +876,15 @@ static int cvm_oct_probe(struct platform_device *pdev)
 			dev->min_mtu = VLAN_ETH_ZLEN - mtu_overhead;
 			dev->max_mtu = OCTEON_MAX_MTU - mtu_overhead;
 

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 881

Quoted 10 lines

drivers/staging/octeon/ethernet.c +3 -7

-			if (priv->of_node && of_phy_is_fixed_link(priv->of_node)) {
-				if (of_phy_register_fixed_link(priv->of_node)) {
-					netdev_err(dev, "Failed to register fixed link for interface %d, port %d\n",
-						   interface, priv->port);
-					free_netdev(dev);
-					continue;
-				}
+			if (cvm_oct_phy_setup_device(dev)) {
+				free_netdev(dev);
+				continue;

This was in the original code, but I don't think this is correct. If there is an error then we should return instead of continuing. This is especially true because cvm_oct_phy_setup_device() returns -EPROBE_DEFER so in theory errors are a matter of course and we have a way to recover from them.

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 887

Quoted 6 lines

drivers/staging/octeon/ethernet.c +1 -0

 			}
 
 			if (register_netdev(dev) < 0) {
 				pr_err("Failed to register ethernet device for interface %d, port %d\n",
 				       interface, priv->port);
+				phylink_destroy(priv->phylink);

Could you create a wrapper around this so that it's more clear that this matches cvm_oct_phy_setup_device()?

void cvm_oct_phy_free_device(struct net_device *dev)

{
	struct octeon_ethernet *priv = netdev_priv(dev);

	phylink_destroy(priv->phylink);
}

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 925

Quoted 10 lines

drivers/staging/octeon/ethernet.c +1 -2

 				free_netdev(dev);
 			} else {
 				cvm_oct_device[priv->port] = dev;

@@ -938,8 +922,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 			struct net_device *dev = cvm_oct_device[port];
 			struct octeon_ethernet *priv = netdev_priv(dev);
 
-			cancel_delayed_work_sync(&priv->port_periodic_work);
-
+			phylink_destroy(priv->phylink);

I don't understand how this works. This call to:

	cancel_delayed_work_sync(&priv->port_periodic_work);

moved to cvm_oct_common_uninit(). But then why are we adding a phylink_destroy(priv->phylink); when the phylink_destroy() is also done in cvm_oct_common_uninit().

drivers/staging/octeon/ethernet.c static int cvm_oct_remove(struct platform_device *pdev) · line 956

Quoted 11 lines

drivers/staging/octeon/ethernet.c +1 -2

 			unregister_netdev(dev);
 			free_netdev(dev);
 			cvm_oct_device[port] = NULL;

@@ -969,9 +952,8 @@ static int cvm_oct_remove(struct platform_device *pdev)
 			struct net_device *dev = cvm_oct_device[port];
 			struct octeon_ethernet *priv = netdev_priv(dev);
 
-			cancel_delayed_work_sync(&priv->port_periodic_work);
-
 			cvm_oct_tx_shutdown_dev(dev);
+			phylink_destroy(priv->phylink);

Same.

drivers/staging/octeon/ethernet.c static int cvm_oct_probe(struct platform_device *pdev) · line 928

Quoted 3 lines

drivers/staging/octeon/ethernet.c +0 -0

 			unregister_netdev(dev);
 			free_netdev(dev);
 			cvm_oct_device[port] = NULL;

regards,
dan carpenter

Quoted 11 lines

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 9 lines

drivers/staging/octeon/ethernet-rgmii.c +0 -1

index 0c4fac31540a..8c6eb0b87254 100644
@@ -115,17 +115,8 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 
 	cvm_oct_check_preamble_errors(dev);
 
-	if (likely(!status_change))

                   ^

Negated.

On purpose.

Quoted 5 lines

Quoted 5 lines

drivers/staging/octeon/ethernet-rgmii.c +0 -5

-		return;
-
-	/* Tell core. */
-	if (link_info.s.link_up) {
-		if (!netif_carrier_ok(dev))

Quoted 8 lines

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 6 lines

drivers/staging/octeon/ethernet-rgmii.c +1 -5

-			netif_carrier_on(dev);
-	} else if (netif_carrier_ok(dev)) {
-		netif_carrier_off(dev);
-	}
-	cvm_oct_note_carrier(priv, link_info);
+	if (likely(status_change))

Originally a status_change was unlikely but now it is likely.

Yes, but originally it returned after condition and now it executes phylink_mac_change. This is just to emulate current bahaviour. Later mac interrupts should be used to drive link change.

Quoted 28 lines · Ladislav Michl

Quoted 11 lines

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 9 lines

drivers/staging/octeon/ethernet-rgmii.c +0 -1

index 0c4fac31540a..8c6eb0b87254 100644
@@ -115,17 +115,8 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 
 	cvm_oct_check_preamble_errors(dev);
 
-	if (likely(!status_change))

                   ^

Negated.

On purpose.

Quoted 13 lines

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 118

Quoted 11 lines

drivers/staging/octeon/ethernet-rgmii.c +1 -10

-		return;
-
-	/* Tell core. */
-	if (link_info.s.link_up) {
-		if (!netif_carrier_ok(dev))
-			netif_carrier_on(dev);
-	} else if (netif_carrier_ok(dev)) {
-		netif_carrier_off(dev);
-	}
-	cvm_oct_note_carrier(priv, link_info);
+	if (likely(status_change))

Originally a status_change was unlikely but now it is likely.

Quoted 3 lines

Yes, but originally it returned after condition and now it executes phylink_mac_change. This is just to emulate current bahaviour. Later mac interrupts should be used to drive link change.

I don't think you have seen the (minor) issue. Originally it was likely that status_change was NOT set. But now it is likely that is *IS* set.

The purpose of this patches is to provide support for SFP cage to Octeon ethernet driver. This is tested with following DT snippet:

	smi0: mdio@1180000001800 {
		compatible = "cavium,octeon-3860-mdio";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x11800 0x00001800 0x0 0x40>;

		/* QSGMII PHY */
		phy0: ethernet-phy@0 {
			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";

			reg = <0>;
			interrupt-parent = <&gpio>;
			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
			marvell,reg-init =
			  <0xff 24 0 0x2800>, <0xff 23 0 0x2001>, /* errata 3.1.1 - PHY Initialization #1 */
			  <0 29 0 3>, <0 30 0 2>, <0 29 0 0>,	  /* errata 3.1.2 - PHY Initialization #2 */

			  <4 26 0 0x2>, 			  /* prekrizeni RX a TX QSGMII sbernice */
			  <4 0 0x1000 0x1000>, 			  /* Q_ANEG workaround: P4R0B12 = 1 */
			  <3 16 0 0x1117>;			  /* nastavení LED: G=link+act, Y=1Gbit */
		};

	pip: pip@11800a0000000 {
		compatible = "cavium,octeon-3860-pip";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x11800 0xa0000000 0x0 0x2000>;

		/* Interface 0 goes to SFP */
		interface@0 {
			compatible = "cavium,octeon-3860-pip-interface";
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>; /* interface */

			ethernet@0 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <0>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				managed = "in-band-status";
				phy-connection-type = "1000base-x";
				sfp = <&sfp>;
			};
		};

		/* Interface 1 goes to eth1-eth4 and is QSGMII */
		interface@1 {
			compatible = "cavium,octeon-3860-pip-interface";
			#address-cells = <1>;

			#size-cells = <0>;
			reg = <1>; /* interface */

			ethernet@0 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <0>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				phy-handle = <&phy0>;

However testing revealed some glitches:
1. driver previously returned -EPROBE_DEFER when no phy was attached. Phylink stack does not seem to do so, which end up with:

Marvell PHY driver as a module:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL) octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off

Marvell PHY driver built-in:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver

2. It is not possible to call phylink_create from ndo_init callcack as it evetually calls sfp_bus_add_upstream which calls rtnl_lock(). As this lock is already taken, it just deadlocks. Is this an unsupported scenario?

Hi Ladislav

For phylink questions, it is a good idea to Cc: the phylink Maintainer. And for general PHY problems, Cc: the phy Maintainers.

Quoted 12 lines · Ladislav Michl

The purpose of this patches is to provide support for SFP cage to Octeon ethernet driver. This is tested with following DT snippet:

	smi0: mdio@1180000001800 {
		compatible = "cavium,octeon-3860-mdio";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x11800 0x00001800 0x0 0x40>;

		/* QSGMII PHY */
		phy0: ethernet-phy@0 {
			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";

Please don't use a compatible for the specific PHY. In fact, compatibles are only used for things which are not PHYs, like Ethernet switches. phylib reads the ID registers of the PHY and uses them to load the correct PHY driver.

Also, C22 is the default, so you don't need that either.

Quoted 6 lines

			reg = <0>;
			interrupt-parent = <&gpio>;
			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
			marvell,reg-init =
			  <0xff 24 0 0x2800>, <0xff 23 0 0x2001>, /* errata 3.1.1 - PHY Initialization #1 */
			  <0 29 0 3>, <0 30 0 2>, <0 29 0 0>,	  /* errata 3.1.2 - PHY Initialization #2 */

Please add C code to deal with these erratas in the marvell PHY driver.

Quoted 4 lines

			  <4 26 0 0x2>, 			  /* prekrizeni RX a TX QSGMII sbernice */
			  <4 0 0x1000 0x1000>, 			  /* Q_ANEG workaround: P4R0B12 = 1 */
			  <3 16 0 0x1117>;			  /* nastavení LED: G=link+act, Y=1Gbit */
		};

Comments are normally in English. The last one seems to be setting the LED. This is tolerated, but not ideal. It is not clear to me what the other two do.

Quoted 30 lines

	pip: pip@11800a0000000 {
		compatible = "cavium,octeon-3860-pip";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x11800 0xa0000000 0x0 0x2000>;

		/* Interface 0 goes to SFP */
		interface@0 {
			compatible = "cavium,octeon-3860-pip-interface";
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>; /* interface */

			ethernet@0 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <0>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				managed = "in-band-status";
				phy-connection-type = "1000base-x";
				sfp = <&sfp>;
			};
		};

		/* Interface 1 goes to eth1-eth4 and is QSGMII */
		interface@1 {
			compatible = "cavium,octeon-3860-pip-interface";
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <1>; /* interface */

Quoted 5 lines

			ethernet@0 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <0>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				phy-handle = <&phy0>;

If this is a QSGMII link, don't you need phy-mode property?

Quoted 18 lines

However testing revealed some glitches:
1. driver previously returned -EPROBE_DEFER when no phy was attached. Phylink stack does not seem to do so, which end up with:

Marvell PHY driver as a module:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL) octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off

Quoted 8 lines

Marvell PHY driver built-in:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver

This is very odd. But it could be a side effect of the compatible. Please try with it removed.

Quoted 4 lines

2. It is not possible to call phylink_create from ndo_init callcack as it evetually calls sfp_bus_add_upstream which calls rtnl_lock(). As this lock is already taken, it just deadlocks. Is this an unsupported scenario?

You normally call phylink_create() in _probe().

Andrew

Quoted 27 lines

Quoted 26 lines

However testing revealed some glitches:
1. driver previously returned -EPROBE_DEFER when no phy was attached. Phylink stack does not seem to do so, which end up with:

Marvell PHY driver as a module:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL) octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off

Marvell PHY driver built-in:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver

Quoted 2 lines

This is very odd. But it could be a side effect of the compatible. Please try with it removed.

That does not make any difference.

Quoted 27 lines

Quoted 27 lines

Quoted 26 lines

However testing revealed some glitches:
1. driver previously returned -EPROBE_DEFER when no phy was attached. Phylink stack does not seem to do so, which end up with:

Marvell PHY driver as a module:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth2: PHY [8001180000001800:01] driver [Generic PHY] (irq=POLL) octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: switched to inband/sgmii link mode octeon_ethernet 11800a0000000.pip eth0: PHY [i2c:sfp:16] driver [Marvell 88E1111] (irq=POLL) octeon_ethernet 11800a0000000.pip eth3: PHY [8001180000001800:02] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth3: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth4: PHY [8001180000001800:03] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth4: configuring for phy/sgmii link mode octeon_ethernet 11800a0000000.pip eth1: Link is Up - 100Mbps/Full - flow control off

Marvell PHY driver built-in:
octeon_ethernet 11800a0000000.pip eth0: configuring for inband/1000base-x link mode octeon_ethernet 11800a0000000.pip eth1: PHY [8001180000001800:00] driver [Marvell 88E1340S] (irq=25) octeon_ethernet 11800a0000000.pip eth1: configuring for phy/sgmii link mode
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver
Error: Driver 'Marvell 88E1101' is already registered, aborting...
libphy: Marvell 88E1101: Error -16 in registering driver

Quoted 4 lines

Quoted 2 lines

This is very odd. But it could be a side effect of the compatible. Please try with it removed.

That does not make any difference.

Then i have no idea. I would suggest you add a WARN_ON() in phy_driver_register() so we get a backtrace. That might give a clue why it is getting registered multiple times.