diff mbox series

[net-next,v3,6/6] net: tn40xx: add PHYLIB support

Message ID 20240429043827.44407-7-fujita.tomonori@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series add ethernet driver for Tehuti Networks TN40xx chips | 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: 933 this patch: 933
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: pabeni@redhat.com kuba@kernel.org edumazet@google.com linux@armlinux.org.uk
netdev/build_clang success Errors and warnings before: 938 this patch: 938
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: 944 this patch: 944
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
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

FUJITA Tomonori April 29, 2024, 4:38 a.m. UTC 
This patch adds supports for multiple PHY hardware with PHYLIB. The
adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.

For now, the PCI ID table of this driver enables adapters using only
QT2025 PHY. I've tested this driver and the QT2025 PHY driver with
Edimax EN-9320 10G adapter.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/ethernet/tehuti/Kconfig    |  2 +
 drivers/net/ethernet/tehuti/Makefile   |  2 +-
 drivers/net/ethernet/tehuti/tn40.c     | 34 +++++++++++---
 drivers/net/ethernet/tehuti/tn40.h     |  7 +++
 drivers/net/ethernet/tehuti/tn40_phy.c | 61 ++++++++++++++++++++++++++
 5 files changed, 99 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c

Comments

Andrew Lunn April 30, 2024, 8:58 p.m. UTC | #1 
On Mon, Apr 29, 2024 at 01:38:27PM +0900, FUJITA Tomonori wrote:
> This patch adds supports for multiple PHY hardware with PHYLIB. The
> adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
> TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.
> 
> For now, the PCI ID table of this driver enables adapters using only
> QT2025 PHY. I've tested this driver and the QT2025 PHY driver with
> Edimax EN-9320 10G adapter.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/ethernet/tehuti/Kconfig    |  2 +
>  drivers/net/ethernet/tehuti/Makefile   |  2 +-
>  drivers/net/ethernet/tehuti/tn40.c     | 34 +++++++++++---
>  drivers/net/ethernet/tehuti/tn40.h     |  7 +++
>  drivers/net/ethernet/tehuti/tn40_phy.c | 61 ++++++++++++++++++++++++++
>  5 files changed, 99 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c
> 
> diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig
> index 4198fd59e42e..94fda9fd4cc0 100644
> --- a/drivers/net/ethernet/tehuti/Kconfig
> +++ b/drivers/net/ethernet/tehuti/Kconfig
> @@ -27,6 +27,8 @@ config TEHUTI_TN40
>  	tristate "Tehuti Networks TN40xx 10G Ethernet adapters"
>  	depends on PCI
>  	select FW_LOADER
> +	select PHYLIB
> +	select PHYLINK

You don't need both. PHYLINK will pull in PHYLIB.

