[REGRESSION, BISECTED] Broken networking with net/phy/marvell

[REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Aaro Koskinen]

Hi,

When upgrading from v4.13 to v4.14-rc6 on OpenRD Client, the box loses
network connectivity.

Bisection points to:

commit 5987feb38aa55e035ce5376c02aba88a604cc881
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Fri Aug 4 11:17:21 2017 +0300

    net: phy: marvell: logical vs bitwise OR typo
    
However, it seems this commit just unhides another issue in the original
commit 864dc729d528 ("net: phy: marvell: Refactor m88e1121 RGMII delay
configuration"): when we are configuring the MSCR delay bits, we are
probably clearing the bits with a wrong mask (i.e. we might be disabling
something else not intended)...

I have tested the below change and it seems to fix the networking. Any
comments?

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 15cbcdb..500d7c1 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -474,7 +474,7 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 		goto out;
 	}
 
-	mscr &= MII_88E1121_PHY_MSCR_DELAY_MASK;
+	mscr &= ~MII_88E1121_PHY_MSCR_DELAY_MASK;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |

A.

Re: [REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Dan Carpenter]

On Thu, Oct 26, 2017 at 03:28:36PM +0300, Aaro Koskinen wrote:
> Hi,
> 
> When upgrading from v4.13 to v4.14-rc6 on OpenRD Client, the box loses
> network connectivity.
> 
> Bisection points to:
> 
> commit 5987feb38aa55e035ce5376c02aba88a604cc881
> Author: Dan Carpenter <dan.carpenter@oracle.com>
> Date:   Fri Aug 4 11:17:21 2017 +0300
> 
>     net: phy: marvell: logical vs bitwise OR typo
>     
> However, it seems this commit just unhides another issue in the original
> commit 864dc729d528 ("net: phy: marvell: Refactor m88e1121 RGMII delay
> configuration"): when we are configuring the MSCR delay bits, we are
> probably clearing the bits with a wrong mask (i.e. we might be disabling
> something else not intended)...
> 
> I have tested the below change and it seems to fix the networking. Any
> comments?
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 15cbcdb..500d7c1 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -474,7 +474,7 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
>  		goto out;
>  	}
>  
> -	mscr &= MII_88E1121_PHY_MSCR_DELAY_MASK;
> +	mscr &= ~MII_88E1121_PHY_MSCR_DELAY_MASK;
>  

I don't think your fix is right.  That mask was like that in the
original m88e1121_config_aneg() code before commit 864dc729d528 ("net:
phy: marvell: Refactor m88e1121 RGMII delay configuration").  Perhaps
the bug is that m88e1116r_config_init() used to not have the mask and
now it does?

Another difference I see is that we also didn't used to call
marvell_set_page(phydev, oldpage); on error, but I think that's a bugfix
and not related to what you're seeing.

I don't know the code well enough to write a fix.

regards,
dan carpenter

Re: [REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Dan Carpenter]

You have a "Marvell 88E1116R" device, right?

We used to set MII_88E1121_PHY_MSCR_RX_DELAY and MII_88E1121_PHY_MSCR_TX_DELAY
bits unconditionally but now it depends on phydev->interface.  I don't
know the code/hardware well enough to say what phydev->interface is for
you.

regards,
dan carpenter

Re: [REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Andrew Lunn]

On Thu, Oct 26, 2017 at 03:28:36PM +0300, Aaro Koskinen wrote:
> Hi,
> 
> When upgrading from v4.13 to v4.14-rc6 on OpenRD Client, the box loses
> network connectivity.

Hi Aaro

What exactly is the PHY in the OpenRD?

>  
> -	mscr &= MII_88E1121_PHY_MSCR_DELAY_MASK;
> +	mscr &= ~MII_88E1121_PHY_MSCR_DELAY_MASK;

Please can you print the value of mscr before it is changed.

One possibility, is that the bootloader is setting the PHY to
rgmii-id. This is now getting cleared, where earlier it was
preserved. If this is true, it should be solved by adding

phy-mode = "rgmii-id" to the ethernet node in the device tree.

	 Andrew

Re: [REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Aaro Koskinen]

Hi,

On Sat, Oct 28, 2017 at 09:01:39PM +0200, Andrew Lunn wrote:
> On Thu, Oct 26, 2017 at 03:28:36PM +0300, Aaro Koskinen wrote:
> > When upgrading from v4.13 to v4.14-rc6 on OpenRD Client, the box loses
> > network connectivity.
> 
> What exactly is the PHY in the OpenRD?

88E1116R.

> Please can you print the value of mscr before it is changed.

[    3.377836] MSCR BEFORE: 00001070
[    3.381176]  MSCR AFTER: 00001040

> One possibility, is that the bootloader is setting the PHY to
> rgmii-id. This is now getting cleared, where earlier it was
> preserved. If this is true, it should be solved by adding
> 
> phy-mode = "rgmii-id" to the ethernet node in the device tree.

Ok, that seems to help:

[    3.377852] MSCR BEFORE: 00001070
[    3.381189]  MSCR AFTER: 00001070

diff --git a/arch/arm/boot/dts/kirkwood-openrd-client.dts b/arch/arm/boot/dts/kirkwood-openrd-client.dts
index 96ff59d..2bf3cb3 100644
--- a/arch/arm/boot/dts/kirkwood-openrd-client.dts
+++ b/arch/arm/boot/dts/kirkwood-openrd-client.dts
@@ -65,6 +65,7 @@
 	status = "okay";
 	ethernet0-port@0 {
 		phy-handle = <&ethphy0>;
+		phy-mode = "rgmii-id";
 	};
 };
 
@@ -72,6 +73,7 @@
 	status = "okay";
 	ethernet1-port@0 {
 		phy-handle = <&ethphy1>;
+		phy-mode = "rgmii-id";
 	};
 };

A.

Re: [REGRESSION, BISECTED] Broken networking with net/phy/marvell

[Andrew Lunn]

On Mon, Oct 30, 2017 at 02:19:54AM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On Sat, Oct 28, 2017 at 09:01:39PM +0200, Andrew Lunn wrote:
> > On Thu, Oct 26, 2017 at 03:28:36PM +0300, Aaro Koskinen wrote:
> > > When upgrading from v4.13 to v4.14-rc6 on OpenRD Client, the box loses
> > > network connectivity.
> > 
> > What exactly is the PHY in the OpenRD?
> 
> 88E1116R.
> 
> > Please can you print the value of mscr before it is changed.
> 
> [    3.377836] MSCR BEFORE: 00001070
> [    3.381176]  MSCR AFTER: 00001040
 
> > One possibility, is that the bootloader is setting the PHY to
> > rgmii-id.

So the initial value does seem to suggest the bootloader is setting
it.

 This is now getting cleared, where earlier it was
> > preserved. If this is true, it should be solved by adding
> > 
> > phy-mode = "rgmii-id" to the ethernet node in the device tree.
> 
> Ok, that seems to help:
> 
> [    3.377852] MSCR BEFORE: 00001070
> [    3.381189]  MSCR AFTER: 00001070

Great. Thanks for testing this. I don't like it regressing though, so
i want to see if it is possible to fix this without needing to add the
phy mode. I hope to look at this in the next couple of days.

    Thanks

	Andrew