| Message ID | 20240501230552.53185-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 |
On Thu, May 02, 2024 at 08:05:52AM +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 | 1 + > 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 | 67 ++++++++++++++++++++++++++ > 5 files changed, 104 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..6ad5d37eb0e4 100644 > --- a/drivers/net/ethernet/tehuti/Kconfig > +++ b/drivers/net/ethernet/tehuti/Kconfig > @@ -27,6 +27,7 @@ config TEHUTI_TN40 > tristate "Tehuti Networks TN40xx 10G Ethernet adapters" > depends on PCI > select FW_LOADER > + 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 db1f781b8063..bf9c00513a0c 100644 > --- a/drivers/net/ethernet/tehuti/tn40.c > +++ b/drivers/net/ethernet/tehuti/tn40.c > @@ -7,6 +7,7 @@ > #include <linux/if_vlan.h> > #include <linux/netdevice.h> > #include <linux/pci.h> > +#include <linux/phylink.h> > > #include "tn40.h" > > @@ -1185,21 +1186,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); > + } What exactly does link_changed mean? The normal use case for calling phylink_mac_change() is that you have received an interrupt from something like the PCS, or the PHY. The MAC driver itself cannot fully evaluate if the link is up because there can be multiple parts in that decision. Is the SFP reporting LOS? Does the PCS SERDES have sync, etc. So all you do is forward the interrupt to phylink. phylink will then look at everything it knows about and decide the state of the link, and maybe call one of the callbacks indicating the link is now up/down. > 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->speed); > priv->link_loop_cnt = 0; This should move into the link_down callback. > - if (!netif_carrier_ok(priv->ndev) && !link) > + if (!netif_carrier_ok(priv->ndev) && !link) { > netif_wake_queue(priv->ndev); and this should be in the link_up callback. > +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) > +{ > + struct tn40_priv *priv = container_of(config, struct tn40_priv, > + phylink_config); > + > + priv->speed = speed; This is where you should take any actions needed to make the MAC send packets, at the correct rate. > +} > + > +static void tn40_link_down(struct phylink_config *config, unsigned int mode, > + phy_interface_t interface) > +{ And here you should stop the MAC sending packets. > +} > + > +static void tn40_mac_config(struct phylink_config *config, unsigned int mode, > + const struct phylink_link_state *state) > +{ I know at the moment you only support 10G. When you add support for 1G, this is where you will need to configure the MAC to swap between the different modes. phylink will tell you which mode to use, 10GBaseX, 1000BaseX, SGMII, etc. You might want to move the existing code for 10GBaseX here. For the next version, please also Cc: Russell King, the phylink Maintainer. Andrew
Hi, On Wed, 8 May 2024 14:21:29 +0200 Andrew Lunn <andrew@lunn.ch> wrote: >> --- a/drivers/net/ethernet/tehuti/tn40.c >> +++ b/drivers/net/ethernet/tehuti/tn40.c >> @@ -7,6 +7,7 @@ >> #include <linux/if_vlan.h> >> #include <linux/netdevice.h> >> #include <linux/pci.h> >> +#include <linux/phylink.h> >> >> #include "tn40.h" >> >> @@ -1185,21 +1186,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); >> + } > > What exactly does link_changed mean? > > The normal use case for calling phylink_mac_change() is that you have > received an interrupt from something like the PCS, or the PHY. The MAC > driver itself cannot fully evaluate if the link is up because there > can be multiple parts in that decision. Is the SFP reporting LOS? Does The original driver receives an interrupt from an PHY (or something), then reads the register (TN40_REG_MAC_LNK_STAT) to evaluate the state of the link; doesn't use information from the PHY. > the PCS SERDES have sync, etc. So all you do is forward the interrupt > to phylink. phylink will then look at everything it knows about and > decide the state of the link, and maybe call one of the callbacks > indicating the link is now up/down. What function should be used to forward an interrupt to phylink? equivalent to phy_mac_interrupt() in phylib. >> 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->speed); >> priv->link_loop_cnt = 0; > > This should move into the link_down callback. I'll try phylink callbacks to see if they would work. >> - if (!netif_carrier_ok(priv->ndev) && !link) >> + if (!netif_carrier_ok(priv->ndev) && !link) { >> netif_wake_queue(priv->ndev); > > and this should be in the link_up callback. >> +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) >> +{ >> + struct tn40_priv *priv = container_of(config, struct tn40_priv, >> + phylink_config); >> + >> + priv->speed = speed; > > This is where you should take any actions needed to make the MAC send > packets, at the correct rate. > >> +} >> + >> +static void tn40_link_down(struct phylink_config *config, unsigned int mode, >> + phy_interface_t interface) >> +{ > > And here you should stop the MAC sending packets. > >> +} > >> + >> +static void tn40_mac_config(struct phylink_config *config, unsigned int mode, >> + const struct phylink_link_state *state) >> +{ > > I know at the moment you only support 10G. When you add support for > 1G, this is where you will need to configure the MAC to swap between > the different modes. phylink will tell you which mode to use, > 10GBaseX, 1000BaseX, SGMII, etc. You might want to move the existing > code for 10GBaseX here. Yeah. The original driver configures the MAC for 10G with QT2025 PHY so I'm not sure things would work well. I'll experiment once I get 1G SFP. > For the next version, please also Cc: Russell King, the phylink > Maintainer. Sure, I'll do in v6.
On Wed, May 08, 2024 at 10:18:51PM +0900, FUJITA Tomonori wrote: > Hi, > > On Wed, 8 May 2024 14:21:29 +0200 > Andrew Lunn <andrew@lunn.ch> wrote: > > >> --- a/drivers/net/ethernet/tehuti/tn40.c > >> +++ b/drivers/net/ethernet/tehuti/tn40.c > >> @@ -7,6 +7,7 @@ > >> #include <linux/if_vlan.h> > >> #include <linux/netdevice.h> > >> #include <linux/pci.h> > >> +#include <linux/phylink.h> > >> > >> #include "tn40.h" > >> > >> @@ -1185,21 +1186,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); > >> + } > > > > What exactly does link_changed mean? > > > > The normal use case for calling phylink_mac_change() is that you have > > received an interrupt from something like the PCS, or the PHY. The MAC > > driver itself cannot fully evaluate if the link is up because there > > can be multiple parts in that decision. Is the SFP reporting LOS? Does > > The original driver receives an interrupt from an PHY (or something), > then reads the register (TN40_REG_MAC_LNK_STAT) to evaluate the state > of the link; doesn't use information from the PHY. So i guess this is the PCS state. Call phylink_mac_change() with this state. Don't do anything else here. Andrew
Hi, On Wed, 08 May 2024 22:18:51 +0900 (JST) FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: >>> 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->speed); >>> priv->link_loop_cnt = 0; >> >> This should move into the link_down callback. > > I'll try phylink callbacks to see if they would work. I found that the link_down callback doesn't work well for the MAC reset above. Currently, when TN40_REG_MAC_LNK_STAT register tells that the link is off, the driver configures the MAC to generate an interrupt periodically; tn40_write_reg(priv, 0x5150, 1000000) is called in tn40_link_changed(). Eventually, the counter is over TN40_LINK_LOOP_MAX and then the driver executes the MAC reset. Without the MAC reset, the NIC will not work. The link_down callback is called only when the link becomes down so it can't be used to trigger the MAC reset.
On Thu, May 09, 2024 at 01:23:41PM +0900, FUJITA Tomonori wrote: > Hi, > > On Wed, 08 May 2024 22:18:51 +0900 (JST) > FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > >>> 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->speed); > >>> priv->link_loop_cnt = 0; > >> > >> This should move into the link_down callback. > > > > I'll try phylink callbacks to see if they would work. > > I found that the link_down callback doesn't work well for the MAC > reset above. > > Currently, when TN40_REG_MAC_LNK_STAT register tells that the link is > off, the driver configures the MAC to generate an interrupt > periodically; tn40_write_reg(priv, 0x5150, 1000000) is called in > tn40_link_changed(). > > Eventually, the counter is over TN40_LINK_LOOP_MAX and then the driver > executes the MAC reset. Without the MAC reset, the NIC will not work. > > The link_down callback is called only when the link becomes down so it > can't be used to trigger the MAC reset. So this sounds like a hardware bug workaround. But it might also be to do with auto-neg. The MAC PCS/SERDES and the PHY PCS/SERDES, depending on the mode, should be performing auto-neg, to indicate things like pause. For some hardware, you need to restart autoneg when the line sides gets link. It could be this hardware has no way to do that, other than hit the whole thing with a reset? Take a look at struct phylink_pcs_ops and see if you can map bits of the driver to this structure. It might be you can implement a PCS, and have the pcs_an_restart do the MAC reset. Andrew
Hi, On Thu, 9 May 2024 15:37:55 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, May 09, 2024 at 01:23:41PM +0900, FUJITA Tomonori wrote: >> Hi, >> >> On Wed, 08 May 2024 22:18:51 +0900 (JST) >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: >> >> >>> 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->speed); >> >>> priv->link_loop_cnt = 0; >> >> >> >> This should move into the link_down callback. >> > >> > I'll try phylink callbacks to see if they would work. >> >> I found that the link_down callback doesn't work well for the MAC >> reset above. >> >> Currently, when TN40_REG_MAC_LNK_STAT register tells that the link is >> off, the driver configures the MAC to generate an interrupt >> periodically; tn40_write_reg(priv, 0x5150, 1000000) is called in >> tn40_link_changed(). >> >> Eventually, the counter is over TN40_LINK_LOOP_MAX and then the driver >> executes the MAC reset. Without the MAC reset, the NIC will not work. >> >> The link_down callback is called only when the link becomes down so it >> can't be used to trigger the MAC reset. > > So this sounds like a hardware bug workaround. Yeah, looks so. > But it might also be to do with auto-neg. The MAC PCS/SERDES and the > PHY PCS/SERDES, depending on the mode, should be performing auto-neg, > to indicate things like pause. For some hardware, you need to restart > autoneg when the line sides gets link. It could be this hardware has > no way to do that, other than hit the whole thing with a reset? I can't find a function to do such in the original driver. > Take a look at struct phylink_pcs_ops and see if you can map bits of > the driver to this structure. It might be you can implement a PCS, and > have the pcs_an_restart do the MAC reset. I can't find anything that checks the link periodically until the link is recovered. I dropped the workaround and does a reset every time the link is down (mac_link_down). Seems that it works.
diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig index 4198fd59e42e..6ad5d37eb0e4 100644 --- a/drivers/net/ethernet/tehuti/Kconfig +++ b/drivers/net/ethernet/tehuti/Kconfig @@ -27,6 +27,7 @@ config TEHUTI_TN40 tristate "Tehuti Networks TN40xx 10G Ethernet adapters" depends on PCI select FW_LOADER + 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 db1f781b8063..bf9c00513a0c 100644 --- a/drivers/net/ethernet/tehuti/tn40.c +++ b/drivers/net/ethernet/tehuti/tn40.c @@ -7,6 +7,7 @@ #include <linux/if_vlan.h> #include <linux/netdevice.h> #include <linux/pci.h> +#include <linux/phylink.h> #include "tn40.h" @@ -1185,21 +1186,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->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; } @@ -1477,6 +1482,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); @@ -1492,10 +1500,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); @@ -1790,19 +1805,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: @@ -1823,6 +1844,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 ce991041caf9..cfe7f2318be2 100644 --- a/drivers/net/ethernet/tehuti/tn40.h +++ b/drivers/net/ethernet/tehuti/tn40.h @@ -161,6 +161,10 @@ 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; + int speed; }; /* RX FREE descriptor - 64bit */ @@ -249,4 +253,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..97aa3e100a3b --- /dev/null +++ b/drivers/net/ethernet/tehuti/tn40_phy.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (c) Tehuti Networks Ltd. */ + +#include <linux/netdevice.h> +#include <linux/pci.h> +#include <linux/phylink.h> + +#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) +{ + struct tn40_priv *priv = container_of(config, struct tn40_priv, + phylink_config); + + priv->speed = speed; +} + +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; + } + + 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); +}
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 | 1 + 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 | 67 ++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c