Message-ID: <20230818142105.kpbdqmm3lckeja5z@skbuf>
Date: Fri, 18 Aug 2023 17:21:05 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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:10:21PM +0100, Russell King (Oracle) wrote:
> > > 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.)

I'm not completely sure that I understand the question. I think you've
answered this yourself, at the end.

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

+1

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

So it would appear. The skewing capability is there, but the skews for
RXC/TXC and RXD/TXD cancel out in that particular configuration. In that
case, it's pretty confusing why they're there at all, to be honest.

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

I see.

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

Well, if we can't distinguish between the PCB traces case and the
RTL8366RB pin strapping case, I think it would be best to keep the
strategic ambiguity in the device tree alone.

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

Speech can be so imprecise :)

I can see how "I'm not seeing any dependency" can be interpreted as
you've done, i.e. "there is no dependency in either direction between
the 2 patches", but that isn't what I wanted to transmit.

I just wanted to say that you don't need Linus' patch to make progress
with the conversion, that's all.

But of course Linus' device tree patch would only work if your
kernel-side patch puts all 4 RGMII modes in supported_interfaces. But
maybe that should be done anyway, with or without Linus' device tree
patch.

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

Yes, this is the situation I was thinking of, where the DSA CPU port
would have "rgmii-id" to denote that the remote side has added the
delays.

At least, that would be the intuitive way for me to describe things
according to our definitions from the documentation.

(open question: if those remote delays are added through pinctrl-gmii
rather than through the MAC OF node, would that count towards DSA's
phy-mode, to make it rgmii-id, or not?)

Though I agree that I can't see the exact phy-mode breaking anything
with a fixed link, given this interpretation, even if it's "wrong".

Powered by blists - more mailing lists