U-Boot DSA driver for microchip KSZ9477 suggestions needed
Tim Harvey
tharvey at gateworks.com
Thu Jun 24 02:28:42 CEST 2021
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.
>
> > > 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 at 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 at 0 {
> reg = <0>;
> status = "disabled";
> };
>
> /* SW1_P1 */
> port at 1 {
> reg = <1>;
> label = "con_2x20";
> phy-mode = "internal";
> phy-handle = <&sw1_port1_base_tx_phy>;
> };
>
> port at 2 {
> reg = <2>;
> ethernet = <&dpmac17>;
> phy-mode = "rgmii-id";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> port at 3 {
> reg = <3>;
> label = "1ge_p1";
> phy-mode = "rgmii-id";
> phy-handle = <&sw1_mii3_phy>;
> };
>
> sw1p4: port at 4 {
> reg = <4>;
> link = <&sw2p1>;
> phy-mode = "sgmii";
>
> fixed-link {
> speed = <1000>;
> full-duplex;
> };
> };
>
> port at 5 {
> reg = <5>;
> label = "trx1";
> phy-mode = "internal";
> phy-handle = <&sw1_port5_base_t1_phy>;
> };
>
> port at 6 {
> reg = <6>;
> label = "trx2";
> phy-mode = "internal";
> phy-handle = <&sw1_port6_base_t1_phy>;
> };
>
> port at 7 {
> reg = <7>;
> label = "trx3";
> phy-mode = "internal";
> phy-handle = <&sw1_port7_base_t1_phy>;
> };
>
> port at 8 {
> reg = <8>;
> label = "trx4";
> phy-mode = "internal";
> phy-handle = <&sw1_port8_base_t1_phy>;
> };
>
> port at 9 {
> reg = <9>;
> label = "trx5";
> phy-mode = "internal";
> phy-handle = <&sw1_port9_base_t1_phy>;
> };
>
> port at a {
> reg = <10>;
> label = "trx6";
> phy-mode = "internal";
> phy-handle = <&sw1_port10_base_t1_phy>;
> };
> };
>
> mdios {
> #address-cells = <1>;
> #size-cells = <0>;
>
> mdio at 0 {
> reg = <0>;
> compatible = "nxp,sja1110-base-t1-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> sw1_port5_base_t1_phy: ethernet-phy at 1 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x1>;
> };
>
> sw1_port6_base_t1_phy: ethernet-phy at 2 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x2>;
> };
>
> sw1_port7_base_t1_phy: ethernet-phy at 3 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x3>;
> };
>
> sw1_port8_base_t1_phy: ethernet-phy at 4 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x4>;
> };
>
> sw1_port9_base_t1_phy: ethernet-phy at 5 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x5>;
> };
>
> sw1_port10_base_t1_phy: ethernet-phy at 6 {
> compatible = "ethernet-phy-ieee802.3-c45";
> reg = <0x6>;
> };
> };
>
> mdio at 1 {
> reg = <1>;
> compatible = "nxp,sja1110-base-tx-mdio";
> #address-cells = <1>;
> #size-cells = <0>;
>
> sw1_port1_base_tx_phy: ethernet-phy at 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 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>;
sw_phy0: ethernet-phy at 0 {
compatible =
"ethernet-phy-ieee8023-c45";
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.
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
More information about the U-Boot
mailing list