[PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[Florian Fainelli]

Hi all,

This patch series eliminates unnecessary software resets of the PHY.
This should hopefully not break anybody's hardware; but I would
appreciate testing to make sure this is is the case.

Sorry for this long email list, I wanted to make sure I reached out to
all people who made changes to the Marvell PHY driver.

Thank you!

Florian Fainelli (2):
  net: phy: Stop with excessive soft reset
  net: phy: marvell: Avoid unnecessary soft reset

 drivers/net/phy/marvell.c    | 63 ++++++++++++------------------------
 drivers/net/phy/phy_device.c |  2 --
 2 files changed, 21 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH RFT net-next 1/2] net: phy: Stop with excessive soft reset

[Florian Fainelli]

While consolidating the PHY reset in phy_init_hw() an unconditionaly
BMCR soft-reset I became quite trigger happy with those. This was later
on deactivated for the Generic PHY driver on the premise that a prior
software entity (e.g: bootloader) might have applied workarounds in
commit 0878fff1f42c ("net: phy: Do not perform software reset for
Generic PHY").

Since we have a hook to wire-up a soft_reset callback, just use that and
get rid of the call to genphy_soft_reset() entirely. This speeds up
initialization and link establishment for most PHYs out there that do
not require a reset.

Fixes: 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index db1172db1e7c..9216d2f8e41e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -880,8 +880,6 @@ int phy_init_hw(struct phy_device *phydev)
 
 	if (phydev->drv->soft_reset)
 		ret = phydev->drv->soft_reset(phydev);
-	else
-		ret = genphy_soft_reset(phydev);
 
 	if (ret < 0)
 		return ret;
-- 
2.17.1

[PATCH RFT net-next 2/2] net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

The BMCR.RESET bit on Marvell PHYs appears to be acting as a commit
operation, so we try to hit that bit which disrupts the link, only when
an actual register programming required a change.

Determine from marvell_set_polarity()'s return code whether the register value
was changed and if it was, propagate that to the logic that hits the software
reset bit. This avoids doing unnecessary soft reset if the PHY is configured in
the same state it was previously.

Since m88e1111_config_aneg() likely just hit the software reset bit
blindly without checking whether this is necessary, eliminate that
function and make it use the generic marvell_config_aneg() function
instead.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/marvell.c | 63 +++++++++++++--------------------------
 1 file changed, 21 insertions(+), 42 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f7c69ca34056..24fc4a73c300 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -265,7 +265,7 @@ static int marvell_set_polarity(struct phy_device *phydev, int polarity)
 			return err;
 	}
 
