* [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration
@ 2020-04-29 9:03 Badel, Laurent
2020-04-29 12:00 ` Badel, Laurent
2020-04-29 17:06 ` Heiner Kallweit
0 siblings, 2 replies; 6+ messages in thread
From: Badel, Laurent @ 2020-04-29 9:03 UTC (permalink / raw)
To: fugang.duan, netdev, andrew, f.fainelli, hkallweit1, linux,
richard.leitner, davem, alexander.levin, gregkh
Cc: Quette, Arnaud
Description: this patch adds a reset of the PHY in phy_init_hw()
for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag.
Rationale: due to the PHY reset reverting the interrupt mask to default,
it is necessary to either perform the reset before PHY configuration,
or re-configure the PHY after reset. This patch implements the former
as it is simpler and more generic.
Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add phy_reset_after_clk_enable() support")
Signed-off-by: Laurent Badel <laurentbadel@eaton.com>
---
drivers/net/phy/phy_device.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 28e3c5c0e..2cc511364 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev)
{
int ret = 0;
- /* Deassert the reset signal */
- phy_device_reset(phydev, 0);
+ /* Deassert the reset signal
+ * If the PHY needs a reset, do it now
+ */
+ if (!phy_reset_after_clk_enable(phydev))
+ phy_device_reset(phydev, 0);
if (!phydev->drv)
return 0;
--
2.17.1
-----------------------------
Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland
-----------------------------
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration 2020-04-29 9:03 [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration Badel, Laurent @ 2020-04-29 12:00 ` Badel, Laurent 2020-04-29 17:06 ` Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Badel, Laurent @ 2020-04-29 12:00 UTC (permalink / raw) To: fugang.duan, netdev, andrew, f.fainelli, hkallweit1, linux, richard.leitner, davem, alexander.levin, gregkh Cc: Quette, Arnaud Description: this patch adds a reset of the PHY in phy_init_hw() for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag. Rationale: due to the PHY reset reverting the interrupt mask to default, it is necessary to either perform the reset before PHY configuration, or re-configure the PHY after reset. This patch implements the former as it is simpler and more generic. Fixes: 1b0a83ac04e3 ("net: fec: add phy_reset_after_clk_enable() support") Signed-off-by: Laurent Badel <laurentbadel@eaton.com> --- drivers/net/phy/phy_device.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 28e3c5c0e..2cc511364 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) { int ret = 0; - /* Deassert the reset signal */ - phy_device_reset(phydev, 0); + /* Deassert the reset signal + * If the PHY needs a reset, do it now + */ + if (!phy_reset_after_clk_enable(phydev)) + phy_device_reset(phydev, 0); if (!phydev->drv) return 0; -- 2.17.1 > ----------------------------- Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland ----------------------------- -----Original Message----- > From: Badel, Laurent > Sent: Wednesday, April 29, 2020 11:04 AM > To: 'fugang.duan@nxp.com' <fugang.duan@nxp.com>; > 'netdev@vger.kernel.org' <netdev@vger.kernel.org>; 'andrew@lunn.ch' > <andrew@lunn.ch>; 'f.fainelli@gmail.com' <f.fainelli@gmail.com>; > 'hkallweit1@gmail.com' <hkallweit1@gmail.com>; 'linux@armlinux.org.uk' > <linux@armlinux.org.uk>; 'richard.leitner@skidata.com' > <richard.leitner@skidata.com>; 'davem@davemloft.net' > <davem@davemloft.net>; 'alexander.levin@microsoft.com' > <alexander.levin@microsoft.com>; 'gregkh@linuxfoundation.org' > <gregkh@linuxfoundation.org> > Cc: Quette, Arnaud <ArnaudQuette@Eaton.com> > Subject: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt > configuration > > Description: this patch adds a reset of the PHY in phy_init_hw() for PHY > drivers bearing the PHY_RST_AFTER_CLK_EN flag. > > Rationale: due to the PHY reset reverting the interrupt mask to default, it is > necessary to either perform the reset before PHY configuration, or re- > configure the PHY after reset. This patch implements the former as it is > simpler and more generic. > > Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add > phy_reset_after_clk_enable() support") > Signed-off-by: Laurent Badel <laurentbadel@eaton.com> > > --- > drivers/net/phy/phy_device.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 28e3c5c0e..2cc511364 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) { > int ret = 0; > > - /* Deassert the reset signal */ > - phy_device_reset(phydev, 0); > + /* Deassert the reset signal > + * If the PHY needs a reset, do it now > + */ > + if (!phy_reset_after_clk_enable(phydev)) > + phy_device_reset(phydev, 0); > > if (!phydev->drv) > return 0; > -- > 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration 2020-04-29 9:03 [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration Badel, Laurent 2020-04-29 12:00 ` Badel, Laurent @ 2020-04-29 17:06 ` Heiner Kallweit 2020-04-30 9:29 ` [EXTERNAL] " Badel, Laurent 1 sibling, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2020-04-29 17:06 UTC (permalink / raw) To: Badel, Laurent, fugang.duan, netdev, andrew, f.fainelli, linux, richard.leitner, davem, alexander.levin, gregkh Cc: Quette, Arnaud On 29.04.2020 11:03, Badel, Laurent wrote: > Description: this patch adds a reset of the PHY in phy_init_hw() > for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag. > > Rationale: due to the PHY reset reverting the interrupt mask to default, > it is necessary to either perform the reset before PHY configuration, > or re-configure the PHY after reset. This patch implements the former > as it is simpler and more generic. > > Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add phy_reset_after_clk_enable() support") > Signed-off-by: Laurent Badel <laurentbadel@eaton.com> > > --- > drivers/net/phy/phy_device.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 28e3c5c0e..2cc511364 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) > { > int ret = 0; > > - /* Deassert the reset signal */ > - phy_device_reset(phydev, 0); > + /* Deassert the reset signal > + * If the PHY needs a reset, do it now > + */ > + if (!phy_reset_after_clk_enable(phydev)) If reset is asserted when entering phy_init_hw(), then phy_reset_after_clk_enable() basically becomes a no-op. Still it should work as expected due to the reset signal being deasserted. It would be worth describing in the comment why the code still works in this case. > + phy_device_reset(phydev, 0); > > if (!phydev->drv) > return 0; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration 2020-04-29 17:06 ` Heiner Kallweit @ 2020-04-30 9:29 ` Badel, Laurent 2020-05-19 8:41 ` Jörg Willmann 0 siblings, 1 reply; 6+ messages in thread From: Badel, Laurent @ 2020-04-30 9:29 UTC (permalink / raw) To: Heiner Kallweit, fugang.duan, netdev, andrew, f.fainelli, linux, richard.leitner, davem, alexander.levin, gregkh Cc: Quette, Arnaud > ----------------------------- Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland ----------------------------- -----Original Message----- > From: Heiner Kallweit <hkallweit1@gmail.com> > Sent: Wednesday, April 29, 2020 7:06 PM > To: Badel, Laurent <LaurentBadel@eaton.com>; fugang.duan@nxp.com; > netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com; > linux@armlinux.org.uk; richard.leitner@skidata.com; > davem@davemloft.net; alexander.levin@microsoft.com; > gregkh@linuxfoundation.org > Cc: Quette, Arnaud <ArnaudQuette@Eaton.com> > Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before > interrupt configuration > > On 29.04.2020 11:03, Badel, Laurent wrote: > > Description: this patch adds a reset of the PHY in phy_init_hw() > > for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag. > > > > Rationale: due to the PHY reset reverting the interrupt mask to default, > > it is necessary to either perform the reset before PHY configuration, > > or re-configure the PHY after reset. This patch implements the former > > as it is simpler and more generic. > > > > Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add > phy_reset_after_clk_enable() support") > > Signed-off-by: Laurent Badel <laurentbadel@eaton.com> > > > > --- > > drivers/net/phy/phy_device.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index 28e3c5c0e..2cc511364 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) > > { > > int ret = 0; > > > > - /* Deassert the reset signal */ > > - phy_device_reset(phydev, 0); > > + /* Deassert the reset signal > > + * If the PHY needs a reset, do it now > > + */ > > + if (!phy_reset_after_clk_enable(phydev)) > > If reset is asserted when entering phy_init_hw(), then > phy_reset_after_clk_enable() basically becomes a no-op. > Still it should work as expected due to the reset signal being > deasserted. It would be worth describing in the comment > why the code still works in this case. > Thank you for the comment, this is a very good point. I will make sure to include some description when resubmitting. I had previously tested this and what I saw was that the first time you bring up the interface, the reset is not asserted so that phy_reset_after_clk_enable() is effective. The subsequent times the interface is brought up, the reset is already asserted when entering phy_init_hw(), so that it becomes a no-op as you correctly pointed out. However, that didn't cause any problem on my board, presumably because in that case the clock is already running when the PHY comes out of reset. I will re-test this carefully against the 'net' tree, though, before coming to conclusions. > > + phy_device_reset(phydev, 0); > > > > if (!phydev->drv) > > return 0; > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration 2020-04-30 9:29 ` [EXTERNAL] " Badel, Laurent @ 2020-05-19 8:41 ` Jörg Willmann 2020-10-26 10:21 ` Badel, Laurent 0 siblings, 1 reply; 6+ messages in thread From: Jörg Willmann @ 2020-05-19 8:41 UTC (permalink / raw) To: Badel, Laurent Cc: Heiner Kallweit, fugang.duan, netdev, andrew, f.fainelli, linux, richard.leitner, davem, alexander.levin, gregkh, Quette, Arnaud [-- Attachment #1: Type: text/plain, Size: 3274 bytes --] On Thu, 30 Apr 2020, Badel, Laurent wrote: > -----Original Message----- >> From: Heiner Kallweit <hkallweit1@gmail.com> >> Sent: Wednesday, April 29, 2020 7:06 PM >> To: Badel, Laurent <LaurentBadel@eaton.com>; fugang.duan@nxp.com; >> netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com; >> linux@armlinux.org.uk; richard.leitner@skidata.com; >> davem@davemloft.net; alexander.levin@microsoft.com; >> gregkh@linuxfoundation.org >> Cc: Quette, Arnaud <ArnaudQuette@Eaton.com> >> Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before >> interrupt configuration >> >> On 29.04.2020 11:03, Badel, Laurent wrote: >>> Description: this patch adds a reset of the PHY in phy_init_hw() >>> for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag. >>> >>> Rationale: due to the PHY reset reverting the interrupt mask to default, >>> it is necessary to either perform the reset before PHY configuration, >>> or re-configure the PHY after reset. This patch implements the former >>> as it is simpler and more generic. >>> >>> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add >> phy_reset_after_clk_enable() support") >>> Signed-off-by: Laurent Badel <laurentbadel@eaton.com> >>> >>> --- >>> drivers/net/phy/phy_device.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >>> index 28e3c5c0e..2cc511364 100644 >>> --- a/drivers/net/phy/phy_device.c >>> +++ b/drivers/net/phy/phy_device.c >>> @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) >>> { >>> int ret = 0; >>> >>> - /* Deassert the reset signal */ >>> - phy_device_reset(phydev, 0); >>> + /* Deassert the reset signal >>> + * If the PHY needs a reset, do it now >>> + */ >>> + if (!phy_reset_after_clk_enable(phydev)) >> >> If reset is asserted when entering phy_init_hw(), then >> phy_reset_after_clk_enable() basically becomes a no-op. >> Still it should work as expected due to the reset signal being >> deasserted. It would be worth describing in the comment >> why the code still works in this case. >> > > Thank you for the comment, this is a very good point. > I will make sure to include some description when resubmitting. > I had previously tested this and what I saw was that the first > time you bring up the interface, the reset is not asserted so > that phy_reset_after_clk_enable() is effective. > The subsequent times the interface is brought up, the reset > is already asserted when entering phy_init_hw(), so that > it becomes a no-op as you correctly pointed out. However, > that didn't cause any problem on my board, presumably because > in that case the clock is already running when the PHY comes > out of reset. > I will re-test this carefully against the 'net' tree, though, > before coming to conclusions. > I have two additional things to take into account: * phy_reset_after_clk_enable() shoulnd't be longer called that way since it is now misleading -> the phy is no longer reset after clock enable but during hw_init() * how about fec_resume()? I don't think hw_init() is called then and so phy_reset_after_clk_enable() will no longer be called. >>> + phy_device_reset(phydev, 0); >>> >>> if (!phydev->drv) >>> return 0; >>> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration 2020-05-19 8:41 ` Jörg Willmann @ 2020-10-26 10:21 ` Badel, Laurent 0 siblings, 0 replies; 6+ messages in thread From: Badel, Laurent @ 2020-10-26 10:21 UTC (permalink / raw) To: Jörg Willmann Cc: Heiner Kallweit, fugang.duan, netdev, andrew, f.fainelli, linux, richard.leitner, davem, alexander.levin, gregkh, Quette, Arnaud > ----------------------------- Eaton Industries Manufacturing GmbH ~ Registered place of business: Route de la Longeraie 7, 1110, Morges, Switzerland ----------------------------- -----Original Message----- > From: Jörg Willmann <joe@clnt.de> > Sent: Tuesday, May 19, 2020 10:41 AM > To: Badel, Laurent <LaurentBadel@eaton.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com>; fugang.duan@nxp.com; > netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com; > linux@armlinux.org.uk; richard.leitner@skidata.com; > davem@davemloft.net; alexander.levin@microsoft.com; > gregkh@linuxfoundation.org; Quette, Arnaud <ArnaudQuette@Eaton.com> > Subject: RE: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before > interrupt configuration > > > > On Thu, 30 Apr 2020, Badel, Laurent wrote: > > > -----Original Message----- > >> From: Heiner Kallweit <hkallweit1@gmail.com> > >> Sent: Wednesday, April 29, 2020 7:06 PM > >> To: Badel, Laurent <LaurentBadel@eaton.com>; fugang.duan@nxp.com; > >> netdev@vger.kernel.org; andrew@lunn.ch; f.fainelli@gmail.com; > >> linux@armlinux.org.uk; richard.leitner@skidata.com; > >> davem@davemloft.net; alexander.levin@microsoft.com; > >> gregkh@linuxfoundation.org > >> Cc: Quette, Arnaud <ArnaudQuette@Eaton.com> > >> Subject: [EXTERNAL] Re: [PATCH 2/2] Reset PHY in phy_init_hw() before > >> interrupt configuration > >> > >> On 29.04.2020 11:03, Badel, Laurent wrote: > >>> Description: this patch adds a reset of the PHY in phy_init_hw() > >>> for PHY drivers bearing the PHY_RST_AFTER_CLK_EN flag. > >>> > >>> Rationale: due to the PHY reset reverting the interrupt mask to > >>> default, it is necessary to either perform the reset before PHY > >>> configuration, or re-configure the PHY after reset. This patch > >>> implements the former as it is simpler and more generic. > >>> > >>> Fixes: 1b0a83ac04e383e3bed21332962b90710fcf2828 ("net: fec: add > >> phy_reset_after_clk_enable() support") > >>> Signed-off-by: Laurent Badel <laurentbadel@eaton.com> > >>> > >>> --- > >>> drivers/net/phy/phy_device.c | 7 +++++-- > >>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/phy/phy_device.c > >>> b/drivers/net/phy/phy_device.c index 28e3c5c0e..2cc511364 100644 > >>> --- a/drivers/net/phy/phy_device.c > >>> +++ b/drivers/net/phy/phy_device.c > >>> @@ -1082,8 +1082,11 @@ int phy_init_hw(struct phy_device *phydev) > { > >>> int ret = 0; > >>> > >>> - /* Deassert the reset signal */ > >>> - phy_device_reset(phydev, 0); > >>> + /* Deassert the reset signal > >>> + * If the PHY needs a reset, do it now > >>> + */ > >>> + if (!phy_reset_after_clk_enable(phydev)) > >> > >> If reset is asserted when entering phy_init_hw(), then > >> phy_reset_after_clk_enable() basically becomes a no-op. > >> Still it should work as expected due to the reset signal being > >> deasserted. It would be worth describing in the comment why the code > >> still works in this case. > >> > > > > Thank you for the comment, this is a very good point. > > I will make sure to include some description when resubmitting. > > I had previously tested this and what I saw was that the first time > > you bring up the interface, the reset is not asserted so that > > phy_reset_after_clk_enable() is effective. > > The subsequent times the interface is brought up, the reset is already > > asserted when entering phy_init_hw(), so that it becomes a no-op as > > you correctly pointed out. However, that didn't cause any problem on > > my board, presumably because in that case the clock is already running > > when the PHY comes out of reset. > > I will re-test this carefully against the 'net' tree, though, before > > coming to conclusions. > > > I have two additional things to take into account: > * phy_reset_after_clk_enable() shoulnd't be longer called that way since it is > now misleading -> the phy is no longer reset after clock enable but during > hw_init() > * how about fec_resume()? I don't think hw_init() is called then and so > phy_reset_after_clk_enable() will no longer be called. > My apologies, in the midst of modifying and preparing a new patch for this issue I now realize that I seem to have failed to reply to your comments. Thank you very much for your time and efforts reviewing the patch; I have taken these into account and indeed there also seems to be an issue with fec_resume that I have now addressed. > >>> + phy_device_reset(phydev, 0); > >>> > >>> if (!phydev->drv) > >>> return 0; > >>> > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-26 10:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 9:03 [PATCH 2/2] Reset PHY in phy_init_hw() before interrupt configuration Badel, Laurent 2020-04-29 12:00 ` Badel, Laurent 2020-04-29 17:06 ` Heiner Kallweit 2020-04-30 9:29 ` [EXTERNAL] " Badel, Laurent 2020-05-19 8:41 ` Jörg Willmann 2020-10-26 10:21 ` Badel, Laurent
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.