Marvell PHY driver fix

Marvell PHY driver fix

[Alexandr Smirnov]

Marvell PHY m88e1111 (not sure about other models, but think they too) works in
two modes: fiber and copper. In Marvell PHY driver (that we have in current
community kernels) code supported only copper mode, and this is not configurable,
bits for copper mode are simply written in registers during PHY
initialization. This patch adds support for both modes.

Signed-off-by: Alexandr Smirnov <asmirnov@ru.mvista.com>

diff -pruN powerpc.orig/drivers/net/phy/marvell.c powerpc/drivers/net/phy/marvell.c
--- powerpc.orig/drivers/net/phy/marvell.c	2008-02-07 14:59:32.000000000 +0300
+++ powerpc/drivers/net/phy/marvell.c	2008-02-19 21:47:34.000000000 +0300
@@ -58,9 +58,28 @@
 #define MII_M1111_RX_DELAY		0x80
 #define MII_M1111_TX_DELAY		0x2
 #define MII_M1111_PHY_EXT_SR		0x1b
-#define MII_M1111_HWCFG_MODE_MASK	0xf
-#define MII_M1111_HWCFG_MODE_RGMII	0xb
+
+#define MII_M1111_HWCFG_MODE_MASK		0xf
+#define MII_M1111_HWCFG_MODE_COPPER_RGMII	0xb
+#define MII_M1111_HWCFG_MODE_FIBER_RGMII	0x3
 #define MII_M1111_HWCFG_MODE_SGMII_NO_CLK	0x4
+#define MII_M1111_HWCFG_FIBER_COPPER_AUTO	0x8000
+#define MII_M1111_HWCFG_FIBER_COPPER_RES	0x2000
+
+#define MII_M1111_PHY_CR		0
+#define MII_M1111_SOFTWARE_RESET	0x8000
+
+#define MII_M1111_COPPER		0
+#define MII_M1111_FIBER			1
+
+#define MII_M1011_PHY_STATUS		0x11
+#define MII_M1011_PHY_STATUS_1000	0x8000
+#define MII_M1011_PHY_STATUS_100	0x4000
+#define MII_M1011_PHY_STATUS_SPD_MASK	0xc000
+#define MII_M1011_PHY_STATUS_FULLDUPLEX	0x2000
+#define MII_M1011_PHY_STATUS_RESOLVED	0x0800
+#define MII_M1011_PHY_STATUS_LINK	0x0400
+
 
 MODULE_DESCRIPTION("Marvell PHY driver");
 MODULE_AUTHOR("Andy Fleming");
