U-Boot DSA driver for microchip KSZ9477 suggestions needed

[Vladimir Oltean]

On Thu, Jun 24, 2021 at 12:26:53PM -0700, Tim Harvey wrote:
> > [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.

If it is a conscious decision then I don't disagree with it. Embedding
the potentially more than one MDIO controller under the same "mdios"
node will lead to more uniform bindings.

Andrew Lunn explained here that even "marvell,mv88e6xxx-mdio-external"
could have benefited from a better organization of multiple MDIO buses,
given some forethought:
https://patchwork.kernel.org/project/netdevbpf/patch/20210524232214.1378937-12-olteanv@gmail.com/

Nonetheless, now they have an "mdio" and an "mdio1" node under the
switch, with "mdio1" not going through the YAML validator. See
arm/boot/dts/vf610-zii-ssmb-dtu.dts in Linux.

> 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);

Don't you have to know the drv_name beforehand, making this tricky to
implement centrally in DM DSA?

If you want it to be common I suppose what you can do is you can offer
it as a library service (driver can optionally call this function
provided by DSA with its drv_name as argument).

>                         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 suspect you will need a KSZ PHY specific driver and the genphy one
will not be enough.

> >
> > > 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?

The reasoning behind allowing the env to set individual MAC addresses
per DSA port is to make people like you happy (don't you want the board
to send packets with the same source MAC address in U-Boot as in Linux?).

One key thing you need to understand is that there are various kinds of
DSA tagging protocols - basically places where the switch expects the
tag to be put, and how it looks like. For felix_switch.c (the only piece
of hardware supported by DM DSA today), the DSA tag is placed in front
of the Ethernet header, and it starts with 6 bytes of ff:ff:ff:ff:ff:ff.
The DSA master perceives this as being the MAC address of all ingress
packets, whereas the MAC address of the individual switch ports is
merely payload in this system (it is shifted to the right by an amount
of bytes equal to the DSA tag length). So the DSA master doesn't have
any RX filtering issues.

Whereas in your case, the MAC address that the DSA master sees is
precisely the MAC address of the switch port. In Linux, what we did is
we added a "bool promisc_on_master" in struct dsa_device_ops.
I recommend that you introduce a new "set_promisc" function pointer to
struct eth_ops, you implement it for the FEC driver, then you make DSA
call it on the DSA master if available. Managing address filters is not
really justified.

> 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