Message-ID: <20221122163012.w5gsoawp22lc3nyl@skbuf>
Date:   Tue, 22 Nov 2022 18:30:12 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

On Tue, Nov 22, 2022 at 11:10:03AM -0500, Sean Anderson wrote:
> On 11/21/22 19:17, Vladimir Oltean wrote:
> > On Mon, Nov 21, 2022 at 05:42:44PM -0500, Sean Anderson wrote:
> >> Are you certain this is the cause of the issue? It's also possible that
> >> there is some errata for the PCS which is causing the issue. I have
> >> gotten no review/feedback from NXP regarding the phylink conversion
> >> (aside from acks for the cleanups).
> > 
> > Erratum which does what out of the ordinary? Your description of the
> > hardware failure seems consistent with the most plausible explanation
> > that doesn't involve any bugs.
> 
> Well, I don't have a setup which doesn't require in-band AN, so I can't
> say one way or the other where the problem lies. To me, the Lynx PCS is
> just as opaque as the phy.

ok.

> > If you enable C37/SGMII AN in the PCS (of the PHY or of the MAC) and AN
> > does not complete (because it's not enabled on the other end), that
> > system side of the link remains down. Which you don't see when you
> > operate in MLO_AN_PHY mode, because phylink only considers the PCS link
> > state in MLO_AN_INBAND mode. So this is why you see the link as up but
> > it doesn't work.
> 
> Actually, I checked the PCS manually in phy mode, and the link was up.
> I expected it to be down, so this was a bit surprising to me.

Well, if autoneg is disabled in the Lynx PCS (which it is in MLO_AN_PHY),
then the link should come up right away, as long as it can lock on some
symbols IIRC. It's a different story for the PHY PCS if autoneg is
enabled there. Still nothing surprising here, really.

> > To confirm whether I'm right or wrong, there's a separate SERDES
> > Interrupt Status Register at page 0xde1 offset 0x12, whose bit 4 is
> > "SERDES link status change" and bit 0 is "SERDES auto-negotiation error".
> > These bits should both be set when you double-read them (regardless of
> > IRQ enable I think) when your link is down with MLO_AN_PHY, but should
> > be cleared with MLO_AN_INBAND.
> 
> This register is always 0s for me...
> 
> >> This is used for SGMII to RGMII bridge mode (figure 4). It doesn't seem
> >> to contain useful information for UTP mode (figure 1).
> > 
> > So it would seem. It was a hasty read last time, sorry. Re-reading, the
> > field says that when it's set, the SGMII code word being transmitted is
> > "selected by the register" SGMII ANAR. And in the SGMII ANLPAR, you can
> > see what the MAC said.
> 
> ... possibly because of this.
> 
> That said, ANLPAR is 0x4001 (all reserved bits) when we use in-band:
> 
> [    8.191146] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=4001
> 
> but all zeros without:
> 
> [   11.263245] RTL8211F Gigabit Ethernet 0x0000000001afc000:04: INER=0000 INSR=0000 ANARSEL=0000 ANAR=0050 ANLPAR=0000

So enabling in-band autoneg in the Lynx PCS does what you'd expect it to do.
I don't know why you don't get a "SERDES auto-negotiation error" bit in
the interrupt status register. Maybe you need to first enable it in the
interrupt enable register?! Who knows. Not sure how far it's worth
diving into this.

> It's all 1s when using RGMII. These bits are reserved, so it's not that
> interesting, but maybe these registers are not as useless as they seem.

Yeah, with RGMII I don't know if the PHY responds to the SERDES registers
over MDIO. All ones may mean the MDIO bus pull-ups.

> > Of course, it doesn't say what happens when the bit for software-driven
> > SGMII autoneg is *not* set, if the process can be at all bypassed.
> > I suppose now that it can't, otherwise the ANLPAR register could also be
> > writable over MDIO, they would have likely reused at least partly the
> > same mechanisms.
> > 
> >> > +	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
> >> 
> >> That said, you have to use the "Indirect access method" to access this
> >> register (per section 8.5). This is something like
> >> 
> >> #define RTL8211F_IAAR				0x1b
> >> #define RTL8211F_IADR				0x1c
> >> 
> >> #define RTL8211F_IAAR_PAGE			GENMASK(15, 4)
> >> #define RTL8211F_IAAR_REG			GENMASK(3, 1)
> >> #define INDIRECT_ADDRESS(page, reg) \
> >> 	(FIELD_PREP(RTL8211F_IAAR_PAGE, page) | \
> >> 	 FIELD_PREP(RTL8211F_IAAR_REG, reg - 16))
> >> 
> >> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_IAAR,
> >> 			      INDIRECT_ADDRESS(0xd08, RTL8211FS_SGMII_ANARSEL));
> >> 	if (ret < 0)
> >> 		return ret;
> >> 
> >> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_IADR);
> >> 	if (ret < 0)
> >> 		return ret;
> >> 
> >> I dumped the rest of the serdes registers using this method, but I
> >> didn't see anything interesting (all defaults).
> > 
> > I'm _really_ not sure where you got the "Indirect access method" via
> > registers 0x1b/0x1c from.
> 
> Huh. Looks like this is a second case of differing datasheets. Mine is
> revision 1.8 dated 2021-04-21. The documentation for indirect access was
> added in revision 1.7 dated 2020-07-08. Although it seems like the
> SERDES registers were also added in this revision, so maybe you just
> missed this section?

