Received-SPF: pass (google.com: domain of linux-kernel+bounces-158976-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 25 Apr 2024 19:33:59 +0200
From: Stefan Eichenberger <eichest@gmail.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
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: <ZiqUB0lwgw7vIozG@eichest-laptop>
References: <Zh6/oVHUvnOVtHaC@shell.armlinux.org.uk>
 <Zh94yqo2EHRq8eEq@eichest-laptop>
 <ZiE156+BPpx/ciL6@shell.armlinux.org.uk>
 <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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiqFOko7zFjfTdz4@shell.armlinux.org.uk>

On Thu, Apr 25, 2024 at 05:30:50PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 25, 2024 at 05:51:57PM +0200, Stefan Eichenberger wrote:
> > On Thu, Apr 25, 2024 at 03:30:52PM +0100, Russell King (Oracle) wrote:
> > > On Thu, Apr 25, 2024 at 01:24:51PM +0200, Stefan Eichenberger wrote:
> > > > I think it should also work for mvneta, at least
> > > > the code looks almost the same. I get the following for the Port
> > > > Auto-Negotiation Configuration Register:
> > > > 
> > > > For 1Gbit/s it switches to SGMII and enables inband AN:
> > > > Memory mapped at address 0xffffa0112000.
> > > > Value at address 0xF2132E0C (0xffffa0112e0c): 0xB0C6
> > > 
> > > So here the link is forced up which is wrong for inband, because then
> > > we have no way to detect the link going down.
> > 
> > Yes I also saw this and didn't understand it. When I clear the force bit
> > it will be set back to 1 again when AN is enabled. I thought this might
> > be a bug of the controller.
> 
> No it isn't, the hardware never changes the value in this register.
> The difference will be because of what I explained previously.
> 
> Because the mvneta and mvpp2 hardware is "weird" in that these two
> registers provide both PCS and MAC functions, we have to deal with
> them in a way that is split between the phylink PCS and MAC
> operations.
> 
> This code was written at a time when all we had was MLO_AN_* and
> none of the PHYLINK_PCS_NEG_* stuff. PCSes got passed the MLO_AN_*
> constants which were the same as what was passed via the MAC
> operations. This made the implementation entirely consistent.
> 
> However, that lead PCS implementers to go off and do their own
> things, so we ended up with a range of different PCS behaviours
> depending on the implementation (because the way people interpreted
> MLO_AN_INBAND, interface mode, and the Autoneg bit in the advertising
> mask was all different.
> 
> So to bring some consistency, I changed the PCS interface to what we
> have now, in the belief that it would later allow us to solve the
> 2500base-X problem.
> 
> However, I'd forgotten about the mvneta/mvpp2 details, but that was
> fine at the time of this change because everything was still
> consistent - we would only ever use PHYLINK_PCS_NEG_OUTBAND or
> PHYLINK_PCS_NEG_NONE for non-MLO_AN_INBAND modes, and
> PHYLINK_PCS_NEG_INBAND_* for interface modes that support inband
> when using MLO_AN_INBAND.
> 
> Now, when trying to solve the 2500base-X problem which needs us to
> use PHYLINK_PCS_NEG_OUTBAND in some situations, this means we can
> end up with MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND, and this is
> what is causing me problems (the link isn't forced up.)
> 
> Conversely, I suspect you have the situation where you have MLO_AN_PHY
> or MLO_AN_FIXED being passed to the mac_config() and mac_link_up()
> operations, but the PCS is being configured for a different mode.
> 
> I am wondering whether we should at the very least move the
> configuration of the control register 0 and 2 to the pcs_config()
> method so at least that's consistent with the PHYLINK_PCS_NEG_*
> value passed to the PCS and thus the values programmed into the
> autoneg config register. However, that still leaves a big hole in
> how we handle the link forcing - which is necessary if inband AN
> is disabled (in other words, if we need to read the link status
> from the PHY as is done in MLO_AN_PHY mode.)
> 

Now I got it, thanks a lot for the explanation. So the issue is that
MLO_AN_INBAND + PHYLINK_PCS_NEG_OUTBAND is happening in my use case and
therefore the link is not forced up because for that MLO_AN_PHY would be
needed. I will also try to think about it.

Thanks,
Stefan