@@ -141,12 +160,22 @@ static int marvell_config_aneg(struct ph
 static int m88e1111_config_init(struct phy_device *phydev)
 {
 	int err;
+	int temp;
+	int mode;
+
+	/* Enable Fiber/Copper auto selection */
+	temp = phy_read(phydev, MII_M1111_PHY_EXT_SR);
+	temp |= MII_M1111_HWCFG_FIBER_COPPER_AUTO;
+	phy_write(phydev, MII_M1111_PHY_EXT_SR, temp);
+
+	temp = phy_read(phydev, MII_M1111_PHY_CR);
+	temp |= MII_M1111_SOFTWARE_RESET;
+	phy_write(phydev, MII_M1111_PHY_CR, temp);
 
 	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII) ||
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) ||
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
 	    (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
-		int temp;
 
 		temp = phy_read(phydev, MII_M1111_PHY_EXT_CR);
 		if (temp < 0)
@@ -171,7 +200,17 @@ static int m88e1111_config_init(struct p
 			return temp;
 
 		temp &= ~(MII_M1111_HWCFG_MODE_MASK);
-		temp |= MII_M1111_HWCFG_MODE_RGMII;
+
+		mode = phy_read(phydev, MII_M1111_PHY_EXT_CR);
+		mode = (mode & MII_M1111_HWCFG_FIBER_COPPER_RES) >> 13;
+
+		switch (mode) {
+		case MII_M1111_COPPER:
+			temp |= MII_M1111_HWCFG_MODE_COPPER_RGMII;
+			break;
+		case MII_M1111_FIBER:
+			temp |= MII_M1111_HWCFG_MODE_FIBER_RGMII;
+		}
 
 		err = phy_write(phydev, MII_M1111_PHY_EXT_SR, temp);
 		if (err < 0)
@@ -262,6 +301,93 @@ static int m88e1145_config_init(struct p
 	return 0;
 }
 
+/* marvell_read_status
+ *
+ * Generic status code does not detect Fiber correctly!
+ * Description: 
+ *   Check the link, then figure out the current state
+ *   by comparing what we advertise with what the link partner
+ *   advertises.  Start by checking the gigabit possibilities,
+ *   then move on to 10/100.
+ */
+static int marvell_read_status(struct phy_device *phydev)
+{
+	int adv;
+	int err;
+	int lpa;
+	int status = 0;
+
+	/* Update the link, but return if there
+	 * was an error */
+	err = genphy_update_link(phydev);
+	if (err)
+		return err;
+
+	if (AUTONEG_ENABLE == phydev->autoneg) {
+		status = phy_read(phydev, MII_M1011_PHY_STATUS);
+		if (status < 0)
+			return status;
+
+		lpa = phy_read(phydev, MII_LPA);
+		if (lpa < 0)
+			return lpa;
+
+		adv = phy_read(phydev, MII_ADVERTISE);
+		if (adv < 0)
+			return adv;
+
+		lpa &= adv;
+
+		if (status & MII_M1011_PHY_STATUS_FULLDUPLEX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		status = status & MII_M1011_PHY_STATUS_SPD_MASK;
+		phydev->pause = phydev->asym_pause = 0;
+
+		switch (status) {
+		case MII_M1011_PHY_STATUS_1000:
+			phydev->speed = SPEED_1000;
+			break;
+
+		case MII_M1011_PHY_STATUS_100:
+			phydev->speed = SPEED_100;
+			break;
+
+		default:
+			phydev->speed = SPEED_10;
+			break;
+		}
+
+		if (phydev->duplex == DUPLEX_FULL) {
+			phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0;
+			phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0;
+		}
+	} else {
+		int bmcr = phy_read(phydev, MII_BMCR);
+
+		if (bmcr < 0)
+			return bmcr;
+
+		if (bmcr & BMCR_FULLDPLX)
+			phydev->duplex = DUPLEX_FULL;
+		else
+			phydev->duplex = DUPLEX_HALF;
+
+		if (bmcr & BMCR_SPEED1000)
+			phydev->speed = SPEED_1000;
+		else if (bmcr & BMCR_SPEED100)
+			phydev->speed = SPEED_100;
+		else
+			phydev->speed = SPEED_10;
+
+		phydev->pause = phydev->asym_pause = 0;
+	}
+
+	return 0;
+}
+
 static struct phy_driver marvell_drivers[] = {
 	{
 		.phy_id = 0x01410c60,
@@ -296,7 +422,7 @@ static struct phy_driver marvell_drivers
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1111_config_init,
 		.config_aneg = &marvell_config_aneg,
-		.read_status = &genphy_read_status,
+		.read_status = &marvell_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },

Re: Marvell PHY driver fix

[Andy Fleming]

On Feb 22, 2008, at 08:34, Alexandr Smirnov wrote:

> Marvell PHY m88e1111 (not sure about other models, but think they  
> too) works in
> two modes: fiber and copper. In Marvell PHY driver (that we have in  
> current
> community kernels) code supported only copper mode, and this is not  
> configurable,
> bits for copper mode are simply written in registers during PHY
> initialization. This patch adds support for both modes.

Excellent.  I've known about this problem for a while, but haven't  
been able to look into it.

However, some comments below

>
> Signed-off-by: Alexandr Smirnov <asmirnov@ru.mvista.com>
>
> diff -pruN powerpc.orig/drivers/net/phy/marvell.c powerpc/drivers/ 
> net/phy/marvell.c
> --- powerpc.orig/drivers/net/phy/marvell.c	2008-02-07  
> 14:59:32.000000000 +0300
> +++ powerpc/drivers/net/phy/marvell.c	2008-02-19 21:47:34.000000000  
> +0300
> @@ -58,9 +58,28 @@

>
> +#define MII_M1111_PHY_CR		0
> +#define MII_M1111_SOFTWARE_RESET	0x8000

You don't need to do this.  There are already constants for both of  
those values.  That's a standard register.

>
>
>  MODULE_DESCRIPTION("Marvell PHY driver");
>  MODULE_AUTHOR("Andy Fleming");
> @@ -141,12 +160,22 @@ static int marvell_config_aneg(struct ph
>  static int m88e1111_config_init(struct phy_device *phydev)
>  {
>  	int err;
> +	int temp;
> +	int mode;
> +
> +	/* Enable Fiber/Copper auto selection */
> +	temp = phy_read(phydev, MII_M1111_PHY_EXT_SR);
> +	temp |= MII_M1111_HWCFG_FIBER_COPPER_AUTO;
> +	phy_write(phydev, MII_M1111_PHY_EXT_SR, temp);
> +
> +	temp = phy_read(phydev, MII_M1111_PHY_CR);
> +	temp |= MII_M1111_SOFTWARE_RESET;
> +	phy_write(phydev, MII_M1111_PHY_CR, temp);


Use the standard constants from mii.h (MII_BMCR, and BMCR_RESET)


>
>
>
>  		temp &= ~(MII_M1111_HWCFG_MODE_MASK);
> -		temp |= MII_M1111_HWCFG_MODE_RGMII;
> +
> +		mode = phy_read(phydev, MII_M1111_PHY_EXT_CR);
> +		mode = (mode & MII_M1111_HWCFG_FIBER_COPPER_RES) >> 13;
> +
> +		switch (mode) {
> +		case MII_M1111_COPPER:
> +			temp |= MII_M1111_HWCFG_MODE_COPPER_RGMII;
> +			break;
> +		case MII_M1111_FIBER:
> +			temp |= MII_M1111_HWCFG_MODE_FIBER_RGMII;
> +		}


I think using a switch statement on a single bit is probably  
overkill.  Much clearer, I think, if you do:

mode = phy_read(phydev, MII_M1111_PHY_EXT_CR);

if (mode & MII_M1111_HWCFG_FIBER_COPPER_RES)
	temp |= MII_M1111_HWCFG_MODE_FIBER_RGMII;
else
	temp |= MII_M1111_HWCFG_MODE_COPPER_RGMII;


>
> +/* marvell_read_status
> + *
> + * Generic status code does not detect Fiber correctly!


After looking at the manual for the 88e1111, I think there may be a  
way to continue using the generic code, rather than replicating the  
generic code with slightly different values.  It looks like the  
problem isn't that Fiber disobeys the MIIM standard, but that it uses  
a paging mechanism to determine whether it's the Fiber Status/Control  
or the Copper Status/Control.  Many of the registers exist in both  
pages, but those two don't.  However, based on this, it seems  
reasonable to use the detected mode (fiber/copper) to determine  
whether we should be operating out of page 0 or 1.

Could you try making sure that register 22[7:0] = 0x1 for fiber, and  
see if you can then just use the standard genphy_read_status()?

Andy