net: dsa: always use phylink

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

Quoted 65 lines · Russell King (Oracle)

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

Please look at the patches and make suggestions on how we can proceed to clean up this quirk of DSA.

Signature · 2 lines

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

Report the maximum speed interface mode for the port, or if we don't have that information, the hardware configured interface mode for the port.

This allows phylink to know which interface mode CPU and DSA ports are operating, which will be necessary when we want to select the maximum speed for the port (required for such ports without a PHY or fixed-link specified in firmware.)

Signed-off-by Russell King (Oracle) <[email protected]>

Add a function for DSA to use to configure phylink, in the absence of any other configuration, to a fixed link operating at the maximum supported link speed.

This is needed so we can support phylink usage on CPU and DSA ports.

We use the default interface that the DSA driver provides (if any) otherwise we attempt to find the first supported interface that gives the maximum speed for the link.

Signed-off-by Russell King (Oracle) <[email protected]>

net/dsa/port.c +1 -0

index 35b4e1f8dc05..34487e62eb03 100644
@@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 	phy_interface_t mode, def_mode;
+	struct device_node *phy_np;
 	int err;
 
 	/* Presence of phylink_mac_link_state or phylink_mac_an_restart is

net/dsa/port.c +2 -4

+	}
+
 	return 0;
 }
 

@@ -1663,20 +1671,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 int dsa_port_link_register_of(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
-	struct device_node *phy_np;
 	int port = dp->index;
 
 	if (!ds->ops->adjust_link) {
-		phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
-		if (of_phy_is_fixed_link(dp->dn) || phy_np) {
-			if (ds->ops->phylink_mac_link_down)

net/dsa/port.c +2 -0

+
+		return dsa_port_phylink_register(dp);
 	}
 
 	dev_warn(ds->dev,

Signature · 1 line

2.30.2

Hello,

Quoted 16 lines · Russell King (Oracle)

net/dsa/port.c +1 -0

index 35b4e1f8dc05..34487e62eb03 100644
@@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 	phy_interface_t mode, def_mode;
+	struct device_node *phy_np;
 	int err;
 
 	/* Presence of phylink_mac_link_state or phylink_mac_an_restart is

@@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 		return PTR_ERR(dp->pl);
 	}
 

net/dsa/port.c int dsa_port_phylink_create(struct dsa_port *dp) · line 1567

Quoted 5 lines

net/dsa/port.c +5 -0

+	if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) {
+		phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+		of_node_put(phy_np);
+		if (!phy_np)
+			err = phylink_set_max_fixed_link(dp->pl);

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

Quoted 5 lines · Vladimir Oltean

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

phylink_set_max_fixed_link() already excludes itself:

        if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)
                return -EBUSY;

intentionally so that if there is anything specified for the port, be that a fixed link or in-band, then phylink_set_max_fixed_link() errors out with -EBUSY.

The only case that it can't detect is if there is a PHY that may be added to phylink at a later time, and that is what the check above is for.

Quoted 10 lines · Russell King (Oracle)

Quoted 5 lines · Vladimir Oltean

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

phylink_set_max_fixed_link() already excludes itself:

        if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)

Quoted 9 lines

                return -EBUSY;

intentionally so that if there is anything specified for the port, be that a fixed link or in-band, then phylink_set_max_fixed_link() errors out with -EBUSY.

The only case that it can't detect is if there is a PHY that may be added to phylink at a later time, and that is what the check above is for.

I've updated the function description to mention this detail:

+/**
+ * phylink_set_max_fixed_link() - set a fixed link configuration for phylink
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Set a maximum speed fixed-link configuration for the chosen interface mode
+ * and MAC capabilities for the phylink instance if the instance has not
+ * already been configured with a SFP, fixed link, or in-band AN mode. If the
+ * interface mode is PHY_INTERFACE_MODE_NA, then search the supported
+ * interfaces bitmap for the first interface that gives the fastest supported
+ * speed.

Does this address your concern?

Thanks.

Quoted 11 lines · Russell King (Oracle)

Quoted 10 lines · Russell King (Oracle)

Quoted 5 lines · Vladimir Oltean

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

phylink_set_max_fixed_link() already excludes itself:

        if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)

                                                      ~~~~~~~~~~

If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() that were initiated by the phylink user between phylink_create() and phylink_set_max_fixed_link(), correct? Those are specified as invalid in the kerneldoc and that's about it - that's not what the checking is for, correct?

Quoted 6 lines

Quoted 6 lines

                return -EBUSY;

intentionally so that if there is anything specified for the port, be that a fixed link or in-band, then phylink_set_max_fixed_link() errors out with -EBUSY.

Quoted 3 lines

Quoted 3 lines

The only case that it can't detect is if there is a PHY that may be added to phylink at a later time, and that is what the check above is for.

Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy() after phylink_set_max_fixed_link(), right?

So this is what I don't understand. If we've called phylink_set_max_fixed_link() we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA predicts if it's going to call either of those connect_phy() functions, and calls phylink_set_max_fixed_link() only if it won't. Right?

You've structured the checks in this "distributed" way because phylink can't really predict whether phylink_{,fwnode_}connect_phy() will be called after phylink_set_max_fixed_link(), right? I mean, it can probably predict the fwnode_ variant, but not phylink_connect_phy, and this is why it is up to the caller to decide when to call and when not to.

Quoted 14 lines

I've updated the function description to mention this detail:

+/**
+ * phylink_set_max_fixed_link() - set a fixed link configuration for phylink
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * Set a maximum speed fixed-link configuration for the chosen interface mode
+ * and MAC capabilities for the phylink instance if the instance has not
+ * already been configured with a SFP, fixed link, or in-band AN mode. If the
+ * interface mode is PHY_INTERFACE_MODE_NA, then search the supported
+ * interfaces bitmap for the first interface that gives the fastest supported
+ * speed.

Quoted 3 lines

Does this address your concern?

Thanks.

Not really, no, sorry, it just confuses me more.

It should maybe also
say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later.

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error

    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

Quoted 12 lines · Vladimir Oltean

Quoted 11 lines · Russell King (Oracle)

Quoted 10 lines · Russell King (Oracle)

Quoted 5 lines · Vladimir Oltean

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

phylink_set_max_fixed_link() already excludes itself:

        if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)

Quoted 8 lines

                                                      ~~~~~~~~~~

If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() that were initiated by the phylink user between phylink_create() and phylink_set_max_fixed_link(), correct? Those are specified as invalid in the kerneldoc and that's about it - that's not what the checking is for, correct?

No, it's not to do with sfps at all, but to do with enforcing the pre-conditions for the function - that entire line is checking that
(a) we are in a sane state to be called, and (b) there is no configuration initialisation beyond the default done by phylink_create() - in other words, there is no in-band or fixed-link specified.

Let's go through this step by step.

1. pl->cfg_link_an_mode != MLO_AN_PHY
The default value for cfg_link_an_mode is MLO_AN_PHY. If it's anything other than that, then a fixed-link or in-band mode has been specified, and we don't want to override that. So this call needs to fail.

2. pl->phydev
If a PHY has been attached, then the pre-condition for calling this function immediately after phylink_create() has been violated, because the only way it can be non-NULL is if someone's called one of the phylink functions that connects a PHY. Note: SFPs will not set their PHY here, because, for them to discover that there's a PHY, the network interface needs to be up, and it will never be up here... but in any case...

Quoted 11 lines

So this is what I don't understand. If we've called phylink_set_max_fixed_link() we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA predicts if it's going to call either of those connect_phy() functions, and calls phylink_set_max_fixed_link() only if it won't. Right?

You've structured the checks in this "distributed" way because phylink can't really predict whether phylink_{,fwnode_}connect_phy() will be called after phylink_set_max_fixed_link(), right? I mean, it can probably predict the fwnode_ variant, but not phylink_connect_phy, and this is why it is up to the caller to decide when to call and when not to.

phylink has no idea whether phylink_fwnode_connect_phy() will be called with the same fwnode as phylink_create(), so it really can't make any assumptions about whether there will be a PHY or not.

Quoted 3 lines

It should maybe also
say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later.

It's already a precondition that phylink_{,fwnode_}connect_phy() fail if we're in fixed-link mode (because PHYs have never been supported when in fixed-link mode - if one remembers, the old fixed-link code used to provide its own emulation of a PHY to make fixed-links work.) So PHYs and fixed-links have always been mutually exclusive before phylink, and continue to be so with phylink.

Quoted 6 lines

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error

Quoted 8 lines

    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this?

Quoted 7 lines · Russell King (Oracle)

Let's go through this step by step.

1. pl->cfg_link_an_mode != MLO_AN_PHY
The default value for cfg_link_an_mode is MLO_AN_PHY. If it's anything other than that, then a fixed-link or in-band mode has been specified, and we don't want to override that. So this call needs to fail.

Thanks for the explanation.

Yes, I noticed that phylink_set_max_fixed_link() relies on the fact that pl->cfg_link_an_mode has the unset value of 0, which coincidentally is MLO_AN_PHY.

Quoted 8 lines

2. pl->phydev
If a PHY has been attached, then the pre-condition for calling this function immediately after phylink_create() has been violated, because the only way it can be non-NULL is if someone's called one of the phylink functions that connects a PHY. Note: SFPs will not set their PHY here, because, for them to discover that there's a PHY, the network interface needs to be up, and it will never be up here... but in any case...

Ok, so this does check for a precondition that the caller did something correctly. But it doesn't (and can't) check that all preconditions and postconditions are satisfied. That's one of my irks, why bother checking the easy to satisfy precondition (which depends on the code organization, static information, easy to check), and give up on the hard one (which depends on the device tree blob, dynamic information, not so easy).

Quoted 12 lines

Quoted 11 lines

So this is what I don't understand. If we've called phylink_set_max_fixed_link() we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA predicts if it's going to call either of those connect_phy() functions, and calls phylink_set_max_fixed_link() only if it won't. Right?

You've structured the checks in this "distributed" way because phylink can't really predict whether phylink_{,fwnode_}connect_phy() will be called after phylink_set_max_fixed_link(), right? I mean, it can probably predict the fwnode_ variant, but not phylink_connect_phy, and this is why it is up to the caller to decide when to call and when not to.

Quoted 3 lines

phylink has no idea whether phylink_fwnode_connect_phy() will be called with the same fwnode as phylink_create(), so it really can't make any assumptions about whether there will be a PHY or not.

This is interesting. Is there a use case for passing a different fwnode_handle to the 2 functions?

Quoted 10 lines

Quoted 3 lines

It should maybe also
say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later.

It's already a precondition that phylink_{,fwnode_}connect_phy() fail if we're in fixed-link mode (because PHYs have never been supported when in fixed-link mode - if one remembers, the old fixed-link code used to provide its own emulation of a PHY to make fixed-links work.) So PHYs and fixed-links have always been mutually exclusive before phylink, and continue to be so with phylink.

Define "fail" exactly, because if I look in phylink_fwnode_phy_connect(), I see:

	/* Fixed links and 802.3z are handled without needing a PHY */
	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
	     phy_interface_mode_is_8023z(pl->link_interface)))
		return 0; <- does this count as failure?

