diff mbox series

[net-next,v3,1/7] net: ngbe: implement phylink to handle PHY device

Message ID 20231206095355.1220086-2-jiawenwu@trustnetic.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Implement more ethtool_ops for Wangxun | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1115 this patch: 1115
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 1142 this patch: 1142
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1142 this patch: 1142
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 270 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiawen Wu Dec. 6, 2023, 9:53 a.m. UTC
Add phylink support for Wangxun 1Gb Ethernet controller.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |   8 ++
 drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  20 ++-
 drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 126 +++++++++++-------
 drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h |   2 +-
 4 files changed, 93 insertions(+), 63 deletions(-)

Comments

Russell King (Oracle) Dec. 6, 2023, 12:06 p.m. UTC | #1
On Wed, Dec 06, 2023 at 05:53:49PM +0800, Jiawen Wu wrote:
> Add phylink support for Wangxun 1Gb Ethernet controller.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  |   8 ++
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  20 ++-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 126 +++++++++++-------
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h |   2 +-
>  4 files changed, 93 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 165e82de772e..9225aaf029f8 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -8,6 +8,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/if_vlan.h>
>  #include <net/ip.h>
> +#include <linux/phylink.h>

Nit: would be better to keep linux/ includes together (and in
alphabetical order to prevent conflicts.)

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 8db804543e66..c61f4b9d79fa 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -9,6 +9,7 @@
>  #include <linux/etherdevice.h>
>  #include <net/ip.h>
>  #include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/if_vlan.h>
>  
>  #include "../libwx/wx_type.h"

As wx_type.h includes linux/phylink.h, which is now fundamental for the
definition of one of the structures in wx_type.h, the include of
linux/phylink.h seems unnecessary here.

