Date:   Wed, 27 Jul 2022 04:29:29 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC net-next 0/2] net: dsa: bcm_sf2: Utilize PHYLINK for all
 ports

On Tue, Jul 26, 2022 at 09:14:17AM -0700, Florian Fainelli wrote:
> > This begs the natural question, is overriding the link status ever needed?
> 
> It was until we started to unconditionally reset the switch using the
> "external" reset method as opposed to the "internal" reset method
> which turned out not to be functional:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eee87e4377a4b86dc2eea0ade162b0dc33f40576

Ok, I see.

> At any rate (no pun intended), 4908 will want a 2GBit/sec IMP port to
> be set-up and we have no way to do that other than by forcing that
> setting, either through the bcm_sf2_imp_setup() method or via a hack
> to the mac_link_up() callback. This is kind of orthogonal in the sense
> that there is no "official" support for speed 2000 mbits/sec anyway in
> the emulated SW PHY, PHYLINK or anywhere in between but if we want to
> fully transition over to PHYLINK to configure all ports, which is
> absolutely the goal, we will need to find a solution one way or
> another.

So I made some tests with speed = <2000>; in the device tree and in a
way I'm more confused than when I started. I was expecting phylink_validate()
to somehow fail but this isn't at all what happened. Instead everything
seems to work just fine, minus some ergonomic details (some prints).

So in the case of a fixed-link, phylink_validate() is actually called
twice, once directly from phylink_create() and once almost immediately
afterwards from phylink_parse_fixedlink(). Both validations are of the
inquisitive kind rather than the confrontational kind, i.e. their return
value isn't checked, and "pl->supported"/"pl->link_config.advertising"
are initially filled by phylink with all ones, in order for the driver
to reduce this to all link mode bits that are supported.
Minor side note, this second validation done during fixed-link parsing
is redundant IMO, because nothing relevant inside the arguments that we
pass to pl->mac_ops->validate() will have changed in any way between the
calls.

Anyway, if phylink_validate() is never going to confront us about the
pl->supported link mode mask becoming zero, you might wonder why it
calls even inquisitively in the first place.

Essentially phylink_parse_fixedlink() just wants to print in case it's
using a link speed that isn't supported by the driver. To do that, it
calls phy_lookup_setting() where one of the arguments is pl->supported
itself. But in our case, there is no link mode for speed 2000, although
that shouldn't matter, since no Ethernet PHY sees or needs to advertise
this speed, so phy_lookup_setting() finds nothing. I suspect this is
largely due to historical reasons, where the link modes were the common
denominator at the level of the driver visible phylink_validate() API.
Today we may simply extend config->mac_capabilities and forgo adding
bogus link modes just for this to work.

Curiously, even if we go to the extra lengths of silencing phylink's
"fixed link not recognised" warning, nothing seems to be broken even if
we don't do that.

Immediately after pl->supported has been populated by the inquisitive
phylink_validate(), phylink clears it (which means that the pl->supported
variable used above could have very well been just a temporary on-stack
variable), and just populates some fields.
Namely the pause fields, and a *single* speed, corresponding to "s"
(what phy_lookup_setting() found).

	linkmode_zero(pl->supported);
	phylink_set(pl->supported, MII);
	phylink_set(pl->supported, Pause);
	phylink_set(pl->supported, Asym_Pause);
	phylink_set(pl->supported, Autoneg);
	if (s) {
		__set_bit(s->bit, pl->supported);
		__set_bit(s->bit, pl->link_config.lp_advertising);
	} else {
		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
			     pl->link_config.speed);
	}

Why phylink even bothers to keep the speed-related linkmode in
pl->supported, if it won't use it anywhere further, I can't answer.
I can even delete the "if (s) ... else ..." block altogether and nothing
seems to be adversely impacted.

In any case, the short version of the code walkthrough is that phylink
can apparently operate in fixed-link mode with a pl->supported and
pl->link_config.lp_advertising mask of link modes that doesn't contain
any speed, and this won't generate any error, although I'm not completely
sure it was intended either.

> I would prefer if also we sort of "transferred" the 'fixed-link'
> parameters from the DSA Ethernet controller attached to the CPU port
> onto the PHYLINK instance of the CPU port in the switch as they ought
> to be strictly identical otherwise it just won't work. This would
> ensure that we continue to force the link and it would make me sleep
> better a night to know that the IMP port is operating strictly the
> same way it was. My script compares register values before/after for
> the registers that are static and this was flagged as a difference.

There are several problems with transferring the parameters. Most
obvious derives from what we discussed about speed = <2000> just above:
the DSA master won't have it, either, because it's a non-standard speed.
Additionally, the DSA master may be missing the phy-mode too.

Second has to do with how we transfer the phy-mode assuming it isn't
missing on the master. RGMII modes are clearly problematic precisely
because we have so many driver interpretations of what they mean.
But "mii" and "rmii" aren't all that clear-cut either. Do we translate
into "mii" and "rmii" for DSA, or "rev-mii" and "rev-rmii"?
bcm_sf2 understands "rev-mii", but mv88e6xxx doesn't.

Powered by blists - more mailing lists