| Message ID | 20240527203928.38206-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 Tue, May 28, 2024 at 05:39:28AM +0900, FUJITA Tomonori wrote: > This patch adds supports for multiple PHY hardware with phylink. 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 (SFP+ > 10G SR) with Edimax EN-9320 10G adapter. > i> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> A few comments - I don't recall seeing previous versions of these patches, despite it being at version 7. > @@ -1082,6 +1083,10 @@ static void tn40_link_changed(struct tn40_priv *priv) > TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT; > > netdev_dbg(priv->ndev, "link changed %u\n", link); > + if (link) > + phylink_mac_change(priv->phylink, true); > + else > + phylink_mac_change(priv->phylink, false); This is only useful if you have a PCS, and I don't see anything in the driver that suggests you do. What link is this referring to? In any case, you could eliminate the if() and just pass !!link if it's not already boolean in nature (the if() suggests it is, so passing just "link" would also work.) > @@ -1381,10 +1389,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); At this point, the link could have come up (mac_link_up() could well be called.) Is the driver prepared to cope with that happening right _now_ in the tn40_open sequence? If not, you will need to move this to a point where the driver is ready to begin operation. phylink_stop() is its opposite, and you need to call that at the point where you want the link to be taken down (iow, where you want .mac_link_down() to be guaranteed to have been called if the link was already up.) > @@ -143,6 +143,9 @@ struct tn40_priv { > char *b0_va; /* Virtual address of buffer */ > > struct mii_bus *mdio; > + struct phy_device *phydev; > + struct phylink *phylink; > + struct phylink_config phylink_config; So phylink_config is embedded in tn40_priv - that's fine. What this does mean is you can trivially go from the phylink_config pointer to a pointer to tn40_priv *without* multiple dereferences: static inline struct tn40_priv * config_to_tn40_priv(struct phylink_config *config) { return container_of(config, struct tn40_priv, phylink_config); } > +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 net_device *ndev = to_net_dev(config->dev); > + struct tn40_priv *priv = netdev_priv(ndev); > + > + tn40_set_link_speed(priv, speed); > + netif_wake_queue(priv->ndev); > +} > + > +static void tn40_link_down(struct phylink_config *config, unsigned int mode, > + phy_interface_t interface) > +{ > + struct net_device *ndev = to_net_dev(config->dev); > + struct tn40_priv *priv = netdev_priv(ndev); > + > + tn40_set_link_speed(priv, 0); > + netif_stop_queue(priv->ndev); Shouldn't the queue be stopped first? > +} > + > +static void tn40_mac_config(struct phylink_config *config, unsigned int mode, > + const struct phylink_link_state *state) > +{ > +} Nothing needs to be done here? > + > +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; MLO_AN_PHY is not a MAC capability, it shouldn't be here. Thanks.
Hi, Thanks for reviewing the patch! On Tue, 28 May 2024 13:02:42 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, May 28, 2024 at 05:39:28AM +0900, FUJITA Tomonori wrote: >> This patch adds supports for multiple PHY hardware with phylink. 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 (SFP+ >> 10G SR) with Edimax EN-9320 10G adapter. >> > i> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > A few comments - I don't recall seeing previous versions of these > patches, despite it being at version 7. Sorry, my bad. The first version doesn't use phylink. I should have added you to the CC list when I changed the driver to use phylink over phylib according to Andrew's comment. To provide some background, a vendor released this driver but it hasn't been merged into the tree. The vendor went out of bushiness. I'm trying to get the original driver to be mainline quality. I don't have hardware spec doc. >> @@ -1082,6 +1083,10 @@ static void tn40_link_changed(struct tn40_priv *priv) >> TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT; >> >> netdev_dbg(priv->ndev, "link changed %u\n", link); >> + if (link) >> + phylink_mac_change(priv->phylink, true); >> + else >> + phylink_mac_change(priv->phylink, false); > > This is only useful if you have a PCS, and I don't see anything in the > driver that suggests you do. What link is this referring to? When I plug the fiber, the above function is triggered with the link on. With the fiber unplugged, the function is called with the link off. The original driver resets the MAC when the link is down. It configures the MAC according to the cable speed (seems the original driver is tested with only 10G though). > In any case, you could eliminate the if() and just pass !!link if it's > not already boolean in nature (the if() suggests it is, so passing just > "link" would also work.) Oops, right. >> @@ -1381,10 +1389,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); > > At this point, the link could have come up (mac_link_up() could well > be called.) Is the driver prepared to cope with that happening right > _now_ in the tn40_open sequence? If not, you will need to move this > to a point where the driver is ready to begin operation. Understood, I'll change the order. > phylink_stop() is its opposite, and you need to call that at the > point where you want the link to be taken down (iow, where you want > .mac_link_down() to be guaranteed to have been called if the link > was already up.) I see. The usage of phylink_stop() looks fine. >> @@ -143,6 +143,9 @@ struct tn40_priv { >> char *b0_va; /* Virtual address of buffer */ >> >> struct mii_bus *mdio; >> + struct phy_device *phydev; >> + struct phylink *phylink; >> + struct phylink_config phylink_config; > > So phylink_config is embedded in tn40_priv - that's fine. What this does > mean is you can trivially go from the phylink_config pointer to a > pointer to tn40_priv *without* multiple dereferences: > > static inline struct tn40_priv * > config_to_tn40_priv(struct phylink_config *config) > { > return container_of(config, struct tn40_priv, phylink_config); > } I'll update the code. >> +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 net_device *ndev = to_net_dev(config->dev); >> + struct tn40_priv *priv = netdev_priv(ndev); >> + >> + tn40_set_link_speed(priv, speed); >> + netif_wake_queue(priv->ndev); >> +} >> + >> +static void tn40_link_down(struct phylink_config *config, unsigned int mode, >> + phy_interface_t interface) >> +{ >> + struct net_device *ndev = to_net_dev(config->dev); >> + struct tn40_priv *priv = netdev_priv(ndev); >> + >> + tn40_set_link_speed(priv, 0); >> + netif_stop_queue(priv->ndev); > > Shouldn't the queue be stopped first? Indeed, I'll fix. >> +} >> + >> +static void tn40_mac_config(struct phylink_config *config, unsigned int mode, >> + const struct phylink_link_state *state) >> +{ >> +} > > Nothing needs to be done here? Seems that nothing is necessary here. When I try something like 1G SFP or BASE-T in the future, I might find something. >> + >> +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; > > MLO_AN_PHY is not a MAC capability, it shouldn't be here. I don't know why I use MLO_AN_PHY here. I thought that I tried to imitate wangxun driver but I can't find. I'll drop it in the next version. Thanks a lot!
diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig index 2b3b5a8c7fbf..6db2c9817445 100644 --- a/drivers/net/ethernet/tehuti/Kconfig +++ b/drivers/net/ethernet/tehuti/Kconfig @@ -28,6 +28,7 @@ config TEHUTI_TN40 depends on PCI select PAGE_POOL 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 a41718f552ed..1584f5f5db97 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 <linux/vmalloc.h> #include <net/page_pool/helpers.h> @@ -944,7 +945,7 @@ static void tn40_tx_push_desc_safe(struct tn40_priv *priv, void *data, int size) } } -static int tn40_set_link_speed(struct tn40_priv *priv, u32 speed) +int tn40_set_link_speed(struct tn40_priv *priv, u32 speed) { u32 val; int i; @@ -1082,6 +1083,10 @@ static void tn40_link_changed(struct tn40_priv *priv) TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT; netdev_dbg(priv->ndev, "link changed %u\n", link); + if (link) + phylink_mac_change(priv->phylink, true); + else + phylink_mac_change(priv->phylink, false); } static void tn40_isr_extra(struct tn40_priv *priv, u32 isr) @@ -1366,6 +1371,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); + napi_disable(&priv->napi); netif_napi_del(&priv->napi); tn40_disable_interrupts(priv); @@ -1381,10 +1389,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); @@ -1664,6 +1679,12 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent) goto err_unset_drvdata; } + ret = tn40_mdiobus_init(priv); + if (ret) { + dev_err(&pdev->dev, "failed to initialize mdio bus.\n"); + goto err_free_irq; + } + priv->stats_flag = ((tn40_read_reg(priv, TN40_FPGA_VER) & 0xFFF) != 308); @@ -1672,19 +1693,26 @@ 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: @@ -1705,6 +1733,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 05a9adf9fe5a..fbdc62612f1f 100644 --- a/drivers/net/ethernet/tehuti/tn40.h +++ b/drivers/net/ethernet/tehuti/tn40.h @@ -143,6 +143,9 @@ struct tn40_priv { char *b0_va; /* Virtual address of buffer */ struct mii_bus *mdio; + struct phy_device *phydev; + struct phylink *phylink; + struct phylink_config phylink_config; }; /* RX FREE descriptor - 64bit */ @@ -220,6 +223,11 @@ static inline void tn40_write_reg(struct tn40_priv *priv, u32 reg, u32 val) writel(val, priv->regs + reg); } +int tn40_set_link_speed(struct tn40_priv *priv, u32 speed); + 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..73791b260fc5 --- /dev/null +++ b/drivers/net/ethernet/tehuti/tn40_phy.c @@ -0,0 +1,73 @@ +// 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 net_device *ndev = to_net_dev(config->dev); + struct tn40_priv *priv = netdev_priv(ndev); + + tn40_set_link_speed(priv, speed); + netif_wake_queue(priv->ndev); +} + +static void tn40_link_down(struct phylink_config *config, unsigned int mode, + phy_interface_t interface) +{ + struct net_device *ndev = to_net_dev(config->dev); + struct tn40_priv *priv = netdev_priv(ndev); + + tn40_set_link_speed(priv, 0); + netif_stop_queue(priv->ndev); +} + +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 phylink. 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 (SFP+ 10G SR) 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 | 35 ++++++++++-- drivers/net/ethernet/tehuti/tn40.h | 8 +++ drivers/net/ethernet/tehuti/tn40_phy.c | 73 ++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c