[PATCH 0/3] staging: octeon: Convert to use phylink

[PATCH 0/3] staging: octeon: Convert to use phylink

[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";
			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 */
		};
		phy1: ethernet-phy@1 {
			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";
			reg = <1>;
			interrupt-parent = <&gpio>;
			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
			marvell,reg-init =
			  <4 0 0x1000 0x1000>,			  /* Q_ANEG workaround: P4R0B12 = 1 */
			  <3 16 0 0x1117>;			  /* nastavení LED: G=link+act, Y=1Gbit */
		};
		phy2: ethernet-phy@2 {
			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";
			reg = <2>;
			interrupt-parent = <&gpio>;
			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
			marvell,reg-init =
			  <4 0 0x1000 0x1000>,			  /* Q_ANEG workaround: P4R0B12 = 1 */
			  <3 16 0 0x1117>;			  /* nastavení LED: G=link+act, Y=1Gbit */
		};
		phy3: ethernet-phy@3 {
			compatible = "marvell,88e154", "ethernet-phy-ieee802.3-c22";
			reg = <3>;
			interrupt-parent = <&gpio>;
			interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
			marvell,reg-init =
			  <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>;
			};
			ethernet@1 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <1>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				phy-handle = <&phy1>;
			};
			ethernet@2 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <2>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				phy-handle = <&phy2>;
			};
			ethernet@3 {
				compatible = "cavium,octeon-3860-pip-port";
				reg = <3>; /* Port */
				local-mac-address = [ 00 00 00 00 00 00 ];
				phy-handle = <&phy3>;
			};
		};
	};

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
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 eth2: PHY [8001180000001800:01] driver [Marvell 88E1340S] (irq=25)
octeon_ethernet 11800a0000000.pip eth2: configuring for phy/sgmii link mode
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

How it is supposed to deal with that?

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?

Comments welcome and appreciated,
	ladis

[PATCH 1/3] staging: octeon: don't panic

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

It is unfortunate to halt kernel just because no network
interfaces was registered.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/staging/octeon/ethernet-rx.c | 33 +++++++++++++++++++++++-----
 drivers/staging/octeon/ethernet-rx.h |  2 +-
 drivers/staging/octeon/ethernet-tx.c | 16 ++++++++------
 drivers/staging/octeon/ethernet-tx.h |  2 +-
 drivers/staging/octeon/ethernet.c    | 31 ++++++++++++++++++++++----
 5 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 965330eec80a..24ae4b3bd58b 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -448,7 +448,7 @@ void cvm_oct_poll_controller(struct net_device *dev)
 }
 #endif
 
-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)
 		}
 	}
 
-	if (!dev_for_napi)
-		panic("No net_devices were allocated.");
+	if (!dev_for_napi) {
+		pr_err("No net_devices were allocated.");
+		return -ENODEV;
+	}
+
 
 	for (i = 0; i < ARRAY_SIZE(oct_rx_group); i++) {
 		int ret;
@@ -479,10 +482,28 @@ void cvm_oct_rx_initialize(void)
 		/* Register an IRQ handler to receive POW interrupts */
 		ret = request_irq(oct_rx_group[i].irq, cvm_oct_do_interrupt, 0,
 				  "Ethernet", &oct_rx_group[i].napi);
-		if (ret)
-			panic("Could not acquire Ethernet IRQ %d\n",
+		if (ret) {
+			int j;
+
+			pr_err("Could not acquire Ethernet IRQ %d\n",
 			      oct_rx_group[i].irq);
 
+			for (j = 0; j < i; j++) {
+				if (!(pow_receive_groups & BIT(j)))
+					continue;
+
+				cvmx_write_csr(OCTEON_IS_MODEL(OCTEON_CN68XX) ?
+						CVMX_SSO_WQ_INT_THRX(j) :
+						CVMX_POW_WQ_INT_THRX(j),
+						0);
+				free_irq(oct_rx_group[j].irq, cvm_oct_device);
+				netif_napi_del(&oct_rx_group[j].napi);
+			}
+
+			return ret;
+		}
+
+
 		disable_irq_nosync(oct_rx_group[i].irq);
 
 		/* Enable POW interrupt when our port has at least one packet */
@@ -518,6 +539,8 @@ void cvm_oct_rx_initialize(void)
 		napi_schedule(&oct_rx_group[i].napi);
 	}
 	atomic_inc(&oct_rx_ready);
+
+	return 0;
 }
 
 void cvm_oct_rx_shutdown(void)
