Received-SPF: pass (google.com: domain of linux-kernel+bounces-166547-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 2 May 2024 15:14:13 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Stefan Eichenberger <eichest@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>, davem@davemloft.net, edumazet@google.com,
	kuba@kernel.org, pabeni@redhat.com, robh@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	lxu@maxlinear.com, hkallweit1@gmail.com, michael@walle.cc,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] net: phy: mxl-gpy: add new device tree property
 to disable SGMII autoneg
Message-ID: <ZjOftdnoToSSsVJ1@shell.armlinux.org.uk>
References: <Zikd+GxuwMRC+5Ae@shell.armlinux.org.uk>
 <Zikrv5UOWvSGjgcv@eichest-laptop>
 <ZilLz8f6vQQCg4NB@shell.armlinux.org.uk>
 <Zio9g9+wsFX39Vkx@eichest-laptop>
 <ZippHJrnvzXsTiK4@shell.armlinux.org.uk>
 <Zip8Hd/ozP3R8ASS@eichest-laptop>
 <ZiqFOko7zFjfTdz4@shell.armlinux.org.uk>
 <ZiqUB0lwgw7vIozG@eichest-laptop>
 <Ziq5+gRXGmqt9bXM@shell.armlinux.org.uk>
 <ZjOYuP5ypnH8GJWd@eichest-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjOYuP5ypnH8GJWd@eichest-laptop>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

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

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

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

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

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