Message-ID: <20220727132309.7g275ytefdhqmxip@skbuf>
Date:   Wed, 27 Jul 2022 16:23:09 +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 07:17:24PM -0700, Florian Fainelli wrote:
> > > 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.
> 
> Yep, you have me convinced. I suppose the course of action for me is to
> update the DTSes to also include a fixed-link property and phy-mode property
> in the CPU node, even if that duplicates what the Ethernet controller node
> already has, and then given a cycle or two, merge this patch series.

I don't want to say that the 'peeking at the master' technique can't be
used at all, maybe if it would only come down to internal ports, and you
could give me a guarantee that the master does have a valid DT description
for your SoCs, we could consider doing this as a transition thing for
bcm_sf2 (on non-4908, see more below).

Having said that, I looked again at my analysis for bcm_sf2:
https://patchwork.kernel.org/project/netdevbpf/patch/20220723164635.1621911-1-vladimir.oltean@nxp.com/

and I notice that I said that arch/arm64/boot/dts/broadcom/bcm4908/bcm4908.dtsi
*does* have a valid (complete at least) OF node description for the CPU port:

	port@8 {
		reg = <8>;
		phy-mode = "internal";
		ethernet = <&enet>;

		fixed-link {
			speed = <1000>;
			full-duplex;
		};
	};

but I didn't have the information that the speed is wrong (you said you
want it to operate at 2G).

Additionally (and curiously), the "enet" node lacks any link
description:

	enet: ethernet@...0 {
		compatible = "brcm,bcm4908-enet";
		reg = <0x2000 0x1000>;

		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "rx", "tx";
	};

My question is, do you use the DTS file as seen in the mainline kernel
at all, or is it just for some sort of reference?

In case the 4908 really does have a fixed-link at 1000 provided by the
bootloader, you should be free to move the IMP setup code to
phylink_mac_link_up(), because phylink does have what it needs to run,
and just hack it to 2G for this SoC due to driver level knowledge,
despite what phylink thinks is going on. This can take place indefinitely.
In the long term, I don't know what else to suggest beyond fixing the
device tree to report the proper speed, sorry.

Powered by blists - more mailing lists