diff --git a/drivers/staging/octeon/ethernet-rx.h b/drivers/staging/octeon/ethernet-rx.h
index ff6482fa20d6..9a98f662d813 100644
--- a/drivers/staging/octeon/ethernet-rx.h
+++ b/drivers/staging/octeon/ethernet-rx.h
@@ -6,7 +6,7 @@
  */
 
 void cvm_oct_poll_controller(struct net_device *dev);
-void cvm_oct_rx_initialize(void);
+int cvm_oct_rx_initialize(void);
 void cvm_oct_rx_shutdown(void);
 
 static inline void cvm_oct_rx_refill_pool(int fill_threshold)
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index a36e36701c74..c02b023ac8c0 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -694,19 +694,21 @@ static irqreturn_t cvm_oct_tx_cleanup_watchdog(int cpl, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-void cvm_oct_tx_initialize(void)
+int cvm_oct_tx_initialize(void)
 {
-	int i;
+	int ret;
 
 	/* Disable the interrupt.  */
 	cvmx_write_csr(CVMX_CIU_TIMX(1), 0);
 	/* Register an IRQ handler to receive CIU_TIMX(1) interrupts */
-	i = request_irq(OCTEON_IRQ_TIMER1,
-			cvm_oct_tx_cleanup_watchdog, 0,
-			"Ethernet", cvm_oct_device);
+	ret = request_irq(OCTEON_IRQ_TIMER1,
+			  cvm_oct_tx_cleanup_watchdog, 0,
+			  "Ethernet", cvm_oct_device);
+
+	if (ret)
+		pr_err("Could not acquire Ethernet IRQ %d\n", OCTEON_IRQ_TIMER1);
 
-	if (i)
-		panic("Could not acquire Ethernet IRQ %d\n", OCTEON_IRQ_TIMER1);
+	return ret;
 }
 
 void cvm_oct_tx_shutdown(void)
diff --git a/drivers/staging/octeon/ethernet-tx.h b/drivers/staging/octeon/ethernet-tx.h
index 6c524668f65a..b4e328a4b499 100644
--- a/drivers/staging/octeon/ethernet-tx.h
+++ b/drivers/staging/octeon/ethernet-tx.h
@@ -9,6 +9,6 @@ netdev_tx_t cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);
 netdev_tx_t cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev);
 int cvm_oct_transmit_qos(struct net_device *dev, void *work_queue_entry,
 			 int do_free, int qos);
