[PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

[PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

[Raghuram Chary J]

Adding Fixed PHY support to the lan78xx driver.

Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@microchip.com>
---
v0->v1:
   * Remove driver version #define
   * Modify netdev_info to netdev_dbg
   * Move lan7801 specific to new routine and add switch case
   * Minor cleanup

v1->v2:
   * Removed fixedphy variable and used phy_is_pseudo_fixed_link() check.
---
 drivers/net/usb/Kconfig   |   1 +
 drivers/net/usb/lan78xx.c | 111 +++++++++++++++++++++++++++++++---------------
 2 files changed, 77 insertions(+), 35 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index f28bd74ac275..418b0904cecb 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -111,6 +111,7 @@ config USB_LAN78XX
 	select MII
 	select PHYLIB
 	select MICROCHIP_PHY
+	select FIXED_PHY
 	help
 	  This option adds support for Microchip LAN78XX based USB 2
 	  & USB 3 10/100/1000 Ethernet adapters.
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 0867f7275852..48925ed71555 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -36,13 +36,12 @@
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/microchipphy.h>
-#include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include "lan78xx.h"
 
 #define DRIVER_AUTHOR	"WOOJUNG HUH <woojung.huh@microchip.com>"
 #define DRIVER_DESC	"LAN78XX USB 3.0 Gigabit Ethernet Devices"
 #define DRIVER_NAME	"lan78xx"
-#define DRIVER_VERSION	"1.0.6"
 
 #define TX_TIMEOUT_JIFFIES		(5 * HZ)
 #define THROTTLE_JIFFIES		(HZ / 8)
@@ -1477,7 +1476,6 @@ static void lan78xx_get_drvinfo(struct net_device *net,
 	struct lan78xx_net *dev = netdev_priv(net);
 
 	strncpy(info->driver, DRIVER_NAME, sizeof(info->driver));
-	strncpy(info->version, DRIVER_VERSION, sizeof(info->version));
 	usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
 }
 
@@ -2003,52 +2001,90 @@ static int ksz9031rnx_fixup(struct phy_device *phydev)
 	return 1;
 }
 
-static int lan78xx_phy_init(struct lan78xx_net *dev)
+static int lan7801_phy_init(struct phy_device **phydev,
+			    struct lan78xx_net *dev)
 {
+	u32 buf;
 	int ret;
-	u32 mii_adv;
-	struct phy_device *phydev;
-
-	phydev = phy_find_first(dev->mdiobus);
-	if (!phydev) {
-		netdev_err(dev->net, "no PHY found\n");
-		return -EIO;
-	}
-
-	if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
-	    (dev->chipid == ID_REV_CHIP_ID_7850_)) {
-		phydev->is_internal = true;
-		dev->interface = PHY_INTERFACE_MODE_GMII;
-
-	} else if (dev->chipid == ID_REV_CHIP_ID_7801_) {
-		if (!phydev->drv) {
+	struct fixed_phy_status fphy_status = {
+		.link = 1,
+		.speed = SPEED_1000,
+		.duplex = DUPLEX_FULL,
+	};
+
+	if (!*phydev) {
+		netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
+		*phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1,
+					     NULL);
+		if (IS_ERR(*phydev)) {
+			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
+			return -ENODEV;
+		}
+		netdev_dbg(dev->net, "Registered FIXED PHY\n");
+		dev->interface = PHY_INTERFACE_MODE_RGMII;
+		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
+					MAC_RGMII_ID_TXC_DELAY_EN_);
+		ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
+		ret = lan78xx_read_reg(dev, HW_CFG, &buf);
+		buf |= HW_CFG_CLK125_EN_;
+		buf |= HW_CFG_REFCLK25_EN_;
+		ret = lan78xx_write_reg(dev, HW_CFG, buf);
+	} else {
+		if (!(*phydev)->drv) {
 			netdev_err(dev->net, "no PHY driver found\n");
 			return -EIO;
 		}
-
 		dev->interface = PHY_INTERFACE_MODE_RGMII;
-
 		/* external PHY fixup for KSZ9031RNX */
 		ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
 						 ksz9031rnx_fixup);
 		if (ret < 0) {
-			netdev_err(dev->net, "fail to register fixup\n");
+			netdev_err(dev->net, "fail to register fixup PHY_KSZ9031RNX\n");
 			return ret;
 		}
 		/* external PHY fixup for LAN8835 */
 		ret = phy_register_fixup_for_uid(PHY_LAN8835, 0xfffffff0,
 						 lan8835_fixup);
 		if (ret < 0) {
-			netdev_err(dev->net, "fail to register fixup\n");
+			netdev_err(dev->net, "fail to register fixup for PHY_LAN8835\n");
 			return ret;
 		}
 		/* add more external PHY fixup here if needed */
 
-		phydev->is_internal = false;
-	} else {
-		netdev_err(dev->net, "unknown ID found\n");
-		ret = -EIO;
-		goto error;
+		(*phydev)->is_internal = false;
+	}
+	return 0;
+}
+
+static int lan78xx_phy_init(struct lan78xx_net *dev)
+{
+	int ret;
+	u32 mii_adv;
+	struct phy_device *phydev;
+
+	phydev = phy_find_first(dev->mdiobus);
+	switch (dev->chipid) {
+	case ID_REV_CHIP_ID_7801_:
+		ret = lan7801_phy_init(&phydev, dev);
+		if (ret < 0) {
+			netdev_err(dev->net, "lan7801: PHY Init Failed");
+			return ret;
+		}
+		break;
+
+	case ID_REV_CHIP_ID_7800_:
+	case ID_REV_CHIP_ID_7850_:
+		if (!phydev) {
+			netdev_err(dev->net, "no PHY found\n");
+			return -EIO;
+		}
+		phydev->is_internal = true;
+		dev->interface = PHY_INTERFACE_MODE_GMII;
+		break;
+
+	default:
+		netdev_err(dev->net, "Unknown CHIP ID found\n");
+		return -EIO;
 	}
 
 	/* if phyirq is not set, use polling mode in phylib */
@@ -2067,6 +2103,12 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	if (ret) {
 		netdev_err(dev->net, "can't attach PHY to %s\n",
 			   dev->mdiobus->id);
+		if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+			phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
+						     0xfffffff0);
+			phy_unregister_fixup_for_uid(PHY_LAN8835,
+						     0xfffffff0);
+		}
 		return -EIO;
 	}
 