> @@ -336,7 +337,8 @@ static void ngbe_disable_device(struct wx *wx)
>  
>  static void ngbe_down(struct wx *wx)
>  {
> -	phy_stop(wx->phydev);
> +	phylink_stop(wx->phylink);
> +	phylink_disconnect_phy(wx->phylink);

I'm not sure why you're moving the PHY disconnection in this patch -
that seems like a separate change to the actual conversion to phylink.
For a pure conversion, you should be able to just replace the phylib
calls with their phylink equivalents.

It seems to me that there's two changes happening here: a conversion to
phylink and a re-ordering of the open/close methods particularly to do
with connecting and disconnecting the PHY. Either this needs to be
described in the commit message (the fact that it's happening and why)
or it should be two patches.

> -static void ngbe_phy_fixup(struct wx *wx)
> +void ngbe_phylink_start(struct wx *wx)
>  {
> -	struct phy_device *phydev = wx->phydev;
> -	struct ethtool_eee eee;
> -
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -
> -	phydev->mac_managed_pm = true;
> -	if (wx->mac_type != em_mac_type_mdi)
> -		return;
> -	/* disable EEE, internal phy does not support eee */
> -	memset(&eee, 0, sizeof(eee));
> -	phy_ethtool_set_eee(phydev, &eee);
> +	struct phylink *phylink = wx->phylink;
> +
> +	phylink_connect_phy(phylink, wx->phydev);

Note that phylink_connect_phy() can fail, so it's return value should
be checked.

Apart from the comments above, I think I'm fine with this.
Jiawen Wu Dec. 8, 2023, 7:12 a.m. UTC | #2
On Wednesday, December 6, 2023 8:06 PM, Russell King (Oracle) wrote:
> On Wed, Dec 06, 2023 at 05:53:49PM +0800, Jiawen Wu wrote:
> > Add phylink support for Wangxun 1Gb Ethernet controller.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >  drivers/net/ethernet/wangxun/libwx/wx_type.h  |   8 ++
> >  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  20 ++-
> >  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 126 +++++++++++-------
> >  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h |   2 +-
> >  4 files changed, 93 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > index 165e82de772e..9225aaf029f8 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> > @@ -8,6 +8,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/if_vlan.h>
> >  #include <net/ip.h>
> > +#include <linux/phylink.h>
> 
> Nit: would be better to keep linux/ includes together (and in
> alphabetical order to prevent conflicts.)
> 
> > diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > index 8db804543e66..c61f4b9d79fa 100644
> > --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/etherdevice.h>
> >  #include <net/ip.h>
> >  #include <linux/phy.h>
> > +#include <linux/phylink.h>
> >  #include <linux/if_vlan.h>
> >
> >  #include "../libwx/wx_type.h"
> 
> As wx_type.h includes linux/phylink.h, which is now fundamental for the
> definition of one of the structures in wx_type.h, the include of
> linux/phylink.h seems unnecessary here.

Should I remove the include of linux/phylink.h that have been added in other .c files?
diff mbox series

Patch

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 165e82de772e..9225aaf029f8 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -8,6 +8,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/if_vlan.h>
 #include <net/ip.h>
+#include <linux/phylink.h>
 
 #define WX_NCSI_SUP                             0x8000
 #define WX_NCSI_MASK                            0x8000
@@ -940,6 +941,8 @@  struct wx {
 	int speed;
 	int duplex;
 	struct phy_device *phydev;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 
 	bool wol_hw_supported;
 	bool ncsi_enabled;
@@ -1045,4 +1048,9 @@  rd64(struct wx *wx, u32 reg)
 #define wx_dbg(wx, fmt, arg...) \
 	dev_dbg(&(wx)->pdev->dev, fmt, ##arg)
 
+static inline struct wx *phylink_to_wx(struct phylink_config *config)
+{
+	return container_of(config, struct wx, phylink_config);
+}
+
 #endif /* _WX_TYPE_H_ */
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
index 8db804543e66..c61f4b9d79fa 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
@@ -9,6 +9,7 @@ 
 #include <linux/etherdevice.h>
 #include <net/ip.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/if_vlan.h>
 
 #include "../libwx/wx_type.h"
@@ -336,7 +337,8 @@  static void ngbe_disable_device(struct wx *wx)
 
 static void ngbe_down(struct wx *wx)
 {
-	phy_stop(wx->phydev);
+	phylink_stop(wx->phylink);
+	phylink_disconnect_phy(wx->phylink);
 	ngbe_disable_device(wx);
 	wx_clean_all_tx_rings(wx);
 	wx_clean_all_rx_rings(wx);
@@ -359,7 +361,7 @@  static void ngbe_up(struct wx *wx)
 	if (wx->gpio_ctrl)
 		ngbe_sfp_modules_txrx_powerctl(wx, true);
 
-	phy_start(wx->phydev);
+	ngbe_phylink_start(wx);
 }
 
 /**
@@ -388,23 +390,18 @@  static int ngbe_open(struct net_device *netdev)
 	if (err)
 		goto err_free_resources;
 
-	err = ngbe_phy_connect(wx);
-	if (err)
-		goto err_free_irq;
-
 	err = netif_set_real_num_tx_queues(netdev, wx->num_tx_queues);
 	if (err)
-		goto err_dis_phy;
+		goto err_free_irq;
 
 	err = netif_set_real_num_rx_queues(netdev, wx->num_rx_queues);
 	if (err)
-		goto err_dis_phy;
+		goto err_free_irq;
 
 	ngbe_up(wx);
 
 	return 0;
-err_dis_phy:
-	phy_disconnect(wx->phydev);
+
 err_free_irq:
 	wx_free_irq(wx);
 err_free_resources:
@@ -430,7 +427,6 @@  static int ngbe_close(struct net_device *netdev)
 	ngbe_down(wx);
 	wx_free_irq(wx);
 	wx_free_resources(wx);
-	phy_disconnect(wx->phydev);
 	wx_control_hw(wx, false);
 
 	return 0;
@@ -680,6 +676,7 @@  static int ngbe_probe(struct pci_dev *pdev,
 	return 0;
 
 err_register:
+	phylink_destroy(wx->phylink);
 	wx_control_hw(wx, false);
 err_clear_interrupt_scheme:
 	wx_clear_interrupt_scheme(wx);
@@ -709,6 +706,7 @@  static void ngbe_remove(struct pci_dev *pdev)
 
 	netdev = wx->netdev;
 	unregister_netdev(netdev);
+	phylink_destroy(wx->phylink);
 	pci_release_selected_regions(pdev,
 				     pci_select_bars(pdev, IORESOURCE_MEM));
 
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
index 6302ecca71bb..324f8af58b97 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c
@@ -5,6 +5,7 @@ 
 #include <linux/iopoll.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 
 #include "../libwx/wx_type.h"
 #include "../libwx/wx_hw.h"
@@ -56,22 +57,26 @@  static int ngbe_phy_write_reg_c22(struct mii_bus *bus, int phy_addr,
 	return ret;
 }
 
-static void ngbe_handle_link_change(struct net_device *dev)
+static void ngbe_mac_config(struct phylink_config *config, unsigned int mode,
+			    const struct phylink_link_state *state)
 {
-	struct wx *wx = netdev_priv(dev);
-	struct phy_device *phydev;
-	u32 lan_speed, reg;
+}
 
-	phydev = wx->phydev;
-	if (!(wx->link != phydev->link ||
-	      wx->speed != phydev->speed ||
-	      wx->duplex != phydev->duplex))
-		return;
+static void ngbe_mac_link_down(struct phylink_config *config,
+			       unsigned int mode, phy_interface_t interface)
+{
+}
+
+static void ngbe_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)
+{
+	struct wx *wx = phylink_to_wx(config);
+	u32 lan_speed, reg;
 
-	wx->link = phydev->link;
-	wx->speed = phydev->speed;
-	wx->duplex = phydev->duplex;
-	switch (phydev->speed) {
+	switch (speed) {
 	case SPEED_10:
 		lan_speed = 0;
 		break;
@@ -83,54 +88,68 @@  static void ngbe_handle_link_change(struct net_device *dev)
 		lan_speed = 2;
 		break;
 	}
+
 	wr32m(wx, NGBE_CFG_LAN_SPEED, 0x3, lan_speed);
 
-	if (phydev->link) {
-		reg = rd32(wx, WX_MAC_TX_CFG);
-		reg &= ~WX_MAC_TX_CFG_SPEED_MASK;
-		reg |= WX_MAC_TX_CFG_SPEED_1G | WX_MAC_TX_CFG_TE;
-		wr32(wx, WX_MAC_TX_CFG, reg);
-		/* Re configure MAC RX */
-		reg = rd32(wx, WX_MAC_RX_CFG);
-		wr32(wx, WX_MAC_RX_CFG, reg);
-		wr32(wx, WX_MAC_PKT_FLT, WX_MAC_PKT_FLT_PR);
-		reg = rd32(wx, WX_MAC_WDG_TIMEOUT);
-		wr32(wx, WX_MAC_WDG_TIMEOUT, reg);
-	}
-	phy_print_status(phydev);
+	reg = rd32(wx, WX_MAC_TX_CFG);
+	reg &= ~WX_MAC_TX_CFG_SPEED_MASK;
+	reg |= WX_MAC_TX_CFG_SPEED_1G | WX_MAC_TX_CFG_TE;
+	wr32(wx, WX_MAC_TX_CFG, reg);
+
+	/* Re configure MAC Rx */
+	reg = rd32(wx, WX_MAC_RX_CFG);
+	wr32(wx, WX_MAC_RX_CFG, reg);
+	wr32(wx, WX_MAC_PKT_FLT, WX_MAC_PKT_FLT_PR);
+	reg = rd32(wx, WX_MAC_WDG_TIMEOUT);
+	wr32(wx, WX_MAC_WDG_TIMEOUT, reg);
 }
 
-int ngbe_phy_connect(struct wx *wx)
+static const struct phylink_mac_ops ngbe_mac_ops = {
+	.mac_config = ngbe_mac_config,
+	.mac_link_down = ngbe_mac_link_down,
+	.mac_link_up = ngbe_mac_link_up,
+};
+
+static int ngbe_phylink_init(struct wx *wx)
 {
-	int ret;
+	struct phylink_config *config;
+	phy_interface_t phy_mode;
+	struct phylink *phylink;
 
-	ret = phy_connect_direct(wx->netdev,
-				 wx->phydev,
-				 ngbe_handle_link_change,
-				 PHY_INTERFACE_MODE_RGMII_ID);
-	if (ret) {
-		wx_err(wx, "PHY connect failed.\n");
-		return ret;
-	}
+	config = &wx->phylink_config;
+	config->dev = &wx->netdev->dev;
+	config->type = PHYLINK_NETDEV;
+	config->mac_capabilities = MAC_1000FD | MAC_100FD | MAC_10FD |
+				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
+	config->mac_managed_pm = true;
+
+	phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
+	__set_bit(PHY_INTERFACE_MODE_RGMII_ID, config->supported_interfaces);
+
+	phylink = phylink_create(config, NULL, phy_mode, &ngbe_mac_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
+
+	wx->phylink = phylink;
 
 	return 0;
 }
 
-static void ngbe_phy_fixup(struct wx *wx)
+void ngbe_phylink_start(struct wx *wx)
 {
-	struct phy_device *phydev = wx->phydev;
-	struct ethtool_eee eee;
-
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
-	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
-
-	phydev->mac_managed_pm = true;
-	if (wx->mac_type != em_mac_type_mdi)
-		return;
-	/* disable EEE, internal phy does not support eee */
-	memset(&eee, 0, sizeof(eee));
-	phy_ethtool_set_eee(phydev, &eee);
+	struct phylink *phylink = wx->phylink;
+
+	phylink_connect_phy(phylink, wx->phydev);
+
+	if (wx->mac_type == em_mac_type_mdi) {
+		struct ethtool_eee eee;
+
+		/* disable EEE, internal phy does not support eee */
+		memset(&eee, 0, sizeof(eee));
+		phylink_ethtool_set_eee(phylink, &eee);
+	}
+
+	phylink_start(phylink);
 }
 
 int ngbe_mdio_init(struct wx *wx)
@@ -165,11 +184,16 @@  int ngbe_mdio_init(struct wx *wx)
 		return -ENODEV;
 
 	phy_attached_info(wx->phydev);
-	ngbe_phy_fixup(wx);
 
 	wx->link = 0;
 	wx->speed = 0;
 	wx->duplex = 0;
 
+	ret = ngbe_phylink_init(wx);
+	if (ret) {
+		wx_err(wx, "failed to init phylink: %d\n", ret);
+		return ret;
+	}
+
 	return 0;
 }
diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h
index 0a6400dd89c4..c5a9386caf0a 100644
--- a/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h
+++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h
@@ -7,6 +7,6 @@ 
 #ifndef _NGBE_MDIO_H_
 #define _NGBE_MDIO_H_
 
-int ngbe_phy_connect(struct wx *wx);
+void ngbe_phylink_start(struct wx *wx);
 int ngbe_mdio_init(struct wx *wx);
 #endif /* _NGBE_MDIO_H_ */