Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sun, Nov 27, 2022 at 10:14:45PM +0000, Russell King (Oracle) wrote:
> On Fri, Nov 25, 2022 at 05:35:55PM +0200, Vladimir Oltean wrote:
> > On Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> > > The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
> > > sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
> > > using 1000BASEX instead (and it should work, although at fixed 1G
> > > speeds). The only reason the module is working in SGMII mode is because,
> > > as you've noticed above, we switch it to SGMII mode in
> > > m88e1111_config_init_sgmii().
> > 
> > Which is an interesting thing, because m88e1111_config_init_1000basex()
> > does not change the HWCFG_MODE_MASK to something with 1000X in it.
> 
> It only changes the hwcfg mode if it was using 1000base-X no-AN -
> switching it instead to be 1000base-X with AN, but as we've established
> the comment above that code describes something which doesn't happen,
> as the fibre page BMCR is unaffected by this change.
> 
> Anyway, with my SourcePhotonics SPGBTXCNFC module (which is a SGMII
> module) I get:
> 
> Marvell 88E1111 i2c:sfp-3:16: extsr: 8084 fiber bmcr: 1140
> 
> although the first time I plugged it in, BMCR was 1940 (pdown set).
> Key thing is this module doesn't have bypass permitted.
> 
> > But there's actually a problem (or maybe two problems).
> > 
> > First is that if I make phylink treat the ON_TIMEOUT capability by using
> > MLO_AN_PHY (basically like this):
> > 
> > phylink_sfp_config_phy():
> > 
> > 	/* Select whether to operate in in-band mode or not, based on the
> > 	 * capability of the PHY in the current link mode.
> > 	 */
> > 	ret = phy_validate_an_inband(phy, iface);
> > 	phylink_err(pl, "PHY driver reported AN inband 0x%x\n", ret);
> > 	if (ret == PHY_AN_INBAND_UNKNOWN) {
> > 		mode = MLO_AN_INBAND;
> > 
> > 		phylink_dbg(pl,
> > 			    "PHY driver does not report in-band autoneg capability, assuming true\n");
> > //	} else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
> > 	} else if (ret & PHY_AN_INBAND_ON) {
> > 		mode = MLO_AN_INBAND;
> > 	} else {
> > 		mode = MLO_AN_PHY;
> > 	}
> > 
> > [   30.059923] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> > [   30.066867] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode // MLO_AN_PHY
> > [   30.153350] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
> > [   30.238970] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > then pinging is broken with mismatched in-band AN settings ("TIMEOUT" in
> > PHY, "OFF" in PCS). I triple-checked this.
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > From 192.168.100.1 icmp_seq=1 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=2 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=3 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=4 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=5 Destination Host Unreachable
> > From 192.168.100.1 icmp_seq=6 Destination Host Unreachable
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 9 packets transmitted, 0 received, +6 errors, 100% packet loss, time 8170ms
> > 
> > 
> > However, if using the same phylink code (to force a mismatch), I unhack
> > sfp_select_interface() and use SGMII mode, the timeout feature does
> > actually work:
> > 
> > [   30.262979] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
> > [   30.270349] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode // MLO_AN_PHY
> > [   30.351066] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x1140 after 0x1140
> > [   30.433236] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > this is a functional link despite the mismatched settings.
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > 64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.885 ms
> > 64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.221 ms
> > 64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
> > 64 bytes from 192.168.100.2: icmp_seq=4 ttl=64 time=0.217 ms
> > 64 bytes from 192.168.100.2: icmp_seq=5 ttl=64 time=0.238 ms
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 5 packets transmitted, 5 received, 0% packet loss, time 4062ms
> > rtt min/avg/max/mdev = 0.216/0.355/0.885/0.264 ms
> > 
> > 
> > The second problem is that not even *matched* settings work if I turn
> > off BMCR_ANENABLE in the PHY fiber page.
> > 
> > [   30.809869] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
> > [   30.817936] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x1140 MII_BMSR 0x9 MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x3 // PCS registers at the end of lynx_pcs_config_giga()
> > [   30.917651] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // ignore; m88e1111_validate_an_inband() is hardcoded for this and does not detect BMCR for BASE-X
> > [   30.924571] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode
> > [   30.932441] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x140 MII_BMSR 0xd MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x1
> > [   31.032547] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x140 after 0x140
> > [   31.117668] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
> > 
> > ping 192.168.100.2
> > PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
> > ^C
> > --- 192.168.100.2 ping statistics ---
> > 4 packets transmitted, 0 received, 100% packet loss, time 3058ms
> > 
> > What's common is that if in-band autoneg is turned off (either forced
> > off or via timeout), 1000BASE-X between the Lynx PCS and the 88E1111
> > simply doesn't work.
> 
> I've just tried an experiment here with my SourcePhotonics module.
> 
> I made m88e1111_validate_an_inband() set the SERIAL_AN_BYPASS bit,
> and then the bit I think you're probably unaware of - the PHY needs
> to be soft-reset in order for that change to take effect. Calling
> genphy_soft_reset() is sufficient.
> 
> Then I made m88e1111_validate_an_inband() return PHY_AN_INBAND_OFF.
> So we now have the PHY setup with BMCR=1140 and EXTSR=9084.
> 
> # ping -I eth4 fe80::222:68ff:fe15:37dd
> ping: Warning: source address might be selected on device other than: eth4
> PING fe80::222:68ff:fe15:37dd(fe80::222:68ff:fe15:37dd) from :: eth4: 56 data bytes
> 64 bytes from fe80::222:68ff:fe15:37dd%eth4: icmp_seq=1 ttl=64 time=0.281 ms
> ^C
> --- fe80::222:68ff:fe15:37dd ping statistics ---
> 1 packets transmitted, 1 received, 0% packet loss, time 0ms
> rtt min/avg/max/mdev = 0.281/0.281/0.281/0.000 ms
> 
> (yes, it did use eth4's source address, I checked with tcpdump on the
> target machine.)
> 
> So the link appears to be functional. Using a highly modified mii-diag
> tool that allows me to read/write registers on the PHY, if I read the
> EXT_SR register, it now contains:
> 
> Reading 0x001b=0x9884
> 
> and bit 11 being set means the PHY went into bypass mode. In other
> words, it didn't see the SGMII acknowledgement from the MAC and
> decided to bring the link up in bypass mode.
> 
> However, I've just tripped over some information in the 88E1111
> manual which states that in SGMII mode, if bypass mode is used, then
> the PHY will apparently renegotiate on the copper side advertising
> 1000baseT HD and FD only, no pause. So I checked what my link partner
> is seeing, and it was seeing the original advertisement.
> 
> So I then triggered a renegotiate from the partner, and it now shows
> only 1000baseT/Half 1000baseT/Full being advertised by the 88E1111.
> Reading the advertisement register, it still contains 0x0d41, which
> shows pause modes, 100FD, 10FD - so the advertisement register doesn't
> reflect what was actually adfertised in this case.
> 
> Also, presumably, based on this observation, it will only renegotiate
> if the copper side hadn't resolved to gigabit. If correct, what this
> means is that when operating in SGMII mode, the the PHY becomes
> gigabit-only if bypass mode gets used.
> 
> Given this behaviour, the fact that it switches to gigabit only when
> the SGMII side enters bypass mode, I think we should _positively_ be
> disabling inband bypass in SGMII mode. This change in advertisement
> is not what phylib would expect, and I suspect could lead to surprises
> e.g. if phylib was told to advertise non-gigabit speeds only.
> 
> However, I'll try this test with 1000base-X mode tomorrow.
> 
> > > I think a more comprehensive test would be to write the fiber page
> > > BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
> > > see whether the BMCR changes value. My suspicion is it won't, and
> > > the hwcfg_mode only has an effect on the settings in the fiber page
> > > under hardware reset conditions, and mode changes have no effect on
> > > the fiber page.
> > 
> > Confirmed that changes to the EXT_SR register don't cause changes to the
> > MII_BMCR register:
> > 
> > [   28.587838] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x140 after 0x140
> > 
> > generated by:
> > 
> > static int m88e1111_config_init_sgmii(struct phy_device *phydev)
> > {
> > 	int fiber_bmcr_before, fiber_bmcr_after;
> > 	int ext_sr_before, ext_sr_after;
> > 	int err;
> > 
> > 	ext_sr_before = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> > 	if (ext_sr_before < 0)
> > 		return ext_sr_before;
> > 
> > 	err = phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> > 			       BMCR_ANENABLE, 0);
> > 	if (err < 0)
> > 		return err;
> > 
> > 	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> > 	if (fiber_bmcr_before < 0)
> > 		return fiber_bmcr_before;
> > 
> > 	err = m88e1111_config_init_hwcfg_mode(
> > 		phydev,
> > 		MII_M1111_HWCFG_MODE_SGMII_NO_CLK,
> > 		MII_M1111_HWCFG_FIBER_COPPER_AUTO);
> > 	if (err < 0)
> > 		return err;
> > 
> > 	ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> > 	if (ext_sr_after < 0)
> > 		return ext_sr_after;
> > 
> > 	fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> > 	if (fiber_bmcr_after < 0)
> > 		return fiber_bmcr_after;
> > 
> > 	phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
> > 		   __func__, ext_sr_before, ext_sr_after,
> > 		   fiber_bmcr_before, fiber_bmcr_after);
> > 
> > 	/* make sure copper is selected */
> > 	return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
> > }
> 
> Thanks for testing. So that means m88e1111_config_init_sgmii() will not
> enable in-band if it was previously disabled. So we need to check the
> fiber ANENABLE bit and unconditionally return PHY_AN_INBAND_OFF if it is
> clear before evaluating anything else.
> 
> Also, given this behaviour of bypass mode, it seems it would only make
> sense if the PHY were operating in 1000base-X mode, which we don't do
> with SFPs, so maybe it makes no sense to support the ON_TIMEOUT as an
> option right now - and as I say above, maybe we should be focing the
> AN bypass allow bit to be clear in SGMII mode.
> 
> I think maybe Andrew needs to be involved in that last bit though.

