Date:   Tue, 22 Nov 2022 19:56:25 +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 04:58:45PM +0000, Russell King (Oracle) wrote:
> The intention of the above is to report how the PHY is going to behave
> with the current code when the PHY is in operation, which I believe to
> be the intention of the validate callback. I'm not proposing at the
> moment to add the config() part, although that can be done later.

Again, all I'm saying is that with validate() but no config(), you should
report a single bit, not PHY_AN_INBAND_ON | PHY_AN_INBAND_TIMEOUT.
The code which you posted does report multiple bits, and we won't know
how to make sense out of them. We'd select what is the most convenient
to us, and we'll call phy_config_an_inband() with that, but it'll return
-EOPNOTSUPP, which will be perfectly reasonable to us. Except that the
PHY may actually not currently operate in the mode it reported as a
supported capability. So things are broken.

> As I stated, with the 88e1111, if we are asked to operate in 1000base-X
> mode, when we configure the PHY we will allow bypass mode and I believe
> in-band will be enabled, because this is what config_init() does today
> when operating in 1000base-X mode. If we add support for your config()
> method, then we will need a way to prevent a later config_init()
> changing that configuration.

If config_init() is going to be kept being called only from
phy_attach_direct() -> phy_init_hw(), then, at least with phylink, we
only call phy_config_an_inband() from phylink_bringup_phy(), so after
the phy_attach_direct() call. So we overwrite what the driver does by
default.

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.

I'll see if I can fix that and delay phy_validate_an_inband() a bit,
maybe up until before right before phy_config_an_inband().

> 
> For SGMII, things are a lot more complicated, the result depends on how
> the PHY has been setup by firmware or possibly reset pin strapping, so
> we need to read registers to work out how it's going to behave. So,
> unless we do that, we just can't report anything with certainty. We
> probably ought to be reading a register to check that in-band is indeed
> enabled.
> 
> Note that a soft-reset doesn't change any of this - it won't enable
> in-band if it was disabled, and it won't disable it if it was previously
> set to be enabled.

Similar to VSC8514 and the U-Boot preconfiguration issue. Soft reset,
but the setting sticks.

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

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.

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"

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

Powered by blists - more mailing lists