Date: Fri, 3 May 2024 18:35:53 +0200
From: Stefan Eichenberger <eichest@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	lxu@...linear.com, hkallweit1@...il.com, michael@...le.cc,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property
 to disable SGMII autoneg

On Thu, May 02, 2024 at 03:14:13PM +0100, Russell King (Oracle) wrote:
> On Thu, May 02, 2024 at 03:44:24PM +0200, Stefan Eichenberger wrote:
> > Hi Russell,
> > 
> > Sorry for the late reply but I wanted to give you some update after
> > testing with the latest version of your patches on net-queue.
> 
> I've also been randomly distracted, and I've been meaning to ping you
> to test some of the updates.
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue
> 
> The current set begins with:
> 
> "net: sfp-bus: constify link_modes to sfp_select_interface()" which is
> now in net-next, then the patches between and including:
> 
> "net: phylink: validate sfp_select_interface() returned interface" to
> "net: phylink: clean up phylink_resolve()"
> 
> That should get enough together for the PCS "neg" mode to be consistent
> with what the MAC driver sees.
> 
> The remaining bits that I still need to sort out is the contents of
> phylink_pcs_neg_mode() for the 802.3z mode with PHY, and also working
> out some way of handling the SGMII case where the PHY and PCS disagree
> (one only supporting inband the other not supporting inband.)
> 
> I'm not sure when I'll be able to get to that - things are getting
> fairly chaotic again, meaning I have again less time to spend on
> mainline... and I'd like to take some vacation time very soon (I really
> need some time off!)

No problem, I'm also quite busy at the moment and I have the workaround
to test the hardware, so it is nothing urgent for me.

> > I think I see the problem you are describing.
> > 
> > When the driver starts it will negotiate MLO_AN_PHY based on the
> > capabilities of the PHY and of the PCS. However when I switch to 1GBit/s
> > it should switch to MLO_AN_INBAND but this does not work. Here the
> > output of phylink:
> 
> I'm designing this to work the other way - inband being able to fall
> back to PHY (out of band) mode rather than PHY mode being able to fall
> forwards to inband mode.

I tested again with 89e0a87ef79db9f3ce879e9d977429ba89ca8229 and I think
in my setup the problem is that it doesn't fall back to PHY mode but
takes it as default mode. Here what happens when I have the mxl-gpy PHY
connected to a 1000 GBit/s port:
[    9.331179] mvpp2 f2000000.ethernet eth1: Using firmware node mac address 00:51:82:11:22:02
[   14.674836] mvpp2 f2000000.ethernet eth1: PHY f212a600.mdio-mii:11 doesn't supply possible interfaces
[   14.674853] mvpp2 f2000000.ethernet eth1:  interface 2 (mii) rate match none supports 0-3,6,13-14
[   14.674864] mvpp2 f2000000.ethernet eth1:  interface 4 (sgmii) rate match none supports 0-3,5-6,13-14
[   14.674871] mvpp2 f2000000.ethernet eth1:  interface 9 (rgmii) rate match none supports 0-3,5-6,13-14
[   14.674877] mvpp2 f2000000.ethernet eth1:  interface 10 (rgmii-id) rate match none supports 0-3,5-6,13-14
[   14.674883] mvpp2 f2000000.ethernet eth1:  interface 11 (rgmii-rxid) rate match none supports 0-3,5-6,13-14
[   14.674889] mvpp2 f2000000.ethernet eth1:  interface 12 (rgmii-txid) rate match none supports 0-3,5-6,13-14
[   14.674895] mvpp2 f2000000.ethernet eth1:  interface 22 (1000base-x) rate match none supports 5-6,13-14
[   14.674900] mvpp2 f2000000.ethernet eth1:  interface 23 (2500base-x) rate match none supports 6,13-14,47
[   14.674907] mvpp2 f2000000.ethernet eth1: PHY [f212a600.mdio-mii:11] driver [Maxlinear Ethernet GPY215C] (irq=POLL)
[   14.685444] mvpp2 f2000000.ethernet eth1: phy: 2500base-x setting supported 00,00000000,00008000,0000606f advertising 00,00000000,00008000,0000606f
[   14.686635] mvpp2 f2000000.ethernet eth1: configuring for phy/2500base-x link mode
[   14.694263] mvpp2 f2000000.ethernet eth1: major config, requested phy/2500base-x
[   14.700402] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/2500base-x
[   14.700408] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/2500base-x/none adv=00,00000000,00000000,00000000 pause=00
[   14.702726] mvpp2 f2000000.ethernet eth1: phy link down 2500base-x/1Gbps/Full/none/off
[   17.768349] mvpp2 f2000000.ethernet eth1: phy link up sgmii/1Gbps/Full/none/rx/tx
[   17.768370] mvpp2 f2000000.ethernet eth1: major config, requested phy/sgmii
[   17.774602] mvpp2 f2000000.ethernet eth1: firmware wants phy mode, but PHY requires inband
[   17.782976] mvpp2 f2000000.ethernet eth1: major config, active phy/outband/sgmii
[   17.784200] mvpp2 f2000000.ethernet eth1: phylink_mac_config: mode=phy/sgmii/none adv=00,00000000,00000000,00000000 pause=03
[   17.784278] mvpp2 f2000000.ethernet eth1: pcs link up
[   17.784485] mvpp2 f2000000.ethernet eth1: can LPI, EEE enabled, active
[   17.784504] mvpp2 f2000000.ethernet eth1: enabling tx_lpi, timer 250us
[   17.784543] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