This is why dsa_port_phylink_register() calls phylink_of_phy_connect() without checking whether it has a fixed-link or a PHY, because it doesn't fail even if it doesn't do anything.

In fact I've wanted to make a correction to my previous phrasing that "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later". The correction is "... with a phy-handle".

Quoted 15 lines

Quoted 6 lines

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error

Quoted 8 lines

    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

Quoted 3 lines

My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this?

Because the end result will be simpler if we respect the separation of concerns that continues to exist, and it's still phylink's business to say what is and isn't valid. DSA still isn't aware of the bindings required by phylink, it just passes its fwnode to it. Practically speaking, I wouldn't be scratching my head as to why we're checking for half the prerequisites of phylink_set_max_fixed_link() in one place and for the other half in another.

True, through this patch set DSA is creating its own context specific extension of phylink bindings, but arguably those existed before DSA was even integrated with phylink, and we're just fixing something now we didn't realize at the time we'd need to do.

I can reverse the question, why would phylink even want to be involved in how the max fixed link parameters are deduced, and it doesn't just require that a fixed-link software node is constructed somehow (irrelevant to phylink how), and phylink is just modified to find and work with that if it exists? Isn't it for the exact same reason, separation of concerns, that it's easiest for phylink to figure out what is the most appropriate maximum fixed-link configuration?

Quoted 4 lines

This is why dsa_port_phylink_register() calls phylink_of_phy_connect() without checking whether it has a fixed-link or a PHY, because it doesn't fail even if it doesn't do anything.

Quoted 3 lines

In fact I've wanted to make a correction to my previous phrasing that "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later". The correction is "... with a phy-handle".

I'm not sure that clarification makes sense when talking about phylink_connect_phy(), so I think if you're clarifying it with a firmware property, you're only talking about phylink_fwnode_connect_phy() now?

Quoted 32 lines

Quoted 15 lines

Quoted 6 lines

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error

Quoted 8 lines

    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

Quoted 3 lines

My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this?

Because the end result will be simpler if we respect the separation of concerns that continues to exist, and it's still phylink's business to say what is and isn't valid. DSA still isn't aware of the bindings required by phylink, it just passes its fwnode to it. Practically speaking, I wouldn't be scratching my head as to why we're checking for half the prerequisites of phylink_set_max_fixed_link() in one place and for the other half in another.

True, through this patch set DSA is creating its own context specific extension of phylink bindings, but arguably those existed before DSA was even integrated with phylink, and we're just fixing something now we didn't realize at the time we'd need to do.

Quoted 7 lines

I can reverse the question, why would phylink even want to be involved in how the max fixed link parameters are deduced, and it doesn't just require that a fixed-link software node is constructed somehow (irrelevant to phylink how), and phylink is just modified to find and work with that if it exists? Isn't it for the exact same reason, separation of concerns, that it's easiest for phylink to figure out what is the most appropriate maximum fixed-link configuration?

If that could be done, I'd love it, because then we don't have this in phylink at all, and it can all be a DSA problem to solve. It also means that others won't be tempted to use the interface incorrectly.

I'm not sure how practical that is when we have both DT and ACPI to deal with, and ACPI is certainly out of my knowledge area to be able to construct a software node to specify a fixed-link. Maybe it can be done at the fwnode layer? I don't know.

Do you have a handy example of what you're suggesting?

Quoted 4 lines · Russell King (Oracle)

Quoted 4 lines

This is why dsa_port_phylink_register() calls phylink_of_phy_connect() without checking whether it has a fixed-link or a PHY, because it doesn't fail even if it doesn't do anything.

Quoted 8 lines

Quoted 3 lines

In fact I've wanted to make a correction to my previous phrasing that "this function shouldn't be called if phylink_{,fwnode_}connect_phy() is going to be called later". The correction is "... with a phy-handle".

I'm not sure that clarification makes sense when talking about phylink_connect_phy(), so I think if you're clarifying it with a firmware property, you're only talking about phylink_fwnode_connect_phy() now?

Yes, it's super hard to verbalize, and this is the reason why I didn't add "... with a phy-handle" in the first place.

I wanted to say: phylink_connect_phy(), OR phylink_fwnode_connect_phy() WITH a phy-handle. I shouldn't have conflated them in the first place.

Quoted 40 lines

Quoted 32 lines

Quoted 15 lines

Quoted 6 lines

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error

Quoted 8 lines

    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}

Quoted 3 lines

My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this?

Because the end result will be simpler if we respect the separation of concerns that continues to exist, and it's still phylink's business to say what is and isn't valid. DSA still isn't aware of the bindings required by phylink, it just passes its fwnode to it. Practically speaking, I wouldn't be scratching my head as to why we're checking for half the prerequisites of phylink_set_max_fixed_link() in one place and for the other half in another.

True, through this patch set DSA is creating its own context specific extension of phylink bindings, but arguably those existed before DSA was even integrated with phylink, and we're just fixing something now we didn't realize at the time we'd need to do.

Quoted 7 lines

I can reverse the question, why would phylink even want to be involved in how the max fixed link parameters are deduced, and it doesn't just require that a fixed-link software node is constructed somehow (irrelevant to phylink how), and phylink is just modified to find and work with that if it exists? Isn't it for the exact same reason, separation of concerns, that it's easiest for phylink to figure out what is the most appropriate maximum fixed-link configuration?

Quoted 8 lines

If that could be done, I'd love it, because then we don't have this in phylink at all, and it can all be a DSA problem to solve. It also means that others won't be tempted to use the interface incorrectly.

I'm not sure how practical that is when we have both DT and ACPI to deal with, and ACPI is certainly out of my knowledge area to be able to construct a software node to specify a fixed-link. Maybe it can be done at the fwnode layer? I don't know.

I don't want to be misunderstood. I brought up software nodes because I'm sure you must have thought about this too, before proposing what you did here. And unless there's a technical reason against software nodes (which there doesn't appear to be, but I don't want to get ahead of myself), I figured you must be OK with phylink absorbing the logic, case in which I just don't understand why you are pushing back on a proposal how to make phylink absorb the logic completely.

Quoted 1 line

Do you have a handy example of what you're suggesting?

No, I didn't, but I thought, how hard can it be, and here's a hacked up attempt on one of my boards:

[    4.315754] sja1105 spi0.1: configuring for fixed/rgmii link mode
[    4.322653] sja1105 spi0.1 swp5 (uninitialized): PHY [mdio@2d24000:06] driver [Broadcom BCM5464] (irq=POLL)
[    4.334796] sja1105 spi0.1 swp2 (uninitialized): PHY [mdio@2d24000:03] driver [Broadcom BCM5464] (irq=POLL)
[    4.345853] sja1105 spi0.1 swp3 (uninitialized): PHY [mdio@2d24000:04] driver [Broadcom BCM5464] (irq=POLL)
[    4.356859] sja1105 spi0.1 swp4 (uninitialized): PHY [mdio@2d24000:05] driver [Broadcom BCM5464] (irq=POLL)
[    4.367245] device eth2 entered promiscuous mode
[    4.371864] DSA: tree 0 setup
[    4.376971] sja1105 spi0.1: Link is Up - 1Gbps/Full - flow control off
(...)
root@black:~# ip link set swp2 up && dhclient -i swp2 && ip addr show swp2
[   64.762756] fsl-gianfar soc:ethernet@2d90000 eth2: Link is Up - 1Gbps/Full - flow control off
[   64.771530] sja1105 spi0.1 swp2: configuring for phy/rgmii-id link mode
[   68.955048] sja1105 spi0.1 swp2: Link is Up - 1Gbps/Full - flow control off
12: swp2@eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 00:1f:7b:63:02:48 brd ff:ff:ff:ff:ff:ff
    inet 10.0.0.68/24 brd 10.0.0.255 scope global dynamic swp2
       valid_lft 600sec preferred_lft 600sec

It's by far the messiest patch I've posted to the list (in the interest of responding quickly), but if you study the code you can obviously see what's missing, basically I've hardcoded the speed to 1000 and I'm copying the phy-mode from the real DT node.

Unfortunately I don't have the time (and most importantly the interest) in pushing this any further than that. If you want to take this from here and integrate it with phylink_get_caps() I'd be glad to review the result. Otherwise, feel free to continue with phylink_set_max_fixed_link().

Quoted 5 lines

Quoted 4 lines

