[PATCH] Revert "net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x"

[PATCH] Revert "net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x"

[Russell King]

This reverts commit 6623c0fba10ef45b64ca213ad5dec926f37fa9a0.

The original diagnosis was incorrect: it appears that the NIC had
PHY polling mode enabled, which meant that it overwrote the PHYs
advertisement register during negotiation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index cbec296107bd..d29c0c3d0c3e 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -847,8 +847,6 @@ 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)
@@ -871,16 +869,6 @@ 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 advertisement 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 m88e1318_config_init(phydev);
-- 
2.7.4

Re: [PATCH] Revert "net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x"

[Andrew Lunn]

On Fri, Jan 25, 2019 at 11:27:57PM +0000, Russell King wrote:
> This reverts commit 6623c0fba10ef45b64ca213ad5dec926f37fa9a0.
> 
> The original diagnosis was incorrect: it appears that the NIC had
> PHY polling mode enabled, which meant that it overwrote the PHYs
> advertisement register during negotiation.

Hi Russell

The NIC wrote to PHY registers? The NIC reading the PHY in hardware is
bad enough, but changing register as well is not good.

What NIC is this? And do you have further patches to really disable
PHY polling?
 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] Revert "net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x"

[Russell King - ARM Linux admin]

On Sat, Jan 26, 2019 at 04:58:34PM +0100, Andrew Lunn wrote:
> On Fri, Jan 25, 2019 at 11:27:57PM +0000, Russell King wrote:
> > This reverts commit 6623c0fba10ef45b64ca213ad5dec926f37fa9a0.
> > 
> > The original diagnosis was incorrect: it appears that the NIC had
> > PHY polling mode enabled, which meant that it overwrote the PHYs
> > advertisement register during negotiation.
> 
> Hi Russell
> 
> The NIC wrote to PHY registers? The NIC reading the PHY in hardware is
> bad enough, but changing register as well is not good.

Yep, it seems so.

> What NIC is this? And do you have further patches to really disable
> PHY polling?

The PP2.2 in 8040 when used with the Marvell mvpp2x driver (that I
have in my mcbin branch) - mainline's mvpp2 driver doesn't suffer
from it as that disables polling mode.

What made the diagnosis hard is that disabling phy polling by poking
the SMI registers doesn't stop the PHY being polled, it seems to need
something extra - and not having the documentation doesn't help.  As
I said, using a 'scope on the MDIO signal and noticing that the
hardware was still polling the PHY despite the poll bit disabled was
key to getting to the bottom of what was really going on.  Once I
arranged to clear it during driver initialisation and rebooted the
platform, the PHY then had the expected behaviour.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] Revert "net: phy: marvell: avoid pause mode on SGMII-to-Copper for 88e151x"

[David Miller]

From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Fri, 25 Jan 2019 23:27:57 +0000

> This reverts commit 6623c0fba10ef45b64ca213ad5dec926f37fa9a0.
> 
> The original diagnosis was incorrect: it appears that the NIC had
> PHY polling mode enabled, which meant that it overwrote the PHYs
> advertisement register during negotiation.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

The code in the 'net' tree has the linkmode_clear_bit() etc. changes
that were done to this driver, so the pause variable is not there
and the pause bits are adjusted using that helper.

Please respin this against current sources so that it actually
applies.

Thank you.