Here's an updated patch.
8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phy: marvell: add validate_an_inband() method

Add the validate_an_inband() method for Marvell 88E1111, the Finisar
version of the 88E1111, and 88E1112.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell.c | 54 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3c54d7d0f17f..1d7e00c4d97a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -669,6 +669,52 @@ static int marvell_config_aneg_fiber(struct phy_device *phydev)
 	return genphy_check_and_restart_aneg(phydev, changed);
 }
 
+static int m88e1111_validate_an_inband(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	int hwcfg_mode, extsr, bmcr;
+
+	if (interface != PHY_INTERFACE_MODE_1000BASEX &&
+	    interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
+	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
+	if (extsr < 0 || bmcr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	/* We make no efforts to enable the ANENABLE bit when switching mode.
+	 * If this bit is clear, then we will not be using in-band signalling.
+	 */
+	if (!(bmcr & BMCR_ANENABLE))
+		return PHY_AN_INBAND_OFF;
+
+	hwcfg_mode = extsr & MII_M1111_HWCFG_MODE_MASK;
+
+	/* If we are in 1000base-X no-AN hwcfg_mode,
+	 * m88e1111_config_init_1000basex() will allow AN bypass, but does not
+	 * enable AN.
+	 */
+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    hwcfg_mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	/* Otherwise, we leave the AN enable bit and the AN bypass bit
+	 * alone, so we need to read the registers to determine how the
+	 * MAC facing side of the PHY has been setup by firmware and/or
+	 * hardware reset.
+	 *
+	 * If the AN enable bit is clear, then all in-band signalling
+	 * on the SGMII/1000base-X side is disabled. Otherwise, AN is
+	 * enabled. If the bypass bit is set, AN can complete without
+	 * a response from the partner (MAC).
+	 */
+	if (extsr & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	return PHY_AN_INBAND_ON;
+}
+
 static int m88e1111_config_aneg(struct phy_device *phydev)
 {
 	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
@@ -915,7 +961,10 @@ static int m88e1111_config_init_1000basex(struct phy_device *phydev)
 	if (extsr < 0)
 		return extsr;
 
-	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled */
+	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
+	 * FIXME: this does not actually enable 1000BaseX auto-negotiation if
+	 * it was previously disabled in the Fiber BMCR!
+	 */
 	mode = extsr & MII_M1111_HWCFG_MODE_MASK;
 	if (mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN) {
 		err = phy_modify(phydev, MII_M1111_PHY_EXT_SR,
@@ -2978,6 +3027,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111,
@@ -2999,6 +3049,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111_FINISAR,
@@ -3020,6 +3071,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1118,
-- 
2.30.2

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