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