DSA: request for your help with your DSA driver

Hello guys,

this is a request for help/comments on the DSA drivers you have been working on in Linux.

I am writing this e-mail because people don't seem to be responding to RFC patches.

Basically Russell and I are trying to do some development on phylink + DSA to move it forward, without breaking existing drivers:

- we want to make it so that phylink is always used for CPU and DSA ports (needed for conversion to phylink_pcs)

- to do that, phylink needs to know the interface type to which the port will be configure

- many drivers don't report that information now:
- some use port's phy-mode property from device-tree
- some leave it at default (HW initialized)
- some choose the mode according to some information the driver computes
- some try to find a mode that gives the maximum possible speed (mv88e6xxx, for example)

for example take a look at mt7530 driver

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mt7530.c?h=v5.19-rc5
    lines 2213-2250
       function mt7530_setup()
          reads phy-mode from device-tree if port 5 has a node,
          otherwise tries to determine the mode from gmac node
    line 2847
       function mt7531_cpu_port_config()
          decides PHY mode according to some prior settings

Russell wrote a RFC series
https://lore.kernel.org/netdev/[email protected]/T/
in which
- mv88e6xxx is patched to report the max speed mode to phylink
- for other drivers, if no default interface is reported, an interface will be inferred from the reported mac capabilities, such that it gives maximum possible speed

It is very probable that this will break your drivers, and so I ask you to look at the RFC series, maybe test it, and give your comments or additional patches that make it work.

Thank you.

Marek

I've seen Russell's patches and looked at the impact on the drivers I maintain:

drivers/net/dsa/realtek/rtl8366rb.c
Uses
phylink_mac_link_up -> rtl8366rb_mac_link_up()

This will simply (possibly erroneously WRT the API) force the CPU port link into 1GB mode with no autonegotiation and then enable the CPU port.

phylink_mac_link_down -> rtl8366rb_mac_link_down()

This will disable the CPU port.

I suspect this is maybe not ideal. I look at the (a bit horrible) vendor driver and I see it supports forcing:

- Link Up/Down
- Link speed (10M, 100M, 1G)
- Duplex (full/half)
- Optional txPause
- Optional rxPause

I suspect I should be handling all this properly with the .phylink_get_caps and augment .phylib_mac_link_up to properly force the requested features.

I'm a bit uncertain about .phylink_mac_config callbacks?

It seems if I should fix up this incomplete implementation I'd best do that on top of Russell's patches? I can also test with the patches applied but the way this driver (ab-)uses the API I think it probably us a no-op.

The other switch chip I comaintain is drivers/net/dsa/vitesse-vsc73xx-core.c which isn't using any phylink callbacks at all, just .adjust_link. It might need some good patching as well but I am uncertain where to start with that one.

Yours,
Linus Walleij

It would be good to know that this series at least does not break the driver.

I don't see anything obvious in there which would prevent a conversion, and I'd suggest doing it after this series has been applied, as it makes the whole way phylink gets used in DSA much more sane (i.o.w. phylink always gets used for every port in a phylink-converted DSA driver, instead of only in most but not all situations.)

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Hi Marek,

<snip>

For drivers/net/dsa/realtek/rtl8365mb:

I think we are OK. The .phylink_get_caps op will populate supported_interfaces faithfully for all port types, so in the absence of a fixed-link I think it is perfectly fine for phylink to calculate the highest speed interface and use that. The driver and hardware will be able to support whatever it advertises.

From reading the series I understand that the behaviour for DTs with fixed-link will remain unchanged.

With the above in mind, I think it's fine not to set default_interface in the .phylink_get_caps callback? This means "let phylink decide the most suitable default".

Kind regards,
Alvin

Yes, that is correct. Thanks for taking a look.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!