Date:   Tue, 22 Nov 2022 14:11:22 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Sean Anderson <sean.anderson@...o.com>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band
 capability check where it belongs

On Tue, Nov 22, 2022 at 11:16:07AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 22, 2022 at 09:38:43AM +0000, Russell King (Oracle) wrote:
> > Also, if we get the Marvell driver implementing validate_an_inband()
> > then I believe we can get rid of other parts of this patch - 88E1111 is
> > the commonly used accessible PHY on gigabit SFPs, as this PHY implements
> > I2C access natively. As I mentioned, Marvell PHYs can be set to no
> > inband, requiring inband, or inband with bypass mode enabled. So we
> > need to decide how we deal with that - especially if we're going to be
> > changing the mode from 1000base-X to SGMII (which we do on some SFP
> > modules so they work at 10/100/1000.)
> 
> For the Marvell 88E1111:
> 
> - If switching into 1000base-X mode, then bypass mode is enabled by
> m88e1111_config_init_1000basex(). However, if AN is disabled in the
> fibre page BMCR (e.g. by firmware), then AN won't be used.
> 
> - If switching into SGMII mode, then bypass mode is left however it was
> originally set by m88e1111_config_init_sgmii() - so it may be allowed
> or it may be disallowed, which means it's whatever the hardware
> defaulted to or firmware set it as.
> 
> For the 88e151x (x=0,2,8) it looks like bypass mode defaults to being
> allowed on hardware reset, but firmware may change that.
> 
> I don't think we make much of an effort to configure other Marvell
> PHYs, relying on their hardware reset defaults or firmware to set
> them appropriately.
> 
> So, I think for 88e151x, we should implement something like:
> 
> 	int mode, bmcr, fscr2;
> 
> 	/* RGMII too? I believe RGMII can signal inband as well, so we
> 	 * may need to handle that as well.
> 	 */
> 	if (interface != PHY_INTERFACE_MODE_SGMII &&
> 	    interface != PHY_INTERFACE_MODE_1000BASE_X)
> 		return PHY_AN_INBAND_UNKNOWN;
> 
> 	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
> 	if (bmcr < 0)
> 		return SOME_ERROR?

There's a limitation in the API presented here, you can't return
SOME_ERROR, you have to return PHY_AN_INBAND_UNKNOWN and maybe log the
error to the console. If the error persists, other PHY methods will
eventually catch it.

> 
> 	mode = PHY_AN_INBAND_OFF;
> 
> 	if (bmcr & BMCR_ANENABLE) {
> 		mode = PHY_AN_INBAND_ON;
> 
> 		fscr2 = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE,
> 				       0x1a);
> 		if (fscr2 & BIT(6))
> 			mode |= PHY_AN_INBAND_TIMEOUT;
> 	}
> 
> 	return mode;
> 
> Obviously adding register definitions for BIT(6) and 01a.
> 
> For the 88E1111:
> 
> 	int mode, hwcfg;
> 
> 	/* If operating in 1000base-X mode, we always turn on inband
> 	 * and allow bypass.
> 	 */
> 	if (interface == PHY_INTERFACE_MODE_1000BASEX)
> 		return PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT;
> 
> 	if (interface == PHY_INTERFACE_MODE_SGMII) {
> 		hwcfg = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> 		if (hwcfg < 0)
> 			return SOME_ERROR?
> 
> 		mode = PHY_AN_INBAND_ON;
> 		if (hwcfg & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
> 			mode |= PHY_AN_INBAND_TIMEOUT;
> 
> 		return mode;
> 	}
> 
> 	return PHY_AN_INBAND_UNKNOWN;
> 
> Maybe?

Hmm, not quite (neither for 88E151x not 88E1111). The intention with the
validate()/config() split is that you either implement just validate(),
or both. If you implement just validate(), you should report just one
bit, corresponding to what the hardware is configured for (so either
PHY_AN_INBAND_ON, *or* PHY_AN_INBAND_TIMEOUT). This is because you'd
otherwise tell phylink that 2 modes are supported, but provide no way to
choose between them, and you don't make it clear which one is in use
either. This will force phylink to adapt to MLO_AN_PHY or MLO_AN_INBAND,
depending on what has a chance of working.

If you implement config_an_inband() too, then the validate procedure
becomes a simple report of what can be configured for that PHY
(OFF | ON | ON_TIMEOUT for 88E151x, and ON | ON_TIMEOUT for 88E1111).
It's then the config_an_inband() procedure that applies to hardware the
mode that is selected by phylink. From config_an_inband() you can return
a negative error code on PHY I/O failure.

If you can prepare some more formal patches for these PHYs for which I
don't have documentation, I think I have a copper SFP module which uses
SGMII and 88E1111, and I can plug it into the Honeycomb and see what
happens.

Powered by blists - more mailing lists