Messages in this thread | | | Subject | Re: [PATCH net-next v4 03/10] net: sparx5: add hostmode with phylink support | From | Steen Hegelund <> | Date | Wed, 23 Jun 2021 15:04:53 +0200 |
| |
Hi Russell,
Thanks for your comments.
On Mon, 2021-06-21 at 15:26 +0100, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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.
Yes the sparx5_destroy_netdev() is currently being added in a later patch. I will move it here instead.
> > 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?
Yes the unregister_netdev is missing. I will add that before the phylink is destroyed.
> > > 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?)
I will move it to the top.
> > > +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?
Hmm. I have now removed that, and will the pcs_config() to collect the advertised pause mode.
> > 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.
OK. I will use the pcs_config() to collect the advertising mode and the .._link_up() to collect the configuration value.
> > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
-- BR Steen
-=-=-=-=-=-=-=-=-=-=-=-=-=-= steen.hegelund@microchip.com
| |