Date:   Tue, 22 Nov 2022 18:28:38 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <vladimir.oltean@....com>
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 07:56:25PM +0200, Vladimir Oltean wrote:
> The problem is not phy_config_an_inband() but phy_validate_an_inband().
> We call that earlier than phy_attach_direct(), so if the PHY driver is
> going to read a register from HW which hasn't yet been written, we get
> an incorrect report of the current capabilities.

Why would it be "incorrect" ?

What the code I'm proposing correctly reports back what inband mode(s)
will be in use should we select the proposed interface mode. Let's
ignore whether we report the TIMEOUT or not for that statement, because
I think that's confusing the discussion.

If we _do_ want to report whether the TIMEOUT mode is going to be used
or not, the code I proposed is what will be necessary, because it
depends on (a) how the PHY is strapped and (b) how firmware or external
EEPROM has setup the device. If we want a single bit, then we would
report just _ON_TIMEOUT if bypass is enabled - but we still need to
read registers to come to a conclusion about whether it's enabled or
not. As I say, we can't blindly say "if interface is X, then bypass
will be enabled" for any X - and what may be correct for one board will
not be correct for another.

Moreover, in the 88e1111 case on a SFP, what's right for one SFP is not
right for another - there are SFPs where the 88e1111 registers are
preloaded from an EEPROM, so whether bypass is enabled or not in SGMII
mode is up to the contents of the EEPROM - the marvell PHY driver does
not interfere with that setting for SGMII.

Hence, to report how the PHY will behave in SGMII mode, with lack of
explicit configuration, we _have_ to read registers and use them to
determine the outcome.

> > > 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.
> > 
> > Don't we have the same problem with PHY_AN_INBAND_TIMEOUT? If a PHY
> > reports that, do we use MLO_AN_INBAND or MLO_AN_PHY?
> 
> Well, I haven't yet written any logic for it.
> 
> To your question: "PHY_AN_INBAND_ON_TIMEOUT => MLO_AN_PHY or MLO_AN_INBAND"?
> I'd say either depending on situation, since my expectation is that it's
> compatible with both.

Yes, it would be compatible with both.

> Always give preference to what's in the device tree if it can work
> somehow. If it can work in fully compatible modes (MLO_AN_PHY with
> PHY_AN_INBAND_OFF; MLO_AN_INBAND with PHY_AN_INBAND_ON), perfect.
> If not, but what's in the device tree can work with PHY_AN_INBAND_ON_TIMEOUT,
> also good => use ON_TIMEOUT.

What do we do for a SFP module with a Marvell PHY on - we need to cover
that in this thought process, especially as 88e1111 is one of the most
popular PHYs on Gigabit copper SFPs. We can't really say "whatever
DT/ACPI firmware says" because that's not relevant to SFPs (we always
override firmware for SFPs.)

> If what's in the device tree needs to be changed, it pretty much means
> that ON_TIMEOUT isn't supported by the PHY.
> 
> Concretely, I would propose something like this (a modification of the
> function added by the patch set, notice the extra "an_inband" argument,
> as well as the new checks for PHY_AN_INBAND_ON_TIMEOUT):
> 
> static void phylink_sync_an_inband(struct phylink *pl, struct phy_device *phy,
> 				   enum phy_an_inband *an_inband)
> {
> 	unsigned int mode = pl->cfg_link_an_mode;
> 	int ret;
> 
> 	if (!pl->config->sync_an_inband)
> 		return;
> 
> 	ret = phy_validate_an_inband(phy, pl->link_config.interface);
> 	if (ret == PHY_AN_INBAND_UNKNOWN) {
> 		phylink_dbg(pl,
> 			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
> 			    phylink_autoneg_inband(mode) ? "true" : "false");
> 
> 		*an_inband = PHY_AN_INBAND_UNKNOWN;
> 	} else if (phylink_autoneg_inband(mode) &&
> 		   !(ret & PHY_AN_INBAND_ON) &&
> 		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
> 		phylink_err(pl,
> 			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
> 
> 		mode = MLO_AN_PHY;
> 		*an_inband = PHY_AN_INBAND_OFF;
> 	} else if (!phylink_autoneg_inband(mode) &&
> 		   !(ret & PHY_AN_INBAND_OFF) &&
> 		   !(ret & PHY_AN_INBAND_ON_TIMEOUT)) {
> 		phylink_dbg(pl,
> 			    "PHY driver requests in-band autoneg, force-enabling it.\n");
> 
> 		mode = MLO_AN_INBAND;
> 		*an_inband = PHY_AN_INBAND_ON;
> 	} else {
> 		/* For the checks below, we've found a common operating
> 		 * mode with the PHY, just need to figure out if we
> 		 * agree fully or if we have to rely on the PHY's
> 		 * timeout ability
> 		 */
> 		if (phylink_autoneg_inband(mode)) {
> 			*an_inband = !!(ret & PHY_AN_INBAND_ON) ? PHY_AN_INBAND_ON :
> 					PHY_AN_INBAND_ON_TIMEOUT;
> 		} else {
> 			*an_inband = !!(ret & PHY_AN_INBAND_OFF) ? PHY_AN_INBAND_OFF :
> 					PHY_AN_INBAND_ON_TIMEOUT;
> 		}
> 	}
> 
> 	pl->cur_link_an_mode = mode;
> }
> 
> then call phy_config_an_inband() with "an_inband" as the mode to use.
> 
> As per Sean's feedback, we force the PHY to report at least one valid
> capability, otherwise, 0 is PHY_AN_INBAND_UNKNOWN and it's also treated
> correctly.
> 
> > > 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.
> > 
> > So it sounds like the decision about which mode to use needs to be
> > coupled with "does the PHY driver implement config_an_inband()"
> 
> So do you recommend that I should put a WARN_ON() somewhere, which
> asserts something like this?
> 
> "if the weight (number of bits set) in the return code of
> phy_validate_an_inband() is larger than 1, then phydev->drv->phy_config_an_inband()
> must be a non-NULL pointer, to allow selecting between them"

I think that would be a good idea, otherwise we are going to have to
rely on reviewers spotting this error, which given the way patches are
picked up on netdev, is prone to being missed. So, I think the more we
can encourage people to do it correctly first time, the better.

> > > 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.
> > 
> > I'm away from home at the moment, which means I don't have a way to
> > do any in-depth tests other than with the SFPs that are plugged into
> > my Honeycomb - which does include some copper SFPs but they're not
> > connected to anything. So I can't test to see if data passes until
> > I'm back home next week.
> 
> I actually meant that I can test on a Solidrun Honeycomb board that I
> happen to have access to, if you have some Marvell PHY code, even untested,
> that I could try out. I'm pretty much in the dark when it comes to their
> hardware documentation.

If we can agree on the reading-registers approach I suggested (with
the multi-bit return values corrected), then I can turn that into a
patch, but I think we need to come to agreement on that first.

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

Powered by blists - more mailing lists