@@ -2084,12 +2126,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	dev->fc_autoneg = phydev->autoneg;
 
 	return 0;
-
-error:
-	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
-	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
-
-	return ret;
 }
 
 static int lan78xx_set_rx_max_frame_length(struct lan78xx_net *dev, int size)
@@ -3487,6 +3523,7 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 	struct lan78xx_net		*dev;
 	struct usb_device		*udev;
 	struct net_device		*net;
+	struct phy_device		*phydev;
 
 	dev = usb_get_intfdata(intf);
 	usb_set_intfdata(intf, NULL);
@@ -3495,12 +3532,16 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 
 	udev = interface_to_usbdev(intf);
 	net = dev->net;
+	phydev = net->phydev;
 
 	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
 	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
 
 	phy_disconnect(net->phydev);
 
+	if (phy_is_pseudo_fixed_link(phydev))
+		fixed_phy_unregister(phydev);
+
 	unregister_netdev(net);
 
 	cancel_delayed_work_sync(&dev->wq);
-- 
2.16.2

Re: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

[Florian Fainelli]

On 04/25/2018 12:07 PM, Raghuram Chary J wrote:
> Adding Fixed PHY support to the lan78xx driver.
> 
> Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@microchip.com>
> ---
> v0->v1:
>    * Remove driver version #define

This should be a separate patch of its own, more comment below.

>    * Modify netdev_info to netdev_dbg
>    * Move lan7801 specific to new routine and add switch case
>    * Minor cleanup

