Message-ID: <20210214131816.GU1463@shell.armlinux.org.uk>
Date:   Sun, 14 Feb 2021 13:18:16 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Antoine Tenart <atenart@...nel.org>,
        Quentin Schulz <quentin.schulz@...tlin.com>,
        Michael Walle <michael@...le.cc>, netdev@...r.kernel.org,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next 1/2] net: phylink: explicitly configure in-band
 autoneg for PHYs that support it

On Sun, Feb 14, 2021 at 01:10:14PM +0200, Vladimir Oltean wrote:
> On Sun, Feb 14, 2021 at 10:35:29AM +0000, Russell King - ARM Linux admin wrote:
> > As mentioned in this thread, we have at least one PHY which is unable
> > to provide the inband signalling in any mode (BCM84881). Currently,
> > phylink detects this PHY on a SFP (in phylink_phy_no_inband()) and
> > adjusts not to use inband mode. This would need to be addressed if we
> > are creating an alterative way to discover whether the PHY supports
> > inband mode or not.
> 
> So I haven't studied the SFP code path too deeply, but I think part of
> the issue is the order in which things are done. It's almost as if there
> should be a validation phase for PHY inband abilities too.
> 
> phylink_sfp_connect_phy
> -> phylink_sfp_config:
>    -> first this checks if phylink_phy_no_inband
>    -> then this changes pl->link_config.interface and pl->cur_link_an_mode
> -> phylink_bringup_phy:
>    -> this is where I'm adding the new phy_config_inband_aneg function
> 
> If we were to use only my phy_config_inband_aneg function, it would need
> to be moved upwards in the code path, to be precise where phylink_phy_no_inband
> currently is. Then we'd have to try MLO_AN_INBAND first, with a fallback
> to MLO_AN_PHY if that fails. I think this would unnecessarily complicate
> the code.

There's another possibility - we could postpone the actual configuration
on the MAC side to always be in phylink_sfp_module_start(), essentially
moving the phylink_mac_initial_config() call to that point while
preserving the current locations where we compute the initial interface
mode. We can then defer the AN mode selection.

> Alternatively, I could create a second PHY driver method, simply for
> validation of supported inband modes. The validation can be done in
> place of the current phylink_phy_noinband(), and I can still have the
> phy_config_inband_aneg() where I put it now, since we shouldn't have any
> surprises w.r.t. supported operating mode, and there should be no reason
> to repeat the attempt as there would be with a single PHY driver method.
> Thoughts?

That also sounds like it should work, and would probably be more
flexible.

> > Also, there needs to be consideration of PHYs that dynamically change
> > their interface type, and whether they support inband signalling.
> > For example, a PHY may support a mode where it dynamically selects
> > between 10GBASE-R, 5GBASE-R, 2500BASE-X and SGMII, where the SGMII
> > mode may have inband signalling enabled or disabled. This is not a
> > theoretical case; we have a PHY like that supported in the kernel and
> > boards use it. What would the semantics of your new call be for a PHY
> > that performs this?
> > 
> > Should we also have a phydev->inband tristate, taking values "unknown,
> > enabled, disabled" which the PHY driver is required to update in their
> > read_status callback if they dynamically change their interface type?
> > (Although then phylink will need to figure out how to deal with that.)
> 
> I don't have such PHY to test with, but I think the easiest way would be
> to just call the validation method again, after we change the
> phydev->interface value.

I'm not sure I follow. It is the PHY driver that is in charge of
changing phydev->interface dynamically, since that is precisely what
the hardware is doing.

88x3310 hardware, when connected using a single lane serdes without
rate adaption will switch the MAC side interface between 10GBASE-R,
5GBASE-R, 2500BASE-X and SGMII depending on the media side speed (10G,
5G, 2.5G, 1G, 100M, 10M.)

BCM84881 does the same, switching dynamically between 10GBASE-R,
2500BASE-X and SGMII (never with inband signalling - it's not
supported in hardware) for 10G, 2.5G, 1G and 100M speeds. 10M is not
supported.

With both these PHYs, you don't get to say "I want you to operate in
_this_ single interface mode", with the exception of 88x3310 with rate
adaption, they aren't designed for that. As soon as there is link on
the media side, the PHYs automatically reconfigure their MAC side with
no intervention from MDIO.

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