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

[PATCH 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!

Changes since RFT:

- added Tested-by tags from Wang, Dongsheng, Andrew, Chris and Clemens

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 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()")
Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
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 af64a9320fb0..ee676d75fe02 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 net-next 2/2] net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

The BMCR.RESET bit on the Marvell PHYs has a special meaning in that
it commits the register writes into the HW for it to latch and be
configured appropriately. Doing software resets causes link drops, and
this is unnecessary disruption if nothing changed.

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, this also eliminates the need for a
m88e1111_config_aneg() function since it now is the same as
marvell_config_aneg().

Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
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..b55a7376bfdc 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 || 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

regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

G'day All,

I've just update from kernel 4.19 to 5.0 on a custom board that has a marvell
dsa mv88e6085 and the phy on the mv88e6085 will only connect at 10Mb/s with
the above mentioned patch applied.

Bisecting the issue lead me to the following patch.

d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)

Revert the patch, and the associated build fix
4b1bd6976945417 (net: phy: marvell: Fix build.)
restores connections to 1Gb/s.

Anyone have any thoughts as to the correct fix?

-- 
Regards
Phil

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

On 3/15/19 1:52 AM, Phil Reid wrote:
> G'day All,
> 
> I've just update from kernel 4.19 to 5.0 on a custom board that has a
> marvell
> dsa mv88e6085 and the phy on the mv88e6085 will only connect at 10Mb/s with
> the above mentioned patch applied.
> 
> Bisecting the issue lead me to the following patch.
> 
> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
> 
> Revert the patch, and the associated build fix
> 4b1bd6976945417 (net: phy: marvell: Fix build.)
> restores connections to 1Gb/s.
> 
> Anyone have any thoughts as to the correct fix?

What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to add a
specific entry in drivers/net/phy/marvell.c to restore the software
reset to commit changes to the register.
-- 
Florian

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 16/03/2019 5:58 am, Florian Fainelli wrote:
> On 3/15/19 1:52 AM, Phil Reid wrote:
>> G'day All,
>>
>> I've just update from kernel 4.19 to 5.0 on a custom board that has a
>> marvell
>> dsa mv88e6085 and the phy on the mv88e6085 will only connect at 10Mb/s with
>> the above mentioned patch applied.
>>
>> Bisecting the issue lead me to the following patch.
>>
>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>
>> Revert the patch, and the associated build fix
>> 4b1bd6976945417 (net: phy: marvell: Fix build.)
>> restores connections to 1Gb/s.
>>
>> Anyone have any thoughts as to the correct fix?
> 
> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to add a
> specific entry in drivers/net/phy/marvell.c to restore the software
> reset to commit changes to the register.
> 
G'day Florian,

OUI is 0x005043
Model is 101011

Phy1ID: 0x0141
Phy2ID: 0x0eb1

The running phy driver is "Marvell 88E1540"
-- 
Regards
Phil

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

On 3/17/19 7:11 PM, Phil Reid wrote:
> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>> G'day All,
>>>
>>> I've just update from kernel 4.19 to 5.0 on a custom board that has a
>>> marvell
>>> dsa mv88e6085 and the phy on the mv88e6085 will only connect at
>>> 10Mb/s with
>>> the above mentioned patch applied.
>>>
>>> Bisecting the issue lead me to the following patch.
>>>
>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>
>>> Revert the patch, and the associated build fix
>>> 4b1bd6976945417 (net: phy: marvell: Fix build.)
>>> restores connections to 1Gb/s.
>>>
>>> Anyone have any thoughts as to the correct fix?
>>
>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to add a
>> specific entry in drivers/net/phy/marvell.c to restore the software
>> reset to commit changes to the register.
>>
> G'day Florian,
> 
> OUI is 0x005043
> Model is 101011
> 
> Phy1ID: 0x0141
> Phy2ID: 0x0eb1
> 
> The running phy driver is "Marvell 88E1540"

Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch, did
you mean that the mv88e6085 compatible string is used in Device Tree to
designate that chip? While the 88E1540 driver is picked up, that driver
is originally for external PHY packages (AFAICT), so there could be some
switch-specific integration of this PHY that makes it behave
differently, having the OUI helps narrow down what might be necessary to
accomplish.

FWIW, the changes were originally tested with a 88e6352.
-- 
Florian

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Andrew Lunn]

> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch, did
> you mean that the mv88e6085 compatible string is used in Device Tree to
> designate that chip? While the 88E1540 driver is picked up, that driver
> is originally for external PHY packages (AFAICT), so there could be some
> switch-specific integration of this PHY that makes it behave
> differently, having the OUI helps narrow down what might be necessary to
> accomplish.
> 
> FWIW, the changes were originally tested with a 88e6352.

Hi Florian

Some of the older Marvell switches use the 1540 for its internal
PHYs. Same OUI as the external PHY.  I don't remember which
switches. Maybe the mv88e6161 in the RDU1?

  Andrew

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Chris Healy]

> Some of the older Marvell switches use the 1540 for its internal
> PHYs. Same OUI as the external PHY.  I don't remember which
> switches. Maybe the mv88e6161 in the RDU1?
>
I'm using a Marvell 88E6161 with 5.0.1 and I get the following output
which I think indicates that the 6161 does not use the 1540 PHY:

[    3.922322] mv88e6085 gpio-0:00: switch 0x1610 detected: Marvell
88E6161, revision 3
[    4.227851] libphy: mv88e6xxx SMI: probed
[    6.179094] mv88e6085 gpio-0:00 netaux (uninitialized): PHY
[mv88e6xxx-1:01] driver [Marvell 88E1121R]
[    6.220290] mv88e6085 gpio-0:00 netright (uninitialized): PHY
[mv88e6xxx-1:03] driver [Marvell 88E1121R]
[    6.268894] mv88e6085 gpio-0:00 netleft (uninitialized): PHY
[mv88e6xxx-1:04] driver [Marvell 88E1121R]

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Andrew Lunn]

> I'm using a Marvell 88E6161 with 5.0.1 and I get the following output
> which I think indicates that the 6161 does not use the 1540 PHY:

Correct. Maybe it is the 6172? I know it is one of the devices i have.
Yes, i mv88e6172 uses 88E1540.

     Andrew

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 19/03/2019 1:09 am, Florian Fainelli wrote:
> On 3/17/19 7:11 PM, Phil Reid wrote:
>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>>> G'day All,
>>>>
>>>> I've just update from kernel 4.19 to 5.0 on a custom board that has a
>>>> marvell
>>>> dsa mv88e6085 and the phy on the mv88e6085 will only connect at
>>>> 10Mb/s with
>>>> the above mentioned patch applied.
>>>>
>>>> Bisecting the issue lead me to the following patch.
>>>>
>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>>
>>>> Revert the patch, and the associated build fix
>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.)
>>>> restores connections to 1Gb/s.
>>>>
>>>> Anyone have any thoughts as to the correct fix?
>>>
>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to add a
>>> specific entry in drivers/net/phy/marvell.c to restore the software
>>> reset to commit changes to the register.
>>>
>> G'day Florian,
>>
>> OUI is 0x005043
>> Model is 101011
>>
>> Phy1ID: 0x0141
>> Phy2ID: 0x0eb1
>>
>> The running phy driver is "Marvell 88E1540"
> 
> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch, did
> you mean that the mv88e6085 compatible string is used in Device Tree to
> designate that chip? While the 88E1540 driver is picked up, that driver
> is originally for external PHY packages (AFAICT), so there could be some
> switch-specific integration of this PHY that makes it behave
> differently, having the OUI helps narrow down what might be necessary to
> accomplish.
> 
> FWIW, the changes were originally tested with a 88e6352.
> 

Oops yes that's the compatible string.
Mfr Code we purchased was 88E6352-A1-TFJ2I000

The switch id registers (0x03) returns 0x3521

Odd it works ok for others.

