netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
@ 2020-10-19 20:49 Robert Hancock
  2020-10-19 21:00 ` Andrew Lunn
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 20:49 UTC (permalink / raw)
  To: andrew, hkallweit1; +Cc: linux, davem, netdev, Robert Hancock

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(+)

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
-- 
2.18.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 20:49 [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111 Robert Hancock
@ 2020-10-19 21:00 ` Andrew Lunn
  2020-10-19 21:43   ` Robert Hancock
  2020-10-19 21:06 ` Andrew Lunn
  2020-10-19 21:08 ` Russell King - ARM Linux admin
  2 siblings, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2020-10-19 21:00 UTC (permalink / raw)
  To: Robert Hancock; +Cc: hkallweit1, linux, davem, netdev

> +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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 20:49 [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111 Robert Hancock
  2020-10-19 21:00 ` Andrew Lunn
@ 2020-10-19 21:06 ` Andrew Lunn
  2020-10-19 21:09   ` Russell King - ARM Linux admin
  2020-10-19 21:08 ` Russell King - ARM Linux admin
  2 siblings, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2020-10-19 21:06 UTC (permalink / raw)
  To: Robert Hancock; +Cc: hkallweit1, linux, davem, netdev

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 20:49 [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111 Robert Hancock
  2020-10-19 21:00 ` Andrew Lunn
  2020-10-19 21:06 ` Andrew Lunn
@ 2020-10-19 21:08 ` Russell King - ARM Linux admin
  2020-10-19 21:32   ` Robert Hancock
  2 siblings, 1 reply; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2020-10-19 21:08 UTC (permalink / raw)
  To: Robert Hancock; +Cc: andrew, hkallweit1, davem, netdev

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:06 ` Andrew Lunn
@ 2020-10-19 21:09   ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2020-10-19 21:09 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Robert Hancock, hkallweit1, davem, netdev

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:08 ` Russell King - ARM Linux admin
@ 2020-10-19 21:32   ` Robert Hancock
  2020-10-19 21:45     ` Andrew Lunn
  2020-10-19 22:03     ` Russell King - ARM Linux admin
  0 siblings, 2 replies; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 21:32 UTC (permalink / raw)
  To: linux; +Cc: hkallweit1, davem, netdev, andrew

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&amp;data=04%7C01%7Crobert.hancock%40calian.com%7C6eda7d636fbf4a70ff2408d8747332a2%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387385403382018%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=cfChA4TBw3f70alrANXPR0HHgNg3Vs%2FNeEYhzZc%2BF9A%3D&amp;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.

-- 
Robert Hancock
Senior Hardware Designer, Advanced Technologies 
www.calian.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:00 ` Andrew Lunn
@ 2020-10-19 21:43   ` Robert Hancock
  2020-10-19 21:59     ` Andrew Lunn
  0 siblings, 1 reply; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 21:43 UTC (permalink / raw)
  To: andrew; +Cc: linux, hkallweit1, davem, netdev

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? 

-- 
Robert Hancock
Senior Hardware Designer, Advanced Technologies 
www.calian.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:32   ` Robert Hancock
@ 2020-10-19 21:45     ` Andrew Lunn
  2020-10-19 21:59       ` Robert Hancock
  2020-10-19 22:03     ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2020-10-19 21:45 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux, hkallweit1, davem, netdev

> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:45     ` Andrew Lunn
@ 2020-10-19 21:59       ` Robert Hancock
  2020-10-19 22:12         ` Andrew Lunn
  0 siblings, 1 reply; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 21:59 UTC (permalink / raw)
  To: andrew; +Cc: linux, hkallweit1, davem, netdev

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.

-- 
Robert Hancock
Senior Hardware Designer, Advanced Technologies 
www.calian.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:43   ` Robert Hancock
@ 2020-10-19 21:59     ` Andrew Lunn
  2020-10-19 22:11       ` Robert Hancock
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2020-10-19 21:59 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux, hkallweit1, davem, netdev

> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:32   ` Robert Hancock
  2020-10-19 21:45     ` Andrew Lunn
@ 2020-10-19 22:03     ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2020-10-19 22:03 UTC (permalink / raw)
  To: Robert Hancock; +Cc: hkallweit1, davem, netdev, 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&amp;data=04%7C01%7Crobert.hancock%40calian.com%7C6eda7d636fbf4a70ff2408d8747332a2%7C23b57807562f49ad92c43bb0f07a1fdf%7C0%7C0%7C637387385403382018%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=cfChA4TBw3f70alrANXPR0HHgNg3Vs%2FNeEYhzZc%2BF9A%3D&amp;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/

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:59     ` Andrew Lunn
@ 2020-10-19 22:11       ` Robert Hancock
  0 siblings, 0 replies; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 22:11 UTC (permalink / raw)
  To: andrew; +Cc: linux, hkallweit1, davem, netdev

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.

-- 
Robert Hancock
Senior Hardware Designer, Advanced Technologies 
www.calian.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 21:59       ` Robert Hancock
@ 2020-10-19 22:12         ` Andrew Lunn
  2020-10-19 22:21           ` Russell King - ARM Linux admin
  2020-10-19 22:26           ` Robert Hancock
  0 siblings, 2 replies; 15+ messages in thread
From: Andrew Lunn @ 2020-10-19 22:12 UTC (permalink / raw)
  To: Robert Hancock; +Cc: linux, hkallweit1, davem, netdev

> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 22:12         ` Andrew Lunn
@ 2020-10-19 22:21           ` Russell King - ARM Linux admin
  2020-10-19 22:26           ` Robert Hancock
  1 sibling, 0 replies; 15+ messages in thread
From: Russell King - ARM Linux admin @ 2020-10-19 22:21 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Robert Hancock, hkallweit1, davem, netdev

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111
  2020-10-19 22:12         ` Andrew Lunn
  2020-10-19 22:21           ` Russell King - ARM Linux admin
@ 2020-10-19 22:26           ` Robert Hancock
  1 sibling, 0 replies; 15+ messages in thread
From: Robert Hancock @ 2020-10-19 22:26 UTC (permalink / raw)
  To: andrew; +Cc: linux, hkallweit1, davem, netdev

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).

-- 
Robert Hancock
Senior Hardware Designer, Advanced Technologies 
www.calian.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-10-19 22:26 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19 20:49 [PATCH] net: phy: marvell: add special handling of Finisar modules with 81E1111 Robert Hancock
2020-10-19 21:00 ` Andrew Lunn
2020-10-19 21:43   ` Robert Hancock
2020-10-19 21:59     ` Andrew Lunn
2020-10-19 22:11       ` Robert Hancock
2020-10-19 21:06 ` Andrew Lunn
2020-10-19 21:09   ` Russell King - ARM Linux admin
2020-10-19 21:08 ` Russell King - ARM Linux admin
2020-10-19 21:32   ` Robert Hancock
2020-10-19 21:45     ` Andrew Lunn
2020-10-19 21:59       ` Robert Hancock
2020-10-19 22:12         ` Andrew Lunn
2020-10-19 22:21           ` Russell King - ARM Linux admin
2020-10-19 22:26           ` Robert Hancock
2020-10-19 22:03     ` Russell King - ARM Linux admin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).