-void cvm_oct_tx_initialize(void);
+int cvm_oct_tx_initialize(void);
 void cvm_oct_tx_shutdown(void);
 void cvm_oct_tx_shutdown_dev(struct net_device *dev);
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index f662739137b5..949ef51bf896 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -672,6 +672,8 @@ static void cvm_set_rgmii_delay(struct octeon_ethernet *priv, int iface,
 
 static int cvm_oct_probe(struct platform_device *pdev)
 {
+	int ret;
+	int port;
 	int num_interfaces;
 	int interface;
 	int fau = FAU_NUM_PACKET_BUFFERS_TO_FREE;
@@ -705,7 +707,6 @@ static int cvm_oct_probe(struct platform_device *pdev)
 	num_interfaces = cvmx_helper_get_number_of_interfaces();
 	for (interface = 0; interface < num_interfaces; interface++) {
 		int num_ports = cvmx_helper_ports_on_interface(interface);
-		int port;
 
 		for (port = cvmx_helper_get_ipd_port(interface, 0);
 		     port < cvmx_helper_get_ipd_port(interface, num_ports);
@@ -801,7 +802,6 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		cvmx_helper_interface_mode_t imode =
 		    cvmx_helper_interface_get_mode(interface);
 		int num_ports = cvmx_helper_ports_on_interface(interface);
-		int port;
 		int port_index;
 
 		for (port_index = 0,
@@ -911,8 +911,15 @@ static int cvm_oct_probe(struct platform_device *pdev)
 		}
 	}
 
-	cvm_oct_tx_initialize();
-	cvm_oct_rx_initialize();
+	ret = cvm_oct_tx_initialize();
+	if (ret)
+		goto err;
+
+	ret = cvm_oct_rx_initialize();
+	if (ret) {
+		cvm_oct_tx_shutdown();
+		goto err;
+	}
 
 	/*
 	 * 150 uS: about 10 1500-byte packets at 1GE.
@@ -922,6 +929,22 @@ static int cvm_oct_probe(struct platform_device *pdev)
 	schedule_delayed_work(&cvm_oct_rx_refill_work, HZ);
 
 	return 0;
+
+err:
+	for (port = 0; port < TOTAL_NUMBER_OF_PORTS; port++) {
+		if (cvm_oct_device[port]) {
+			struct net_device *dev = cvm_oct_device[port];
+			struct octeon_ethernet *priv = netdev_priv(dev);
+
+			cancel_delayed_work_sync(&priv->port_periodic_work);
+
+			unregister_netdev(dev);
+			free_netdev(dev);
+			cvm_oct_device[port] = NULL;
+		}
+	}
+
+	return ret;
 }
 
 static int cvm_oct_remove(struct platform_device *pdev)
-- 
2.32.0

[PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Etherdev is allocated and then tested for valid interface,
then it is immediately freed after port is found unsupported.
Move that decision out of the port loop.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 drivers/staging/octeon/ethernet.c | 114 +++++++++++++++---------------
 1 file changed, 58 insertions(+), 56 deletions(-)

diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index 949ef51bf896..466d43a71d34 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -799,18 +799,63 @@ static int cvm_oct_probe(struct platform_device *pdev)
 
 	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) {
+		case CVMX_HELPER_INTERFACE_MODE_NPI:
+			ops = &cvm_oct_npi_netdev_ops;
+			name = "npi%d";
+			break;
+
+		case CVMX_HELPER_INTERFACE_MODE_XAUI:
+			ops = &cvm_oct_xaui_netdev_ops;
+			name = "xaui%d";
+			break;
+
+		case CVMX_HELPER_INTERFACE_MODE_LOOP:
+			ops = &cvm_oct_npi_netdev_ops;
+			name = "loop%d";
+			break;
+
+		case CVMX_HELPER_INTERFACE_MODE_SGMII:
+			ops = &cvm_oct_sgmii_netdev_ops;
+			name = "eth%d";
+			phy_mode = PHY_INTERFACE_MODE_SGMII;
+			break;
+
+		case CVMX_HELPER_INTERFACE_MODE_SPI:
+			ops = &cvm_oct_spi_netdev_ops;
+			name = "spi%d";
+			break;
+
+		case CVMX_HELPER_INTERFACE_MODE_GMII:
+			ops = &cvm_oct_rgmii_netdev_ops;
+			name = "eth%d";
+			phy_mode = PHY_INTERFACE_MODE_GMII;
+			break;
 
+		case CVMX_HELPER_INTERFACE_MODE_RGMII:
+			ops = &cvm_oct_rgmii_netdev_ops;
+			name = "eth%d";
+			break;
+
+		default:
+			continue;
+		}
+
+		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++) {
 			struct octeon_ethernet *priv;
 			struct net_device *dev =
-			    alloc_etherdev(sizeof(struct octeon_ethernet));
+				alloc_etherdev(sizeof(struct octeon_ethernet));
 			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;
+			if (imode == CVMX_HELPER_INTERFACE_MODE_RGMII)
+				cvm_set_rgmii_delay(priv, interface,
+						    port_index);
+			dev->netdev_ops = ops;
+			strscpy(dev->name, name, sizeof(dev->name));
 			for (qos = 0; qos < 16; qos++)
 				skb_queue_head_init(&priv->tx_free_list[qos]);
 			for (qos = 0; qos < cvmx_pko_get_num_queues(port);
@@ -839,64 +889,16 @@ 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;
 
-			switch (priv->imode) {
-			/* These types don't support ports to IPD/PKO */
-			case CVMX_HELPER_INTERFACE_MODE_DISABLED:
-			case CVMX_HELPER_INTERFACE_MODE_PCIE:
-			case CVMX_HELPER_INTERFACE_MODE_PICMG:
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_NPI:
-				dev->netdev_ops = &cvm_oct_npi_netdev_ops;
-				strscpy(dev->name, "npi%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_XAUI:
-				dev->netdev_ops = &cvm_oct_xaui_netdev_ops;
-				strscpy(dev->name, "xaui%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_LOOP:
-				dev->netdev_ops = &cvm_oct_npi_netdev_ops;
-				strscpy(dev->name, "loop%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_SGMII:
-				priv->phy_mode = PHY_INTERFACE_MODE_SGMII;
-				dev->netdev_ops = &cvm_oct_sgmii_netdev_ops;
-				strscpy(dev->name, "eth%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_SPI:
-				dev->netdev_ops = &cvm_oct_spi_netdev_ops;
-				strscpy(dev->name, "spi%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_GMII:
-				priv->phy_mode = PHY_INTERFACE_MODE_GMII;
-				dev->netdev_ops = &cvm_oct_rgmii_netdev_ops;
-				strscpy(dev->name, "eth%d", sizeof(dev->name));
-				break;
-
-			case CVMX_HELPER_INTERFACE_MODE_RGMII:
-				dev->netdev_ops = &cvm_oct_rgmii_netdev_ops;
-				strscpy(dev->name, "eth%d", sizeof(dev->name));
-				cvm_set_rgmii_delay(priv, interface,
-						    port_index);
-				break;
-			}
-
 			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);
-					dev->netdev_ops = NULL;
+					free_netdev(dev);
+					continue;
 				}
 			}
 
-			if (!dev->netdev_ops) {
-				free_netdev(dev);
-			} else if (register_netdev(dev) < 0) {
+			if (register_netdev(dev) < 0) {
 				pr_err("Failed to register ethernet device for interface %d, port %d\n",
 				       interface, priv->port);
 				free_netdev(dev);
-- 
2.32.0

[RFC 3/3] staging: octeon: convert to use phylink

[Ladislav Michl]

From: Ladislav Michl <ladis@linux-mips.org>

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 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(-)

diff --git a/drivers/staging/octeon/Kconfig b/drivers/staging/octeon/Kconfig
index 5319909eb2f6..fda90025710d 100644
--- a/drivers/staging/octeon/Kconfig
+++ b/drivers/staging/octeon/Kconfig
@@ -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
diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
index b3049108edc4..a14fb4dbb2fd 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -9,7 +9,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/ratelimit.h>
-#include <linux/of_mdio.h>
+#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",
-				      priv->port, priv->queue);
-	} else {
-		pr_notice_ratelimited("%s: Link down\n",
-				      netdev_name(priv->netdev));
-	}
 }
 
-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);
 	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;
+	priv->link_info		= link_info.u64;
+
+	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;
 }
diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
index 0c4fac31540a..8c6eb0b87254 100644
--- a/drivers/staging/octeon/ethernet-rgmii.c
+++ b/drivers/staging/octeon/ethernet-rgmii.c
@@ -115,17 +115,8 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 
 	cvm_oct_check_preamble_errors(dev);
 
-	if (likely(!status_change))
-		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))
+		phylink_mac_change(priv->phylink, link_info.s.link_up);
 }
 
 int cvm_oct_rgmii_open(struct net_device *dev)
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index 466d43a71d34..21892f805245 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -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);
 