> @@ -1179,21 +1179,25 @@ static void tn40_link_changed(struct tn40_priv *priv)
>  	u32 link = tn40_read_reg(priv,
>  				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
>  	if (!link) {
> -		if (netif_carrier_ok(priv->ndev) && priv->link)
> +		if (netif_carrier_ok(priv->ndev) && priv->link) {
>  			netif_stop_queue(priv->ndev);
> +			phylink_mac_change(priv->phylink, false);
> +		}
>  
>  		priv->link = 0;
>  		if (priv->link_loop_cnt++ > TN40_LINK_LOOP_MAX) {
>  			/* MAC reset */
>  			tn40_set_link_speed(priv, 0);
> +			tn40_set_link_speed(priv, priv->phydev->speed);

You should not be references priv->phydev if you are using
phylink. When phylink is managing an SFP, there might not be a phydev.
phylink will tell you the speed when it calls your tn40_mac_config()
callback.

I suggest you read the documentation in include/linux/phylink.h
because this is very wrong.

	Andrew
FUJITA Tomonori May 1, 2024, 6:41 a.m. UTC | #2 
Hi,

On Tue, 30 Apr 2024 22:58:17 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Apr 29, 2024 at 01:38:27PM +0900, FUJITA Tomonori wrote:
>> This patch adds supports for multiple PHY hardware with PHYLIB. The
>> adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
>> TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.
>> 
>> For now, the PCI ID table of this driver enables adapters using only
>> QT2025 PHY. I've tested this driver and the QT2025 PHY driver with
>> Edimax EN-9320 10G adapter.
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  drivers/net/ethernet/tehuti/Kconfig    |  2 +
>>  drivers/net/ethernet/tehuti/Makefile   |  2 +-
>>  drivers/net/ethernet/tehuti/tn40.c     | 34 +++++++++++---
>>  drivers/net/ethernet/tehuti/tn40.h     |  7 +++
>>  drivers/net/ethernet/tehuti/tn40_phy.c | 61 ++++++++++++++++++++++++++
>>  5 files changed, 99 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c
>> 
>> diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig
>> index 4198fd59e42e..94fda9fd4cc0 100644
>> --- a/drivers/net/ethernet/tehuti/Kconfig
>> +++ b/drivers/net/ethernet/tehuti/Kconfig
>> @@ -27,6 +27,8 @@ config TEHUTI_TN40
>>  	tristate "Tehuti Networks TN40xx 10G Ethernet adapters"
>>  	depends on PCI
>>  	select FW_LOADER
>> +	select PHYLIB
>> +	select PHYLINK
> 
> You don't need both. PHYLINK will pull in PHYLIB.

Fixed.

>> @@ -1179,21 +1179,25 @@ static void tn40_link_changed(struct tn40_priv *priv)
>>  	u32 link = tn40_read_reg(priv,
>>  				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
>>  	if (!link) {
>> -		if (netif_carrier_ok(priv->ndev) && priv->link)
>> +		if (netif_carrier_ok(priv->ndev) && priv->link) {
>>  			netif_stop_queue(priv->ndev);
>> +			phylink_mac_change(priv->phylink, false);
>> +		}
>>  
>>  		priv->link = 0;
>>  		if (priv->link_loop_cnt++ > TN40_LINK_LOOP_MAX) {
>>  			/* MAC reset */
>>  			tn40_set_link_speed(priv, 0);
>> +			tn40_set_link_speed(priv, priv->phydev->speed);
> 
> You should not be references priv->phydev if you are using
> phylink. When phylink is managing an SFP, there might not be a phydev.
> phylink will tell you the speed when it calls your tn40_mac_config()
> callback.
> 
> I suggest you read the documentation in include/linux/phylink.h
> because this is very wrong.

Understood. I fixed the code not to use priv->phydev (except for
phylink_connect_phy).

Looks like phylink.h recommends using mac_link_up() over mac_config()
to get the link speed. So tn40_link_up() stores the speed in tn40_priv
for tn40_link_changed(). I also tried mac_config() but state->speed
isn't initialized properly.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig
index 4198fd59e42e..94fda9fd4cc0 100644
--- a/drivers/net/ethernet/tehuti/Kconfig
+++ b/drivers/net/ethernet/tehuti/Kconfig
@@ -27,6 +27,8 @@  config TEHUTI_TN40
 	tristate "Tehuti Networks TN40xx 10G Ethernet adapters"
 	depends on PCI
 	select FW_LOADER
+	select PHYLIB
+	select PHYLINK
 	help
 	  This driver supports 10G Ethernet adapters using Tehuti Networks
 	  TN40xx chips. Currently, adapters with Applied Micro Circuits
diff --git a/drivers/net/ethernet/tehuti/Makefile b/drivers/net/ethernet/tehuti/Makefile
index 7a0fe586a243..0d4f4d63a65c 100644
--- a/drivers/net/ethernet/tehuti/Makefile
+++ b/drivers/net/ethernet/tehuti/Makefile
@@ -5,5 +5,5 @@ 
 
 obj-$(CONFIG_TEHUTI) += tehuti.o
 
-tn40xx-y := tn40.o tn40_mdio.o
+tn40xx-y := tn40.o tn40_mdio.o tn40_phy.o
 obj-$(CONFIG_TEHUTI_TN40) += tn40xx.o
diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c
index c87ca150b583..a7e25cbb037b 100644
--- a/drivers/net/ethernet/tehuti/tn40.c
+++ b/drivers/net/ethernet/tehuti/tn40.c
@@ -1179,21 +1179,25 @@  static void tn40_link_changed(struct tn40_priv *priv)
 	u32 link = tn40_read_reg(priv,
 				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
 	if (!link) {
-		if (netif_carrier_ok(priv->ndev) && priv->link)
+		if (netif_carrier_ok(priv->ndev) && priv->link) {
 			netif_stop_queue(priv->ndev);
+			phylink_mac_change(priv->phylink, false);
+		}
 
 		priv->link = 0;
 		if (priv->link_loop_cnt++ > TN40_LINK_LOOP_MAX) {
 			/* MAC reset */
 			tn40_set_link_speed(priv, 0);
+			tn40_set_link_speed(priv, priv->phydev->speed);
 			priv->link_loop_cnt = 0;
 		}
 		tn40_write_reg(priv, 0x5150, 1000000);
 		return;
 	}
-	if (!netif_carrier_ok(priv->ndev) && !link)
+	if (!netif_carrier_ok(priv->ndev) && !link) {
 		netif_wake_queue(priv->ndev);
-
+		phylink_mac_change(priv->phylink, true);
+	}
 	priv->link = link;
 }
 
@@ -1201,6 +1205,7 @@  static inline void tn40_isr_extra(struct tn40_priv *priv, u32 isr)
 {
 	if (isr & (TN40_IR_LNKCHG0 | TN40_IR_LNKCHG1 | TN40_IR_TMR0)) {
 		netdev_dbg(priv->ndev, "isr = 0x%x\n", isr);
+		phy_mac_interrupt(priv->phydev);
 		tn40_link_changed(priv);
 	}
 }
@@ -1472,6 +1477,9 @@  static int tn40_close(struct net_device *ndev)
 {
 	struct tn40_priv *priv = netdev_priv(ndev);
 
+	phylink_stop(priv->phylink);
+	phylink_disconnect_phy(priv->phylink);
+
 	netif_napi_del(&priv->napi);
 	napi_disable(&priv->napi);
 	tn40_disable_interrupts(priv);
@@ -1487,10 +1495,17 @@  static int tn40_open(struct net_device *dev)
 	struct tn40_priv *priv = netdev_priv(dev);
 	int ret;
 
+	ret = phylink_connect_phy(priv->phylink, priv->phydev);
+	if (ret)
+		return ret;
+
 	tn40_sw_reset(priv);
+	phylink_start(priv->phylink);
 	ret = tn40_start(priv);
 	if (ret) {
 		netdev_err(dev, "failed to start %d\n", ret);
+		phylink_stop(priv->phylink);
+		phylink_disconnect_phy(priv->phylink);
 		return ret;
 	}
 	napi_enable(&priv->napi);
@@ -1785,19 +1800,25 @@  static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		TN40_IR_TMR1;
 
 	tn40_mac_init(priv);
-
+	ret = tn40_phy_register(priv);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to set up PHY.\n");
+		goto err_free_irq;
+	}
 	ret = tn40_priv_init(priv);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize tn40_priv.\n");
-		goto err_free_irq;
+		goto err_unregister_phydev;
 	}
 
 	ret = register_netdev(ndev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register netdev.\n");
-		goto err_free_irq;
+		goto err_unregister_phydev;
 	}
 	return 0;
+err_unregister_phydev:
+	tn40_phy_unregister(priv);
 err_free_irq:
 	pci_free_irq_vectors(pdev);
 err_unset_drvdata:
@@ -1818,6 +1839,7 @@  static void tn40_remove(struct pci_dev *pdev)
 
 	unregister_netdev(ndev);
 
+	tn40_phy_unregister(priv);
 	pci_free_irq_vectors(priv->pdev);
 	pci_set_drvdata(pdev, NULL);
 	iounmap(priv->regs);
diff --git a/drivers/net/ethernet/tehuti/tn40.h b/drivers/net/ethernet/tehuti/tn40.h
index 2719a31fe86c..472952e66c6b 100644
--- a/drivers/net/ethernet/tehuti/tn40.h
+++ b/drivers/net/ethernet/tehuti/tn40.h
@@ -17,6 +17,7 @@ 
 #include <linux/netdevice.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/tcp.h>
 #include <linux/udp.h>
 
@@ -177,6 +178,9 @@  struct tn40_priv {
 
 	struct tn40_rx_page_table rx_page_table;
 	struct mii_bus *mdio;
+	struct phy_device *phydev;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 };
 
 /* RX FREE descriptor - 64bit */
@@ -265,4 +269,7 @@  static inline void tn40_write_reg(struct tn40_priv *priv, u32 reg, u32 val)
 
 int tn40_mdiobus_init(struct tn40_priv *priv);
 
+int tn40_phy_register(struct tn40_priv *priv);
+void tn40_phy_unregister(struct tn40_priv *priv);
+
 #endif /* _TN40XX_H */
diff --git a/drivers/net/ethernet/tehuti/tn40_phy.c b/drivers/net/ethernet/tehuti/tn40_phy.c
new file mode 100644
index 000000000000..1484f15c07bb
--- /dev/null
+++ b/drivers/net/ethernet/tehuti/tn40_phy.c
@@ -0,0 +1,61 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) Tehuti Networks Ltd. */
+
+#include "tn40.h"
+
+static void tn40_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)
+{
+}
+
+static void tn40_link_down(struct phylink_config *config, unsigned int mode,
+			   phy_interface_t interface)
+{
+}
+
+static void tn40_mac_config(struct phylink_config *config, unsigned int mode,
+			    const struct phylink_link_state *state)
+{
+}
+
+static const struct phylink_mac_ops tn40_mac_ops = {
+	.mac_config = tn40_mac_config,
+	.mac_link_up = tn40_link_up,
+	.mac_link_down = tn40_link_down,
+};
+
+int tn40_phy_register(struct tn40_priv *priv)
+{
+	struct phylink_config *config;
+	struct phy_device *phydev;
+	struct phylink *phylink;
+
+	phydev = phy_find_first(priv->mdio);
+	if (!phydev) {
+		dev_err(&priv->pdev->dev, "PHY isn't found\n");
+		return -1;
+	}
+
+	phydev->irq = PHY_MAC_INTERRUPT;
+
+	config = &priv->phylink_config;
+	config->dev = &priv->ndev->dev;
+	config->type = PHYLINK_NETDEV;
+	config->mac_capabilities = MAC_10000FD | MLO_AN_PHY;
+	__set_bit(PHY_INTERFACE_MODE_XAUI, config->supported_interfaces);
+
+	phylink = phylink_create(config, NULL, PHY_INTERFACE_MODE_XAUI,
+				 &tn40_mac_ops);
+	if (IS_ERR(phylink))
+		return PTR_ERR(phylink);
+
+	priv->phydev = phydev;
+	priv->phylink = phylink;
+	return 0;
+}
+
+void tn40_phy_unregister(struct tn40_priv *priv)
+{
+	phylink_destroy(priv->phylink);
+}