[PATCH] phy/marvell: prevent unneeded page switching

[PATCH] phy/marvell: prevent unneeded page switching

[Frank de Brabander]

marvell_read_status() no longer first switches to the fiber page, instead
looks at the already active register page first. In case the link is down,
switches to the other (fiber/copper) register page.

Unneeded register page switching of the Marvell 88E1510 PHY can cause
the ethernet driver to register a link down & up event.

Signed-off-by: Frank de Brabander <debrabander@gmail.com>
---
 drivers/net/phy/marvell.c | 59 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index abb7876..31de2db 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -194,6 +194,11 @@ static int marvell_read_page(struct phy_device *phydev)
 	return __phy_read(phydev, MII_MARVELL_PHY_PAGE);
 }
 
+static int marvell_get_page(struct phy_device *phydev)
+{
+	return phy_read(phydev, MII_MARVELL_PHY_PAGE);
+}
+
 static int marvell_write_page(struct phy_device *phydev, int page)
 {
 	return __phy_write(phydev, MII_MARVELL_PHY_PAGE, page);
@@ -1245,47 +1250,59 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
  * Some Marvell's phys have two modes: fiber and copper.
  * Both need status checked.
  * Description:
- *   First, check the fiber link and status.
- *   If the fiber link is down, check the copper link and status which
- *   will be the default value if both link are down.
+ *   First, check the link of the currently selected page. Unneeded page
+ *   switches should be prevented, they can trigger the ethernet driver to
+ *   register a link down event.
+ *   If the first checked link type is down, continue with checking the other
+ *   link type. This is needed for platforms that have a fiber and copper link
+ *   connected to the same phy.
  */
 static int marvell_read_status(struct phy_device *phydev)
 {
 	int err;
+	int page;
 
-	/* Check the fiber mode first */
 	if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
 			      phydev->supported) &&
 	    phydev->interface != PHY_INTERFACE_MODE_SGMII) {
-		err = marvell_set_page(phydev, MII_MARVELL_FIBER_PAGE);
-		if (err < 0)
-			goto error;
+		page = marvell_get_page(phydev);
+		if (page < 0)
+			return page;
+
+		if (page != MII_MARVELL_FIBER_PAGE &&
+		    page != MII_MARVELL_COPPER_PAGE) {
+			page = MII_MARVELL_FIBER_PAGE;
 
-		err = marvell_read_status_page(phydev, MII_MARVELL_FIBER_PAGE);
+			err = marvell_set_page(phydev, page);
+			if (err < 0)
+				return err;
+		}
+
+		err = marvell_read_status_page(phydev, page);
 		if (err < 0)
-			goto error;
+			return err;
 
-		/* If the fiber link is up, it is the selected and
-		 * used link. In this case, we need to stay in the
-		 * fiber page. Please to be careful about that, avoid
-		 * to restore Copper page in other functions which
-		 * could break the behaviour for some fiber phy like
+		/* If the link is up, it is the selected and
+		 * used link. In this case, we need to stay in that
+		 * page. Please to be careful about that, avoid
+		 * to restore the page in other functions which
+		 * could break the behaviour for some phy's like
 		 * 88E1512.
 		 */
 		if (phydev->link)
 			return 0;
 
-		/* If fiber link is down, check and save copper mode state */
-		err = marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
+		page = (page == MII_MARVELL_FIBER_PAGE) ?
+			MII_MARVELL_COPPER_PAGE : MII_MARVELL_FIBER_PAGE;
+
+		err = marvell_set_page(phydev, page);
 		if (err < 0)
-			goto error;
+			return err;
+
+		return marvell_read_status_page(phydev, page);
 	}
 
 	return marvell_read_status_page(phydev, MII_MARVELL_COPPER_PAGE);
-
-error:
-	marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
-	return err;
 }
 
 /* marvell_suspend
-- 
2.7.4

Re: [PATCH] phy/marvell: prevent unneeded page switching

[Andrew Lunn]

On Tue, Feb 19, 2019 at 08:21:33AM +0100, Frank de Brabander wrote:
> marvell_read_status() no longer first switches to the fiber page, instead
> looks at the already active register page first. In case the link is down,
> switches to the other (fiber/copper) register page.
> 
> Unneeded register page switching of the Marvell 88E1510 PHY can cause
> the ethernet driver to register a link down & up event.

Hi Frank

I think the page change itself is unlikely to cause a link down/up.
It seems more likely there is something else going on here. The wrong
page being read, fibre page being read when there is no support for
fibre, etc.

Do you get link down/up reported when you read the temperature sensor?
That also causes a page change?

I think i would first start by adding locking to
marvell_read_status().

phy_select_page() takes a lock before changing the page.
phy_restore_page() releases the lock. Use those around reading the
status.

	Andrew