Message-ID: <ZIyD31CaVxjSDtz3@shell.armlinux.org.uk>
Date: Fri, 16 Jun 2023 16:46:39 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Alexander Couzens <lynxis@...0.eu>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Cc@....codeaurora.org:Claudiu Beznea <claudiu.beznea@...rochip.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Daniel Machon <daniel.machon@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	DENG Qingfang <dqfext@...il.com>, Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	Landen Chao <Landen.Chao@...iatek.com>,
	Lars Povlsen <lars.povlsen@...rochip.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Madalin Bucur <madalin.bucur@....com>,
	Marcin Wojtas <mw@...ihalf.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Michal Simek <michal.simek@....com>, netdev@...r.kernel.org,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>,
	Sean Anderson <sean.anderson@...o.com>,
	Sean Wang <sean.wang@...iatek.com>,
	Steen Hegelund <Steen.Hegelund@...rochip.com>,
	Taras Chornyi <taras.chornyi@...ision.eu>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next 0/15] Add and use helper for PCS negotiation
 modes

On Fri, Jun 16, 2023 at 06:00:55PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 01:05:52PM +0100, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Earlier this month, I proposed a helper for deciding whether a PCS
> > should use inband negotiation modes or not. There was some discussion
> > around this topic, and I believe there was no disagreement about
> > providing the helper.
> > 
> > The initial discussion can be found at:
> > 
> > https://lore.kernel.org/r/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk
> > 
> > Subsequently, I posted a RFC series back in May:
> > 
> > https://lore.kernel.org/r/ZGzhvePzPjJ0v2En@shell.armlinux.org.uk
> > 
> > that added a helper, phylink_pcs_neg_mode() which PCS drivers could use
> > to parse the state, and updated a bunch of drivers to use it. I got
> > a couple of bits of feedback to it, including some ACKs.
> > 
> > However, I've decided to take this slightly further and change the
> > "mode" parameter to both the pcs_config() and pcs_link_up() methods
> > when a PCS driver opts in to this (by setting "neg_mode" in the
> > phylink_pcs structure.) If this is not set, we default to the old
> > behaviour. That said, this series converts all the PCS implementations
> > I can find currently in net-next.
> > 
> > Doing this has the added benefit that the negotiation mode parameter
> > is also available to the pcs_link_up() function, which can now know
> > whether inband negotiation was in fact enabled or not at pcs_config()
> > time.
> > 
> > It has been posted as RFC at:
> > 
> > https://lore.kernel.org/r/ZIh/CLQ3z89g0Ua0@shell.armlinux.org.uk
> > 
> > and received one reply, thanks Elad, which is a similar amount of
> > interest to previous postings. Let's post it as non-RFC and see
> > whether we get more reaction.
> 
> Sorry, I was in the process of reviewing the RFC, but I'm not sure I
> know what to ask to make sure that I understand the motivation :-/
> Here's a question that might or might not result in a code change.
> 
> In the single-patch RFC at:
> https://lore.kernel.org/all/ZGIkGmyL8yL1q1zp@shell.armlinux.org.uk/
> you bring sparx5 and lan966x as a motivation for introducing
> PHYLINK_PCS_NEG_OUTBAND as separate from PHYLINK_PCS_NEG_INBAND_DISABLED,
> with both meaning that in-band autoneg isn't used, but in the former
> case it's not enabled at all, while in the latter it's disabled through
> ethtool (if I get that right?).

Correct.

conf.inband is set true if phylink_autoneg_inband(mode) is true, which
equates to MLO_AN_INBAND.

conf.autoneg is set true if the ethtool Autoneg flag in the advertising
mask is set.

That goes through some incomprehensible logic in
sparx5_port_pcs_low_set() which I'm not going to even try to unpick
because it looks buggy to me, except that conf.autoneg is only looked
at if conf.inband were true at some point in the past.

So, what I care about here is keeping the behaviour pretty much the
same, especially as far as conf.inband.

With the new neg_mode:

conf.inband is set when we have one of the NEG_INBAND states. These are
set when in 1000BASE-X, 2500BASE-X or one of the SGMII family and
phylink_autoneg_inband(mode) is true. So, 100% identical when one
considers that the driver only supports SGMII, QSGMII, 1000BASE-X and
2500BASE-X for this path.

conf.autoneg will only be set when we have NEG_INBAND_ENABLED state,
and that is only set when in SGMII mode (irrespective of Autoneg) or
in *BASE-X, we're in in-band mode (so conf.inband is set) and the
advertising mask has the Autoneg bit set. As this is only looked at
if conf.inband was set the _last_ time around (which seems like a
bug in the driver...) and we're in 1000BASE-X mode, this is identical
logic where it matters.

So, conf.inband is 100% identical logic, and conf.autoneg is very
similar and for how it's actually used, completely identical.

> ... trying to find
> exactly what the PCS1G_MODE_CFG.SGMII_MODE_ENA field does (which is
> controlled in sparx5 and lan966x based on the presence or absence of the
> managed = "in-band-status" property).
> 
> Do you know for sure what this bit does and whether it makes sense for
> drivers to even distinguish between OUTBAND and INBAND_DISABLED in the
> way that this series is proposing?

I have no idea, and I didn't bother investigating - I don't want to go
around trying to disect drivers to figure out whether they're buggy or
not.

However, what I would say is that this is not where these modes came
from. They came from me asking myself the question "what would be the
logical set of information to give a PCS driver about the negotiation
state of the link?" and that's what I came up with _without_ reference
to this driver. The states are all documented in the first patch and
what they mean.

So, no, the Microchip driver code is not the reason why these
definitions were chosen. They were chosen because it's the logical
set that gives PCS drivers what they need to know.

Start from inband + autoneg. Then inband + !autoneg. Then inband
possible but not being used. Then "there's no inband possible for this
mode". That's the four states.

I think having this level of detail is important if we want to think
about those pesky inband-AN bypass modes, which make sense for only
really the PHYLINK_PCS_NEG_INBAND_DISABLED state and not OUTBAND nor
NONE state. Bypass mode doesn't make sense for e.g. SGMII because
one needs to know the speed for the link to come up, and if you're
getting that through an out-of-band mechanism, you're into forcing
the configuration at the PCS end.

Makes sense?

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

Powered by blists - more mailing lists