On Wed, Jun 23, 2021 at 3:37 AM Vladimir Oltean <[email protected]> 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 <[email protected]> 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/[email protected]/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/[email protected]/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
