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 21B8AC35274 for ; Mon, 18 Dec 2023 14:22:02 +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-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: 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=b5THWX3TlKWGJTfNZtWTCguSJlk4OPn5bjWbKa4E70A=; b=sCavCn6LqP6EdJ k0bOgf9knxj6VACqrZaElEh6cYq/WhY1hRpuX6wWqr1ps/mYJ9ohFYhj0rAmh6TZtO4mH9BrP2ik4 RWgA67kG+JXnQzB5jPF3V05NYzbOGHOMAiakzuuAxqRJ6iWdF/X9dLZJpzY+d3jkuMZsAsi+6wcjn av0uyFEja6q+bndlPotlJ8hOIvHpvF0kBRMSxnIRbaqX8x76RYxsqJ1YZeMpQ4bYU3fZ8gPLSNxiZ aOHE2vGb3oPqiXxHRmmlyPFuo0vXqmGg03X7hE/nAScRBVg/9po7Nkajj48sydcN49v3GiM8RxZDe GH3dmavjucKH3c+2st/g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rFEUv-00B1Pr-2U; Mon, 18 Dec 2023 14:21:33 +0000 Received: from relay4-d.mail.gandi.net ([217.70.183.196]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rFEUn-00B1Kf-1D for linux-arm-kernel@lists.infradead.org; Mon, 18 Dec 2023 14:21:27 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id A0A34E000B; Mon, 18 Dec 2023 14:21:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1702909281; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NQw29QHd8zTMi7v6dsnaa4P9rlD914DPy1yBSIn+0Cg=; b=K8RybCH1i6Prm5fa0N3t6j9euM7ngUB07bbFmAdpt+An6uh4uFbqfK4M7CzJFJFSDWxlum inexve6GYD5Sy6m6FBgATSjQ6GUJBNvfcUBXEO4dhq4u4LzdYr7wmyf/KwMMOe1DntNOLX S/e5Ms+/5q4DZIECN0SN9FcxietLbMQ0a4vC1DauiU7gZufwGKHwQ/MzGI8YTbHi2yfdbw 9o0vTP63H+W9xYd/5zNmbSuG4saDGvT6WpWSGauWcb8/80aDwVBTRAYip03UoSvLrxUh7n XtU1PSDuLs0lGrK3Luy6b8awrD94GB7xUoWYec5SpU2r3hNzM7GBtbStd+UfUg== Date: Mon, 18 Dec 2023 15:21:12 +0100 From: Maxime Chevallier To: Sneh Shah Cc: Vinod Koul , Bhupesh Sharma , Alexandre Torgue , Jose Abreu , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , netdev@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Andrew Halaney , kernel@quicinc.com Subject: Re: [PATCH net-next] net: stmmac: dwmac-qcom-ethqos: Add support for 2.5G SGMII Message-ID: <20231218152112.4adc5961@device-28.home> In-Reply-To: <20231218071118.21879-1-quic_snehshah@quicinc.com> References: <20231218071118.21879-1-quic_snehshah@quicinc.com> Organization: Bootlin X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-GND-Sasl: maxime.chevallier@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231218_062125_697754_CF16DCD3 X-CRM114-Status: GOOD ( 22.88 ) 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 Hi, On Mon, 18 Dec 2023 12:41:18 +0530 Sneh Shah wrote: > Serdes phy needs to operate at 2500 mode for 2.5G speed and 1000 > mode for 1G/100M/10M speed. > Added changes to configure serdes phy and mac based on link speed. > > Signed-off-by: Sneh Shah > --- > .../stmicro/stmmac/dwmac-qcom-ethqos.c | 31 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > index d3bf42d0fceb..b3a28dc19161 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c > @@ -21,6 +21,7 @@ > #define RGMII_IO_MACRO_CONFIG2 0x1C > #define RGMII_IO_MACRO_DEBUG1 0x20 > #define EMAC_SYSTEM_LOW_POWER_DEBUG 0x28 > +#define ETHQOS_MAC_AN_CTRL 0xE0 > > /* RGMII_IO_MACRO_CONFIG fields */ > #define RGMII_CONFIG_FUNC_CLK_EN BIT(30) > @@ -78,6 +79,10 @@ > #define ETHQOS_MAC_CTRL_SPEED_MODE BIT(14) > #define ETHQOS_MAC_CTRL_PORT_SEL BIT(15) > > +/*ETHQOS_MAC_AN_CTRL bits */ > +#define ETHQOS_MAC_AN_CTRL_RAN BIT(9) > +#define ETHQOS_MAC_AN_CTRL_ANE BIT(12) > + > struct ethqos_emac_por { > unsigned int offset; > unsigned int value; > @@ -109,6 +114,7 @@ struct qcom_ethqos { > unsigned int num_por; > bool rgmii_config_loopback_en; > bool has_emac_ge_3; > + unsigned int serdes_speed; Looks like you are storing SPEED_XXX definitions here, which can be negative in case of SPEED_UNKNOWN, so this should be an int. > }; > > static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset) > @@ -600,27 +606,47 @@ static int ethqos_configure_rgmii(struct qcom_ethqos *ethqos) > > static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos) > { > - int val; > - > + int val, mac_an_value; > val = readl(ethqos->mac_base + MAC_CTRL_REG); > + mac_an_value = readl(ethqos->mac_base + ETHQOS_MAC_AN_CTRL); > > switch (ethqos->speed) { > + case SPEED_2500: > + val &= ~ETHQOS_MAC_CTRL_PORT_SEL; > + rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG, > + RGMII_CONFIG2_RGMII_CLK_SEL_CFG, > + RGMII_IO_MACRO_CONFIG2); > + if (ethqos->serdes_speed != SPEED_2500) > + phy_set_speed(ethqos->serdes_phy, ethqos->speed); > + mac_an_value &= ~ETHQOS_MAC_AN_CTRL_ANE; > + break; > case SPEED_1000: > val &= ~ETHQOS_MAC_CTRL_PORT_SEL; > rgmii_updatel(ethqos, RGMII_CONFIG2_RGMII_CLK_SEL_CFG, > RGMII_CONFIG2_RGMII_CLK_SEL_CFG, > RGMII_IO_MACRO_CONFIG2); > + if (ethqos->serdes_speed != SPEED_1000) > + phy_set_speed(ethqos->serdes_phy, ethqos->speed); > + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE; > break; > case SPEED_100: > val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE; > + if (ethqos->serdes_speed != SPEED_1000) > + phy_set_speed(ethqos->serdes_phy, ethqos->speed); I understand that SGMII's serdes always runs at 1000 / 2500, but this check doesn't make much sense then, if the speed isn't 1000, then you set the serdes PHY's speed to 100, and the assignment that comes after that switch-case will also set the serdes speed to 100. Also, if the serdes PHY really needs to be configured differently for 10/100/1000, then switching from speed 1000 to speed 100 for example won't trigger a serdes PHY reconfiguration here. My guess is that you want something like : phy_set_speed(ethqos->serdes_phy, SPEED_1000) and the assignment at the end of the switch-case should be SPEED_1000/SPEED_2500 only (see the comment bellow). > + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE; > break; > case SPEED_10: > val |= ETHQOS_MAC_CTRL_PORT_SEL; > val &= ~ETHQOS_MAC_CTRL_SPEED_MODE; > + if (ethqos->serdes_speed != SPEED_1000) > + phy_set_speed(ethqos->serdes_phy, ethqos->speed); Same remark here > + mac_an_value |= ETHQOS_MAC_AN_CTRL_RAN | ETHQOS_MAC_AN_CTRL_ANE; > break; > } > > writel(val, ethqos->mac_base + MAC_CTRL_REG); > + writel(mac_an_value, ethqos->mac_base + ETHQOS_MAC_AN_CTRL); > + ethqos->serdes_speed = ethqos->speed; Although the code will behave the same, as you are storing the true serdes speed here, shouldn't it be either SPEED_1000 or SPEED_2500 ? You'll end-up storing SPEED_10 / SPEED_100 should the link use these speeds, which doesn't represent the true serdes speed. This would spare serdes reconfigurations when alternating between 10/100/1000M speeds. > return val; > } > @@ -789,6 +815,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev) > "Failed to get serdes phy\n"); > > ethqos->speed = SPEED_1000; > + ethqos->serdes_speed = SPEED_1000; > ethqos_update_link_clk(ethqos, SPEED_1000); > ethqos_set_func_clk_en(ethqos); > Thanks, Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel