Re: [PATCH net-next 3/6] net: dsa: add support for retrieving the interface mode

On Thu, Jul 21, 2022 at 07:21:45PM +0200, Marek Behún wrote:

On Thu, 21 Jul 2022 18:15:33 +0300
Vladimir Oltean [off-list ref] wrote:

quoted

On Thu, Jul 21, 2022 at 03:54:15PM +0100, Russell King (Oracle) wrote:

quoted

Yes, which is why I said on July 7th:

"So I also don't see a problem - sja1105 rejects DTs that fail to
describe a port using at least one of a phy-handle, a fixed-link, or
a managed in-band link, and I don't think it needs to do further
validation, certainly not for the phy describing properties that
the kernel has chosen to deprecate for new implementations."

I had assumed you knew of_phy_is_fixed_link() returns true in this
case. Do you now see that sja1105's validation is close enough
(except for the legacy phy phandle properties which we don't care
about),  

This is why your comment struck me as odd for mentioning managed in-band.

quoted

and thus do we finally have agreement on this point?  

Yes we do.

quoted

quoted

On the other hand I found arm64/boot/dts/marvell/cn9130-crb.dtsi, where
the switch, a "marvell,mv88e6190"-compatible (can't determine going just
by that what it actually is) has this:

			port@a {
				reg = <10>;
				label = "cpu";
				ethernet = <&cp0_eth0>;
			};  

Port 10 on 88E6393X supports 10GBASE-R, and maybe one day someone will
get around to implementing USXGMII. This description relies upon this
defaulting behaviour - as Andrew has described, this has been entirely
normal behaviour with mv88e6xxx.
  

quoted

To illustrate how odd the situation is, I am able to follow the phandle
to the CPU port and find a comment that it's a 88E6393X, and that the
CPU port uses managed = "in-band-status":

&cp0_eth0 {
	/* This port is connected to 88E6393X switch */
	status = "okay";
	phy-mode = "10gbase-r";
	managed = "in-band-status";
	phys = <&cp0_comphy4 0>;
};  

10GBASE-R has no in-band signalling per-se, so the only effect this has
on the phylink instance on the CPU side is to read the status from the
PCS as it does for any other in-band mode. In the case of 10GBASE-R, the
only retrievable parameter is the link up/down status. This is no
different from a 10GBASE-R based fibre link in that regard.  

Is there any formal definition for what managed = "in-band-status"
actually means? Is it context-specific depending on phy-mode?
In the case of SGMII, would it also mean that clause 37 exchange would
also take place (and its absence would mean it wouldn't), or does it
mean just that, that the driver should read the status from the PCS?

quoted

A fixed link on the other hand would not read status from the PCS but
would assume that the link is always up.
  

quoted

Open question: is it sane to even do what we're trying here, to create a
fixed-link for port@a (which makes the phylink instance use MLO_AN_FIXED)
when &cp0_eth0 uses MLO_AN_INBAND? My simple mind thinks that if all
involved drivers were to behave correctly and not have bugs that cancel
out other bugs, the above device tree shouldn't work. The host port
would expect a clause 37 base page exchange to take place, the switch
wouldn't send any in-band information, and the SERDES lane would never
transition to data mode. To fix the above, we'd really need to chase the
"ethernet" phandle and attempt to mimic what the DSA master did. This is
indeed logic that never existed before, and I don't particularly feel
like adding it. How far do we want to go? It seems like never-ending
insanity the more I look at it.  

10GBASE-R doesn't support clause 37 AN. 10GBASE-KR does support
inband AN, but it's a different clause and different format.  

I thought it wouldn't, but then I was led to believe, after seeing it
here, that just the hardware I'm working with doesn't. How about
2500base-x in Marvell, is there any base page exchange, or is this still
only about retrieving link status from the PCS?

Marvell documentation says that 2500base-x does not implement inband
AN.

But when it was first implemented, for some reason it was thought that
2500base-x is just 1000base-x at 2.5x speed, and 1000base-x does
support inband AN. Also it worked during tests for both switches and
SOC NICs, so it was enabled.

It comes from Marvell NETA and PP2 documentation, which clearly states
that when a port is operating in 1000base-X mode, autonegotiation must
be enabled. It then implements 2500base-X by up-clocking the Serdes by
2.5x.

Therefore, to get 2500base-X using 1000base-X mode on these devices, it
follows that autonegotiation must be enabled. Since phylink's origins
are for these devices, and 2500base-X was not standardised at the time,
and there was no documentation online to describe what 2500base-X
actually was, a decision had to be made, and the logical thing to do
at that point was to support AN, especially as one can use it to
negotiate pause modes.

Note that NETA and PP2 have never supported half-duplex on 1G or 2.5G
speeds, so the clause 37 negotiation has only ever dealt with pause
modes, and again, it seemed logical and sensible at the time to allow
pause modes to still be negotiated at 2500base-X speed. Especially as
one can use FibreChannel SFPs to link two machines together using
2500base-X in the same way that you can link them together using
1000base-X.

Had manufacturers been more open with their documentation, and had they
used a common term, maybe this could have been different, but in an
information vacuum, decisions have to be made, even if they turn out to
be wrong later on - but we have to live with those consequences.

As I've stated before, I know I'm not alone in this - there are SFPs
that support 2500base-X (particularly GPON SFPs) that appear to expect
caluse 37 AN on 2500base-X, since the Armada 3700 uDPU product is
designed to work with them and it was a requirement to have a working
2.5G connection between the NETA interfaces and the GPON SFPs. And as
I say, NETA wants AN enabled.

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