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:

quoted

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 <redacted>
Signed-off-by: Lars Povlsen <redacted>

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.

quoted

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.

quoted

+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.

quoted

+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