| Message ID | E1mpwRs-00D8LK-N3@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Allow DSA drivers to set all phylink capabilities | expand |
On 11/24/21 9:52 AM, Russell King (Oracle) wrote: > Populate the supported interfaces and MAC capabilities for the bcm_sf2 > DSA switch and remove the old validate implementation to allow DSA to > use phylink_generic_validate() for this switch driver. > > The exclusion of Gigabit linkmodes for MII and Reverse MII links is > handled within phylink_generic_validate() in phylink, so there is no > need to make them conditional on the interface mode in the driver. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Tested-by: Florian Fainelli <f.fainelli@gmail.com> but it looks like the fixed link ports are reporting some pretty strange advertisement values one of my two platforms running the same kernel image: # ethtool rgmii_2 Settings for rgmii_2: Supported ports: [ MII ] Supported link modes: 1000baseKX/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 1000baseKX/Full Advertised pause frame use: Symmetric Receive-only Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 1000baseKX/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: on Port: MII PHYAD: 0 Transceiver: internal Supports Wake-on: gsf Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: yes # These should be 1000BaseT/Full since these are RGMII fixed links: # ethtool rgmii_2 Settings for rgmii_2: Supported ports: [ MII ] Supported link modes: 1000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 1000baseT/Full Advertised pause frame use: Symmetric Receive-only Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 1000baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Auto-negotiation: on Port: MII PHYAD: 0 Transceiver: internal Supports Wake-on: gsf Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: yes # There is no problem with Linus' master branch at net-5.16-rc4-173-ga51e3ac43ddb, let me see if I can bisect this and/or fix it in the next days. Thanks!
On 12/3/21 12:03 PM, Florian Fainelli wrote: > On 11/24/21 9:52 AM, Russell King (Oracle) wrote: >> Populate the supported interfaces and MAC capabilities for the bcm_sf2 >> DSA switch and remove the old validate implementation to allow DSA to >> use phylink_generic_validate() for this switch driver. >> >> The exclusion of Gigabit linkmodes for MII and Reverse MII links is >> handled within phylink_generic_validate() in phylink, so there is no >> need to make them conditional on the interface mode in the driver. >> >> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Tested-by: Florian Fainelli <f.fainelli@gmail.com> > > but it looks like the fixed link ports are reporting some pretty strange > advertisement values one of my two platforms running the same kernel image: We would want to amend your patch with something that caters a bit more towards how the ports have been configured: diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index d6ef0fb0d943..88933c3feddd 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port) static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, struct phylink_config *config) { - __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces); - __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces); - __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); - __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); - __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces); - phy_interface_set_rgmii(config->supported_interfaces); + struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); + + if (priv->int_phy_mask & BIT(port)) + __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); + else if (priv->moca_port == port) + __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces); + else { + __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); + phy_interface_set_rgmii(config->supported_interfaces); + } config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000; Now, with respect to the fixed link ports reporting 1000baseKX/Full this is introduced by switching to your patch, it works before and it "breaks" after. The first part that is a bit weird is that we seem to be calling phylink_generic_validate() twice in a row from the same call site. For fixed link ports, instead of masking with what the fixed link actually supports, we seem to be using a supported mask which is all 1s which seems a bit excessive for a fixed link. This is an excerpt with the internal PHY: [ 4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): Calling phylink_generic_validate [ 4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0 [ 4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities: 0xff [ 4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff [ 4.239463] before anding supported with mask: 0000000,00000000,000062ff [ 4.246189] after anding supported with mask: 0000000,00000000,000062ff [ 4.252829] before anding advertising with mask: c000018,00000200,00036fff [ 4.259729] after anding advertising with mask: c000018,00000200,00036fff [ 4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL) and this is what a fixed link port looks like: [ 4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized): Calling phylink_generic_validate [ 4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0 [ 4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff [ 4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff [ 4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff [ 4.465811] after anding supported with mask: c000018,00000200,00036fff [ 4.472450] before anding advertising with mask: c000018,00000200,00036fff [ 4.479349] after anding advertising with mask: c000018,00000200,00036fff or maybe the problem is with phylink_get_ksettings... ran out of time tonight to look further into it.
On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > On 12/3/21 12:03 PM, Florian Fainelli wrote: > > On 11/24/21 9:52 AM, Russell King (Oracle) wrote: > > > Populate the supported interfaces and MAC capabilities for the bcm_sf2 > > > DSA switch and remove the old validate implementation to allow DSA to > > > use phylink_generic_validate() for this switch driver. > > > > > > The exclusion of Gigabit linkmodes for MII and Reverse MII links is > > > handled within phylink_generic_validate() in phylink, so there is no > > > need to make them conditional on the interface mode in the driver. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > Tested-by: Florian Fainelli <f.fainelli@gmail.com> > > > > but it looks like the fixed link ports are reporting some pretty strange > > advertisement values one of my two platforms running the same kernel image: > > We would want to amend your patch with something that caters a bit more > towards how the ports have been configured: > > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c > index d6ef0fb0d943..88933c3feddd 100644 > --- a/drivers/net/dsa/bcm_sf2.c > +++ b/drivers/net/dsa/bcm_sf2.c > @@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct > dsa_switch *ds, int port) > static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, > struct phylink_config *config) > { > - __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces); > - __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces); > - __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); > - __set_bit(PHY_INTERFACE_MODE_INTERNAL, > config->supported_interfaces); > - __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces); > - phy_interface_set_rgmii(config->supported_interfaces); > + struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); > + > + if (priv->int_phy_mask & BIT(port)) > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, > config->supported_interfaces); > + else if (priv->moca_port == port) > + __set_bit(PHY_INTERFACE_MODE_MOCA, > config->supported_interfaces); > + else { > + __set_bit(PHY_INTERFACE_MODE_MII, > config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_REVMII, > config->supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_GMII, > config->supported_interfaces); > + phy_interface_set_rgmii(config->supported_interfaces); > + } > > config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > MAC_10 | MAC_100 | MAC_1000; That's fine, thanks for the update. > Now, with respect to the fixed link ports reporting 1000baseKX/Full this is > introduced by switching to your patch, it works before and it "breaks" > after. > > The first part that is a bit weird is that we seem to be calling > phylink_generic_validate() twice in a row from the same call site. > > For fixed link ports, instead of masking with what the fixed link actually > supports, we seem to be using a supported mask which is all 1s which seems a > bit excessive for a fixed link. > > This is an excerpt with the internal PHY: > > [ 4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): > Calling phylink_generic_validate > [ 4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0 > [ 4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities: > 0xff > [ 4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff > [ 4.239463] before anding supported with mask: 0000000,00000000,000062ff > [ 4.246189] after anding supported with mask: 0000000,00000000,000062ff > [ 4.252829] before anding advertising with mask: > c000018,00000200,00036fff > [ 4.259729] after anding advertising with mask: c000018,00000200,00036fff > [ 4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY > [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL) > > and this is what a fixed link port looks like: > > [ 4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized): > Calling phylink_generic_validate > [ 4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0 > [ 4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff > [ 4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff > [ 4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff > [ 4.465811] after anding supported with mask: c000018,00000200,00036fff > [ 4.472450] before anding advertising with mask: > c000018,00000200,00036fff > [ 4.479349] after anding advertising with mask: c000018,00000200,00036fff > > or maybe the problem is with phylink_get_ksettings... ran out of time > tonight to look further into it. It will be: s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex, pl->supported, true); linkmode_zero(pl->supported); phylink_set(pl->supported, MII); phylink_set(pl->supported, Pause); phylink_set(pl->supported, Asym_Pause); phylink_set(pl->supported, Autoneg); if (s) { __set_bit(s->bit, pl->supported); __set_bit(s->bit, pl->link_config.lp_advertising); Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting() returns the first entry it finds in the supported table: /* 1G */ PHY_SETTING( 1000, FULL, 1000baseKX_Full ), PHY_SETTING( 1000, FULL, 1000baseT_Full ), PHY_SETTING( 1000, HALF, 1000baseT_Half ), PHY_SETTING( 1000, FULL, 1000baseT1_Full ), PHY_SETTING( 1000, FULL, 1000baseX_Full ), Consequently, 1000baseKX_Full is preferred over 1000baseT_Full. Fixed links don't specify their underlying technology, only the speed and duplex, so going from speed and duplex to an ethtool link mode is not easy. I suppose we could drop 1000baseKX_Full from the supported bitmap in phylink_parse_fixedlink() before the first phylink_validate() call. Alternatively, the table could be re-ordered. It was supposed to be grouped by speed and sorted in descending match priority as specified by the comment above the table. Does it really make sense that 1000baseKX_Full is supposed to be preferred over all the other 1G speeds? I suppose that's a question for Tom Lendacky <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80 ("phy: Expand phy speed/duplex settings array") back in 2014.
On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > > On 12/3/21 12:03 PM, Florian Fainelli wrote: > > > On 11/24/21 9:52 AM, Russell King (Oracle) wrote: > > > > Populate the supported interfaces and MAC capabilities for the bcm_sf2 > > > > DSA switch and remove the old validate implementation to allow DSA to > > > > use phylink_generic_validate() for this switch driver. > > > > > > > > The exclusion of Gigabit linkmodes for MII and Reverse MII links is > > > > handled within phylink_generic_validate() in phylink, so there is no > > > > need to make them conditional on the interface mode in the driver. > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > Tested-by: Florian Fainelli <f.fainelli@gmail.com> > > > > > > but it looks like the fixed link ports are reporting some pretty strange > > > advertisement values one of my two platforms running the same kernel image: > > > > We would want to amend your patch with something that caters a bit more > > towards how the ports have been configured: > > > > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c > > index d6ef0fb0d943..88933c3feddd 100644 > > --- a/drivers/net/dsa/bcm_sf2.c > > +++ b/drivers/net/dsa/bcm_sf2.c > > @@ -675,12 +675,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct > > dsa_switch *ds, int port) > > static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, > > struct phylink_config *config) > > { > > - __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces); > > - __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces); > > - __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); > > - __set_bit(PHY_INTERFACE_MODE_INTERNAL, > > config->supported_interfaces); > > - __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces); > > - phy_interface_set_rgmii(config->supported_interfaces); > > + struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); > > + > > + if (priv->int_phy_mask & BIT(port)) > > + __set_bit(PHY_INTERFACE_MODE_INTERNAL, > > config->supported_interfaces); > > + else if (priv->moca_port == port) > > + __set_bit(PHY_INTERFACE_MODE_MOCA, > > config->supported_interfaces); > > + else { > > + __set_bit(PHY_INTERFACE_MODE_MII, > > config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_REVMII, > > config->supported_interfaces); > > + __set_bit(PHY_INTERFACE_MODE_GMII, > > config->supported_interfaces); > > + phy_interface_set_rgmii(config->supported_interfaces); > > + } > > > > config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > > MAC_10 | MAC_100 | MAC_1000; > > That's fine, thanks for the update. Here's the resulting updated patch. I've changed it slightly to avoid the wrapping, and updated the commit text - please let me know if you'd like any attributations added. Thanks! 8<=== From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> Subject: [PATCH RFC v2 net-next] net: dsa: bcm_sf2: convert to phylink_generic_validate() Populate the supported interfaces and MAC capabilities for the bcm_sf2 DSA switch and remove the old validate implementation to allow DSA to use phylink_generic_validate() for this switch driver. The exclusion of Gigabit linkmodes for MII and Reverse MII links is handled within phylink_generic_validate() in phylink, so there is no need to make them conditional on the interface mode in the driver. Thanks to Florian Fainelli for suggesting how to populate the supported interfaces. Link: https://lore.kernel.org/r/3b3fed98-0c82-99e9-dc72-09fe01c2bcf3@gmail.com Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/bcm_sf2.c | 54 +++++++++++---------------------------- 1 file changed, 15 insertions(+), 39 deletions(-) diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 13aa43b5cffd..114d4ba7716f 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -672,49 +672,25 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port) PHY_BRCM_IDDQ_SUSPEND; } -static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port, - unsigned long *supported, - struct phylink_link_state *state) +static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) { + unsigned long *interfaces = config->supported_interfaces; struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; - - if (!phy_interface_mode_is_rgmii(state->interface) && - state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_REVMII && - state->interface != PHY_INTERFACE_MODE_GMII && - state->interface != PHY_INTERFACE_MODE_INTERNAL && - state->interface != PHY_INTERFACE_MODE_MOCA) { - linkmode_zero(supported); - if (port != core_readl(priv, CORE_IMP0_PRT_ID)) - dev_err(ds->dev, - "Unsupported interface: %d for port %d\n", - state->interface, port); - return; - } - - /* Allow all the expected bits */ - phylink_set(mask, Autoneg); - phylink_set_port_modes(mask); - phylink_set(mask, Pause); - phylink_set(mask, Asym_Pause); - /* With the exclusion of MII and Reverse MII, we support Gigabit, - * including Half duplex - */ - if (state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_REVMII) { - phylink_set(mask, 1000baseT_Full); - phylink_set(mask, 1000baseT_Half); + if (priv->int_phy_mask & BIT(port)) { + __set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces); + } else if (priv->moca_port == port) { + __set_bit(PHY_INTERFACE_MODE_MOCA, interfaces); + } else { + __set_bit(PHY_INTERFACE_MODE_MII, interfaces); + __set_bit(PHY_INTERFACE_MODE_REVMII, interfaces); + __set_bit(PHY_INTERFACE_MODE_GMII, interfaces); + phy_interface_set_rgmii(interfaces); } - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 100baseT_Full); - - linkmode_and(supported, supported, mask); - linkmode_and(state->advertising, state->advertising, mask); + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; } static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port, @@ -1181,7 +1157,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = { .get_sset_count = bcm_sf2_sw_get_sset_count, .get_ethtool_phy_stats = b53_get_ethtool_phy_stats, .get_phy_flags = bcm_sf2_sw_get_phy_flags, - .phylink_validate = bcm_sf2_sw_validate, + .phylink_get_caps = bcm_sf2_sw_get_caps, .phylink_mac_config = bcm_sf2_sw_mac_config, .phylink_mac_link_down = bcm_sf2_sw_mac_link_down, .phylink_mac_link_up = bcm_sf2_sw_mac_link_up,
On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > > Now, with respect to the fixed link ports reporting 1000baseKX/Full this is > > introduced by switching to your patch, it works before and it "breaks" > > after. > > > > The first part that is a bit weird is that we seem to be calling > > phylink_generic_validate() twice in a row from the same call site. > > > > For fixed link ports, instead of masking with what the fixed link actually > > supports, we seem to be using a supported mask which is all 1s which seems a > > bit excessive for a fixed link. > > > > This is an excerpt with the internal PHY: > > > > [ 4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): > > Calling phylink_generic_validate > > [ 4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0 > > [ 4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities: > > 0xff > > [ 4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff > > [ 4.239463] before anding supported with mask: 0000000,00000000,000062ff > > [ 4.246189] after anding supported with mask: 0000000,00000000,000062ff > > [ 4.252829] before anding advertising with mask: > > c000018,00000200,00036fff > > [ 4.259729] after anding advertising with mask: c000018,00000200,00036fff > > [ 4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY > > [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL) > > > > and this is what a fixed link port looks like: > > > > [ 4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized): > > Calling phylink_generic_validate > > [ 4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0 > > [ 4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff > > [ 4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff > > [ 4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff > > [ 4.465811] after anding supported with mask: c000018,00000200,00036fff > > [ 4.472450] before anding advertising with mask: > > c000018,00000200,00036fff > > [ 4.479349] after anding advertising with mask: c000018,00000200,00036fff > > > > or maybe the problem is with phylink_get_ksettings... ran out of time > > tonight to look further into it. > > It will be: > > s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex, > pl->supported, true); > linkmode_zero(pl->supported); > phylink_set(pl->supported, MII); > phylink_set(pl->supported, Pause); > phylink_set(pl->supported, Asym_Pause); > phylink_set(pl->supported, Autoneg); > if (s) { > __set_bit(s->bit, pl->supported); > __set_bit(s->bit, pl->link_config.lp_advertising); > > Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting() > returns the first entry it finds in the supported table: > > /* 1G */ > PHY_SETTING( 1000, FULL, 1000baseKX_Full ), > PHY_SETTING( 1000, FULL, 1000baseT_Full ), > PHY_SETTING( 1000, HALF, 1000baseT_Half ), > PHY_SETTING( 1000, FULL, 1000baseT1_Full ), > PHY_SETTING( 1000, FULL, 1000baseX_Full ), > > Consequently, 1000baseKX_Full is preferred over 1000baseT_Full. > > Fixed links don't specify their underlying technology, only the speed > and duplex, so going from speed and duplex to an ethtool link mode is > not easy. I suppose we could drop 1000baseKX_Full from the supported > bitmap in phylink_parse_fixedlink() before the first phylink_validate() > call. Alternatively, the table could be re-ordered. It was supposed to > be grouped by speed and sorted in descending match priority as specified > by the comment above the table. Does it really make sense that > 1000baseKX_Full is supposed to be preferred over all the other 1G > speeds? I suppose that's a question for Tom Lendacky > <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80 > ("phy: Expand phy speed/duplex settings array") back in 2014. Here's a patch for one of my suggestions above. Tom, I'd appreciate if you could look at this please. Thanks. 8<=== From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> Subject: [PATCH net-next] net: phy: prefer 1000baseT over 1000baseKX The PHY settings table is supposed to be sorted by descending match priority - in other words, earlier entries are preferred over later entries. The order of 1000baseKX/Full and 1000baseT/Full is such that we prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is a lot rarer than 1000baseT/Full, and thus is much less likely to be preferred. This causes phylink problems - it means a fixed link specifying a speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full rather than 1000baseT/Full as would be expected - and since we offer userspace a software emulation of a conventional copper PHY, we want to offer copper modes in preference to anything else. However, we do still want to allow the rarer modes as well. Hence, let's reorder these two modes to prefer copper. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c index 2870c33b8975..271fc01f7f7f 100644 --- a/drivers/net/phy/phy-core.c +++ b/drivers/net/phy/phy-core.c @@ -162,11 +162,11 @@ static const struct phy_setting settings[] = { PHY_SETTING( 2500, FULL, 2500baseT_Full ), PHY_SETTING( 2500, FULL, 2500baseX_Full ), /* 1G */ - PHY_SETTING( 1000, FULL, 1000baseKX_Full ), PHY_SETTING( 1000, FULL, 1000baseT_Full ), PHY_SETTING( 1000, HALF, 1000baseT_Half ), PHY_SETTING( 1000, FULL, 1000baseT1_Full ), PHY_SETTING( 1000, FULL, 1000baseX_Full ), + PHY_SETTING( 1000, FULL, 1000baseKX_Full ), /* 100M */ PHY_SETTING( 100, FULL, 100baseT_Full ), PHY_SETTING( 100, FULL, 100baseT1_Full ),
> The order of 1000baseKX/Full and 1000baseT/Full is such that we > prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is > a lot rarer than 1000baseT/Full, and thus is much less likely to > be preferred. > > This causes phylink problems - it means a fixed link specifying a > speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full > rather than 1000baseT/Full as would be expected - and since we offer > userspace a software emulation of a conventional copper PHY, we want > to offer copper modes in preference to anything else. However, we do > still want to allow the rarer modes as well. 2.5G already places T before X, so it makes it more uniform with that. For 10G, T comes last. Maybe we should also consider this case? Do we see more 10G copper than fibre/backplane? Andrew
On Sat, Dec 04, 2021 at 04:01:32PM +0100, Andrew Lunn wrote: > > The order of 1000baseKX/Full and 1000baseT/Full is such that we > > prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is > > a lot rarer than 1000baseT/Full, and thus is much less likely to > > be preferred. > > > > This causes phylink problems - it means a fixed link specifying a > > speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full > > rather than 1000baseT/Full as would be expected - and since we offer > > userspace a software emulation of a conventional copper PHY, we want > > to offer copper modes in preference to anything else. However, we do > > still want to allow the rarer modes as well. > > 2.5G already places T before X, so it makes it more uniform with that. > > For 10G, T comes last. Maybe we should also consider this case? Do we > see more 10G copper than fibre/backplane? For a fixed link, I'm not sure - but given that speeds higher than 1G can't be emulated by the C22 PHY emulation, does it really matter? Looking through the DTs that we have in the kernel tree, we have some fixed-links at 10G speed, and no one has complained, so I'd suggest leaving sleeping dogs and all that.
On 12/4/21 8:52 AM, Russell King (Oracle) wrote: > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: >> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > if you could look at this please. Thanks. I think it's fine to move the setting down. The driver that I was working on at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the array and why I added it), so I don't see an issue with moving it down. A quick build and test showed that I was able to successfully connect at 1 gbps. I didn't dive any deeper than that. Thanks, Tom > > 8<=== > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> > Subject: [PATCH net-next] net: phy: prefer 1000baseT over 1000baseKX > > The PHY settings table is supposed to be sorted by descending match > priority - in other words, earlier entries are preferred over later > entries. > > The order of 1000baseKX/Full and 1000baseT/Full is such that we > prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is > a lot rarer than 1000baseT/Full, and thus is much less likely to > be preferred. > > This causes phylink problems - it means a fixed link specifying a > speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full > rather than 1000baseT/Full as would be expected - and since we offer > userspace a software emulation of a conventional copper PHY, we want > to offer copper modes in preference to anything else. However, we do > still want to allow the rarer modes as well. > > Hence, let's reorder these two modes to prefer copper. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/phy-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c > index 2870c33b8975..271fc01f7f7f 100644 > --- a/drivers/net/phy/phy-core.c > +++ b/drivers/net/phy/phy-core.c > @@ -162,11 +162,11 @@ static const struct phy_setting settings[] = { > PHY_SETTING( 2500, FULL, 2500baseT_Full ), > PHY_SETTING( 2500, FULL, 2500baseX_Full ), > /* 1G */ > - PHY_SETTING( 1000, FULL, 1000baseKX_Full ), > PHY_SETTING( 1000, FULL, 1000baseT_Full ), > PHY_SETTING( 1000, HALF, 1000baseT_Half ), > PHY_SETTING( 1000, FULL, 1000baseT1_Full ), > PHY_SETTING( 1000, FULL, 1000baseX_Full ), > + PHY_SETTING( 1000, FULL, 1000baseKX_Full ), > /* 100M */ > PHY_SETTING( 100, FULL, 100baseT_Full ), > PHY_SETTING( 100, FULL, 100baseT1_Full ), >
On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote: > On 12/4/21 8:52 AM, Russell King (Oracle) wrote: > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > > > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > > > > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > > if you could look at this please. Thanks. > > I think it's fine to move the setting down. The driver that I was working on > at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the > array and why I added it), so I don't see an issue with moving it down. > > A quick build and test showed that I was able to successfully connect at 1 > gbps. I didn't dive any deeper than that. Thanks Tom! Can I use that to add your tested-by to this change please?
On 12/6/21 10:13 AM, Russell King (Oracle) wrote: > On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote: >> On 12/4/21 8:52 AM, Russell King (Oracle) wrote: >>> On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: >>>> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: >> >>> >>> Here's a patch for one of my suggestions above. Tom, I'd appreciate >>> if you could look at this please. Thanks. >> >> I think it's fine to move the setting down. The driver that I was working on >> at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the >> array and why I added it), so I don't see an issue with moving it down. >> >> A quick build and test showed that I was able to successfully connect at 1 >> gbps. I didn't dive any deeper than that. > > Thanks Tom! Can I use that to add your tested-by to this change please? Certainly. Tested-by: Tom Lendacky <thomas.lendacky@amd.com> >
On Mon, Dec 06, 2021 at 10:36:24AM -0600, Tom Lendacky wrote: > On 12/6/21 10:13 AM, Russell King (Oracle) wrote: > > On Mon, Dec 06, 2021 at 09:59:46AM -0600, Tom Lendacky wrote: > > > On 12/4/21 8:52 AM, Russell King (Oracle) wrote: > > > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > > > > > On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: > > > > > > > > > > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > > > > if you could look at this please. Thanks. > > > > > > I think it's fine to move the setting down. The driver that I was working on > > > at the time only advertised 1000baseKX_Full for 1gpbs (which wasn't in the > > > array and why I added it), so I don't see an issue with moving it down. > > > > > > A quick build and test showed that I was able to successfully connect at 1 > > > gbps. I didn't dive any deeper than that. > > > > Thanks Tom! Can I use that to add your tested-by to this change please? > > Certainly. > > Tested-by: Tom Lendacky <thomas.lendacky@amd.com> Thanks!
On 12/4/21 6:52 AM, Russell King (Oracle) wrote: > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: >> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote: >>> Now, with respect to the fixed link ports reporting 1000baseKX/Full this is >>> introduced by switching to your patch, it works before and it "breaks" >>> after. >>> >>> The first part that is a bit weird is that we seem to be calling >>> phylink_generic_validate() twice in a row from the same call site. >>> >>> For fixed link ports, instead of masking with what the fixed link actually >>> supports, we seem to be using a supported mask which is all 1s which seems a >>> bit excessive for a fixed link. >>> >>> This is an excerpt with the internal PHY: >>> >>> [ 4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): >>> Calling phylink_generic_validate >>> [ 4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0 >>> [ 4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities: >>> 0xff >>> [ 4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff >>> [ 4.239463] before anding supported with mask: 0000000,00000000,000062ff >>> [ 4.246189] after anding supported with mask: 0000000,00000000,000062ff >>> [ 4.252829] before anding advertising with mask: >>> c000018,00000200,00036fff >>> [ 4.259729] after anding advertising with mask: c000018,00000200,00036fff >>> [ 4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY >>> [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL) >>> >>> and this is what a fixed link port looks like: >>> >>> [ 4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized): >>> Calling phylink_generic_validate >>> [ 4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0 >>> [ 4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff >>> [ 4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff >>> [ 4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff >>> [ 4.465811] after anding supported with mask: c000018,00000200,00036fff >>> [ 4.472450] before anding advertising with mask: >>> c000018,00000200,00036fff >>> [ 4.479349] after anding advertising with mask: c000018,00000200,00036fff >>> >>> or maybe the problem is with phylink_get_ksettings... ran out of time >>> tonight to look further into it. >> >> It will be: >> >> s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex, >> pl->supported, true); >> linkmode_zero(pl->supported); >> phylink_set(pl->supported, MII); >> phylink_set(pl->supported, Pause); >> phylink_set(pl->supported, Asym_Pause); >> phylink_set(pl->supported, Autoneg); >> if (s) { >> __set_bit(s->bit, pl->supported); >> __set_bit(s->bit, pl->link_config.lp_advertising); >> >> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting() >> returns the first entry it finds in the supported table: >> >> /* 1G */ >> PHY_SETTING( 1000, FULL, 1000baseKX_Full ), >> PHY_SETTING( 1000, FULL, 1000baseT_Full ), >> PHY_SETTING( 1000, HALF, 1000baseT_Half ), >> PHY_SETTING( 1000, FULL, 1000baseT1_Full ), >> PHY_SETTING( 1000, FULL, 1000baseX_Full ), >> >> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full. >> >> Fixed links don't specify their underlying technology, only the speed >> and duplex, so going from speed and duplex to an ethtool link mode is >> not easy. I suppose we could drop 1000baseKX_Full from the supported >> bitmap in phylink_parse_fixedlink() before the first phylink_validate() >> call. Alternatively, the table could be re-ordered. It was supposed to >> be grouped by speed and sorted in descending match priority as specified >> by the comment above the table. Does it really make sense that >> 1000baseKX_Full is supposed to be preferred over all the other 1G >> speeds? I suppose that's a question for Tom Lendacky >> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80 >> ("phy: Expand phy speed/duplex settings array") back in 2014. > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > if you could look at this please. Thanks. I don't have objections on the patch per-se, but I am still wary that this is going to break another driver in terms of what its fixed link ports are supposed to report, so maybe the generic validation approach needs to be provided some additional hints as to what port link modes are supported, or rather, not supported. So I would suggest we have bcm_sf2 continue to implement ds->ops->validate which does call phylink_generic_validate() but also prunes unsupported link modes for its fixed link ports, what do you think?
On Mon, Dec 06, 2021 at 09:06:53AM -0800, Florian Fainelli wrote: > On 12/4/21 6:52 AM, Russell King (Oracle) wrote: > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > >> It will be: > >> > >> s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex, > >> pl->supported, true); > >> linkmode_zero(pl->supported); > >> phylink_set(pl->supported, MII); > >> phylink_set(pl->supported, Pause); > >> phylink_set(pl->supported, Asym_Pause); > >> phylink_set(pl->supported, Autoneg); > >> if (s) { > >> __set_bit(s->bit, pl->supported); > >> __set_bit(s->bit, pl->link_config.lp_advertising); > >> > >> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting() > >> returns the first entry it finds in the supported table: > >> > >> /* 1G */ > >> PHY_SETTING( 1000, FULL, 1000baseKX_Full ), > >> PHY_SETTING( 1000, FULL, 1000baseT_Full ), > >> PHY_SETTING( 1000, HALF, 1000baseT_Half ), > >> PHY_SETTING( 1000, FULL, 1000baseT1_Full ), > >> PHY_SETTING( 1000, FULL, 1000baseX_Full ), > >> > >> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full. > >> > >> Fixed links don't specify their underlying technology, only the speed > >> and duplex, so going from speed and duplex to an ethtool link mode is > >> not easy. I suppose we could drop 1000baseKX_Full from the supported > >> bitmap in phylink_parse_fixedlink() before the first phylink_validate() > >> call. Alternatively, the table could be re-ordered. It was supposed to > >> be grouped by speed and sorted in descending match priority as specified > >> by the comment above the table. Does it really make sense that > >> 1000baseKX_Full is supposed to be preferred over all the other 1G > >> speeds? I suppose that's a question for Tom Lendacky > >> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80 > >> ("phy: Expand phy speed/duplex settings array") back in 2014. > > > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > > if you could look at this please. Thanks. > > I don't have objections on the patch per-se, but I am still wary that > this is going to break another driver in terms of what its fixed link > ports are supposed to report, so maybe the generic validation approach > needs to be provided some additional hints as to what port link modes > are supported, or rather, not supported. Honestly, I'm not sure I'd call this a breakage, when we haven't really cared that the link modes for fixed links reflect the underlying link in the past. Fixed-links started out (and still are for the vast majority of drivers that use phylib for their fixed links) as being an emulation of a copper PHY. Thus, they end up with baseT linkmodes no matter what the actual underlying link is. Phylink provides a fixed-link implementation that is supposed to be broadly similar to the original phylib based implementation without using the emulation of a PHY directly, allowing it greater flexibility in the link speeds that it can support. It was never intended for a MAC driver to restrict the linkmode technologies - and doing so goes completely against the phylink design principle and also the phylink documentation. Restricting the linkmodes based on technologoy encodes information about the setup of the world outside of the MAC block into the MAC driver. This is actually a problem that needs to be sorted. Consider a driver which decides to restrict linkmodes based on technology, such as the Marvell DSA which presently allows only 1000baseX and 1000baseT linkmodes (at the time, there was no 1000baseKX ethtool linkmode.) Now someone comes along with a board design that interfaces one of the Marvell DSA ports to a PHY that supports 1000baseKX. They add 1000baseKX to the linkmodes that Marvell DSA now lets through. Any fixed link on Marvell DSA now ends up reporting 1000baseKX instead of 1000baseT as it used to before, even if the underlying link was actually 1000baseX. (This is why I say, we don't actually care what technology has historically been reported - it demonstrably is very much the case.) Now, going on further, there is the argument that we should be reporting baseT link modes for fixed links up to 1G speeds, since fixed links provide emulation of a copper PHY. For non-phylink, that copper PHY emulation was to allow phylib to be re-used for fixed-links, and thus you only ever get baseT linkmodes reported (and phylib-based fixed links only go up to 1G speeds.) If we don't fix this, then converting to phylink results in 1000baseKX being selected if one is compliant with the above. Then there's the argument that we have never really cared for the actual technology of a fixed link. For example, on the VF610 ZII rev B board, which uses the Freescale FEC driver (using phylib, not phylink), the port used to talk to the DSA switches reports: Settings for eth1: Supported ports: [ TP MII ] Supported link modes: 100baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 100baseT/Full ... Even though there is no twisted-pair copper link in sight - it's actually a RMII link, but we don't have any ethtool link modes to describe that. This was also true for Clearfog with its DSA switch connected via 1000BASE-X, except there we used to get 1000baseT/Full with the phylib- based fixed link prior to phylink. Then there's fixed links that use 10000/Full - these will end up being 10000baseCR/Full... even though they are not 10000baseCR - and again, we don't actually have an ethtool linkmode that reports what they are. So, really, the whole "technology" thing for fixed links is very vague and we have never actually cared if it actually reports the link. If MAC drivers restrict the technology to make fixed links "work" as they expect, they are restricting the technologies that the connection as a whole can support when not operating in fixed-link mode, and that is plain wrong. > So I would suggest we have bcm_sf2 continue to implement > ds->ops->validate which does call phylink_generic_validate() but also > prunes unsupported link modes for its fixed link ports, what do you > think? I'm not keen as I want to kill off .validate entirely, and not have its legacy hanging around making future development (e.g. to properly support rate-adaption) more difficult. I've been looking at this recently and I just can't come up with a clean way to have a MAC and PCS split where either the PCS or PHY do rate adaption without the MAC being fully aware that is going on in its validate() method. So, rather than keeping this around, if there is a need to specify the technology, I would rather introduce another field into phylink_config, misnamed, as "mac_techologies" which indicates whether we wish to restrict to baseT/baseX/baseKX etc and this _only_ gets used for fixed-links. It's misnamed because the MAC should have nothing to do with this, and it's a hack to allow people to have their preferred ethtool linkmodes.
On Mon, Dec 06, 2021 at 07:26:48PM +0000, Russell King (Oracle) wrote: > On Mon, Dec 06, 2021 at 09:06:53AM -0800, Florian Fainelli wrote: > > On 12/4/21 6:52 AM, Russell King (Oracle) wrote: > > > On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote: > > >> It will be: > > >> > > >> s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex, > > >> pl->supported, true); > > >> linkmode_zero(pl->supported); > > >> phylink_set(pl->supported, MII); > > >> phylink_set(pl->supported, Pause); > > >> phylink_set(pl->supported, Asym_Pause); > > >> phylink_set(pl->supported, Autoneg); > > >> if (s) { > > >> __set_bit(s->bit, pl->supported); > > >> __set_bit(s->bit, pl->link_config.lp_advertising); > > >> > > >> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting() > > >> returns the first entry it finds in the supported table: > > >> > > >> /* 1G */ > > >> PHY_SETTING( 1000, FULL, 1000baseKX_Full ), > > >> PHY_SETTING( 1000, FULL, 1000baseT_Full ), > > >> PHY_SETTING( 1000, HALF, 1000baseT_Half ), > > >> PHY_SETTING( 1000, FULL, 1000baseT1_Full ), > > >> PHY_SETTING( 1000, FULL, 1000baseX_Full ), > > >> > > >> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full. > > >> > > >> Fixed links don't specify their underlying technology, only the speed > > >> and duplex, so going from speed and duplex to an ethtool link mode is > > >> not easy. I suppose we could drop 1000baseKX_Full from the supported > > >> bitmap in phylink_parse_fixedlink() before the first phylink_validate() > > >> call. Alternatively, the table could be re-ordered. It was supposed to > > >> be grouped by speed and sorted in descending match priority as specified > > >> by the comment above the table. Does it really make sense that > > >> 1000baseKX_Full is supposed to be preferred over all the other 1G > > >> speeds? I suppose that's a question for Tom Lendacky > > >> <thomas.lendacky@amd.com>, who introduced this in 3e7077067e80 > > >> ("phy: Expand phy speed/duplex settings array") back in 2014. > > > > > > Here's a patch for one of my suggestions above. Tom, I'd appreciate > > > if you could look at this please. Thanks. > > > > I don't have objections on the patch per-se, but I am still wary that > > this is going to break another driver in terms of what its fixed link > > ports are supposed to report, so maybe the generic validation approach > > needs to be provided some additional hints as to what port link modes > > are supported, or rather, not supported. > > Honestly, I'm not sure I'd call this a breakage, when we haven't really > cared that the link modes for fixed links reflect the underlying link > in the past. > > Fixed-links started out (and still are for the vast majority of > drivers that use phylib for their fixed links) as being an emulation of > a copper PHY. Thus, they end up with baseT linkmodes no matter what the > actual underlying link is. > > Phylink provides a fixed-link implementation that is supposed to be > broadly similar to the original phylib based implementation without > using the emulation of a PHY directly, allowing it greater flexibility > in the link speeds that it can support. > > It was never intended for a MAC driver to restrict the linkmode > technologies - and doing so goes completely against the phylink design > principle and also the phylink documentation. Restricting the linkmodes > based on technologoy encodes information about the setup of the world > outside of the MAC block into the MAC driver. This is actually a > problem that needs to be sorted. > > Consider a driver which decides to restrict linkmodes based on > technology, such as the Marvell DSA which presently allows only > 1000baseX and 1000baseT linkmodes (at the time, there was no 1000baseKX > ethtool linkmode.) Now someone comes along with a board design that > interfaces one of the Marvell DSA ports to a PHY that supports > 1000baseKX. They add 1000baseKX to the linkmodes that Marvell DSA now > lets through. > > Any fixed link on Marvell DSA now ends up reporting 1000baseKX instead > of 1000baseT as it used to before, even if the underlying link was > actually 1000baseX. (This is why I say, we don't actually care what > technology has historically been reported - it demonstrably is very > much the case.) > > Now, going on further, there is the argument that we should be > reporting baseT link modes for fixed links up to 1G speeds, since fixed > links provide emulation of a copper PHY. For non-phylink, that copper > PHY emulation was to allow phylib to be re-used for fixed-links, and > thus you only ever get baseT linkmodes reported (and phylib-based fixed > links only go up to 1G speeds.) If we don't fix this, then converting > to phylink results in 1000baseKX being selected if one is compliant > with the above. > > Then there's the argument that we have never really cared for the > actual technology of a fixed link. For example, on the VF610 ZII rev B > board, which uses the Freescale FEC driver (using phylib, not phylink), > the port used to talk to the DSA switches reports: > > Settings for eth1: > Supported ports: [ TP MII ] > Supported link modes: 100baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Supported FEC modes: Not reported > Advertised link modes: 100baseT/Full > ... > > Even though there is no twisted-pair copper link in sight - it's > actually a RMII link, but we don't have any ethtool link modes to > describe that. > > This was also true for Clearfog with its DSA switch connected via > 1000BASE-X, except there we used to get 1000baseT/Full with the phylib- > based fixed link prior to phylink. > > Then there's fixed links that use 10000/Full - these will end up being > 10000baseCR/Full... even though they are not 10000baseCR - and again, > we don't actually have an ethtool linkmode that reports what they are. > > So, really, the whole "technology" thing for fixed links is very vague > and we have never actually cared if it actually reports the link. > > If MAC drivers restrict the technology to make fixed links "work" as > they expect, they are restricting the technologies that the connection > as a whole can support when not operating in fixed-link mode, and that > is plain wrong. > > > So I would suggest we have bcm_sf2 continue to implement > > ds->ops->validate which does call phylink_generic_validate() but also > > prunes unsupported link modes for its fixed link ports, what do you > > think? > > I'm not keen as I want to kill off .validate entirely, and not have its > legacy hanging around making future development (e.g. to properly > support rate-adaption) more difficult. I've been looking at this > recently and I just can't come up with a clean way to have a MAC and > PCS split where either the PCS or PHY do rate adaption without the MAC > being fully aware that is going on in its validate() method. > > So, rather than keeping this around, if there is a need to specify the > technology, I would rather introduce another field into phylink_config, > misnamed, as "mac_techologies" which indicates whether we wish to > restrict to baseT/baseX/baseKX etc and this _only_ gets used for > fixed-links. It's misnamed because the MAC should have nothing to do > with this, and it's a hack to allow people to have their preferred > ethtool linkmodes. ... and this is not just a problem for bcm_sf2. I was about to send the Marvell DSA conversion to phylink_generic_validate(), and the fixed-link I have for lan6 on Clearfog shows the same change to 1000baseKX. The same is going to be true across the board. I've thought about this more. There is precedent for changing the order - we've done it already with 1000baseX vs 1000baseT... silently. See 3c6b59d6f07c ("net: phy: Add more link modes to the settings table") - it was originally: ETHTOOL_LINK_MODE_2500baseX_Full_BIT ETHTOOL_LINK_MODE_1000baseKX_Full_BIT ETHTOOL_LINK_MODE_1000baseX_Full_BIT ETHTOOL_LINK_MODE_1000baseT_Full_BIT ETHTOOL_LINK_MODE_1000baseT_Half_BIT and became: ETHTOOL_LINK_MODE_2500baseT_Full_BIT ETHTOOL_LINK_MODE_2500baseX_Full_BIT ETHTOOL_LINK_MODE_1000baseKX_Full_BIT ETHTOOL_LINK_MODE_1000baseT_Full_BIT ETHTOOL_LINK_MODE_1000baseT_Half_BIT ETHTOOL_LINK_MODE_1000baseX_Full_BIT This would have made a fixed-link on a Marvell DSA switch to change from reporting 1000baseX/Full to reporting 1000baseT/Full... and no one noticed that change. So, I doubt anyone will notice if we move 1000baseKX_Full down below the 1000baseT entries. I suspect also that there are very few who even care what link modes are reported for a fixed-link. However, as I said above, we could introduce something like a bitmask of media technologies to allow the preference for fixed-links to be specified. That said, if fixed-links were something new today, I think we'd be saying about having that come from DT/firmware on the grounds that "DT describes the hardware" and this is very much a hardware property!
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 13aa43b5cffd..d6ef0fb0d943 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -672,49 +672,18 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port) PHY_BRCM_IDDQ_SUSPEND; } -static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port, - unsigned long *supported, - struct phylink_link_state *state) +static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) { - struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds); - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; - - if (!phy_interface_mode_is_rgmii(state->interface) && - state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_REVMII && - state->interface != PHY_INTERFACE_MODE_GMII && - state->interface != PHY_INTERFACE_MODE_INTERNAL && - state->interface != PHY_INTERFACE_MODE_MOCA) { - linkmode_zero(supported); - if (port != core_readl(priv, CORE_IMP0_PRT_ID)) - dev_err(ds->dev, - "Unsupported interface: %d for port %d\n", - state->interface, port); - return; - } - - /* Allow all the expected bits */ - phylink_set(mask, Autoneg); - phylink_set_port_modes(mask); - phylink_set(mask, Pause); - phylink_set(mask, Asym_Pause); - - /* With the exclusion of MII and Reverse MII, we support Gigabit, - * including Half duplex - */ - if (state->interface != PHY_INTERFACE_MODE_MII && - state->interface != PHY_INTERFACE_MODE_REVMII) { - phylink_set(mask, 1000baseT_Full); - phylink_set(mask, 1000baseT_Half); - } - - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 100baseT_Full); - - linkmode_and(supported, supported, mask); - linkmode_and(state->advertising, state->advertising, mask); + __set_bit(PHY_INTERFACE_MODE_MII, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_REVMII, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_GMII, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_MOCA, config->supported_interfaces); + phy_interface_set_rgmii(config->supported_interfaces); + + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | + MAC_10 | MAC_100 | MAC_1000; } static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port, @@ -1181,7 +1150,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = { .get_sset_count = bcm_sf2_sw_get_sset_count, .get_ethtool_phy_stats = b53_get_ethtool_phy_stats, .get_phy_flags = bcm_sf2_sw_get_phy_flags, - .phylink_validate = bcm_sf2_sw_validate, + .phylink_get_caps = bcm_sf2_sw_get_caps, .phylink_mac_config = bcm_sf2_sw_mac_config, .phylink_mac_link_down = bcm_sf2_sw_mac_link_down, .phylink_mac_link_up = bcm_sf2_sw_mac_link_up,
Populate the supported interfaces and MAC capabilities for the bcm_sf2 DSA switch and remove the old validate implementation to allow DSA to use phylink_generic_validate() for this switch driver. The exclusion of Gigabit linkmodes for MII and Reverse MII links is handled within phylink_generic_validate() in phylink, so there is no need to make them conditional on the interface mode in the driver. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/bcm_sf2.c | 55 +++++++++------------------------------ 1 file changed, 12 insertions(+), 43 deletions(-)