DSA: request for your help with your DSA driver

DSA: request for your help with your DSA driver

[Marek Behún]

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/YsQIjC7UpcGWJovx@shell.armlinux.org.uk/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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DSA: request for your help with your DSA driver

[Linus Walleij]

On Tue, Jul 5, 2022 at 2:22 PM Marek Behún <kabel@kernel.org> wrote:

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DSA: request for your help with your DSA driver

[Russell King (Oracle)]

On Tue, Jul 05, 2022 at 03:32:50PM +0200, Linus Walleij wrote:
> 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.

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

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

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DSA: request for your help with your DSA driver

[Alvin Šipraga]

Hi Marek,

On Tue, Jul 05, 2022 at 02:22:36PM +0200, Marek Behún wrote:
> 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

<snip>

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

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: DSA: request for your help with your DSA driver

[Russell King (Oracle)]

On Tue, Jul 05, 2022 at 01:45:42PM +0000, Alvin Šipraga wrote:
> 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".

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel