* [PATCH] net: bcmgenet: Fix EPHY reset in power up
@ 2016-09-23 13:20 Jaedon Shin
2016-09-23 14:06 ` Andrew Lunn
0 siblings, 1 reply; 5+ messages in thread
From: Jaedon Shin @ 2016-09-23 13:20 UTC (permalink / raw)
To: Florian Fainelli, David S . Miller; +Cc: Philippe Reynes, netdev, Jaedon Shin
The bcmgenet_mii_reset() is always not running in power up sequence
after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
struct net_device")'. This'll show extremely high latency and duplicate
packets while interface down and up repeatedly.
For now, adds again a private phydev for mii reset when runs power up to
open interface.
Signed-off-by: Jaedon Shin <jaedon.shin@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 9 ++++++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0f0868c56f05..1e2dc34d331a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -597,6 +597,7 @@ struct bcmgenet_priv {
/* MDIO bus variables */
wait_queue_head_t wq;
+ struct phy_device *phydev;
bool internal_phy;
struct device_node *phy_dn;
struct device_node *mdio_dn;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index e907acd81da9..b2bd5302c478 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -183,9 +183,9 @@ void bcmgenet_mii_reset(struct net_device *dev)
if (GENET_IS_V4(priv))
return;
- if (dev->phydev) {
- phy_init_hw(dev->phydev);
- phy_start_aneg(dev->phydev);
+ if (priv->phydev) {
+ phy_init_hw(priv->phydev);
+ phy_start_aneg(priv->phydev);
}
}
@@ -383,6 +383,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
}
}
+ priv->phydev = phydev;
+
/* Configure port multiplexer based on what the probed PHY device since
* reading the 'max-speed' property determines the maximum supported
* PHY speed which is needed for bcmgenet_mii_config() to configure
@@ -605,6 +607,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
}
+ priv->phydev = phydev;
priv->phy_interface = pd->phy_interface;
return 0;
--
2.10.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up
2016-09-23 13:20 [PATCH] net: bcmgenet: Fix EPHY reset in power up Jaedon Shin
@ 2016-09-23 14:06 ` Andrew Lunn
2016-09-23 15:04 ` Jaedon Shin
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2016-09-23 14:06 UTC (permalink / raw)
To: Jaedon Shin; +Cc: Florian Fainelli, David S . Miller, Philippe Reynes, netdev
On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
> The bcmgenet_mii_reset() is always not running in power up sequence
> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
> struct net_device")'. This'll show extremely high latency and duplicate
> packets while interface down and up repeatedly.
>
> For now, adds again a private phydev for mii reset when runs power up to
> open interface.
Hi Jaedon
How does this fix the issue? It sounds like you are papering over the
crack, not truly fixing it.
Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up
2016-09-23 14:06 ` Andrew Lunn
@ 2016-09-23 15:04 ` Jaedon Shin
2016-09-23 16:54 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Jaedon Shin @ 2016-09-23 15:04 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, Philippe Reynes, netdev
Hi Andrew,
On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
>> The bcmgenet_mii_reset() is always not running in power up sequence
>> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
>> struct net_device")'. This'll show extremely high latency and duplicate
>> packets while interface down and up repeatedly.
>>
>> For now, adds again a private phydev for mii reset when runs power up to
>> open interface.
>
> Hi Jaedon
>
> How does this fix the issue? It sounds like you are papering over the
> crack, not truly fixing it.
>
> Andrew
Yes, It feel like a workaround, but I think it must need v4.8 stable
version. If we find better way that fixes internal PHY to initialize
after re-open interface, this patch will be dropped.
Additionally,
http://www.spinics.net/lists/netdev/msg350506.html
Thanks,
Jaedon
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up
2016-09-23 15:04 ` Jaedon Shin
@ 2016-09-23 16:54 ` Florian Fainelli
2016-09-23 20:55 ` Jaedon Shin
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2016-09-23 16:54 UTC (permalink / raw)
To: Jaedon Shin, Andrew Lunn; +Cc: David Miller, Philippe Reynes, netdev
On 09/23/2016 08:04 AM, Jaedon Shin wrote:
> Hi Andrew,
>
> On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
>>> The bcmgenet_mii_reset() is always not running in power up sequence
>>> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
>>> struct net_device")'. This'll show extremely high latency and duplicate
>>> packets while interface down and up repeatedly.
>>>
>>> For now, adds again a private phydev for mii reset when runs power up to
>>> open interface.
>>
>> Hi Jaedon
>>
>> How does this fix the issue? It sounds like you are papering over the
>> crack, not truly fixing it.
>>
>> Andrew
>
> Yes, It feel like a workaround, but I think it must need v4.8 stable
> version. If we find better way that fixes internal PHY to initialize
> after re-open interface, this patch will be dropped.
I can observe the faulting behavior with 4.8-rc7 that the link below
fixed initially:
# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.352 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.472 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.496 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.517 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.536 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.557 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=752.448 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.291 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.421 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.444 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.464 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.483 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.505 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=24.964 ms (DUP!)
If we revert this patch, we indeed get the normal and expected behavior
back:
# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=0 ttl=61 time=0.417 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=0.415 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=0.424 ms
Actually, the key thing is this:
- without Philippe's patch we call twice bcmgenet_mii_reset, and that is
intended:
- first time from bcmgenet_power_up() to make sure the PHY is
initialized *before* we get to initialize the UniMAC, this is critical
- second time from bcmgenet_mii_probe(), through the normal phy_init_hw()
- with Philippe's patch, we only get to call bcmgenet_mii_reset once, in
bcmgenet_mii_probe() because the first time in bcmgenet_power_up(),
dev->phydev is NULL, because of a prior call to phy_disconnect() in
bcmgenet_close(), unfortunately, there has been MAC activity, so the PHY
gets in a bad state
Jaedon, feel free to use the explanation above, and send a plain revert
of commit 62469c76007e11428e2ee3c6de90cbe74b588d44.
Thanks!
Thanks!
--
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up
2016-09-23 16:54 ` Florian Fainelli
@ 2016-09-23 20:55 ` Jaedon Shin
0 siblings, 0 replies; 5+ messages in thread
From: Jaedon Shin @ 2016-09-23 20:55 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, David Miller, Philippe Reynes, netdev
Hi Florian,
> On 24 Sep 2016, at 1:54 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 09/23/2016 08:04 AM, Jaedon Shin wrote:
>> Hi Andrew,
>>
>> On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>>
>>> On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
>>>> The bcmgenet_mii_reset() is always not running in power up sequence
>>>> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
>>>> struct net_device")'. This'll show extremely high latency and duplicate
>>>> packets while interface down and up repeatedly.
>>>>
>>>> For now, adds again a private phydev for mii reset when runs power up to
>>>> open interface.
>>>
>>> Hi Jaedon
>>>
>>> How does this fix the issue? It sounds like you are papering over the
>>> crack, not truly fixing it.
>>>
>>> Andrew
>>
>> Yes, It feel like a workaround, but I think it must need v4.8 stable
>> version. If we find better way that fixes internal PHY to initialize
>> after re-open interface, this patch will be dropped.
>
> I can observe the faulting behavior with 4.8-rc7 that the link below
> fixed initially:
>
> # ping fainelli-linux
> PING fainelli-linux (10.112.156.244): 56 data bytes
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.352 ms
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.472 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.496 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.517 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.536 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.557 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=752.448 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.291 ms
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.421 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.444 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.464 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.483 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.505 ms (DUP!)
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=24.964 ms (DUP!)
>
> If we revert this patch, we indeed get the normal and expected behavior
> back:
>
> # ping fainelli-linux
> PING fainelli-linux (10.112.156.244): 56 data bytes
> 64 bytes from 10.112.156.244: seq=0 ttl=61 time=0.417 ms
> 64 bytes from 10.112.156.244: seq=1 ttl=61 time=0.415 ms
> 64 bytes from 10.112.156.244: seq=2 ttl=61 time=0.424 ms
>
> Actually, the key thing is this:
>
> - without Philippe's patch we call twice bcmgenet_mii_reset, and that is
> intended:
> - first time from bcmgenet_power_up() to make sure the PHY is
> initialized *before* we get to initialize the UniMAC, this is critical
> - second time from bcmgenet_mii_probe(), through the normal phy_init_hw()
>
> - with Philippe's patch, we only get to call bcmgenet_mii_reset once, in
> bcmgenet_mii_probe() because the first time in bcmgenet_power_up(),
> dev->phydev is NULL, because of a prior call to phy_disconnect() in
> bcmgenet_close(), unfortunately, there has been MAC activity, so the PHY
> gets in a bad state
>
> Jaedon, feel free to use the explanation above, and send a plain revert
> of commit 62469c76007e11428e2ee3c6de90cbe74b588d44.
>
Will send revert patch.
Thanks,
Jaedon
> Thanks!
>
> Thanks!
> --
> Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-23 20:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-23 13:20 [PATCH] net: bcmgenet: Fix EPHY reset in power up Jaedon Shin
2016-09-23 14:06 ` Andrew Lunn
2016-09-23 15:04 ` Jaedon Shin
2016-09-23 16:54 ` Florian Fainelli
2016-09-23 20:55 ` Jaedon Shin
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.