| Message ID | 20180423044630.2672-1-raghuramchary.jallipalli@microchip.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] lan78xx: Lan7801 Support for Fixed PHY | expand |
> #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
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
> 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
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
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?
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
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);
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(-)