[PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

[PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

[Jacky Chou]

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(-)

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;
 
-- 
2.25.1

Re: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

[Jakub Kicinski]

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?
-- 
pw-bot: cr

RE: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

[Jacky Chou]

> > 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.