I have Rev. 1.2. dated July 2014. Either that, or I'm holding the book
upside down...

> > My datasheet for RTL8211FS doesn't show
> > offsets 0x1b and 0x1c in page 0xa43.
> 
> Neither does mine. These registers are only documented by reference from
> section 8.5. They also aren't named, so the above defines are my own
> coinage.
> 
> > Additionally, I cross-checked with
> > other registers that are accessed by the driver (like the Interrupt
> > Enable Register), and the driver access procedure -
> > phy_write_paged(phydev, 0xa42, RTL821x_INER, val) - seems to be pretty
> > much in line with what my datasheet shows.
> 
> | The SERDES related registers should be read and written through indirect
> | access method. The registers include Page 0xdc0 to Page 0xdcf and Page
> | 0xde0 to Page 0xdf0.
> 
> Each register accessed this way also has
> 
> | Note: This register requires indirect access.
> 
> below the register table.

Ok, possible. And none of the registers accessed by Linux using
phy_read_paged() / phy_write_paged() have the "indirect access" note?
Maybe it was a documentation update as you say, which I don't have.

> >> I think it would be better to just return PHY_AN_INBAND_ON when using
> >> SGMII.
> > 
> > Well, of course hardcoding PHY_AN_INBAND_ON in the driver is on the
> > table, if it isn't possible to alter this setting to the best of our
> > knowledge (or if it's implausible that someone modified it). And this
> > seems more and more like the case.
> 
> I meant something like
> 
> 	if (interface == PHY_INTERFACE_MODE_SGMII)
> 		return PHY_AN_INBAND_ON;
> 
> 	return PHY_AN_INBAND_UNKNOWN;

Absolutely, I understood the first time. So you confirm that such a
change makes your Lynx PCS promote to MLO_AN_INBAND, which makes the
RTL8211FS work, right?

> Although for RGMII, in-band status is (per MIICR2):
> 
> - Enabled by default
> - Disablable
> - Optional
> 
> So maybe we should do (PHY_AN_INBAND_ON | PHY_AN_INBAND_OFF) in that
> case. That said, RGMII in-band is not supported by phylink (yet).

Well, it kinda is. I even said this in one of the commit messages

|    net: phy: at803x: validate in-band autoneg for AT8031/AT8033
|
|    These PHYs also support RGMII, and for that mode, we report that in-band
|    AN is unknown, which means that phylink will not change the mode from
|    the device tree. Since commit d73ffc08824d ("net: phylink: allow
|    RGMII/RTBI in-band status"), RGMII in-band status is a thing, and I
|    don't want to meddle with that unless I have a reason for it.

Although I'd be much more comfortable for now if we could concentrate on
SERDES protocols. I'm not exactly sure what are the hardware state machines
and responsible standards for RGMII in-band status, what will happen on
settings mismatch (I know that NXP MACs fail to link up if we enable the
feature but we attach a switch and not a PHY to RGMII - see c76a97218dcb
("net: enetc: force the RGMII speed and duplex instead of operating in
inband mode") - but not much more). Essentially I don't know enough
right now to even attempt to make any generalizations. Although I suppose
a discussion could be started about it.

Powered by blists - more mailing lists