> -static int lan78xx_phy_init(struct lan78xx_net *dev)
> +static int lan7801_phy_init(struct phy_device **phydev,
> +			    struct lan78xx_net *dev)

Passing a **phydev looks really unnecessary here, why don't just return
a struct phy_device * as a return argument here? If you need to
communicate a specific return value, you can use ERR_PTR()/PTR_ERR() for
that purpose.

>  {
> +	u32 buf;
>  	int ret;
> -	u32 mii_adv;
> -	struct phy_device *phydev;
> -
> -	phydev = phy_find_first(dev->mdiobus);
> -	if (!phydev) {
> -		netdev_err(dev->net, "no PHY found\n");
> -		return -EIO;
> -	}
> -
> -	if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
> -	    (dev->chipid == ID_REV_CHIP_ID_7850_)) {
> -		phydev->is_internal = true;
> -		dev->interface = PHY_INTERFACE_MODE_GMII;
> -
> -	} else if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> -		if (!phydev->drv) {
> +	struct fixed_phy_status fphy_status = {
> +		.link = 1,
> +		.speed = SPEED_1000,
> +		.duplex = DUPLEX_FULL,
> +	};
> +
> +	if (!*phydev) {
> +		netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> +		*phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1,
> +					     NULL);
> +		if (IS_ERR(*phydev)) {
> +			netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> +			return -ENODEV;
> +		}
> +		netdev_dbg(dev->net, "Registered FIXED PHY\n");
> +		dev->interface = PHY_INTERFACE_MODE_RGMII;
> +		ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> +					MAC_RGMII_ID_TXC_DELAY_EN_);
> +		ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
> +		ret = lan78xx_read_reg(dev, HW_CFG, &buf);
> +		buf |= HW_CFG_CLK125_EN_;
> +		buf |= HW_CFG_REFCLK25_EN_;
> +		ret = lan78xx_write_reg(dev, HW_CFG, buf);
> +	} else {
> +		if (!(*phydev)->drv) {
>  			netdev_err(dev->net, "no PHY driver found\n");
>  			return -EIO;
>  		}
> -
>  		dev->interface = PHY_INTERFACE_MODE_RGMII;
> -
>  		/* external PHY fixup for KSZ9031RNX */
>  		ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
>  						 ksz9031rnx_fixup);
>  		if (ret < 0) {
> -			netdev_err(dev->net, "fail to register fixup\n");
> +			netdev_err(dev->net, "fail to register fixup PHY_KSZ9031RNX\n");

Unrelated message change, that should be a separate commit.

>  			return ret;
>  		}
>  		/* external PHY fixup for LAN8835 */
>  		ret = phy_register_fixup_for_uid(PHY_LAN8835, 0xfffffff0,
>  						 lan8835_fixup);
>  		if (ret < 0) {
> -			netdev_err(dev->net, "fail to register fixup\n");
> +			netdev_err(dev->net, "fail to register fixup for PHY_LAN8835\n");

Same here, can be grouped in the same commit as the one right above.

>  			return ret;
>  		}
>  		/* add more external PHY fixup here if needed */
>  
> -		phydev->is_internal = false;


