* [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
@ 2017-12-13 9:22 Russell King
2017-12-13 16:44 ` Andrew Lunn
2017-12-13 21:14 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Russell King @ 2017-12-13 9:22 UTC (permalink / raw
To: Andrew Lunn, Florian Fainelli; +Cc: Jon Nettleton, netdev
Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
is unreliable. While the pause bits can be set in the advertisment
register, they clear shortly after negotiation with a link partner
commences irrespective of the cause of the negotiation.
While these bits may be correctly conveyed to the link partner on the
first negotiation, a subsequent negotiation (eg, due to negotiation
restart by the link partner, or reconnection of the cable) will result
in the link partner seeing these bits as zero, while the kernel
believes that it has advertised pause modes.
This leads to the local kernel evaluating (eg) symmetric pause mode,
while the remote end evaluates that we have no pause mode capability.
Since we can't guarantee the advertisment, disable pause mode support
with this PHY when used in SGMII-to-Copper mode.
The 88e1510 in RGMII-to-Copper mode appears to behave correctly.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 710110ead6ad..f4d25ac85e30 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -924,6 +924,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
if (err < 0)
return err;
+
+ /* There appears to be a bug in the 88e1512 when used in
+ * SGMII to copper mode, where the AN advertisment register
+ * clears the pause bits each time a negotiation occurs.
+ * This means we can never be truely sure what was advertised,
+ * so disable Pause support.
+ */
+ pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+ phydev->supported &= ~pause;
+ phydev->advertising &= ~pause;
}
return m88e1121_config_init(phydev);
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-13 9:22 [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x Russell King
@ 2017-12-13 16:44 ` Andrew Lunn
2017-12-13 21:14 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2017-12-13 16:44 UTC (permalink / raw
To: Russell King; +Cc: Florian Fainelli, Jon Nettleton, netdev
On Wed, Dec 13, 2017 at 09:22:09AM +0000, Russell King wrote:
> Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
> is unreliable. While the pause bits can be set in the advertisment
> register, they clear shortly after negotiation with a link partner
> commences irrespective of the cause of the negotiation.
>
> While these bits may be correctly conveyed to the link partner on the
> first negotiation, a subsequent negotiation (eg, due to negotiation
> restart by the link partner, or reconnection of the cable) will result
> in the link partner seeing these bits as zero, while the kernel
> believes that it has advertised pause modes.
>
> This leads to the local kernel evaluating (eg) symmetric pause mode,
> while the remote end evaluates that we have no pause mode capability.
>
> Since we can't guarantee the advertisment, disable pause mode support
> with this PHY when used in SGMII-to-Copper mode.
>
> The 88e1510 in RGMII-to-Copper mode appears to behave correctly.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-13 9:22 [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x Russell King
2017-12-13 16:44 ` Andrew Lunn
@ 2017-12-13 21:14 ` David Miller
2017-12-15 15:45 ` Russell King - ARM Linux
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2017-12-13 21:14 UTC (permalink / raw
To: rmk+kernel; +Cc: andrew, f.fainelli, jon, netdev
From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Wed, 13 Dec 2017 09:22:09 +0000
> @@ -924,6 +924,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
> err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
> if (err < 0)
> return err;
> +
> + /* There appears to be a bug in the 88e1512 when used in
> + * SGMII to copper mode, where the AN advertisment register
> + * clears the pause bits each time a negotiation occurs.
> + * This means we can never be truely sure what was advertised,
> + * so disable Pause support.
> + */
> + pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
> + phydev->supported &= ~pause;
> + phydev->advertising &= ~pause;
> }
>
This function doesn't have a local 'pause' variable in any of my trees.
I wonder what you generated this against, and even more importantly, what
tree the reviewers were considering when looking at this patch :)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-13 21:14 ` David Miller
@ 2017-12-15 15:45 ` Russell King - ARM Linux
2017-12-15 15:48 ` Andrew Lunn
2017-12-15 16:15 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Russell King - ARM Linux @ 2017-12-15 15:45 UTC (permalink / raw
To: David Miller; +Cc: andrew, f.fainelli, jon, netdev
On Wed, Dec 13, 2017 at 04:14:23PM -0500, David Miller wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Date: Wed, 13 Dec 2017 09:22:09 +0000
>
> > @@ -924,6 +924,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
> > err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
> > if (err < 0)
> > return err;
> > +
> > + /* There appears to be a bug in the 88e1512 when used in
> > + * SGMII to copper mode, where the AN advertisment register
> > + * clears the pause bits each time a negotiation occurs.
> > + * This means we can never be truely sure what was advertised,
> > + * so disable Pause support.
> > + */
> > + pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
> > + phydev->supported &= ~pause;
> > + phydev->advertising &= ~pause;
> > }
> >
>
> This function doesn't have a local 'pause' variable in any of my trees.
>
> I wonder what you generated this against, and even more importantly, what
> tree the reviewers were considering when looking at this patch :)
Sorry about that, not quite sure how that happened, but here's a
replacement patch. I assume patchwork will do the right thing by
including the patch in this mail...
8<====
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for
88e151x
Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
is unreliable. While the pause bits can be set in the advertisment
register, they clear shortly after negotiation with a link partner
commences irrespective of the cause of the negotiation.
While these bits may be correctly conveyed to the link partner on the
first negotiation, a subsequent negotiation (eg, due to negotiation
restart by the link partner, or reconnection of the cable) will result
in the link partner seeing these bits as zero, while the kernel
believes that it has advertised pause modes.
This leads to the local kernel evaluating (eg) symmetric pause mode,
while the remote end evaluates that we have no pause mode capability.
Since we can't guarantee the advertisment, disable pause mode support
with this PHY when used in SGMII-to-Copper mode.
The 88e1510 in RGMII-to-Copper mode appears to behave correctly.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b5a8f750e433..2510acc0feb0 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -879,6 +879,8 @@ static int m88e1510_config_init(struct phy_device *phydev)
/* SGMII-to-Copper mode initialization */
if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+ u32 pause;
+
/* Select page 18 */
err = marvell_set_page(phydev, 18);
if (err < 0)
@@ -902,6 +904,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
if (err < 0)
return err;
+
+ /* There appears to be a bug in the 88e1512 when used in
+ * SGMII to copper mode, where the AN advertisment register
+ * clears the pause bits each time a negotiation occurs.
+ * This means we can never be truely sure what was advertised,
+ * so disable Pause support.
+ */
+ pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+ phydev->supported &= ~pause;
+ phydev->advertising &= ~pause;
}
return m88e1121_config_init(phydev);
--
2.7.4
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-15 15:45 ` Russell King - ARM Linux
@ 2017-12-15 15:48 ` Andrew Lunn
2017-12-15 16:15 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2017-12-15 15:48 UTC (permalink / raw
To: Russell King - ARM Linux; +Cc: David Miller, f.fainelli, jon, netdev
> Sorry about that, not quite sure how that happened, but here's a
> replacement patch. I assume patchwork will do the right thing by
> including the patch in this mail...
Hi Russell
No, it does not. You need to submit a whole new patch.
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
@ 2017-12-15 16:10 Russell King
2017-12-18 20:04 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Russell King @ 2017-12-15 16:10 UTC (permalink / raw
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
is unreliable. While the pause bits can be set in the advertisment
register, they clear shortly after negotiation with a link partner
commences irrespective of the cause of the negotiation.
While these bits may be correctly conveyed to the link partner on the
first negotiation, a subsequent negotiation (eg, due to negotiation
restart by the link partner, or reconnection of the cable) will result
in the link partner seeing these bits as zero, while the kernel
believes that it has advertised pause modes.
This leads to the local kernel evaluating (eg) symmetric pause mode,
while the remote end evaluates that we have no pause mode capability.
Since we can't guarantee the advertisment, disable pause mode support
with this PHY when used in SGMII-to-Copper mode.
The 88e1510 in RGMII-to-Copper mode appears to behave correctly.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/marvell.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b5a8f750e433..2510acc0feb0 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -879,6 +879,8 @@ static int m88e1510_config_init(struct phy_device *phydev)
/* SGMII-to-Copper mode initialization */
if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+ u32 pause;
+
/* Select page 18 */
err = marvell_set_page(phydev, 18);
if (err < 0)
@@ -902,6 +904,16 @@ static int m88e1510_config_init(struct phy_device *phydev)
err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
if (err < 0)
return err;
+
+ /* There appears to be a bug in the 88e1512 when used in
+ * SGMII to copper mode, where the AN advertisment register
+ * clears the pause bits each time a negotiation occurs.
+ * This means we can never be truely sure what was advertised,
+ * so disable Pause support.
+ */
+ pause = SUPPORTED_Pause | SUPPORTED_Asym_Pause;
+ phydev->supported &= ~pause;
+ phydev->advertising &= ~pause;
}
return m88e1121_config_init(phydev);
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-15 15:45 ` Russell King - ARM Linux
2017-12-15 15:48 ` Andrew Lunn
@ 2017-12-15 16:15 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2017-12-15 16:15 UTC (permalink / raw
To: linux; +Cc: andrew, f.fainelli, jon, netdev
From: Russell King - ARM Linux <linux@armlinux.org.uk>
Date: Fri, 15 Dec 2017 15:45:40 +0000
> Sorry about that, not quite sure how that happened, but here's a
> replacement patch. I assume patchwork will do the right thing by
> including the patch in this mail...
Please use fresh patch postings when posting a new or fixed version
of a patch.
Otherwsie I have to manually edit what ends up in patchwork in order
to get rid of the quoted discussion text and other irrelevant material
in the commit message.
Thank you.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x
2017-12-15 16:10 Russell King
@ 2017-12-18 20:04 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2017-12-18 20:04 UTC (permalink / raw
To: rmk+kernel; +Cc: andrew, f.fainelli, netdev
From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Fri, 15 Dec 2017 16:10:20 +0000
> Observed on the 88e1512 in SGMII-to-Copper mode, negotiating pause
> is unreliable. While the pause bits can be set in the advertisment
> register, they clear shortly after negotiation with a link partner
> commences irrespective of the cause of the negotiation.
>
> While these bits may be correctly conveyed to the link partner on the
> first negotiation, a subsequent negotiation (eg, due to negotiation
> restart by the link partner, or reconnection of the cable) will result
> in the link partner seeing these bits as zero, while the kernel
> believes that it has advertised pause modes.
>
> This leads to the local kernel evaluating (eg) symmetric pause mode,
> while the remote end evaluates that we have no pause mode capability.
>
> Since we can't guarantee the advertisment, disable pause mode support
> with this PHY when used in SGMII-to-Copper mode.
>
> The 88e1510 in RGMII-to-Copper mode appears to behave correctly.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-18 20:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-13 9:22 [PATCH] net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x Russell King
2017-12-13 16:44 ` Andrew Lunn
2017-12-13 21:14 ` David Miller
2017-12-15 15:45 ` Russell King - ARM Linux
2017-12-15 15:48 ` Andrew Lunn
2017-12-15 16:15 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2017-12-15 16:10 Russell King
2017-12-18 20:04 ` David Miller
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.