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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> Hi Vladimir,
> 
> On Fri, Nov 25, 2022 at 02:30:42PM +0200, Vladimir Oltean wrote:
> > Hi Russell,
> > 
> > Sorry for the delay. Had to do something else yesterday and the day before.
> 
> I think there was some kind of celebration going on in the US for at
> least one of those days...

Yeah, but I don't celebrate that. I had to write some documentation.

> > So the default EXT_SR is being changed by the PHY driver from 0x9088 to
> > 0x9084 (MII_M1111_HWCFG_MODE_COPPER_1000X_AN -> MII_M1111_HWCFG_MODE_SGMII_NO_CLK).
> > 
> > I don't know if it's possible to force these modules to operate in
> > 1000BASE-X mode. If you're interested in the results there, please give
> > me some guidance.
> 
> 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.

Anyway, with sfp_select_interface() hacked, I can confirm it works in
1000BASE-X with AN enabled too.

[   69.746643] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[   69.845784] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[   69.852764] fsl_dpaa2_eth dpni.1 dpmac7: switched to inband/1000base-x link mode // MLO_AN_INBAND
[   69.934191] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
[   70.015735] 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.
64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.874 ms
64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.225 ms
64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
^C
--- 192.168.100.2 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2019ms
rtt min/avg/max/mdev = 0.216/0.438/0.874/0.308 ms

printed with code:

static int m88e1111_config_init_1000basex(struct phy_device *phydev)
{
	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
	int fiber_bmcr_before, fiber_bmcr_after;
	int ext_sr_after;
	int err, mode;

	if (extsr < 0)
		return extsr;

	fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
	if (fiber_bmcr_before < 0)
		return fiber_bmcr_before;

	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
	 * FIXME: does this actually enable 1000BaseX auto-negotiation if it
	 * was previously disabled in the Fiber BMCR? 2.3.1.6 suggests not!
	 */
	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,
				 MII_M1111_HWCFG_MODE_MASK |
				 MII_M1111_HWCFG_SERIAL_AN_BYPASS,
				 MII_M1111_HWCFG_MODE_COPPER_1000X_AN |
				 MII_M1111_HWCFG_SERIAL_AN_BYPASS);
		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__, extsr, ext_sr_after,
		   fiber_bmcr_before, fiber_bmcr_after);
	return 0;
}

Furthermore, I can confirm that if the fiber page BMCR has BMCR_ANENABLE
disabled, the link with my Lynx PCS in MLO_AN_INBAND is broken (and that
the write to EXT_SR doesn't change the value of the BMCR).



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 was curious if the fiber page BMCR has an effect for in-band autoneg,
> > and at least for SGMII it surely does. If I add this to
> > m88e1111_config_init_sgmii():
> > 
> > 	phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> > 			 BMCR_ANENABLE, 0);
> > 
> > (and force an intentional mismatch) then I am able to break the link
> > with my Lynx PCS.
> 
> Yes, the fiber page is re-used for the host side of the link when
> operating in SGMII and 1000baseX modes, so changes there have the
> expected effect.
> 
> > If my hunch is correct that this also holds true for 1000BASE-X, then
> > you are also correct that m88e1111_config_init_1000basex() only allows
> > AN bypass but does not seem to enable in-band AN in itself, if it wasn't
> > enabled.
> > 
> > The implication here is that maybe we should test for the fiber page
> > BMCR in both SGMII and 1000BASE-X modes?
> 
> 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);
}