| Message ID | 20201019204913.467287-1-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell: add special handling of Finisar modules with 81E1111 | expand |
> +static int m88e1111_finisar_config_init(struct phy_device *phydev) > +{ > + int err; > + int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR); > + > + if (extsr < 0) > + return extsr; > + > + /* If using 1000BaseX and 1000BaseX auto-negotiation is disabled, enable it */ > + if (phydev->interface == PHY_INTERFACE_MODE_1000BASEX && > + (extsr & MII_M1111_HWCFG_MODE_MASK) == > + MII_M1111_HWCFG_MODE_COPPER_1000BX_NOAN) { > + err = phy_modify(phydev, MII_M1111_PHY_EXT_SR, > + MII_M1111_HWCFG_MODE_MASK | > + MII_M1111_HWCFG_SERIAL_AN_BYPASS, > + MII_M1111_HWCFG_MODE_COPPER_1000BX_AN | > + MII_M1111_HWCFG_SERIAL_AN_BYPASS); > + if (err < 0) > + return err; > + } > + > + return m88e1111_config_init(phydev); > +} Hi Robert Is this really specific to the Finisar? It seems like any application of the m88e1111 in 1000BaseX would benefit from this? Andrew
On Mon, Oct 19, 2020 at 02:49:13PM -0600, Robert Hancock wrote: > The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel 81E1111 PHY > with a modified PHY ID, and by default does not have 1000BaseX > auto-negotiation enabled, which is not generally desirable with Linux > networking drivers. I could be wrong, but i thought phylink used SGMII with copper SFPs, so that 10/100 works as well as 1G. So why is 1000BaseX an issue? Do you have a MAC which cannot do SGMII, only 1000BaseX? Andrew
On Mon, Oct 19, 2020 at 02:49:13PM -0600, Robert Hancock wrote: > The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel 81E1111 PHY You mean 88E1111 here. > with a modified PHY ID, and by default does not have 1000BaseX > auto-negotiation enabled, which is not generally desirable with Linux > networking drivers. Add handling to enable 1000BaseX auto-negotiation. > Also, it requires some special handling to ensure that 1000BaseT auto- > negotiation is enabled properly when desired. > > Based on existing handling in the AMD xgbe driver and the information in > the Finisar FAQ: > https://www.finisar.com/sites/default/files/resources/an-2036_1000base-t_sfp_faqreve1.pdf There's lots of modules that have the 88E1111 PHY on, and we switch it to SGMII mode if it's not already in SGMII mode if we have access to it. Why is your setup different?
On Mon, Oct 19, 2020 at 11:06:54PM +0200, Andrew Lunn wrote: > On Mon, Oct 19, 2020 at 02:49:13PM -0600, Robert Hancock wrote: > > The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel 81E1111 PHY > > with a modified PHY ID, and by default does not have 1000BaseX > > auto-negotiation enabled, which is not generally desirable with Linux > > networking drivers. > > I could be wrong, but i thought phylink used SGMII with copper SFPs, > so that 10/100 works as well as 1G. So why is 1000BaseX an issue? > Do you have a MAC which cannot do SGMII, only 1000BaseX? Indeed it does.
On Mon, 2020-10-19 at 22:08 +0100, Russell King - ARM Linux admin wrote: > On Mon, Oct 19, 2020 at 02:49:13PM -0600, Robert Hancock wrote: > > The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel > > 81E1111 PHY > > You mean 88E1111 here. > Whoops, will fix in an updated version. > > with a modified PHY ID, and by default does not have 1000BaseX > > auto-negotiation enabled, which is not generally desirable with > > Linux > > networking drivers. Add handling to enable 1000BaseX auto- > > negotiation. > > Also, it requires some special handling to ensure that 1000BaseT > > auto- > > negotiation is enabled properly when desired. > > > > Based on existing handling in the AMD xgbe driver and the > > information in > > the Finisar FAQ: > > https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.finisar.com%2Fsites%2Fdefault%2Ffiles%2Fresources%2Fan-2036_1000base-t_sfp_faqreve1.pdf&data=04%7C01%7Crobert.hancock%40calian.com%7C6eda7d636fbf4a70ff2408d8747332a2%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387385403382018%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cfChA4TBw3f70alrANXPR0HHgNg3Vs%2FNeEYhzZc%2BF9A%3D&reserved=0 > > There's lots of modules that have the 88E1111 PHY on, and we switch > it to SGMII mode if it's not already in SGMII mode if we have access > to it. Why is your setup different? This is in our setup using the Xilinx axienet driver, which is stuck with whatever interface mode the FPGA logic is set up for at synthesis time. In our case since we need to support fiber modules, that means we are stuck with 1000BaseX mode with the copper modules as well. Note that there is some more work that needs to be done for this to work completely, as phylink currently will only attempt to use SGMII with copper modules and fails out if that's not supported. I have a local patch that just falls back to trying 1000BaseX mode if the driver reports SGMII isn't supported and it seems like it might be a copper module, but that is a bit of a hack that may need to be handled differently.
On Mon, 2020-10-19 at 23:00 +0200, Andrew Lunn wrote: > > +static int m88e1111_finisar_config_init(struct phy_device *phydev) > > +{ > > + int err; > > + int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR); > > + > > + if (extsr < 0) > > + return extsr; > > + > > + /* If using 1000BaseX and 1000BaseX auto-negotiation is > > disabled, enable it */ > > + if (phydev->interface == PHY_INTERFACE_MODE_1000BASEX && > > + (extsr & MII_M1111_HWCFG_MODE_MASK) == > > + MII_M1111_HWCFG_MODE_COPPER_1000BX_NOAN) { > > + err = phy_modify(phydev, MII_M1111_PHY_EXT_SR, > > + MII_M1111_HWCFG_MODE_MASK | > > + MII_M1111_HWCFG_SERIAL_AN_BYPASS, > > + MII_M1111_HWCFG_MODE_COPPER_1000BX_AN > > | > > + MII_M1111_HWCFG_SERIAL_AN_BYPASS); > > + if (err < 0) > > + return err; > > + } > > + > > + return m88e1111_config_init(phydev); > > +} > > Hi Robert > > Is this really specific to the Finisar? It seems like any application > of the m88e1111 in 1000BaseX would benefit from this? I suppose that part would be pretty harmless, as you would likely want that behavior whenever that if condition was triggered. So m88e1111_finisar_config_init could likely be merged into m88e1111_config_init. Mainly what stopped me from making all of these changes generic to all 88E1111 is that I'm a bit less confident in the different config_aneg behavior, it might be more specific to this SFP copper module case?
> I have a local patch that just falls back to trying 1000BaseX mode > if the driver reports SGMII isn't supported and it seems like it > might be a copper module, but that is a bit of a hack that may need > to be handled differently. Do you also modify what the PHY is advertising to remove the modes your cannot support? Andrew
On Mon, 2020-10-19 at 23:45 +0200, Andrew Lunn wrote: > > I have a local patch that just falls back to trying 1000BaseX mode > > if the driver reports SGMII isn't supported and it seems like it > > might be a copper module, but that is a bit of a hack that may need > > to be handled differently. > > Do you also modify what the PHY is advertising to remove the modes > your cannot support? I think in my case those extra modes only supported in SGMII mode, like 10 and 100Mbps modes, effectively get filtered out because the MAC doesn't support them in the 1000BaseX mode either. But yes, that probably should be fixed up in the PHY capabilities in a "proper" solution. The auto-negotiation is a bit of a weird thing in this case, as there are two negotiations occurring, the 1000BaseX between the PCS/PMA PHY and the module PHY, and the 1000BaseT between the module PHY and the copper link partner. I believe the 88E1111 has some smarts to delay the copper negotiation until it gets the advertisement over 1000BaseX, uses those to figure out its advertisement, and then uses the copper link partner's response to determine the 1000BaseX response.
> I suppose that part would be pretty harmless, as you would likely want > that behavior whenever that if condition was triggered. So > m88e1111_finisar_config_init could likely be merged into > m88e1111_config_init. I think so as well. > Mainly what stopped me from making all of these changes generic to all > 88E1111 is that I'm a bit less confident in the different config_aneg > behavior, it might be more specific to this SFP copper module case? Well, for 1000BaseX, i don't think we currently have an SFP devices using it, since phylink does not support it. So it is a question are there any none-SFP m88e1111 out there you might break? Andrew
On Mon, Oct 19, 2020 at 09:32:56PM +0000, Robert Hancock wrote: > On Mon, 2020-10-19 at 22:08 +0100, Russell King - ARM Linux admin > wrote: > > On Mon, Oct 19, 2020 at 02:49:13PM -0600, Robert Hancock wrote: > > > The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel > > > 81E1111 PHY > > > > You mean 88E1111 here. > > > > Whoops, will fix in an updated version. > > > > with a modified PHY ID, and by default does not have 1000BaseX > > > auto-negotiation enabled, which is not generally desirable with > > > Linux > > > networking drivers. Add handling to enable 1000BaseX auto- > > > negotiation. > > > Also, it requires some special handling to ensure that 1000BaseT > > > auto- > > > negotiation is enabled properly when desired. > > > > > > Based on existing handling in the AMD xgbe driver and the > > > information in > > > the Finisar FAQ: > > > https://can01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.finisar.com%2Fsites%2Fdefault%2Ffiles%2Fresources%2Fan-2036_1000base-t_sfp_faqreve1.pdf&data=04%7C01%7Crobert.hancock%40calian.com%7C6eda7d636fbf4a70ff2408d8747332a2%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387385403382018%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cfChA4TBw3f70alrANXPR0HHgNg3Vs%2FNeEYhzZc%2BF9A%3D&reserved=0 > > > > There's lots of modules that have the 88E1111 PHY on, and we switch > > it to SGMII mode if it's not already in SGMII mode if we have access > > to it. Why is your setup different? > > This is in our setup using the Xilinx axienet driver, which is stuck > with whatever interface mode the FPGA logic is set up for at synthesis > time. In our case since we need to support fiber modules, that means we > are stuck with 1000BaseX mode with the copper modules as well. Hmm, okay. > Note that there is some more work that needs to be done for this to > work completely, as phylink currently will only attempt to use SGMII > with copper modules and fails out if that's not supported. I have a > local patch that just falls back to trying 1000BaseX mode if the driver > reports SGMII isn't supported and it seems like it might be a copper > module, but that is a bit of a hack that may need to be handled > differently. I have worked on patches that implement a replacement system of working out which interface mode to use, not only for optical modules, but also for PHYs as well. It needs network drivers and PHYs to advertise a bitmap of which PHY_INTERFACE_MODE_xxx each support, and we use an ordered list of preferred modes to find the most suitable interface mode supported by both the network driver and the module/PHY. I never fully finished the patches though. PHYs need to fill in this bitmap before they are bound to a network driver, because we need to work out which interface mode to use before we can attach the PHY. The patches for this are in my net-queue branch on git://git.armlinux.org.uk/~rmk/linux-arm.git/
On Mon, 2020-10-19 at 23:59 +0200, Andrew Lunn wrote: > > I suppose that part would be pretty harmless, as you would likely > > want > > that behavior whenever that if condition was triggered. So > > m88e1111_finisar_config_init could likely be merged into > > m88e1111_config_init. > > I think so as well. > > > Mainly what stopped me from making all of these changes generic to > > all > > 88E1111 is that I'm a bit less confident in the different > > config_aneg > > behavior, it might be more specific to this SFP copper module > > case? > > Well, for 1000BaseX, i don't think we currently have an SFP devices > using it, since phylink does not support it. So it is a question are > there any none-SFP m88e1111 out there you might break? Yeah, I don't really know the answer to that question as I'm not familiar with all the other setups where this part would be used. So I'm inclined to leave that part specific to this ID.
> I think in my case those extra modes only supported in SGMII mode, like > 10 and 100Mbps modes, effectively get filtered out because the MAC > doesn't support them in the 1000BaseX mode either. There are different things here. What ethtool reports, and what is programmed into the advertise register. Clearly, you don't want it advertising 10 and 100 modes. If somebody connects it to a 10Half link partner, you need auto-get to fail. You don't want auto-neg to succeed, and then all the frames get thrown away with bad CRCs, overruns etc. > The auto-negotiation is a bit of a weird thing in this case, as there > are two negotiations occurring, the 1000BaseX between the PCS/PMA PHY > and the module PHY, and the 1000BaseT between the module PHY and the > copper link partner. I believe the 88E1111 has some smarts to delay the > copper negotiation until it gets the advertisement over 1000BaseX, uses > those to figure out its advertisement, and then uses the copper link > partner's response to determine the 1000BaseX response. But as far as i know you can only report duplex and pause over 1000BaseX, not speed, since it is always 1G. It would be interesting to run ethtool on the link partner to see what it thinks the SFP is advertising. If it just 1000BaseT/Full? Andrew
On Tue, Oct 20, 2020 at 12:12:32AM +0200, Andrew Lunn wrote: > > The auto-negotiation is a bit of a weird thing in this case, as there > > are two negotiations occurring, the 1000BaseX between the PCS/PMA PHY > > and the module PHY, and the 1000BaseT between the module PHY and the > > copper link partner. I believe the 88E1111 has some smarts to delay the > > copper negotiation until it gets the advertisement over 1000BaseX, uses > > those to figure out its advertisement, and then uses the copper link > > partner's response to determine the 1000BaseX response. > > But as far as i know you can only report duplex and pause over > 1000BaseX, not speed, since it is always 1G. That is correct.
On Tue, 2020-10-20 at 00:12 +0200, Andrew Lunn wrote: > > I think in my case those extra modes only supported in SGMII mode, > > like > > 10 and 100Mbps modes, effectively get filtered out because the MAC > > doesn't support them in the 1000BaseX mode either. > > There are different things here. What ethtool reports, and what is > programmed into the advertise register. Clearly, you don't want it > advertising 10 and 100 modes. If somebody connects it to a 10Half > link > partner, you need auto-get to fail. You don't want auto-neg to > succeed, and then all the frames get thrown away with bad CRCs, > overruns etc. > Right, I don't know that we have direct control over what the PHY is advertising to the copper side in this case. I think it's based on its auto-magical translation of the 1000BaseX advertisements from the PCS/PMA PHY in this case, where we only ever advertise 1000 modes in 1000BaseX mode (not sure if any other speeds are even defined for those messages). Presumably the 88E1111 is smart enough to only advertise 1000 modes on the copper side when in 1000BaseX mode. > > The auto-negotiation is a bit of a weird thing in this case, as > > there > > are two negotiations occurring, the 1000BaseX between the PCS/PMA > > PHY > > and the module PHY, and the 1000BaseT between the module PHY and > > the > > copper link partner. I believe the 88E1111 has some smarts to delay > > the > > copper negotiation until it gets the advertisement over 1000BaseX, > > uses > > those to figure out its advertisement, and then uses the copper > > link > > partner's response to determine the 1000BaseX response. > > But as far as i know you can only report duplex and pause over > 1000BaseX, not speed, since it is always 1G. > > It would be interesting to run ethtool on the link partner to see > what > it thinks the SFP is advertising. If it just 1000BaseT/Full? Well the device is plugged into a Linux PC on the other end, but unfortunately ethtool doesn't seem to report the link partner advertisements on that adapter, only the host advertisements (Intel 82574L, e1000e).
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 5aec673a0120..8d85c96209ad 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -80,8 +80,11 @@ #define MII_M1111_HWCFG_MODE_FIBER_RGMII 0x3 #define MII_M1111_HWCFG_MODE_SGMII_NO_CLK 0x4 #define MII_M1111_HWCFG_MODE_RTBI 0x7 +#define MII_M1111_HWCFG_MODE_COPPER_1000BX_AN 0x8 #define MII_M1111_HWCFG_MODE_COPPER_RTBI 0x9 #define MII_M1111_HWCFG_MODE_COPPER_RGMII 0xb +#define MII_M1111_HWCFG_MODE_COPPER_1000BX_NOAN 0xc +#define MII_M1111_HWCFG_SERIAL_AN_BYPASS BIT(12) #define MII_M1111_HWCFG_FIBER_COPPER_RES BIT(13) #define MII_M1111_HWCFG_FIBER_COPPER_AUTO BIT(15) @@ -629,6 +632,39 @@ static int marvell_config_aneg_fiber(struct phy_device *phydev) return genphy_check_and_restart_aneg(phydev, changed); } +static int m88e1111_finisar_config_aneg(struct phy_device *phydev) +{ + int err; + + err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE); + if (err < 0) + goto error; + + /* Configure the copper link first */ + err = marvell_config_aneg(phydev); + if (err < 0) + goto error; + + /* Do not touch the fiber page if we're in copper->sgmii mode */ + if (phydev->interface == PHY_INTERFACE_MODE_SGMII) + return 0; + + /* Then the fiber link */ + err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE); + if (err < 0) + goto error; + + err = marvell_config_aneg_fiber(phydev); + if (err < 0) + goto error; + + return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE); + +error: + marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE); + return err; +} + static int m88e1510_config_aneg(struct phy_device *phydev) { int err; @@ -843,6 +879,30 @@ static int m88e1111_config_init(struct phy_device *phydev) return genphy_soft_reset(phydev); } +static int m88e1111_finisar_config_init(struct phy_device *phydev) +{ + int err; + int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR); + + if (extsr < 0) + return extsr; + + /* If using 1000BaseX and 1000BaseX auto-negotiation is disabled, enable it */ + if (phydev->interface == PHY_INTERFACE_MODE_1000BASEX && + (extsr & MII_M1111_HWCFG_MODE_MASK) == + MII_M1111_HWCFG_MODE_COPPER_1000BX_NOAN) { + err = phy_modify(phydev, MII_M1111_PHY_EXT_SR, + MII_M1111_HWCFG_MODE_MASK | + MII_M1111_HWCFG_SERIAL_AN_BYPASS, + MII_M1111_HWCFG_MODE_COPPER_1000BX_AN | + MII_M1111_HWCFG_SERIAL_AN_BYPASS); + if (err < 0) + return err; + } + + return m88e1111_config_init(phydev); +} + static int m88e1111_get_downshift(struct phy_device *phydev, u8 *data) { int val, cnt, enable; @@ -2672,6 +2732,27 @@ static struct phy_driver marvell_drivers[] = { .get_tunable = m88e1111_get_tunable, .set_tunable = m88e1111_set_tunable, }, + { + .phy_id = MARVELL_PHY_ID_88E1111_FINISAR, + .phy_id_mask = MARVELL_PHY_ID_MASK, + .name = "Marvell 88E1111 (Finisar)", + /* PHY_GBIT_FEATURES */ + .probe = marvell_probe, + .config_init = &m88e1111_finisar_config_init, + .config_aneg = &m88e1111_finisar_config_aneg, + .read_status = &marvell_read_status, + .ack_interrupt = &marvell_ack_interrupt, + .config_intr = &marvell_config_intr, + .resume = &genphy_resume, + .suspend = &genphy_suspend, + .read_page = marvell_read_page, + .write_page = marvell_write_page, + .get_sset_count = marvell_get_sset_count, + .get_strings = marvell_get_strings, + .get_stats = marvell_get_stats, + .get_tunable = m88e1111_get_tunable, + .set_tunable = m88e1111_set_tunable, + }, { .phy_id = MARVELL_PHY_ID_88E1118, .phy_id_mask = MARVELL_PHY_ID_MASK, @@ -2989,6 +3070,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = { { MARVELL_PHY_ID_88E1101, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1112, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1111, MARVELL_PHY_ID_MASK }, + { MARVELL_PHY_ID_88E1111_FINISAR, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1118, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1121R, MARVELL_PHY_ID_MASK }, { MARVELL_PHY_ID_88E1145, MARVELL_PHY_ID_MASK }, diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h index ff7b7607c8cf..52b1610eae68 100644 --- a/include/linux/marvell_phy.h +++ b/include/linux/marvell_phy.h @@ -25,6 +25,9 @@ #define MARVELL_PHY_ID_88X3310 0x002b09a0 #define MARVELL_PHY_ID_88E2110 0x002b09b0 +/* Marvel 88E1111 in Finisar SFP module with modified PHY ID */ +#define MARVELL_PHY_ID_88E1111_FINISAR 0x01ff0cc0 + /* The MV88e6390 Ethernet switch contains embedded PHYs. These PHYs do * not have a model ID. So the switch driver traps reads to the ID2 * register and returns the switch family ID
The Finisar FCLF8520P2BTL 1000BaseT SFP module uses a Marvel 81E1111 PHY with a modified PHY ID, and by default does not have 1000BaseX auto-negotiation enabled, which is not generally desirable with Linux networking drivers. Add handling to enable 1000BaseX auto-negotiation. Also, it requires some special handling to ensure that 1000BaseT auto- negotiation is enabled properly when desired. Based on existing handling in the AMD xgbe driver and the information in the Finisar FAQ: https://www.finisar.com/sites/default/files/resources/an-2036_1000base-t_sfp_faqreve1.pdf Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/net/phy/marvell.c | 82 +++++++++++++++++++++++++++++++++++++ include/linux/marvell_phy.h | 3 ++ 2 files changed, 85 insertions(+)