-	if (priv->poll)
+	if (priv->poll_used && priv->poll)
 		priv->poll(cvm_oct_device[priv->port]);
 
 	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);
+	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;
 	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)
 		}
 	}
 
-	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;
@@ -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;
 
-			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;
 			}
 
 			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);
 				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);
 			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);
 			unregister_netdev(dev);
 			free_netdev(dev);
 			cvm_oct_device[port] = NULL;
diff --git a/drivers/staging/octeon/octeon-ethernet.h b/drivers/staging/octeon/octeon-ethernet.h
index a6140705706f..fc9bc0974a2a 100644
--- a/drivers/staging/octeon/octeon-ethernet.h
+++ b/drivers/staging/octeon/octeon-ethernet.h
@@ -12,7 +12,7 @@
 #define OCTEON_ETHERNET_H
 
 #include <linux/of.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 
 #ifdef CONFIG_CAVIUM_OCTEON_SOC
 
@@ -62,6 +62,8 @@ struct octeon_ethernet {
 	int imode;
 	/* PHY mode */
 	phy_interface_t phy_mode;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 	/* List of outstanding tx buffers per queue */
 	struct sk_buff_head tx_free_list[16];
 	unsigned int last_speed;
@@ -69,6 +71,7 @@ struct octeon_ethernet {
 	/* Last negotiated link state */
 	u64 link_info;
 	/* Called periodically to check link status */
+	bool poll_used;
 	void (*poll)(struct net_device *dev);
 	struct delayed_work	port_periodic_work;
 	struct device_node	*of_node;
@@ -86,12 +89,9 @@ void cvm_oct_spi_uninit(struct net_device *dev);
 
 int cvm_oct_common_init(struct net_device *dev);
 void cvm_oct_common_uninit(struct net_device *dev);
-void cvm_oct_adjust_link(struct net_device *dev);
 int cvm_oct_common_stop(struct net_device *dev);
 int cvm_oct_common_open(struct net_device *dev,
 			void (*link_poll)(struct net_device *));
-void cvm_oct_note_carrier(struct octeon_ethernet *priv,
-			  union cvmx_helper_link_info li);
 void cvm_oct_link_poll(struct net_device *dev);
 
 extern int always_use_pow;
-- 
2.32.0

Re: [RFC 3/3] staging: octeon: convert to use phylink

[Dan Carpenter]

On Thu, Apr 13, 2023 at 04:15:01PM +0200, Ladislav Michl wrote:
> From: Ladislav Michl <ladis@linux-mips.org>
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

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.

> ---
>  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(-)
> 
> diff --git a/drivers/staging/octeon/Kconfig b/drivers/staging/octeon/Kconfig
> index 5319909eb2f6..fda90025710d 100644
> --- a/drivers/staging/octeon/Kconfig
> +++ b/drivers/staging/octeon/Kconfig
> @@ -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
> diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> index b3049108edc4..a14fb4dbb2fd 100644
> --- a/drivers/staging/octeon/ethernet-mdio.c
> +++ b/drivers/staging/octeon/ethernet-mdio.c
> @@ -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?

> +#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",
> -				      priv->port, priv->queue);
> -	} else {
> -		pr_notice_ratelimited("%s: Link down\n",
> -				      netdev_name(priv->netdev));
> -	}
>  }

