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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 958B6C47422 for ; Mon, 29 Jan 2024 13:16:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DeLCxM8cnR/IFL5aBWFz8eDyBKI1aGUY+AvmZvPfO/I=; b=LSC61mrdB+AC+y 1lxJ0pI5c9A6lka07pkGZiZWIFwUPc0k1ZidKnipqt9I9hve8oxQsWMziYZcYD+2Ebj3cGEDeV7sT rVgzXbxvDW1tN/X9/KM4mewiDALHb9cI2qNIRcvZMhPk3BlMXbfl6XxTXqR9ITiEy9XnDC14j3Ap/ NvV3l5jsfEDQmp03QfpQSExyiy8pa3zfpOaQXHtStrfa2zlHdFo5V76RQ+jL+CGxZnp6j8ymNyG/7 DLkSH8zIo/mdCSS/NSqS0gb7m9i2S3/QptRxTOi/6RdOeddfq/2zbvmRsJZehtBq4n3tBwAVJsJqk vH+dAveBssMZcl0sh3Eg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rURUa-0000000Comv-06gy; Mon, 29 Jan 2024 13:16:04 +0000 Received: from mgamail.intel.com ([192.198.163.11]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rURUX-0000000ComJ-1J3s for linux-arm-kernel@lists.infradead.org; Mon, 29 Jan 2024 13:16:02 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1706534161; x=1738070161; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=AudW6lchVMASv39ToIRXtG1kL5HX7XZ/XXM+ITeoqgg=; b=FWm/sQVEEy31CUcCWXWuPJOEBxVAXEBREtJ4UXSgW3eOKNujiOQnfGdd 7PYLNp84HKeurntDxBSwbT1d1x7LtqFJ7+/eQ1QYywFufsHfaJLYtzFU5 0ZOe1Yn+qraxdbcNh1O4rOoZb7GUQ/KKMFcN6RpnNWi66uKNXoTwshwed 8yrlfQdjsrFgt5po6mhx+Dz7hNydU3r/EMCaYrc0N2xrdpZ02X4J4hn27 K1SVzIidF1kR3L87nbJZTGbsaABobe1TqfkrlFSs7wJYbt/rdfenpCC6g ylPtepkMfLzhCuQzRhKeN/eLP1fkTOA9lyFdrsFJUwMNjGIbUiiZuxNVJ A==; X-IronPort-AV: E=McAfee;i="6600,9927,10967"; a="9618536" X-IronPort-AV: E=Sophos;i="6.05,227,1701158400"; d="scan'208";a="9618536" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by fmvoesa105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jan 2024 05:15:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.05,227,1701158400"; d="scan'208";a="3461005" Received: from choongyo-mobl.gar.corp.intel.com (HELO [10.247.122.111]) ([10.247.122.111]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Jan 2024 05:15:46 -0800 Message-ID: <8b9d62e4-be3d-4190-83e7-4058487995af@linux.intel.com> Date: Mon, 29 Jan 2024 21:15:43 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v3 2/5] net: pcs: xpcs: combine C37 SGMII AN and 2500BASEX for Intel mGbE controller Content-Language: en-US To: Serge Semin , Russell King Cc: Rajneesh Bhardwaj , David E Box , Hans de Goede , Mark Gross , Jose Abreu , Andrew Lunn , Heiner Kallweit , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , =?UTF-8?Q?Marek_Beh=C3=BAn?= , Jean Delvare , Guenter Roeck , Giuseppe Cavallaro , Alexandre Torgue , Jose Abreu , Maxime Coquelin , Richard Cochran , Philipp Zabel , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , Wong Vee Khee , Jon Hunter , Jesse Brandeburg , Revanth Kumar Uppala , Shenwei Wang , Andrey Konovalov , Jochen Henneberg , David E Box , Andrew Halaney , Simon Horman , Bartosz Golaszewski , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, platform-driver-x86@vger.kernel.org, linux-hwmon@vger.kernel.org, bpf@vger.kernel.org, Voon Wei Feng , Tan Tee Min , Michael Sit Wei Hong , Lai Peter Jun Ann References: <20230921121946.3025771-1-yong.liang.choong@linux.intel.com> <20230921121946.3025771-3-yong.liang.choong@linux.intel.com> From: Choong Yong Liang In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240129_051601_430198_9247D897 X-CRM114-Status: GOOD ( 33.85 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 26/9/2023 6:51 pm, Serge Semin wrote: > Hi Choong > > On Thu, Sep 21, 2023 at 08:19:43PM +0800, Choong Yong Liang wrote: >> From: "Tan, Tee Min" >> >> This commit introduces xpcs_sgmii_2500basex_features[] that combine >> xpcs_sgmii_features[] and xpcs_2500basex_features[] for Intel mGbE >> controller that desire to interchange the speed mode of >> 10/100/1000/2500Mbps at runtime. >> >> Also, we introduce xpcs_config_aneg_c37_sgmii_2500basex() function >> which is called by the xpcs_do_config() with the new AN mode: >> DW_SGMII_2500BASEX, and this new function will proceed next-level >> calling to perform C37 SGMII AN/2500BASEX configuration based on >> the PHY interface updated by PHY driver. > > Why do you even need all of those changes? Please thoroughly justify > because ... (see below) > >> >> Signed-off-by: Tan, Tee Min >> Signed-off-by: Choong Yong Liang >> --- >> drivers/net/pcs/pcs-xpcs.c | 72 ++++++++++++++++++++++++++++++------ >> include/linux/pcs/pcs-xpcs.h | 1 + >> 2 files changed, 62 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c >> index 4dbc21f604f2..60d90191677d 100644 >> --- a/drivers/net/pcs/pcs-xpcs.c >> +++ b/drivers/net/pcs/pcs-xpcs.c >> @@ -104,6 +104,21 @@ static const int xpcs_2500basex_features[] = { >> __ETHTOOL_LINK_MODE_MASK_NBITS, >> }; >> > >> +static const int xpcs_sgmii_2500basex_features[] = { >> + ETHTOOL_LINK_MODE_Pause_BIT, >> + ETHTOOL_LINK_MODE_Asym_Pause_BIT, >> + ETHTOOL_LINK_MODE_Autoneg_BIT, >> + ETHTOOL_LINK_MODE_10baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_10baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_100baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_100baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_2500baseX_Full_BIT, >> + ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> + __ETHTOOL_LINK_MODE_MASK_NBITS, >> +}; >> + >> static const phy_interface_t xpcs_usxgmii_interfaces[] = { >> PHY_INTERFACE_MODE_USXGMII, >> }; >> @@ -133,6 +148,12 @@ static const phy_interface_t xpcs_2500basex_interfaces[] = { >> PHY_INTERFACE_MODE_MAX, >> }; >> >> +static const phy_interface_t xpcs_sgmii_2500basex_interfaces[] = { >> + PHY_INTERFACE_MODE_SGMII, >> + PHY_INTERFACE_MODE_2500BASEX, >> + PHY_INTERFACE_MODE_MAX, >> +}; >> + > > ... these are just a combination of the > xpcs_sgmii_features[]/xpcs_sgmii_interfaces[] and > xpcs_2500basex_features[]/xpcs_2500basex_interfaces[] data which are > already supported by the generic DW XPCS code. All of these features > and interfaces are checked in the xpcs_create() method and then get to > be combined in the framework of the xpcs_validate() and > xpcs_get_interfaces() functions. And ... > >> enum { >> DW_XPCS_USXGMII, >> DW_XPCS_10GKR, >> @@ -141,6 +162,7 @@ enum { >> DW_XPCS_SGMII, >> DW_XPCS_1000BASEX, >> DW_XPCS_2500BASEX, >> + DW_XPCS_SGMII_2500BASEX, >> DW_XPCS_INTERFACE_MAX, >> }; >> >> @@ -290,6 +312,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs, >> case DW_AN_C37_SGMII: >> case DW_2500BASEX: >> case DW_AN_C37_1000BASEX: >> + case DW_SGMII_2500BASEX: >> dev = MDIO_MMD_VEND2; >> break; >> default: >> @@ -748,6 +771,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, >> if (xpcs->dev_flag == DW_DEV_TXGBE) >> ret |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL; >> > >> + /* Disable 2.5G GMII for SGMII C37 mode */ >> + ret &= ~DW_VR_MII_DIG_CTRL1_2G5_EN; > > * This is the only specific change in this patch. But it can be > * applied independently from the rest of the changes. Although I agree > * with Russel, it must be double checked since may cause regressions > * on the other platforms. > >> ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); >> if (ret < 0) >> return ret; >> @@ -848,6 +873,26 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) >> return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret); >> } >> > >> +static int xpcs_config_aneg_c37_sgmii_2500basex(struct dw_xpcs *xpcs, >> + unsigned int neg_mode, >> + phy_interface_t interface) >> +{ >> + int ret = -EOPNOTSUPP; >> + >> + switch (interface) { >> + case PHY_INTERFACE_MODE_SGMII: >> + ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); >> + break; >> + case PHY_INTERFACE_MODE_2500BASEX: >> + ret = xpcs_config_2500basex(xpcs); >> + break; >> + default: >> + break; >> + } >> + >> + return ret; >> +} >> + > > ... this is just a copy of the code which is already available in xpcs_do_config(): > > < compat = xpcs_find_compat(xpcs->id, interface); > < if (!compat) > < return -ENODEV; > < > < switch (compat->an_mode) { > < ... > < case DW_AN_C37_SGMII: > < ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); > < if (ret) > < return ret; > < break; > < ... > < case DW_2500BASEX: > < ret = xpcs_config_2500basex(xpcs); > < if (ret) > < return ret; > < break; > > So based on the passed interface xpcs_find_compat() will find a proper > compat descriptor, which an_mode field will be then utilized to call > the respective config method. Thus, unless I miss something, basically > you won't need any of the changes below and the most of the changes > above reducing the patch to just a few lines. > > -Serge(y) > >> int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, >> const unsigned long *advertising, unsigned int neg_mode) >> { >> @@ -890,6 +935,12 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, >> if (ret) >> return ret; >> break; >> + case DW_SGMII_2500BASEX: >> + ret = xpcs_config_aneg_c37_sgmii_2500basex(xpcs, neg_mode, >> + interface); >> + if (ret) >> + return ret; >> + break; >> default: >> return -1; >> } >> @@ -1114,6 +1165,11 @@ static void xpcs_get_state(struct phylink_pcs *pcs, >> } >> break; >> case DW_AN_C37_SGMII: >> + case DW_SGMII_2500BASEX: >> + /* 2500BASEX is not supported for in-band AN mode. */ >> + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) >> + break; >> + >> ret = xpcs_get_state_c37_sgmii(xpcs, state); >> if (ret) { >> pr_err("xpcs_get_state_c37_sgmii returned %pe\n", >> @@ -1266,23 +1322,17 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = { >> .num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces), >> .an_mode = DW_10GBASER, >> }, >> - [DW_XPCS_SGMII] = { >> - .supported = xpcs_sgmii_features, >> - .interface = xpcs_sgmii_interfaces, >> - .num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces), >> - .an_mode = DW_AN_C37_SGMII, >> - }, >> [DW_XPCS_1000BASEX] = { >> .supported = xpcs_1000basex_features, >> .interface = xpcs_1000basex_interfaces, >> .num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces), >> .an_mode = DW_AN_C37_1000BASEX, >> }, >> - [DW_XPCS_2500BASEX] = { >> - .supported = xpcs_2500basex_features, >> - .interface = xpcs_2500basex_interfaces, >> - .num_interfaces = ARRAY_SIZE(xpcs_2500basex_interfaces), >> - .an_mode = DW_2500BASEX, >> + [DW_XPCS_SGMII_2500BASEX] = { >> + .supported = xpcs_sgmii_2500basex_features, >> + .interface = xpcs_sgmii_2500basex_interfaces, >> + .num_interfaces = ARRAY_SIZE(xpcs_sgmii_2500basex_features), >> + .an_mode = DW_SGMII_2500BASEX, >> }, >> }; >> >> diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h >> index da3a6c30f6d2..f075d2fca54a 100644 >> --- a/include/linux/pcs/pcs-xpcs.h >> +++ b/include/linux/pcs/pcs-xpcs.h >> @@ -19,6 +19,7 @@ >> #define DW_2500BASEX 3 >> #define DW_AN_C37_1000BASEX 4 >> #define DW_10GBASER 5 >> +#define DW_SGMII_2500BASEX 6 >> >> /* device vendor OUI */ >> #define DW_OUI_WX 0x0018fc80 >> -- >> 2.25.1 >> >> Hi Serge and Russell This patch does not serve the purpose correctly, I did remove this patch and handle it properly in the new patch series. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel