| Message ID | 20240822031945.3102130-1-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: ftgmac100: Get link speed and duplex for NC-SI | expand |
On Thu, 22 Aug 2024 11:19:45 +0800 Jacky Chou wrote: > The ethtool of this driver uses the phy API of ethtool > to get the link information from PHY driver. > Because the NC-SI is forced on 100Mbps and full duplex, > the driver connects a fixed-link phy driver for NC-SI. replace: the driver connects -> connect > The ethtool will get the link information from the > fixed-link phy driver. Hm. I defer to the PHY experts on the merits. > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > --- > v2: > - use static for struct fixed_phy_status ncsi_phy_status > - Stop phy device at net_device stop when using NC-SI. > - Start phy device at net_device start when using NC-SI. > --- > drivers/net/ethernet/faraday/ftgmac100.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index fddfd1dd5070..93862b027be0 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -26,6 +26,7 @@ > #include <linux/of_net.h> > #include <net/ip.h> > #include <net/ncsi.h> > +#include <linux/phy_fixed.h> Keep the headers sorted, put the new one after of_net.h > #include "ftgmac100.h" > > @@ -1794,6 +1805,7 @@ static int ftgmac100_probe(struct platform_device *pdev) > struct net_device *netdev; > struct ftgmac100 *priv; > struct device_node *np; > + struct phy_device *phydev; keep the variable declarations sorted longest to shortest if possible > int err = 0; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -1879,6 +1891,14 @@ static int ftgmac100_probe(struct platform_device *pdev) > err = -EINVAL; > goto err_phy_connect; > } > + > + phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); > + err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > + PHY_INTERFACE_MODE_MII); > + if (err) { > + dev_err(&pdev->dev, "Connecting PHY failed\n"); > + goto err_phy_connect; > + } Very suspicious that you register it but you never unregister it. Are you sure the error path and .remove don't need to be changed?
> > The ethtool of this driver uses the phy API of ethtool to get the link > > information from PHY driver. > > Because the NC-SI is forced on 100Mbps and full duplex, the driver > > connects a fixed-link phy driver for NC-SI. > > replace: the driver connects -> connect I will refine the commit message. > > > The ethtool will get the link information from the fixed-link phy > > driver. > > Hm. I defer to the PHY experts on the merits. > > > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > > --- > > v2: > > - use static for struct fixed_phy_status ncsi_phy_status > > - Stop phy device at net_device stop when using NC-SI. > > - Start phy device at net_device start when using NC-SI. > > --- > > drivers/net/ethernet/faraday/ftgmac100.c | 24 > > ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > b/drivers/net/ethernet/faraday/ftgmac100.c > > index fddfd1dd5070..93862b027be0 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -26,6 +26,7 @@ > > #include <linux/of_net.h> > > #include <net/ip.h> > > #include <net/ncsi.h> > > +#include <linux/phy_fixed.h> > > Keep the headers sorted, put the new one after of_net.h Agree. I will adjust the <linux/phy_fixed.h> header. > > > #include "ftgmac100.h" > > > > > @@ -1794,6 +1805,7 @@ static int ftgmac100_probe(struct platform_device > *pdev) > > struct net_device *netdev; > > struct ftgmac100 *priv; > > struct device_node *np; > > + struct phy_device *phydev; > > keep the variable declarations sorted longest to shortest if possible Agree. I will change the variable next version. > > > int err = 0; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1879,6 > > +1891,14 @@ static int ftgmac100_probe(struct platform_device *pdev) > > err = -EINVAL; > > goto err_phy_connect; > > } > > + > > + phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); > > + err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, > > + PHY_INTERFACE_MODE_MII); > > + if (err) { > > + dev_err(&pdev->dev, "Connecting PHY failed\n"); > > + goto err_phy_connect; > > + } > > Very suspicious that you register it but you never unregister it. > Are you sure the error path and .remove don't need to be changed? Agree. It needs the unregister for the PHY device. When using NC-SI to register fixed-link PHY device, I wiil add the unregister function in ftgmac100_phy_disconnect(). Thanks.
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index fddfd1dd5070..93862b027be0 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -26,6 +26,7 @@ #include <linux/of_net.h> #include <net/ip.h> #include <net/ncsi.h> +#include <linux/phy_fixed.h> #include "ftgmac100.h" @@ -50,6 +51,15 @@ #define FTGMAC_100MHZ 100000000 #define FTGMAC_25MHZ 25000000 +/* For NC-SI to register a fixed-link phy device */ +static struct fixed_phy_status ncsi_phy_status = { + .link = 1, + .speed = SPEED_100, + .duplex = DUPLEX_FULL, + .pause = 0, + .asym_pause = 0 +}; + struct ftgmac100 { /* Registers */ struct resource *res; @@ -1531,7 +1541,8 @@ static int ftgmac100_open(struct net_device *netdev) if (netdev->phydev) { /* If we have a PHY, start polling */ phy_start(netdev->phydev); - } else if (priv->use_ncsi) { + } + if (priv->use_ncsi) { /* If using NC-SI, set our carrier on and start the stack */ netif_carrier_on(netdev); @@ -1577,7 +1588,7 @@ static int ftgmac100_stop(struct net_device *netdev) netif_napi_del(&priv->napi); if (netdev->phydev) phy_stop(netdev->phydev); - else if (priv->use_ncsi) + if (priv->use_ncsi) ncsi_stop_dev(priv->ndev); ftgmac100_stop_hw(priv); @@ -1794,6 +1805,7 @@ static int ftgmac100_probe(struct platform_device *pdev) struct net_device *netdev; struct ftgmac100 *priv; struct device_node *np; + struct phy_device *phydev; int err = 0; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1879,6 +1891,14 @@ static int ftgmac100_probe(struct platform_device *pdev) err = -EINVAL; goto err_phy_connect; } + + phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL); + err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link, + PHY_INTERFACE_MODE_MII); + if (err) { + dev_err(&pdev->dev, "Connecting PHY failed\n"); + goto err_phy_connect; + } } else if (np && of_phy_is_fixed_link(np)) { struct phy_device *phy;
The ethtool of this driver uses the phy API of ethtool to get the link information from PHY driver. Because the NC-SI is forced on 100Mbps and full duplex, the driver connects a fixed-link phy driver for NC-SI. The ethtool will get the link information from the fixed-link phy driver. Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> --- v2: - use static for struct fixed_phy_status ncsi_phy_status - Stop phy device at net_device stop when using NC-SI. - Start phy device at net_device start when using NC-SI. --- drivers/net/ethernet/faraday/ftgmac100.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)