| Message ID | E1o6XAV-004pW2-Ct@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: always use phylink | expand |
On Wed, 29 Jun 2022 13:51:43 +0100 "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote: > Currently, we only use phylink for CPU and DSA ports if there is a > fixed-link specification, or a PHY specified. The reason for this > behaviour is that when neither is specified, there was no way for > phylink to know the link parameters. > > Now that we have phylink_set_max_link_speed() (which has become > possible through the addition of mac_capabilities) we now have the > ability to know the maximum link speed for a specific link, and can > now use phylink for this case as well. > > However, we need DSA drivers to report the interface mode being used > on these ports so that we can select a maximum speed appropriate for > the interface mode that hardware may have configured for the port. > > This is especially important with the conversion of DSA drivers to > phylink_pcs, as the PCS code only gets called if we are using > phylink for the port. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Marek Behún <kabel@kernel.org> So this is the one that may break other drivers? Marek
On Fri, Jul 01, 2022 at 09:34:35PM +0200, Marek Behún wrote: > On Wed, 29 Jun 2022 13:51:43 +0100 > "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote: > > > Currently, we only use phylink for CPU and DSA ports if there is a > > fixed-link specification, or a PHY specified. The reason for this > > behaviour is that when neither is specified, there was no way for > > phylink to know the link parameters. > > > > Now that we have phylink_set_max_link_speed() (which has become > > possible through the addition of mac_capabilities) we now have the > > ability to know the maximum link speed for a specific link, and can > > now use phylink for this case as well. > > > > However, we need DSA drivers to report the interface mode being used > > on these ports so that we can select a maximum speed appropriate for > > the interface mode that hardware may have configured for the port. > > > > This is especially important with the conversion of DSA drivers to > > phylink_pcs, as the PCS code only gets called if we are using > > phylink for the port. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Marek Behún <kabel@kernel.org> > > So this is the one that may break other drivers? It's the one that I'm most concerned about breakage happening, because the other drivers won't be setting the default interface for the port, which means if they're taking advantage of this "defaulting" feature, this patch will break them. The original code just did nothing when this "defaulting" feature was used - it didn't call the DSA phylink_mac_link_down() op, and didn't register with phylink. The phylink_mac_link_down() there was to ensure that the ports are in a link-down state prior to setting up phylink, because that's what phylink expects. The problem comes is that once we've called phylink_mac_link_down(), there isn't a way to back out of that without knowing the interface mode, speed, duplex etc - which is the problem with this defaulting feature, none of that is specified in DT, it can only come from the drivers. Note that the mv88e6xxx series I posted earlier depends on getting this problem sorted - if we don't, I can't send the mv88e6xxx pcs conversion because mv88e6xxx boards that _do_ make use of this defaulting feature will break (and there are a number that do make use of it.) If I send this RFC series (minus the top patch) then all the drivers that make use of this defualting feature that aren't mv88e6xxx will probably break - but I've no idea which make use of this feature because it's not documented. It's not really documented in the DT binding either, it's just something that Andrew "knows about", mv88e6xxx makes use of, and Andrew has suggested to some other DSA driver authors. This absolutely needs Andrew's involvement.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 1c6b4b00d58d..e19732782742 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3287,9 +3287,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) { struct device_node *phy_handle = NULL; struct dsa_switch *ds = chip->ds; - phy_interface_t mode; struct dsa_port *dp; - int tx_amp, speed; + int tx_amp; int err; u16 reg; @@ -3298,40 +3297,10 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) dp = dsa_to_port(ds, port); - /* MAC Forcing register: don't force link, speed, duplex or flow control - * state to any particular values on physical ports, but force the CPU - * port and all DSA ports to their maximum bandwidth and full duplex. - */ - if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) { - unsigned long caps = dp->pl_config.mac_capabilities; - - if (chip->info->ops->port_max_speed_mode) - mode = chip->info->ops->port_max_speed_mode(port); - else - mode = PHY_INTERFACE_MODE_NA; - - if (caps & MAC_10000FD) - speed = SPEED_10000; - else if (caps & MAC_5000FD) - speed = SPEED_5000; - else if (caps & MAC_2500FD) - speed = SPEED_2500; - else if (caps & MAC_1000) - speed = SPEED_1000; - else if (caps & MAC_100) - speed = SPEED_100; - else - speed = SPEED_10; - - err = mv88e6xxx_port_setup_mac(chip, port, LINK_FORCED_UP, - speed, DUPLEX_FULL, - PAUSE_OFF, mode); - } else { - err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED, - SPEED_UNFORCED, DUPLEX_UNFORCED, - PAUSE_ON, - PHY_INTERFACE_MODE_NA); - } + err = mv88e6xxx_port_setup_mac(chip, port, LINK_UNFORCED, + SPEED_UNFORCED, DUPLEX_UNFORCED, + PAUSE_ON, + PHY_INTERFACE_MODE_NA); if (err) return err; diff --git a/net/dsa/port.c b/net/dsa/port.c index 35b4e1f8dc05..34487e62eb03 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -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); } + 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); + } + 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) - ds->ops->phylink_mac_link_down(ds, port, - MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); - of_node_put(phy_np); - return dsa_port_phylink_register(dp); - } - of_node_put(phy_np); - return 0; + if (ds->ops->phylink_mac_link_down) + ds->ops->phylink_mac_link_down(ds, port, + MLO_AN_FIXED, PHY_INTERFACE_MODE_NA); + + return dsa_port_phylink_register(dp); } dev_warn(ds->dev,
Currently, we only use phylink for CPU and DSA ports if there is a fixed-link specification, or a PHY specified. The reason for this behaviour is that when neither is specified, there was no way for phylink to know the link parameters. Now that we have phylink_set_max_link_speed() (which has become possible through the addition of mac_capabilities) we now have the ability to know the maximum link speed for a specific link, and can now use phylink for this case as well. However, we need DSA drivers to report the interface mode being used on these ports so that we can select a maximum speed appropriate for the interface mode that hardware may have configured for the port. This is especially important with the conversion of DSA drivers to phylink_pcs, as the PCS code only gets called if we are using phylink for the port. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++---------------------------- net/dsa/port.c | 24 ++++++++++--------- 2 files changed, 18 insertions(+), 47 deletions(-)