-	return 0;
+	return val != reg;
 }
 
 static int marvell_set_downshift(struct phy_device *phydev, bool enable,
@@ -287,12 +287,15 @@ static int marvell_set_downshift(struct phy_device *phydev, bool enable,
 
 static int marvell_config_aneg(struct phy_device *phydev)
 {
+	int changed = 0;
 	int err;
 
 	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
+	changed = err;
+
 	err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
 			MII_M1111_PHY_LED_DIRECT);
 	if (err < 0)
@@ -302,7 +305,7 @@ static int marvell_config_aneg(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
-	if (phydev->autoneg != AUTONEG_ENABLE) {
+	if (phydev->autoneg != AUTONEG_ENABLE || changed) {
 		/* A write to speed/duplex bits (that is performed by
 		 * genphy_config_aneg() call above) must be followed by
 		 * a software reset. Otherwise, the write has no effect.
@@ -350,42 +353,6 @@ static int m88e1101_config_aneg(struct phy_device *phydev)
 	return marvell_config_aneg(phydev);
 }
 
-static int m88e1111_config_aneg(struct phy_device *phydev)
-{
-	int err;
-
-	/* The Marvell PHY has an errata which requires
-	 * that certain registers get written in order
-	 * to restart autonegotiation
-	 */
-	err = genphy_soft_reset(phydev);
-
-	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
-	if (err < 0)
-		return err;
-
-	err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
-			MII_M1111_PHY_LED_DIRECT);
-	if (err < 0)
-		return err;
-
-	err = genphy_config_aneg(phydev);
-	if (err < 0)
-		return err;
-
-	if (phydev->autoneg != AUTONEG_ENABLE) {
-		/* A write to speed/duplex bits (that is performed by
-		 * genphy_config_aneg() call above) must be followed by
-		 * a software reset. Otherwise, the write has no effect.
-		 */
-		err = genphy_soft_reset(phydev);
-		if (err < 0)
-			return err;
-	}
-
-	return 0;
-}
-
 #ifdef CONFIG_OF_MDIO
 /* Set and/or override some configuration registers based on the
  * marvell,reg-init property stored in the of_node for the phydev.
@@ -479,6 +446,7 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 
 static int m88e1121_config_aneg(struct phy_device *phydev)
 {
+	int changed = 0;
 	int err = 0;
 
 	if (phy_interface_is_rgmii(phydev)) {
@@ -487,15 +455,26 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 			return err;
 	}
 
-	err = genphy_soft_reset(phydev);
+	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
-	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
+	changed = err;
+
+	err = genphy_config_aneg(phydev);
 	if (err < 0)
 		return err;
 
-	return genphy_config_aneg(phydev);
+	if (phydev->autoneg != AUTONEG_ENABLE || changed) {
+		/* A software reset is used to ensure a "commit" of the
+		 * changes is done.
+		 */
+		err = genphy_soft_reset(phydev);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
 }
 
 static int m88e1318_config_aneg(struct phy_device *phydev)
@@ -2067,7 +2046,7 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.probe = marvell_probe,
 		.config_init = &m88e1111_config_init,
-		.config_aneg = &m88e1111_config_aneg,
+		.config_aneg = &marvell_config_aneg,
 		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
-- 
2.17.1

Re: [PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[Chris Healy]

> This patch series eliminates unnecessary software resets of the PHY.
> This should hopefully not break anybody's hardware; but I would
> appreciate testing to make sure this is is the case.

Tested-by: Chris Healy <cphealy@gmail.com>

Tested with Marvell 88E6161 and Marvell 88E6352 switches (which use
Marvell PHY driver.)

Re: [PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[Andrew Lunn]

On Tue, Sep 18, 2018 at 06:35:03PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series eliminates unnecessary software resets of the PHY.
> This should hopefully not break anybody's hardware; but I would
> appreciate testing to make sure this is is the case.
> 
> Sorry for this long email list, I wanted to make sure I reached out to
> all people who made changes to the Marvell PHY driver.

Hi Florian

I want to test this, but i'm battling other issues at the moment and
not got around to it.

But i see it has been tested by two people. So i think we should merge
it, and see if anybody screams.

  Andrew

Re: [PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[Andrew Lunn]

On Tue, Sep 18, 2018 at 06:35:03PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series eliminates unnecessary software resets of the PHY.
> This should hopefully not break anybody's hardware; but I would
> appreciate testing to make sure this is is the case.
> 
> Sorry for this long email list, I wanted to make sure I reached out to
> all people who made changes to the Marvell PHY driver.

I got around to testing this at last. Not too much testing, just
booting it on five different systems using Marvell and Micrel PHYs.
They all got link as expected.

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

    Andrew

Re: [PATCH RFT net-next 0/2] net: phy: Eliminate unnecessary soft

[Clemens Gruber]

Hi,

On Tue, Sep 18, 2018 at 06:35:03PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series eliminates unnecessary software resets of the PHY.
> This should hopefully not break anybody's hardware; but I would
> appreciate testing to make sure this is is the case.
> 
> Sorry for this long email list, I wanted to make sure I reached out to
> all people who made changes to the Marvell PHY driver.
> 
> Thank you!
> 
> Florian Fainelli (2):
>   net: phy: Stop with excessive soft reset
>   net: phy: marvell: Avoid unnecessary soft reset
> 
>  drivers/net/phy/marvell.c    | 63 ++++++++++++------------------------
>  drivers/net/phy/phy_device.c |  2 --
>  2 files changed, 21 insertions(+), 44 deletions(-)
> 
> -- 
> 2.17.1
>

I tested your patches on our board with a Marvell 88E1510. Looks good!

Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>

Clemens