Date: Fri, 18 Aug 2023 14:10:21 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Linus Walleij <linus.walleij@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

On Fri, Aug 18, 2023 at 02:40:55PM +0300, Vladimir Oltean wrote:
> On Fri, Aug 18, 2023 at 12:11:15PM +0100, Russell King (Oracle) wrote:
> > On Thu, Aug 17, 2023 at 10:17:54PM +0300, Vladimir Oltean wrote:
> > > On Thu, Aug 17, 2023 at 08:52:12PM +0200, Andrew Lunn wrote:
> > > > > Andrew, I'd argue that the MAC-PHY relationship between the DSA master
> > > > > and the CPU port is equally clear as between 2 arbitrary cascade ports.
> > > > > Which is: not clear at all. The RGMII standard does not talk about the
> > > > > existence of a MAC role and a PHY role, to my knowledge.
> > > > 
> > > > The standard does talk about an optional in band status, placed onto
> > > > the RXD pins during the inter packet gap. For that to work, there
> > > > needs to be some notion of MAC and PHY side.
> > > 
> > > Well, opening the RGMII standard, it was quite stupid of myself to say
> > > that. It says that the purpose of RGMII is to interconnect the MAC and
> > > the PHY right from the first phrase.
> > > 
> > > You're also completely right in pointing out that the optional in-band
> > > status is provided by the PHY on RXD[3:0].
> > > 
> > > Actually, MAC-to-MAC is not explicitly supported anywhere in the standard
> > > (RGMII 2.0, 4/1/2002) that I can find. It simply seems to be a case of:
> > > "whatever the PHY is required by the standard to do is specified in such
> > > a way that when another MAC is put in its place (with RX and TX signals
> > > inverted), the protocol still makes sense".
> > > 
> > > But, with that stretching of the standard considered, I'm still not
> > > necessarily seeing which side is the MAC and which side is the PHY in a
> > > MAC-to-MAC scenario.
> > > 
> > > With a bit of imagination, I could actually see 2 back-to-back MAC IPs
> > > which both have logic to provide the optional in-band status (with
> > > hardcoded information) to the link partner's RXD[3:0]. No theory seems
> > > to be broken by this (though I can't point to any real implementation).
> > > 
> > > So a MAC role would be the side that expects the in-band status to be
> > > present on its RXD[3:0], and a PHY role would be the side that provides
> > > it, and being in the MAC role does not preclude being in the PHY role?
> > 
> > ... trying to find an appropriate place in this thread to put this
> > as I would like to post the realtek patch adding the phylink_get_caps
> > method.
> > 
> > So, given the discussion so far, we have two patches to consider.
> > 
> > One patch from Linus which changes one of the users of the Realtek
> > DSA driver to use "rgmii-id" instead of "rgmii". Do we still think
> > that this a correct change given that we seem to be agreeing that
> > the only thing that matters on the DSA end of this is that it's
> > "rgmii" and the delays for the DSA end should be specified using
>   ~~~~~~~
>   I'd say not necessarily specifically "rgmii", but rather "rgmii*"
> 
> > the [tr]x-internal-delay-ps properties?

For a CPU link though, where there is no "phy", does specifying anything
other than "rgmii" make sense? (since there's no "remote" side that
would take a blind bit of notice of it.)

> As long as it is understood that changing "rgmii" to "rgmii-id" is
> expected to be inconsequential (placebo) for a fixed-link, I don't have
> an objection (in principle) to that patch.
> 
> Though, to have more confidence in the validity of the change, I'd need
> the phy-mode of the &gmac0 node from arch/arm/boot/dts/gemini/gemini-dlink-dir-685.dts,
> and I'm not seeing it.

Gemini DT source is hard to follow, because despite there being the
labels, they aren't always used. gmac0 points at
/soc/ethernet@...0000/ethernet-port@0 and finding that in
arch/arm/boot/dts/gemini/gemini-dlink-dir-685.dts gives us:

gmac0 specifies a fixed link with:
	phy-mode = "rgmii";

It would be helpful if the labels were used consistently!

> Looking at its driver (drivers/net/ethernet/cortina/gemini.c), I don't
> see any handling of RGMII delays, and it accepts any RGMII phy-mode.

As discussed previously in this thread with Linus, Gemini apparently
has the capability to add the delays in via the pinctrl layer. In
this case, in the pinctrl-gmii node, everything has the same skew delay
so the Gemini end of the link looks like it has no delays.

On the Realtek DSA end, we don't know how it's strapped. RTL8366 (*not*
RB) has the ability to pinstrap the required delays, and read the
pinstrapping status out of a register. That register address is used
for an entirely different purpose by RTL8366RB, so we can't easily
find out that way.

> So, if neither the Ethernet controller nor the RTL8366RB switch are
> adding RGMII delays, it becomes plausible that there are skews added
> through PCB traces. In that case, the current phy-mode = "rgmii" would
> be the correct choice, and changing that would be invalid. Some more
> documentary work would be needed.

It could also be that RTL8366RB is pinstrapped to add the delays. If
RTL8366RB is pinstrapped for delays on both rx and tx, then that would
be equivalent to a DT description of e.g.:

	phy-mode = "rgmii";
	rx-internal-delay-ps = <2000>;
	tx-internal-delay-ps = <2000>;

on the DSA end, and:

	phy-mode = "rgmii-id";

on the gmac0 end.

I believe the DSA end in this case should be "rgmii" because there are
no delays being added at the CPU side of the connection, and in _this_
case in gemini-dlink-dir-685.dts, it would be incorrect to change the
DSA side from "rgmii".

Whether the delays are added by the switch or by trace routing is
something we can't answer, thus we can't say whether the CPU end should
use "rgmii-id" or "rgmii", nor whether the delay-ps properties should
be added on the DSA side to make a complete "modern" description.

> In any case, I wouldn't rush a change to arch/arm/boot/dts/gemini/gemini-dlink-dir-685.dts,
> and I'm not seeing any dependency between that and your phylink_get_caps
> conversion for the rtl8366rb.

If the DSA side is changed from "rgmii" to "rgmii-id" then only doing:

                __set_bit(PHY_INTERFACE_MODE_RGMII, interfaces);

means that when phylink_create() is called with
PHY_INTERFACE_MODE_RGMII_ID due to Linus' change, the phylink_validate()
in phylink_parse_fixedlink() will continue to fail (as it is today) and
if that bug ever gets fixed, then rtl8366rb.c will break.

This negates the whole point of adding phylink_get_caps() for realtek.

> > The second patch is my patch adding a phylink_get_caps method for
> > Realtek drivers - should that allow all "rgmii" interface types,
> > or do we want to just allow "rgmii" to encourage the use of the
> > [tr]x-internal-delay-ps properties?
> 
> Same opinion as above. As long as it's understood that the RTL8366RB
> MAC, like any other MAC, shouldn't be acting upon the phy-mode when
> adding delays, let's just accept all 4 variants, with future support to
> be added for [rt]x-internal-delay-ps if there turn out to be
> configurable MAC-side delays present.

Yes, I think you're right, because we could have the situation where
the CPU side is adding the delays, and the DSA side is not, which
should be described in DT as:

	phy-mode = "rgmii-id";

on the DSA side, and e.g.:

	phy-mode = "rgmii";
	rx-internal-delay-ps = <2000>;
	tx-internal-delay-ps = <2000>;

on the CPU side. Yes?

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