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.
Can you confirm that the FEC will not attempt to add internal RGMII delays in this case? Either the KSZ switch or the FEC should add delays, but not both (the link will not work otherwise).
Correct, the FEC does not configure delays, only the PHY side.
In the case of DSA for dt it is correct to have phy-mode defined both in the MAC and the PHY (each cpu port) correct?
It is, correct, but I'm not quite sure they're both supposed to be "rgmii-id", more like one is "rgmii-id" and the other is plain "rgmii". Anyhow.
If I hack around that my moving i2c in imx8mm.dtsi after fec I see a U-Boot net device for all the ports including the cpu port.
Do you have patch e5d7d119287e ("net: dsa: probe master device")? Does that not work?
Yes, I do have that patch and it seems I did not understand the issue properly.
The issue I'm seeing looks like it has to do with the fec driver. Whe fecmxc_probe is called it calls fec_get_miibus()->fec_mii_setspeed()->fec_get_ckl_rate() which fails because it uses 'uclass_get_device(UCLASS_ETH, 0, &dev) to find the first eth device assuming its the fec and instead its the lan1 port. It seems the fec_get_clk_rate() makes a poor assumption that the first UCLASS_ETH is the fec device and I'll have to find a way to fix that.
I'm not familiar with the FEC driver internals, so you might need to ping some of the driver authors for help if needed, but any assumption about UCLASS_ETH device indices and then calling dev_get_priv() on them looks wrong, yes.
I've also found that if I do not have a 'fixed-link' node in each of my downstream rj45 ports (lan1/lan2/lan3/lan4 above) else dsa_port_probe() will return -ENODEV due to dm_eth_phy_connect()->dm_eth_connect_phy_handle() failing because it does not see the link as fixed. Should I really need a 'fixed-link' in my non-cpu port children above? Not only is it not needed for Linux dsa to work if I add it to the Linux dt, the Linux ksz9477 driver crashes (at least in 5.4). I feel like perhaps dm_eth_phy_connect() should default to thinking of the ports as fixed links if they have no phy-handle/phy/phy-device node.
See below.
I'm not quite sure what the necessity of the cpu port is here because I assume the idea is to set 'ethact' to one of the physical ports before initiating a cmd that sends/receives traffic but I suppose the cpu interface is there because that's what Linux dsa does.
I am quite unsure what you mean. 'CPU port' == 'DSA master', i.e. the FEC? You're asking why does the FEC driver register a UCLASS_ETH device, considering that it should know it's a DSA master and you should only use the DSA UCLASS_ETH devices for RX/TX? I mean, if you can make that change and keep the drivers unaware of the fact that they are DSA masters, I guess patches are welcome...
Without adding any tagging and just doing a 'setenv ethact lan1; ping <serverip>' I'm not seeing any response.
What do you mean 'response'? If you tcpdump on the other end do you see the packet transmitted by U-Boot coming through? You might want to add a debugging patch printing some port statistics.
And are you expecting too? Is the switch configured to forward packets received from the host port which have no tail tag? Does it do that? If not, is it an electrical problem (see the RGMII delay question)?
I understand that tagging is needed here in order to determine the port the packet was meant for. When I add tagging of any kind the fec driver appears to crash (memory alignment issue?) so I started off by not tagging (dsa_set_tagging(dev, 0, 0)) and only enabling lan1 but found a ping fails. I know that I've got my RGMII delay configured correctly for the switch's cpu port so I'm not sure where to look next.
Again, this would need more debugging in fecmxc_send() to see what crashes and why.
I'm not exactly clear what to be using here for tagging. The Linux ksz driver [3] appears to add a 2 byte tag on ingress packets but a 1 byte on egress packets.
That's the expectation, to use the tail tagger to be able to steer packets towards the desired port, correct.
I can really use anything I want for tagging because the tag is only present to/from master to cpu port right? So I can use bytes on head, or bytes on tail, and any magic I want right?
You can use anything you want for tagging as long as the hardware understands it, so in practice that means "not really anything"... In particular, I think the KSZ switches use tail tags, not headers, so that's what you need to work with. I'm not sure how clear this is for you, but here's an image from the Linux Documentation/networking/dsa/dsa.rst file:
Unaware application opens and binds socket | ^ | | +-----------v--|--------------------+ |+------+ +------+ +------+ +------+| || swp0 | | swp1 | | swp2 | | swp3 || |+------+-+------+-+------+-+------+| | DSA switch driver | +-----------------------------------+ | ^ Tag added by | | Tag consumed by switch driver | | switch driver v | +-----------------------------------+ | Unmodified host interface driver | Software--------+-----------------------------------+------------ | Host interface (eth0) | Hardware +-----------------------------------+ | ^ Tag consumed by | | Tag added by switch hardware | | switch hardware v | +-----------------------------------+ | Switch | |+------+ +------+ +------+ +------+| || swp0 | | swp1 | | swp2 | | swp3 || ++------+-+------+-+------+-+------++
The reason why I suggested you can use VLANs is because switches can typically be configured to understand VLANs. On ingress from the outside world, you configure port i with pvid i so that it tags all untagged packets with that VID (and you configure the CPU port as egress-tagged in that VLAN, so that software can recover it). On egress from the CPU, you set up one VLAN per each front-facing port, which contains only the CPU port and the destination where you want that packet to go (i.e. one front port). You configure the front port as egress-untagged in that VLAN so that the fact a VLAN is being transmitted between the host port and the switch (effectively a DSA tagging protocol) is invisible to everybody on the wire. Of course, you don't have to use VLANs, it was just a suggestion.
I'm also not quite clear if I should be implementing link detect on the ports.
DSA assumes that every port has a phy-handle to a PHY driver. In Linux, the integrated PHYs from the KSZ switches are implemented using an MDIO bus registered by the DSA switch driver, with custom logic to access each PHY register from the port memory space, because these switches are kinda quirky and don't really expose a clause 22 MDIO register map. I'm not sure how that would work out in U-Boot, but it is the starting point I would say.
I can have my ksz dsa driver register an mdio bus for this and this is probably the piece that I'm currently missing
This is what I've been trying to tell you. Linux DSA offers some helpers for accessing internal PHYs, which in my opinion it should have not offered, see dsa_slave_mii_bus_init(). In particular it assumes that port i will have a PHY at MDIO address i. U-Boot DSA does not offer this helper, your only constructs are: (a) fixed-link: this means there is no PHY and that the MAC operates at a constant speed/duplex setting throughout the runtime of the device, with no means of link detection. This is probably what you want for the MAC-to-MAC link between the host port and the switch CPU port, but not what you want for the front-facing RJ45 ports which can switch speeds between 10/100/1000.
yes, this is what I want for the FEC to cpu-port (port5) link.
(b) phy-handle: this is a phandle to a device tree description of a PHY device, be it an integrated PHY or a discrete one. Of course, your big problem is that you don't have a struct phy_device for your PHYs integrated in the KSZ switch, especially one with OF bindings.
yes, this is what I need for the lan1/lan2/lan3/lan4 ports.
For reference, the sja1105 Linux driver also supports switches with internal PHYs, here's how it describes things in the device tree. Because on this switch, pinmuxing is complex, and certain ports like port 1 can either be connected to an internal 100base-TX PHY or to an SGMII PCS, describing the phy-handle in the device tree is necessary even if it is towards a struct phy_device registered by an MDIO bus provided by the same driver (instead of making the assumption that the pins are strapped one way or another in the driver itself).
&spi_bridge { /* SW1 */ ethernet-switch@0 { compatible = "nxp,sja1110a"; reg = <0>; spi-max-frequency = <4000000>; spi-cpol; dsa,member = <0 0>;
ethernet-ports { #address-cells = <1>; #size-cells = <0>; /* Microcontroller port */ port@0 { reg = <0>; status = "disabled"; }; /* SW1_P1 */ port@1 { reg = <1>; label = "con_2x20"; phy-mode = "internal"; phy-handle = <&sw1_port1_base_tx_phy>; }; port@2 { reg = <2>; ethernet = <&dpmac17>; phy-mode = "rgmii-id"; fixed-link { speed = <1000>; full-duplex; }; }; port@3 { reg = <3>; label = "1ge_p1"; phy-mode = "rgmii-id"; phy-handle = <&sw1_mii3_phy>; }; sw1p4: port@4 { reg = <4>; link = <&sw2p1>; phy-mode = "sgmii"; fixed-link { speed = <1000>; full-duplex; }; }; port@5 { reg = <5>; label = "trx1"; phy-mode = "internal"; phy-handle = <&sw1_port5_base_t1_phy>; }; port@6 { reg = <6>; label = "trx2"; phy-mode = "internal"; phy-handle = <&sw1_port6_base_t1_phy>; }; port@7 { reg = <7>; label = "trx3"; phy-mode = "internal"; phy-handle = <&sw1_port7_base_t1_phy>; }; port@8 { reg = <8>; label = "trx4"; phy-mode = "internal"; phy-handle = <&sw1_port8_base_t1_phy>; }; port@9 { reg = <9>; label = "trx5"; phy-mode = "internal"; phy-handle = <&sw1_port9_base_t1_phy>; }; port@a { reg = <10>; label = "trx6"; phy-mode = "internal"; phy-handle = <&sw1_port10_base_t1_phy>; }; }; mdios { #address-cells = <1>; #size-cells = <0>; mdio@0 { reg = <0>; compatible = "nxp,sja1110-base-t1-mdio"; #address-cells = <1>; #size-cells = <0>; sw1_port5_base_t1_phy: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x1>; }; sw1_port6_base_t1_phy: ethernet-phy@2 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x2>; }; sw1_port7_base_t1_phy: ethernet-phy@3 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x3>; }; sw1_port8_base_t1_phy: ethernet-phy@4 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x4>; }; sw1_port9_base_t1_phy: ethernet-phy@5 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x5>; }; sw1_port10_base_t1_phy: ethernet-phy@6 { compatible = "ethernet-phy-ieee802.3-c45"; reg = <0x6>; }; }; mdio@1 { reg = <1>; compatible = "nxp,sja1110-base-tx-mdio"; #address-cells = <1>; #size-cells = <0>; sw1_port1_base_tx_phy: ethernet-phy@1 { reg = <0x1>; }; }; }; };};
[ 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 ]
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.
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>;
sw_phy0: ethernet-phy@0 { compatible = "ethernet-phy-ieee8023-c45"; 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.
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.
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?
Thanks for your help!
Tim