Message ID | 20230214090314.2026067-8-o.rempel@pengutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2854360wrn; Tue, 14 Feb 2023 01:07:05 -0800 (PST) X-Google-Smtp-Source: AK7set/yZ0E005F/mwU5hSwfkMm2jNTNlW6wnAtNDHebdVya1o+DKovlr+3XZmVs/vm60lGk4lj1 X-Received: by 2002:a17:906:d9dd:b0:878:72f7:bd99 with SMTP id qk29-20020a170906d9dd00b0087872f7bd99mr1958795ejb.6.1676365625695; Tue, 14 Feb 2023 01:07:05 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1676365625; cv=none; d=google.com; s=arc-20160816; b=i/YiewQu9MLjCoK2IPMHXvYrgyAYJ0HEMvpj4yG32vWfKc2c39iyHEeEMU6xrmaiXM qUGyF+c4tVHFQ8bRejxOqMj5XXTjuk09+JCSoIkh2s/mkbFJ4+shVNaov3ycel2tugh2 lUGdmJcek/mrzoGygVOWoyXCjFDWnCTdwhhen5plrcKzztui1oenStc89WxCWJqTuYKA /085kBZx0BjXt/CfZNZY8Do57QGiQk7ZnCncD9bXkwNiuv2DMgRnFpsR1BlANH59gnXu Z8T6Z745HQ6otIkbeXIldWQ40F8sE+0jaOJ3ax+B0RXjEJN62qnvLKqvvxLUnF4GQBnk PWgg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=rbmh5yUNjpZV6g1TXnEwaMyJAXwP5cdXeNM56svkGzc=; b=Ca3O+sSVw+9ivANO6kCRvbl3qJmuSy4f7ZL3Mm2Pc6yY5JzMRkMMlJL6siNiqmw4vI ubT0sG7B45dYLPe/IkKPTxmJpoxyYcL9gOynJeeE7H2s2RXCKUtkVlRhTVu5jlRLDHl8 90d7Cqq5fUbCmgKaz+k+lebEDq7xN9zaArgg0IT/E8CSJAfjAPd/n8PpSW/iCmJPRWuv BaRhi+/UyAL7IEQnnC07qiqrS+m1LIWY992zTG2sPcmKOXtbphlplLynQ5orQjOAKm7b +fVEp0+8FBG9eyIRLMkxQpJuyBNEwp3rQ5P9+FBKxcPIk5SLWPO5rDF86I/Tmqqz5mhl 6MPA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 22-20020a170906025600b008777ae12842si10110936ejl.53.2023.02.14.01.06.42; Tue, 14 Feb 2023 01:07:05 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231899AbjBNJEB (ORCPT <rfc822;henrikjoshmiranda3@gmail.com> + 99 others); Tue, 14 Feb 2023 04:04:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37162 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231499AbjBNJD3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 14 Feb 2023 04:03:29 -0500 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4576BD505 for <linux-kernel@vger.kernel.org>; Tue, 14 Feb 2023 01:03:26 -0800 (PST) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <ore@pengutronix.de>) id 1pRrDa-0004UG-JN; Tue, 14 Feb 2023 10:03:18 +0100 Received: from [2a0a:edc0:0:1101:1d::ac] (helo=dude04.red.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.94.2) (envelope-from <ore@pengutronix.de>) id 1pRrDY-004qDm-G1; Tue, 14 Feb 2023 10:03:17 +0100 Received: from ore by dude04.red.stw.pengutronix.de with local (Exim 4.94.2) (envelope-from <ore@pengutronix.de>) id 1pRrDX-008V6W-Js; Tue, 14 Feb 2023 10:03:15 +0100 From: Oleksij Rempel <o.rempel@pengutronix.de> To: Andrew Lunn <andrew@lunn.ch>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Wei Fang <wei.fang@nxp.com>, Heiner Kallweit <hkallweit1@gmail.com> Cc: Oleksij Rempel <o.rempel@pengutronix.de>, kernel@pengutronix.de, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Shenwei Wang <shenwei.wang@nxp.com>, Clark Wang <xiaoning.wang@nxp.com>, NXP Linux Team <linux-imx@nxp.com> Subject: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support Date: Tue, 14 Feb 2023 10:03:14 +0100 Message-Id: <20230214090314.2026067-8-o.rempel@pengutronix.de> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230214090314.2026067-1-o.rempel@pengutronix.de> References: <20230214090314.2026067-1-o.rempel@pengutronix.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: ore@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1757796762002274078?= X-GMAIL-MSGID: =?utf-8?q?1757796762002274078?= |
Series |
make SmartEEE support controllable
|
|
Commit Message
Oleksij Rempel
Feb. 14, 2023, 9:03 a.m. UTC
Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
But this chips are used sometimes in combinations with SmartEEE capable
PHYs.
So, instead of aborting get/set_eee access on MACs without EEE support,
ask PHY if it is able to do the EEE job by using SmartEEE.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
Comments
On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote: > Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support. > But this chips are used sometimes in combinations with SmartEEE capable > PHYs. > So, instead of aborting get/set_eee access on MACs without EEE support, > ask PHY if it is able to do the EEE job by using SmartEEE. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index c73e25f8995e..00f3703db69d 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata) > struct fec_enet_private *fep = netdev_priv(ndev); > struct ethtool_eee *p = &fep->eee; > > - if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) > - return -EOPNOTSUPP; > + if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) { > + if (!netif_running(ndev)) > + return -ENETDOWN; > + > + if (!phy_has_smarteee(ndev->phydev)) > + return -EOPNOTSUPP; > + > + return phy_ethtool_get_eee(ndev->phydev, edata); > + } I can see two different ways we do this. As you have here, we modify every MAC driver which is paired to a SmartEEE PHY and have it call into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP, and there is an ndev->phydev call directly into phylib. That should make all boards with SmartEEE supported. We do this for a few calls, TS Info, and SFP module info. Either way, i don't think we need phy_has_smarteee() exposed outside of phylib. SmartEEE is supposed to be transparent to the MAC, so it should not need to care. Same as WOL, the MAC does not care if the PHY supports WoL, it should just call the APIs to get and set WoL and assume they do the right thing. What is also unclear to me is how we negotiate between EEE and SmartEEE. I assume if the MAC is EEE capable, we prefer that over SmartEEE. But i don't think i've seen anything in these patches which addresses this. Maybe we want phy_init_eee() to disable SmartEEE? Andrew
On Tue, Feb 14, 2023 at 02:26:36PM +0100, Andrew Lunn wrote: > On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote: > > Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support. > > But this chips are used sometimes in combinations with SmartEEE capable > > PHYs. > > So, instead of aborting get/set_eee access on MACs without EEE support, > > ask PHY if it is able to do the EEE job by using SmartEEE. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > > index c73e25f8995e..00f3703db69d 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata) > > struct fec_enet_private *fep = netdev_priv(ndev); > > struct ethtool_eee *p = &fep->eee; > > > > - if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) > > - return -EOPNOTSUPP; > > + if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) { > > + if (!netif_running(ndev)) > > + return -ENETDOWN; > > + > > + if (!phy_has_smarteee(ndev->phydev)) > > + return -EOPNOTSUPP; > > + > > + return phy_ethtool_get_eee(ndev->phydev, edata); > > + } > > I can see two different ways we do this. As you have here, we modify > every MAC driver which is paired to a SmartEEE PHY and have it call > into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP, > and there is an ndev->phydev call directly into phylib. That should > make all boards with SmartEEE supported. We do this for a few calls, > TS Info, and SFP module info. > > Either way, i don't think we need phy_has_smarteee() exposed outside > of phylib. SmartEEE is supposed to be transparent to the MAC, so it > should not need to care. Same as WOL, the MAC does not care if the PHY > supports WoL, it should just call the APIs to get and set WoL and > assume they do the right thing. > > What is also unclear to me is how we negotiate between EEE and > SmartEEE. I assume if the MAC is EEE capable, we prefer that over > SmartEEE. But i don't think i've seen anything in these patches which > addresses this. Maybe we want phy_init_eee() to disable SmartEEE? > > Andrew I agree with the attitude that we shouldn't expect legacy MAC driver to handle fancy features of PHY, so modifying core code instead of driver code would be a much better solution. Also, I too would like to see the case when both MAC EEE and SmartEEE are enabled, to be prevented or acknowledged, if it's actually useful for something. Other than that, patchset look fine to me, so in v2, you can put my Reviewed-By (Larysa Zaremba <larysa.zaremba@intel.com>) under any of the previous patches which stay unchanged (except the 4th one, you have to correct 'specifica' in the title).
Hi Andrew, On Tue, Feb 14, 2023 at 02:26:36PM +0100, Andrew Lunn wrote: > On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote: > I can see two different ways we do this. As you have here, we modify > every MAC driver which is paired to a SmartEEE PHY and have it call > into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP, > and there is an ndev->phydev call directly into phylib. That should > make all boards with SmartEEE supported. We do this for a few calls, > TS Info, and SFP module info. ACK. I'm working on this. > Either way, i don't think we need phy_has_smarteee() exposed outside > of phylib. SmartEEE is supposed to be transparent to the MAC, so it > should not need to care. Same as WOL, the MAC does not care if the PHY > supports WoL, it should just call the APIs to get and set WoL and > assume they do the right thing. > > What is also unclear to me is how we negotiate between EEE and > SmartEEE. I assume if the MAC is EEE capable, we prefer that over > SmartEEE. But i don't think i've seen anything in these patches which > addresses this. Maybe we want phy_init_eee() to disable SmartEEE? > > Andrew > I would prefer to not touch phy_init_eee(). At least not in this patch set. With this function we have following situation: drivers/net/dsa/b53/b53_common.c:2173: This driver will enable EEE if link partners agreed to do so. But never disable it, if link partner decided to turn off EEE or other link partner without EEE support was attached. drivers/net/dsa/mt7530.c:2862: Seems to be ok. drivers/net/ethernet/broadcom/genet/bcmgenet.c:1353: EEE is not enabled link up. It will work only with ethtool and only if link was already active. drivers/net/ethernet/freescale/fec_main.c:3078: EEE is not enabled link up. It will work only with ethtool and only if link was already active. drivers/net/ethernet/marvell/mvneta.c:4225: Seems to be ok. drivers/net/ethernet/microchip/lan743x_ethtool.c:1115: EEE is not enabled link up. It will work only with ethtool and only if link was already active. drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c:130: EEE will be enabled on open, but only if PHY was fast enough to detect the link. drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:1084: May partially work, by driver has many reason to not enable EEE, even if PHY will continue to advertise it. In all broken or partially broken cases, the PHY will continue to advertise EEE support. And the link partner will even potentially try to make use of it. No Idea if this works good. Hm.. I need to admit, EEE should not be advertised by default. Only if MAC driver calls something like phy_support_eee(), we should start doing it. In case some Intel Ethernet drivers developer read this. There are some issue too. For example: net/ethernet/intel/igb/igb_ethtool.c igb_get_eee() if (adapter->link_duplex == HALF_DUPLEX) { edata->eee_enabled = false; edata->eee_active = false; edata->tx_lpi_enabled = false; edata->advertised &= ~edata->advertised; } This part of code will make EEE permanently disabled if link partner switched to HALF duplex and then back to full duplex. It can be reproduce with following steps: system B: ethtool -s end0 advertise 0x008 system A: ethtool --show-eee enp1s0f1 EEE status: enabled - active system B: ethtool -s end0 advertise 0x004 system A: ethtool --show-eee enp1s0f1 EEE status: disabled system B: ethtool -s end0 advertise 0x008 ethtool --show-eee enp1s0f1 EEE status: disabled drivers/net/ethernet/intel/igc/igc_ethtool.c is affected as well.
> I would prefer to not touch phy_init_eee(). At least not in this patch > set. With this function we have following situation: We have a complete mess :-( I spent yesterday re-writing the MAC driver side of EEE. Most get it completely wrong, as you point out. So i changed the API a bit, making it more like other negotiated things, so i hope developers will get it correct in the future. I will post an RFC/RFT series soon. > Hm.. I need to admit, EEE should not be advertised by default. Only > if MAC driver calls something like phy_support_eee(), we should start doing it. This i've not looked at yet. But i agree. Andrew
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index c73e25f8995e..00f3703db69d 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata) struct fec_enet_private *fep = netdev_priv(ndev); struct ethtool_eee *p = &fep->eee; - if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) - return -EOPNOTSUPP; + if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) { + if (!netif_running(ndev)) + return -ENETDOWN; + + if (!phy_has_smarteee(ndev->phydev)) + return -EOPNOTSUPP; + + return phy_ethtool_get_eee(ndev->phydev, edata); + } if (!netif_running(ndev)) return -ENETDOWN; @@ -3123,8 +3130,15 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata) struct ethtool_eee *p = &fep->eee; int ret = 0; - if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) - return -EOPNOTSUPP; + if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) { + if (!netif_running(ndev)) + return -ENETDOWN; + + if (!phy_has_smarteee(ndev->phydev)) + return -EOPNOTSUPP; + + return phy_ethtool_set_eee(ndev->phydev, edata); + } if (!netif_running(ndev)) return -ENETDOWN;