I'm not sure how practical that is when we have both DT and ACPI to deal with, and ACPI is certainly out of my knowledge area to be able to construct a software node to specify a fixed-link. Maybe it can be done at the fwnode layer? I don't know.

Quoted 7 lines

I don't want to be misunderstood. I brought up software nodes because I'm sure you must have thought about this too, before proposing what you did here. And unless there's a technical reason against software nodes (which there doesn't appear to be, but I don't want to get ahead of myself), I figured you must be OK with phylink absorbing the logic, case in which I just don't understand why you are pushing back on a proposal how to make phylink absorb the logic completely.

The reason I hadn't is because switching DSA to fwnode brings with it issues for ACPI, and Andrew wants to be very careful about ACPI in networking - and I think quite rightly. As soon as one switches from DT APIs to fwnode APIs, you basically permit people an easy path to re-use DT properties in ACPI-land without the ACPI issues being first considered.
Yep - there's at least one other property we need to carry over from the DT node, which is the "ethernet" property.

+static int dsa_port_fixup_broken_dt(struct dsa_port *dp)

net/dsa/port.c +11 -0

+{
+	struct property_entry fixed_link_props[] = {
+		PROPERTY_ENTRY_BOOL("full-duplex"),
+		PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */
+		{},
+	};
+	struct property_entry port_props[3] = {};
+	struct fwnode_handle *fixed_link_fwnode;
+	struct fwnode_handle *new_port_fwnode;
+	struct device_node *dn = dp->dn;
+	phy_interface_t mode;

Hi,

Quoted 1 line · Vladimir Oltean

+static int dsa_port_fixup_broken_dt(struct dsa_port *dp)

As I mentioned, I doubt that Andrew considers this "broken DT" as he's been promoting this as a standard DSA feature.

Quoted 11 lines

net/dsa/port.c +11 -0

+{
+	struct property_entry fixed_link_props[] = {
+		PROPERTY_ENTRY_BOOL("full-duplex"),
+		PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */
+		{},
+	};
+	struct property_entry port_props[3] = {};
+	struct fwnode_handle *fixed_link_fwnode;
+	struct fwnode_handle *new_port_fwnode;
+	struct device_node *dn = dp->dn;
+	phy_interface_t mode;

net/dsa/port.c static int dsa_port_phylink_register(struct dsa_port *dp) · line 1675

Quoted 10 lines

net/dsa/port.c +10 -0

+	int err;
+
+	if (of_parse_phandle(dn, "phy-handle", 0) ||
+	    of_phy_is_fixed_link(dn))
+		/* Nothing broken, nothing to fix.
+		 * TODO: As discussed with Russell, maybe phylink could provide
+		 * a more comprehensive helper to determine what constitutes a
+		 * valid fwnode binding than this guerilla kludge.
+		 */
+		return 0;  

I think this is sufficient. Yes, phylink accepts "phy" and "phy-device" because it has to for compatibility with other drivers, but the binding document for DSA quite clearly states that "phy-handle" is what DSA accepts, so DT in the kernel will be validated against the yaml file and enforce correctness here.

We do need to check for "sfp" being present as well.

net/dsa/port.c static int dsa_port_phylink_register(struct dsa_port *dp) · line 1682

Quoted 7 lines

net/dsa/port.c +7 -0

+
+	err = of_get_phy_mode(dn, &mode);
+	if (err)
+		/* TODO this may be missing too, ask the driver for the
+		 * max-speed interface mode for this port
+		 */
+		mode = PHY_INTERFACE_MODE_NA;  

I think it would be easier to omit the phy-mode property in the swnode if it isn't present in DT, because then we can handle that in dsa_port_phylink_create() as I've done in my patch series via the ds->ops->phylink_get_caps() method.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	port_props[0] = PROPERTY_ENTRY_U32("reg", dp->index);  

You said in one of your other replies that this node we're constructing is only for phylink, do we need the "reg" property? phylink doesn't care about it.

Hello,

Quoted 16 lines · Russell King (Oracle)

net/dsa/port.c +1 -0

index 35b4e1f8dc05..34487e62eb03 100644
@@ -1525,6 +1525,7 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
 	phy_interface_t mode, def_mode;
+	struct device_node *phy_np;
 	int err;
 
 	/* Presence of phylink_mac_link_state or phylink_mac_an_restart is

@@ -1559,6 +1560,13 @@ int dsa_port_phylink_create(struct dsa_port *dp)
 		return PTR_ERR(dp->pl);
 	}
 

net/dsa/port.c int dsa_port_phylink_create(struct dsa_port *dp) · line 1567

Quoted 5 lines

net/dsa/port.c +5 -0

+	if (dp->type == DSA_PORT_TYPE_CPU || dp->type == DSA_PORT_TYPE_DSA) {
+		phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+		of_node_put(phy_np);
+		if (!phy_np)
+			err = phylink_set_max_fixed_link(dp->pl);

Can we please limit phylink_set_max_link_speed() to just the CPU ports where a fixed-link property is also missing, not just a phy-handle? Although to be entirely correct, we can also have MLO_AN_INBAND, which wouldn't be covered by these 2 checks and would still represent a valid DT binding.

More importantly, we need your input on Ocelot, which you are listed as a maintainer for, and Ocelot is the only DSA driver that does stuff differently (due to the rate adapting PCS). It doesn't set mac_capabilities, and therefore phylink_set_max_fixed_link() will not work here.

Has Ocelot ever made use of this DSA feature where, when nothing is specified for a CPU or DSA port, we use an effective fixed-link setup with an interface mode that gives the highest speed? Or does this not apply to this DSA driver?

Thanks.

Quoted 6 lines · Russell King (Oracle)

More importantly, we need your input on Ocelot, which you are listed as a maintainer for, and Ocelot is the only DSA driver that does stuff differently (due to the rate adapting PCS). It doesn't set mac_capabilities, and therefore phylink_set_max_fixed_link() will not work here.

Quoted 6 lines

Has Ocelot ever made use of this DSA feature where, when nothing is specified for a CPU or DSA port, we use an effective fixed-link setup with an interface mode that gives the highest speed? Or does this not apply to this DSA driver?

Thanks.

I'm fine with both the ocelot and sja1105 drivers.

The ocelot driver has 3 users:

- felix_vsc9959 (arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi) on NXP LS1028A, where the CPU ports have and have always had a fixed-link node in the SoC dtsi. LS1028A based boards should include the SoC dtsi. If other board DT writers don't do that or if they delete the fixed-link node from the CPU ports, that's not my problem and I don't really want to help them.

- seville_vsc9953 (arch/powerpc/boot/dts/fsl/t1040si-post.dtsi) on NXP T1040. Same thing, embedded switch, not my fault if the fixed-link disappears from the SoC dtsi.

- Colin Foster's SPI-controlled VSC7512 (still downstream). He has an Ethernet cable connecting the CPU port to a Beaglebone Black, so he has a phy-handle on the CPU port, so definitely not nothing. I believe his work hasn't made it to production in any case, so enforcing validation now shouldn't bother him too much if at all.

As for sja1105, there is DT validation that checks for the presence of all required properties in sja1105_parse_ports_node().

There is some DT validation in felix_parse_ports_node() too, but it doesn't check that all specifiers that phylink might use are there.

I'd really like to add some validation before I gain any involuntary users, but all open-coded constructs I can come up with are clumsy. What would you suggest, if I explicitly don't want to rely on context-specific phylink interpretation of empty OF nodes, and rather error out?

Great, so I'll mark ocelot is safe.

Quoted 2 lines

As for sja1105, there is DT validation that checks for the presence of all required properties in sja1105_parse_ports_node().

Looking at those, it requires all of:

- a phy mode to be specified (as determined by of_get_phy_mode())
- a phy-handle or of_phy_is_fixed_link() to return true

otherwise it errors out.

Quoted 2 lines

There is some DT validation in felix_parse_ports_node() too, but it doesn't check that all specifiers that phylink might use are there.

Phylink (correction, fwnode_get_phy_node() which is not part of phylink anymore) will look for phy-handle, phy, or phy-device. This is I don't see that there's any incompatibility between what the driver is doing and what phylink does.

If there's a fixed-link property, then sja1105_parse_ports_node() is happy, and so will phylink. If there's a phy-handle, the same is true. If there's a "phy" or "phy-device" then sja1105_parse_ports_node() errors out. That's completely fine.

"phy" and "phy-device" are the backwards compatibility for DT - I believe one of them is the ePAPR specified property that we in Linux have decided to only fall back on if there's not our more modern "phy-handle" property.

It seems We have a lot of users of "phy" in DT today, so we can't drop that from generic code such as phylink, but I haven't found any users of "phy-device".

Quoted 5 lines

I'd really like to add some validation before I gain any involuntary users, but all open-coded constructs I can come up with are clumsy. What would you suggest, if I explicitly don't want to rely on context-specific phylink interpretation of empty OF nodes, and rather error out?

So I also don't see a problem - sja1105 rejects DTs that fail to describe a port using at least one of a phy-handle, a fixed-link, or a managed in-band link, and I don't think it needs to do further validation, certainly not for the phy describing properties that the kernel has chosen to deprecate for new implementations.

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

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

Quoted 83 lines · Russell King (Oracle)

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

Quoted 51 lines · Russell King (Oracle)

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?

Quoted 3 lines

Quoted 3 lines

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?

Quoted 8 lines

Quoted 8 lines

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.

Quoted 10 lines · Russell King (Oracle)

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.

Quoted 1 line

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.

Hi Florian,

Quoted 11 lines · Florian Fainelli

Quoted 10 lines · Russell King (Oracle)

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.

Quoted 5 lines

Quoted 1 line

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.

What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts?

Quoted 16 lines · Vladimir Oltean

Hi Florian,

Quoted 13 lines · Florian Fainelli

Quoted 11 lines · Russell King (Oracle)

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

Quoted 5 lines

Quoted 3 lines

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.

What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts?

You found one of the devices that I do not have access to and did not test, thanks. We do expect to run the port at 2GBits/sec on these devices however there is no "official" way to advertise that a port can run at 2Gbits/sec, as this is not even a "sanctioned" speed. I do have a similar device however, so let me run some more tests, we won't see a regression however since we do not use the NATP accelerator which would be the reason to run the port at 2Gbits/sec.

Quoted 25 lines · Florian Fainelli

Quoted 16 lines · Vladimir Oltean

Hi Florian,

Quoted 13 lines · Florian Fainelli

Quoted 11 lines · Russell King (Oracle)

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

Quoted 7 lines

Quoted 5 lines

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.

What about arch/arm/boot/dts/bcm47189-tenda-ac9.dts?

Quoted 7 lines

You found one of the devices that I do not have access to and did not test, thanks. We do expect to run the port at 2GBits/sec on these devices however there is no "official" way to advertise that a port can run at 2Gbits/sec, as this is not even a "sanctioned" speed. I do have a similar device however, so let me run some more tests, we won't see a regression however since we do not use the NATP accelerator which would be the reason to run the port at 2Gbits/sec.

I will try this change on some devices with the lantiq gswip driver at the weekend.

On the SoC supported by the lantiq gswip driver the switch is integrated in the SoC and there is a internal link with more than 1GBit/s connecting the switch to the rest of the system. I think it is also around 2GBit/s. We can not configure the interface speed or many other interface settings for the link between the switch and the CPU. How should the device tree ideally look for this setup?

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

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

Pulled in the patch set on top of net-next and tested on the D-Link DIR-685 with the RTL8366RB switch with no regressions, so:

Tested-by Linus Walleij <[email protected]>

The plan is to enhance the phylink handling in the RTL8366RB on top of Russell's patch in some time.