| Message ID | 20240731094245.1967834-9-shaojijie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support of HIBMCGE Ethernet Driver | expand |
> +static void hbg_ethtool_get_drvinfo(struct net_device *netdev, > + struct ethtool_drvinfo *drvinfo) > +{ > + strscpy(drvinfo->version, HBG_MOD_VERSION, sizeof(drvinfo->version)); > + drvinfo->version[sizeof(drvinfo->version) - 1] = '\0'; A version is pointless, it tells you nothing useful. If you don't touch version, the core will fill it with the uname, so you know exactly what kernel this is, which is useful. > +static u32 hbg_ethtool_get_link(struct net_device *netdev) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + > + return priv->mac.link_status; > +} > + > +static int hbg_ethtool_get_ksettings(struct net_device *netdev, > + struct ethtool_link_ksettings *ksettings) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + > + phy_ethtool_ksettings_get(priv->mac.phydev, ksettings); You can avoid this wrapper since phy_attach_direct sets netdev->phydev to phydev. You can then call phy_ethtool_get_link_ksettings() etc. > +static int hbg_ethtool_set_ksettings(struct net_device *netdev, > + const struct ethtool_link_ksettings *cmd) > +{ > + struct hbg_priv *priv = netdev_priv(netdev); > + > + if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF) > + return -EINVAL; So long as you have told phylib about not supporting 1000Base/Half, phy_ethtool_set_link_ksettings() will return an error for you. > +static const struct ethtool_ops hbg_ethtool_ops = { > + .get_drvinfo = hbg_ethtool_get_drvinfo, > + .get_link = hbg_ethtool_get_link, Why not ethtool_op_get_link() ? > + .get_link_ksettings = hbg_ethtool_get_ksettings, > + .set_link_ksettings = hbg_ethtool_set_ksettings, > +}; > +static void hbg_update_link_status(struct hbg_priv *priv) > +{ > + u32 link; > + > + link = hbg_get_link_status(priv); > + if (link == priv->mac.link_status) > + return; > + > + priv->mac.link_status = link; > + if (link == HBG_LINK_DOWN) { > + netif_carrier_off(priv->netdev); > + netif_tx_stop_all_queues(priv->netdev); > + dev_info(&priv->pdev->dev, "link down!"); > + } else { > + netif_tx_wake_all_queues(priv->netdev); > + netif_carrier_on(priv->netdev); > + dev_info(&priv->pdev->dev, "link up!"); > + } > +} Why do you need this? phylib will poll the PHY once per second and call the adjust_link callback whenever the link changes state. > @@ -177,12 +226,17 @@ static int hbg_init(struct net_device *netdev) > ret = hbg_irq_init(priv); > if (ret) > return ret; > - > ret = devm_add_action_or_reset(&priv->pdev->dev, hbg_irq_uninit, priv); This white space change does not belong here. Andrew
on 2024/8/1 9:10, Andrew Lunn wrote: >> +static void hbg_ethtool_get_drvinfo(struct net_device *netdev, >> + struct ethtool_drvinfo *drvinfo) >> +{ >> + strscpy(drvinfo->version, HBG_MOD_VERSION, sizeof(drvinfo->version)); >> + drvinfo->version[sizeof(drvinfo->version) - 1] = '\0'; > A version is pointless, it tells you nothing useful. If you don't > touch version, the core will fill it with the uname, so you know > exactly what kernel this is, which is useful. > >> +static u32 hbg_ethtool_get_link(struct net_device *netdev) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + >> + return priv->mac.link_status; >> +} >> + >> +static int hbg_ethtool_get_ksettings(struct net_device *netdev, >> + struct ethtool_link_ksettings *ksettings) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + >> + phy_ethtool_ksettings_get(priv->mac.phydev, ksettings); > You can avoid this wrapper since phy_attach_direct sets netdev->phydev > to phydev. You can then call phy_ethtool_get_link_ksettings() etc. Yes, It`s ok > >> +static int hbg_ethtool_set_ksettings(struct net_device *netdev, >> + const struct ethtool_link_ksettings *cmd) >> +{ >> + struct hbg_priv *priv = netdev_priv(netdev); >> + >> + if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF) >> + return -EINVAL; > So long as you have told phylib about not supporting 1000Base/Half, > phy_ethtool_set_link_ksettings() will return an error for you. okay, > >> +static const struct ethtool_ops hbg_ethtool_ops = { >> + .get_drvinfo = hbg_ethtool_get_drvinfo, >> + .get_link = hbg_ethtool_get_link, > Why not ethtool_op_get_link() ? It`s a good idea > >> + .get_link_ksettings = hbg_ethtool_get_ksettings, >> + .set_link_ksettings = hbg_ethtool_set_ksettings, >> +}; >> +static void hbg_update_link_status(struct hbg_priv *priv) >> +{ >> + u32 link; >> + >> + link = hbg_get_link_status(priv); >> + if (link == priv->mac.link_status) >> + return; >> + >> + priv->mac.link_status = link; >> + if (link == HBG_LINK_DOWN) { >> + netif_carrier_off(priv->netdev); >> + netif_tx_stop_all_queues(priv->netdev); >> + dev_info(&priv->pdev->dev, "link down!"); >> + } else { >> + netif_tx_wake_all_queues(priv->netdev); >> + netif_carrier_on(priv->netdev); >> + dev_info(&priv->pdev->dev, "link up!"); >> + } >> +} > Why do you need this? phylib will poll the PHY once per second and > call the adjust_link callback whenever the link changes state. However, we hope that the network port can be linked only when the PHY and MAC are linked. The adjust_link callback can ensure that the PHY status is normal, but cannot ensure that the MAC address is linked. so, in hbg_get_link_status: +/* include phy link and mac link */ +u32 hbg_get_link_status(struct hbg_priv *priv) +{ + struct phy_device *phydev = priv->mac.phydev; + int ret; + + if (!phydev) + return HBG_LINK_DOWN; + + phy_read_status(phydev); + if ((phydev->state != PHY_UP && phydev->state != PHY_RUNNING) || + !phydev->link) + return HBG_LINK_DOWN; + + ret = hbg_hw_sgmii_autoneg(priv); + if (ret) + return HBG_LINK_DOWN; + + return HBG_LINK_UP; +} > >> @@ -177,12 +226,17 @@ static int hbg_init(struct net_device *netdev) >> ret = hbg_irq_init(priv); >> if (ret) >> return ret; >> - >> ret = devm_add_action_or_reset(&priv->pdev->dev, hbg_irq_uninit, priv); > This white space change does not belong here. Yes, I'll fix that in V2 Thanks again, Jijie Shao
> > Why do you need this? phylib will poll the PHY once per second and > > call the adjust_link callback whenever the link changes state. > > However, we hope that the network port can be linked only when > the PHY and MAC are linked. > The adjust_link callback can ensure that the PHY status is normal, > but cannot ensure that the MAC address is linked. So why would the SGMII link be down? My experience with SGMII is that the link comes up as soon as both ends have power. You are also not using in-band signalling, you configure the MAC based on the adjust_link callback. Basically, whenever you do something which no other driver does, you need to explain why. Do you see any other MAC driver using SGMII doing this? Andrew
on 2024/8/1 20:26, Andrew Lunn wrote: >>> Why do you need this? phylib will poll the PHY once per second and >>> call the adjust_link callback whenever the link changes state. >> However, we hope that the network port can be linked only when >> the PHY and MAC are linked. >> The adjust_link callback can ensure that the PHY status is normal, >> but cannot ensure that the MAC address is linked. > So why would the SGMII link be down? My experience with SGMII is that > the link comes up as soon as both ends have power. You are also not > using in-band signalling, you configure the MAC based on the > adjust_link callback. > > Basically, whenever you do something which no other driver does, you > need to explain why. Do you see any other MAC driver using SGMII doing > this? > > Andrew Yes, it was my mistake, I should explain why. If the network port is linked, but the link fails between the SGMII and PHY, is there any method to find out the cause? I've had a problem with phy link but SGMII no link due to poor contact. In this case, the network port no link. Therefore, we can quickly find and analyze the cause. Or maybe we shouldn't think about the case. because the link is up but packets cannot be received or sent. Thanks Jijie Shao
> If the network port is linked, but the link fails between the SGMII and PHY, > is there any method to find out the cause? > > I've had a problem with phy link but SGMII no link due to poor contact. So a hardware design issue? Has it been solved now, or is there shipped hardware with this problem? I would say there is a difference between the first prototype board, and real products. If the real product has issues, then we should try to address it. If it is just a prototype board, i would ignore it. One option might be to expose your PCS as a phylib PCS. Do you get interrupts from it when it has link up? Link down? phylink will then combine media side status with PCS status. Andrew
diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h index 25563af04897..45ec4b463e70 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h @@ -146,6 +146,7 @@ struct hbg_priv { struct hbg_vector vectors; struct hbg_ring tx_ring; struct hbg_ring rx_ring; + struct delayed_work service_task; }; #endif diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c new file mode 100644 index 000000000000..3acd6eae189e --- /dev/null +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: GPL-2.0+ +// Copyright (c) 2024 Hisilicon Limited. + +#include <linux/ethtool.h> +#include <linux/phy.h> +#include "hbg_common.h" +#include "hbg_ethtool.h" +#include "hbg_hw.h" +#include "hbg_main.h" +#include "hbg_mdio.h" + +static void hbg_ethtool_get_drvinfo(struct net_device *netdev, + struct ethtool_drvinfo *drvinfo) +{ + strscpy(drvinfo->version, HBG_MOD_VERSION, sizeof(drvinfo->version)); + drvinfo->version[sizeof(drvinfo->version) - 1] = '\0'; +} + +static u32 hbg_ethtool_get_link(struct net_device *netdev) +{ + struct hbg_priv *priv = netdev_priv(netdev); + + return priv->mac.link_status; +} + +static int hbg_ethtool_get_ksettings(struct net_device *netdev, + struct ethtool_link_ksettings *ksettings) +{ + struct hbg_priv *priv = netdev_priv(netdev); + + phy_ethtool_ksettings_get(priv->mac.phydev, ksettings); + return 0; +} + +static int hbg_ethtool_set_ksettings(struct net_device *netdev, + const struct ethtool_link_ksettings *cmd) +{ + struct hbg_priv *priv = netdev_priv(netdev); + + if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF) + return -EINVAL; + + return phy_ethtool_ksettings_set(priv->mac.phydev, cmd); +} + +static const struct ethtool_ops hbg_ethtool_ops = { + .get_drvinfo = hbg_ethtool_get_drvinfo, + .get_link = hbg_ethtool_get_link, + .get_link_ksettings = hbg_ethtool_get_ksettings, + .set_link_ksettings = hbg_ethtool_set_ksettings, +}; + +void hbg_ethtool_set_ops(struct net_device *netdev) +{ + netdev->ethtool_ops = &hbg_ethtool_ops; +} diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.h new file mode 100644 index 000000000000..628707ec2686 --- /dev/null +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* Copyright (c) 2024 Hisilicon Limited. */ + +#ifndef __HBG_ETHTOOL_H +#define __HBG_ETHTOOL_H + +#include <linux/netdevice.h> + +void hbg_ethtool_set_ops(struct net_device *netdev); + +#endif diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c index bb5f8321da8a..bea596123c37 100644 --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_main.c @@ -6,12 +6,15 @@ #include <linux/netdevice.h> #include <linux/pci.h> #include "hbg_common.h" +#include "hbg_ethtool.h" #include "hbg_hw.h" #include "hbg_irq.h" #include "hbg_main.h" #include "hbg_mdio.h" #include "hbg_txrx.h" +static struct workqueue_struct *hbg_workqueue; + static void hbg_enable_intr(struct hbg_priv *priv, bool enabled) { u32 i; @@ -136,6 +139,52 @@ static const struct net_device_ops hbg_netdev_ops = { .ndo_tx_timeout = hbg_net_tx_timeout, }; +static void hbg_update_link_status(struct hbg_priv *priv) +{ + u32 link; + + link = hbg_get_link_status(priv); + if (link == priv->mac.link_status) + return; + + priv->mac.link_status = link; + if (link == HBG_LINK_DOWN) { + netif_carrier_off(priv->netdev); + netif_tx_stop_all_queues(priv->netdev); + dev_info(&priv->pdev->dev, "link down!"); + } else { + netif_tx_wake_all_queues(priv->netdev); + netif_carrier_on(priv->netdev); + dev_info(&priv->pdev->dev, "link up!"); + } +} + +static void hbg_service_task(struct work_struct *work) +{ + struct hbg_priv *priv = + container_of(work, struct hbg_priv, service_task.work); + + hbg_update_link_status(priv); + + mod_delayed_work(hbg_workqueue, &priv->service_task, + round_jiffies_relative(HZ)); +} + +static void hbg_delaywork_init(struct hbg_priv *priv) +{ + INIT_DELAYED_WORK(&priv->service_task, hbg_service_task); + mod_delayed_work(hbg_workqueue, &priv->service_task, + round_jiffies_relative(HZ)); +} + +static void hbg_delaywork_uninit(void *data) +{ + struct hbg_priv *priv = data; + + if (priv->service_task.work.func) + cancel_delayed_work_sync(&priv->service_task); +} + static const u32 hbg_mode_ability[] = { ETHTOOL_LINK_MODE_1000baseT_Full_BIT, ETHTOOL_LINK_MODE_100baseT_Full_BIT, @@ -177,12 +226,17 @@ static int hbg_init(struct net_device *netdev) ret = hbg_irq_init(priv); if (ret) return ret; - ret = devm_add_action_or_reset(&priv->pdev->dev, hbg_irq_uninit, priv); if (ret) return ret; - return hbg_mac_init(priv); + ret = hbg_mac_init(priv); + if (ret) + return ret; + + hbg_delaywork_init(priv); + return devm_add_action_or_reset(&priv->pdev->dev, + hbg_delaywork_uninit, priv); } static int hbg_pci_init(struct pci_dev *pdev) @@ -249,6 +303,7 @@ static int hbg_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return dev_err_probe(&pdev->dev, ret, "failed to register netdev\n"); + hbg_ethtool_set_ops(netdev); set_bit(HBG_NIC_STATE_INITED, &priv->state); return 0; } @@ -264,7 +319,42 @@ static struct pci_driver hbg_driver = { .id_table = hbg_pci_tbl, .probe = hbg_probe, }; -module_pci_driver(hbg_driver); + +static int __init hbg_module_init(void) +{ + int ret; + + hbg_workqueue = alloc_workqueue("%s", WQ_UNBOUND, 0, HBG_DEV_NAME); + if (!hbg_workqueue) { + pr_err("%s: failed to create workqueue\n", HBG_DEV_NAME); + return -ENOMEM; + } + + ret = pci_register_driver(&hbg_driver); + if (ret) { + pr_err("%s: failed to register PCI driver, ret = %d\n", + HBG_DEV_NAME, ret); + goto err_destroy_workqueue; + } + + return 0; + +err_destroy_workqueue: + destroy_workqueue(hbg_workqueue); + hbg_workqueue = NULL; + + return ret; +} +module_init(hbg_module_init); + +static void __exit hbg_module_exit(void) +{ + pci_unregister_driver(&hbg_driver); + flush_workqueue(hbg_workqueue); + destroy_workqueue(hbg_workqueue); + hbg_workqueue = NULL; +} +module_exit(hbg_module_exit); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Huawei Tech. Co., Ltd.");
Implement a workqueue in this module, The workqueue is invoked once every second to update link status. Implement the .get_drvinfo .get_link .get_link_ksettings to get the basic information and working status of the driver. Implement the .set_link_ksettings to modify the rate, duplex, and auto-negotiation status. Signed-off-by: Jijie Shao <shaojijie@huawei.com> --- .../ethernet/hisilicon/hibmcge/hbg_common.h | 1 + .../ethernet/hisilicon/hibmcge/hbg_ethtool.c | 56 +++++++++++ .../ethernet/hisilicon/hibmcge/hbg_ethtool.h | 11 +++ .../net/ethernet/hisilicon/hibmcge/hbg_main.c | 96 ++++++++++++++++++- 4 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.c create mode 100644 drivers/net/ethernet/hisilicon/hibmcge/hbg_ethtool.h