| Message ID | 1430417955-28252-4-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 04/30/2015 11:19 AM, Tim Harvey wrote: > If an i210 is configured for 1000BASE-BX link_mode and has an external > phy specified, then register an mii bus using the external phy address as > a mask. > > An i210 hooked to an external standard phy will be configured with a > link_mode of SGMII in which case phy ops will be configured and used > internal in the igb driver for link status. However, in certain cases > one might be using a backplane SerDes connection to something that talks > on the mdio bus but is not a standard phy, such as a switch. In this case > by registering an mdio bus a phy driver can manage the device. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> So I think I am staring to see a pattern here between the i210 and the i354 it looks like the EEPROMs for these interfaces are not getting set up correctly. Unfortunately for the i210 you cannot change the EEPROM from ethtool so you would need to work out with your manufacturer how to get that fixed for your device. As far as the code below I think there may be an easy way to work around a bunch of this code. The quick trick for most of this would be to update igb_init_phy_params_82575 to return a new value, maybe make up something like E1000_ERR_PHY_UNKNOWN when the PHY ID is unrecognized. Then you could add the phylib code as a handler for if you encounter that error code. Also in terms of naming I would recommend sticking with one convention. Either igb_mdio_ or igb_mii_ for all of these functions. It is kind of confusing to be switching back and forth and I don't really see the point of adding the "enet" to the prefixes. > --- > drivers/net/ethernet/intel/igb/e1000_82575.c | 16 +++ > drivers/net/ethernet/intel/igb/e1000_hw.h | 7 ++ > drivers/net/ethernet/intel/igb/igb_main.c | 163 ++++++++++++++++++++++++++- > 3 files changed, 181 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c > index d2afd7b..e80617b 100644 > --- a/drivers/net/ethernet/intel/igb/e1000_82575.c > +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c > @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw *hw) > switch (link_mode) { > case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: > hw->phy.media_type = e1000_media_type_internal_serdes; > + if (igb_sgmii_uses_mdio_82575(hw)) { > + u32 mdicnfg = rd32(E1000_MDICNFG); > + > + mdicnfg &= E1000_MDICNFG_PHY_MASK; > + hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT; > + hw_dbg("1000BASE_KX w/ external MDIO device at 0x%x\n", > + hw->phy.addr); > + } else { > + hw_dbg("1000BASE_KX"); > + } > break; You should not be running SGMII under KX mode. If you are then you have an EEPROM bug. The correct link mode is SGMII below. Unfortunately you would need to check with your vendor on that one as I believe the EEPROMs for i210/i211 parts are not writable via ethtool. As for a hack workaround for now what you could do is use a !igb_sgmii_uses_mdio_82575() check to determine if you want to break, or to clear the LINK_MODE field and rewrite it as SGMII and then just fall though to SGMII which will help to clear up many issues you are probably seeing. You might keep that as a separate local patch until you can get the EEPROM issue resolved. As far as pulling the phy.addr value that should be taken care of by igb_get_phy_id_82575 if the link mode is correctly updated in the EEPROM to LINK_MODE_SGMII. > case E1000_CTRL_EXT_LINK_MODE_SGMII: > /* Get phy control interface type set (MDIO vs. I2C)*/ > if (igb_sgmii_uses_mdio_82575(hw)) { > hw->phy.media_type = e1000_media_type_copper; > dev_spec->sgmii_active = true; > + hw_dbg("SGMII with external MDIO PHY"); > break; > + } else { > + hw_dbg("SGMII with external I2C PHY"); > } > /* fall through for I2C based SGMII */ > case E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES: These hw_dbg messages can probably be dropped since they aren't adding much value. > @@ -621,8 +634,11 @@ static s32 igb_get_invariants_82575(struct e1000_hw *hw) > hw->phy.media_type = e1000_media_type_copper; > dev_spec->sgmii_active = true; > } > + hw_dbg("SERDES with external SFP"); > > break; > + } else { > + hw_dbg("SERDES"); > } > > /* do not change link mode for 100BaseFX */ Same for these. > diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h > index 2003b37..2864779 100644 > --- a/drivers/net/ethernet/intel/igb/e1000_hw.h > +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h > @@ -27,6 +27,7 @@ > #include <linux/delay.h> > #include <linux/io.h> > #include <linux/netdevice.h> > +#include <linux/phy.h> > > #include "e1000_regs.h" > #include "e1000_defines.h" > @@ -543,6 +544,12 @@ struct e1000_hw { > struct e1000_mbx_info mbx; > struct e1000_host_mng_dhcp_cookie mng_cookie; > > +#ifdef CONFIG_PHYLIB > + /* Phylib and MDIO interface */ > + struct mii_bus *mii_bus; > + struct phy_device *phy_dev; > + phy_interface_t phy_interface; > +#endif > union { > struct e1000_dev_spec_82575 _82575; > } dev_spec; Just to save the Intel guys a ton of grief you may want to consider moving these into the igb_adapter struct instead of the e1000_hw struct. The fact is these are all Linux specific interfaces and the e1000_hw is meant to be more-or-less OS agnostic. > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index f366b3b..9b5f538 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -41,6 +41,7 @@ > #include <linux/if_vlan.h> > #include <linux/pci.h> > #include <linux/pci-aspm.h> > +#include <linux/phy.h> > #include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/ip.h> > @@ -2223,6 +2224,121 @@ static s32 igb_init_i2c(struct igb_adapter *adapter) > return status; > } > > +#ifdef CONFIG_PHYLIB > +static int igb_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum) > +{ > + struct e1000_hw *hw = bus->priv; > + u16 out; > + int err; > + > + err = igb_read_reg_gs40g(hw, mii_id, regnum, &out); > + if (err) > + return err; > + return out; > +} > + I'm not really a fan of the igb_enet_ stuff, why not just igb_mdio_read, igb_mdio_write, etc..? > +static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, > + u16 val) > +{ > + struct e1000_hw *hw = bus->priv; > + > + return igb_write_reg_gs40g(hw, mii_id, regnum, val); > +} > + There shouldn't be any need to actually pass the PHY address assuming you let the driver code take care of pulling the address of the PHY from the EEPROM before hand. As such you can probably just not use the mii_id value. > +static int igb_enet_mdio_reset(struct mii_bus *bus) > +{ > + usleep_range(1000, 2000); > + return 0; > +} > + Is there a reason for having a sleep here if you aren't doing anything? You could probably just not define this since it doesn't do anything. > +static void igb_enet_mii_link(struct net_device *netdev) > +{ > +} > + Seems like there should probably be something here, but I don't know enough about phylib yet to say what. > +/* Probe the mdio bus for phys and connect them */ > +static int igb_enet_mii_probe(struct net_device *netdev) > +{ > + struct igb_adapter *adapter = netdev_priv(netdev); > + struct e1000_hw *hw = &adapter->hw; > + struct phy_device *phy_dev = NULL; > + int phy_id; > + > + /* check for attached phy */ > + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { > + if (hw->mii_bus->phy_map[phy_id]) { > + phy_dev = hw->mii_bus->phy_map[phy_id]; > + break; > + } > + } There is code that should have already figured this out in the driver. We just need to check the PHY addr after get_invarians has been called, assuming the EEPROM is correct. So you should be able to just get the phy.addr from the e1000_hw struct. > + if (!phy_dev) { > + netdev_err(netdev, "no PHY found\n"); > + return -ENODEV; > + } > + > + hw->phy_interface = PHY_INTERFACE_MODE_RGMII; > + phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev), > + igb_enet_mii_link, hw->phy_interface); > + if (IS_ERR(phy_dev)) { > + netdev_err(netdev, "could not attach to PHY\n"); > + return PTR_ERR(phy_dev); > + } > + > + hw->phy_dev = phy_dev; > + netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n", > + hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev)); > + > + return 0; > +} > + > +/* Create and register mdio bus */ > +static int igb_enet_mii_init(struct pci_dev *pdev) > +{ > + struct mii_bus *mii_bus; > + struct net_device *netdev = pci_get_drvdata(pdev); > + struct igb_adapter *adapter = netdev_priv(netdev); > + struct e1000_hw *hw = &adapter->hw; > + int err; > + > + mii_bus = mdiobus_alloc(); > + if (!mii_bus) { > + err = -ENOMEM; > + goto err_out; > + } > + > + mii_bus->name = "igb_enet_mii_bus"; > + mii_bus->read = igb_enet_mdio_read; > + mii_bus->write = igb_enet_mdio_write; > + mii_bus->reset = igb_enet_mdio_reset; > + snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", > + pci_name(pdev), hw->device_id + 1); > + mii_bus->priv = hw; > + mii_bus->parent = &pdev->dev; > + mii_bus->phy_mask = ~(1 << hw->phy.addr); > + > + err = mdiobus_register(mii_bus); > + if (err) { > + netdev_err(netdev, "failed to register mii_bus: %d\n", err); > + goto err_out_free_mdiobus; > + } > + hw->mii_bus = mii_bus; > + > + return 0; > + > +err_out_free_mdiobus: > + mdiobus_free(mii_bus); > +err_out: > + return err; > +} > + > +static void igb_enet_mii_remove(struct e1000_hw *hw) > +{ > + if (hw->mii_bus) { > + mdiobus_unregister(hw->mii_bus); > + mdiobus_free(hw->mii_bus); > + } > +} > +#endif /* CONFIG_PHYLIB */ > + > /** > * igb_probe - Device Initialization Routine > * @pdev: PCI device information struct > @@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > } > } > pm_runtime_put_noidle(&pdev->dev); > + > +#ifdef CONFIG_PHYLIB > + /* create and register the mdio bus if using ext phy */ > + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) > + igb_enet_mii_init(pdev); > +#endif > + > return 0; > > err_register: The MDICNFG register doesn't exist for all devices as I recall. What you may want to do is make enabling the mii_bus optional dependent on get_invariants returning an error that the PHY is not recognized. > @@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev) > struct e1000_hw *hw = &adapter->hw; > > pm_runtime_get_noresume(&pdev->dev); > +#ifdef CONFIG_PHYLIB > + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) > + igb_enet_mii_remove(hw); > +#endif > #ifdef CONFIG_IGB_HWMON > igb_sysfs_exit(adapter); > #endif Same thing here. I would say what you could do is just check to see if a mii_bus is allocated and if so remove it. You could probably push the check inside of igb_mii_remove. > @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, bool resuming) > if (!resuming) > pm_runtime_put(&pdev->dev); > > +#ifdef CONFIG_PHYLIB > + /* Probe and connect to PHY if using ext phy */ > + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) > + igb_enet_mii_probe(netdev); > +#endif > + > /* start the watchdog. */ > hw->mac.get_link_status = 1; > schedule_work(&adapter->watchdog_task); Same here, use the existance of a mii_bus, not the register check. > @@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count) > static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) > { > struct igb_adapter *adapter = netdev_priv(netdev); > + struct e1000_hw *hw = &adapter->hw; > struct mii_ioctl_data *data = if_mii(ifr); > > - if (adapter->hw.phy.media_type != e1000_media_type_copper) > + if (adapter->hw.phy.media_type != e1000_media_type_copper && > + !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)) > return -EOPNOTSUPP; > > switch (cmd) { > case SIOCGMIIPHY: > - data->phy_id = adapter->hw.phy.addr; > + data->phy_id = hw->phy.addr; > break; > case SIOCGMIIREG: > - if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, > - &data->val_out)) > - return -EIO; > + if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) { > + if (igb_read_reg_gs40g(&adapter->hw, data->phy_id, > + data->reg_num & 0x1F, > + &data->val_out)) > + return -EIO; > + } else { > + if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, > + &data->val_out)) > + return -EIO; > + } > break; > case SIOCSMIIREG: > + if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) { > + if (igb_write_reg_gs40g(hw, data->phy_id, > + data->reg_num & 0x1F, > + data->val_in)) > + return -EIO; > + } else { > + if (igb_write_phy_reg(hw, data->reg_num & 0x1F, > + data->val_in)) > + return -EIO; > + } > + break; > default: > return -EOPNOTSUPP; > } > These changes just shouldn't be needed. I'd say they could be dropped. The phy_id should be static and configured by the hardware before the driver is even loaded.
On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > On 04/30/2015 11:19 AM, Tim Harvey wrote: >> >> If an i210 is configured for 1000BASE-BX link_mode and has an external >> phy specified, then register an mii bus using the external phy address as >> a mask. >> >> An i210 hooked to an external standard phy will be configured with a >> link_mode of SGMII in which case phy ops will be configured and used >> internal in the igb driver for link status. However, in certain cases >> one might be using a backplane SerDes connection to something that talks >> on the mdio bus but is not a standard phy, such as a switch. In this case >> by registering an mdio bus a phy driver can manage the device. >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > So I think I am staring to see a pattern here between the i210 and the i354 > it looks like the EEPROMs for these interfaces are not getting set up > correctly. Unfortunately for the i210 you cannot change the EEPROM from > ethtool so you would need to work out with your manufacturer how to get that > fixed for your device. Alexander, Thanks for the review! I think much of my patchset is more clear if I give a bit more background of how we use the i210 on our GW16083 (http://www.gateworks.com/product/item/ventana-gw16083-mezzanine). This board is an i210 connected to a Marvell MV88E6176 6-port switch via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and instead of having a register interface that utilizes a single phy address, it uses several addresses between 0x10 and 0x1d. The link between the two chips is thus 1gbps full-duplex without the need of link management (why I consider it phy-less). I configured the EEPROM with the following details: - link-mode: 1000BASE-KX for phy-less operation - external-mdio - phy_addr 0x10 (the first one used by the MV88E6176 and the one that maps more closely to standard phy registers than any others) - device_id: 0x157C (SerDes flashless - looking back perhaps this should have been 0x1537 but then this isn't used in igb to distinguish any details so its a don't-care) The i210 datasheet defines the following link-modes: 0 - Internal PHY 1 - 1000BASE-KX 2 - SGMII 3 - SerDes/1000BASE-BX The differences are still not crystal clear to me and is really all about how the igb driver uses this value. When I wrote support for the GW16083 a year ago I sent out a query to the e1000-devel list regarding link_mode and how I should proceed but I received no reply's (http://permalink.gmane.org/gmane.linux.drivers.e1000.devel/13942) and unfortunately am only now getting back to a point of having any bandwidth to try to mainline my changes. I would have to dig a bit to find where I got this info but my understanding was that 1000BASE-KX was for fixed 1gbps 'phy-less' SerDes links (requiring no link negotiation) and because I have the i210 connected in to a port on the MV88E6176 that is always 1gpbs full-duplex this made sense to me to let the driver treat it as 'phy-less'. > > As far as the code below I think there may be an easy way to work around a > bunch of this code. The quick trick for most of this would be to update > igb_init_phy_params_82575 to return a new value, maybe make up something > like E1000_ERR_PHY_UNKNOWN when the PHY ID is unrecognized. Then you could > add the phylib code as a handler for if you encounter that error code. Are you sure this shouldn't be based off of link mode? It seems to me that if your link-mode indicates an external phy then this is exactly when you would want to register an mii bus with phylib. However, also doing so as a fallback if PHY ID can't be recognized also makes sense in general for perhaps boards with mis-programmed EEPROM's. > > Also in terms of naming I would recommend sticking with one convention. > Either igb_mdio_ or igb_mii_ for all of these functions. It is kind of > confusing to be switching back and forth and I don't really see the point of > adding the "enet" to the prefixes. sure - I saw your comment about getting rid of the igb_enet notation and I can do this in a next-rev patch. > >> --- >> drivers/net/ethernet/intel/igb/e1000_82575.c | 16 +++ >> drivers/net/ethernet/intel/igb/e1000_hw.h | 7 ++ >> drivers/net/ethernet/intel/igb/igb_main.c | 163 >> ++++++++++++++++++++++++++- >> 3 files changed, 181 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c >> b/drivers/net/ethernet/intel/igb/e1000_82575.c >> index d2afd7b..e80617b 100644 >> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c >> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c >> @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw >> *hw) >> switch (link_mode) { >> case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: >> hw->phy.media_type = e1000_media_type_internal_serdes; >> + if (igb_sgmii_uses_mdio_82575(hw)) { >> + u32 mdicnfg = rd32(E1000_MDICNFG); >> + >> + mdicnfg &= E1000_MDICNFG_PHY_MASK; >> + hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT; >> + hw_dbg("1000BASE_KX w/ external MDIO device at >> 0x%x\n", >> + hw->phy.addr); >> + } else { >> + hw_dbg("1000BASE_KX"); >> + } >> break; > > > You should not be running SGMII under KX mode. If you are then you have an > EEPROM bug. The correct link mode is SGMII below. Unfortunately you would > need to check with your vendor on that one as I believe the EEPROMs for > i210/i211 parts are not writable via ethtool. > > As for a hack workaround for now what you could do is use a > !igb_sgmii_uses_mdio_82575() check to determine if you want to break, or to > clear the LINK_MODE field and rewrite it as SGMII and then just fall though > to SGMII which will help to clear up many issues you are probably seeing. > You might keep that as a separate local patch until you can get the EEPROM > issue resolved. I certainly hope this isn't considered an EEPROM programming issue because at the time I could get no clarification from the community or my Intel FAE on what mode we should be using (the FAE understandably likely didn't know because this is an implementation detail within the driver the way I see it). We have been shipping boards for a year supported by a phylib driver and it would not be easy to field update these because the i210 nvram is only supported via the non-redistributable licensed eepromARMtool (yuck!). One of the key U-Boot developers tried to get Intel to allow him to add source to U-Boot for EEPROM programming and was denied. (again... yuck!). > > As far as pulling the phy.addr value that should be taken care of by > igb_get_phy_id_82575 if the link mode is correctly updated in the EEPROM to > LINK_MODE_SGMII. > >> case E1000_CTRL_EXT_LINK_MODE_SGMII: >> /* Get phy control interface type set (MDIO vs. I2C)*/ >> if (igb_sgmii_uses_mdio_82575(hw)) { >> hw->phy.media_type = e1000_media_type_copper; >> dev_spec->sgmii_active = true; >> + hw_dbg("SGMII with external MDIO PHY"); >> break; >> + } else { >> + hw_dbg("SGMII with external I2C PHY"); >> } >> /* fall through for I2C based SGMII */ >> case E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES: > > > These hw_dbg messages can probably be dropped since they aren't adding much > value. > >> @@ -621,8 +634,11 @@ static s32 igb_get_invariants_82575(struct e1000_hw >> *hw) >> hw->phy.media_type = >> e1000_media_type_copper; >> dev_spec->sgmii_active = true; >> } >> + hw_dbg("SERDES with external SFP"); >> >> break; >> + } else { >> + hw_dbg("SERDES"); >> } >> >> /* do not change link mode for 100BaseFX */ > > > Same for these. ok > >> diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h >> b/drivers/net/ethernet/intel/igb/e1000_hw.h >> index 2003b37..2864779 100644 >> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h >> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h >> @@ -27,6 +27,7 @@ >> #include <linux/delay.h> >> #include <linux/io.h> >> #include <linux/netdevice.h> >> +#include <linux/phy.h> >> >> #include "e1000_regs.h" >> #include "e1000_defines.h" >> @@ -543,6 +544,12 @@ struct e1000_hw { >> struct e1000_mbx_info mbx; >> struct e1000_host_mng_dhcp_cookie mng_cookie; >> >> +#ifdef CONFIG_PHYLIB >> + /* Phylib and MDIO interface */ >> + struct mii_bus *mii_bus; >> + struct phy_device *phy_dev; >> + phy_interface_t phy_interface; >> +#endif >> union { >> struct e1000_dev_spec_82575 _82575; >> } dev_spec; > > > Just to save the Intel guys a ton of grief you may want to consider moving > these into the igb_adapter struct instead of the e1000_hw struct. The fact > is these are all Linux specific interfaces and the e1000_hw is meant to be > more-or-less OS agnostic. ok > > >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >> b/drivers/net/ethernet/intel/igb/igb_main.c >> index f366b3b..9b5f538 100644 >> --- a/drivers/net/ethernet/intel/igb/igb_main.c >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >> @@ -41,6 +41,7 @@ >> #include <linux/if_vlan.h> >> #include <linux/pci.h> >> #include <linux/pci-aspm.h> >> +#include <linux/phy.h> >> #include <linux/delay.h> >> #include <linux/interrupt.h> >> #include <linux/ip.h> >> @@ -2223,6 +2224,121 @@ static s32 igb_init_i2c(struct igb_adapter >> *adapter) >> return status; >> } >> >> +#ifdef CONFIG_PHYLIB >> +static int igb_enet_mdio_read(struct mii_bus *bus, int mii_id, int >> regnum) >> +{ >> + struct e1000_hw *hw = bus->priv; >> + u16 out; >> + int err; >> + >> + err = igb_read_reg_gs40g(hw, mii_id, regnum, &out); >> + if (err) >> + return err; >> + return out; >> +} >> + > > > I'm not really a fan of the igb_enet_ stuff, why not just igb_mdio_read, > igb_mdio_write, etc..? sure, I can rename them. > >> +static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int >> regnum, >> + u16 val) >> +{ >> + struct e1000_hw *hw = bus->priv; >> + >> + return igb_write_reg_gs40g(hw, mii_id, regnum, val); >> +} >> + > > > There shouldn't be any need to actually pass the PHY address assuming you > let the driver code take care of pulling the address of the PHY from the > EEPROM before hand. As such you can probably just not use the mii_id value. Again, the reason for not using the address from the EEPROM is this use case where there is not a single phy address. This is also why I'm patching the mdio read/write functions to take a parameter instead of using the internal value (and creating wrappers for when they are used internally in igb). > >> +static int igb_enet_mdio_reset(struct mii_bus *bus) >> +{ >> + usleep_range(1000, 2000); >> + return 0; >> +} >> + > > > Is there a reason for having a sleep here if you aren't doing anything? You > could probably just not define this since it doesn't do anything. I can drop this - I think I may have been trying to meet some delay spec after reset but since as you say this doesn't actually reset something it shouldn't go here anyway. > >> +static void igb_enet_mii_link(struct net_device *netdev) >> +{ >> +} >> + > > > Seems like there should probably be something here, but I don't know enough > about phylib yet to say what. I'm not sure either. Perhaps someone with more knowledge of phylib can comment. > >> +/* Probe the mdio bus for phys and connect them */ >> +static int igb_enet_mii_probe(struct net_device *netdev) >> +{ >> + struct igb_adapter *adapter = netdev_priv(netdev); >> + struct e1000_hw *hw = &adapter->hw; >> + struct phy_device *phy_dev = NULL; >> + int phy_id; >> + >> + /* check for attached phy */ >> + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { >> + if (hw->mii_bus->phy_map[phy_id]) { >> + phy_dev = hw->mii_bus->phy_map[phy_id]; >> + break; >> + } >> + } > > > There is code that should have already figured this out in the driver. We > just need to check the PHY addr after get_invarians has been called, > assuming the EEPROM is correct. So you should be able to just get the > phy.addr from the e1000_hw struct. It is true that in igb_enet_mii_init() where the mii bus is registered with phylib that I set the phy_mask to include only the phy address from the eeprom. In my case I put 0x10 in the EEPROM as the phy address as that is the first phy address that the switch responds to and so I use that as a way to detect the phy. I can remove the loop and just set phy_dev = hw->mii_bus->phy_map[hw->phy.addr] in this case. > > >> + if (!phy_dev) { >> + netdev_err(netdev, "no PHY found\n"); >> + return -ENODEV; >> + } >> + >> + hw->phy_interface = PHY_INTERFACE_MODE_RGMII; >> + phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev), >> + igb_enet_mii_link, hw->phy_interface); >> + if (IS_ERR(phy_dev)) { >> + netdev_err(netdev, "could not attach to PHY\n"); >> + return PTR_ERR(phy_dev); >> + } >> + >> + hw->phy_dev = phy_dev; >> + netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n", >> + hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev)); >> + >> + return 0; >> +} >> + >> +/* Create and register mdio bus */ >> +static int igb_enet_mii_init(struct pci_dev *pdev) >> +{ >> + struct mii_bus *mii_bus; >> + struct net_device *netdev = pci_get_drvdata(pdev); >> + struct igb_adapter *adapter = netdev_priv(netdev); >> + struct e1000_hw *hw = &adapter->hw; >> + int err; >> + >> + mii_bus = mdiobus_alloc(); >> + if (!mii_bus) { >> + err = -ENOMEM; >> + goto err_out; >> + } >> + >> + mii_bus->name = "igb_enet_mii_bus"; >> + mii_bus->read = igb_enet_mdio_read; >> + mii_bus->write = igb_enet_mdio_write; >> + mii_bus->reset = igb_enet_mdio_reset; >> + snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", >> + pci_name(pdev), hw->device_id + 1); >> + mii_bus->priv = hw; >> + mii_bus->parent = &pdev->dev; >> + mii_bus->phy_mask = ~(1 << hw->phy.addr); >> + >> + err = mdiobus_register(mii_bus); >> + if (err) { >> + netdev_err(netdev, "failed to register mii_bus: %d\n", >> err); >> + goto err_out_free_mdiobus; >> + } >> + hw->mii_bus = mii_bus; >> + >> + return 0; >> + >> +err_out_free_mdiobus: >> + mdiobus_free(mii_bus); >> +err_out: >> + return err; >> +} >> + >> +static void igb_enet_mii_remove(struct e1000_hw *hw) >> +{ >> + if (hw->mii_bus) { >> + mdiobus_unregister(hw->mii_bus); >> + mdiobus_free(hw->mii_bus); >> + } >> +} >> +#endif /* CONFIG_PHYLIB */ >> + >> /** >> * igb_probe - Device Initialization Routine >> * @pdev: PCI device information struct >> @@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const >> struct pci_device_id *ent) >> } >> } >> pm_runtime_put_noidle(&pdev->dev); >> + >> +#ifdef CONFIG_PHYLIB >> + /* create and register the mdio bus if using ext phy */ >> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >> + igb_enet_mii_init(pdev); >> +#endif >> + >> return 0; >> >> err_register: > > > The MDICNFG register doesn't exist for all devices as I recall. What you > may want to do is make enabling the mii_bus optional dependent on > get_invariants returning an error that the PHY is not recognized. > >> @@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev) >> struct e1000_hw *hw = &adapter->hw; >> >> pm_runtime_get_noresume(&pdev->dev); >> +#ifdef CONFIG_PHYLIB >> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >> + igb_enet_mii_remove(hw); >> +#endif >> #ifdef CONFIG_IGB_HWMON >> igb_sysfs_exit(adapter); >> #endif > > > Same thing here. I would say what you could do is just check to see if a > mii_bus is allocated and if so remove it. You could probably push the check > inside of igb_mii_remove. > >> @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, >> bool resuming) >> if (!resuming) >> pm_runtime_put(&pdev->dev); >> >> +#ifdef CONFIG_PHYLIB >> + /* Probe and connect to PHY if using ext phy */ >> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >> + igb_enet_mii_probe(netdev); >> +#endif >> + >> /* start the watchdog. */ >> hw->mac.get_link_status = 1; >> schedule_work(&adapter->watchdog_task); > > > Same here, use the existance of a mii_bus, not the register check. > This goes back to the discussion on how to decide if we should register with phylib and will depend on how that discussion plays out. There are about 4 places I need to know if we're using phylib and whatever mechanism is agree'd upon can be used in all these cases: igb_probe() - register the mii bus with phylib igb_remove() - remove the mii bus igb_open() - to call the mii probe igb_mii_ioctl() - to know to use the mdio read/write wrappers > >> @@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring >> *rx_ring, u16 cleaned_count) >> static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, >> int cmd) >> { >> struct igb_adapter *adapter = netdev_priv(netdev); >> + struct e1000_hw *hw = &adapter->hw; >> struct mii_ioctl_data *data = if_mii(ifr); >> >> - if (adapter->hw.phy.media_type != e1000_media_type_copper) >> + if (adapter->hw.phy.media_type != e1000_media_type_copper && >> + !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)) >> return -EOPNOTSUPP; >> >> switch (cmd) { >> case SIOCGMIIPHY: >> - data->phy_id = adapter->hw.phy.addr; >> + data->phy_id = hw->phy.addr; >> break; >> case SIOCGMIIREG: >> - if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, >> - &data->val_out)) >> - return -EIO; >> + if (hw->mac.type == e1000_i210 || hw->mac.type == >> e1000_i211) { >> + if (igb_read_reg_gs40g(&adapter->hw, data->phy_id, >> + data->reg_num & 0x1F, >> + &data->val_out)) >> + return -EIO; >> + } else { >> + if (igb_read_phy_reg(&adapter->hw, data->reg_num & >> 0x1F, >> + &data->val_out)) >> + return -EIO; >> + } >> break; >> case SIOCSMIIREG: >> + if (hw->mac.type == e1000_i210 || hw->mac.type == >> e1000_i211) { >> + if (igb_write_reg_gs40g(hw, data->phy_id, >> + data->reg_num & 0x1F, >> + data->val_in)) >> + return -EIO; >> + } else { >> + if (igb_write_phy_reg(hw, data->reg_num & 0x1F, >> + data->val_in)) >> + return -EIO; >> + } >> + break; >> default: >> return -EOPNOTSUPP; >> } >> > > These changes just shouldn't be needed. I'd say they could be dropped. The > phy_id should be static and configured by the hardware before the driver is > even loaded. The above also adds SIOCGMIIREG which previously was not supported, but I also need to allow multiple phy addresses here and thus can't just default to igb_write_phy_reg() which uses the single phy addr from the EEPROM (unless I move multi-addr support there). Tim
On 05/11/2015 11:42 AM, Tim Harvey wrote: > On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck > <alexander.duyck@gmail.com> wrote: >> On 04/30/2015 11:19 AM, Tim Harvey wrote: >>> If an i210 is configured for 1000BASE-BX link_mode and has an external >>> phy specified, then register an mii bus using the external phy address as >>> a mask. >>> >>> An i210 hooked to an external standard phy will be configured with a >>> link_mode of SGMII in which case phy ops will be configured and used >>> internal in the igb driver for link status. However, in certain cases >>> one might be using a backplane SerDes connection to something that talks >>> on the mdio bus but is not a standard phy, such as a switch. In this case >>> by registering an mdio bus a phy driver can manage the device. >>> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> >> >> So I think I am staring to see a pattern here between the i210 and the i354 >> it looks like the EEPROMs for these interfaces are not getting set up >> correctly. Unfortunately for the i210 you cannot change the EEPROM from >> ethtool so you would need to work out with your manufacturer how to get that >> fixed for your device. > Alexander, > > Thanks for the review! > > I think much of my patchset is more clear if I give a bit more > background of how we use the i210 on our GW16083 > (http://www.gateworks.com/product/item/ventana-gw16083-mezzanine). > This board is an i210 connected to a Marvell MV88E6176 6-port switch > via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and > instead of having a register interface that utilizes a single phy > address, it uses several addresses between 0x10 and 0x1d. The link > between the two chips is thus 1gbps full-duplex without the need of > link management (why I consider it phy-less). Okay, so this is likely much more complicated then your original patch let on. What you are actually doing is enabling phylib so that you can enable a DSA switch on top of the i210 interface, do I have that right? > I configured the EEPROM with the following details: > - link-mode: 1000BASE-KX for phy-less operation > - external-mdio > - phy_addr 0x10 (the first one used by the MV88E6176 and the one that > maps more closely to standard phy registers than any others) > - device_id: 0x157C (SerDes flashless - looking back perhaps this > should have been 0x1537 but then this isn't used in igb to distinguish > any details so its a don't-care) > > The i210 datasheet defines the following link-modes: > 0 - Internal PHY > 1 - 1000BASE-KX > 2 - SGMII > 3 - SerDes/1000BASE-BX > > The differences are still not crystal clear to me and is really all > about how the igb driver uses this value. When I wrote support for the > GW16083 a year ago I sent out a query to the e1000-devel list > regarding link_mode and how I should proceed but I received no reply's > (http://permalink.gmane.org/gmane.linux.drivers.e1000.devel/13942) and > unfortunately am only now getting back to a point of having any > bandwidth to try to mainline my changes. It looks like you are learning the same lesson many have when it comes to this kind of stuff. Upstream first is usually a much better policy because then you figure out all of these issues much earlier and you aren't stuck supporting something that has implementation issues you weren't aware of. > I would have to dig a bit to find where I got this info but my > understanding was that 1000BASE-KX was for fixed 1gbps 'phy-less' > SerDes links (requiring no link negotiation) and because I have the > i210 connected in to a port on the MV88E6176 that is always 1gpbs > full-duplex this made sense to me to let the driver treat it as > 'phy-less'. The 1000BASE-KX is really meant for pure SerDes configuration where there will be no PHY attached at all. So for example if you were going to connect directly to another 1000BASE-KX port on either a backplane switch or another adapter. It is implied that by using this you aren't going to use the MDIO interface at all. There are essentially 3 modes that do external 1gbs 'phy-less' and the problem is that KX implies a special mode that is typically used for backplane w/o any link negotiation. Since there is a PHY like entity on the other end you would want SGMII, and in this case the variant that uses the MDIO registers for configuration. The 1000Base-BX was probably closer to what you had envisioned. That is meant to be used for pure SerDes with some sort of transceiver in between. As such it is normally used for configurations such as fiber since it supports standard 1Gb/s Ethernet over fiber. Really if you were planning to connect to an external switch or PHY that required MDIO configuration you should have gone with SGMII as it is essentially the same as the 1000Base-BX however it includes the configuration bits for managing an external PHY over either I2C or the MDIO interfaces. >> As far as the code below I think there may be an easy way to work around a >> bunch of this code. The quick trick for most of this would be to update >> igb_init_phy_params_82575 to return a new value, maybe make up something >> like E1000_ERR_PHY_UNKNOWN when the PHY ID is unrecognized. Then you could >> add the phylib code as a handler for if you encounter that error code. > Are you sure this shouldn't be based off of link mode? It seems to me > that if your link-mode indicates an external phy then this is exactly > when you would want to register an mii bus with phylib. However, also > doing so as a fallback if PHY ID can't be recognized also makes sense > in general for perhaps boards with mis-programmed EEPROM's. The problem is the link mode doesn't tell you anything other than the type of interface being used. So in your case you want to advertise the interface as SGMII because you will want SerDes and to perform configuration via the external MDIO interface. >>> --- >>> drivers/net/ethernet/intel/igb/e1000_82575.c | 16 +++ >>> drivers/net/ethernet/intel/igb/e1000_hw.h | 7 ++ >>> drivers/net/ethernet/intel/igb/igb_main.c | 163 >>> ++++++++++++++++++++++++++- >>> 3 files changed, 181 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c >>> b/drivers/net/ethernet/intel/igb/e1000_82575.c >>> index d2afd7b..e80617b 100644 >>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c >>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c >>> @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw >>> *hw) >>> switch (link_mode) { >>> case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: >>> hw->phy.media_type = e1000_media_type_internal_serdes; >>> + if (igb_sgmii_uses_mdio_82575(hw)) { >>> + u32 mdicnfg = rd32(E1000_MDICNFG); >>> + >>> + mdicnfg &= E1000_MDICNFG_PHY_MASK; >>> + hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT; >>> + hw_dbg("1000BASE_KX w/ external MDIO device at >>> 0x%x\n", >>> + hw->phy.addr); >>> + } else { >>> + hw_dbg("1000BASE_KX"); >>> + } >>> break; >> >> You should not be running SGMII under KX mode. If you are then you have an >> EEPROM bug. The correct link mode is SGMII below. Unfortunately you would >> need to check with your vendor on that one as I believe the EEPROMs for >> i210/i211 parts are not writable via ethtool. >> >> As for a hack workaround for now what you could do is use a >> !igb_sgmii_uses_mdio_82575() check to determine if you want to break, or to >> clear the LINK_MODE field and rewrite it as SGMII and then just fall though >> to SGMII which will help to clear up many issues you are probably seeing. >> You might keep that as a separate local patch until you can get the EEPROM >> issue resolved. > I certainly hope this isn't considered an EEPROM programming issue > because at the time I could get no clarification from the community or > my Intel FAE on what mode we should be using (the FAE understandably > likely didn't know because this is an implementation detail within the > driver the way I see it). The problem is KX has a very specific meaning, and is supposed to be reserved for backplane Ethernet connections between MACs. It makes assumptions about things that could have an impact on link negotiation. Odds are if it is working for you it is because it is "good enough" however it isn't really meant for the type of connection you are using it for. You might try working things out with Intel to see if you get get either a driver quirk directly added to igb, or possibly have something added to the PCI quirks in the Linux kernel. All that really needs to happen is to update the KX go SGMII at driver load, or during PCI bus probe as the value should be persistent through reboots. In your EEPROM implementation did you do anything that could be used to uniquely identify your device. For example, is there a sub-device or sub-vendor ID registered in the PCI config? > We have been shipping boards for a year supported by a phylib driver > and it would not be easy to field update these because the i210 nvram > is only supported via the non-redistributable licensed eepromARMtool > (yuck!). One of the key U-Boot developers tried to get Intel to allow > him to add source to U-Boot for EEPROM programming and was denied. > (again... yuck!). Yeah, very yuck indeed. Unfortunately you are adding PHY support to a link mode that doesn't support external PHYs. The likelihood of that causing regressions for other implementations is very high. That is why if nothing else it would be better to add a specific quirk for your i210 w/ KX than something that just takes all KX and converts it into something like SGMII. >>> +static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int >>> regnum, >>> + u16 val) >>> +{ >>> + struct e1000_hw *hw = bus->priv; >>> + >>> + return igb_write_reg_gs40g(hw, mii_id, regnum, val); >>> +} >>> + >> >> There shouldn't be any need to actually pass the PHY address assuming you >> let the driver code take care of pulling the address of the PHY from the >> EEPROM before hand. As such you can probably just not use the mii_id value. > Again, the reason for not using the address from the EEPROM is this > use case where there is not a single phy address. This is also why I'm > patching the mdio read/write functions to take a parameter instead of > using the internal value (and creating wrappers for when they are used > internally in igb). Okay, so in the case of your switch the phy_mask applies to probe only then. I was thinking of traditional PHYs where there is usually only one address that applies to the PHY belonging to a given MAC. What you may want to do is look at adding your own igb_mii_write/read functions based off of something like the igb_(write/read)_phy_reg_82580 code. The gs40g has some very specific logic built into it to convert a 32b register into a page and offset which you likely don't need. The only change you would need from the 82580 functions would be to add a few lines for configuring the MDICNFG address. I wouldn't bother with restoring the address after you have performed your operation. There is always a possible scenario where something gets hung or crashes in the middle of your operation and that will foul up the MDICNFG register. Instead I would recommend looking at something like modifying igb_reset_mdicnfg_82580 as it might be preferable to have it apply to all hardware 82580 and newer instead of just limiting it to 82580. That way the value is restored on reset or driver load so that as long as the EEPROM is there you will always read the correct value. >>> +/* Probe the mdio bus for phys and connect them */ >>> +static int igb_enet_mii_probe(struct net_device *netdev) >>> +{ >>> + struct igb_adapter *adapter = netdev_priv(netdev); >>> + struct e1000_hw *hw = &adapter->hw; >>> + struct phy_device *phy_dev = NULL; >>> + int phy_id; >>> + >>> + /* check for attached phy */ >>> + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { >>> + if (hw->mii_bus->phy_map[phy_id]) { >>> + phy_dev = hw->mii_bus->phy_map[phy_id]; >>> + break; >>> + } >>> + } >> >> There is code that should have already figured this out in the driver. We >> just need to check the PHY addr after get_invarians has been called, >> assuming the EEPROM is correct. So you should be able to just get the >> phy.addr from the e1000_hw struct. > It is true that in igb_enet_mii_init() where the mii bus is registered > with phylib that I set the phy_mask to include only the phy address > from the eeprom. In my case I put 0x10 in the EEPROM as the phy > address as that is the first phy address that the switch responds to > and so I use that as a way to detect the phy. > > I can remove the loop and just set phy_dev = > hw->mii_bus->phy_map[hw->phy.addr] in this case. Yes, that probably would be cleaner. > >> >>> + if (!phy_dev) { >>> + netdev_err(netdev, "no PHY found\n"); >>> + return -ENODEV; >>> + } >>> + >>> + hw->phy_interface = PHY_INTERFACE_MODE_RGMII; >>> + phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev), >>> + igb_enet_mii_link, hw->phy_interface); >>> + if (IS_ERR(phy_dev)) { >>> + netdev_err(netdev, "could not attach to PHY\n"); >>> + return PTR_ERR(phy_dev); >>> + } >>> + >>> + hw->phy_dev = phy_dev; >>> + netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n", >>> + hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev)); >>> + >>> + return 0; >>> +} >>> + >>> +/* Create and register mdio bus */ >>> +static int igb_enet_mii_init(struct pci_dev *pdev) >>> +{ >>> + struct mii_bus *mii_bus; >>> + struct net_device *netdev = pci_get_drvdata(pdev); >>> + struct igb_adapter *adapter = netdev_priv(netdev); >>> + struct e1000_hw *hw = &adapter->hw; >>> + int err; >>> + >>> + mii_bus = mdiobus_alloc(); >>> + if (!mii_bus) { >>> + err = -ENOMEM; >>> + goto err_out; >>> + } >>> + >>> + mii_bus->name = "igb_enet_mii_bus"; >>> + mii_bus->read = igb_enet_mdio_read; >>> + mii_bus->write = igb_enet_mdio_write; >>> + mii_bus->reset = igb_enet_mdio_reset; >>> + snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", >>> + pci_name(pdev), hw->device_id + 1); >>> + mii_bus->priv = hw; >>> + mii_bus->parent = &pdev->dev; >>> + mii_bus->phy_mask = ~(1 << hw->phy.addr); >>> + >>> + err = mdiobus_register(mii_bus); >>> + if (err) { >>> + netdev_err(netdev, "failed to register mii_bus: %d\n", >>> err); >>> + goto err_out_free_mdiobus; >>> + } >>> + hw->mii_bus = mii_bus; >>> + >>> + return 0; >>> + >>> +err_out_free_mdiobus: >>> + mdiobus_free(mii_bus); >>> +err_out: >>> + return err; >>> +} >>> + >>> +static void igb_enet_mii_remove(struct e1000_hw *hw) >>> +{ >>> + if (hw->mii_bus) { >>> + mdiobus_unregister(hw->mii_bus); >>> + mdiobus_free(hw->mii_bus); >>> + } >>> +} >>> +#endif /* CONFIG_PHYLIB */ >>> + >>> /** >>> * igb_probe - Device Initialization Routine >>> * @pdev: PCI device information struct >>> @@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const >>> struct pci_device_id *ent) >>> } >>> } >>> pm_runtime_put_noidle(&pdev->dev); >>> + >>> +#ifdef CONFIG_PHYLIB >>> + /* create and register the mdio bus if using ext phy */ >>> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >>> + igb_enet_mii_init(pdev); >>> +#endif >>> + >>> return 0; >>> >>> err_register: >> >> The MDICNFG register doesn't exist for all devices as I recall. What you >> may want to do is make enabling the mii_bus optional dependent on >> get_invariants returning an error that the PHY is not recognized. >> >>> @@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev) >>> struct e1000_hw *hw = &adapter->hw; >>> >>> pm_runtime_get_noresume(&pdev->dev); >>> +#ifdef CONFIG_PHYLIB >>> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >>> + igb_enet_mii_remove(hw); >>> +#endif >>> #ifdef CONFIG_IGB_HWMON >>> igb_sysfs_exit(adapter); >>> #endif >> >> Same thing here. I would say what you could do is just check to see if a >> mii_bus is allocated and if so remove it. You could probably push the check >> inside of igb_mii_remove. >> >>> @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, >>> bool resuming) >>> if (!resuming) >>> pm_runtime_put(&pdev->dev); >>> >>> +#ifdef CONFIG_PHYLIB >>> + /* Probe and connect to PHY if using ext phy */ >>> + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) >>> + igb_enet_mii_probe(netdev); >>> +#endif >>> + >>> /* start the watchdog. */ >>> hw->mac.get_link_status = 1; >>> schedule_work(&adapter->watchdog_task); >> >> Same here, use the existance of a mii_bus, not the register check. >> > This goes back to the discussion on how to decide if we should > register with phylib and will depend on how that discussion plays out. > There are about 4 places I need to know if we're using phylib and > whatever mechanism is agree'd upon can be used in all these cases: > igb_probe() - register the mii bus with phylib > igb_remove() - remove the mii bus > igb_open() - to call the mii probe > igb_mii_ioctl() - to know to use the mdio read/write wrappers I really think you would be better off just performing any kind of check in igb_probe. Specifically if the link_mode is SGMII, and there is a PHY, but it is unrecognized then we should be using phylib and should setup an mii_bus. That way it should play well with other implementations such as the stuff the Cumulus guys will need. Otherwise the mii_bus should not be configured. Then all of the other calls can simply check to see if the mii_bus exists and queue off of that for the phylib calls. >>> @@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring >>> *rx_ring, u16 cleaned_count) >>> static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, >>> int cmd) >>> { >>> struct igb_adapter *adapter = netdev_priv(netdev); >>> + struct e1000_hw *hw = &adapter->hw; >>> struct mii_ioctl_data *data = if_mii(ifr); >>> >>> - if (adapter->hw.phy.media_type != e1000_media_type_copper) >>> + if (adapter->hw.phy.media_type != e1000_media_type_copper && >>> + !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)) >>> return -EOPNOTSUPP; >>> >>> switch (cmd) { >>> case SIOCGMIIPHY: >>> - data->phy_id = adapter->hw.phy.addr; >>> + data->phy_id = hw->phy.addr; >>> break; >>> case SIOCGMIIREG: >>> - if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, >>> - &data->val_out)) >>> - return -EIO; >>> + if (hw->mac.type == e1000_i210 || hw->mac.type == >>> e1000_i211) { >>> + if (igb_read_reg_gs40g(&adapter->hw, data->phy_id, >>> + data->reg_num & 0x1F, >>> + &data->val_out)) >>> + return -EIO; >>> + } else { >>> + if (igb_read_phy_reg(&adapter->hw, data->reg_num & >>> 0x1F, >>> + &data->val_out)) >>> + return -EIO; >>> + } >>> break; >>> case SIOCSMIIREG: >>> + if (hw->mac.type == e1000_i210 || hw->mac.type == >>> e1000_i211) { >>> + if (igb_write_reg_gs40g(hw, data->phy_id, >>> + data->reg_num & 0x1F, >>> + data->val_in)) >>> + return -EIO; >>> + } else { >>> + if (igb_write_phy_reg(hw, data->reg_num & 0x1F, >>> + data->val_in)) >>> + return -EIO; >>> + } >>> + break; >>> default: >>> return -EOPNOTSUPP; >>> } >>> >> These changes just shouldn't be needed. I'd say they could be dropped. The >> phy_id should be static and configured by the hardware before the driver is >> even loaded. > The above also adds SIOCGMIIREG which previously was not supported, > but I also need to allow multiple phy addresses here and thus can't > just default to igb_write_phy_reg() which uses the single phy addr > from the EEPROM (unless I move multi-addr support there). > > Tim The problem with SIOCSMIIREG is that you are basically opening the door for a user-space driver of some sort to manage the PHY, or at least that is what I would assume. The Cumulus guys had a similar patch that they have withdrawn as it was mostly just for debugging. You might want to pull this block out and place it in some sort of separate patch. You would need to justify why you need to expose this functionality.
On Mon, May 11, 2015 at 1:44 PM, Alexander Duyck < alexander.h.duyck@redhat.com> wrote: > On 05/11/2015 11:42 AM, Tim Harvey wrote: >> >> On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck >> <alexander.duyck@gmail.com> wrote: >>> >>> On 04/30/2015 11:19 AM, Tim Harvey wrote: >>>> >>>> If an i210 is configured for 1000BASE-BX link_mode and has an external >>>> phy specified, then register an mii bus using the external phy address >>>> as >>>> a mask. >>>> >>>> An i210 hooked to an external standard phy will be configured with a >>>> link_mode of SGMII in which case phy ops will be configured and used >>>> internal in the igb driver for link status. However, in certain cases >>>> one might be using a backplane SerDes connection to something that talks >>>> on the mdio bus but is not a standard phy, such as a switch. In this >>>> case >>>> by registering an mdio bus a phy driver can manage the device. >>>> >>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >>> >>> >>> >>> So I think I am staring to see a pattern here between the i210 and the >>> i354 >>> it looks like the EEPROMs for these interfaces are not getting set up >>> correctly. Unfortunately for the i210 you cannot change the EEPROM from >>> ethtool so you would need to work out with your manufacturer how to get >>> that >>> fixed for your device. >> >> Alexander, >> >> Thanks for the review! >> >> I think much of my patchset is more clear if I give a bit more >> background of how we use the i210 on our GW16083 >> (http://www.gateworks.com/product/item/ventana-gw16083-mezzanine). >> This board is an i210 connected to a Marvell MV88E6176 6-port switch >> via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and >> instead of having a register interface that utilizes a single phy >> address, it uses several addresses between 0x10 and 0x1d. The link >> between the two chips is thus 1gbps full-duplex without the need of >> link management (why I consider it phy-less). > > > Okay, so this is likely much more complicated then your original patch let > on. What you are actually doing is enabling phylib so that you can enable a > DSA switch on top of the i210 interface, do I have that right? correct but note that the MV88E6176 data interface to the i210 is SERDES, not SGMII. > <snip> > > The 1000BASE-KX is really meant for pure SerDes configuration where there > will be no PHY attached at all. So for example if you were going to connect > directly to another 1000BASE-KX port on either a backplane switch or another > adapter. It is implied that by using this you aren't going to use the MDIO > interface at all. There are essentially 3 modes that do external 1gbs > 'phy-less' and the problem is that KX implies a special mode that is > typically used for backplane w/o any link negotiation. Since there is a PHY > like entity on the other end you would want SGMII, and in this case the > variant that uses the MDIO registers for configuration. > > The 1000Base-BX was probably closer to what you had envisioned. That is > meant to be used for pure SerDes with some sort of transceiver in between. > As such it is normally used for configurations such as fiber since it > supports standard 1Gb/s Ethernet over fiber. > > Really if you were planning to connect to an external switch or PHY that > required MDIO configuration you should have gone with SGMII as it is > essentially the same as the 1000Base-BX however it includes the > configuration bits for managing an external PHY over either I2C or the MDIO > interfaces. > <snip> > > The problem is the link mode doesn't tell you anything other than the type > of interface being used. So in your case you want to advertise the > interface as SGMII because you will want SerDes and to perform configuration > via the external MDIO interface. > > <snip >>>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c >>>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c >>>> @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct >>>> e1000_hw >>>> *hw) >>>> switch (link_mode) { >>>> case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: >>>> hw->phy.media_type = e1000_media_type_internal_serdes; >>>> + if (igb_sgmii_uses_mdio_82575(hw)) { >>>> + u32 mdicnfg = rd32(E1000_MDICNFG); >>>> + >>>> + mdicnfg &= E1000_MDICNFG_PHY_MASK; >>>> + hw->phy.addr = mdicnfg >> >>>> E1000_MDICNFG_PHY_SHIFT; >>>> + hw_dbg("1000BASE_KX w/ external MDIO device at >>>> 0x%x\n", >>>> + hw->phy.addr); >>>> + } else { >>>> + hw_dbg("1000BASE_KX"); >>>> + } >>>> break; >>> >>> <snip> > > The problem is KX has a very specific meaning, and is supposed to be > reserved for backplane Ethernet connections between MACs. It makes > assumptions about things that could have an impact on link negotiation. > Odds are if it is working for you it is because it is "good enough" however > it isn't really meant for the type of connection you are using it for. For clarity, our whole discussion about EEPROM configuration (link-mode, external MDIO, and phy-address) revolves around the patch above. I initially did try to go with link-mode of SGMII but I find that hw->phy.media_type = e1000_media_type_copper just plain doesn't work for my configuration and I need hw->phy.media_type = e1000_media_type_internal_serdes. That's when I realized that SERDES != RGMII and that the i210 SerDes interface needs to be powered up instead of setting up the internal phy.ops.read/write/reset media-detect functions. Even though the MV88E6176 has a phy-like interface it uses SerDes wired on the PCB instead of link negotiation. The MAC<->MAC case you describe 1000BASE_KX being intended for is truely what I have but I do want an external MDIO for the purpose of phylib to the switch. I don't know who is currently using 1000BASE_KX but if they are not using external MDIO then my changes should not affect them and the use of MDICNFG.Destination (what igb_sgmii_uses_mdio_82575 uses for i210) makes sense for enabling phylib. The way I read the use of link_mode in igb_get_invariants_82575 before my patch is: - 1000BASE_KX: uses internal serdes instead of copper with phy read/write/reset hooks - SGMII: uses copper with phy read/write/reset hooks and phy negotiation and can be internal or external MDIO - SERDES - reads SFP info via i2c to determine media type I still can see the logic in using a return of -E1000_PHY_ERR from get_invariants also as a condition for enabling phylib support and that my be what Johnathan needs for his phy. > > You might try working things out with Intel to see if you get get either a > driver quirk directly added to igb, or possibly have something added to the > PCI quirks in the Linux kernel. All that really needs to happen is to > update the KX go SGMII at driver load, or during PCI bus probe as the value > should be persistent through reboots. In your EEPROM implementation did you > do anything that could be used to uniquely identify your device. For > example, is there a sub-device or sub-vendor ID registered in the PCI > config? > >> We have been shipping boards for a year supported by a phylib driver >> and it would not be easy to field update these because the i210 nvram >> is only supported via the non-redistributable licensed eepromARMtool >> (yuck!). One of the key U-Boot developers tried to get Intel to allow >> him to add source to U-Boot for EEPROM programming and was denied. >> (again... yuck!). > > > Yeah, very yuck indeed. Unfortunately you are adding PHY support to a link > mode that doesn't support external PHYs. The likelihood of that causing > regressions for other implementations is very high. That is why if nothing > else it would be better to add a specific quirk for your i210 w/ KX than > something that just takes all KX and converts it into something like SGMII. > Assuming you are able to convince me that my EEPROM/NVRAM config 'is' wrong and we somehow figure out how to make it work with SGMII and external PHY support, I can see either a device-tree binding link-mode property being added that overrides EEPROM/NVRAM or a PCI quirk/fixup working. > <snip> > > Okay, so in the case of your switch the phy_mask applies to probe only then. > I was thinking of traditional PHYs where there is usually only one address > that applies to the PHY belonging to a given MAC. > > What you may want to do is look at adding your own igb_mii_write/read > functions based off of something like the igb_(write/read)_phy_reg_82580 > code. The gs40g has some very specific logic built into it to convert a 32b > register into a page and offset which you likely don't need. The only > change you would need from the 82580 functions would be to add a few lines > for configuring the MDICNFG address. Yes this is where I need advise from people like you that know the various chipsets supported by igb. I have no idea what is old, what is new, what is fragile and shouldn't be touched. The igb driver is a big mysterious (hate to say mess) of chip identifiers and its difficult to understand exactly what Intel chipsets are supported (would love to see a list/table in Kconfig or source!). So I patched the generic igb_{read,write}_phy_reg_mdic to accept an addr which are called by both igb_read_phy_reg_82580 and igb_read_phy_reg_gs40g. I have no idea what the gs40g is, but note that igb_{read,write}_phy_reg_gs40g are used for i210/i211 for copper media mode phy negotiation. What enum e1000_mac_type values are capable of MDIO and applicable for phylib? Do some/all of these have a register bit such as the i210 MDICNFG.Destination that indicates external MDIO vs internal that can be used? What is the difference between an 82580 and 82585 and how do i350/i354 and i210/i211 relate to them? > > I wouldn't bother with restoring the address after you have performed your > operation. There is always a possible scenario where something gets hung or > crashes in the middle of your operation and that will foul up the MDICNFG > register. Instead I would recommend looking at something like modifying > igb_reset_mdicnfg_82580 as it might be preferable to have it apply to all > hardware 82580 and newer instead of just limiting it to 82580. That way the > value is restored on reset or driver load so that as long as the EEPROM is > there you will always read the correct value. Makes sense, but this would involve the chip information that I'm asking for above - I only know the i210. When you say '82580 and newer' I don't know what that means. It sounds like your talking about various enum e1000_mac_type values. > > >>>> +/* Probe the mdio bus for phys and connect them */ >>>> +static int igb_enet_mii_probe(struct net_device *netdev) >>>> +{ >>>> + struct igb_adapter *adapter = netdev_priv(netdev); >>>> + struct e1000_hw *hw = &adapter->hw; >>>> + struct phy_device *phy_dev = NULL; >>>> + int phy_id; >>>> + >>>> + /* check for attached phy */ >>>> + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { >>>> + if (hw->mii_bus->phy_map[phy_id]) { >>>> + phy_dev = hw->mii_bus->phy_map[phy_id]; >>>> + break; >>>> + } >>>> + } >>> >>> >>> There is code that should have already figured this out in the driver. We >>> just need to check the PHY addr after get_invarians has been called, >>> assuming the EEPROM is correct. So you should be able to just get the >>> phy.addr from the e1000_hw struct. >> >> It is true that in igb_enet_mii_init() where the mii bus is registered >> with phylib that I set the phy_mask to include only the phy address >> from the eeprom. In my case I put 0x10 in the EEPROM as the phy >> address as that is the first phy address that the switch responds to >> and so I use that as a way to detect the phy. >> >> I can remove the loop and just set phy_dev = >> hw->mii_bus->phy_map[hw->phy.addr] in this case. > > > Yes, that probably would be cleaner. this is in my pending v2 changes > > <snip> > > I really think you would be better off just performing any kind of check in > igb_probe. Specifically if the link_mode is SGMII, and there is a PHY, but > it is unrecognized then we should be using phylib and should setup an > mii_bus. That way it should play well with other implementations such as > the stuff the Cumulus guys will need. Otherwise the mii_bus should not be > configured. Then all of the other calls can simply check to see if the > mii_bus exists and queue off of that for the phylib calls. > agreed - this is in my pending v2 changes to make the decision to register the MDIO bus in igb_probe (as a field in struct igb_adapter now) of the and the other places that operate on it just check for a non-null adapter->mii_bus. > <snip> > > The problem with SIOCSMIIREG is that you are basically opening the door for > a user-space driver of some sort to manage the PHY, or at least that is what > I would assume. The Cumulus guys had a similar patch that they have > withdrawn as it was mostly just for debugging. You might want to pull this > block out and place it in some sort of separate patch. You would need to > justify why you need to expose this functionality. agreed - I will separate this into a different patch. I think SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace tools like ethtool and others that allow direct mii register access. I'm not clear what the general consusus is but there do seem to be many ethernet drivers that support SIOCMIIREG including the phylib default ioctl handler. Regards, Tim
On 05/12/2015 03:37 PM, Tim Harvey wrote: > On Mon, May 11, 2015 at 1:44 PM, Alexander Duyck > <alexander.h.duyck@redhat.com <mailto:alexander.h.duyck@redhat.com>> > wrote: > > On 05/11/2015 11:42 AM, Tim Harvey wrote: > >> > >> On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck > >> <alexander.duyck@gmail.com <mailto:alexander.duyck@gmail.com>> wrote: > >>> > >>> On 04/30/2015 11:19 AM, Tim Harvey wrote: > >>>> > >>>> If an i210 is configured for 1000BASE-BX link_mode and has an > external > >>>> phy specified, then register an mii bus using the external phy > address > >>>> as > >>>> a mask. > >>>> > >>>> An i210 hooked to an external standard phy will be configured with a > >>>> link_mode of SGMII in which case phy ops will be configured and used > >>>> internal in the igb driver for link status. However, in certain cases > >>>> one might be using a backplane SerDes connection to something > that talks > >>>> on the mdio bus but is not a standard phy, such as a switch. In this > >>>> case > >>>> by registering an mdio bus a phy driver can manage the device. > >>>> > >>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com > <mailto:tharvey@gateworks.com>> > >>> > >>> > >>> > >>> So I think I am staring to see a pattern here between the i210 and the > >>> i354 > >>> it looks like the EEPROMs for these interfaces are not getting set up > >>> correctly. Unfortunately for the i210 you cannot change the > EEPROM from > >>> ethtool so you would need to work out with your manufacturer how > to get > >>> that > >>> fixed for your device. > >> > >> Alexander, > >> > >> Thanks for the review! > >> > >> I think much of my patchset is more clear if I give a bit more > >> background of how we use the i210 on our GW16083 > >> (http://www.gateworks.com/product/item/ventana-gw16083-mezzanine). > >> This board is an i210 connected to a Marvell MV88E6176 6-port switch > >> via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and > >> instead of having a register interface that utilizes a single phy > >> address, it uses several addresses between 0x10 and 0x1d. The link > >> between the two chips is thus 1gbps full-duplex without the need of > >> link management (why I consider it phy-less). > > > > > > Okay, so this is likely much more complicated then your original > patch let > > on. What you are actually doing is enabling phylib so that you can > enable a > > DSA switch on top of the i210 interface, do I have that right? > > correct but note that the MV88E6176 data interface to the i210 is > SERDES, not SGMII. > It is essentially the same thing. Either of those two modes should work if I recall correctly. The only problem is the expectations on the PHY in the case of SGMII. > > > <snip> > > > > The 1000BASE-KX is really meant for pure SerDes configuration where > there > > will be no PHY attached at all. So for example if you were going to > connect > > directly to another 1000BASE-KX port on either a backplane switch or > another > > adapter. It is implied that by using this you aren't going to use > the MDIO > > interface at all. There are essentially 3 modes that do external 1gbs > > 'phy-less' and the problem is that KX implies a special mode that is > > typically used for backplane w/o any link negotiation. Since there > is a PHY > > like entity on the other end you would want SGMII, and in this case the > > variant that uses the MDIO registers for configuration. > > > > The 1000Base-BX was probably closer to what you had envisioned. That is > > meant to be used for pure SerDes with some sort of transceiver in > between. > > As such it is normally used for configurations such as fiber since it > > supports standard 1Gb/s Ethernet over fiber. > > > > Really if you were planning to connect to an external switch or PHY that > > required MDIO configuration you should have gone with SGMII as it is > > essentially the same as the 1000Base-BX however it includes the > > configuration bits for managing an external PHY over either I2C or > the MDIO > > interfaces. > > > <snip> > > > > The problem is the link mode doesn't tell you anything other than > the type > > of interface being used. So in your case you want to advertise the > > interface as SGMII because you will want SerDes and to perform > configuration > > via the external MDIO interface. > > > > > <snip > >>>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c > >>>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c > >>>> @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct > >>>> e1000_hw > >>>> *hw) > >>>> switch (link_mode) { > >>>> case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: > >>>> hw->phy.media_type = > e1000_media_type_internal_serdes; > >>>> + if (igb_sgmii_uses_mdio_82575(hw)) { > >>>> + u32 mdicnfg = rd32(E1000_MDICNFG); > >>>> + > >>>> + mdicnfg &= E1000_MDICNFG_PHY_MASK; > >>>> + hw->phy.addr = mdicnfg >> > >>>> E1000_MDICNFG_PHY_SHIFT; > >>>> + hw_dbg("1000BASE_KX w/ external MDIO > device at > >>>> 0x%x\n", > >>>> + hw->phy.addr); > >>>> + } else { > >>>> + hw_dbg("1000BASE_KX"); > >>>> + } > >>>> break; > >>> > >>> > <snip> > > > > The problem is KX has a very specific meaning, and is supposed to be > > reserved for backplane Ethernet connections between MACs. It makes > > assumptions about things that could have an impact on link negotiation. > > Odds are if it is working for you it is because it is "good enough" > however > > it isn't really meant for the type of connection you are using it for. > > For clarity, our whole discussion about EEPROM configuration > (link-mode, external MDIO, and phy-address) revolves around the patch > above. Yes. KX has a very specific meaning in terms of what is present on the adapter. > I initially did try to go with link-mode of SGMII but I find that > hw->phy.media_type = e1000_media_type_copper just plain doesn't work > for my configuration and I need hw->phy.media_type = > e1000_media_type_internal_serdes. That's when I realized that SERDES > != RGMII and that the i210 SerDes interface needs to be powered up > instead of setting up the internal phy.ops.read/write/reset > media-detect functions. Even though the MV88E6176 has a phy-like > interface it uses SerDes wired on the PCB instead of link negotiation. So does SGMII. SGMII is essentially just GMII over SerDes. If you look at the code for SGMII it is going through and powering on the SerDes link and then trying to send commands across the MII interface to the PHY, if that fails it tries to get the info over I2C. The problems you ran into are likely due to the fact that the switch was being treated as an unsupported PHY. From what I can tell there would be a couple of spots that you need to skip over code if you have an "unknown phy" that is being handled by phylib. > > The MAC<->MAC case you describe 1000BASE_KX being intended for is > truely what I have but I do want an external MDIO for the purpose of > phylib to the switch. I don't know who is currently using 1000BASE_KX > but if they are not using external MDIO then my changes should not > affect them and the use of MDICNFG.Destination (what > igb_sgmii_uses_mdio_82575 uses for i210) makes sense for enabling phylib. The problem is what you have isn't MAC<->MAC. If it was we wouldn't need the MDIO interface and that is where things get messy. It isn't clearly documented what the behavior is if you try to read I2C/MDIO pins that aren't connected to anything and in the KX case that is exactly what we could be looking at. The other problem is we have no way to verify what bits are or are not set in any of the number of KX interfaces out there. This could very well introduce some very odd issues if floating pins are read/written, or unused EEPROM bits are now read. > > The way I read the use of link_mode in igb_get_invariants_82575 before > my patch is: > - 1000BASE_KX: uses internal serdes instead of copper with phy > read/write/reset hooks > - SGMII: uses copper with phy read/write/reset hooks and phy > negotiation and can be internal or external MDIO SGMII uses SerDes as well. So for all intents and purposes it is SerDes that includes supports reading a PHY over either I2C or MDIO. > - SERDES - reads SFP info via i2c to determine media type This probably would have been a better match for your setup than the KX, the difference between the two is that SerDes is technically 1000Base-BX which is your standard 1Gb/s over SerDes, and you are expected to have the I2C pins connected to something either MDIO or I2C based. > > I still can see the logic in using a return of -E1000_PHY_ERR from > get_invariants also as a condition for enabling phylib support and > that my be what Johnathan needs for his phy. Yes you would probably need to return something like -E1000_ERR_UNKNOWN_PHY (error value TBD) and then you might also need to modify the code in a few spots so that if the phy_type values is phy_unknown you don't poke any of the registers and just leave it to phylib. Either that or you could have the media type changed back to serdes from copper when you detect an unknown PHY. We will also should audit the driver code to clean out all of the spots that think that the only external PHY supported is a Marvell such as igb_phy_hw_reset_sgmii_82575(). > > > > > You might try working things out with Intel to see if you get get > either a > > driver quirk directly added to igb, or possibly have something added > to the > > PCI quirks in the Linux kernel. All that really needs to happen is to > > update the KX go SGMII at driver load, or during PCI bus probe as > the value > > should be persistent through reboots. In your EEPROM implementation > did you > > do anything that could be used to uniquely identify your device. For > > example, is there a sub-device or sub-vendor ID registered in the PCI > > config? > > > >> We have been shipping boards for a year supported by a phylib driver > >> and it would not be easy to field update these because the i210 nvram > >> is only supported via the non-redistributable licensed eepromARMtool > >> (yuck!). One of the key U-Boot developers tried to get Intel to allow > >> him to add source to U-Boot for EEPROM programming and was denied. > >> (again... yuck!). > > > > > > Yeah, very yuck indeed. Unfortunately you are adding PHY support to > a link > > mode that doesn't support external PHYs. The likelihood of that causing > > regressions for other implementations is very high. That is why if > nothing > > else it would be better to add a specific quirk for your i210 w/ KX than > > something that just takes all KX and converts it into something like > SGMII. > > > > Assuming you are able to convince me that my EEPROM/NVRAM config 'is' > wrong and we somehow figure out how to make it work with SGMII and > external PHY support, I can see either a device-tree binding link-mode > property being added that overrides EEPROM/NVRAM or a PCI quirk/fixup > working. That works. All you really need is something that will overwrite the link mode bits in the CTRL_EXT register before the driver is loaded. If I recall those bits are not cleared in reset so you would only need to do it once. The problem is we have to make multiple scenarios work with the phylib logic and SGMII would be the preferred type to have reported since you aren't using I2C. As I said though if we can workaround it in software it would be best to have something that makes it clear this is a quirk so it doesn't become the way things are done as otherwise it will just make stuff messier. > > > > <snip> > > > > Okay, so in the case of your switch the phy_mask applies to probe > only then. > > I was thinking of traditional PHYs where there is usually only one > address > > that applies to the PHY belonging to a given MAC. > > > > What you may want to do is look at adding your own igb_mii_write/read > > functions based off of something like the igb_(write/read)_phy_reg_82580 > > code. The gs40g has some very specific logic built into it to > convert a 32b > > register into a page and offset which you likely don't need. The only > > change you would need from the 82580 functions would be to add a few > lines > > for configuring the MDICNFG address. > > Yes this is where I need advise from people like you that know the > various chipsets supported by igb. I have no idea what is old, what is > new, what is fragile and shouldn't be touched. The igb driver is a big > mysterious (hate to say mess) of chip identifiers and its difficult to > understand exactly what Intel chipsets are supported (would love to > see a list/table in Kconfig or source!). > > So I patched the generic igb_{read,write}_phy_reg_mdic to accept an > addr which are called by both igb_read_phy_reg_82580 and > igb_read_phy_reg_gs40g. I have no idea what the gs40g is, but note > that igb_{read,write}_phy_reg_gs40g are used for i210/i211 for copper > media mode phy negotiation. What enum e1000_mac_type values are > capable of MDIO and applicable for phylib? Do some/all of these have a > register bit such as the i210 MDICNFG.Destination that indicates > external MDIO vs internal that can be used? > > What is the difference between an 82580 and 82585 and how do i350/i354 > and i210/i211 relate to them? The 82580 is basically the head of family for the i350/i354/i210/i211 parts. The reason why I suggested using it is because it is a simplified version of things if I recall correctly as the PHY only had the standard registers and didn't go into the pages like the other parts did to support things like energy efficient Ethernet. The extra overhead for the other read/write_phy calls is from the fact that they encoded the page number into the PHY offsets and had to update the page number on each read/write to a register to make sure they accessed the correct page in the PHY. > > > > > I wouldn't bother with restoring the address after you have > performed your > > operation. There is always a possible scenario where something gets > hung or > > crashes in the middle of your operation and that will foul up the > MDICNFG > > register. Instead I would recommend looking at something like modifying > > igb_reset_mdicnfg_82580 as it might be preferable to have it apply > to all > > hardware 82580 and newer instead of just limiting it to 82580. That > way the > > value is restored on reset or driver load so that as long as the > EEPROM is > > there you will always read the correct value. > > Makes sense, but this would involve the chip information that I'm > asking for above - I only know the i210. Well I worked on the 82580 and i350 back in the day. The i350/i354 should be able to take the same workaround with no side effects. All you are doing is restoring bits that would have been written from the EEPROM at power on. Although I just realized that the workaround doesn't cover the PHY address itself. For that you would have to read NVM_INIT_CTRL_4. The register layout is defined in the i210 data sheet, and it should be the same for every part since the 82580. > > When you say '82580 and newer' I don't know what that means. It sounds > like your talking about various enum e1000_mac_type values. > Yes, if you look into igb_reset_mdicnfg_82580 there is a bit of logic that says hw->mac.type == e1000_mac_82580, what I am suggesting is you replace the == with >=. > > > > > >>>> +/* Probe the mdio bus for phys and connect them */ > >>>> +static int igb_enet_mii_probe(struct net_device *netdev) > >>>> +{ > >>>> + struct igb_adapter *adapter = netdev_priv(netdev); > >>>> + struct e1000_hw *hw = &adapter->hw; > >>>> + struct phy_device *phy_dev = NULL; > >>>> + int phy_id; > >>>> + > >>>> + /* check for attached phy */ > >>>> + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { > >>>> + if (hw->mii_bus->phy_map[phy_id]) { > >>>> + phy_dev = hw->mii_bus->phy_map[phy_id]; > >>>> + break; > >>>> + } > >>>> + } > >>> > >>> > >>> There is code that should have already figured this out in the > driver. We > >>> just need to check the PHY addr after get_invarians has been called, > >>> assuming the EEPROM is correct. So you should be able to just get the > >>> phy.addr from the e1000_hw struct. > >> > >> It is true that in igb_enet_mii_init() where the mii bus is registered > >> with phylib that I set the phy_mask to include only the phy address > >> from the eeprom. In my case I put 0x10 in the EEPROM as the phy > >> address as that is the first phy address that the switch responds to > >> and so I use that as a way to detect the phy. > >> > >> I can remove the loop and just set phy_dev = > >> hw->mii_bus->phy_map[hw->phy.addr] in this case. > > > > > > Yes, that probably would be cleaner. > > this is in my pending v2 changes Okay. > > > > > > <snip> > > > > I really think you would be better off just performing any kind of > check in > > igb_probe. Specifically if the link_mode is SGMII, and there is a > PHY, but > > it is unrecognized then we should be using phylib and should setup an > > mii_bus. That way it should play well with other implementations > such as > > the stuff the Cumulus guys will need. Otherwise the mii_bus should > not be > > configured. Then all of the other calls can simply check to see if the > > mii_bus exists and queue off of that for the phylib calls. > > > > agreed - this is in my pending v2 changes to make the decision to > register the MDIO bus in igb_probe (as a field in struct igb_adapter > now) of the and the other places that operate on it just check for a > non-null adapter->mii_bus. > Okay. > > > <snip> > > > > The problem with SIOCSMIIREG is that you are basically opening the > door for > > a user-space driver of some sort to manage the PHY, or at least that > is what > > I would assume. The Cumulus guys had a similar patch that they have > > withdrawn as it was mostly just for debugging. You might want to > pull this > > block out and place it in some sort of separate patch. You would > need to > > justify why you need to expose this functionality. > > agreed - I will separate this into a different patch. I think > SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace > tools like ethtool and others that allow direct mii register access. > I'm not clear what the general consusus is but there do seem to be > many ethernet drivers that support SIOCMIIREG including the phylib > default ioctl handler. My advice would be to leave the SIOCGMIIREG as is but add the ability to read any phy address/offset pair for e1000_phy_unknown. I would say you could probably make SIOCSMIIREG only work w/ e1000_phy_unknown, as that way you allow managing a PHY that cannot be managed by the driver.
On 5/12/15 6:37 PM, Tim Harvey wrote: > > agreed - I will separate this into a different patch. I think > SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace > tools like ethtool and others that allow direct mii register access. I'm > not clear what the general consusus is but there do seem to be many > ethernet drivers that support SIOCMIIREG including the phylib default > ioctl handler. I am on holiday, will get a chance to read over the thread this weekend. Regards, -Jon
On Thu, May 14, 2015 at 9:08 PM, Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote: > On 5/12/15 6:37 PM, Tim Harvey wrote: >> >> >> agreed - I will separate this into a different patch. I think >> SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace >> tools like ethtool and others that allow direct mii register access. I'm >> not clear what the general consusus is but there do seem to be many >> ethernet drivers that support SIOCMIIREG including the phylib default >> ioctl handler. > > > I am on holiday, will get a chance to read over the thread this weekend. > > Regards, > -Jon Jon, I will be posting a followup patch very soon (hopefully in the next couple of days) for adding phylib support to igb hopefully addressing Alexander's comments and concerns noted in this thread. My phylib phy driver is an un-posted work-in-progress and because my phy operates likely different from yours, you will need to write a phylib phy driver to test the patch when its posted. Tim
On 5/20/15 11:46 AM, Tim Harvey wrote: > On Thu, May 14, 2015 at 9:08 PM, Jonathan Toppins > <jtoppins@cumulusnetworks.com> wrote: >> On 5/12/15 6:37 PM, Tim Harvey wrote: >>> >>> >>> agreed - I will separate this into a different patch. I think >>> SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace >>> tools like ethtool and others that allow direct mii register access. I'm >>> not clear what the general consusus is but there do seem to be many >>> ethernet drivers that support SIOCMIIREG including the phylib default >>> ioctl handler. >> >> >> I am on holiday, will get a chance to read over the thread this weekend. >> >> Regards, >> -Jon > > Jon, > > I will be posting a followup patch very soon (hopefully in the next > couple of days) for adding phylib support to igb hopefully addressing > Alexander's comments and concerns noted in this thread. My phylib phy > driver is an un-posted work-in-progress and because my phy operates > likely different from yours, you will need to write a phylib phy > driver to test the patch when its posted. > > Tim > Thanks for the update Tim. I have not had time to look at this for now but also have not seen your v2 series either, did I miss that? -Jon
On Fri, May 29, 2015 at 8:26 AM, Jonathan Toppins <jtoppins@cumulusnetworks.com> wrote: > On 5/20/15 11:46 AM, Tim Harvey wrote: <snip> > > Thanks for the update Tim. I have not had time to look at this for now but > also have not seen your v2 series either, did I miss that? > > -Jon Jon, You haven't missed it - I've gotten pulled off onto some other things and haven't gotten back to it. Where it stands right now I have a patch that should be completely 'safe' and eliminate any backwards compatibility concerns however I'm not convinced I'm skipping enough phy related code when phylib mode is used. Arguably those could come later as issues are found with phylib phy drivers, but I feel like if I can just get another half day into it I could possibly work out those issues as well. Tim
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c index d2afd7b..e80617b 100644 --- a/drivers/net/ethernet/intel/igb/e1000_82575.c +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c @@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw *hw) switch (link_mode) { case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX: hw->phy.media_type = e1000_media_type_internal_serdes; + if (igb_sgmii_uses_mdio_82575(hw)) { + u32 mdicnfg = rd32(E1000_MDICNFG); + + mdicnfg &= E1000_MDICNFG_PHY_MASK; + hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT; + hw_dbg("1000BASE_KX w/ external MDIO device at 0x%x\n", + hw->phy.addr); + } else { + hw_dbg("1000BASE_KX"); + } break; case E1000_CTRL_EXT_LINK_MODE_SGMII: /* Get phy control interface type set (MDIO vs. I2C)*/ if (igb_sgmii_uses_mdio_82575(hw)) { hw->phy.media_type = e1000_media_type_copper; dev_spec->sgmii_active = true; + hw_dbg("SGMII with external MDIO PHY"); break; + } else { + hw_dbg("SGMII with external I2C PHY"); } /* fall through for I2C based SGMII */ case E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES: @@ -621,8 +634,11 @@ static s32 igb_get_invariants_82575(struct e1000_hw *hw) hw->phy.media_type = e1000_media_type_copper; dev_spec->sgmii_active = true; } + hw_dbg("SERDES with external SFP"); break; + } else { + hw_dbg("SERDES"); } /* do not change link mode for 100BaseFX */ diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h index 2003b37..2864779 100644 --- a/drivers/net/ethernet/intel/igb/e1000_hw.h +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h @@ -27,6 +27,7 @@ #include <linux/delay.h> #include <linux/io.h> #include <linux/netdevice.h> +#include <linux/phy.h> #include "e1000_regs.h" #include "e1000_defines.h" @@ -543,6 +544,12 @@ struct e1000_hw { struct e1000_mbx_info mbx; struct e1000_host_mng_dhcp_cookie mng_cookie; +#ifdef CONFIG_PHYLIB + /* Phylib and MDIO interface */ + struct mii_bus *mii_bus; + struct phy_device *phy_dev; + phy_interface_t phy_interface; +#endif union { struct e1000_dev_spec_82575 _82575; } dev_spec; diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index f366b3b..9b5f538 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -41,6 +41,7 @@ #include <linux/if_vlan.h> #include <linux/pci.h> #include <linux/pci-aspm.h> +#include <linux/phy.h> #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/ip.h> @@ -2223,6 +2224,121 @@ static s32 igb_init_i2c(struct igb_adapter *adapter) return status; } +#ifdef CONFIG_PHYLIB +static int igb_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum) +{ + struct e1000_hw *hw = bus->priv; + u16 out; + int err; + + err = igb_read_reg_gs40g(hw, mii_id, regnum, &out); + if (err) + return err; + return out; +} + +static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, + u16 val) +{ + struct e1000_hw *hw = bus->priv; + + return igb_write_reg_gs40g(hw, mii_id, regnum, val); +} + +static int igb_enet_mdio_reset(struct mii_bus *bus) +{ + usleep_range(1000, 2000); + return 0; +} + +static void igb_enet_mii_link(struct net_device *netdev) +{ +} + +/* Probe the mdio bus for phys and connect them */ +static int igb_enet_mii_probe(struct net_device *netdev) +{ + struct igb_adapter *adapter = netdev_priv(netdev); + struct e1000_hw *hw = &adapter->hw; + struct phy_device *phy_dev = NULL; + int phy_id; + + /* check for attached phy */ + for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) { + if (hw->mii_bus->phy_map[phy_id]) { + phy_dev = hw->mii_bus->phy_map[phy_id]; + break; + } + } + if (!phy_dev) { + netdev_err(netdev, "no PHY found\n"); + return -ENODEV; + } + + hw->phy_interface = PHY_INTERFACE_MODE_RGMII; + phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev), + igb_enet_mii_link, hw->phy_interface); + if (IS_ERR(phy_dev)) { + netdev_err(netdev, "could not attach to PHY\n"); + return PTR_ERR(phy_dev); + } + + hw->phy_dev = phy_dev; + netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n", + hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev)); + + return 0; +} + +/* Create and register mdio bus */ +static int igb_enet_mii_init(struct pci_dev *pdev) +{ + struct mii_bus *mii_bus; + struct net_device *netdev = pci_get_drvdata(pdev); + struct igb_adapter *adapter = netdev_priv(netdev); + struct e1000_hw *hw = &adapter->hw; + int err; + + mii_bus = mdiobus_alloc(); + if (!mii_bus) { + err = -ENOMEM; + goto err_out; + } + + mii_bus->name = "igb_enet_mii_bus"; + mii_bus->read = igb_enet_mdio_read; + mii_bus->write = igb_enet_mdio_write; + mii_bus->reset = igb_enet_mdio_reset; + snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", + pci_name(pdev), hw->device_id + 1); + mii_bus->priv = hw; + mii_bus->parent = &pdev->dev; + mii_bus->phy_mask = ~(1 << hw->phy.addr); + + err = mdiobus_register(mii_bus); + if (err) { + netdev_err(netdev, "failed to register mii_bus: %d\n", err); + goto err_out_free_mdiobus; + } + hw->mii_bus = mii_bus; + + return 0; + +err_out_free_mdiobus: + mdiobus_free(mii_bus); +err_out: + return err; +} + +static void igb_enet_mii_remove(struct e1000_hw *hw) +{ + if (hw->mii_bus) { + mdiobus_unregister(hw->mii_bus); + mdiobus_free(hw->mii_bus); + } +} +#endif /* CONFIG_PHYLIB */ + /** * igb_probe - Device Initialization Routine * @pdev: PCI device information struct @@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) } } pm_runtime_put_noidle(&pdev->dev); + +#ifdef CONFIG_PHYLIB + /* create and register the mdio bus if using ext phy */ + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) + igb_enet_mii_init(pdev); +#endif + return 0; err_register: @@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev) struct e1000_hw *hw = &adapter->hw; pm_runtime_get_noresume(&pdev->dev); +#ifdef CONFIG_PHYLIB + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) + igb_enet_mii_remove(hw); +#endif #ifdef CONFIG_IGB_HWMON igb_sysfs_exit(adapter); #endif @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, bool resuming) if (!resuming) pm_runtime_put(&pdev->dev); +#ifdef CONFIG_PHYLIB + /* Probe and connect to PHY if using ext phy */ + if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO) + igb_enet_mii_probe(netdev); +#endif + /* start the watchdog. */ hw->mac.get_link_status = 1; schedule_work(&adapter->watchdog_task); @@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count) static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) { struct igb_adapter *adapter = netdev_priv(netdev); + struct e1000_hw *hw = &adapter->hw; struct mii_ioctl_data *data = if_mii(ifr); - if (adapter->hw.phy.media_type != e1000_media_type_copper) + if (adapter->hw.phy.media_type != e1000_media_type_copper && + !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)) return -EOPNOTSUPP; switch (cmd) { case SIOCGMIIPHY: - data->phy_id = adapter->hw.phy.addr; + data->phy_id = hw->phy.addr; break; case SIOCGMIIREG: - if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, - &data->val_out)) - return -EIO; + if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) { + if (igb_read_reg_gs40g(&adapter->hw, data->phy_id, + data->reg_num & 0x1F, + &data->val_out)) + return -EIO; + } else { + if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F, + &data->val_out)) + return -EIO; + } break; case SIOCSMIIREG: + if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) { + if (igb_write_reg_gs40g(hw, data->phy_id, + data->reg_num & 0x1F, + data->val_in)) + return -EIO; + } else { + if (igb_write_phy_reg(hw, data->reg_num & 0x1F, + data->val_in)) + return -EIO; + } + break; default: return -EOPNOTSUPP; }
If an i210 is configured for 1000BASE-BX link_mode and has an external phy specified, then register an mii bus using the external phy address as a mask. An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something that talks on the mdio bus but is not a standard phy, such as a switch. In this case by registering an mdio bus a phy driver can manage the device. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/net/ethernet/intel/igb/e1000_82575.c | 16 +++ drivers/net/ethernet/intel/igb/e1000_hw.h | 7 ++ drivers/net/ethernet/intel/igb/igb_main.c | 163 ++++++++++++++++++++++++++- 3 files changed, 181 insertions(+), 5 deletions(-)