Skip to contentSkip to conversation

Marvell Phy (1510) issue since v4.7 kernel

PATCH Murali Karicheri 4 participants 21 messages Jan 9, 2017 – Jan 12, 2017

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Murali Karicheri

author
#0 lore raw

Hello Charles-Antoine, Andrew,

We have recently upgraded our kernel to v4.9 and started seeing an issue on our Keystone EVMs (K2E/L) that uses Marvel Phy 1510. The issue is that when we do a reboot command from the Linux console or do a SoC reset, the DHCP times out at U-Boot due to Phy auto negotiation failure. This works fine when the board is power cycled.

This is traced back to v4.7 where following commits were introduced:-

3758be3dc162b56ea Marvell phy: add functions to suspend and resume both interfaces: fiber and copper links. 78301ebe9b5a2645d Marvell phy: add configuration of autonegociation for fiber link. 2170fef78a400ca3c Marvell phy: add field to get errors from fiber link.
6cfb3bcc064110995 Marvell phy: check link status in case of fiber link

Specifically the commit 6cfb3bcc0641109951a124019cd2e0623107d18d which changed the marvell_read_status() behavior

Once we add the below HACK, this works fine.

commit e5bd8bfe7f544df03772c094331bb27e1a5a5600

Author: Murali Karicheri <[email protected]>
Date:   Fri Jan 6 12:22:13 2017 -0500

TEMP: work around in marvel Phy driver for u-boot dhcp timeout

Signed-off-by Murali Karicheri <[email protected]>
drivers/net/phy/marvell.c +1 -1
index c2dcf02..e91bdd3 100644
@@ -1194,7 +1194,7 @@ static int marvell_read_status(struct phy_device *phydev)
        int err;
 
        /* Check the fiber mode first */
