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 Tue, Nov 22, 2022 at 06:28:38PM +0000, Russell King (Oracle) wrote:
> 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.
> 

I'll re-read this tomorrow to make sure I didn't miss something because
of being tired.

I may have mixed up interface modes in the validate() code for MV88E1111
that you posted. I was under the impression that PHY_AN_INBAND_TIMEOUT
always gets reported based on reading a hardware register, the same
hardware register that gets overwritten to MII_M1111_HWCFG_SERIAL_AN_BYPASS
in m88e1111_config_init_1000basex().

But your proposed code is actually a mix between reading the existing
hardware configuration for SGMII, and returning something hardcoded for
1000base-x. For 1000base-x, we will return PHY_AN_INBAND_TIMEOUT, not
because the hardware is currently configured like that, but because it
will be, later. And the timing of the validate() call isn't going to be
a problem, so there isn't a reason to move it.

I'm okay with that, I just didn't understand.

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

Ok, I only answered to part of the question - which is how do we
interpret phy_validate_an_inband()'s result from phylink_sync_an_inband() -
the on-board PHY code path.

If the code path we're talking about is from phylink_sfp_config_phy(),
then the modified code, to account for TIMEOUT, would look like this:

	/* 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);
	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)) {
		mode = MLO_AN_INBAND;
	} else {
		mode = MLO_AN_PHY;
	}

or in words, essentially prefer MLO_AN_INBAND except when the PHY driver
says that it requires in-band disabled.

At least that's for now, because we assume that the PCS always supports
MLO_AN_INBAND. For the purpose of this series, let's assume that's a given.

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

I think we're in agreement, but please let's wait until tomorrow, I need
to take a break for today.