Delete this dummy function.

>  
> -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);
>  	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 = {};

> +	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...

> +
> +	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;
>  }
> diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> index 0c4fac31540a..8c6eb0b87254 100644
> --- a/drivers/staging/octeon/ethernet-rgmii.c
> +++ b/drivers/staging/octeon/ethernet-rgmii.c
> @@ -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.

> -		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.


> +		phylink_mac_change(priv->phylink, link_info.s.link_up);
>  }
>  
>  int cvm_oct_rgmii_open(struct net_device *dev)
> diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
> index 466d43a71d34..21892f805245 100644
> --- a/drivers/staging/octeon/ethernet.c
> +++ b/drivers/staging/octeon/ethernet.c
> @@ -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);
>  
> -	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?

>  
>  	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);
> +	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;

>  	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)
>  		}
>  	}
>  
> -	num_interfaces = cvmx_helper_get_number_of_interfaces();

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

>  	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;
>  
> -			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.

>  			}
>  
>  			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);
}

>  				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().

>  			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.

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

regards,
dan carpenter

Re: [PATCH 0/3] staging: octeon: Convert to use phylink

[Andrew Lunn]

Hi Ladislav

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

On Thu, Apr 13, 2023 at 04:11:07PM +0200, Ladislav Michl wrote:
> 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.