-       if (phydev->supported & SUPPORTED_FIBRE) {
+       if (!(phydev->supported & SUPPORTED_FIBRE)) {
                err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
                if (err < 0)
                        goto error;

Also we see comments/logic in the code that contradicts with each other

For example in marvell_read_status(), it says "Check the fiber mode first and uses the logic

     if (phydev->supported & SUPPORTED_FIBRE)

Where as in marvell_resume() comment says 'Resume the fiber mode first' and uses the logic

     if (!(phydev->supported & SUPPORTED_FIBRE))

Both are not symmetrical. Could you please explain?

Signature · 2 lines
Murali Karicheri
Linux Kernel, Keystone
maintainer
#1 lore raw
Quoted 7 lines · Murali Karicheri

Hello Charles-Antoine, Andrew,

We have recently upgraded our kernel to v4.9 and started seeing an issue on our Keystone EVMs (K2E/L) that uses Marvel Phy 1510. The issue is that when we do a reboot command from the Linux console or do a SoC reset, the DHCP times out at U-Boot due to Phy auto negotiation failure. This works fine when the board is power cycled.

I've seen similar before. What version of kernel are you upgrading from?

The problem i've had is that on shutdown/reboot, linux powers the PHYs off. The uboot i have does not power them on again. Clearly a uboot issue, it should not assume the PHYs are powered on.

I work around this with a uboot command:

mii write 1 0 0x3100

where the PHY is at address 1 on the bus. This writes to register 0, and the import bit which needs setting to 0 is bit 11.

If mii read shows bit 11 is set, you know there uboot did not power up the PHY.

Andrew

#2 lore raw

Hi Andrew,

Quoted 31 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 5:57 PM
To: Karicheri, Muralidharan
Cc: [email protected]; Kwok, WingMan
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 3 lines · Murali Karicheri

Hello Charles-Antoine, Andrew,

We have recently upgraded our kernel to v4.9 and started seeing an

issue

Quoted 1 line

on our Keystone EVMs (K2E/L) that uses Marvel Phy 1510. The issue is

that

Quoted 1 line

when we do a reboot command from the Linux console or do a SoC

reset,

Quoted 1 line

the DHCP times out at U-Boot due to Phy auto negotiation failure.

This works

Quoted 1 line

fine when the board is power cycled.

I've seen similar before. What version of kernel are you upgrading from?

The problem i've had is that on shutdown/reboot, linux powers the PHYs off. The uboot i have does not power them on again. Clearly a uboot issue, it should not assume the PHYs are powered on.

I work around this with a uboot command:

mii write 1 0 0x3100

Quoted 7 lines

where the PHY is at address 1 on the bus. This writes to register 0, and the import bit which needs setting to 0 is bit 11.

If mii read shows bit 11 is set, you know there uboot did not power up the PHY.

Andrew

Thanks for the suggestion.

But when kernel is reboot into u-boot, "mii read 0 0" shows 0x1000, ie. bit 11 is 0, and still the phy auto-nego times out.

The "mii read 0 0" shows the same value 0x1000 if we power cycle board and the phy auto-nego works fine.

Could there be some other registers that need to be re-configured?

We are upgrading from kernel v4.4 to v4.9. And the u-boot is 2016.05.

Regards,
WingMan

maintainer
#3 lore raw
Quoted 2 lines

But when kernel is reboot into u-boot, "mii read 0 0" shows 0x1000, ie. bit 11 is 0, and still the phy auto-nego times out.

O.K, not so simple then.

I suggest you dump all the registers, in both the good and bad state, and see how they differ.

Andrew

#4 lore raw
Quoted 12 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 6:56 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 2 lines

But when kernel is reboot into u-boot, "mii read 0 0" shows 0x1000, ie. bit 11 is 0, and still the phy auto-nego times out.

Quoted 6 lines

O.K, not so simple then.

I suggest you dump all the registers, in both the good and bad state, and see how they differ.

Andrew

From Marvell's brief description http://www.marvell.com/transceivers/alaska-gbe/,
it seems that 88E1510/1518 don't support fiber. Only 88E1512 does. In that case, the fiber support patch is not applicable to 88E1510/1518.

I investigate a little more of the u-boot auto-nego time out problem after reboot and found that the problem is caused by marvell_read_status() which leaves the fiber page out because the phydev->link is TRUE. But if fiber is not applicable to 88E1510/1518, this is an invalid check.

Any suggestion?

WingMan

maintainer
#5 lore raw
Quoted 3 lines

From Marvell's brief description http://www.marvell.com/transceivers/alaska-gbe/,
it seems that 88E1510/1518 don't support fiber. Only 88E1512 does. In that case, the fiber support patch is not applicable to 88E1510/1518.

O.K. That makes it easier.

Please add the relevant IDs to include/linux/marvell.h, and add entries to driver/net/phy/marvell.c for 1512 with SUPPORTED_FIBRE and 1510 and 1518 without.

Andrew

#6 lore raw
Quoted 16 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 8:54 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 1 line

From Marvell's brief description

http://www.marvell.com/transceivers/alaska-gbe/,

Quoted 1 line

it seems that 88E1510/1518 don't support fiber. Only 88E1512 does.

In

Quoted 1 line

that case, the fiber support patch is not applicable to

88E1510/1518.

Quoted 7 lines

O.K. That makes it easier.

Please add the relevant IDs to include/linux/marvell.h, and add entries to driver/net/phy/marvell.c for 1512 with SUPPORTED_FIBRE and 1510 and 1518 without.

Andrew

By any chance you have the ID of 1512?

Thanks,
WingMan

maintainer
#7 lore raw
Quoted 18 lines · WingMan
Quoted 18 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 8:54 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 1 line

From Marvell's brief description

http://www.marvell.com/transceivers/alaska-gbe/,

Quoted 1 line

it seems that 88E1510/1518 don't support fiber. Only 88E1512 does.

In

Quoted 1 line

that case, the fiber support patch is not applicable to

88E1510/1518.

O.K. That makes it easier.

Quoted 7 lines
Quoted 5 lines

Please add the relevant IDs to include/linux/marvell.h, and add entries to driver/net/phy/marvell.c for 1512 with SUPPORTED_FIBRE and 1510 and 1518 without.

Andrew

By any chance you have the ID of 1512?

Nope, sorry.

Try Russell King.

Andrew

#8 lore raw
Quoted 36 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Wednesday, January 11, 2017 5:28 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 9 lines · WingMan
Quoted 9 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 8:54 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

From Marvell's brief description

http://www.marvell.com/transceivers/alaska-gbe/,

Quoted 1 line

it seems that 88E1510/1518 don't support fiber. Only 88E1512

does.

Quoted 8 lines
Quoted 8 lines

In

Quoted 1 line

that case, the fiber support patch is not applicable to

88E1510/1518.

O.K. That makes it easier.

Please add the relevant IDs to include/linux/marvell.h, and add entries to driver/net/phy/marvell.c for 1512 with SUPPORTED_FIBRE

and

Quoted 4 lines
Quoted 3 lines

1510 and 1518 without.

Andrew

Quoted 7 lines
Quoted 1 line

By any chance you have the ID of 1512?

Nope, sorry.

Try Russell King.

Andrew

Russell,

Do you have the ID of Marvell PHY 88E1512?

Thanks,
WingMan

#9 lore raw
In reply to Andrew Lunn
Quoted 52 lines

-----Original Message-----
From: Kwok, WingMan
Sent: Wednesday, January 11, 2017 5:32 PM
To: 'Andrew Lunn'; '[email protected]'
Cc: Karicheri, Muralidharan; [email protected]
Subject: RE: Marvell Phy (1510) issue since v4.7 kernel

Quoted 17 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Wednesday, January 11, 2017 5:28 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 9 lines · WingMan
Quoted 9 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Monday, January 09, 2017 8:54 PM
To: Kwok, WingMan
Cc: Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 12 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

From Marvell's brief description

http://www.marvell.com/transceivers/alaska-gbe/,

Quoted 1 line

it seems that 88E1510/1518 don't support fiber. Only 88E1512

does.

Quoted 8 lines
Quoted 8 lines

In

Quoted 1 line

that case, the fiber support patch is not applicable to

88E1510/1518.

O.K. That makes it easier.

Please add the relevant IDs to include/linux/marvell.h, and add entries to driver/net/phy/marvell.c for 1512 with

SUPPORTED_FIBRE

Quoted 11 lines

and

Quoted 5 lines
Quoted 3 lines

1510 and 1518 without.

Andrew

By any chance you have the ID of 1512?

Nope, sorry.

Try Russell King.

Quoted 9 lines
Quoted 1 line

Andrew

Russell,

Do you have the ID of Marvell PHY 88E1512?

Thanks,
WingMan

looping in Charles-Antoine (author of patch
with commit id 6cfb3bcc)

Charles-Antoine,

Do you have the ID of Marvell PHY 88E1512?

Thanks,
WingMan

maintainer
#10 lore raw
Quoted 6 lines

looping in Charles-Antoine (author of patch
with commit id 6cfb3bcc)

Charles-Antoine,

Do you have the ID of Marvell PHY 88E1512?

I suspect that is the wrong question to ask.

The Marvell driver is being loaded, so it must be using on of the IDs in the driver. There is no ID in the driver specifically for the 88E1512. It seems like the 88E1512 uses the 88E1510 ID.

So i think the correct question should be, how can we tell the 88E1512 from the 88E1510 if they have the same ID in register 3.

It appears that for the 88E1512, page 0 are the copper registers and page 1 is the fibre registers. Maybe the 88E1512 has an ID in page 1 register 3? Maybe the 88E1510 does not have an ID in page 1 register 3?

Andrew

#11 lore raw
Quoted 26 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Wednesday, January 11, 2017 6:00 PM
To: Kwok, WingMan
Cc: [email protected]; [email protected]; Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 6 lines

looping in Charles-Antoine (author of patch
with commit id 6cfb3bcc)

Charles-Antoine,

Do you have the ID of Marvell PHY 88E1512?

I suspect that is the wrong question to ask.

The Marvell driver is being loaded, so it must be using on of the IDs in the driver. There is no ID in the driver specifically for the 88E1512. It seems like the 88E1512 uses the 88E1510 ID.

So i think the correct question should be, how can we tell the 88E1512 from the 88E1510 if they have the same ID in register 3.

Quoted 6 lines

It appears that for the 88E1512, page 0 are the copper registers and page 1 is the fibre registers. Maybe the 88E1512 has an ID in page 1 register 3? Maybe the 88E1510 does not have an ID in page 1 register 3?

Andrew

Andrew,

Would Charles-Antoine be the better person to submit a patch to fix the original problem then, since he tested the original fiber support patch with 1512? I unfortunately don't have the datasheet for 1512, and it does not seem to be available publicly.

Regards,
WingMan

maintainer
#12 lore raw
Quoted 2 lines
Quoted 2 lines

So i think the correct question should be, how can we tell the 88E1512 from the 88E1510 if they have the same ID in register 3.

Hi WingMan

Do you know you really have a 1510? You can see it written on the chip?

Please can you read page 18, register 30 and let us know what value you get.

Andrew

#13 lore raw
Quoted 10 lines

Hi WingMan

Do you know you really have a 1510? You can see it written on the chip?

Please can you read page 18, register 30 and let us know what value you get.

Andrew

Removed Charles-Antonie from the list since the email address is no longer valid.

Hi Andrew,

I double checked that ours is actually a 88E1514. According to Marvell's brief description, it seems that it does support fiber.

Reading page 18 reg 30 returns 6.

Thanks,
WingMan

maintainer
#14 lore raw
Quoted 7 lines

Hi Andrew,

I double checked that ours is actually a 88E1514. According to Marvell's brief description, it seems that it does support fiber.

Reading page 18 reg 30 returns 6.

O.K, that is consistent. Looking at the Marvell SDK:

phy/Include/madApiDefs.h:#define    MAD_SUB_88E1510  4   /* 88E1510 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1518  5   /* 88E1518 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1512  6   /* 88E1512/88E1514 */

I took another look at the patch adding Fibre support. The suspend/resume functions are wrong.

        /* Suspend the fiber mode first */
        if (!(phydev->supported & SUPPORTED_FIBRE)) {

The ! should not be there. Does removing them both fix your problem?

Andrew

#15 lore raw
Quoted 14 lines
Quoted 6 lines

I double checked that ours is actually a 88E1514. According to Marvell's brief description, it seems that it does support fiber.

Reading page 18 reg 30 returns 6.

O.K, that is consistent. Looking at the Marvell SDK:

phy/Include/madApiDefs.h:#define    MAD_SUB_88E1510  4   /* 88E1510 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1518  5   /* 88E1518 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1512  6   /*

88E1512/88E1514 */

Quoted 8 lines

I took another look at the patch adding Fibre support. The suspend/resume functions are wrong.

        /* Suspend the fiber mode first */
        if (!(phydev->supported & SUPPORTED_FIBRE)) {

The ! should not be there. Does removing them both fix your problem?

Hi Andrew,

Thanks for taking the time to look into those patches. Yes we notice the error in the suspend/resume functions also.

But our problem is caused by the read_status function:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		/* 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 88E1512.
		* */
		if (phydev->link)
			return 0;

which keeps the fiber page if phydev->link is true (for some reason this is the case even though we are not using fiber)

However, this causes a problem in kernel reboot because neither the suspend/resume is called to restore the copper page and u-boot marvell phy driver does not support 1510 fiber, which will then result in writing to the wrong phy regs and causes a sgmii auto-nego time out.

In addition to fixing the ! in suspend/resume, my suggestion would be to change also the read_status function to always restore the copper page after doing the fiber stuffs:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
		if (err < 0)
			goto error;

		/* 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 88E1512.
		* */
		if (phydev->link)
			return 0;

This makes sure that it is always the copper page brought up and whenever it needs to do some fiber stuffs, the fiber page needs to be brought out explicitly, which is already the case currently.

Another issue is that, as of now, FIBER is enabled regardless of the specific 88e151x. But I believe there is 88e151x chip(s) that does not support fiber. Should fiber be enabled only for those that do support fiber?

WingMan

reviewer
#16 lore raw
Quoted 48 lines · WingMan
Quoted 14 lines
Quoted 6 lines

I double checked that ours is actually a 88E1514. According to Marvell's brief description, it seems that it does support fiber.

Reading page 18 reg 30 returns 6.

O.K, that is consistent. Looking at the Marvell SDK:

phy/Include/madApiDefs.h:#define    MAD_SUB_88E1510  4   /* 88E1510 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1518  5   /* 88E1518 */
phy/Include/madApiDefs.h:#define    MAD_SUB_88E1512  6   /*

88E1512/88E1514 */

Quoted 8 lines

I took another look at the patch adding Fibre support. The suspend/resume functions are wrong.

        /* Suspend the fiber mode first */
        if (!(phydev->supported & SUPPORTED_FIBRE)) {

The ! should not be there. Does removing them both fix your problem?

Hi Andrew,

Thanks for taking the time to look into those patches. Yes we notice the error in the suspend/resume functions also.

But our problem is caused by the read_status function:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		/* 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 88E1512.
		* */
		if (phydev->link)
			return 0;
Quoted 2 lines

which keeps the fiber page if phydev->link is true (for some reason this is the case even though we are not using fiber)

See below.

Quoted 5 lines

However, this causes a problem in kernel reboot because neither the suspend/resume is called to restore the copper page and u-boot marvell phy driver does not support 1510 fiber, which will then result in writing to the wrong phy regs and causes a sgmii auto-nego time out.

So what we need to do is to have a .shutdown function installed - but it's not that simple...

	.mdiodrv = {
		.driver = {
			.shutdown = mv88e1512_shutdown,
		},
	},

in the appropriate phy_driver array element, and then have:

static void mv88e1512_shutdown(struct device *dev)

{
	struct phy_device *phydev = to_phy_device(dev);

	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
}

which will restore the copper page on non-emergency reboots. For emergency reboots, we're out of luck... the real fix would be for uboot to ensure that when it detects this PHY, it ensures that the right page is selected.

Now, that all said, there's a bug in this code, which is fixed by a patch I recently submitted. If you have an 88E151x connected via SGMII, then the read_status function incorrectly believes it should be reading from the fiber page - when in fact the fiber page is reporting the status of the host-side link. So, before trying the above modification, please try this patch - it's already been merged into the net tree, so should hit -rc soon.

My guess from what you've said above is that your 88E151x is connected via SGMII, so you need the patch below rather than trying to install a shutdown function.

8<====
From: Russell King <[email protected]>
Subject: [PATCH] net: phy: marvell: fix Marvell 88E1512 used in SGMII mode

When an Marvell 88E1512 PHY is connected to a nic in SGMII mode, the fiber page is used for the SGMII host-side connection. The PHY driver notices that SUPPORTED_FIBRE is set, so it tries reading the fiber page for the link status, and ends up reading the MAC-side status instead of the outgoing (copper) link. This leads to incorrect results reported via ethtool.

If the PHY is connected via SGMII to the host, ignore the fiber page. However, continue to allow the existing power management code to suspend and resume the fiber page.

Fixes 6cfb3bcc0641 ("Marvell phy: check link status in case of fiber link.")
Signed-off-by Russell King <[email protected]>
---
 drivers/net/phy/marvell.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/phy/marvell.c +2 -1
index 6ad76829c7cd..04e439ad5cff 100644
@@ -1190,7 +1190,8 @@ static int marvell_read_status(struct phy_device *phydev)
 	int err;
 
 	/* Check the fiber mode first */
-	if (phydev->supported & SUPPORTED_FIBRE) {
+	if (phydev->supported & SUPPORTED_FIBRE &&
+	    phydev->interface != PHY_INTERFACE_MODE_SGMII) {
 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
 		if (err < 0)
 			goto error;
Signature · 7 lines
2.7.4


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#17 lore raw
Quoted 106 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:[email protected]]
Sent: Thursday, January 12, 2017 4:42 PM
To: Kwok, WingMan
Cc: Andrew Lunn; Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 10 lines · WingMan
Quoted 10 lines
Quoted 6 lines

I double checked that ours is actually a 88E1514. According to Marvell's brief description, it seems that it does support fiber.

Reading page 18 reg 30 returns 6.

O.K, that is consistent. Looking at the Marvell SDK:

phy/Include/madApiDefs.h:#define MAD_SUB_88E1510 4 /*

88E1510 */

Quoted 1 line
Quoted 1 line

phy/Include/madApiDefs.h:#define MAD_SUB_88E1518 5 /*

88E1518 */

Quoted 10 lines
Quoted 10 lines

phy/Include/madApiDefs.h:#define MAD_SUB_88E1512 6 /*
88E1512/88E1514 */

I took another look at the patch adding Fibre support. The suspend/resume functions are wrong.

        /* Suspend the fiber mode first */
        if (!(phydev->supported & SUPPORTED_FIBRE)) {

The ! should not be there. Does removing them both fix your

problem?

Quoted 11 lines
Quoted 1 line

Hi Andrew,

Thanks for taking the time to look into those patches. Yes we notice the error in the suspend/resume functions also.

But our problem is caused by the read_status function:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE,

MII_M1111_FIBER);

Quoted 8 lines
		if (err < 0)
			goto error;

		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;

		/* If the fiber link is up, it is the selected and used

link.

Quoted 2 lines

* In this case, we need to stay in the fiber page.
* Please to be careful about that, avoid to restore Copper

page

Quoted 8 lines

* in other functions which could break the behaviour
* for some fiber phy like 88E1512.
* */
if (phydev->link)
return 0;

which keeps the fiber page if phydev->link is true (for some reason this is the case even though we are not using fiber)

See below.

Quoted 5 lines

However, this causes a problem in kernel reboot because neither the suspend/resume is called to restore the copper page and u-boot marvell phy driver does not support 1510 fiber, which will then result in writing to the wrong phy regs and causes a sgmii auto-nego time out.

So what we need to do is to have a .shutdown function installed - but it's not that simple...

	.mdiodrv = {
		.driver = {
			.shutdown = mv88e1512_shutdown,
		},
	},

in the appropriate phy_driver array element, and then have:

static void mv88e1512_shutdown(struct device *dev)

{
	struct phy_device *phydev = to_phy_device(dev);

	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
}

which will restore the copper page on non-emergency reboots. For emergency reboots, we're out of luck... the real fix would be for uboot to ensure that when it detects this PHY, it ensures that the right page is selected.

Now, that all said, there's a bug in this code, which is fixed by a patch I recently submitted. If you have an 88E151x connected via SGMII, then the read_status function incorrectly believes it should be reading from the fiber page - when in fact the fiber page is reporting the status of the host-side link. So, before trying the above modification, please try this patch - it's already been merged into the net tree, so should hit -rc soon.

Quoted 9 lines

My guess from what you've said above is that your 88E151x is connected via SGMII, so you need the patch below rather than trying to install a shutdown function.

8<====
From: Russell King <[email protected]>
Subject: [PATCH] net: phy: marvell: fix Marvell 88E1512 used in SGMII mode

Hi Russell,

Thanks for the explanations. Yes, our 88e1514 is connected to the host via sgmii and your patch does provide a solution to our problem.

Regards,
WingMan

maintainer
#18 lore raw
In reply to Kwok, WingMan
Quoted 20 lines

But our problem is caused by the read_status function:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;
		
		/* 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 88E1512.
		* */
		if (phydev->link)
			return 0;
Quoted 2 lines

which keeps the fiber page if phydev->link is true (for some reason this is the case even though we are not using fiber)

How are you using the PHY. What phy-mode do you have set? Do you happen to be using it as an RGMII to SERDES/SGMII bridge? This is what Russell King is doing, i think.

Have you tried the patch Russell submitted recently.

Author: Russell King <[email protected]>
Date:   Tue Jan 10 23:13:45 2017 +0000

net: phy: marvell: fix Marvell 88E1512 used in SGMII mode

When an Marvell 88E1512 PHY is connected to a nic in SGMII mode, the fiber page is used for the SGMII host-side connection. The PHY driver notices that SUPPORTED_FIBRE is set, so it tries reading the fiber page for the link status, and ends up reading the MAC-side status instead of the outgoing (copper) link. This leads to incorrect results reported via ethtool.

If the PHY is connected via SGMII to the host, ignore the fiber page. However, continue to allow the existing power management code to suspend and resume the fiber page.

Quoted 5 lines

However, this causes a problem in kernel reboot because neither the suspend/resume is called to restore the copper page and u-boot marvell phy driver does not support 1510 fiber, which will then result in writing to the wrong phy regs and causes a sgmii auto-nego time out.

This is still a u-boot bug. It should not assume the PHY is in a sane state. It should reset it and configure it as needed. So far, i don't think you have reported any issues with Linux usage of the PHY. There clearly are bugs, but your real problem is u-boot.

Quoted 3 lines

In addition to fixing the ! in suspend/resume, my suggestion would be to change also the read_status function to always restore the copper page after doing the fiber stuffs:

Nope. This is done deliberately, as the comment suggests:

Quoted 8 lines

/* 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 88E1512.
* */
if (phydev->link)
return 0;

The point is, the phylib will continue polling the PHY registers, reading them. If the FIBRE is up, we want to read the FIBRE values, not the copper.

Quoted 4 lines

Another issue is that, as of now, FIBER is enabled regardless of the specific 88e151x. But I believe there is 88e151x chip(s) that does not support fiber. Should fiber be enabled only for those that do support fiber?

Yes, we should look at register 30, page 18 any set SUPPORTED_FIBRE based on that.

Andrew

reviewer
#19 lore raw
Quoted 3 lines · Andrew Lunn

How are you using the PHY. What phy-mode do you have set? Do you happen to be using it as an RGMII to SERDES/SGMII bridge? This is what Russell King is doing, i think.

No - the 88E1512 here is connected to the ethernet host via SGMII.

	Armada 8040 ----SGMII----> 88E1512 ----> RJ45

The problem that my commit fixes is that when the 88E1512 is used in this mode, the copper page reflects the status of the RJ45-side link, and the fiber page reflects the Armada 8040 side of the link.

Without my patch, the 88E1512 read_status function spots that the fiber page reports its link is up (because the Armada 8040 side is indeed up) and decides to report that as the link settings, rather than the (correct) copper side.

Quoted 3 lines

The point is, the phylib will continue polling the PHY registers, reading them. If the FIBRE is up, we want to read the FIBRE values, not the copper.

... but only if we aren't connected in SGMII mode to the ethernet host!

The 88E1512 has the RGMII pins, and it has the Serdes pins. The Serdes pins are used for the optical transceiver, or they're used for a SGMII connection to an ethernet host. So they can't do both.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#20 lore raw
In reply to Andrew Lunn

Hi Andrew,

Quoted 31 lines

-----Original Message-----
From: Andrew Lunn [mailto:[email protected]]
Sent: Thursday, January 12, 2017 4:50 PM
To: Kwok, WingMan
Cc: [email protected]; Karicheri, Muralidharan; [email protected]
Subject: Re: Marvell Phy (1510) issue since v4.7 kernel

Quoted 4 lines

But our problem is caused by the read_status function:

	if ((phydev->supported & SUPPORTED_FIBRE)) {
		err = phy_write(phydev, MII_MARVELL_PHY_PAGE,

MII_M1111_FIBER);

Quoted 8 lines
		if (err < 0)
			goto error;

		err = marvell_read_status_page(phydev, MII_M1111_FIBER);
		if (err < 0)
			goto error;

		/* If the fiber link is up, it is the selected and used

link.

Quoted 2 lines

* In this case, we need to stay in the fiber page.
* Please to be careful about that, avoid to restore Copper

page

Quoted 6 lines

* in other functions which could break the behaviour
* for some fiber phy like 88E1512.
* */
if (phydev->link)
return 0;

Quoted 7 lines
Quoted 2 lines

which keeps the fiber page if phydev->link is true (for some reason this is the case even though we are not using fiber)

How are you using the PHY. What phy-mode do you have set? Do you happen to be using it as an RGMII to SERDES/SGMII bridge? This is what Russell King is doing, i think.

our 88e1514 is connected to the host via sgmii.

Quoted 19 lines

Have you tried the patch Russell submitted recently.

Author: Russell King <[email protected]>
Date:   Tue Jan 10 23:13:45 2017 +0000

net: phy: marvell: fix Marvell 88E1512 used in SGMII mode

When an Marvell 88E1512 PHY is connected to a nic in SGMII mode,
the
fiber page is used for the SGMII host-side connection. The PHY
driver
notices that SUPPORTED_FIBRE is set, so it tries reading the fiber
page
for the link status, and ends up reading the MAC-side status
instead of
the outgoing (copper) link. This leads to incorrect results
reported
via ethtool.

Quoted 5 lines

If the PHY is connected via SGMII to the host, ignore the fiber
page.
However, continue to allow the existing power management code to suspend and resume the fiber page.

Thanks for pointer. It does fix the problem.

Quoted 11 lines
Quoted 5 lines

However, this causes a problem in kernel reboot because neither the suspend/resume is called to restore the copper page and u-boot marvell phy driver does not support 1510 fiber, which will then result in writing to the wrong phy regs and causes a sgmii auto-nego time out.

This is still a u-boot bug. It should not assume the PHY is in a sane state. It should reset it and configure it as needed. So far, i don't think you have reported any issues with Linux usage of the PHY. There clearly are bugs, but your real problem is u-boot.

Yes. Agree.

Quoted 17 lines
Quoted 3 lines

In addition to fixing the ! in suspend/resume, my suggestion would be to change also the read_status function to always restore the copper page after doing the fiber stuffs:

Nope. This is done deliberately, as the comment suggests:

Quoted 1 line
		/* If the fiber link is up, it is the selected and used

link.

Quoted 2 lines

* In this case, we need to stay in the fiber page.
* Please to be careful about that, avoid to restore Copper

page

Quoted 5 lines

* in other functions which could break the behaviour
* for some fiber phy like 88E1512.
* */
if (phydev->link)
return 0;

Quoted 4 lines

The point is, the phylib will continue polling the PHY registers, reading them. If the FIBRE is up, we want to read the FIBRE values, not the copper.

Thanks for the explanations.

Quoted 8 lines
Quoted 4 lines

Another issue is that, as of now, FIBER is enabled regardless of the specific 88e151x. But I believe there is 88e151x chip(s) that does not support fiber. Should fiber be enabled only for those that do support fiber?

Yes, we should look at register 30, page 18 any set SUPPORTED_FIBRE based on that.

Thanks for the suggestions.

Quoted 1 line

Andrew

Just want to know if there is already a patch in the net tree fixing the incorrect ! in the suspend/resume functions also?

WingMan