Here's the relevant dmesgs (with the patch reverted)

[    1.332652] bus: 'mdio_bus': add driver mv88e6085
[    1.368572] bus: 'mdio_bus': driver_probe_device: matched device stmmac-0:00 with driver mv88e6085
[    1.368584] bus: 'mdio_bus': really_probe: probing driver mv88e6085 with device stmmac-0:00
[    1.368603] mv88e6085 stmmac-0:00: no pinctrl handle
[    1.368630] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
[    1.368638] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
[    1.368687] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
[    1.368696] mv88e6085 stmmac-0:00: No GPIO consumer reset found
[    1.368835] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352, revision 1
[    1.401845] mdio_bus stmmac-0:00: Driver mv88e6085 requests probe deferral
[    2.497476] bus: 'mdio_bus': driver_probe_device: matched device stmmac-0:00 with driver mv88e6085
[    2.497486] bus: 'mdio_bus': really_probe: probing driver mv88e6085 with device stmmac-0:00
[    2.497505] mv88e6085 stmmac-0:00: no pinctrl handle
[    2.497536] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
[    2.497544] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
[    2.497595] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
[    2.497604] mv88e6085 stmmac-0:00: No GPIO consumer reset found
[    2.497750] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352, revision 1
[    2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell 88E1540]
[    2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell 88E1540]
[    2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell 88E1540]
[    2.917034] driver: 'mv88e6085': driver_bound: bound to device 'stmmac-0:00'
[    2.917127] bus: 'mdio_bus': really_probe: bound device stmmac-0:00 to driver mv88e6085
[    8.293526] mv88e6085 stmmac-0:00 lan3: configuring for phy/ link mode
[    8.305856] mv88e6085 stmmac-0:00 lan2: configuring for phy/ link mode
[    8.325009] mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode
[    8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link mode
[    8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up - 100Mbps/Full - flow control off
[   12.034974] mv88e6085 stmmac-0:00 lan1: Link is Up - 1Gbps/Full - flow control rx/tx


-- 
Regards
Phil

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

On 3/18/19 6:32 PM, Phil Reid wrote:
> On 19/03/2019 1:09 am, Florian Fainelli wrote:
>> On 3/17/19 7:11 PM, Phil Reid wrote:
>>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>>>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>>>> G'day All,
>>>>>
>>>>> I've just update from kernel 4.19 to 5.0 on a custom board that has a
>>>>> marvell
>>>>> dsa mv88e6085 and the phy on the mv88e6085 will only connect at
>>>>> 10Mb/s with
>>>>> the above mentioned patch applied.
>>>>>
>>>>> Bisecting the issue lead me to the following patch.
>>>>>
>>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>>>
>>>>> Revert the patch, and the associated build fix
>>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.)
>>>>> restores connections to 1Gb/s.
>>>>>
>>>>> Anyone have any thoughts as to the correct fix?
>>>>
>>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to
>>>> add a
>>>> specific entry in drivers/net/phy/marvell.c to restore the software
>>>> reset to commit changes to the register.
>>>>
>>> G'day Florian,
>>>
>>> OUI is 0x005043
>>> Model is 101011
>>>
>>> Phy1ID: 0x0141
>>> Phy2ID: 0x0eb1
>>>
>>> The running phy driver is "Marvell 88E1540"
>>
>> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch, did
>> you mean that the mv88e6085 compatible string is used in Device Tree to
>> designate that chip? While the 88E1540 driver is picked up, that driver
>> is originally for external PHY packages (AFAICT), so there could be some
>> switch-specific integration of this PHY that makes it behave
>> differently, having the OUI helps narrow down what might be necessary to
>> accomplish.
>>
>> FWIW, the changes were originally tested with a 88e6352.
>>
> 
> Oops yes that's the compatible string.
> Mfr Code we purchased was 88E6352-A1-TFJ2I000
> 
> The switch id registers (0x03) returns 0x3521
> 
> Odd it works ok for others.

One possible difference is that the device this was tested with has an
EEPROM which gets involved in configuring the switch during PoR (IIRC)
and that could produce different results. Do you have any switch
initialization happening in the boot loader by any chance?

> 
> Here's the relevant dmesgs (with the patch reverted)
> 
> [    1.332652] bus: 'mdio_bus': add driver mv88e6085
> [    1.368572] bus: 'mdio_bus': driver_probe_device: matched device
> stmmac-0:00 with driver mv88e6085
> [    1.368584] bus: 'mdio_bus': really_probe: probing driver mv88e6085
> with device stmmac-0:00
> [    1.368603] mv88e6085 stmmac-0:00: no pinctrl handle
> [    1.368630] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
> [    1.368638] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
> [    1.368687] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
> [    1.368696] mv88e6085 stmmac-0:00: No GPIO consumer reset found
> [    1.368835] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell
> 88E6352, revision 1
> [    1.401845] mdio_bus stmmac-0:00: Driver mv88e6085 requests probe
> deferral
> [    2.497476] bus: 'mdio_bus': driver_probe_device: matched device
> stmmac-0:00 with driver mv88e6085
> [    2.497486] bus: 'mdio_bus': really_probe: probing driver mv88e6085
> with device stmmac-0:00
> [    2.497505] mv88e6085 stmmac-0:00: no pinctrl handle
> [    2.497536] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
> [    2.497544] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
> [    2.497595] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
> [    2.497604] mv88e6085 stmmac-0:00: No GPIO consumer reset found
> [    2.497750] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell
> 88E6352, revision 1
> [    2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized): PHY
> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell 88E1540]
> [    2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized): PHY
> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell 88E1540]
> [    2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized): PHY
> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell 88E1540]
> [    2.917034] driver: 'mv88e6085': driver_bound: bound to device
> 'stmmac-0:00'
> [    2.917127] bus: 'mdio_bus': really_probe: bound device stmmac-0:00
> to driver mv88e6085
> [    8.293526] mv88e6085 stmmac-0:00 lan3: configuring for phy/ link mode
> [    8.305856] mv88e6085 stmmac-0:00 lan2: configuring for phy/ link mode
> [    8.325009] mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode
> [    8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link mode
> [    8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up - 100Mbps/Full -
> flow control off
> [   12.034974] mv88e6085 stmmac-0:00 lan1: Link is Up - 1Gbps/Full -
> flow control rx/tx
> 
> 


-- 
Florian

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 20/03/2019 12:53 am, Florian Fainelli wrote:
> On 3/18/19 6:32 PM, Phil Reid wrote:
>> On 19/03/2019 1:09 am, Florian Fainelli wrote:
>>> On 3/17/19 7:11 PM, Phil Reid wrote:
>>>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>>>>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>>>>> G'day All,
>>>>>>
>>>>>> I've just update from kernel 4.19 to 5.0 on a custom board that has a
>>>>>> marvell
>>>>>> dsa mv88e6085 and the phy on the mv88e6085 will only connect at
>>>>>> 10Mb/s with
>>>>>> the above mentioned patch applied.
>>>>>>
>>>>>> Bisecting the issue lead me to the following patch.
>>>>>>
>>>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>>>>
>>>>>> Revert the patch, and the associated build fix
>>>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.)
>>>>>> restores connections to 1Gb/s.
>>>>>>
>>>>>> Anyone have any thoughts as to the correct fix?
>>>>>
>>>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need to
>>>>> add a
>>>>> specific entry in drivers/net/phy/marvell.c to restore the software
>>>>> reset to commit changes to the register.
>>>>>
>>>> G'day Florian,
>>>>
>>>> OUI is 0x005043
>>>> Model is 101011
>>>>
>>>> Phy1ID: 0x0141
>>>> Phy2ID: 0x0eb1
>>>>
>>>> The running phy driver is "Marvell 88E1540"
>>>
>>> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch, did
>>> you mean that the mv88e6085 compatible string is used in Device Tree to
>>> designate that chip? While the 88E1540 driver is picked up, that driver
>>> is originally for external PHY packages (AFAICT), so there could be some
>>> switch-specific integration of this PHY that makes it behave
>>> differently, having the OUI helps narrow down what might be necessary to
>>> accomplish.
>>>
>>> FWIW, the changes were originally tested with a 88e6352.
>>>
>>
>> Oops yes that's the compatible string.
>> Mfr Code we purchased was 88E6352-A1-TFJ2I000
>>
>> The switch id registers (0x03) returns 0x3521
>>
>> Odd it works ok for others.
> 
> One possible difference is that the device this was tested with has an
> EEPROM which gets involved in configuring the switch during PoR (IIRC)
> and that could produce different results. Do you have any switch
> initialization happening in the boot loader by any chance?
> 
Yes uboot is configuring the switch at boot as a simple network switch.
Then we reconfig it in the kernel. I see if I can get some time to bypass
the uboot init and just rely on the kernel to config the chip.

The external reset pin is available as well. I tried adding that to the dts but
that seems to make things worse. With the patch reverted and external reset pin
enabled  I see a message saying that the phy connected at 1G but can not receive
anything, even on the fixed phy link to our wifi device.

This is the config being applied by uboot if it helps.

/* ------------------------------------------------------------------------- */
struct mv88e_sw_reg extsw_conf[] = {
	/*
	 * port 0, PIGGY4, autoneg
	 * first the fix for the 1000Mbits Autoneg, this is from
	 * a Marvell errata, the regs are undocumented
	 */
	{ PHY(0), PHY_PAGE, AN1000FIX_PAGE },
	{ PHY(0), PHY_STATUS, AN1000FIX },
	{ PHY(0), PHY_PAGE, 0 },
	/* now the real port and phy configuration */
	/* port 0 unused */
	{ PORT(0), PORT_CTRL, PORT_DIS },
	{ PHY (0), PHY_PAGE, 0 },
	{ PHY (0), PHY_CTRL, PHY_PWR_DOWN },
	{ PHY (0), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
	/* port 1 unused */
	{ PORT(1), PORT_CTRL, PORT_DIS },
	{ PHY (1), PHY_PAGE, 0 },
	{ PHY (1), PHY_CTRL, PHY_PWR_DOWN },
	{ PHY (1), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
	/* port 2, (physical jack 0) */
	{ PORT(2), PORT_PHY, NO_SPEED_FOR },
	{ PORT(2), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
	{ PHY (2), PHY_PAGE, 0 },
	{ PHY (2), PHY_1000_CTRL, ADV_1000_FDPX },
	{ PHY (2), PHY_SPEC_CTRL, AUTO_MDIX_EN },
	{ PHY (2), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN | AUTONEG_RST | FULL_DUPLEX },
	/* port 3, (physical jack 1)  */
	{ PORT(3), PORT_PHY, NO_SPEED_FOR },
	{ PORT(3), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
	{ PHY (3), PHY_PAGE, 0 },
	{ PHY (3), PHY_1000_CTRL, ADV_1000_FDPX },
	{ PHY (3), PHY_SPEC_CTRL, AUTO_MDIX_EN },
	{ PHY (3), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN | AUTONEG_RST | FULL_DUPLEX },
	/* port 4, (physical jack 2)  */
	{ PORT(4), PORT_PHY, NO_SPEED_FOR },
	{ PORT(4), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
	{ PHY (4), PHY_PAGE, 0 },
	{ PHY (4), PHY_1000_CTRL, ADV_1000_FDPX },
	{ PHY (4), PHY_SPEC_CTRL, AUTO_MDIX_EN },
	{ PHY (4), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN | AUTONEG_RST | FULL_DUPLEX },
	/* port 5, CPU_RGMII */
	//{ PORT(5), PORT_CTRL, PORT_DIS },
	{ PORT(5), PORT_PHY,                               FLOW_CTRL_EN | FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX | FULL_DPX_FOR | SPEED_100_FOR },
	{ PORT(5), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
	{ PORT(5), PORT_LED, PORT_LED_UPDATE | PORT_LED_CTL | PORT5_L0_ACT5 | PORT5_L1_ACT6 },
	/* port 6, unused, this port has no phy */
	{ PORT(6), PORT_PHY, RX_RGMII_TIM | TX_RGMII_TIM | FLOW_CTRL_EN | FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX | FULL_DPX_FOR | SPEED_1000_FOR },
	{ PORT(6), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
};

>>
>> Here's the relevant dmesgs (with the patch reverted)
>>
>> [    1.332652] bus: 'mdio_bus': add driver mv88e6085
>> [    1.368572] bus: 'mdio_bus': driver_probe_device: matched device
>> stmmac-0:00 with driver mv88e6085
>> [    1.368584] bus: 'mdio_bus': really_probe: probing driver mv88e6085
>> with device stmmac-0:00
>> [    1.368603] mv88e6085 stmmac-0:00: no pinctrl handle
>> [    1.368630] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
>> [    1.368638] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
>> [    1.368687] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
>> [    1.368696] mv88e6085 stmmac-0:00: No GPIO consumer reset found
>> [    1.368835] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell
>> 88E6352, revision 1
>> [    1.401845] mdio_bus stmmac-0:00: Driver mv88e6085 requests probe
>> deferral
>> [    2.497476] bus: 'mdio_bus': driver_probe_device: matched device
>> stmmac-0:00 with driver mv88e6085
>> [    2.497486] bus: 'mdio_bus': really_probe: probing driver mv88e6085
>> with device stmmac-0:00
>> [    2.497505] mv88e6085 stmmac-0:00: no pinctrl handle
>> [    2.497536] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
>> [    2.497544] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
>> [    2.497595] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
>> [    2.497604] mv88e6085 stmmac-0:00: No GPIO consumer reset found
>> [    2.497750] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell
>> 88E6352, revision 1
>> [    2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized): PHY
>> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell 88E1540]
>> [    2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized): PHY
>> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell 88E1540]
>> [    2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized): PHY
>> [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell 88E1540]
>> [    2.917034] driver: 'mv88e6085': driver_bound: bound to device
>> 'stmmac-0:00'
>> [    2.917127] bus: 'mdio_bus': really_probe: bound device stmmac-0:00
>> to driver mv88e6085
>> [    8.293526] mv88e6085 stmmac-0:00 lan3: configuring for phy/ link mode
>> [    8.305856] mv88e6085 stmmac-0:00 lan2: configuring for phy/ link mode
>> [    8.325009] mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode
>> [    8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link mode
>> [    8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up - 100Mbps/Full -
>> flow control off
>> [   12.034974] mv88e6085 stmmac-0:00 lan1: Link is Up - 1Gbps/Full -
>> flow control rx/tx
>>
>>
> 
> 


-- 
Regards
Phil

RE: regression from: net: phy: marvell: Avoid unnecessary soft reset

[liweihang]

Hi all,

I've met a similar issue and sent an email to discuss about it before:
Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy

d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
I reverted this patch and the auto-negotiation works ok.

Florian, could you please read my previous email and give me some advice?


> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Phil Reid
> Sent: Wednesday, March 20, 2019 9:34 AM
> To: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org
> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
> cphealy@gmail.com; clemens.gruber@pqgruber.com; hkallweit1@gmail.com;
> nbd@nbd.name; harini.katakam@xilinx.com
> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
> 
> On 20/03/2019 12:53 am, Florian Fainelli wrote:
> > On 3/18/19 6:32 PM, Phil Reid wrote:
> >> On 19/03/2019 1:09 am, Florian Fainelli wrote:
> >>> On 3/17/19 7:11 PM, Phil Reid wrote:
> >>>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
> >>>>> On 3/15/19 1:52 AM, Phil Reid wrote:
> >>>>>> G'day All,
> >>>>>>
> >>>>>> I've just update from kernel 4.19 to 5.0 on a custom board that
> >>>>>> has a marvell dsa mv88e6085 and the phy on the mv88e6085 will
> >>>>>> only connect at 10Mb/s with the above mentioned patch applied.
> >>>>>>
> >>>>>> Bisecting the issue lead me to the following patch.
> >>>>>>
> >>>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
> >>>>>>
> >>>>>> Revert the patch, and the associated build fix
> >>>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.) restores
> >>>>>> connections to 1Gb/s.
> >>>>>>
> >>>>>> Anyone have any thoughts as to the correct fix?
> >>>>>
> >>>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need
> to
> >>>>> add a specific entry in drivers/net/phy/marvell.c to restore the
> >>>>> software reset to commit changes to the register.
> >>>>>
> >>>> G'day Florian,
> >>>>
> >>>> OUI is 0x005043
> >>>> Model is 101011
> >>>>
> >>>> Phy1ID: 0x0141
> >>>> Phy2ID: 0x0eb1
> >>>>
> >>>> The running phy driver is "Marvell 88E1540"
> >>>
> >>> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch,
> >>> did you mean that the mv88e6085 compatible string is used in Device
> >>> Tree to designate that chip? While the 88E1540 driver is picked up,
> >>> that driver is originally for external PHY packages (AFAICT), so
> >>> there could be some switch-specific integration of this PHY that
> >>> makes it behave differently, having the OUI helps narrow down what
> >>> might be necessary to accomplish.
> >>>
> >>> FWIW, the changes were originally tested with a 88e6352.
> >>>
> >>
> >> Oops yes that's the compatible string.
> >> Mfr Code we purchased was 88E6352-A1-TFJ2I000
> >>
> >> The switch id registers (0x03) returns 0x3521
> >>
> >> Odd it works ok for others.
> >
> > One possible difference is that the device this was tested with has an
> > EEPROM which gets involved in configuring the switch during PoR (IIRC)
> > and that could produce different results. Do you have any switch
> > initialization happening in the boot loader by any chance?
> >
> Yes uboot is configuring the switch at boot as a simple network switch.
> Then we reconfig it in the kernel. I see if I can get some time to bypass the
> uboot init and just rely on the kernel to config the chip.
> 
> The external reset pin is available as well. I tried adding that to the dts but
> that seems to make things worse. With the patch reverted and external
> reset pin enabled  I see a message saying that the phy connected at 1G but
> can not receive anything, even on the fixed phy link to our wifi device.
> 
> This is the config being applied by uboot if it helps.
> 
> /* ------------------------------------------------------------------------- */ struct
> mv88e_sw_reg extsw_conf[] = {
> 	/*
> 	 * port 0, PIGGY4, autoneg
> 	 * first the fix for the 1000Mbits Autoneg, this is from
> 	 * a Marvell errata, the regs are undocumented
> 	 */
> 	{ PHY(0), PHY_PAGE, AN1000FIX_PAGE },
> 	{ PHY(0), PHY_STATUS, AN1000FIX },
> 	{ PHY(0), PHY_PAGE, 0 },
> 	/* now the real port and phy configuration */
> 	/* port 0 unused */
> 	{ PORT(0), PORT_CTRL, PORT_DIS },
> 	{ PHY (0), PHY_PAGE, 0 },
> 	{ PHY (0), PHY_CTRL, PHY_PWR_DOWN },
> 	{ PHY (0), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
> 	/* port 1 unused */
> 	{ PORT(1), PORT_CTRL, PORT_DIS },
> 	{ PHY (1), PHY_PAGE, 0 },
> 	{ PHY (1), PHY_CTRL, PHY_PWR_DOWN },
> 	{ PHY (1), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
> 	/* port 2, (physical jack 0) */
> 	{ PORT(2), PORT_PHY, NO_SPEED_FOR },
> 	{ PORT(2), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
> 	{ PHY (2), PHY_PAGE, 0 },
> 	{ PHY (2), PHY_1000_CTRL, ADV_1000_FDPX },
> 	{ PHY (2), PHY_SPEC_CTRL, AUTO_MDIX_EN },
> 	{ PHY (2), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
> | AUTONEG_RST | FULL_DUPLEX },
> 	/* port 3, (physical jack 1)  */
> 	{ PORT(3), PORT_PHY, NO_SPEED_FOR },
> 	{ PORT(3), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
> 	{ PHY (3), PHY_PAGE, 0 },
> 	{ PHY (3), PHY_1000_CTRL, ADV_1000_FDPX },
> 	{ PHY (3), PHY_SPEC_CTRL, AUTO_MDIX_EN },
> 	{ PHY (3), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
> | AUTONEG_RST | FULL_DUPLEX },
> 	/* port 4, (physical jack 2)  */
> 	{ PORT(4), PORT_PHY, NO_SPEED_FOR },
> 	{ PORT(4), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
> 	{ PHY (4), PHY_PAGE, 0 },
> 	{ PHY (4), PHY_1000_CTRL, ADV_1000_FDPX },
> 	{ PHY (4), PHY_SPEC_CTRL, AUTO_MDIX_EN },
> 	{ PHY (4), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
> | AUTONEG_RST | FULL_DUPLEX },
> 	/* port 5, CPU_RGMII */
> 	//{ PORT(5), PORT_CTRL, PORT_DIS },
> 	{ PORT(5), PORT_PHY,                               FLOW_CTRL_EN |
> FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX | FULL_DPX_FOR |
> SPEED_100_FOR },
> 	{ PORT(5), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
> 	{ PORT(5), PORT_LED, PORT_LED_UPDATE | PORT_LED_CTL |
> PORT5_L0_ACT5 | PORT5_L1_ACT6 },
> 	/* port 6, unused, this port has no phy */
> 	{ PORT(6), PORT_PHY, RX_RGMII_TIM | TX_RGMII_TIM |
> FLOW_CTRL_EN | FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX |
> FULL_DPX_FOR | SPEED_1000_FOR },
> 	{ PORT(6), PORT_CTRL, FORWARDING | EGRS_FLD_ALL }, };
> 
> >>
> >> Here's the relevant dmesgs (with the patch reverted)
> >>
> >> [    1.332652] bus: 'mdio_bus': add driver mv88e6085 [    1.368572]
> >> bus: 'mdio_bus': driver_probe_device: matched device
> >> stmmac-0:00 with driver mv88e6085
> >> [    1.368584] bus: 'mdio_bus': really_probe: probing driver
> >> mv88e6085 with device stmmac-0:00 [    1.368603] mv88e6085
> >> stmmac-0:00: no pinctrl handle [    1.368630] mv88e6085 stmmac-0:00:
> >> GPIO lookup for consumer reset [    1.368638] mv88e6085 stmmac-0:00:
> >> using device tree for GPIO lookup [    1.368687] mv88e6085
> >> stmmac-0:00: using lookup tables for GPIO lookup [    1.368696]
> >> mv88e6085 stmmac-0:00: No GPIO consumer reset found [    1.368835]
> >> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
> >> revision 1 [    1.401845] mdio_bus stmmac-0:00: Driver mv88e6085
> >> requests probe deferral [    2.497476] bus: 'mdio_bus':
> >> driver_probe_device: matched device
> >> stmmac-0:00 with driver mv88e6085
> >> [    2.497486] bus: 'mdio_bus': really_probe: probing driver
> >> mv88e6085 with device stmmac-0:00 [    2.497505] mv88e6085
> >> stmmac-0:00: no pinctrl handle [    2.497536] mv88e6085 stmmac-0:00:
> >> GPIO lookup for consumer reset [    2.497544] mv88e6085 stmmac-0:00:
> >> using device tree for GPIO lookup [    2.497595] mv88e6085
> >> stmmac-0:00: using lookup tables for GPIO lookup [    2.497604]
> >> mv88e6085 stmmac-0:00: No GPIO consumer reset found [    2.497750]
> >> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
> >> revision 1 [    2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized):
> >> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell
> >> 88E1540] [    2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized):
> >> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell
> >> 88E1540] [    2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized):
> >> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell
> >> 88E1540] [    2.917034] driver: 'mv88e6085': driver_bound: bound to
> >> device 'stmmac-0:00'
> >> [    2.917127] bus: 'mdio_bus': really_probe: bound device
> >> stmmac-0:00 to driver mv88e6085 [    8.293526] mv88e6085 stmmac-0:00
> >> lan3: configuring for phy/ link mode [    8.305856] mv88e6085
> >> stmmac-0:00 lan2: configuring for phy/ link mode [    8.325009]
> >> mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode [
> >> 8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link
> >> mode [    8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up -
> >> 100Mbps/Full - flow control off [   12.034974] mv88e6085 stmmac-0:00
> >> lan1: Link is Up - 1Gbps/Full - flow control rx/tx
> >>
> >>
> >
> >
> 
> 
> --
> Regards
> Phil

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Florian Fainelli]

On 3/19/2019 7:34 PM, liweihang wrote:
> Hi all,
> 
> I've met a similar issue and sent an email to discuss about it before:
> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
> 
> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
> I reverted this patch and the auto-negotiation works ok.
> 
> Florian, could you please read my previous email and give me some advice?

If you can copy the patch author on that email the next time that will
help expedite things.

So the problem seems to come from the fact that unless the BCMR_RESET
bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
does that sound like what you are observing?

Does the following work for you (Phil and yourself)?

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3ccba37bd6dd..6a1ea4c2042a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
*phydev)
                err = m88e1121_config_aneg_rgmii_delays(phydev);
                if (err < 0)
                        return err;
+
+               err = genphy_soft_reset(phydev);
+               if (err < 0)
+                       return err;
        }

        err = marvell_set_polarity(phydev, phydev->mdix_ctrl);

> 
> 
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Phil Reid
>> Sent: Wednesday, March 20, 2019 9:34 AM
>> To: Florian Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org
>> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
>> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
>> cphealy@gmail.com; clemens.gruber@pqgruber.com; hkallweit1@gmail.com;
>> nbd@nbd.name; harini.katakam@xilinx.com
>> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
>>
>> On 20/03/2019 12:53 am, Florian Fainelli wrote:
>>> On 3/18/19 6:32 PM, Phil Reid wrote:
>>>> On 19/03/2019 1:09 am, Florian Fainelli wrote:
>>>>> On 3/17/19 7:11 PM, Phil Reid wrote:
>>>>>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>>>>>>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>>>>>>> G'day All,
>>>>>>>>
>>>>>>>> I've just update from kernel 4.19 to 5.0 on a custom board that
>>>>>>>> has a marvell dsa mv88e6085 and the phy on the mv88e6085 will
>>>>>>>> only connect at 10Mb/s with the above mentioned patch applied.
>>>>>>>>
>>>>>>>> Bisecting the issue lead me to the following patch.
>>>>>>>>
>>>>>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>>>>>>
>>>>>>>> Revert the patch, and the associated build fix
>>>>>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.) restores
>>>>>>>> connections to 1Gb/s.
>>>>>>>>
>>>>>>>> Anyone have any thoughts as to the correct fix?
>>>>>>>
>>>>>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need
>> to
>>>>>>> add a specific entry in drivers/net/phy/marvell.c to restore the
>>>>>>> software reset to commit changes to the register.
>>>>>>>
>>>>>> G'day Florian,
>>>>>>
>>>>>> OUI is 0x005043
>>>>>> Model is 101011
>>>>>>
>>>>>> Phy1ID: 0x0141
>>>>>> Phy2ID: 0x0eb1
>>>>>>
>>>>>> The running phy driver is "Marvell 88E1540"
>>>>>
>>>>> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch,
>>>>> did you mean that the mv88e6085 compatible string is used in Device
>>>>> Tree to designate that chip? While the 88E1540 driver is picked up,
>>>>> that driver is originally for external PHY packages (AFAICT), so
>>>>> there could be some switch-specific integration of this PHY that
>>>>> makes it behave differently, having the OUI helps narrow down what
>>>>> might be necessary to accomplish.
>>>>>
>>>>> FWIW, the changes were originally tested with a 88e6352.
>>>>>
>>>>
>>>> Oops yes that's the compatible string.
>>>> Mfr Code we purchased was 88E6352-A1-TFJ2I000
>>>>
>>>> The switch id registers (0x03) returns 0x3521
>>>>
>>>> Odd it works ok for others.
>>>
>>> One possible difference is that the device this was tested with has an
>>> EEPROM which gets involved in configuring the switch during PoR (IIRC)
>>> and that could produce different results. Do you have any switch
>>> initialization happening in the boot loader by any chance?
>>>
>> Yes uboot is configuring the switch at boot as a simple network switch.
>> Then we reconfig it in the kernel. I see if I can get some time to bypass the
>> uboot init and just rely on the kernel to config the chip.
>>
>> The external reset pin is available as well. I tried adding that to the dts but
>> that seems to make things worse. With the patch reverted and external
>> reset pin enabled  I see a message saying that the phy connected at 1G but
>> can not receive anything, even on the fixed phy link to our wifi device.
>>
>> This is the config being applied by uboot if it helps.
>>
>> /* ------------------------------------------------------------------------- */ struct
>> mv88e_sw_reg extsw_conf[] = {
>> 	/*
>> 	 * port 0, PIGGY4, autoneg
>> 	 * first the fix for the 1000Mbits Autoneg, this is from
>> 	 * a Marvell errata, the regs are undocumented
>> 	 */
>> 	{ PHY(0), PHY_PAGE, AN1000FIX_PAGE },
>> 	{ PHY(0), PHY_STATUS, AN1000FIX },
>> 	{ PHY(0), PHY_PAGE, 0 },
>> 	/* now the real port and phy configuration */
>> 	/* port 0 unused */
>> 	{ PORT(0), PORT_CTRL, PORT_DIS },
>> 	{ PHY (0), PHY_PAGE, 0 },
>> 	{ PHY (0), PHY_CTRL, PHY_PWR_DOWN },
>> 	{ PHY (0), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
>> 	/* port 1 unused */
>> 	{ PORT(1), PORT_CTRL, PORT_DIS },
>> 	{ PHY (1), PHY_PAGE, 0 },
>> 	{ PHY (1), PHY_CTRL, PHY_PWR_DOWN },
>> 	{ PHY (1), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
>> 	/* port 2, (physical jack 0) */
>> 	{ PORT(2), PORT_PHY, NO_SPEED_FOR },
>> 	{ PORT(2), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> 	{ PHY (2), PHY_PAGE, 0 },
>> 	{ PHY (2), PHY_1000_CTRL, ADV_1000_FDPX },
>> 	{ PHY (2), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> 	{ PHY (2), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> 	/* port 3, (physical jack 1)  */
>> 	{ PORT(3), PORT_PHY, NO_SPEED_FOR },
>> 	{ PORT(3), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> 	{ PHY (3), PHY_PAGE, 0 },
>> 	{ PHY (3), PHY_1000_CTRL, ADV_1000_FDPX },
>> 	{ PHY (3), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> 	{ PHY (3), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> 	/* port 4, (physical jack 2)  */
>> 	{ PORT(4), PORT_PHY, NO_SPEED_FOR },
>> 	{ PORT(4), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> 	{ PHY (4), PHY_PAGE, 0 },
>> 	{ PHY (4), PHY_1000_CTRL, ADV_1000_FDPX },
>> 	{ PHY (4), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> 	{ PHY (4), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> 	/* port 5, CPU_RGMII */
>> 	//{ PORT(5), PORT_CTRL, PORT_DIS },
>> 	{ PORT(5), PORT_PHY,                               FLOW_CTRL_EN |
>> FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX | FULL_DPX_FOR |
>> SPEED_100_FOR },
>> 	{ PORT(5), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> 	{ PORT(5), PORT_LED, PORT_LED_UPDATE | PORT_LED_CTL |
>> PORT5_L0_ACT5 | PORT5_L1_ACT6 },
>> 	/* port 6, unused, this port has no phy */
>> 	{ PORT(6), PORT_PHY, RX_RGMII_TIM | TX_RGMII_TIM |
>> FLOW_CTRL_EN | FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX |
>> FULL_DPX_FOR | SPEED_1000_FOR },
>> 	{ PORT(6), PORT_CTRL, FORWARDING | EGRS_FLD_ALL }, };
>>
>>>>
>>>> Here's the relevant dmesgs (with the patch reverted)
>>>>
>>>> [    1.332652] bus: 'mdio_bus': add driver mv88e6085 [    1.368572]
>>>> bus: 'mdio_bus': driver_probe_device: matched device
>>>> stmmac-0:00 with driver mv88e6085
>>>> [    1.368584] bus: 'mdio_bus': really_probe: probing driver
>>>> mv88e6085 with device stmmac-0:00 [    1.368603] mv88e6085
>>>> stmmac-0:00: no pinctrl handle [    1.368630] mv88e6085 stmmac-0:00:
>>>> GPIO lookup for consumer reset [    1.368638] mv88e6085 stmmac-0:00:
>>>> using device tree for GPIO lookup [    1.368687] mv88e6085
>>>> stmmac-0:00: using lookup tables for GPIO lookup [    1.368696]
>>>> mv88e6085 stmmac-0:00: No GPIO consumer reset found [    1.368835]
>>>> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
>>>> revision 1 [    1.401845] mdio_bus stmmac-0:00: Driver mv88e6085
>>>> requests probe deferral [    2.497476] bus: 'mdio_bus':
>>>> driver_probe_device: matched device
>>>> stmmac-0:00 with driver mv88e6085
>>>> [    2.497486] bus: 'mdio_bus': really_probe: probing driver
>>>> mv88e6085 with device stmmac-0:00 [    2.497505] mv88e6085
>>>> stmmac-0:00: no pinctrl handle [    2.497536] mv88e6085 stmmac-0:00:
>>>> GPIO lookup for consumer reset [    2.497544] mv88e6085 stmmac-0:00:
>>>> using device tree for GPIO lookup [    2.497595] mv88e6085
>>>> stmmac-0:00: using lookup tables for GPIO lookup [    2.497604]
>>>> mv88e6085 stmmac-0:00: No GPIO consumer reset found [    2.497750]
>>>> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
>>>> revision 1 [    2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized):
>>>> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell
>>>> 88E1540] [    2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized):
>>>> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell
>>>> 88E1540] [    2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized):
>>>> PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell
>>>> 88E1540] [    2.917034] driver: 'mv88e6085': driver_bound: bound to
>>>> device 'stmmac-0:00'
>>>> [    2.917127] bus: 'mdio_bus': really_probe: bound device
>>>> stmmac-0:00 to driver mv88e6085 [    8.293526] mv88e6085 stmmac-0:00
>>>> lan3: configuring for phy/ link mode [    8.305856] mv88e6085
>>>> stmmac-0:00 lan2: configuring for phy/ link mode [    8.325009]
>>>> mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode [
>>>> 8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link
>>>> mode [    8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up -
>>>> 100Mbps/Full - flow control off [   12.034974] mv88e6085 stmmac-0:00
>>>> lan1: Link is Up - 1Gbps/Full - flow control rx/tx
>>>>
>>>>
>>>
>>>
>>
>>
>> --
>> Regards
>> Phil

-- 
Florian

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 20/03/2019 11:37 am, Florian Fainelli wrote:
> 
> 
> On 3/19/2019 7:34 PM, liweihang wrote:
>> Hi all,
>>
>> I've met a similar issue and sent an email to discuss about it before:
>> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
>>
>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
>> I reverted this patch and the auto-negotiation works ok.
>>
>> Florian, could you please read my previous email and give me some advice?
> 
> If you can copy the patch author on that email the next time that will
> help expedite things.
> 
> So the problem seems to come from the fact that unless the BCMR_RESET
> bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
> does that sound like what you are observing?
> 
> Does the following work for you (Phil and yourself)?
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 3ccba37bd6dd..6a1ea4c2042a 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
> *phydev)
>                  err = m88e1121_config_aneg_rgmii_delays(phydev);
>                  if (err < 0)
>                          return err;
> +
> +               err = genphy_soft_reset(phydev);
> +               if (err < 0)
> +                       return err;
>          }
> 
>          err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> 


G'day Florian,

Nope that didn't work for me.
But based on that patch and liweihang email I found the following works for me:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 46c8672..de71aef 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1827,7 +1827,13 @@ int genphy_soft_reset(struct phy_device *phydev)
  {
         int ret;

-       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
+       phydev_err(phydev, "genphy_soft_reset");
+
+       ret = phy_read(phydev, MII_BMCR);
+       if (ret < 0)
+               return ret;
+
+       ret = phy_write(phydev, MII_BMCR, ret | BMCR_RESET);
         if (ret < 0)
                 return ret;








-- 
Regards
Phil

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Heiner Kallweit]

On 20.03.2019 06:16, Phil Reid wrote:
> On 20/03/2019 11:37 am, Florian Fainelli wrote:
>>
>>
>> On 3/19/2019 7:34 PM, liweihang wrote:
>>> Hi all,
>>>
>>> I've met a similar issue and sent an email to discuss about it before:
>>> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
>>>
>>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
>>> I reverted this patch and the auto-negotiation works ok.
>>>
>>> Florian, could you please read my previous email and give me some advice?
>>
>> If you can copy the patch author on that email the next time that will
>> help expedite things.
>>
>> So the problem seems to come from the fact that unless the BCMR_RESET
>> bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
>> does that sound like what you are observing?
>>
>> Does the following work for you (Phil and yourself)?
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index 3ccba37bd6dd..6a1ea4c2042a 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
>> *phydev)
>>                  err = m88e1121_config_aneg_rgmii_delays(phydev);
>>                  if (err < 0)
>>                          return err;
>> +
>> +               err = genphy_soft_reset(phydev);
>> +               if (err < 0)
>> +                       return err;
>>          }
>>
>>          err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>>
> 
> 
> G'day Florian,
> 
> Nope that didn't work for me.
> But based on that patch and liweihang email I found the following works for me:
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 46c8672..de71aef 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1827,7 +1827,13 @@ int genphy_soft_reset(struct phy_device *phydev)
>  {
>         int ret;
> 
> -       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
> +       phydev_err(phydev, "genphy_soft_reset");
> +
> +       ret = phy_read(phydev, MII_BMCR);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = phy_write(phydev, MII_BMCR, ret | BMCR_RESET);

Hmm, that would mean in your case some set bit needs to be preserved.
Usually that's not needed. Could you please check which value is read
from MII_BMCR?

>         if (ret < 0)
>                 return ret;
> 
> 
> 
> 
> 
> 
> 
> 

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 20/03/2019 2:39 pm, Heiner Kallweit wrote:
> On 20.03.2019 06:16, Phil Reid wrote:
>> On 20/03/2019 11:37 am, Florian Fainelli wrote:
>>>
>>>
>>> On 3/19/2019 7:34 PM, liweihang wrote:
>>>> Hi all,
>>>>
>>>> I've met a similar issue and sent an email to discuss about it before:
>>>> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
>>>>
>>>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
>>>> I reverted this patch and the auto-negotiation works ok.
>>>>
>>>> Florian, could you please read my previous email and give me some advice?
>>>
>>> If you can copy the patch author on that email the next time that will
>>> help expedite things.
>>>
>>> So the problem seems to come from the fact that unless the BCMR_RESET
>>> bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
>>> does that sound like what you are observing?
>>>
>>> Does the following work for you (Phil and yourself)?
>>>
>>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>>> index 3ccba37bd6dd..6a1ea4c2042a 100644
>>> --- a/drivers/net/phy/marvell.c
>>> +++ b/drivers/net/phy/marvell.c
>>> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
>>> *phydev)
>>>                   err = m88e1121_config_aneg_rgmii_delays(phydev);
>>>                   if (err < 0)
>>>                           return err;
>>> +
>>> +               err = genphy_soft_reset(phydev);
>>> +               if (err < 0)
>>> +                       return err;
>>>           }
>>>
>>>           err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>>>
>>
>>
>> G'day Florian,
>>
>> Nope that didn't work for me.
>> But based on that patch and liweihang email I found the following works for me:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 46c8672..de71aef 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1827,7 +1827,13 @@ int genphy_soft_reset(struct phy_device *phydev)
>>   {
>>          int ret;
>>
>> -       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
>> +       phydev_err(phydev, "genphy_soft_reset");
>> +
>> +       ret = phy_read(phydev, MII_BMCR);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = phy_write(phydev, MII_BMCR, ret | BMCR_RESET);
> 
> Hmm, that would mean in your case some set bit needs to be preserved.
> Usually that's not needed. Could you please check which value is read
> from MII_BMCR?
> 

The relevant log is:
[    2.513985] bus: 'mdio_bus': driver_probe_device: matched device stmmac-0:00 with driver mv88e6085
[    2.513995] bus: 'mdio_bus': really_probe: probing driver mv88e6085 with device stmmac-0:00
[    2.514011] mv88e6085 stmmac-0:00: no pinctrl handle
[    2.514040] mv88e6085 stmmac-0:00: GPIO lookup for consumer reset
[    2.514048] mv88e6085 stmmac-0:00: using device tree for GPIO lookup
[    2.514097] mv88e6085 stmmac-0:00: using lookup tables for GPIO lookup
[    2.514106] mv88e6085 stmmac-0:00: No GPIO consumer reset found
[    2.514251] mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352, revision 1
[    2.884936] mv88e6085 stmmac-0:00 lan1 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:02] driver [Marvell 88E1540]
[    2.905230] mv88e6085 stmmac-0:00 lan2 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:03] driver [Marvell 88E1540]
[    2.927978] mv88e6085 stmmac-0:00 lan3 (uninitialized): PHY [!soc!ethernet@ff702000!mdio!switch0@0!mdio:04] driver [Marvell 88E1540]
[    2.941852] driver: 'mv88e6085': driver_bound: bound to device 'stmmac-0:00'
[    2.941900] bus: 'mdio_bus': really_probe: bound device stmmac-0:00 to driver mv88e6085
[    8.447971] mv88e6085 stmmac-0:00 lan2: configuring for phy/ link mode
[    8.462773] mv88e6085 stmmac-0:00 lan3: configuring for phy/ link mode
[    8.476192] mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode
[    8.488751] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link mode
[    8.511087] mv88e6085 stmmac-0:00 wifi: Link is Up - 100Mbps/Full - flow control off
[    8.553462] Marvell 88E1540 !soc!ethernet@ff702000!mdio!switch0@0!mdio:03: genphy_soft_reset 00001140
[    8.564818] Marvell 88E1540 !soc!ethernet@ff702000!mdio!switch0@0!mdio:04: genphy_soft_reset 00001140
[    8.588558] Marvell 88E1540 !soc!ethernet@ff702000!mdio!switch0@0!mdio:02: genphy_soft_reset 00001140
[   12.330708] mv88e6085 stmmac-0:00 lan2: Link is Up - 1Gbps/Full - flow control rx/tx

A read of the 88e6352 docs indicates that the reset bit must be written
simultaneously with the auto negotiation bit. (bit 12)
This also seems to be the case for the link speed and I think the
duplex mode.

-- 
Regards
Phil

RE: regression from: net: phy: marvell: Avoid unnecessary soft reset

[liweihang]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Wednesday, March 20, 2019 11:37 AM
> To: liweihang <liweihang@hisilicon.com>; Phil Reid
> <preid@electromag.com.au>; netdev@vger.kernel.org
> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
> cphealy@gmail.com; clemens.gruber@pqgruber.com; hkallweit1@gmail.com;
> nbd@nbd.name; harini.katakam@xilinx.com
> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
> 
> 
> 
> On 3/19/2019 7:34 PM, liweihang wrote:
> > Hi all,
> >
> > I've met a similar issue and sent an email to discuss about it before:
> > Question about setting speed and duplex failed after auto-negotiation
> > disabled on marvell phy
> >
> > d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset I
> > reverted this patch and the auto-negotiation works ok.
> >
> > Florian, could you please read my previous email and give me some advice?
> 
> If you can copy the patch author on that email the next time that will help
> expedite things.
> 
> So the problem seems to come from the fact that unless the BCMR_RESET bit
> is written, then m88e1121_config_aneg_rgmii_delays() has no effect, does
> that sound like what you are observing?
> 
> Does the following work for you (Phil and yourself)?

Thank you, Florian. But that didn't work for me either. I think the key question is
as what Heiner said, some bits need to be preserved.

The MII_BMCR contained information of speed and duplex mode, but when we 
call genphy_soft_reset(), these bits will be cleared.

> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index
> 3ccba37bd6dd..6a1ea4c2042a 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
> *phydev)
>                 err = m88e1121_config_aneg_rgmii_delays(phydev);
>                 if (err < 0)
>                         return err;
> +
> +               err = genphy_soft_reset(phydev);
> +               if (err < 0)
> +                       return err;
>         }
> 
>         err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> 

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Heiner Kallweit]

On 20.03.2019 13:22, liweihang wrote:
> 
> 
>> -----Original Message-----
>> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
>> Sent: Wednesday, March 20, 2019 11:37 AM
>> To: liweihang <liweihang@hisilicon.com>; Phil Reid
>> <preid@electromag.com.au>; netdev@vger.kernel.org
>> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
>> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
>> cphealy@gmail.com; clemens.gruber@pqgruber.com; hkallweit1@gmail.com;
>> nbd@nbd.name; harini.katakam@xilinx.com
>> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
>>
>>
>>
>> On 3/19/2019 7:34 PM, liweihang wrote:
>>> Hi all,
>>>
>>> I've met a similar issue and sent an email to discuss about it before:
>>> Question about setting speed and duplex failed after auto-negotiation
>>> disabled on marvell phy
>>>
>>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset I
>>> reverted this patch and the auto-negotiation works ok.
>>>
>>> Florian, could you please read my previous email and give me some advice?
>>
>> If you can copy the patch author on that email the next time that will help
>> expedite things.
>>
>> So the problem seems to come from the fact that unless the BCMR_RESET bit
>> is written, then m88e1121_config_aneg_rgmii_delays() has no effect, does
>> that sound like what you are observing?
>>
>> Does the following work for you (Phil and yourself)?
> 
> Thank you, Florian. But that didn't work for me either. I think the key question is
> as what Heiner said, some bits need to be preserved.
> 
> The MII_BMCR contained information of speed and duplex mode, but when we 
> call genphy_soft_reset(), these bits will be cleared.
> 
I think instead of

ret = phy_write(phydev, MII_BMCR, BMCR_RESET);

we should use

ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);

This is still in line with Clause 22 but covers more PHY's. A lot of PHY's
won't be affected because they reset all BMCR bits to a default anyway.
Could you please test this? If it's ok for you I'd submit a patch.

>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index
>> 3ccba37bd6dd..6a1ea4c2042a 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
>> *phydev)
>>                 err = m88e1121_config_aneg_rgmii_delays(phydev);
>>                 if (err < 0)
>>                         return err;
>> +
>> +               err = genphy_soft_reset(phydev);
>> +               if (err < 0)
>> +                       return err;
>>         }
>>
>>         err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>>
> 

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Maxim Uvarov]

also it's suspicions that in m88e1116r_config_init() delay is present
after first soft_reset() and missed after second.

Maxim.

ср, 20 мар. 2019 г. в 21:17, Heiner Kallweit <hkallweit1@gmail.com>:
>
> On 20.03.2019 13:22, liweihang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> >> Sent: Wednesday, March 20, 2019 11:37 AM
> >> To: liweihang <liweihang@hisilicon.com>; Phil Reid
> >> <preid@electromag.com.au>; netdev@vger.kernel.org
> >> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
> >> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
> >> cphealy@gmail.com; clemens.gruber@pqgruber.com; hkallweit1@gmail.com;
> >> nbd@nbd.name; harini.katakam@xilinx.com
> >> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
> >>
> >>
> >>
> >> On 3/19/2019 7:34 PM, liweihang wrote:
> >>> Hi all,
> >>>
> >>> I've met a similar issue and sent an email to discuss about it before:
> >>> Question about setting speed and duplex failed after auto-negotiation
> >>> disabled on marvell phy
> >>>
> >>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset I
> >>> reverted this patch and the auto-negotiation works ok.
> >>>
> >>> Florian, could you please read my previous email and give me some advice?
> >>
> >> If you can copy the patch author on that email the next time that will help
> >> expedite things.
> >>
> >> So the problem seems to come from the fact that unless the BCMR_RESET bit
> >> is written, then m88e1121_config_aneg_rgmii_delays() has no effect, does
> >> that sound like what you are observing?
> >>
> >> Does the following work for you (Phil and yourself)?
> >
> > Thank you, Florian. But that didn't work for me either. I think the key question is
> > as what Heiner said, some bits need to be preserved.
> >
> > The MII_BMCR contained information of speed and duplex mode, but when we
> > call genphy_soft_reset(), these bits will be cleared.
> >
> I think instead of
>
> ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
>
> we should use
>
> ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
>
> This is still in line with Clause 22 but covers more PHY's. A lot of PHY's
> won't be affected because they reset all BMCR bits to a default anyway.
> Could you please test this? If it's ok for you I'd submit a patch.
>
> >>
> >> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index
> >> 3ccba37bd6dd..6a1ea4c2042a 100644
> >> --- a/drivers/net/phy/marvell.c
> >> +++ b/drivers/net/phy/marvell.c
> >> @@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
> >> *phydev)
> >>                 err = m88e1121_config_aneg_rgmii_delays(phydev);
> >>                 if (err < 0)
> >>                         return err;
> >> +
> >> +               err = genphy_soft_reset(phydev);
> >> +               if (err < 0)
> >> +                       return err;
> >>         }
> >>
> >>         err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> >>
> >
>


-- 
Best regards,
Maxim Uvarov

RE: regression from: net: phy: marvell: Avoid unnecessary soft reset

[liweihang]

> -----Original Message-----
> From: Heiner Kallweit [mailto:hkallweit1@gmail.com]
> Sent: Thursday, March 21, 2019 2:15 AM
> To: liweihang <liweihang@hisilicon.com>; Florian Fainelli
> <f.fainelli@gmail.com>; Phil Reid <preid@electromag.com.au>;
> netdev@vger.kernel.org
> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
> cphealy@gmail.com; clemens.gruber@pqgruber.com; nbd@nbd.name;
> harini.katakam@xilinx.com
> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
> 
> On 20.03.2019 13:22, liweihang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> >> Sent: Wednesday, March 20, 2019 11:37 AM
> >> To: liweihang <liweihang@hisilicon.com>; Phil Reid
> >> <preid@electromag.com.au>; netdev@vger.kernel.org
> >> Cc: Andrew Lunn <andrew@lunn.ch>; David S. Miller
> >> <davem@davemloft.net>; dongsheng.wang@hxt-semitech.com;
> >> cphealy@gmail.com; clemens.gruber@pqgruber.com;
> hkallweit1@gmail.com;
> >> nbd@nbd.name; harini.katakam@xilinx.com
> >> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary
> >> soft reset
> >>
> >>
> >>
> >> On 3/19/2019 7:34 PM, liweihang wrote:
> >>> Hi all,
> >>>
> >>> I've met a similar issue and sent an email to discuss about it before:
> >>> Question about setting speed and duplex failed after
> >>> auto-negotiation disabled on marvell phy
> >>>
> >>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset I
> >>> reverted this patch and the auto-negotiation works ok.
> >>>
> >>> Florian, could you please read my previous email and give me some
> advice?
> >>
> >> If you can copy the patch author on that email the next time that
> >> will help expedite things.
> >>
> >> So the problem seems to come from the fact that unless the BCMR_RESET
> >> bit is written, then m88e1121_config_aneg_rgmii_delays() has no
> >> effect, does that sound like what you are observing?
> >>
> >> Does the following work for you (Phil and yourself)?
> >
> > Thank you, Florian. But that didn't work for me either. I think the
> > key question is as what Heiner said, some bits need to be preserved.
> >
> > The MII_BMCR contained information of speed and duplex mode, but
> when
> > we call genphy_soft_reset(), these bits will be cleared.
> >
> I think instead of
> 
> ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
> 
> we should use
> 
> ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> 
> This is still in line with Clause 22 but covers more PHY's. A lot of PHY's won't
> be affected because they reset all BMCR bits to a default anyway.
> Could you please test this? If it's ok for you I'd submit a patch.
> 

Thank you, Heiner. It works ok on my device with following change as you
suggested.

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 509d940..7241646 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1874,7 +1874,7 @@ int genphy_soft_reset(struct phy_device *phydev)
 {
        int ret;
 
-       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
+       ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
        if (ret < 0)
                return ret;

Re: regression from: net: phy: marvell: Avoid unnecessary soft reset

[Phil Reid]

On 21/03/2019 2:16 pm, liweihang wrote:
> 
>>>>
>>>> On 3/19/2019 7:34 PM, liweihang wrote:
>>>>> Hi all,
>>>>>
>>>>> I've met a similar issue and sent an email to discuss about it before:
>>>>> Question about setting speed and duplex failed after
>>>>> auto-negotiation disabled on marvell phy
>>>>>
>>>>> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset I
>>>>> reverted this patch and the auto-negotiation works ok.
>>>>>
>>>>> Florian, could you please read my previous email and give me some
>> advice?
>>>>
>>>> If you can copy the patch author on that email the next time that
>>>> will help expedite things.
>>>>
>>>> So the problem seems to come from the fact that unless the BCMR_RESET
>>>> bit is written, then m88e1121_config_aneg_rgmii_delays() has no
>>>> effect, does that sound like what you are observing?
>>>>
>>>> Does the following work for you (Phil and yourself)?
>>>
>>> Thank you, Florian. But that didn't work for me either. I think the
>>> key question is as what Heiner said, some bits need to be preserved.
>>>
>>> The MII_BMCR contained information of speed and duplex mode, but
>> when
>>> we call genphy_soft_reset(), these bits will be cleared.
>>>
>> I think instead of
>>
>> ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
>>
>> we should use
>>
>> ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
>>
>> This is still in line with Clause 22 but covers more PHY's. A lot of PHY's won't
>> be affected because they reset all BMCR bits to a default anyway.
>> Could you please test this? If it's ok for you I'd submit a patch.
>>
> 
> Thank you, Heiner. It works ok on my device with following change as you
> suggested.
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 509d940..7241646 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1874,7 +1874,7 @@ int genphy_soft_reset(struct phy_device *phydev)
>   {
>          int ret;
>   
> -       ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
> +       ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
>          if (ret < 0)
>                  return ret;
> 

Thank you, Heiner.
I confirm this also works for me.


-- 
Regards
Phil

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

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 25 Sep 2018 11:28:44 -0700

> 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!
> 
> Changes since RFT:
> 
> - added Tested-by tags from Wang, Dongsheng, Andrew, Chris and Clemens

Series applied.