U-Boot DSA driver for microchip KSZ9477 suggestions needed

Tim Harvey tharvey at gateworks.com
Tue Jun 22 18:38:52 CEST 2021


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.

>
> > The KSZ9477 switch is used in the Gateworks GW7901 IMX8M Mini based
> > board and is hooked up via I2C (instead of MDIO) so the relevant dt
> > (already in Mainline Linux) looks like this:
> >
> > / {
> >         aliases {
> >                 ethernet0 = &fec1;
> >                 ethernet1 = &lan1;
> >                 ethernet2 = &lan2;
> >                 ethernet3 = &lan3;
> >                 ethernet4 = &lan4;
> >                 usb0 = &usbotg1;
> >                 usb1 = &usbotg2;
> >         };
> > };
> >
> > &fec1 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_fec1>;
> >         phy-mode = "rgmii-id";
>
> 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?

>
> >         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>;
> >                 phy-mode = "rgmii-id";
>
> This is strange. Why is phy-mode in the 'switch' node (it is a per-port
> property)?

Right, this is wrong and just did not get caught. I will remove it.

>
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         lan1: port at 0 {
> >                                 reg = <0>;
> >                                 label = "lan1";
> >                                 local-mac-address = [00 00 00 00 00 00];
> >                         };
> >                         lan2: port at 1 {
> >                                 reg = <1>;
> >                                 label = "lan2";
> >                                 local-mac-address = [00 00 00 00 00 00];
> >                         };
> >                         lan3: port at 2 {
> >                                 reg = <2>;
> >                                 label = "lan3";
> >                                 local-mac-address = [00 00 00 00 00 00];
> >                         };
> >                         lan4: port at 3 {
> >                                 reg = <3>;
> >                                 label = "lan4";
> >                                 local-mac-address = [00 00 00 00 00 00];
> >                         };
> >                         port at 5 {
> >                                 reg = <5>;
> >                                 label = "cpu";
> >                                 ethernet = <&fec1>;
> >                                 phy-mode = "rgmii-id";
> >
> >                                 fixed-link {
> >                                         speed = <1000>;
> >                                         full-duplex;
> >                                 };
> >                         };
> >                 };
> >         };
> > };
> >
> > I'm running into an issue trying to get a DSA driver working for this
> > switch on this board. My UCLASS_DSA driver is getting registered and
> > dsa_post_bind successfully sees the ports and the master_dev however
> > when eth_initialize() is called my dsa driver probes before the fec1
> > master because i2c comes before fec in the imx8mm.dtsi device-tree.
> > Any suggestions on how and where to properly resolve this (Likely
> > there is a way to force a probe)?
> >
> > 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'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.

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

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

> If you have problems with that you can always use VLANs too, like
> sja1105 does:
> https://patchwork.ozlabs.org/project/uboot/patch/20191215205302.13325-4-olteanv@gmail.com/
>

Thanks - I'll look that over. Currently the only dsa driver examples
in U-Boot are the felix_switch.c and dsa_sandbox.c

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

>
> > Any suggestions would be greatly appreciated. I would like to get this
> > dsa driver working and submitted and then perhaps move on to added a
> > DSA driver or DSA support to drivers/net/phy/mv88e61xx.c
>
> That would be nice. I also need to find some time to resend the sja1105
> driver.

Thanks for the feedback!

Tim


More information about the U-Boot mailing list