> 			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.

> 			  <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.

> 	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>;

If this is a QSGMII link, don't you need phy-mode property?
 
> 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

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

> 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

Re: [PATCH 1/3] staging: octeon: don't panic

[Andrew Lunn]

> -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)
>  		}
>  	}
>  
> -	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

Re: [RFC 3/3] staging: octeon: convert to use phylink

[Ladislav Michl]

Hi Dan,

On Thu, Apr 13, 2023 at 06:35:55PM +0300, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 04:15:01PM +0200, Ladislav Michl wrote:
> > From: Ladislav Michl <ladis@linux-mips.org>
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> 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.

This is just RFC, I expect much more versions until it is eventually
accepted and this patch might be posibly splitted into more patches.

I'm glad it didn't stop you from providing feedback :)

> > ---
> >  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(-)
> > 
> > diff --git a/drivers/staging/octeon/Kconfig b/drivers/staging/octeon/Kconfig
> > index 5319909eb2f6..fda90025710d 100644
> > --- a/drivers/staging/octeon/Kconfig
> > +++ b/drivers/staging/octeon/Kconfig
> > @@ -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
> > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> > index b3049108edc4..a14fb4dbb2fd 100644
> > --- a/drivers/staging/octeon/ethernet-mdio.c
> > +++ b/drivers/staging/octeon/ethernet-mdio.c
> > @@ -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?

No. We are using phylink now, so it is not needed. This file should
eventually vanish, small part being merged into ethernet.c and the
into appropriate mac files (ethernet-sgmii.c, ethernet-rgmii.c...).

> > +#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",
> > -				      priv->port, priv->queue);
> > -	} else {
> > -		pr_notice_ratelimited("%s: Link down\n",
> > -				      netdev_name(priv->netdev));
> > -	}
> >  }
> 
> Delete this dummy function.

This is not possible. Core uses this callback, but I can at least add
comment stating there is nothing do to here.

Later there should be mac initialization, but currently it is called from
cvmx-helper in arch/mips/cavium-octeon/executive/
 
> > -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);
> >  	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:

ok.

> 	union cvmx_helper_link_info link_info = {};
> 
> > +	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...

I used the way common in Octeon drivers, but agree here. Will drop this.

> > +
> > +	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;
> >  }
> > diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> > index 0c4fac31540a..8c6eb0b87254 100644
> > --- a/drivers/staging/octeon/ethernet-rgmii.c
> > +++ b/drivers/staging/octeon/ethernet-rgmii.c
> > @@ -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.

> > -		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.

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.

> > +		phylink_mac_change(priv->phylink, link_info.s.link_up);
> >  }
> >  
> >  int cvm_oct_rgmii_open(struct net_device *dev)
> > diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
> > index 466d43a71d34..21892f805245 100644
> > --- a/drivers/staging/octeon/ethernet.c
> > +++ b/drivers/staging/octeon/ethernet.c
> > @@ -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);
> >  
> > -	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?

Yes. I was just trying to make changes minimal. I'd preffer to clean this
up in the follow up patches.

> >  	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);
> > +	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;

Ok.

> >  	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)
> >  		}
> >  	}
> >  
> > -	num_interfaces = cvmx_helper_get_number_of_interfaces();
> 
> This change is correct, but I wish it were in a different commit.  It
> is unrelated.

Ok. I'll move it to separate preparation commit.

> >  	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;
> >  
> > -			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.

You are right, but this is very hard to achieve. I'd rather convert driver
to phylink first and then fix all this as doing it in different order is more
painfull.

Please note, there are many things in this driver which we would consider
incorrect. Most code is two decades old and got only cosmetic changes.

> >  			}
> >  
> >  			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);
> }

Ok.

> >  				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().

