Date:   Thu, 21 Jul 2022 15:54:15 +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>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Alvin __ipraga <alsi@...g-olufsen.dk>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        DENG Qingfang <dqfext@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        George McCollister <george.mccollister@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Sean Wang <sean.wang@...iatek.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Marek Behún <kabel@...nel.org>
Subject: Re: [PATCH net-next 3/6] net: dsa: add support for retrieving the
 interface mode

On Thu, Jul 21, 2022 at 04:46:18PM +0300, Vladimir Oltean wrote:
> On Thu, Jul 21, 2022 at 01:44:47AM +0300, Vladimir Oltean wrote:
> > I really wish there was a ready-made helper for validating phylink's
> > OF node; I mentioned this already, it needs to cater for all of
> > fixed-link/phy-handle/managed/sfp.
> 
> While I was going to expand on this point and state that DSA doesn't
> currently instantiate phylink for this OF node:
> 
> 			port@9 {
> 				reg = <0x9>;
> 				label = "cpu";
> 				ethernet = <&eth1>;
> 				phy-mode = "2500base-x";
> 				managed = "in-band-status";
> 			};
> 
> I was proven wrong. Today I learned that of_phy_is_fixed_link() returns
> true if the "managed" property exists and its value differs from "auto".
> So in the above case, of_phy_is_fixed_link() returns true, hmmm.

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), and thus do we finally have agreement on this point?

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

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

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

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

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

Powered by blists - more mailing lists