* [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.