On Wed, Jun 23, 2021 at 05:28:42PM -0700, Tim Harvey wrote:
On Wed, Jun 23, 2021 at 3:37 AM Vladimir Oltean vladimir.oltean@nxp.com wrote:
On Tue, Jun 22, 2021 at 09:38:52AM -0700, Tim Harvey wrote:
On Mon, Jun 21, 2021 at 4:49 PM Vladimir Oltean olteanv@gmail.com wrote:
On Mon, Jun 21, 2021 at 04:10:51PM -0700, Tim Harvey wrote:
Greetings,
I've written a U-Boot phy driver for a Microchip KSZ9477 ethernet switch that I submitted some time ago as an RFC [1] which simply enables the ports in the dt and puts them in forwarding mode with link detect. However I think this would be much better suited as a DSA driver now that UCLASS_DSA exists.
Define 'link detect'. I don't know how the switch-as-PHY drivers work. I suppose the DSA master is made to connect to a PHY device which is implemented by the switch driver, but whose port's link status is being reported?
Yes, each downward port's link status can be detected. Each port has standard GbE PHY registers which can be indirectly read.
What do you mean by 'each port'? With the switch-as-PHY driver, how many UCLASS_ETH devices are being registered? One for the FEC and one for each front-facing switch port (same as DSA would)? Only one, and that for the FEC? If the latter, how does U-Boot know which of the front-facing switch ports to use as the link status reporting for the FEC device?
With my ksz9477 'switch-as-phy' driver (https://www.mail-archive.com/u-boot@lists.denx.de/msg389714.html) there is only 1 UCLASS_ETH device registered, just fec. The driver provides a UCLASS_ETH_PHY and its probe parses the dt to find the ports then registers an mdio bus for the indirect register access and registers a phy driver that matches the id's of the switch port.
This only worked with another patch (https://www.mail-archive.com/u-boot@lists.denx.de/msg389712.html) to the UCLASS_ETH_PHY driver that allowed the phy driver to bind the the fec mac. Therefore this acts as a single phy on the MAC and when phy_config is called it sets up auto-negotiation on all the non-cpu ports and when phy_startup is called it performs link detect on all the non-cpu ports (if any port is linked its considered a link). The phy_config configured all the ports in forwarding mode.
This was modeled after the drivers/net/phy/mv88e61xx.c (switch-as-phy) driver and the only difference between that is that it attempted to use driver-model and get its config from dt. The big downside of this method is that the switch is configured in forwarding mode so you end up with a bridge loop while active in U-Boot if you have more than one port connected to your network.
I think if you look over that relatively simple driver it will make sense how the ksz switch works in case I'm explaining it wrong or with the wrong terminology.
Ah, ok, so it enables switching. Basically my misunderstanding was, if the strategy is simply 'if any of the switch internal PHYs has link then the phy-device exposed to the host port has link', how can this work and not confuse the user badly. Say a cable is plugged, but not into the port you are trying to ping over.
But you've answered my question, the packets are replicated by the switch to all user ports if the situation demands it (FDB entry not present for that MAC DA), and once the destination is learned, packets will flow only towards the desired port.
I should have known better.
Actually there's a third type of switch driver now in U-Boot, which is drivers/net/vsc9953.c which uses some overly bloated struct ethsw_command_func operations and a dedicated "ethsw" U-Boot command to manage/call them. This seems to be completely missing the point of U-Boot networking (what user in their right mind needs to manage VLANs, the FDB, link aggregation groups, ingress filtering from U-Boot?). With this "ethsw" framework, it's a bit of a mix between the switch-as-PHY model and the DM DSA model.
The switch is managed through a plain U-Boot command, so there's no DM concept to speak of. The host port uses a fixed-link (same as DM DSA) but is otherwise completely unaware that there's a switch there, as it doesn't have a phy-handle to it. That's also an advantage, the switch might not be MDIO based, so a phy-handle to a memory-based device is kind of strange, and as you've seen in the change you made for the FEC, it can be quite limiting. Side note, I do happen to have the hardware for it, so I guess one day I should take the time to convert vsc9953.c to the DM DSA model as well.
DM DSA follows the Linux mentality that switches are just ports with some optional hardware acceleration. They behave and are used just like regular ports, up to the point where the hwaccel features are stripped to the bone and switching is not even supported because as you say it is dangerous to leave it enabled without a network stack properly prepared to deal with switch control protocols like STP. The one big use case for networking in U-Boot is TFTP/friends to load the next stage image and that is about it, whoever needs more is IMO doing it wrong. So DM DSA is modeled around this idea precisely.
[ please disregard the fact that there are 2 mdio buses, one for 100base-T1 and another for 100base-TX PHYs. There are reasons why that is, but I think you should only have one "mdio" node under the switch ]
^remember what I said here, I'll reference it here [1]
By following this example I believe you can model your ports with an internal PHY in pretty much the same way, while keeping the simple model that U-Boot DSA offers - and this should answer your other question too (fixed-link - no, phy-handle - yes). Please, let's keep the U-Boot DSA framework simple and without the bells and whistles from Linux, even if this means that the OF bindings for the switch need to be more descriptive and less free-form. This was a conscious design decision and I don't think it is limiting.
Interesting. So you're telling me that even though my current dt bindings work and are correct for 'Linux DSA' I need to change them (specifically, augment them) for U-Boot? This really seems wrong unless perhaps the augmentation needed for U-Boot DSA is not 'incorrect' for Linux DSA.
Yeah, you got the point.
It seems wrong until you consider that Linux DSA has 13 years of baggage, while U-Boot DM DSA is fresh and doesn't have to make all the same mistakes again. Having OF bindings, but not having a fixed-link or a phy-handle is ambiguous as to what the author meant to say. When Linux DSA transitioned from phylib to phylink on the CPU port a few years ago, this was a big problem, because the CPU port in some device trees was in the exact same situation (no phy-handle, no fixed-link), and phylink rightfully didn't know what to make of it. The (biased) interpretation given back then was "if you don't have a fixed-link on a port, then it just has an implicit fixed-link for the highest supported speed. And oh yeah, only if it's a CPU port btw!" https://lore.kernel.org/netdev/20200213144615.GH18808@shell.armlinux.org.uk/ Now you are saying "if a front-facing port does not have a fixed-link or a phy-handle specifier, then it is connected to an internal PHY". Obviously...
It gets more and more ambiguous each time the meaning is overloaded. How about we just say "no" and just go with the generic format, which covers this particular case too, even if more needs to be spelled out in the DT? Again, maybe for some switch drivers there isn't much information to be gained (the configuration is fixed, there is nothing that could have not been hardcoded in the driver), but if there is any sort of flexibility in the pinmuxing at all (say port 1 can go either to an internal PHY or to a SGMII SERDES lane which is fixed at gigabit), then the way the link is being reported/negotiated changes drastically and you do need to tell the driver what to do.
If your concern is that the bindings might diverge between Linux and U-Boot, I might be somewhat open to that argument (in the sense that I don't get why it is a big deal, but I might be able to help with reviews if you want to make the Linux KSZ driver support the fully-OF method of connecting to the internal PHY).
This does explain how I'm supposed to bind the non-cpu ports to a phy. So now I have:
&fec1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_fec1>; phy-mode = "rgmii-id"; local-mac-address = [00 00 00 00 00 00]; status = "okay";
fixed-link { speed = <1000>; full-duplex; };};
&i2c3 { clock-frequency = <400000>; pinctrl-names = "default"; pinctrl-0 = <&pinctrl_i2c3>; status = "okay";
switch: switch@5f { compatible = "microchip,ksz9897"; reg = <0x5f>; pinctrl-0 = <&pinctrl_ksz>; interrupt-parent = <&gpio4>; interrupts = <18 IRQ_TYPE_EDGE_FALLING>; ports { #address-cells = <1>; #size-cells = <0>; lan1: port@0 { reg = <0>; label = "lan1"; local-mac-address = [00 00 00 00 00 00]; phy-handle = <&sw_phy0>; phy-mode = "internal"; }; lan2: port@1 { reg = <1>; label = "lan2"; local-mac-address = [00 00 00 00 00 00]; phy-handle = <&sw_phy1>; // added for uboot phy-mode = "internal"; // added for uboot }; lan3: port@2 { reg = <2>; label = "lan3"; local-mac-address = [00 00 00 00 00 00]; phy-handle = <&sw_phy2>; // added for uboot phy-mode = "internal"; // added for uboot }; lan4: port@3 { reg = <3>; label = "lan4"; local-mac-address = [00 00 00 00 00 00]; phy-handle = <&sw_phy3>; // added for uboot phy-mode = "internal"; // added for uboot }; port@5 { reg = <5>; label = "cpu"; ethernet = <&fec1>; phy-mode = "rgmii-id"; fixed-link { speed = <1000>; full-duplex; }; }; }; /* added for U-Boot */ mdios { #address-cells = <1>; #size-cells = <0>; mdio@0 { reg = <0>; compatible = "microchip,ksz-mdio"; #address-cells = <1>; #size-cells = <0>;
[1] So if you are known to have a single MDIO bus, then as mentioned, instead of going
ethernet-switch { mdios { mdio@0 { ethernet-phy@0 { }; }; }; };
you can probably just do
ethernet-switch { mdio { ethernet-phy@0 { }; }; };
sw_phy0: ethernet-phy@0 { compatible = "ethernet-phy-ieee8023-c45";
This compatible string is for clause 45 PHYs, are your PHYs clause 45 (is their addressing scheme two-layered, with an MMD and a register address, or just a register address)? It's likely that this compatible string is simply ignored by U-Boot, you should remove it.
reg = <0x0>; }; sw_phy1: ethernet-phy@1 { compatible = "ethernet-phy-ieee8023-c45"; reg = <0x1>; }; sw_phy2: ethernet-phy@2 { compatible = "ethernet-phy-ieee8023-c45"; reg = <0x2>; }; sw_phy3: ethernet-phy@3 { compatible = "ethernet-phy-ieee8023-c45"; reg = <0x3>; }; }; }; };};
I've added UCLASS_MDIO driver with compatible matching 'microchip,ksz-mdio' and had to add some code to my switch probe func to probe the mdios:
ofnode node, mdios; mdios = dev_read_subnode(dev, "mdios"); if (ofnode_valid(mdios)) { ofnode_for_each_subnode(node, mdios) { const char *name = ofnode_get_name(node); struct udevice *pdev; ret = device_bind_driver_to_node(dev, KSZ_MDIO_CHILD_DRV_NAME, name, node, &pdev); if (ret) dev_err(dev, "failed to probe %s: %d\n", name, ret); } }I didn't see anything like that in your driver but I assume this was correct.
I should have assumed you were going to search the Dec 2019 submission for it. The answer is that only the NXP SJA1110 has internal PHYs, and the SJA1110 is not covered by that 2019 patch, only the older SJA1105 is. It might be when I find some time to resend.
So now I have a ksz-mdio driver which is getting probed and I see it properly reading phy_id registers for the non-cpu ports. I also have a ksz-phy-driver and its probe is called after the non-cpu port phys are identified. However I never do see the phy driver's config/startup/shutdown functions get called and am still digging into that.
Your switch driver needs to call these functions, does it do that? See felix_port_enable().
I'm also seeing packets go out the non-cpu ports properly but nothing is received from the fec driver and I'm thinking its because the fec driver is setting up a MAC addr filter only allowing packets for its MAC which clearly needs to be removed when used with a DSA driver I would think?
That seems highly plausible, but why? If you look inside dsa_port_probe() you'll find there is logic specifically for that: by default, DSA ports inherit the MAC address of their DSA master (FEC for you) if their env variable is not populated. Does that not work for you? How does the DSA port end up having a different MAC address compared to the FEC? Are you working with CONFIG_NET_RANDOM_ETHADDR enabled? But it should work even then. Have you manually changed the ethNaddr env variables?