> -	} else {
> -		netdev_err(dev->net, "unknown ID found\n");
> -		ret = -EIO;
> -		goto error;
> +		(*phydev)->is_internal = false;
> +	}
> +	return 0;
> +}
> +
> +static int lan78xx_phy_init(struct lan78xx_net *dev)
> +{
> +	int ret;
> +	u32 mii_adv;
> +	struct phy_device *phydev;
> +
> +	phydev = phy_find_first(dev->mdiobus);
> +	switch (dev->chipid) {
> +	case ID_REV_CHIP_ID_7801_:
> +		ret = lan7801_phy_init(&phydev, dev);
> +		if (ret < 0) {
> +			netdev_err(dev->net, "lan7801: PHY Init Failed");
> +			return ret;
> +		}
> +		break;
> +
> +	case ID_REV_CHIP_ID_7800_:
> +	case ID_REV_CHIP_ID_7850_:
> +		if (!phydev) {
> +			netdev_err(dev->net, "no PHY found\n");
> +			return -EIO;
> +		}
> +		phydev->is_internal = true;

Uggh you are not really supposed to set that, the matching PHY driver
should have PHY_IS_INTERNAL in the .flags member to indicate that to the
core PHY library.

> +		dev->interface = PHY_INTERFACE_MODE_GMII;
> +		break;
> +
> +	default:
> +		netdev_err(dev->net, "Unknown CHIP ID found\n");
> +		return -EIO;
>  	}
>  
>  	/* if phyirq is not set, use polling mode in phylib */
> @@ -2067,6 +2103,12 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  	if (ret) {
>  		netdev_err(dev->net, "can't attach PHY to %s\n",
>  			   dev->mdiobus->id);
> +		if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> +			phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> +						     0xfffffff0);
> +			phy_unregister_fixup_for_uid(PHY_LAN8835,
> +						     0xfffffff0);
> +		}
>  		return -EIO;
>  	}
>  
> @@ -2084,12 +2126,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  	dev->fc_autoneg = phydev->autoneg;
>  
>  	return 0;
> -
> -error:
> -	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
> -	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
> -
> -	return ret;
>  }
>  
>  static int lan78xx_set_rx_max_frame_length(struct lan78xx_net *dev, int size)
> @@ -3487,6 +3523,7 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>  	struct lan78xx_net		*dev;
>  	struct usb_device		*udev;
>  	struct net_device		*net;
> +	struct phy_device		*phydev;
>  
>  	dev = usb_get_intfdata(intf);
>  	usb_set_intfdata(intf, NULL);
> @@ -3495,12 +3532,16 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>  
>  	udev = interface_to_usbdev(intf);
>  	net = dev->net;
> +	phydev = net->phydev;
>  
>  	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
>  	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
>  
>  	phy_disconnect(net->phydev);
>  
> +	if (phy_is_pseudo_fixed_link(phydev))
> +		fixed_phy_unregister(phydev);

That part now looks good.

> +
>  	unregister_netdev(net);
>  
>  	cancel_delayed_work_sync(&dev->wq);
> 


-- 
Florian

RE: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

[RaghuramChary.Jallipalli]

Hi Florian,

> > v0->v1:
> >    * Remove driver version #define
> 
> This should be a separate patch of its own, more comment below.
> 
OK. Patch should be for net, correct?
 
> > -static int lan78xx_phy_init(struct lan78xx_net *dev)
> > +static int lan7801_phy_init(struct phy_device **phydev,
> > +			    struct lan78xx_net *dev)
> 
> Passing a **phydev looks really unnecessary here, why don't just return a
> struct phy_device * as a return argument here? If you need to communicate
> a specific return value, you can use ERR_PTR()/PTR_ERR() for that purpose.
> 
Done.

> >  		if (ret < 0) {
> > -			netdev_err(dev->net, "fail to register fixup\n");
> > +			netdev_err(dev->net, "fail to register fixup
> PHY_KSZ9031RNX\n");
> 
> Unrelated message change, that should be a separate commit.

OK.

> > +		phydev->is_internal = true;
> 
> Uggh you are not really supposed to set that, the matching PHY driver should
> have PHY_IS_INTERNAL in the .flags member to indicate that to the core PHY
> library.
>
OK. Will handle this too in separate patch/commit for net. 

Thanks,
Raghu
 

Re: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY

[Andrew Lunn]

On Thu, Apr 26, 2018 at 06:27:57AM +0000, RaghuramChary.Jallipalli@microchip.com wrote:
> Hi Florian,
> 
> > > v0->v1:
> > >    * Remove driver version #define
> > 
> > This should be a separate patch of its own, more comment below.
> > 
> OK. Patch should be for net, correct?

net-next. As far as i can see, none of this is a fix.

	  Andrew