It agrees on PHY mode when starting, which then sets MLO_AN_PHY.
However, when it sees a 1 GBit/s link it will just print the error
message "firmware wants phy mode, but PHY requires inband". After "Link
is Up" is shown the link will not work becuse of the missmatch between
the PCS and the PHY.
 
> > The problem is that the PCS continues to be in phy mode but the PHY
> > driver currently only supports LINK_INBAND_ENABLE and SGMII for 1GBit/s.
> > 
> > What I'm wondering is if it wouldn't make sense to adapt the phy driver
> > to support MLO_AN_PHY in SGMII/1000BASE-X mode.
> 
> PHYs have no idea about MLO_AN_xxx at all, neither should they. This
> is not part of phylib's API, and I don't think PHYs should ever know
> about MLO_AN_xxx stuff (which is something purely between phylink and
> the MAC driver.) The structure here is:
> 
>       MAC            PCS                   PHY
>        ^              ^                    ^ ^-----.
>        v              v                    v       |
>    MAC driver <-> PCS driver <-------> PHY driver  |
>        ^           ^                    ^          |
>        |           |                    |          |
>   MLO_AN_xxx  PHYLINK_PCS_NEG_xxx       |          |
>        `           '                    |          |
>         \         /                     v          |
>           phylink <----------------> phylib <------'
> 
> MLO_AN_xxx is far beyond the PHY, and more or less defines the overall
> "system" operating mode. PHYLINK_PCS_NEG_xxx defines the properties
> used for the PCS link to the next device towards the media. This is
> more of relevance to what the PHY should be using on its MAC-facing
> interface.

Okay this makes sense. I more thought about if the PHY e.g. would
signalise it can do inband/outband and the PCS can only do outband.
Basically exactly what you write in the last comment.

> > Currently the mxl-gpy phy driver can only support:
> > 10/100/1000 MBit/s: SGMII with MLO_AN_INBAND
> > 2500MBit/s          2500Base-X with MLO_AN_PHY
> > 
> > However, the PHY would also support the following mode:
> > 10/100/1000 MBit/s: SGMII with MLO_AN_PHY
> 
> The problem with this is some PHYs will not pass data _unless_ their
> SGMII control frame to the PCS has been acknowledged by the PCS - in
> other words, inband has to be used. However, that can be coped with,
> because such a PHY driver should be saying that it only supports
> LINK_INBAND_ENABLE in SGMII mode... and firmware must tell phylink
> that it wants to use inband mode (as that's exactly what firmware
> must do today in this situation.)
> 
> > I just don't know how the PHY driver could know about what it should
> > configure.
> 
> Currently, I haven't added an interface to cope with the case where
> a PHY says LINK_INBAND_ENABLE | LINK_INBAND_DISABLE to allow it to be
> configured in that case... that's something that will eventually be
> needed.

Thanks a lot for the work and help so far.

Regards,
Stefan

Powered by blists - more mailing lists