Re: [PATCH RFC net-next 09/12] net: dsa: ocelot: convert to phylink_generic_validate()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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
>