Message ID | 20240109205223.40219-1-wsa+renesas@sang-engineering.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2411:b0:101:2151:f287 with SMTP id m17csp396138dyi; Tue, 9 Jan 2024 12:53:24 -0800 (PST) X-Google-Smtp-Source: AGHT+IGaKDfWN2laBFFt/vFYNdqM83/dcpSn7bqYWW3PXxr1bKvmbbwM0D/2o2nAHAqXQjXxzGoo X-Received: by 2002:a17:907:84c5:b0:a2b:298a:4bab with SMTP id oh5-20020a17090784c500b00a2b298a4babmr20395ejc.95.1704833603864; Tue, 09 Jan 2024 12:53:23 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1704833603; cv=none; d=google.com; s=arc-20160816; b=kVweoDNPe5QIOD+XWgtbgZDVgSeB5CyK60QyRBXlkDHf9Fdse/jgPaRrjUXhZ6PR2s aYT8VE/Ga1Lg2pxAZIP4RYcwpA7VuFdrsBxBwdOTi/peXNvSg/VSWO5rkDo3YjqCpsnk dYFND2U4ZVA43rpokwcY4op/2Gu0vbYADtlA9QLezarAQXkbqmpFQ2Sr9UlAWFLHPukM NeLpquWORZYjfTm3O/+IsUFgzX5EjYo4VZWtDRgjhdA1atBsjIRDnPsoUdyyYj8f/sVv V0+AAkiWd3XKLrYq/EAAqzDxFuo9whEZGWp/tkThotAwDYM8XRZNLpMlZJZEr2AhYaq8 OVOw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=nAIjW3Vd6AO7npgt9Xb/HqJrmYFkWeDl3YQgZw/BnJA=; fh=jBEbAODqrgdKK1JxuHCSX8HOPapmLla0AeCIag1F1oE=; b=DAmCO9Y9aq+9bSCuQABEEnfumm0wPWNjaha0lVY1g+xcOFFVLtfMaCJgGZmvyq6IPH yXTQD30HzYX/LoqtAyvGHsKSeTeN2Kc8uDsCGBE7Eu0ppwqjGroNolimkrxCXAq6YI0v FXg3Bt/wSi3Vuh0oP830SS7zAQQy9xNEf3JBK/t38uUPwwV7C4OYlogeWUSLvvvV/run ms/s+3pr760Oyt9GgwRu2JrCAZjesoQ6Pd6g+1GUsRsQhxH4YIgiP9mYpChZDfANx6qx w/CuzKnAqNo4GRVkOGmPYDrzbRCm5DZWPjNAoyEK+M8NbwG0hrF6H37mU0LAmqYS+TgT M8Mg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sang-engineering.com header.s=k1 header.b="aoRbrk/G"; spf=pass (google.com: domain of linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org" Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id br24-20020a170906d15800b00a26ab41d1bfsi1059500ejb.120.2024.01.09.12.53.23 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Jan 2024 12:53:23 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@sang-engineering.com header.s=k1 header.b="aoRbrk/G"; spf=pass (google.com: domain of linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-21402-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 795471F25FF4 for <ouuuleilei@gmail.com>; Tue, 9 Jan 2024 20:53:23 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 2812B3DB92; Tue, 9 Jan 2024 20:53:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=sang-engineering.com header.i=@sang-engineering.com header.b="aoRbrk/G" Received: from mail.zeus03.de (www.zeus03.de [194.117.254.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1CA253D0B9 for <linux-kernel@vger.kernel.org>; Tue, 9 Jan 2024 20:52:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=sang-engineering.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sang-engineering.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= sang-engineering.com; h=from:to:cc:subject:date:message-id :mime-version:content-transfer-encoding; s=k1; bh=nAIjW3Vd6AO7np gt9Xb/HqJrmYFkWeDl3YQgZw/BnJA=; b=aoRbrk/G0kEMU3fMqSP/zuBGxnipqQ W/IL/0pu0zKEh5gwSrc2625INKWMeAdvM0BCmicDgZkqT3l+FOGPeNwdU4Paw4cJ FCWb6LG2qB2FiCHDinX3nkuJ5VXk64DDa44Rfr3kTw+GDaXvtTPHBk2VQhuSRgCW rHr6S4uRQ9fggP+cWqWU7BGb4d1HbYj5CXn345uOHizdf26IMVFlwAKNZAwXUT5b vXauhpOxhixgq0zt62ge8Az5TABXH90s8QTQDrRFWP+sal4V3FKBhJE7N9ccYd3x BqRbakLoAeT0c1ILGMmOZJ4TMKw8I6niTxRbs9Xybfr5HiIfMMG32QHQ== Received: (qmail 354471 invoked from network); 9 Jan 2024 21:52:45 +0100 Received: by mail.zeus03.de with ESMTPSA (TLS_AES_256_GCM_SHA384 encrypted, authenticated); 9 Jan 2024 21:52:45 +0100 X-UD-Smtp-Session: l3s3148p1@Ly0Of4kOTpRehhrQ From: Wolfram Sang <wsa+renesas@sang-engineering.com> To: linux-renesas-soc@vger.kernel.org Cc: netdev@vger.kernel.org, Philippe Schenker <philippe.schenker@toradex.com>, Francesco Dolcini <francesco.dolcini@toradex.com>, Wolfram Sang <wsa+renesas@sang-engineering.com>, Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org Subject: [RFC PATCH] net: phy: micrel: reset KSZ9x31 when resuming Date: Tue, 9 Jan 2024 21:52:22 +0100 Message-Id: <20240109205223.40219-1-wsa+renesas@sang-engineering.com> X-Mailer: git-send-email 2.39.2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1787647600958395793 X-GMAIL-MSGID: 1787647600958395793 |
Series |
[RFC] net: phy: micrel: reset KSZ9x31 when resuming
|
|
Commit Message
Wolfram Sang
Jan. 9, 2024, 8:52 p.m. UTC
On a Renesas Ebisu board, the KSZ9031 PHY is stalled after resuming if
the interface has not been brought up before suspending. If it had been
brought up before, phylib ensures that reset is asserted before
suspending. But if it had never been brought up, there is no instance
which could ensure that reset is asserted. And upon resume, the PHY is
in an unknown state without reset being asserted. To bring it back to a
known state, simply reset it when it is about to be resumed.
This likely also helps another issue [1] where a KSZ9131 can be powered
using regulators. After switching power on again in resume, a reset is
also needed.
[1] https://patchwork.kernel.org/project/netdevbpf/patch/20211214121638.138784-4-philippe.schenker@toradex.com/
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
This is a different solution to a problem I already tried to solve
here[2]. Back then, I added code to the MAC, but I now believe it should
be solved on PHY level. We never saw the problem with other PHYs.
Looking at [1], it seems that KSZ9x31 is also sensitive to being
powered down without reset being asserted. I know it is not a perfect
proof, but I guess these assumptions are all we have.
Philippe, Francesco: do you still have access to machines with this
issue? Could you kindly test if so?
Patch is based on 6.7. Looking forward for comments if this is the
correct layer to tackle the problem. Thanks!
[2] https://lore.kernel.org/all/20230321103357.18940-1-wsa+renesas@sang-engineering.com/
drivers/net/phy/micrel.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
Comments
> +static int ksz9x31_resume(struct phy_device *phydev) > +{ > + phy_device_reset(phydev, 1); > + phy_device_reset(phydev, 0); > + > + return kszphy_resume(phydev); > +} > + > static int kszphy_probe(struct phy_device *phydev) > { > const struct kszphy_type *type = phydev->drv->driver_data; > @@ -4778,7 +4786,7 @@ static struct phy_driver ksphy_driver[] = { > .get_strings = kszphy_get_strings, > .get_stats = kszphy_get_stats, > .suspend = kszphy_suspend, > - .resume = kszphy_resume, > + .resume = ksz9x31_resume, Humm, i'm not so sure about this. phy_resume() is called by mdio_bus_phy_resume(). That first does a call to phy_init_hw(), which will perform a soft reset on the PHY, call the drivers config_init() callback, and the config_intr() callback. Then it calls phy_resume(). Does phy_resume() hitting it with a reset clear out the configuration done by config_init() and the interrupt configuration performed by config_intr()? Andrew
- Philippe, email address is no longer valid. On Tue, Jan 09, 2024 at 09:52:22PM +0100, Wolfram Sang wrote: > On a Renesas Ebisu board, the KSZ9031 PHY is stalled after resuming if > the interface has not been brought up before suspending. If it had been > brought up before, phylib ensures that reset is asserted before > suspending. But if it had never been brought up, there is no instance > which could ensure that reset is asserted. And upon resume, the PHY is > in an unknown state without reset being asserted. To bring it back to a > known state, simply reset it when it is about to be resumed. > > This likely also helps another issue [1] where a KSZ9131 can be powered > using regulators. After switching power on again in resume, a reset is > also needed. > > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211214121638.138784-4-philippe.schenker@toradex.com/ > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > This is a different solution to a problem I already tried to solve > here[2]. Back then, I added code to the MAC, but I now believe it should > be solved on PHY level. We never saw the problem with other PHYs. > Looking at [1], it seems that KSZ9x31 is also sensitive to being > powered down without reset being asserted. I know it is not a perfect > proof, but I guess these assumptions are all we have. > > Philippe, Francesco: do you still have access to machines with this > issue? Could you kindly test if so? I have access, however - Philippe is long gone from Toradex and he was the one looking into this topic - we did solve the issue in a different way, e.g. we no longer power-off the phy in suspend Therefore is not straightforward to provide valuable feedback to you now. > > Patch is based on 6.7. Looking forward for comments if this is the > correct layer to tackle the problem. Thanks! > > > [2] https://lore.kernel.org/all/20230321103357.18940-1-wsa+renesas@sang-engineering.com/ > > drivers/net/phy/micrel.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 08e3915001c3..c38d7982c06c 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1984,6 +1984,14 @@ static int kszphy_resume(struct phy_device *phydev) > return 0; > } > > +static int ksz9x31_resume(struct phy_device *phydev) > +{ > + phy_device_reset(phydev, 1); > + phy_device_reset(phydev, 0); Is something like that fine? Don't we need to reconfigure the ethernet phy completely on resume if we do reset it? kszphy_config_reset() is taking care of something, but I think that the phy being reset on resume is not handled correctly. Francesco
Hi Francesco, > - Philippe, email address is no longer valid. OK, thanks for the heads up. > Therefore is not straightforward to provide valuable feedback to you > now. Thanks for answering anyhow. > > +static int ksz9x31_resume(struct phy_device *phydev) > > +{ > > + phy_device_reset(phydev, 1); > > + phy_device_reset(phydev, 0); > > Is something like that fine? > Don't we need to reconfigure the ethernet phy completely on resume > if we do reset it? kszphy_config_reset() is taking care of something, > but I think that the phy being reset on resume is not handled > correctly. If the interface is up before suspending, then suspend will assert the reset-line. If the interface is disabled before suspending, then close will assert the reset line. Deassertion will happen on resume/open. So, unless I am overlooking something, my code does not really add something new. It only makes sure that the reset line always gets asserted before deasserting. Because in the case that the interface has never been up before, there is no instance which could assert reset. Or, at least, I could not find one. Makes sense? Tests work fine here, at least. All the best, Wolfram
> Does phy_resume() hitting it with a reset clear out the configuration > done by config_init() and the interrupt configuration performed by > config_intr()? Hmm, I should have answered your mail first. Instrumentation says you are correct. Back to the drawing board :/
On Wed, Jan 17, 2024 at 02:33:53PM +0100, Wolfram Sang wrote: > > > +static int ksz9x31_resume(struct phy_device *phydev) > > > +{ > > > + phy_device_reset(phydev, 1); > > > + phy_device_reset(phydev, 0); > > > > Is something like that fine? > > Don't we need to reconfigure the ethernet phy completely on resume > > if we do reset it? kszphy_config_reset() is taking care of something, > > but I think that the phy being reset on resume is not handled > > correctly. > > If the interface is up before suspending, then suspend will assert the > reset-line. If the interface is disabled before suspending, then close > will assert the reset line. Deassertion will happen on resume/open. > > So, unless I am overlooking something, my code does not really add > something new. It only makes sure that the reset line always gets > asserted before deasserting. Because in the case that the interface has > never been up before, there is no instance which could assert reset. Or, > at least, I could not find one. > > Makes sense? Tests work fine here, at least. What I do not know, is what happen to any configuration that was done to the phy before. What if you have disabled gigabit ethernet from auto negotiation before suspend, it will be enabled again after the phy get out of reset. Is the ethernet phy subsystem taking care of this? Ensuring that this configuration is restored into the phy? I was starting to debug something around this a few days ago, with the difference that in that case the reset was asserted/de-asserted by the hardware and the end results was not really the expected one ... Francesco
On Tue, Feb 06, 2024 at 06:26:45PM +0100, Francesco Dolcini wrote: > On Wed, Jan 17, 2024 at 02:33:53PM +0100, Wolfram Sang wrote: > > > > +static int ksz9x31_resume(struct phy_device *phydev) > > > > +{ > > > > + phy_device_reset(phydev, 1); > > > > + phy_device_reset(phydev, 0); > > > > > > Is something like that fine? > > > Don't we need to reconfigure the ethernet phy completely on resume > > > if we do reset it? kszphy_config_reset() is taking care of something, > > > but I think that the phy being reset on resume is not handled > > > correctly. > > > > If the interface is up before suspending, then suspend will assert the > > reset-line. If the interface is disabled before suspending, then close > > will assert the reset line. Deassertion will happen on resume/open. > > > > So, unless I am overlooking something, my code does not really add > > something new. It only makes sure that the reset line always gets > > asserted before deasserting. Because in the case that the interface has > > never been up before, there is no instance which could assert reset. Or, > > at least, I could not find one. > > > > Makes sense? Tests work fine here, at least. > > What I do not know, is what happen to any configuration that was done to > the phy before. I'm assuming here WoL was not enabled, so the PHY did actually suspend. mdio_bus_phy_suspend() calls phy_stop_machine() which will set the state of the PHY to UP. During resume mdio_bus_phy_resume() calls phy_init_hw(). That should do a soft reset, call the config_init() callback, and configure interrupts. After that phy_resume() is called and then phy_state_machine(). Do to setting the state to UP, the state machine will kick off auto-negotiation, which will cause any auto-neg parameters to be written to the PHY. > What if you have disabled gigabit ethernet from auto negotiation before > suspend, it will be enabled again after the phy get out of reset. If you have set in fixed mode, the wrongly named phy_config_aneg() will set the fixed modes, same as it would set the auto-neg modes. So they should be preserved over suspend/resume. Andrew
On Tue, Feb 06, 2024 at 06:57:37PM +0100, Andrew Lunn wrote: > On Tue, Feb 06, 2024 at 06:26:45PM +0100, Francesco Dolcini wrote: > > On Wed, Jan 17, 2024 at 02:33:53PM +0100, Wolfram Sang wrote: > > > Makes sense? Tests work fine here, at least. > > > > What I do not know, is what happen to any configuration that was done to > > the phy before. > > I'm assuming here WoL was not enabled, so the PHY did actually > suspend. > > mdio_bus_phy_suspend() calls phy_stop_machine() which will set the > state of the PHY to UP. > > During resume mdio_bus_phy_resume() calls phy_init_hw(). That should > do a soft reset, call the config_init() callback, and configure > interrupts. After that phy_resume() is called and then > phy_state_machine(). Do to setting the state to UP, the state machine > will kick off auto-negotiation, which will cause any auto-neg > parameters to be written to the PHY. > > > What if you have disabled gigabit ethernet from auto negotiation before > > suspend, it will be enabled again after the phy get out of reset. > > If you have set in fixed mode, the wrongly named phy_config_aneg() > will set the fixed modes, same as it would set the auto-neg modes. So > they should be preserved over suspend/resume. Thanks for the detailed explanation Andrew. What if the configuration was done using ethtool? Francesco
> What if the configuration was done using ethtool?
The MAC driver when handling ethtool requests should call
phy_ethtool_ksettings_set(). That manipulates phydev->advertising,
phydev->autoneg etc. These are the values passed by phy_config_aneg()
to the PHY driver.
Andrew
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 08e3915001c3..c38d7982c06c 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1984,6 +1984,14 @@ static int kszphy_resume(struct phy_device *phydev) return 0; } +static int ksz9x31_resume(struct phy_device *phydev) +{ + phy_device_reset(phydev, 1); + phy_device_reset(phydev, 0); + + return kszphy_resume(phydev); +} + static int kszphy_probe(struct phy_device *phydev) { const struct kszphy_type *type = phydev->drv->driver_data; @@ -4778,7 +4786,7 @@ static struct phy_driver ksphy_driver[] = { .get_strings = kszphy_get_strings, .get_stats = kszphy_get_stats, .suspend = kszphy_suspend, - .resume = kszphy_resume, + .resume = ksz9x31_resume, .cable_test_start = ksz9x31_cable_test_start, .cable_test_get_status = ksz9x31_cable_test_get_status, }, { @@ -4851,7 +4859,7 @@ static struct phy_driver ksphy_driver[] = { .get_strings = kszphy_get_strings, .get_stats = kszphy_get_stats, .suspend = kszphy_suspend, - .resume = kszphy_resume, + .resume = ksz9x31_resume, .cable_test_start = ksz9x31_cable_test_start, .cable_test_get_status = ksz9x31_cable_test_get_status, .get_features = ksz9477_get_features,