forcedeth: updated phy errata

PATCH In review Ayaz Abdulla 2 participants 6 messages Sep 1, 2009 – Sep 4, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ayaz Abdulla

author

2009-09-01 04:41 UTC #0 lore raw

PATCH 1/1 forcedeth: updated phy errata

This patch updates the special programming (and/or errata) needed in order to setup the phy for various vendor models.

The new models include:
Marvell E1116
Marvell E1111
Marvell E1011
Marvell E3016
Broadcom 9507
Broadcom AC131
Broadcom 50610

Signed-off-by Ayaz Abdulla <[email protected]>

drivers/net/forcedeth.c +117 -8

@@ -506,6 +506,7 @@
 #define PHY_OUI_VITESSE		0x01c1
 #define PHY_OUI_REALTEK		0x0732
 #define PHY_OUI_REALTEK2	0x0020
+#define PHY_OUI_BROADCOM	0x50ef
 #define PHYID1_OUI_MASK	0x03ff
 #define PHYID1_OUI_SHFT	6
 #define PHYID2_OUI_MASK	0xfc00

@@ -517,7 +518,18 @@
 #define PHY_REV_REALTEK_8211C		0x0001
 #define PHY_MODEL_REALTEK_8201		0x0200
 #define PHY_MODEL_MARVELL_E3016		0x0220
-#define PHY_MARVELL_E3016_INITMASK	0x0300
+#define PHY_MODEL_MARVELL_E1116		0x0210
+#define PHY_MODEL_MARVELL_E1111		0x00c0
+#define PHY_MODEL_MARVELL_E1011		0x00b0
+#define PHY_MODEL_BROADCOM_9507		0x00a0
+#define PHY_MODEL_BROADCOM_AC131	0x0070
+#define PHY_MODEL_BROADCOM_50610	0x0160
+#define PHY_MARVELL_INIT_REG1	0x16
+#define PHY_MARVELL_INIT_REG2	0x10
+#define PHY_MARVELL_INIT1	0x0300
+#define PHY_MARVELL_INIT2	0x4000
+#define PHY_MARVELL_INIT_MSK1	0x0300
+#define PHY_MARVELL_INIT_MSK2	0x00ff
 #define PHY_CICADA_INIT1	0x0f000
 #define PHY_CICADA_INIT2	0x0e00
 #define PHY_CICADA_INIT3	0x01000

@@ -559,6 +571,19 @@
 #define PHY_REALTEK_INIT10	0x0005
 #define PHY_REALTEK_INIT11	0x0200
 #define PHY_REALTEK_INIT_MSK1	0x0003
+#define PHY_BROADCOM_INIT_REG1	0x1c
+#define PHY_BROADCOM_INIT_REG2	0x1f
+#define PHY_BROADCOM_INIT_REG3	0x1b
+#define PHY_BROADCOM_INIT_REG4	0x17
+#define PHY_BROADCOM_INIT_REG5	0x15
+#define PHY_BROADCOM_INIT1	0x2820
+#define PHY_BROADCOM_INIT2	0x0080
+#define PHY_BROADCOM_INIT3	0x0020
+#define PHY_BROADCOM_INIT4	0x8c00
+#define PHY_BROADCOM_INIT5	0x0f08
+#define PHY_BROADCOM_INIT6	0x0001
+#define PHY_BROADCOM_INIT7	0x0f00
+#define PHY_BROADCOM_INIT_MSK1	0x7c20
 
 #define PHY_GIGABIT	0x0100

@@ -1198,13 +1223,14 @@
 	u8 __iomem *base = get_hwbase(dev);
 	u32 phyinterface, phy_reserved, mii_status, mii_control, mii_control_1000,reg;
 
-	/* phy errata for E3016 phy */
-	if (np->phy_model == PHY_MODEL_MARVELL_E3016) {
-		reg = mii_rw(dev, np->phyaddr, MII_NCONFIG, MII_READ);
-		reg &= ~PHY_MARVELL_E3016_INITMASK;
-		if (mii_rw(dev, np->phyaddr, MII_NCONFIG, reg)) {
-			printk(KERN_INFO "%s: phy write to errata reg failed.\n", pci_name(np->pci_dev));
-			return PHY_ERROR;
+	if (np->phy_oui == PHY_OUI_MARVELL) {
+		if (np->phy_model == PHY_MODEL_MARVELL_E3016) {
+			reg = mii_rw(dev, np->phyaddr, MII_NCONFIG, MII_READ);
+			reg &= ~PHY_MARVELL_INIT_MSK1;
+			if (mii_rw(dev, np->phyaddr, MII_NCONFIG, reg)) {
+				printk(KERN_INFO "%s: phy write to errata reg failed.\n", pci_name(np->pci_dev));
+				return PHY_ERROR;
+			}
 		}
 	}
 	if (np->phy_oui == PHY_OUI_REALTEK) {

@@ -1340,6 +1366,89 @@
 	}
 
 	/* phy vendor specific configuration */
+	if (np->phy_oui == PHY_OUI_MARVELL) {
+		if (np->driver_data & DEV_NEED_PHY_INIT_FIX) {
+			if (np->phy_model == PHY_MODEL_MARVELL_E1116) {
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG1, MII_READ);
+				phy_reserved &= ~PHY_MARVELL_INIT_MSK2;
+				if (mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG1,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2, MII_READ);
+				phy_reserved |= PHY_MARVELL_INIT1;
+				if (mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+			}
+			if (np->phy_model == PHY_MODEL_MARVELL_E1111) {
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2, MII_READ);
+				phy_reserved |= PHY_MARVELL_INIT1;
+				if (mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+			}
+			if (np->phy_model == PHY_MODEL_MARVELL_E3016) {
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2, MII_READ);
+				phy_reserved |= PHY_MARVELL_INIT2;
+				if (mii_rw(dev, np->phyaddr, PHY_MARVELL_INIT_REG2 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+			}
+		}
+	}
+
+	if (np->phy_oui == PHY_OUI_BROADCOM) {
+		if (np->driver_data & DEV_NEED_PHY_INIT_FIX) {
+			if (np->phy_model == PHY_MODEL_BROADCOM_9507) {
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG1, MII_READ);
+				phy_reserved &= ~PHY_BROADCOM_INIT_MSK1;
+				phy_reserved |= PHY_BROADCOM_INIT1;
+				if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG1 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+			}
+			if (np->phy_model == PHY_MODEL_BROADCOM_AC131) {
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG2, MII_READ);
+				phy_reserved |= PHY_BROADCOM_INIT2;
+				if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG2 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+
+				phy_reserved = mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG3, MII_READ);
+				phy_reserved |= PHY_BROADCOM_INIT3;
+				if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG3 ,phy_reserved)) {
+					printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+					return PHY_ERROR;
+				}
+			}
+		}
+		if (np->phy_model == PHY_MODEL_BROADCOM_50610) {
+			if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG1, PHY_BROADCOM_INIT4)) {
+ 				printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+ 				return PHY_ERROR;
+ 			}
+			if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG4, PHY_BROADCOM_INIT5)) {
+ 				printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+ 				return PHY_ERROR;
+   			}
+			if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG5, PHY_BROADCOM_INIT6)) {
+ 				printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+ 				return PHY_ERROR;
+ 			}
+			if (mii_rw(dev, np->phyaddr, PHY_BROADCOM_INIT_REG4, PHY_BROADCOM_INIT7)) {
+ 				printk(KERN_INFO "%s: phy init failed.\n", pci_name(np->pci_dev));
+ 				return PHY_ERROR;
+ 			}
+		}
+	}
+
 	if ((np->phy_oui == PHY_OUI_CICADA) && (phyinterface & PHY_RGMII) ) {
 		phy_reserved = mii_rw(dev, np->phyaddr, MII_RESV1, MII_READ);
 		phy_reserved &= ~(PHY_CICADA_INIT1 | PHY_CICADA_INIT2);

David Miller

maintainer

2009-09-03 06:21 UTC #1 lore raw

From: Ayaz Abdulla <[email protected]>
Date: Mon, 31 Aug 2009 19:08:37 -0400

Quoted 12 lines

This patch updates the special programming (and/or errata) needed in order to setup the phy for various vendor models.
The new models include:
Marvell E1116
Marvell E1111
Marvell E1011
Marvell E3016
Broadcom 9507
Broadcom AC131
Broadcom 50610

Quoted 1 line

Signed-off-by Ayaz Abdulla <[email protected]>

Please document what these individual bits mean which you are clearing, by using an individual define for each register bit to describe it's purpose, and then define the mask as a concatenation of these bits.

Having an opaque bitmask is not how to do this.

Thanks.

Ayaz Abdulla

author

2009-09-04 19:25 UTC #2 lore raw

Quoted 19 lines · David Miller

From: Ayaz Abdulla <[email protected]>
Date: Mon, 31 Aug 2009 19:08:37 -0400
Quoted 12 lines
This patch updates the special programming (and/or errata) needed in order to setup the phy for various vendor models.
The new models include:
Marvell E1116
Marvell E1111
Marvell E1011
Marvell E3016
Broadcom 9507
Broadcom AC131
Broadcom 50610
Quoted 1 line
Signed-off-by Ayaz Abdulla <[email protected]>

Quoted 6 lines

Please document what these individual bits mean which you are clearing, by using an individual define for each register bit to describe it's purpose, and then define the mask as a concatenation of these bits.
Having an opaque bitmask is not how to do this.

Unfortunately, the phy vendors don't want us exposing the meaning of their non-standard bits.

Quoted 2 lines

Thanks.

David Miller

maintainer

2009-09-04 22:00 UTC #3 lore raw

From: Ayaz Abdulla <[email protected]>
Date: Fri, 04 Sep 2009 09:57:59 -0400

Quoted 2 lines

Unfortunately, the phy vendors don't want us exposing the meaning of their non-standard bits.

So when you go away and some other developer tries to debug a problem in this area, then what happens? Do they just guess what those bits mean?

Sorry, this sort of situation is unacceptable in this modern day and age. You'll need to work this out before I'm willing to take the patch.

Ayaz Abdulla

author

2009-09-04 22:48 UTC #4 lore raw

Quoted 11 lines · David Miller

From: Ayaz Abdulla <[email protected]>
Date: Fri, 04 Sep 2009 09:57:59 -0400
Quoted 2 lines
Unfortunately, the phy vendors don't want us exposing the meaning of their non-standard bits.
So when you go away and some other developer tries to debug a problem in this area, then what happens? Do they just guess what those bits mean?

I understand your concern. However, NVIDIA will always be the maintainer for these phy erratas and work with users to solve any issues. We have access to the confidential documents provided by the vendors.

Phy vendors have said "No" in the past. I can request for 'opening up' the bits again, but not all vendors will comply - resulting in no support for forcedeth. This is currently the only way to provide a good experience for the end user.

Quoted 4 lines

Sorry, this sort of situation is unacceptable in this modern day and age. You'll need to work this out before I'm willing to take the patch.

David Miller

maintainer

2009-09-04 22:54 UTC #5 lore raw

From: Ayaz Abdulla <[email protected]>
Date: Fri, 04 Sep 2009 13:22:06 -0400

Quoted 4 lines

Phy vendors have said "No" in the past. I can request for 'opening up' the bits again, but not all vendors will comply - resulting in no support for forcedeth. This is currently the only way to provide a good experience for the end user.

If I make it more difficult for you to get you're patch into the tree than to get permission from the vendor, I'm sure you'll find a way to make it happen.