Ooops... Sorry about that. I forgot it here from an attempt to put phylink
creation/destruction into ndo_init and ndo_uninit, but that does not work;
see cover letter.

> >  			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.
> 
> >  			unregister_netdev(dev);
> >  			free_netdev(dev);
> >  			cvm_oct_device[port] = NULL;
> 
> regards,
> dan carpenter

Thank you,
	ladis

Re: [RFC 3/3] staging: octeon: convert to use phylink

[Dan Carpenter]

On Thu, Apr 13, 2023 at 06:04:47PM +0200, Ladislav Michl wrote:
> > > diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> > > index 0c4fac31540a..8c6eb0b87254 100644
> > > --- a/drivers/staging/octeon/ethernet-rgmii.c
> > > +++ b/drivers/staging/octeon/ethernet-rgmii.c
> > > @@ -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.
> 
> > > -		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.
> 
> 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.

regards,
dan carpenter

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Andrew Lunn]

>  	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) {
> +		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.

> +		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++) {
>  			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.


>  			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

Re: [PATCH 1/3] staging: octeon: don't panic

[Ladislav Michl]

Hello Andrew,

On Thu, Apr 13, 2023 at 05:57:00PM +0200, Andrew Lunn wrote:
> > -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)
> >  		}
> >  	}
> >  
> > -	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.

Well, it would then need few more preparation commits to use proper
logging.

> checkpatch is probably warning you about this.
> 
> Once you have a registered netdev, you should then use netdev_err(),
> netdev_dbg() etc.

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).

Perhaps use per netdev probe function first?

> 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)".

On the side note, this (panic) cannot happen in current code as
it is using DT as a guidance only, but interfaces are hardcoded.
Later on, when DT is used to provide links, it can fail and then
kernel would panic.

>       Andrew

Thank you,
	ladis

Re: [RFC 3/3] staging: octeon: convert to use phylink

[Ladislav Michl]

On Thu, Apr 13, 2023 at 07:10:47PM +0300, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 06:04:47PM +0200, Ladislav Michl wrote:
> > > > diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> > > > index 0c4fac31540a..8c6eb0b87254 100644
> > > > --- a/drivers/staging/octeon/ethernet-rgmii.c
> > > > +++ b/drivers/staging/octeon/ethernet-rgmii.c
> > > > @@ -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.
> > 
> > > > -		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.
> > 
> > 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.

Ahh, will fix that. Thank you,
	ladis

Re: [PATCH 1/3] staging: octeon: don't panic

[Andrew Lunn]

> 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).

You always have pdev->dev, which you can use until you have a
registered netdev. That is a common pattern.

	   Andrew

Re: [PATCH 0/3] staging: octeon: Convert to use phylink

[Ladislav Michl]

Hi Andrew,

On Thu, Apr 13, 2023 at 05:45:13PM +0200, Andrew Lunn wrote:
> Hi Ladislav
> 
> For phylink questions, it is a good idea to Cc: the phylink
> Maintainer. And for general PHY problems, Cc: the phy Maintainers.
> 
> On Thu, Apr 13, 2023 at 04:11:07PM +0200, Ladislav Michl wrote:
> > 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.

Thanks, it works equally well with compatible removed.

> > 			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.

I need to dig into 4.9 ventor (and custom) tree for them. Will do later.

> > 			  <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.

I'm sorry for that. I took DT from local tree. It is not meant for upstream.

> > 	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>;
> 
> If this is a QSGMII link, don't you need phy-mode property?

I would normally need that, but the way how this driver works makes it
optional.

Interfaces and their types are hardwired as well as various register address.
In the ideal world they should come from DT instead of from various
OCTEON_IS_MODEL(OCTEON_CNXXXX) ifdefery.

> > 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
> 
> 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.

> > 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().

Ok, thank you.

>     Andrew

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl]

Hi Andrew,

