U-Boot DSA driver for microchip KSZ9477 suggestions needed

Vladimir Oltean olteanv at gmail.com
Thu Jun 24 04:05:47 CEST 2021


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 at 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 at 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 at 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 at 0 {
>                                 reg = <0>;
>                                 label = "lan1";
>                                 local-mac-address = [00 00 00 00 00 00];
>                                 phy-handle = <&sw_phy0>;
>                                 phy-mode = "internal";
>                         };
> 
>                         lan2: port at 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 at 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 at 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 at 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 at 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 at 0 {
			ethernet-phy at 0 {
			};
		};
	};
};

you can probably just do

ethernet-switch {
	mdio {
		ethernet-phy at 0 {
		};
	};
};

> 
>                                 sw_phy0: ethernet-phy at 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 at 1 {
>                                         compatible = "ethernet-phy-ieee8023-c45";
>                                         reg = <0x1>;
>                                 };
>                                 sw_phy2: ethernet-phy at 2 {
>                                         compatible = "ethernet-phy-ieee8023-c45";
>                                         reg = <0x2>;
>                                 };
> 
>                                 sw_phy3: ethernet-phy at 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?


More information about the U-Boot mailing list