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

[PATCH 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>
---
 drivers/net/usb/Kconfig   |  1 +
 drivers/net/usb/lan78xx.c | 43 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 40 insertions(+), 4 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 207a3e18c08f..0d52f37c6cf4 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -36,13 +36,13 @@
 #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 DRIVER_VERSION	"1.0.7"
 
 #define TX_TIMEOUT_JIFFIES		(5 * HZ)
 #define THROTTLE_JIFFIES		(HZ / 8)
@@ -426,6 +426,7 @@ struct lan78xx_net {
 	struct statstage	stats;
 
 	struct irq_domain_data	domain_data;
+	struct phy_device	*fixedphy;
 };
 
 /* define external phy id */
@@ -2062,11 +2063,39 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 	int ret;
 	u32 mii_adv;
 	struct phy_device *phydev;
+	struct fixed_phy_status fphy_status = {
+		.link = 1,
+		.speed = SPEED_1000,
+		.duplex = DUPLEX_FULL,
+	};
 
 	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_7801_) {
+			u32 buf;
+
+			netdev_info(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_info(dev->net, "Registered FIXED PHY\n");
+			dev->interface = PHY_INTERFACE_MODE_RGMII;
+			dev->fixedphy = phydev;
+			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);
+			goto phyinit;
+		} else {
+			netdev_err(dev->net, "no PHY found\n");
+			return -EIO;
+		}
 	}
 
 	if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
@@ -2105,6 +2134,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 		goto error;
 	}
 
+phyinit:
 	/* if phyirq is not set, use polling mode in phylib */
 	if (dev->domain_data.phyirq > 0)
 		phydev->irq = dev->domain_data.phyirq;
@@ -3555,6 +3585,11 @@ static void lan78xx_disconnect(struct usb_interface *intf)
 
 	phy_disconnect(net->phydev);
 
+	if (dev->fixedphy) {
+		fixed_phy_unregister(dev->fixedphy);
+		dev->fixedphy = NULL;
+	}
+
 	unregister_netdev(net);
 
 	cancel_delayed_work_sync(&dev->wq);
-- 
2.16.2

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

[Andrew Lunn]

>  #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 DRIVER_VERSION	"1.0.7"

Hi Raghuram

Driver version strings a pretty pointless. You might want to remove
it.

>  
>  #define TX_TIMEOUT_JIFFIES		(5 * HZ)
>  #define THROTTLE_JIFFIES		(HZ / 8)
> @@ -426,6 +426,7 @@ struct lan78xx_net {
>  	struct statstage	stats;
>  
>  	struct irq_domain_data	domain_data;
> +	struct phy_device	*fixedphy;
>  };
>  
>  /* define external phy id */
> @@ -2062,11 +2063,39 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  	int ret;
>  	u32 mii_adv;
>  	struct phy_device *phydev;
> +	struct fixed_phy_status fphy_status = {
> +		.link = 1,
> +		.speed = SPEED_1000,
> +		.duplex = DUPLEX_FULL,
> +	};
>  
>  	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_7801_) {
> +			u32 buf;
> +
> +			netdev_info(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_info(dev->net, "Registered FIXED PHY\n");

There are too many detdev_info() messages here. Maybe make them both
netdev_dbg().

> +			dev->interface = PHY_INTERFACE_MODE_RGMII;
> +			dev->fixedphy = phydev;

You can use 

if (!phy_is_pseudo_fixed_link(phydev))

to determine is a PHY is a fixed phy. I think you can then do without
dev->fixedphy.

> +			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);
> +			goto phyinit;

Please don't use a goto like this. Maybe turn this into a switch statement?

> +		} else {
> +			netdev_err(dev->net, "no PHY found\n");
> +			return -EIO;
> +		}
>  	}
>  
>  	if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
> @@ -2105,6 +2134,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		goto error;

Please take a look at what happens at error: It does not look
correct. Probably now is a good time to refactor the whole of lan78xx_phy_init()

	 Andrew

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

[RaghuramChary.Jallipalli]

Hi Andrew,

> > +#define DRIVER_VERSION	"1.0.7"
> 
> Hi Raghuram
> 
> Driver version strings a pretty pointless. You might want to remove it.
>
OK, will remove it.
 
