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

Hi Russell,

On Wed, Nov 23, 2022 at 01:11:23PM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 23, 2022 at 12:08:11PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 22, 2022 at 09:36:59PM +0200, Vladimir Oltean wrote:
> > > I think we're in agreement, but please let's wait until tomorrow, I need
> > > to take a break for today.
> > 
> > I think we do have a sort of agreement... but lets give this a go. The
> > following should be sufficient for copper SFP modules using the 88E1111
> > PHY. However, I haven't build-tested this patch yet.
> > 
> > Reading through the documentation has brought up some worms in this
> > area. :(
> > 
> > It may be worth printing the fiber page BMCR and extsr at various
> > strategic points in this driver and reporting back if things don't
> > seem to be working right for your modules. In the mean time, I'll try
> > to see how the modules in the Honeycomb appear to be setup at power-up
> > and after the driver has configured the PHY... assuming I left both
> > MicroUSBs connected and the board has a network connection via the
> > main ethernet jack.
> 
> Unfortunately, I don't have a SFP with an 88e1111 plugged in, only the
> bcm84881, so I can't test my patch remotely. However, it builds fine
> when the appropriate TIMEOUT definition is added.

Sorry for the delay. Had to do something else yesterday and the day before.

I tested the patch and it does detect the operating mode of my PHY.

My modules are these:

[    6.465788] sfp sfp-0: module UBNT  UF-RJ45-1G  rev 1.0  sn X20072804742  dc 200617
ethtool -m dpmac7
        Identifier              : 0x03 (SFP)
        Extended identifier     : 0x04 (GBIC/SFP defined by 2-wire interface ID)
        Connector               : 0x00 (unknown or unspecified)
        Transceiver codes       : 0x00 0x00 0x00 0x08 0x00 0x00 0x00 0x00 0x00
        Transceiver type        : Ethernet: 1000BASE-T
        Encoding                : 0x01 (8B/10B)
        BR, Nominal             : 1300MBd
        Rate identifier         : 0x00 (unspecified)
        Length (SMF,km)         : 0km
        Length (SMF)            : 0m
        Length (50um)           : 0m
        Length (62.5um)         : 0m
        Length (Copper)         : 100m
        Length (OM3)            : 0m
        Laser wavelength        : 0nm
        Vendor name             : UBNT
        Vendor OUI              : 00:00:00
        Vendor PN               : UF-RJ45-1G
        Vendor rev              : 1.0
        Option values           : 0x00 0x1a
        Option                  : RX_LOS implemented
        Option                  : TX_FAULT implemented
        Option                  : TX_DISABLE implemented
        BR margin, max          : 0%
        BR margin, min          : 0%
        Vendor SN               : X20072804742
        Date code               : 200617

Here is how the PHY driver does a few things:

[ 3079.596985] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[ 3079.689892] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[ 3079.696826] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode
[ 3079.779656] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR 0x1140
[ 3079.865386] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)

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.

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.

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?

Should we call m88e1111_validate_an_inband() also for the Finisar
variant of the 88E1111? What about 88E1112?