Re: [PATCH net-next v4 03/10] net: sparx5: add hostmode with phylink support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jun 15, 2021 at 10:50:27AM +0200, Steen Hegelund wrote:
> This patch adds netdevs and phylink support for the ports in the switch.
> It also adds register based injection and extraction for these ports.
> 
> Frame DMA support for injection and extraction will be added in a later
> series.
> 
> Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> Signed-off-by: Bjarni Jonasson <bjarni.jonasson@microchip.com>
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

Hi,

While looking at this patch, I found sparx5_destroy_netdev() which seems
to be unreferenced - it may be referenced in a future patch. However,
this means that while sparx5_create_port() creates the phylink
structure, there is nothing in this patch that cleans it up.

I'm puzzled by the call to phylink_disconnect_phy() in
sparx5_destroy_netdev() too - surely if we get to the point of tearing
down stuff that we've created at initialisation, the interface had
better be down?

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c b/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c
> new file mode 100644
> index 000000000000..c17a3502645a
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_phylink.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Microchip Sparx5 Switch driver
> + *
> + * Copyright (c) 2021 Microchip Technology Inc. and its subsidiaries.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/phylink.h>
> +#include <linux/device.h>
> +#include <linux/netdevice.h>
> +#include <linux/sfp.h>
> +
> +#include "sparx5_main_regs.h"
> +#include "sparx5_main.h"
> +
> +static void sparx5_phylink_validate(struct phylink_config *config,
> +				    unsigned long *supported,
> +				    struct phylink_link_state *state)
> +{
> +	struct sparx5_port *port = netdev_priv(to_net_dev(config->dev));
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set_port_modes(mask);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_5GBASER:
> +	case PHY_INTERFACE_MODE_10GBASER:
> +	case PHY_INTERFACE_MODE_25GBASER:
> +	case PHY_INTERFACE_MODE_NA:
> +		if (port->conf.bandwidth == SPEED_5000)
> +			phylink_set(mask, 5000baseT_Full);
> +		if (port->conf.bandwidth == SPEED_10000) {
> +			phylink_set(mask, 5000baseT_Full);
> +			phylink_set(mask, 10000baseT_Full);
> +			phylink_set(mask, 10000baseCR_Full);
> +			phylink_set(mask, 10000baseSR_Full);
> +			phylink_set(mask, 10000baseLR_Full);
> +			phylink_set(mask, 10000baseLRM_Full);
> +			phylink_set(mask, 10000baseER_Full);
> +		}
> +		if (port->conf.bandwidth == SPEED_25000) {
> +			phylink_set(mask, 5000baseT_Full);
> +			phylink_set(mask, 10000baseT_Full);
> +			phylink_set(mask, 10000baseCR_Full);
> +			phylink_set(mask, 10000baseSR_Full);
> +			phylink_set(mask, 10000baseLR_Full);
> +			phylink_set(mask, 10000baseLRM_Full);
> +			phylink_set(mask, 10000baseER_Full);
> +			phylink_set(mask, 25000baseCR_Full);
> +			phylink_set(mask, 25000baseSR_Full);
> +		}

I really need to fix phylink so we shouldn't be lying about which
speeds are supported over a 10GBASER link... but that's something
for the future.

> +static bool port_conf_has_changed(struct sparx5_port_config *a, struct sparx5_port_config *b)
> +{
> +	if (a->speed != b->speed ||
> +	    a->portmode != b->portmode ||
> +	    a->autoneg != b->autoneg ||
> +	    a->pause != b->pause ||
> +	    a->power_down != b->power_down ||
> +	    a->media != b->media)
> +		return true;
> +	return false;
> +}

Should this be positioned somewhere else rather than in the middle of
the sparx5 phylink functions (top of file maybe?)

> +static void sparx5_phylink_mac_config(struct phylink_config *config,
> +				      unsigned int mode,
> +				      const struct phylink_link_state *state)
> +{
> +	struct sparx5_port *port = netdev_priv(to_net_dev(config->dev));
> +
> +	port->conf.autoneg = state->an_enabled;
> +	port->conf.pause = state->pause;

What are you doing with state->pause? It looks to me like you're using
both of these to carry configuration to pcs_config?

Generally, an_enabled can be pulled out of the advertising mask, it
should always reflect ETHTOOL_LINK_MODE_Autoneg_BIT. The "pause"
interpretation of the pause bits here are somewhat hardware specific.
It depends whether the MAC automatically receives state information
from the PCS or not. If the hardware does, then MLO_PAUSE_AN indicates
whether that should be permitted or not.

Otherwise, the advertising mask in pcs_config() indicates which pause
modes should be advertised, and the tx_pause/rx_pause in the
*_link_up() indicates what should actually be set.

Thanks.

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