> > +			netdev_info(dev->net, "Registered FIXED PHY\n");
> 
> There are too many detdev_info() messages here. Maybe make them both
> netdev_dbg().
>

OK. Will modify to netdev_dbg()
 
> > +			dev->interface = PHY_INTERFACE_MODE_RGMII;
> > +			dev->fixedphy = phydev;
> 
> You can use
> 
> if (!phy_is_pseudo_fixed_link(phydev))
> 
> to determine is a PHY is a fixed phy. I think you can then do without
> dev->fixedphy.
> 
dev->fixedphy stores the fixed phydev, which will be passed to the 
fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check is not necessary.

> > +			ret = lan78xx_write_reg(dev, HW_CFG, buf);
> > +			goto phyinit;
> 
> Please don't use a goto like this. Maybe turn this into a switch statement?
>
OK. Will modify.
 
> > static int lan78xx_phy_init(struct lan78xx_net *dev)
> >  		goto error;
> 
> Please take a look at what happens at error: It does not look correct.
> Probably now is a good time to refactor the whole of lan78xx_phy_init()
> 

OK. Will take care.

Thanks,
-Raghu

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

[Andrew Lunn]

> OK. Will modify to netdev_dbg()
>  
> > > +			dev->interface = PHY_INTERFACE_MODE_RGMII;
> > > +			dev->fixedphy = phydev;
> > 
> > You can use
> > 
> > if (!phy_is_pseudo_fixed_link(phydev))
> > 
> > to determine is a PHY is a fixed phy. I think you can then do without
> > dev->fixedphy.
> > 
> dev->fixedphy stores the fixed phydev, which will be passed to the 
> fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check is not necessary.

I'm saying you can get rid of dev->fixedphy, and just use
netdev->phydev, and phy_is_pseudo_fixed_link(netdev->phydev)

	Andrew

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

[RaghuramChary.Jallipalli]

Hi Andrew,
> > >
> > dev->fixedphy stores the fixed phydev, which will be passed to the
> > fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check
> is not necessary.
> 
> I'm saying you can get rid of dev->fixedphy, and just use
> netdev->phydev, and phy_is_pseudo_fixed_link(netdev->phydev)
> 
After phy_disconnect() , the netdev->phydev becomes null, but the phydev->mdio instances
are still valid. So I'm saving the phydev ptr and passing to unregister the fixed phy.
If I try to unregister first and disconnect, I see panic at sysfs remove link. 
I believe having dev->fixedphy should not cause any problem.

Thanks,
Raghu

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

[Florian Fainelli]

On 04/25/2018 10:04 AM, RaghuramChary.Jallipalli@microchip.com wrote:
> Hi Andrew,
>>>>
>>> dev->fixedphy stores the fixed phydev, which will be passed to the
>>> fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check
>> is not necessary.
>>
>> I'm saying you can get rid of dev->fixedphy, and just use
>> netdev->phydev, and phy_is_pseudo_fixed_link(netdev->phydev)
>>
> After phy_disconnect() , the netdev->phydev becomes null, but the phydev->mdio instances
> are still valid. So I'm saving the phydev ptr and passing to unregister the fixed phy.
> If I try to unregister first and disconnect, I see panic at sysfs remove link. 
> I believe having dev->fixedphy should not cause any problem.

It still is completely unnecessary, you can do something like the following:

	struct phy_device *phydev = netdev->phydev;

	phy_disconnect(phydev);
	if (phy_is_pseudo_fixed_link(phydev))
		fixed_phy_unregister(phydev);

while netdev->phydev becomes NULL after phy_disconnect(), you retained a
reference to the original PHY device before disconnecting, in order to
unregister it. Can you see if that works?
-- 
Florian

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

[RaghuramChary.Jallipalli]

Hi Florian,
> It still is completely unnecessary, you can do something like the following:
> 
> 	struct phy_device *phydev = netdev->phydev;
> 
> 	phy_disconnect(phydev);
> 	if (phy_is_pseudo_fixed_link(phydev))
> 		fixed_phy_unregister(phydev);
> 
> while netdev->phydev becomes NULL after phy_disconnect(), you retained
> a reference to the original PHY device before disconnecting, in order to
> unregister it. Can you see if that works?
> --
Done. Thanks.

Thanks,
Raghu