| Message ID | E1mpwSD-00D8Ln-88@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 Wed, Nov 24, 2021 at 05:53:09PM +0000, Russell King (Oracle) wrote: > Populate the supported interfaces and MAC capabilities for the Ocelot > DSA switches and remove the old validate implementation to allow DSA to > use phylink_generic_validate() for this switch driver. > > The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a > single interface mode, defined by ocelot_port->phy_mode, so we indicate > only this interface mode to phylink. Since phylink restricts the > ethtool link modes based on interface, we do not need to make the MAC > capabilities dependent on the interface mode. Yes, and this driver cannot make use of phylink_generic_validate() unless something changes in phylink_get_linkmodes(). You've said a number of times that PHY rate adaptation via PAUSE frames is not something that is supported, yet it works with 2500base-x and the felix driver, and we use this functionality on LS1028A-QDS boards and the AQR412 PHY, and customer boards using LS1028A probably use it too. See this comment in ocelot_phylink_mac_link_up(): /* Handle RX pause in all cases, with 2500base-X this is used for rate * adaptation. */ mac_fc_cfg |= SYS_MAC_FC_CFG_RX_FC_ENA; The reason why you've said it isn't supported is because "it will confuse MAC and PCS drivers at the moment", which is something that does not happen for this particular combination of MAC and PCS (Lynx) drivers and SERDES protocol (because the speed is fixed, there is no reason to look at the "speed" argument which represents the media-side link speed). And what has to change in phylink_get_linkmodes() is this: void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface, unsigned long mac_capabilities) { unsigned long caps = MAC_SYM_PAUSE | MAC_ASYM_PAUSE; switch (interface) { (...) case PHY_INTERFACE_MODE_2500BASEX: caps |= MAC_2500FD; break; (...) } phylink_caps_to_linkmodes(linkmodes, caps & mac_capabilities); } As long as phylink_caps_to_linkmodes() doesn't have additional logic to not remove the gigabit and lower link modes from the PHY advertisement and supported masks when MAC_2500FD is set but MAC_1000FD and lower aren't (and the driver requests rate adaptation via PAUSE frames for this PHY mode), then the generic validation method is going to introduce regressions here, which are not acceptable. Otherwise the patch looks good. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/dsa/ocelot/felix.c | 11 ++++--- > drivers/net/dsa/ocelot/felix.h | 5 ++-- > drivers/net/dsa/ocelot/felix_vsc9959.c | 37 +++++------------------- > drivers/net/dsa/ocelot/seville_vsc9953.c | 34 +++++----------------- > 4 files changed, 21 insertions(+), 66 deletions(-) > > diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c > index e487143709da..26529db951fc 100644 > --- a/drivers/net/dsa/ocelot/felix.c > +++ b/drivers/net/dsa/ocelot/felix.c > @@ -801,15 +801,14 @@ static int felix_vlan_del(struct dsa_switch *ds, int port, > return ocelot_vlan_del(ocelot, port, vlan->vid); > } > > -static void felix_phylink_validate(struct dsa_switch *ds, int port, > - unsigned long *supported, > - struct phylink_link_state *state) > +static void felix_phylink_get_caps(struct dsa_switch *ds, int port, > + struct phylink_config *config) > { > struct ocelot *ocelot = ds->priv; > struct felix *felix = ocelot_to_felix(ocelot); > > - if (felix->info->phylink_validate) > - felix->info->phylink_validate(ocelot, port, supported, state); > + if (felix->info->phylink_get_caps) > + felix->info->phylink_get_caps(ocelot, port, config); > } > > static void felix_phylink_mac_config(struct dsa_switch *ds, int port, > @@ -1644,7 +1643,7 @@ const struct dsa_switch_ops felix_switch_ops = { > .get_ethtool_stats = felix_get_ethtool_stats, > .get_sset_count = felix_get_sset_count, > .get_ts_info = felix_get_ts_info, > - .phylink_validate = felix_phylink_validate, > + .phylink_get_caps = felix_phylink_get_caps, > .phylink_mac_config = felix_phylink_mac_config, > .phylink_mac_link_down = felix_phylink_mac_link_down, > .phylink_mac_link_up = felix_phylink_mac_link_up, > diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h > index dfe08dddd262..1060add151de 100644 > --- a/drivers/net/dsa/ocelot/felix.h > +++ b/drivers/net/dsa/ocelot/felix.h > @@ -43,9 +43,8 @@ struct felix_info { > > int (*mdio_bus_alloc)(struct ocelot *ocelot); > void (*mdio_bus_free)(struct ocelot *ocelot); > - void (*phylink_validate)(struct ocelot *ocelot, int port, > - unsigned long *supported, > - struct phylink_link_state *state); > + void (*phylink_get_caps)(struct ocelot *ocelot, int port, > + struct phylink_config *config); > int (*prevalidate_phy_mode)(struct ocelot *ocelot, int port, > phy_interface_t phy_mode); > int (*port_setup_tc)(struct dsa_switch *ds, int port, > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c > index 42ac1952b39a..091d33a52e41 100644 > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c > @@ -938,39 +938,16 @@ static int vsc9959_reset(struct ocelot *ocelot) > return 0; > } > > -static void vsc9959_phylink_validate(struct ocelot *ocelot, int port, > - unsigned long *supported, > - struct phylink_link_state *state) > +static void vsc9959_phylink_get_caps(struct ocelot *ocelot, int port, > + struct phylink_config *config) > { > struct ocelot_port *ocelot_port = ocelot->ports[port]; > - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > - if (state->interface != PHY_INTERFACE_MODE_NA && > - state->interface != ocelot_port->phy_mode) { > - linkmode_zero(supported); > - return; > - } > - > - phylink_set_port_modes(mask); > - phylink_set(mask, Autoneg); > - phylink_set(mask, Pause); > - phylink_set(mask, Asym_Pause); > - phylink_set(mask, 10baseT_Half); > - phylink_set(mask, 10baseT_Full); > - phylink_set(mask, 100baseT_Half); > - phylink_set(mask, 100baseT_Full); > - phylink_set(mask, 1000baseT_Half); > - phylink_set(mask, 1000baseT_Full); > - > - if (state->interface == PHY_INTERFACE_MODE_INTERNAL || > - state->interface == PHY_INTERFACE_MODE_2500BASEX || > - state->interface == PHY_INTERFACE_MODE_USXGMII) { > - phylink_set(mask, 2500baseT_Full); > - phylink_set(mask, 2500baseX_Full); > - } > + __set_bit(ocelot_port->phy_mode, > + config->supported_interfaces); This fits on a single line. > > - 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 | MAC_2500FD; > } > > static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port, > @@ -2161,7 +2138,7 @@ static const struct felix_info felix_info_vsc9959 = { > .ptp_caps = &vsc9959_ptp_caps, > .mdio_bus_alloc = vsc9959_mdio_bus_alloc, > .mdio_bus_free = vsc9959_mdio_bus_free, > - .phylink_validate = vsc9959_phylink_validate, > + .phylink_get_caps = vsc9959_phylink_get_caps, > .prevalidate_phy_mode = vsc9959_prevalidate_phy_mode, > .port_setup_tc = vsc9959_port_setup_tc, > .port_sched_speed_set = vsc9959_sched_speed_set, > diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c > index 899b98193b4a..37759853e1b2 100644 > --- a/drivers/net/dsa/ocelot/seville_vsc9953.c > +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c > @@ -994,36 +994,16 @@ static int vsc9953_reset(struct ocelot *ocelot) > return 0; > } > > -static void vsc9953_phylink_validate(struct ocelot *ocelot, int port, > - unsigned long *supported, > - struct phylink_link_state *state) > +static void vsc9953_phylink_get_caps(struct ocelot *ocelot, int port, > + struct phylink_config *config) > { > struct ocelot_port *ocelot_port = ocelot->ports[port]; > - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > - if (state->interface != PHY_INTERFACE_MODE_NA && > - state->interface != ocelot_port->phy_mode) { > - linkmode_zero(supported); > - return; > - } > - > - phylink_set_port_modes(mask); > - phylink_set(mask, Autoneg); > - phylink_set(mask, Pause); > - phylink_set(mask, Asym_Pause); > - phylink_set(mask, 10baseT_Full); > - phylink_set(mask, 10baseT_Half); > - phylink_set(mask, 100baseT_Full); > - phylink_set(mask, 100baseT_Half); > - phylink_set(mask, 1000baseT_Full); > - > - if (state->interface == PHY_INTERFACE_MODE_INTERNAL) { > - phylink_set(mask, 2500baseT_Full); > - phylink_set(mask, 2500baseX_Full); > - } > + __set_bit(ocelot_port->phy_mode, > + config->supported_interfaces); > > - 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_1000FD | MAC_2500FD; I would prefer if MAC_10 was aligned with MAC_ASYM_PAUSE, if possible. Thanks. > } > > static int vsc9953_prevalidate_phy_mode(struct ocelot *ocelot, int port, > @@ -1185,7 +1165,7 @@ static const struct felix_info seville_info_vsc9953 = { > .num_tx_queues = OCELOT_NUM_TC, > .mdio_bus_alloc = vsc9953_mdio_bus_alloc, > .mdio_bus_free = vsc9953_mdio_bus_free, > - .phylink_validate = vsc9953_phylink_validate, > + .phylink_get_caps = vsc9953_phylink_get_caps, > .prevalidate_phy_mode = vsc9953_prevalidate_phy_mode, > }; > > -- > 2.30.2 >
On Wed, Nov 24, 2021 at 08:07:49PM +0000, Vladimir Oltean wrote: > On Wed, Nov 24, 2021 at 05:53:09PM +0000, Russell King (Oracle) wrote: > > Populate the supported interfaces and MAC capabilities for the Ocelot > > DSA switches and remove the old validate implementation to allow DSA to > > use phylink_generic_validate() for this switch driver. > > > > The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a > > single interface mode, defined by ocelot_port->phy_mode, so we indicate > > only this interface mode to phylink. Since phylink restricts the > > ethtool link modes based on interface, we do not need to make the MAC > > capabilities dependent on the interface mode. > > Yes, and this driver cannot make use of phylink_generic_validate() > unless something changes in phylink_get_linkmodes(). You've said a > number of times that PHY rate adaptation via PAUSE frames is not > something that is supported, yet it works with 2500base-x and the felix > driver, and we use this functionality on LS1028A-QDS boards and the > AQR412 PHY, and customer boards using LS1028A probably use it too. See > this comment in ocelot_phylink_mac_link_up(): I'll drop this for now, since the issues around rate adaption should not be handled by phylink_generic_validate(). The point of this generic helper is to deal with the common case. We don't get have a way of knowing that the PHY is using rate adaption, and when rate adaption is in use, it changes the requirements for the validation path quite substantially. So, we need: 1) phylib to be able to tell us that rate adaption is happening on the PHY. 2) change the way we restrict the PHY support/advertisements when rate adaption is in use. I view this as an entirely separate change to this series; it needs to change in phylink_bringup_phy(), where the restriction is applied to the PHY, and not in phylink_validate() or below that function. Thanks.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index e487143709da..26529db951fc 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -801,15 +801,14 @@ static int felix_vlan_del(struct dsa_switch *ds, int port, return ocelot_vlan_del(ocelot, port, vlan->vid); } -static void felix_phylink_validate(struct dsa_switch *ds, int port, - unsigned long *supported, - struct phylink_link_state *state) +static void felix_phylink_get_caps(struct dsa_switch *ds, int port, + struct phylink_config *config) { struct ocelot *ocelot = ds->priv; struct felix *felix = ocelot_to_felix(ocelot); - if (felix->info->phylink_validate) - felix->info->phylink_validate(ocelot, port, supported, state); + if (felix->info->phylink_get_caps) + felix->info->phylink_get_caps(ocelot, port, config); } static void felix_phylink_mac_config(struct dsa_switch *ds, int port, @@ -1644,7 +1643,7 @@ const struct dsa_switch_ops felix_switch_ops = { .get_ethtool_stats = felix_get_ethtool_stats, .get_sset_count = felix_get_sset_count, .get_ts_info = felix_get_ts_info, - .phylink_validate = felix_phylink_validate, + .phylink_get_caps = felix_phylink_get_caps, .phylink_mac_config = felix_phylink_mac_config, .phylink_mac_link_down = felix_phylink_mac_link_down, .phylink_mac_link_up = felix_phylink_mac_link_up, diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index dfe08dddd262..1060add151de 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -43,9 +43,8 @@ struct felix_info { int (*mdio_bus_alloc)(struct ocelot *ocelot); void (*mdio_bus_free)(struct ocelot *ocelot); - void (*phylink_validate)(struct ocelot *ocelot, int port, - unsigned long *supported, - struct phylink_link_state *state); + void (*phylink_get_caps)(struct ocelot *ocelot, int port, + struct phylink_config *config); int (*prevalidate_phy_mode)(struct ocelot *ocelot, int port, phy_interface_t phy_mode); int (*port_setup_tc)(struct dsa_switch *ds, int port, diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 42ac1952b39a..091d33a52e41 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -938,39 +938,16 @@ static int vsc9959_reset(struct ocelot *ocelot) return 0; } -static void vsc9959_phylink_validate(struct ocelot *ocelot, int port, - unsigned long *supported, - struct phylink_link_state *state) +static void vsc9959_phylink_get_caps(struct ocelot *ocelot, int port, + struct phylink_config *config) { struct ocelot_port *ocelot_port = ocelot->ports[port]; - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; - if (state->interface != PHY_INTERFACE_MODE_NA && - state->interface != ocelot_port->phy_mode) { - linkmode_zero(supported); - return; - } - - phylink_set_port_modes(mask); - phylink_set(mask, Autoneg); - phylink_set(mask, Pause); - phylink_set(mask, Asym_Pause); - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 100baseT_Full); - phylink_set(mask, 1000baseT_Half); - phylink_set(mask, 1000baseT_Full); - - if (state->interface == PHY_INTERFACE_MODE_INTERNAL || - state->interface == PHY_INTERFACE_MODE_2500BASEX || - state->interface == PHY_INTERFACE_MODE_USXGMII) { - phylink_set(mask, 2500baseT_Full); - phylink_set(mask, 2500baseX_Full); - } + __set_bit(ocelot_port->phy_mode, + config->supported_interfaces); - 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 | MAC_2500FD; } static int vsc9959_prevalidate_phy_mode(struct ocelot *ocelot, int port, @@ -2161,7 +2138,7 @@ static const struct felix_info felix_info_vsc9959 = { .ptp_caps = &vsc9959_ptp_caps, .mdio_bus_alloc = vsc9959_mdio_bus_alloc, .mdio_bus_free = vsc9959_mdio_bus_free, - .phylink_validate = vsc9959_phylink_validate, + .phylink_get_caps = vsc9959_phylink_get_caps, .prevalidate_phy_mode = vsc9959_prevalidate_phy_mode, .port_setup_tc = vsc9959_port_setup_tc, .port_sched_speed_set = vsc9959_sched_speed_set, diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index 899b98193b4a..37759853e1b2 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -994,36 +994,16 @@ static int vsc9953_reset(struct ocelot *ocelot) return 0; } -static void vsc9953_phylink_validate(struct ocelot *ocelot, int port, - unsigned long *supported, - struct phylink_link_state *state) +static void vsc9953_phylink_get_caps(struct ocelot *ocelot, int port, + struct phylink_config *config) { struct ocelot_port *ocelot_port = ocelot->ports[port]; - __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; - if (state->interface != PHY_INTERFACE_MODE_NA && - state->interface != ocelot_port->phy_mode) { - linkmode_zero(supported); - return; - } - - phylink_set_port_modes(mask); - phylink_set(mask, Autoneg); - phylink_set(mask, Pause); - phylink_set(mask, Asym_Pause); - phylink_set(mask, 10baseT_Full); - phylink_set(mask, 10baseT_Half); - phylink_set(mask, 100baseT_Full); - phylink_set(mask, 100baseT_Half); - phylink_set(mask, 1000baseT_Full); - - if (state->interface == PHY_INTERFACE_MODE_INTERNAL) { - phylink_set(mask, 2500baseT_Full); - phylink_set(mask, 2500baseX_Full); - } + __set_bit(ocelot_port->phy_mode, + config->supported_interfaces); - 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_1000FD | MAC_2500FD; } static int vsc9953_prevalidate_phy_mode(struct ocelot *ocelot, int port, @@ -1185,7 +1165,7 @@ static const struct felix_info seville_info_vsc9953 = { .num_tx_queues = OCELOT_NUM_TC, .mdio_bus_alloc = vsc9953_mdio_bus_alloc, .mdio_bus_free = vsc9953_mdio_bus_free, - .phylink_validate = vsc9953_phylink_validate, + .phylink_get_caps = vsc9953_phylink_get_caps, .prevalidate_phy_mode = vsc9953_prevalidate_phy_mode, };
Populate the supported interfaces and MAC capabilities for the Ocelot DSA switches and remove the old validate implementation to allow DSA to use phylink_generic_validate() for this switch driver. The felix_vsc9959 and seville_vsc9953 sub-drivers only supports a single interface mode, defined by ocelot_port->phy_mode, so we indicate only this interface mode to phylink. Since phylink restricts the ethtool link modes based on interface, we do not need to make the MAC capabilities dependent on the interface mode. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/ocelot/felix.c | 11 ++++--- drivers/net/dsa/ocelot/felix.h | 5 ++-- drivers/net/dsa/ocelot/felix_vsc9959.c | 37 +++++------------------- drivers/net/dsa/ocelot/seville_vsc9953.c | 34 +++++----------------- 4 files changed, 21 insertions(+), 66 deletions(-)