On Thu, Apr 13, 2023 at 06:12:28PM +0200, Andrew Lunn wrote:
> >  	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) {
> > +		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.

That would break all userspace (which is often running vendor's kernel).
But since driver is in staging and noone cares about vendor's kernel
I guess it is okay...

> > +		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++) {
> >  			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.

Sorry, I overlooked this.

> >  			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.

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?

> 	 Andrew

Thank you,
	ladis

Re: [PATCH 0/3] staging: octeon: Convert to use phylink

[Andrew Lunn]

> > > 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
> > 
> > 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.

	 Andrew

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Andrew Lunn]

> 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

Re: [PATCH 0/3] staging: octeon: Convert to use phylink

[Ladislav Michl]

On Thu, Apr 13, 2023 at 07:12:29PM +0200, Andrew Lunn wrote:
> > > > 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
> > > 
> > > 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.

And it indeed did. There was kernel/drivers/net/phy/marvell.ko
left in /lib/modules. Clearly my mistake, sorry for the noise and
thank you for help.

So now only that -EPROBE_DEFER handling in module case is waiting for
debugging.

	ladis

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl]

On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote:
> > 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.

I agree. However it is a way to move it out as once phylink_mac_ops
for each mac gets implemented, most code from
arch/mips/cavium-octeon/executive could then be moved into respective
phylink_mac_op, so driver become self contained.

> 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?

Ok. I will continue to add cleanup patches before phylink support and
we'll see how far we can get. That oddity has pretty simple reasoning:
mainline kernel should be useable instead of vendor's solution (which
does dirty SFP tricks from userpace and also supports AGL interface
which is missing in staging driver). Without this, it will end as a
spare time activity with a low priority. See this thread for context:
https://lore.kernel.org/linux-mips/Y6rsbaT0l5cNBGbu@lenoch/

> But staging is not my usual domain.

Network drivers are not my usual domain, but I'll try to deal
with that :)

> 	 Andrew

Thanks for the patience,
	ladis

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl]

+David Daney, Steven J. Hill

On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote:
> > 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.

A side question here: Once upon the time there was an "Cavium OCTEON-III
network driver" patchset floating around, reached v9, at least that's
the last one I was able to find [1]. Prerequisites were intended to go
via linux-mips tree [2].

I was unable to find any further traces and this driver is not even
in Cavium's 5.4 vendor tree. What happened with that? It is for different
hardware, but some design decisions might be interesting here as well.

> 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

[1] https://www.spinics.net/lists/netdev/msg498700.html
[2] https://www.spinics.net/lists/netdev/msg498696.html

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Dan Carpenter]

On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote:
> > 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?

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

regards,
dan carpenter

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Ladislav Michl]

On Mon, Apr 17, 2023 at 11:37:30AM +0300, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 07:20:08PM +0200, Andrew Lunn wrote:
> > > 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?
> 
> We already have that rule.  But I don't know anything about phy vs
> phylink...

Let me elaborate here a bit then.

Current Octeon driver comes from Cavium's vendor kernel tree. Cavium
started this about two decades ago based on their own ideas and tries
to bend kernel interfaces around them.

Driver is based aroud Packet Input Processing/Input Packet Data (PIP/IPD)
units which can connect their data streams to various interfaces.
SGMII/1000BASE-X/QSGMII and RGMII are just two of them.

Currently driver iterates over all interfaces and all ports to bind
interfaces to PIP/IPD. There is a lot of code deciding which
interfaces/ports exits on given Octeon SoC, see
arch/mips/cavium-octeon/executive/
Driver code then calls those helpers with interface/port aguments
and they do the magic using switches deciding what to do based
on interface type.

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.

Thanks,
	ladis

Re: [PATCH 2/3] staging: octeon: avoid needless device allocation

[Andrew Lunn]

> 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.

It is a reasonable solution, and does help clean up some of the mess
keeping it in staging.

But as Maintainers, we also have to consider, are you just going to
add the cleanup you need in order that phylink supports SFPs, which is
what you seem most interesting in. And then stop the cleanup. Despite
the code being in staging, it is good enough for you.

So converting to phylink, with the existing feature set, is fine.

Maybe adding SFP support is a new feature, and we might consider not
accepting such patches until the driver has made it out of staging?

	  Andrew