From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8E43AC4743C for ; Mon, 21 Jun 2021 14:28:32 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4F95B61151 for ; Mon, 21 Jun 2021 14:28:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F95B61151 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=armlinux.org.uk Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DdEGE1BXvV/EhQ6muzi9qfAAktJWdK/BycYgF7kK6Eg=; b=gkyo8nvMQjhYAy 1idceu9926lwWwpd9A8jeFJGocsjXWoP+/U1gBd4I+azR/tEBD8JVybsPJO7olm/i1r8zAiR8XZqe cxBW8LINIo5y/n977QQ7r9mVu6eRv/935u3YhLDnRliUBfcEy3GkWs/33rsihCrqFgEXJNK888Fje EiegXXkLLRDo5kn17bln3ZbHkvdzTXKqlz71LSmxtiU2TyX04dwFbG7894Mf1VQMIE/7w2ml+yi39 3La5i7itxmoHaLdo4u0EUfu4qUVsTQRud6GcHyG55zyucqwcQH462hUj8wZd+lN0m91J5mgulf21B cwAyVzJgUISF4YoaSXtg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lvKt3-003kkR-Ax; Mon, 21 Jun 2021 14:26:53 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lvKsy-003khj-AW for linux-arm-kernel@lists.infradead.org; Mon, 21 Jun 2021 14:26:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=+FLy97mYiZUhpCmnjYrs9sM1itPFoTYvaxLbjIGV0Vo=; b=ra6UFsNTyawCbvwUOQ4AmcVjY ZUEnTXHRmXzkEE3aIfpcLE3COwLKLFVy2ZBqsybs9U7dwlMCYflbJgWW/jdbfGAnL8sY0b7mXq3MZ +GBGwc8815KLbSQP17ex7rB5Fm/Fndju7fS64XbZErWb36BvqDelQtDONcKy2VAK4Gmtw5i0dV8lo 7IRdiAgloL0DaeTeI3OVatJPcBHqIScdDl+1efRNgJpR4KGtFouyAvjteszHpTs26L0BdgBUGHQZT za8H9bdN+7EOovuOrhR3/1xW3Vs5/HFiXHQMl12MNd34Lq61deVnGb2GJzBi34taHIDpdXnjPtDWi aLw7i5Yww==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:45226) by pandora.armlinux.org.uk with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lvKsh-0004FE-NN; Mon, 21 Jun 2021 15:26:31 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.92) (envelope-from ) id 1lvKse-0002xR-Iz; Mon, 21 Jun 2021 15:26:28 +0100 Date: Mon, 21 Jun 2021 15:26:28 +0100 From: "Russell King (Oracle)" To: Steen Hegelund Cc: "David S. Miller" , Jakub Kicinski , Andrew Lunn , Microchip Linux Driver Support , Alexandre Belloni , Madalin Bucur , Mark Einon , Masahiro Yamada , Arnd Bergmann , Philipp Zabel , Simon Horman , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Bjarni Jonasson , Lars Povlsen Subject: Re: [PATCH net-next v4 03/10] net: sparx5: add hostmode with phylink support Message-ID: <20210621142628.GM22278@shell.armlinux.org.uk> References: <20210615085034.1262457-1-steen.hegelund@microchip.com> <20210615085034.1262457-4-steen.hegelund@microchip.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210615085034.1262457-4-steen.hegelund@microchip.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210621_072648_560015_A5EDC0A6 X-CRM114-Status: GOOD ( 28.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > Signed-off-by: Bjarni Jonasson > Signed-off-by: Lars Povlsen 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 > +#include > +#include > +#include > +#include > + > +#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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel