U-Boot DSA driver for microchip KSZ9477 suggestions needed
Tim Harvey
tharvey at gateworks.com
Thu Jun 24 21:26:53 CEST 2021
On Wed, Jun 23, 2021 at 7:05 PM Vladimir Oltean <olteanv at gmail.com> wrote:
>
> 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.
agreed, this is confusing but this is how the mv88e61xx.c
switch-as-phy driver works which I modeled my switch-as-phy driver
from.
>From a user perspective with this kind of switch-as-phy approach
network traffic works through any port with a link due to the switch
being in forwarding mode and there is no need to know what port is
active but personally if you are using DSA in Linux such that each
port has a network interface then I do like the model of doing the
same in U-Boot.
>
> 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.
this seems like a hack that was simply done because it pre-dated U-Boot DSA.
>
> 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).
>
I'm ok with it. The gw7901 dts is already upstream and working with
the Linux DSA drivers
(https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mm-venice-gw7901.dts?h=for-next).
The way I see it by adding the phy-handle and phy-mode props to the
non-cpu ports and adding an mdio device I'm only adding more clarity
to the dt for U-Boot and the binding changes would not affect Linux
DSA drivers if somehow they were used for Linux.
> > 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 {
> };
> };
> };
>
I wonder though if for consistency of U-Boot DSA dt's it would be best
to encapsulate the 1 or more mdio's in an 'mdios' child. The dsa
driver needs to specifically bind the mdio driver and at least that
code can look the same for various dsa drivers, or perhaps that code
can even be put in uclass-dsa.
Perhaps I made the binding of the mdio driver overly-complicated. This
is what I'm doing to bind any children matching a specific compatible
within the 'mdios' child of the switch:
#define KSZ_MDIO_CHILD_DRV_NAME "ksz_mdio"
static const struct udevice_id ksz_mdio_ids[] = {
{ .compatible = "microchip,ksz-mdio" },
{ }
};
U_BOOT_DRIVER(ksz_mdio) = {
.name = KSZ_MDIO_CHILD_DRV_NAME,
.id = UCLASS_MDIO,
.of_match = ksz_mdio_ids,
.bind = ksz_mdio_bind,
.probe = ksz_mdio_probe,
.ops = &ksz_mdio_ops,
.priv_auto = sizeof(struct ksz_mdio_priv),
.plat_auto = sizeof(struct mdio_perdev_priv),
};
static int ksz_i2c_probe(struct udevice *dev)
{
...
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);
}
}
...
}
> >
> > 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.
>
ok - when I remove it the genphy driver continues to work for per-port
link detect
> > 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().
Yes, that's what I needed - thanks for pointing that out!
Now per-port auto negotiation and link detect is working for me. I do
find that if a non-cpu port has no link the network transaction
continues on like it does have a link so there is likely a check that
should be added somewhere. I will look into that.
>
> > 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?
I do not use CONFIG_NET_RANDOM_ETHADDR but I do have env addr env for
each mac as for Linux DSA we want each non-cpu port to have a unique
MAC with the board mfg OID so this was the culprit. Setting the
eth*addr env vars and having a local-mac-address prop in the non-cpu
ports is a way of passing that mac address along from U-Boot to Linux
thus I would loose that capability if I didn't set that env. Now that
I understand the issue and realize that the non-cpu ports 'must' share
the MAC of the master, why wouldn't that just be enforced by
uclass-dsa? What is the reasoning behind allowing env to set them?
So now that all of my ports are working with auto-negotiation and link
detect I suppose the final thing I need to do is add tagging or vlanid
to be able to tell dsa what port the received packets came from.
Thanks,
Tim
More information about the U-Boot
mailing list