On 7/5/22 02:46, Russell King (Oracle) wrote:

A new revision of the series which incorporates changes that Marek
suggested. Specifically, the changes are:

1. Patch 2 - use the phylink_get_caps method in mv88e6xxx to get the
    default interface rather than re-using port_max_speed_mode()

2. Patch 4 - if no default interface is provided, use the supported
    interface mask to search for the first interface that gives the
    fastest speed.

3. Patch 5 - now also removes the port_max_speed_mode() method

This was tested with bcm_sf2.c and b53_srab.b and did not cause 
regressions, however we do have a 'fixed-link' property for the CPU port 
(always have had one), so there was no regression expected.

See answers to your RFC v1 below.

  drivers/net/dsa/b53/b53_common.c       |   3 +-
  drivers/net/dsa/bcm_sf2.c              |   3 +-
  drivers/net/dsa/hirschmann/hellcreek.c |   3 +-
  drivers/net/dsa/lantiq_gswip.c         |   6 +-
  drivers/net/dsa/microchip/ksz_common.c |   3 +-
  drivers/net/dsa/mt7530.c               |   3 +-
  drivers/net/dsa/mv88e6xxx/chip.c       | 136 +++++++++++++++---------------
  drivers/net/dsa/mv88e6xxx/chip.h       |   6 +-
  drivers/net/dsa/mv88e6xxx/port.c       |  32 -------
  drivers/net/dsa/mv88e6xxx/port.h       |   5 --
  drivers/net/dsa/ocelot/felix.c         |   3 +-
  drivers/net/dsa/qca/ar9331.c           |   3 +-
  drivers/net/dsa/qca8k.c                |   3 +-
  drivers/net/dsa/realtek/rtl8365mb.c    |   3 +-
  drivers/net/dsa/sja1105/sja1105_main.c |   3 +-
  drivers/net/dsa/xrs700x/xrs700x.c      |   3 +-
  drivers/net/phy/phylink.c              | 150 ++++++++++++++++++++++++++++++---
  include/linux/phylink.h                |   5 ++
  include/net/dsa.h                      |   3 +-
  net/dsa/port.c                         |  47 +++++++----
  20 files changed, 270 insertions(+), 153 deletions(-)

On Wed, Jun 29, 2022 at 01:49:57PM +0100, Russell King (Oracle) wrote:

quoted

Mostly the same as the previous RFC, except:

1) incldues the phylink_validate_mask_caps() function
2) has Marek's idea of searching the supported_interfaces bitmap for the
    fastest interface we can use
3) includes a final patch to add a print which will be useful to hear
    from people testing it.

Some of the questions from the original RFC remain though, so I've
included that text below. I'm guessing as they remain unanswered that
no one has any opinions on them?

  drivers/net/dsa/b53/b53_common.c       |   3 +-
  drivers/net/dsa/bcm_sf2.c              |   3 +-
  drivers/net/dsa/hirschmann/hellcreek.c |   3 +-
  drivers/net/dsa/lantiq_gswip.c         |   6 +-
  drivers/net/dsa/microchip/ksz_common.c |   3 +-
  drivers/net/dsa/mt7530.c               |   3 +-
  drivers/net/dsa/mv88e6xxx/chip.c       |  53 ++++--------
  drivers/net/dsa/ocelot/felix.c         |   3 +-
  drivers/net/dsa/qca/ar9331.c           |   3 +-
  drivers/net/dsa/qca8k.c                |   3 +-
  drivers/net/dsa/realtek/rtl8365mb.c    |   3 +-
  drivers/net/dsa/sja1105/sja1105_main.c |   3 +-
  drivers/net/dsa/xrs700x/xrs700x.c      |   3 +-
  drivers/net/phy/phylink.c              | 148 ++++++++++++++++++++++++++++++---
  include/linux/phylink.h                |   5 ++
  include/net/dsa.h                      |   3 +-
  net/dsa/port.c                         |  47 +++++++----
  17 files changed, 215 insertions(+), 80 deletions(-)

On Fri, Jun 24, 2022 at 12:41:26PM +0100, Russell King (Oracle) wrote:

quoted

Hi,

Currently, the core DSA code conditionally uses phylink for CPU and DSA
ports depending on whether the firmware specifies a fixed-link or a PHY.
If either of these are specified, then phylink is used for these ports,
otherwise phylink is not, and we rely on the DSA drivers to "do the
right thing". However, this detail is not mentioned in the DT binding,
but Andrew has said that this behaviour has always something that DSA
wants.

mv88e6xxx has had support for this for a long time with its "SPEED_MAX"
thing, which I recently reworked to make use of the mac_capabilities in
preparation to solving this more fully.

This series is an experiment to solve this properly, and it does this
in two steps.

The first step consists of the first two patches. Phylink needs to
know the PHY interface mode that is being used so it can (a) pass the
right mode into the MAC/PCS etc and (b) know the properties of the
link and therefore which speeds can be supported across it.

In order to achieve this, the DSA phylink_get_caps() method has an
extra argument added to it so that DSA drivers can report the
interface mode that they will be using for this port back to the core
DSA code, thereby allowing phylink to be initialised with the correct
interface mode.

Note that this can only be used for CPU and DSA ports as "user" ports
need a different behaviour - they rely on getting the interface mode
from phylib, which will only happen if phylink is initialised with
PHY_INTERFACE_MODE_NA. Unfortunately, changing this behaviour is likely
to cause widespread regressions.

Obvious questions:
1. Should phylink_get_caps() be augmented in this way, or should it be
    a separate method?

2. DSA has traditionally used "interface mode for the maximum supported
    speed on this port" where the interface mode is programmable (via
    its internal port_max_speed_mode() method) but this is only present
    for a few of the sub-drivers. Is reporting the current interface
    mode correct where this method is not implemented?

The second step is to introduce a function that allows phylink to be
reconfigured after creation time to operate at max-speed fixed-link
mode for the PHY interface mode, also using the MAC capabilities to
determine the speed and duplex mode we should be using.

Obvious questions:
1. Should we be allowing half-duplex for this?

Except for testing, not sure I do see a point as it should not be a 
configuration being used at all?

quoted

quoted

2. If we do allow half-duplex, should we prefer fastest speed over
    duplex setting, or should we prefer fastest full-duplex speed
    over any half-duplex?

I would opt for fastest speed over duplex setting.

quoted

quoted

3. How do we sanely switch DSA from its current behaviour to always
    using phylink for these ports without breakage - this is the
    difficult one, because it's not obvious which drivers have been
    coded to either work around this quirk of the DSA implementation.
    For example, if we start forcing the link down before calling
    dsa_port_phylink_create(), and we then fail to set max-fixed-link,
    then the CPU/DSA port is going to fail, and we're going to have
    lots of regressions.

Good question, we already have a legacy_pre_march2020 behavior for a 
piece of infrastructure code that is not so old, I doubt that we would 
want to add more of that type of quirk.
-- 
Florian