igb: add i210/i211 external phy support

The i210/i211 uses the MDICNFG register for the phy address instead of the MDIC register.

Signed-off-by Tim Harvey <[email protected]>

---
  drivers/net/ethernet/intel/igb/e1000_phy.c | 71 ++++++++++++++++++++++++++----
  1 file changed, 62 insertions(+), 9 deletions(-)

drivers/net/ethernet/intel/igb/e1000_phy.c +13 -4

index c1bb64d..2307ac6 100644
@@ -135,7 +135,7 @@ out:
  s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
  {
  	struct e1000_phy_info *phy = &hw->phy;
-	u32 i, mdic = 0;
+	u32 i, mdicnfg, mdic = 0;
  	s32 ret_val = 0;
 
  	if (offset > MAX_PHY_REG_ADDRESS) {

@@ -148,11 +148,25 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
  	 * Control register.  The MAC will take care of interfacing with the
  	 * PHY to retrieve the desired data.
  	 */
-	mdic = ((offset << E1000_MDIC_REG_SHIFT) |
-		(phy->addr << E1000_MDIC_PHY_SHIFT) |
-		(E1000_MDIC_OP_READ));
+	switch (hw->mac.type) {
+	case e1000_i210:
+	case e1000_i211:
+		mdicnfg = rd32(E1000_MDICNFG);
+		mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+		mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdicnfg);
+		mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+			(E1000_MDIC_OP_READ));
+		break;
+	default:
+		mdic = ((offset << E1000_MDIC_REG_SHIFT) |

drivers/net/ethernet/intel/igb/e1000_phy.c +14 -5

@@ -177,6 +191,18 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
  	*data = (u16) mdic;
 
  out:
+	switch (hw->mac.type) {
+	/* restore MDICNFG to have phy's addr */
+	case e1000_i210:
+	case e1000_i211:
+		mdicnfg = rd32(E1000_MDICNFG);
+		mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+		mdicnfg |= (hw->phy.addr << E1000_MDICNFG_PHY_SHIFT);
+		wr32(E1000_MDICNFG, mdicnfg);
+		break;
+	default:
+		break;
+	}
  	return ret_val;
  }
 

@@ -191,7 +217,7 @@ out:
  s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
  {
  	struct e1000_phy_info *phy = &hw->phy;
-	u32 i, mdic = 0;
+	u32 i, mdicnfg, mdic = 0;
  	s32 ret_val = 0;
 
  	if (offset > MAX_PHY_REG_ADDRESS) {

@@ -204,12 +230,27 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
  	 * Control register.  The MAC will take care of interfacing with the
  	 * PHY to retrieve the desired data.
  	 */
-	mdic = (((u32)data) |
-		(offset << E1000_MDIC_REG_SHIFT) |
-		(phy->addr << E1000_MDIC_PHY_SHIFT) |
-		(E1000_MDIC_OP_WRITE));
+	switch (hw->mac.type) {

drivers/net/ethernet/intel/igb/e1000_phy.c +5 -0

@@ -233,6 +274,18 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
  	}
 
  out:
+	switch (hw->mac.type) {
+	/* restore MDICNFG to have phy's addr */
+	case e1000_i210:
+	case e1000_i211:
+		mdicnfg = rd32(E1000_MDICNFG);

Quoted 4 lines · Tim Harvey

The i210/i211 uses the MDICNFG register for the phy address instead of the MDIC register.

Quoted 1 line

Signed-off-by Tim Harvey <[email protected]>

This patch probably is not needed. The existing functions should work as long as you have a separate means for updating the PHY addr which should only need to be updated while searching for the PHY, and since the PHY isn't pluggable it should probably be stored in the EEPROM.

Quoted 49 lines

---
  drivers/net/ethernet/intel/igb/e1000_phy.c | 71
++++++++++++++++++++++++++----
  1 file changed, 62 insertions(+), 9 deletions(-)

drivers/net/ethernet/intel/igb/e1000_phy.c +1 -1

index c1bb64d..2307ac6 100644
@@ -135,7 +135,7 @@ out:
  s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
  {
        struct e1000_phy_info *phy = &hw->phy;
-       u32 i, mdic = 0;
+       u32 i, mdicnfg, mdic = 0;
        s32 ret_val = 0;
 
        if (offset > MAX_PHY_REG_ADDRESS) {

@@ -148,11 +148,25 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 *data)
         * Control register.  The MAC will take care of interfacing with

the
* PHY to retrieve the desired data.
*/

drivers/net/ethernet/intel/igb/e1000_phy.c +17 -3

-       mdic = ((offset << E1000_MDIC_REG_SHIFT) |
-               (phy->addr << E1000_MDIC_PHY_SHIFT) |
-               (E1000_MDIC_OP_READ));
+       switch (hw->mac.type) {
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+                       (E1000_MDIC_OP_READ));
+               break;
+       default:
+               mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
+                       (E1000_MDIC_OP_READ));
+               break;
+       }
 
        wr32(E1000_MDIC, mdic);
+       wrfl();

Quoted 2 lines

/* Poll the ready bit to see if the MDI read completed
* Increasing the time out as testing showed failures with

I'm not really a fan of this section of code. Why is it that you need to be able to change the phy address? It should be something that is set once for the device once you have your PHY and stays that way.

Quoted 48 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +13 -1

@@ -177,6 +191,18 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 *data)
        *data = (u16) mdic;
 
  out:
+       switch (hw->mac.type) {
+       /* restore MDICNFG to have phy's addr */
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (hw->phy.addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               break;
+       default:
+               break;
+       }
        return ret_val;
  }

@@ -191,7 +217,7 @@ out:
  s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
  {
        struct e1000_phy_info *phy = &hw->phy;
-       u32 i, mdic = 0;
+       u32 i, mdicnfg, mdic = 0;
        s32 ret_val = 0;
 
        if (offset > MAX_PHY_REG_ADDRESS) {

@@ -204,12 +230,27 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 data)
         * Control register.  The MAC will take care of interfacing with

the
* PHY to retrieve the desired data.
*/

drivers/net/ethernet/intel/igb/e1000_phy.c +7 -4

-       mdic = (((u32)data) |
-               (offset << E1000_MDIC_REG_SHIFT) |
-               (phy->addr << E1000_MDIC_PHY_SHIFT) |
-               (E1000_MDIC_OP_WRITE));
+       switch (hw->mac.type) {
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data) · line 243

Quoted 4 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +4 -0

+               mdic = (((u32)data) |
+                       (offset << E1000_MDIC_REG_SHIFT) |
+                       (E1000_MDIC_OP_WRITE));
+               break;

You could just fall though. The area covered by the PHY_SHIFT should be read/only and will not be impacted if any value is placed there.

Quoted 4 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +3 -0

+       default:
+               mdic = (((u32)data) |
+                       (offset << E1000_MDIC_REG_SHIFT) |

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data) · line 256

Quoted 10 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +5 -0

+                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
+                       (E1000_MDIC_OP_WRITE));
+               break;
+       }
 
        wr32(E1000_MDIC, mdic);
+       wrfl();
 
        /* Poll the ready bit to see if the MDI read completed
         * Increasing the time out as testing showed failures with

Same thing for this one. You shouldn't need to do anything fancy to write it.

Quoted 15 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +10 -0

@@ -233,6 +274,18 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 data)
        }
 
  out:
+       switch (hw->mac.type) {
+       /* restore MDICNFG to have phy's addr */
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (hw->phy.addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               break;
+       default:

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data) · line 207

Quoted 6 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +2 -0

+               break;
+       }
        return ret_val;
  }

This bit doesn't add any value. You could definitely drop this.

Quoted 11 lines · Alexander Duyck

Quoted 4 lines · Tim Harvey

The i210/i211 uses the MDICNFG register for the phy address instead of the MDIC register.

Signed-off-by Tim Harvey <[email protected]>

This patch probably is not needed. The existing functions should work as long as you have a separate means for updating the PHY addr which should only need to be updated while searching for the PHY, and since the PHY isn't pluggable it should probably be stored in the EEPROM.

Alexander,

Thanks for the review!

The i210 requires that the PHY address be in the MDICNFG register, whereas other earlier chips supported by the igb driver stored the phy in the MDIC register directly. You are probably thinking this is not necessary because typically you would use flash configuration control words to pre-load this value and that is correct. However, if we are to add support for phylib we need to export functions that allow a phy id to be passed in, and so I'm explicitly setting it up (as it was when the phy address was only in MDIC for earlier chips).

Quoted 48 lines

Quoted 48 lines

---
   drivers/net/ethernet/intel/igb/e1000_phy.c | 71
++++++++++++++++++++++++++----
   1 file changed, 62 insertions(+), 9 deletions(-)

drivers/net/ethernet/intel/igb/e1000_phy.c +1 -1

index c1bb64d..2307ac6 100644
@@ -135,7 +135,7 @@ out:
   s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
   {
         struct e1000_phy_info *phy = &hw->phy;
-       u32 i, mdic = 0;
+       u32 i, mdicnfg, mdic = 0;
         s32 ret_val = 0;
 
         if (offset > MAX_PHY_REG_ADDRESS) {

@@ -148,11 +148,25 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 *data)
          * Control register.  The MAC will take care of interfacing with

the
* PHY to retrieve the desired data.
*/

drivers/net/ethernet/intel/igb/e1000_phy.c +17 -3

-       mdic = ((offset << E1000_MDIC_REG_SHIFT) |
-               (phy->addr << E1000_MDIC_PHY_SHIFT) |
-               (E1000_MDIC_OP_READ));
+       switch (hw->mac.type) {
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+                       (E1000_MDIC_OP_READ));
+               break;
+       default:
+               mdic = ((offset << E1000_MDIC_REG_SHIFT) |
+                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
+                       (E1000_MDIC_OP_READ));
+               break;
+       }
 
         wr32(E1000_MDIC, mdic);
+       wrfl();

Quoted 6 lines

Quoted 2 lines

/* Poll the ready bit to see if the MDI read completed
* Increasing the time out as testing showed failures with

I'm not really a fan of this section of code. Why is it that you need to be able to change the phy address? It should be something that is set once for the device once you have your PHY and stays that way.

I'm not changing the phy address - I'm putting it in the correct register for the i210/i211.

Quoted 46 lines

Quoted 46 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +13 -1

@@ -177,6 +191,18 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 *data)
         *data = (u16) mdic;
 
   out:
+       switch (hw->mac.type) {
+       /* restore MDICNFG to have phy's addr */
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (hw->phy.addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               break;
+       default:
+               break;
+       }
         return ret_val;
   }

@@ -191,7 +217,7 @@ out:
   s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
   {
         struct e1000_phy_info *phy = &hw->phy;
-       u32 i, mdic = 0;
+       u32 i, mdicnfg, mdic = 0;
         s32 ret_val = 0;
 
         if (offset > MAX_PHY_REG_ADDRESS) {

@@ -204,12 +230,27 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 data)
          * Control register.  The MAC will take care of interfacing with

the
* PHY to retrieve the desired data.
*/

drivers/net/ethernet/intel/igb/e1000_phy.c +6 -4

-       mdic = (((u32)data) |
-               (offset << E1000_MDIC_REG_SHIFT) |
-               (phy->addr << E1000_MDIC_PHY_SHIFT) |
-               (E1000_MDIC_OP_WRITE));
+       switch (hw->mac.type) {
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data) · line 243

Quoted 5 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +5 -0

+               wr32(E1000_MDICNFG, mdicnfg);
+               mdic = (((u32)data) |
+                       (offset << E1000_MDIC_REG_SHIFT) |
+                       (E1000_MDIC_OP_WRITE));
+               break;

You could just fall though. The area covered by the PHY_SHIFT should be read/only and will not be impacted if any value is placed there.

The i210 Programming Interface document specifies that MDIC[25:21] are 'Reserved' and should be written with 0.

I would tend to agree that these bits are likely ignored on the i210/i211 but I'm just following the documentation here and not chancing it. If its agree'd that I don't need to do this, I can simplify the code by falling through.

I can also rename the mdic to reg and use it for both registers if its desired to not have an additional variable on the stack.

Quoted 31 lines

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data) · line 256

Quoted 13 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +8 -0

+       default:
+               mdic = (((u32)data) |
+                       (offset << E1000_MDIC_REG_SHIFT) |
+                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
+                       (E1000_MDIC_OP_WRITE));
+               break;
+       }
 
         wr32(E1000_MDIC, mdic);
+       wrfl();
 
         /* Poll the ready bit to see if the MDI read completed
          * Increasing the time out as testing showed failures with

Same thing for this one. You shouldn't need to do anything fancy to write it.

Quoted 14 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +9 -0

@@ -233,6 +274,18 @@ s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32
 offset, u16 data)
         }
 
   out:
+       switch (hw->mac.type) {
+       /* restore MDICNFG to have phy's addr */
+       case e1000_i210:
+       case e1000_i211:
+               mdicnfg = rd32(E1000_MDICNFG);
+               mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
+               mdicnfg |= (hw->phy.addr << E1000_MDICNFG_PHY_SHIFT);
+               wr32(E1000_MDICNFG, mdicnfg);
+               break;

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_phy.c s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data) · line 207

Quoted 7 lines

drivers/net/ethernet/intel/igb/e1000_phy.c +3 -0

+       default:
+               break;
+       }
         return ret_val;
   }

This bit doesn't add any value. You could definitely drop this.

This is likely more relevant to the 2nd patch that allows a phy address to be passed in.

Tim

Add igb_write_reg_gs40g/igb_read_reg_gs40g that can be passed a phy address. The existing igb_write_phy_reg_gs40g/igb_read_phy_reg_gs40g become wrappers to this function.

Signed-off-by Tim Harvey <[email protected]>

---
  drivers/net/ethernet/intel/igb/e1000_82575.c |  4 +-
  drivers/net/ethernet/intel/igb/e1000_phy.c   | 70 ++++++++++++++++++++--------
  drivers/net/ethernet/intel/igb/e1000_phy.h   |  6 ++-
  3 files changed, 56 insertions(+), 24 deletions(-)

drivers/net/ethernet/intel/igb/e1000_82575.c +2 -2

index 0f69ef8..d2afd7b 100644
@@ -2129,7 +2129,7 @@ static s32 igb_read_phy_reg_82580(struct e1000_hw *hw, u32 offset, u16 *data)
  	if (ret_val)
  		goto out;
 
-	ret_val = igb_read_phy_reg_mdic(hw, offset, data);
+	ret_val = igb_read_phy_reg_mdic(hw, hw->phy.addr, offset, data);
 
  	hw->phy.ops.release(hw);
 

@@ -2154,7 +2154,7 @@ static s32 igb_write_phy_reg_82580(struct e1000_hw *hw, u32 offset, u16 data)
  	if (ret_val)
  		goto out;
 
-	ret_val = igb_write_phy_reg_mdic(hw, offset, data);
+	ret_val = igb_write_phy_reg_mdic(hw, hw->phy.addr, offset, data);
 
  	hw->phy.ops.release(hw);
 

drivers/net/ethernet/intel/igb/e1000_phy.c +50 -20

index 2307ac6..66c2a09 100644
@@ -132,9 +132,8 @@ out:
   *  Reads the MDI control regsiter in the PHY at offset and stores the
   *  information read to data.
   **/
-s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
+s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u8 addr, u32 offset, u16 *data)
  {
-	struct e1000_phy_info *phy = &hw->phy;
  	u32 i, mdicnfg, mdic = 0;
  	s32 ret_val = 0;
 

@@ -153,14 +152,14 @@ s32 igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data)
  	case e1000_i211:
  		mdicnfg = rd32(E1000_MDICNFG);
  		mdicnfg &= ~(E1000_MDICNFG_PHY_MASK);
-		mdicnfg |= (phy->addr << E1000_MDICNFG_PHY_SHIFT);
+		mdicnfg |= (addr << E1000_MDICNFG_PHY_SHIFT);
  		wr32(E1000_MDICNFG, mdicnfg);
  		mdic = ((offset << E1000_MDIC_REG_SHIFT) |
  			(E1000_MDIC_OP_READ));
  		break;
  	default:
  		mdic = ((offset << E1000_MDIC_REG_SHIFT) |
-			(phy->addr << E1000_MDIC_PHY_SHIFT) |
+			(addr << E1000_MDIC_PHY_SHIFT) |
  			(E1000_MDIC_OP_READ));
  		break;
  	}

@@ -214,9 +213,8 @@ out:
   *
   *  Writes data to MDI control register in the PHY at offset.
   **/
-s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data)
+s32 igb_write_phy_reg_mdic(struct e1000_hw *hw, u8 addr, u32 offset, u16 data)
  {
-	struct e1000_phy_info *phy = &hw->phy;
  	u32 i, mdicnfg, mdic = 0;
  	s32 ret_val = 0;
 

@@ -464,7 +462,7 @@ s32 igb_read_phy_reg_igp(struct e1000_hw *hw, u32 offset, u16 *data)
  		goto out;
 
  	if (offset > MAX_PHY_MULTI_PAGE_REG) {
-		ret_val = igb_write_phy_reg_mdic(hw,
+		ret_val = igb_write_phy_reg_mdic(hw, hw->phy.addr,
  						 IGP01E1000_PHY_PAGE_SELECT,
  						 (u16)offset);
  		if (ret_val) {

@@ -473,8 +471,8 @@ s32 igb_read_phy_reg_igp(struct e1000_hw *hw, u32 offset, u16 *data)
  		}
  	}
 
-	ret_val = igb_read_phy_reg_mdic(hw, MAX_PHY_REG_ADDRESS & offset,
-					data);
+	ret_val = igb_read_phy_reg_mdic(hw, hw->phy.addr,
+					MAX_PHY_REG_ADDRESS & offset, data);
 
  	hw->phy.ops.release(hw);
 

@@ -503,7 +501,7 @@ s32 igb_write_phy_reg_igp(struct e1000_hw *hw, u32 offset, u16 data)
  		goto out;
 
  	if (offset > MAX_PHY_MULTI_PAGE_REG) {
-		ret_val = igb_write_phy_reg_mdic(hw,
+		ret_val = igb_write_phy_reg_mdic(hw, hw->phy.addr,
  						 IGP01E1000_PHY_PAGE_SELECT,
  						 (u16)offset);
  		if (ret_val) {

@@ -512,8 +510,8 @@ s32 igb_write_phy_reg_igp(struct e1000_hw *hw, u32 offset, u16 data)
  		}
  	}
 
-	ret_val = igb_write_phy_reg_mdic(hw, MAX_PHY_REG_ADDRESS & offset,
-					 data);
+	ret_val = igb_write_phy_reg_mdic(hw, hw->phy.addr,
+					 MAX_PHY_REG_ADDRESS & offset, data);
 
  	hw->phy.ops.release(hw);
 

@@ -2464,8 +2462,9 @@ out:
  }
 
  /**
- *  igb_write_phy_reg_gs40g - Write GS40G PHY register
+ *  igb_write_reg_gs40g - Write GS40G PHY register
   *  @hw: pointer to the HW structure
+ *  @addr: phy address to write to
   *  @offset: lower half is register offset to write to
   *     upper half is page to use.
   *  @data: data to write at register offset

@@ -2473,7 +2472,7 @@ out:
   *  Acquires semaphore, if necessary, then writes the data to PHY register
   *  at the offset.  Release any acquired semaphores before exiting.
   **/
-s32 igb_write_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 data)
+s32 igb_write_reg_gs40g(struct e1000_hw *hw, u8 addr, u32 offset, u16 data)
  {
  	s32 ret_val;
  	u16 page = offset >> GS40G_PAGE_SHIFT;

@@ -2483,10 +2482,10 @@ s32 igb_write_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 data)
  	if (ret_val)
  		return ret_val;
 
-	ret_val = igb_write_phy_reg_mdic(hw, GS40G_PAGE_SELECT, page);
+	ret_val = igb_write_phy_reg_mdic(hw, addr, GS40G_PAGE_SELECT, page);
  	if (ret_val)
  		goto release;
-	ret_val = igb_write_phy_reg_mdic(hw, offset, data);
+	ret_val = igb_write_phy_reg_mdic(hw, addr, offset, data);
 
  release:
  	hw->phy.ops.release(hw);

@@ -2494,8 +2493,24 @@ release:
  }
 
  /**
- *  igb_read_phy_reg_gs40g - Read GS40G  PHY register
+ *  igb_write_phy_reg_gs40g - Write GS40G PHY register
+ *  @hw: pointer to the HW structure
+ *  @offset: lower half is register offset to write to
+ *     upper half is page to use.
+ *  @data: data to write at register offset
+ *
+ *  Acquires semaphore, if necessary, then writes the data to PHY register
+ *  at the offset.  Release any acquired semaphores before exiting.
+ **/
+s32 igb_write_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 data)
+{
+	return igb_write_reg_gs40g(hw, hw->phy.addr, offset, data);
+}
+
+/**
+ *  igb_read_reg_gs40g - Read GS40G  PHY register
   *  @hw: pointer to the HW structure
+ *  @addr: phy address to read from
   *  @offset: lower half is register offset to read to
   *     upper half is page to use.
   *  @data: data to read at register offset

@@ -2503,7 +2518,7 @@ release:
   *  Acquires semaphore, if necessary, then reads the data in the PHY register
   *  at the offset.  Release any acquired semaphores before exiting.
   **/
-s32 igb_read_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 *data)
+s32 igb_read_reg_gs40g(struct e1000_hw *hw, u8 addr, u32 offset, u16 *data)
  {
  	s32 ret_val;
  	u16 page = offset >> GS40G_PAGE_SHIFT;

@@ -2513,10 +2528,10 @@ s32 igb_read_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 *data)
  	if (ret_val)
  		return ret_val;
 
-	ret_val = igb_write_phy_reg_mdic(hw, GS40G_PAGE_SELECT, page);
+	ret_val = igb_write_phy_reg_mdic(hw, addr, GS40G_PAGE_SELECT, page);
  	if (ret_val)
  		goto release;
-	ret_val = igb_read_phy_reg_mdic(hw, offset, data);
+	ret_val = igb_read_phy_reg_mdic(hw, addr, offset, data);
 
  release:
  	hw->phy.ops.release(hw);

@@ -2524,6 +2539,21 @@ release:
  }
 
  /**
+ *  igb_read_phy_reg_gs40g - Read GS40G  PHY register
+ *  @hw: pointer to the HW structure
+ *  @offset: lower half is register offset to read to
+ *     upper half is page to use.
+ *  @data: data to read at register offset
+ *
+ *  Acquires semaphore, if necessary, then reads the data in the PHY register
+ *  at the offset.  Release any acquired semaphores before exiting.
+ **/
+s32 igb_read_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 *data)
+{
+	return igb_read_reg_gs40g(hw, hw->phy.addr, offset, data);
+}
+
+/**
   *  igb_set_master_slave_mode - Setup PHY for Master/slave mode
   *  @hw: pointer to the HW structure
   *

drivers/net/ethernet/intel/igb/e1000_phy.h +4 -2

index 7af4ffa..6256e76 100644
@@ -61,8 +61,8 @@ s32  igb_phy_has_link(struct e1000_hw *hw, u32 iterations,
  void igb_power_up_phy_copper(struct e1000_hw *hw);
  void igb_power_down_phy_copper(struct e1000_hw *hw);
  s32  igb_phy_init_script_igp3(struct e1000_hw *hw);
-s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 *data);
-s32  igb_write_phy_reg_mdic(struct e1000_hw *hw, u32 offset, u16 data);
+s32  igb_read_phy_reg_mdic(struct e1000_hw *hw, u8 addr, u32 offset, u16 *data);
+s32  igb_write_phy_reg_mdic(struct e1000_hw *hw, u8 addr, u32 offset, u16 data);
  s32  igb_read_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 *data);
  s32  igb_write_phy_reg_i2c(struct e1000_hw *hw, u32 offset, u16 data);
  s32  igb_read_sfp_data_byte(struct e1000_hw *hw, u16 offset, u8 *data);

@@ -72,6 +72,8 @@ s32  igb_phy_force_speed_duplex_82580(struct e1000_hw *hw);
  s32  igb_get_cable_length_82580(struct e1000_hw *hw);
  s32  igb_read_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 *data);
  s32  igb_write_phy_reg_gs40g(struct e1000_hw *hw, u32 offset, u16 data);
+s32  igb_read_reg_gs40g(struct e1000_hw *hw, u8 addr, u32 offset, u16 *data);
+s32  igb_write_reg_gs40g(struct e1000_hw *hw, u8 addr, u32 offset, u16 data);
  s32  igb_check_polarity_m88(struct e1000_hw *hw);
 
  /* IGP01E1000 Specific Registers */

Quoted 7 lines · Tim Harvey

Add igb_write_reg_gs40g/igb_read_reg_gs40g that can be passed a phy address.
The existing igb_write_phy_reg_gs40g/igb_read_phy_reg_gs40g become wrappers
to this function.

Quoted 1 line

Signed-off-by Tim Harvey <[email protected]>

I'm pretty sure this isn't useful as well. You really should not be doing any phy address/MDICNFG manipulation in these calls.

Quoted 11 lines · Alexander Duyck

Quoted 7 lines · Tim Harvey

Add igb_write_reg_gs40g/igb_read_reg_gs40g that can be passed a phy address.
The existing igb_write_phy_reg_gs40g/igb_read_phy_reg_gs40g become wrappers
to this function.

Signed-off-by Tim Harvey <[email protected]>

I'm pretty sure this isn't useful as well. You really should not be doing any phy address/MDICNFG manipulation in these calls.

Alexander,

This patch is to support phylib which requires mdio functions that can pass a phy address rather than take it from the e1000_phy_info structure.

Tim

If an i210 is configured for 1000BASE-BX link_mode and has an external phy specified, then register an mii bus using the external phy address as a mask.

An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something that talks on the mdio bus but is not a standard phy, such as a switch. In this case by registering an mdio bus a phy driver can manage the device.

Signed-off-by Tim Harvey <[email protected]>

---
  drivers/net/ethernet/intel/igb/e1000_82575.c |  16 +++
  drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
  drivers/net/ethernet/intel/igb/igb_main.c    | 163 ++++++++++++++++++++++++++-
  3 files changed, 181 insertions(+), 5 deletions(-)

drivers/net/ethernet/intel/igb/e1000_82575.c +1 -0

index d2afd7b..e80617b 100644
@@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw *hw)
  	switch (link_mode) {
  	case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX:
  		hw->phy.media_type = e1000_media_type_internal_serdes;
+		if (igb_sgmii_uses_mdio_82575(hw)) {

drivers/net/ethernet/intel/igb/e1000_hw.h +1 -0

index 2003b37..2864779 100644
@@ -27,6 +27,7 @@
  #include <linux/delay.h>
  #include <linux/io.h>
  #include <linux/netdevice.h>
+#include <linux/phy.h>
 
  #include "e1000_regs.h"
  #include "e1000_defines.h"

@@ -543,6 +544,12 @@ struct e1000_hw {
  	struct e1000_mbx_info mbx;
  	struct e1000_host_mng_dhcp_cookie mng_cookie;

drivers/net/ethernet/intel/igb/igb_main.c +4 -0

index f366b3b..9b5f538 100644
@@ -41,6 +41,7 @@
  #include <linux/if_vlan.h>
  #include <linux/pci.h>
  #include <linux/pci-aspm.h>
+#include <linux/phy.h>
  #include <linux/delay.h>
  #include <linux/interrupt.h>
  #include <linux/ip.h>

@@ -2223,6 +2224,121 @@ static s32 igb_init_i2c(struct igb_adapter *adapter)
  	return status;
  }
 
+#ifdef CONFIG_PHYLIB
+static int igb_enet_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{

drivers/net/ethernet/intel/igb/igb_main.c +5 -0

+/* Probe the mdio bus for phys and connect them */
+static int igb_enet_mii_probe(struct net_device *netdev)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;

drivers/net/ethernet/intel/igb/igb_main.c +69 -0

+	if (!phy_dev) {
+		netdev_err(netdev, "no PHY found\n");
+		return -ENODEV;
+	}
+
+	hw->phy_interface = PHY_INTERFACE_MODE_RGMII;
+	phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev),
+			      igb_enet_mii_link, hw->phy_interface);
+	if (IS_ERR(phy_dev)) {
+		netdev_err(netdev, "could not attach to PHY\n");
+		return PTR_ERR(phy_dev);
+	}
+
+	hw->phy_dev = phy_dev;
+	netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n",
+		    hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev));
+
+	return 0;
+}
+
+/* Create and register mdio bus */
+static int igb_enet_mii_init(struct pci_dev *pdev)
+{
+	struct mii_bus *mii_bus;
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	int err;
+
+	mii_bus = mdiobus_alloc();
+	if (!mii_bus) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	mii_bus->name = "igb_enet_mii_bus";
+	mii_bus->read = igb_enet_mdio_read;
+	mii_bus->write = igb_enet_mdio_write;
+	mii_bus->reset = igb_enet_mdio_reset;
+	snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
+		 pci_name(pdev), hw->device_id + 1);
+	mii_bus->priv = hw;
+	mii_bus->parent = &pdev->dev;
+	mii_bus->phy_mask = ~(1 << hw->phy.addr);
+
+	err = mdiobus_register(mii_bus);
+	if (err) {
+		netdev_err(netdev, "failed to register mii_bus: %d\n", err);
+		goto err_out_free_mdiobus;
+	}
+	hw->mii_bus = mii_bus;
+
+	return 0;
+
+err_out_free_mdiobus:
+	mdiobus_free(mii_bus);
+err_out:
+	return err;
+}
+
+static void igb_enet_mii_remove(struct e1000_hw *hw)
+{
+	if (hw->mii_bus) {
+		mdiobus_unregister(hw->mii_bus);
+		mdiobus_free(hw->mii_bus);
+	}
+}
+#endif /* CONFIG_PHYLIB */
+
  /**
   *  igb_probe - Device Initialization Routine
   *  @pdev: PCI device information struct

@@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
  		}
  	}
  	pm_runtime_put_noidle(&pdev->dev);

drivers/net/ethernet/intel/igb/igb_main.c +18 -5

@@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
  static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
  {
  	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
  	struct mii_ioctl_data *data = if_mii(ifr);
 
-	if (adapter->hw.phy.media_type != e1000_media_type_copper)
+	if (adapter->hw.phy.media_type != e1000_media_type_copper &&
+	    !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO))
  		return -EOPNOTSUPP;
 
  	switch (cmd) {
  	case SIOCGMIIPHY:
-		data->phy_id = adapter->hw.phy.addr;
+		data->phy_id = hw->phy.addr;
  		break;
  	case SIOCGMIIREG:
-		if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
-				     &data->val_out))
-			return -EIO;
+		if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) {
+			if (igb_read_reg_gs40g(&adapter->hw, data->phy_id,
+					       data->reg_num & 0x1F,
+					       &data->val_out))
+				return -EIO;
+		} else {
+			if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
+					     &data->val_out))
+				return -EIO;
+		}
  		break;
  	case SIOCSMIIREG:
+		if (hw->mac.type == e1000_i210 || hw->mac.type == e1000_i211) {
+			if (igb_write_reg_gs40g(hw, data->phy_id,
+						data->reg_num & 0x1F,
+						data->val_in))

Quoted 5 lines · Tim Harvey

If an i210 is configured for 1000BASE-BX link_mode and has an external phy specified, then register an mii bus using the external phy address as a mask.

Quoted 8 lines

An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something that talks on the mdio bus but is not a standard phy, such as a switch. In this case by registering an mdio bus a phy driver can manage the device.

Signed-off-by Tim Harvey <[email protected]>

So I think I am staring to see a pattern here between the i210 and the i354 it looks like the EEPROMs for these interfaces are not getting set up correctly. Unfortunately for the i210 you cannot change the EEPROM from ethtool so you would need to work out with your manufacturer how to get that fixed for your device.

As far as the code below I think there may be an easy way to work around a bunch of this code. The quick trick for most of this would be to update igb_init_phy_params_82575 to return a new value, maybe make up something like E1000_ERR_PHY_UNKNOWN when the PHY ID is unrecognized. Then you could add the phylib code as a handler for if you encounter that error code.

Also in terms of naming I would recommend sticking with one convention. Either igb_mdio_ or igb_mii_ for all of these functions. It is kind of confusing to be switching back and forth and I don't really see the point of adding the "enet" to the prefixes.

Quoted 20 lines

---
  drivers/net/ethernet/intel/igb/e1000_82575.c |  16 +++
  drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
  drivers/net/ethernet/intel/igb/igb_main.c    | 163
++++++++++++++++++++++++++-
  3 files changed, 181 insertions(+), 5 deletions(-)

drivers/net/ethernet/intel/igb/e1000_82575.c +2 -0

index d2afd7b..e80617b 100644
@@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw
 *hw)
        switch (link_mode) {
        case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX:
                hw->phy.media_type = e1000_media_type_internal_serdes;
+               if (igb_sgmii_uses_mdio_82575(hw)) {
+                       u32 mdicnfg = rd32(E1000_MDICNFG);

drivers/net/ethernet/intel/igb/e1000_82575.c static s32 igb_get_invariants_82575(struct e1000_hw · line 613

Quoted 10 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +8 -0

+
+                       mdicnfg &= E1000_MDICNFG_PHY_MASK;
+                       hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT;
+                       hw_dbg("1000BASE_KX w/ external MDIO device at
 0x%x\n",
+                              hw->phy.addr);
+               } else {
+                       hw_dbg("1000BASE_KX");
+               }
                break;

You should not be running SGMII under KX mode. If you are then you have an EEPROM bug. The correct link mode is SGMII below. Unfortunately you would need to check with your vendor on that one as I believe the EEPROMs for i210/i211 parts are not writable via ethtool.

As for a hack workaround for now what you could do is use a !igb_sgmii_uses_mdio_82575() check to determine if you want to break, or to clear the LINK_MODE field and rewrite it as SGMII and then just fall though to SGMII which will help to clear up many issues you are probably seeing. You might keep that as a separate local patch until you can get the EEPROM issue resolved.

As far as pulling the phy.addr value that should be taken care of by igb_get_phy_id_82575 if the link mode is correctly updated in the EEPROM to LINK_MODE_SGMII.

Quoted 10 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +3 -0

        case E1000_CTRL_EXT_LINK_MODE_SGMII:
                /* Get phy control interface type set (MDIO vs. I2C)*/
                if (igb_sgmii_uses_mdio_82575(hw)) {
                        hw->phy.media_type = e1000_media_type_copper;
                        dev_spec->sgmii_active = true;
+                       hw_dbg("SGMII with external MDIO PHY");
                        break;
+               } else {
+                       hw_dbg("SGMII with external I2C PHY");
                }

Quoted 2 lines

                /* fall through for I2C based SGMII */
        case E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES:

These hw_dbg messages can probably be dropped since they aren't adding much value.

Quoted 13 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +3 -0

@@ -621,8 +634,11 @@ static s32 igb_get_invariants_82575(struct e1000_hw
 *hw)
                                hw->phy.media_type =
 e1000_media_type_copper;
                                dev_spec->sgmii_active = true;
                        }
+                       hw_dbg("SERDES with external SFP");
 
                        break;
+               } else {
+                       hw_dbg("SERDES");
                }

Quoted 1 line

                /* do not change link mode for 100BaseFX */

Same for these.

Quoted 18 lines

drivers/net/ethernet/intel/igb/e1000_hw.h +1 -0

index 2003b37..2864779 100644
@@ -27,6 +27,7 @@
  #include <linux/delay.h>
  #include <linux/io.h>
  #include <linux/netdevice.h>
+#include <linux/phy.h>
 
  #include "e1000_regs.h"
  #include "e1000_defines.h"

@@ -543,6 +544,12 @@ struct e1000_hw {
        struct e1000_mbx_info mbx;
        struct e1000_host_mng_dhcp_cookie mng_cookie;
 

drivers/net/ethernet/intel/igb/e1000_hw.h struct e1000_hw { · line 555

Quoted 9 lines

drivers/net/ethernet/intel/igb/e1000_hw.h +6 -0

+#ifdef CONFIG_PHYLIB
+       /* Phylib and MDIO interface */
+       struct mii_bus *mii_bus;
+       struct phy_device *phy_dev;
+       phy_interface_t phy_interface;
+#endif
        union {
                struct e1000_dev_spec_82575     _82575;
        } dev_spec;

Just to save the Intel guys a ton of grief you may want to consider moving these into the igb_adapter struct instead of the e1000_hw struct. The fact is these are all Linux specific interfaces and the e1000_hw is meant to be more-or-less OS agnostic.

Quoted 30 lines

drivers/net/ethernet/intel/igb/igb_main.c +10 -0

index f366b3b..9b5f538 100644
@@ -41,6 +41,7 @@
  #include <linux/if_vlan.h>
  #include <linux/pci.h>
  #include <linux/pci-aspm.h>
+#include <linux/phy.h>
  #include <linux/delay.h>
  #include <linux/interrupt.h>
  #include <linux/ip.h>

@@ -2223,6 +2224,121 @@ static s32 igb_init_i2c(struct igb_adapter
 *adapter)
        return status;
  }
 
+#ifdef CONFIG_PHYLIB
+static int igb_enet_mdio_read(struct mii_bus *bus, int mii_id, int
 regnum)
+{
+       struct e1000_hw *hw = bus->priv;
+       u16 out;
+       int err;
+
+       err = igb_read_reg_gs40g(hw, mii_id, regnum, &out);
+       if (err)

drivers/net/ethernet/intel/igb/igb_main.c static s32 igb_init_i2c(struct igb_adapter *adapter) · line 2239

Quoted 4 lines

drivers/net/ethernet/intel/igb/igb_main.c +4 -0

+               return err;
+       return out;
+}
+

I'm not really a fan of the igb_enet_ stuff, why not just igb_mdio_read, igb_mdio_write, etc..?

drivers/net/ethernet/intel/igb/igb_main.c static s32 igb_init_i2c(struct igb_adapter *adapter) · line 2247

Quoted 10 lines

drivers/net/ethernet/intel/igb/igb_main.c +8 -0

+static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int
 regnum,
+                              u16 val)
+{
+       struct e1000_hw *hw = bus->priv;
+
+       return igb_write_reg_gs40g(hw, mii_id, regnum, val);
+}
+

There shouldn't be any need to actually pass the PHY address assuming you let the driver code take care of pulling the address of the PHY from the EEPROM before hand. As such you can probably just not use the mii_id value.

drivers/net/ethernet/intel/igb/igb_main.c static s32 igb_init_i2c(struct igb_adapter *adapter) · line 2253

Quoted 7 lines

drivers/net/ethernet/intel/igb/igb_main.c +6 -0

+static int igb_enet_mdio_reset(struct mii_bus *bus)
+{
+       usleep_range(1000, 2000);
+       return 0;
+}
+

Is there a reason for having a sleep here if you aren't doing anything? You could probably just not define this since it doesn't do anything.

drivers/net/ethernet/intel/igb/igb_main.c static s32 igb_init_i2c(struct igb_adapter *adapter) · line 2257

Quoted 5 lines

drivers/net/ethernet/intel/igb/igb_main.c +4 -0

+static void igb_enet_mii_link(struct net_device *netdev)
+{
+}
+

Seems like there should probably be something here, but I don't know enough about phylib yet to say what.

Quoted 14 lines

drivers/net/ethernet/intel/igb/igb_main.c +13 -0

+/* Probe the mdio bus for phys and connect them */
+static int igb_enet_mii_probe(struct net_device *netdev)
+{
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       struct phy_device *phy_dev = NULL;
+       int phy_id;
+
+       /* check for attached phy */
+       for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {
+               if (hw->mii_bus->phy_map[phy_id]) {
+                       phy_dev = hw->mii_bus->phy_map[phy_id];
+                       break;

drivers/net/ethernet/intel/igb/igb_main.c static s32 igb_init_i2c(struct igb_adapter · line 2275

Quoted 2 lines

drivers/net/ethernet/intel/igb/igb_main.c +2 -0

+               }
+       }

There is code that should have already figured this out in the driver. We just need to check the PHY addr after get_invarians has been called, assuming the EEPROM is correct. So you should be able to just get the phy.addr from the e1000_hw struct.

Quoted 89 lines

drivers/net/ethernet/intel/igb/igb_main.c +48 -0

+       if (!phy_dev) {
+               netdev_err(netdev, "no PHY found\n");
+               return -ENODEV;
+       }
+
+       hw->phy_interface = PHY_INTERFACE_MODE_RGMII;
+       phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev),
+                             igb_enet_mii_link, hw->phy_interface);
+       if (IS_ERR(phy_dev)) {
+               netdev_err(netdev, "could not attach to PHY\n");
+               return PTR_ERR(phy_dev);
+       }
+
+       hw->phy_dev = phy_dev;
+       netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n",
+                   hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev));
+
+       return 0;
+}
+
+/* Create and register mdio bus */
+static int igb_enet_mii_init(struct pci_dev *pdev)
+{
+       struct mii_bus *mii_bus;
+       struct net_device *netdev = pci_get_drvdata(pdev);
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       int err;
+
+       mii_bus = mdiobus_alloc();
+       if (!mii_bus) {
+               err = -ENOMEM;
+               goto err_out;
+       }
+
+       mii_bus->name = "igb_enet_mii_bus";
+       mii_bus->read = igb_enet_mdio_read;
+       mii_bus->write = igb_enet_mdio_write;
+       mii_bus->reset = igb_enet_mdio_reset;
+       snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
+                pci_name(pdev), hw->device_id + 1);
+       mii_bus->priv = hw;
+       mii_bus->parent = &pdev->dev;
+       mii_bus->phy_mask = ~(1 << hw->phy.addr);
+
+       err = mdiobus_register(mii_bus);
+       if (err) {
+               netdev_err(netdev, "failed to register mii_bus: %d\n",

err);

drivers/net/ethernet/intel/igb/igb_main.c +28 -0

+               goto err_out_free_mdiobus;
+       }
+       hw->mii_bus = mii_bus;
+
+       return 0;
+
+err_out_free_mdiobus:
+       mdiobus_free(mii_bus);
+err_out:
+       return err;
+}
+
+static void igb_enet_mii_remove(struct e1000_hw *hw)
+{
+       if (hw->mii_bus) {
+               mdiobus_unregister(hw->mii_bus);
+               mdiobus_free(hw->mii_bus);
+       }
+}
+#endif /* CONFIG_PHYLIB */
+
  /**
   *  igb_probe - Device Initialization Routine
   *  @pdev: PCI device information struct

@@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const
 struct pci_device_id *ent)
                }
        }
        pm_runtime_put_noidle(&pdev->dev);
+
+#ifdef CONFIG_PHYLIB
+       /* create and register the mdio bus if using ext phy */
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_init(pdev);
+#endif
+
        return 0;
 

Quoted 1 line

err_register:

The MDICNFG register doesn't exist for all devices as I recall. What you may want to do is make enabling the mii_bus optional dependent on get_invariants returning an error that the PHY is not recognized.

drivers/net/ethernet/intel/igb/igb_main.c static void igb_remove(struct pci_dev *pdev) · line 2920

Quoted 11 lines

drivers/net/ethernet/intel/igb/igb_main.c +4 -0

@@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev)
        struct e1000_hw *hw = &adapter->hw;
 
        pm_runtime_get_noresume(&pdev->dev);
+#ifdef CONFIG_PHYLIB
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_remove(hw);
+#endif
  #ifdef CONFIG_IGB_HWMON
        igb_sysfs_exit(adapter);
  #endif

Same thing here. I would say what you could do is just check to see if a mii_bus is allocated and if so remove it. You could probably push the check inside of igb_mii_remove.

Quoted 5 lines

drivers/net/ethernet/intel/igb/igb_main.c +0 -0

@@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev,
 bool resuming)
        if (!resuming)
                pm_runtime_put(&pdev->dev);
 

Quoted 9 lines

drivers/net/ethernet/intel/igb/igb_main.c +6 -0

+#ifdef CONFIG_PHYLIB
+       /* Probe and connect to PHY if using ext phy */
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_probe(netdev);
+#endif
+
        /* start the watchdog. */
        hw->mac.get_link_status = 1;

        schedule_work(&adapter->watchdog_task);

Same here, use the existance of a mii_bus, not the register check.

Quoted 50 lines

drivers/net/ethernet/intel/igb/igb_main.c +15 -5

@@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring
 *rx_ring, u16 cleaned_count)
  static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
 int cmd)
  {
        struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
        struct mii_ioctl_data *data = if_mii(ifr);
 
-       if (adapter->hw.phy.media_type != e1000_media_type_copper)
+       if (adapter->hw.phy.media_type != e1000_media_type_copper &&
+           !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO))
                return -EOPNOTSUPP;
 
        switch (cmd) {
        case SIOCGMIIPHY:
-               data->phy_id = adapter->hw.phy.addr;
+               data->phy_id = hw->phy.addr;
                break;
        case SIOCGMIIREG:
-               if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
-                                    &data->val_out))
-                       return -EIO;
+               if (hw->mac.type == e1000_i210 || hw->mac.type ==
 e1000_i211) {
+                       if (igb_read_reg_gs40g(&adapter->hw, data->phy_id,
+                                              data->reg_num & 0x1F,
+                                              &data->val_out))
+                               return -EIO;
+               } else {
+                       if (igb_read_phy_reg(&adapter->hw, data->reg_num &
 0x1F,
+                                            &data->val_out))
+                               return -EIO;
+               }
                break;
        case SIOCSMIIREG:
+               if (hw->mac.type == e1000_i210 || hw->mac.type ==

e1000_i211) {

drivers/net/ethernet/intel/igb/igb_main.c +10 -0

+                       if (igb_write_reg_gs40g(hw, data->phy_id,
+                                               data->reg_num & 0x1F,
+                                               data->val_in))
+                               return -EIO;
+               } else {
+                       if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
+                                             data->val_in))
+                               return -EIO;
+               }
+               break;

Quoted 4 lines

        default:
                return -EOPNOTSUPP;
        }

These changes just shouldn't be needed. I'd say they could be dropped. The phy_id should be static and configured by the hardware before the driver is even loaded.

Quoted 12 lines · Alexander Duyck

Quoted 11 lines · Tim Harvey

If an i210 is configured for 1000BASE-BX link_mode and has an external phy specified, then register an mii bus using the external phy address as a mask.

An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something that talks on the mdio bus but is not a standard phy, such as a switch. In this case by registering an mdio bus a phy driver can manage the device.

Quoted 8 lines

Quoted 1 line

Signed-off-by Tim Harvey <[email protected]>

So I think I am staring to see a pattern here between the i210 and the i354 it looks like the EEPROMs for these interfaces are not getting set up correctly. Unfortunately for the i210 you cannot change the EEPROM from ethtool so you would need to work out with your manufacturer how to get that fixed for your device.

Alexander,

Thanks for the review!

I think much of my patchset is more clear if I give a bit more background of how we use the i210 on our GW16083
(http://www.gateworks.com/product/item/ventana-gw16083-mezzanine).
This board is an i210 connected to a Marvell MV88E6176 6-port switch via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and instead of having a register interface that utilizes a single phy address, it uses several addresses between 0x10 and 0x1d. The link between the two chips is thus 1gbps full-duplex without the need of link management (why I consider it phy-less).

I configured the EEPROM with the following details:
- link-mode: 1000BASE-KX for phy-less operation
- external-mdio
- phy_addr 0x10 (the first one used by the MV88E6176 and the one that
maps more closely to standard phy registers than any others)
- device_id: 0x157C (SerDes flashless - looking back perhaps this
should have been 0x1537 but then this isn't used in igb to distinguish any details so its a don't-care)

The i210 datasheet defines the following link-modes:
0 - Internal PHY
1 - 1000BASE-KX
2 - SGMII
3 - SerDes/1000BASE-BX

The differences are still not crystal clear to me and is really all about how the igb driver uses this value. When I wrote support for the GW16083 a year ago I sent out a query to the e1000-devel list regarding link_mode and how I should proceed but I received no reply's
(http://permalink.gmane.org/gmane.linux.drivers.e1000.devel/13942) and unfortunately am only now getting back to a point of having any bandwidth to try to mainline my changes.

I would have to dig a bit to find where I got this info but my understanding was that 1000BASE-KX was for fixed 1gbps 'phy-less' SerDes links (requiring no link negotiation) and because I have the i210 connected in to a port on the MV88E6176 that is always 1gpbs full-duplex this made sense to me to let the driver treat it as 'phy-less'.

Quoted 5 lines

As far as the code below I think there may be an easy way to work around a bunch of this code. The quick trick for most of this would be to update igb_init_phy_params_82575 to return a new value, maybe make up something like E1000_ERR_PHY_UNKNOWN when the PHY ID is unrecognized. Then you could add the phylib code as a handler for if you encounter that error code.

Are you sure this shouldn't be based off of link mode? It seems to me that if your link-mode indicates an external phy then this is exactly when you would want to register an mii bus with phylib. However, also doing so as a fallback if PHY ID can't be recognized also makes sense in general for perhaps boards with mis-programmed EEPROM's.

Quoted 35 lines

Quoted 19 lines

---
   drivers/net/ethernet/intel/igb/e1000_82575.c |  16 +++
   drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
   drivers/net/ethernet/intel/igb/igb_main.c    | 163
++++++++++++++++++++++++++-
   3 files changed, 181 insertions(+), 5 deletions(-)

drivers/net/ethernet/intel/igb/e1000_82575.c +2 -0

index d2afd7b..e80617b 100644
@@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct e1000_hw
 *hw)
         switch (link_mode) {
         case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX:
                 hw->phy.media_type = e1000_media_type_internal_serdes;
+               if (igb_sgmii_uses_mdio_82575(hw)) {
+                       u32 mdicnfg = rd32(E1000_MDICNFG);

drivers/net/ethernet/intel/igb/e1000_82575.c static s32 igb_get_invariants_82575(struct e1000_hw · line 613

Quoted 10 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +8 -0

+
+                       mdicnfg &= E1000_MDICNFG_PHY_MASK;
+                       hw->phy.addr = mdicnfg >> E1000_MDICNFG_PHY_SHIFT;
+                       hw_dbg("1000BASE_KX w/ external MDIO device at
 0x%x\n",
+                              hw->phy.addr);
+               } else {
+                       hw_dbg("1000BASE_KX");
+               }
                 break;

You should not be running SGMII under KX mode. If you are then you have an EEPROM bug. The correct link mode is SGMII below. Unfortunately you would need to check with your vendor on that one as I believe the EEPROMs for i210/i211 parts are not writable via ethtool.

Quoted 6 lines

As for a hack workaround for now what you could do is use a !igb_sgmii_uses_mdio_82575() check to determine if you want to break, or to clear the LINK_MODE field and rewrite it as SGMII and then just fall though to SGMII which will help to clear up many issues you are probably seeing. You might keep that as a separate local patch until you can get the EEPROM issue resolved.

I certainly hope this isn't considered an EEPROM programming issue because at the time I could get no clarification from the community or my Intel FAE on what mode we should be using (the FAE understandably likely didn't know because this is an implementation detail within the driver the way I see it).

We have been shipping boards for a year supported by a phylib driver and it would not be easy to field update these because the i210 nvram is only supported via the non-redistributable licensed eepromARMtool (yuck!). One of the key U-Boot developers tried to get Intel to allow him to add source to U-Boot for EEPROM programming and was denied. (again... yuck!).

Quoted 5 lines

Quoted 5 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +4 -0

+static int igb_enet_mdio_write(struct mii_bus *bus, int mii_id, int
 regnum,
+                              u16 val)
+{
+       struct e1000_hw *hw = bus->priv;

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_82575.c static s32 igb_get_invariants_82575(struct e1000_hw · line 622

Quoted 4 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +4 -0

+
+       return igb_write_reg_gs40g(hw, mii_id, regnum, val);
+}
+

There shouldn't be any need to actually pass the PHY address assuming you let the driver code take care of pulling the address of the PHY from the EEPROM before hand. As such you can probably just not use the mii_id value.

Again, the reason for not using the address from the EEPROM is this use case where there is not a single phy address. This is also why I'm patching the mdio read/write functions to take a parameter instead of using the internal value (and creating wrappers for when they are used internally in igb).

Quoted 12 lines

Quoted 12 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +12 -0

+/* Probe the mdio bus for phys and connect them */
+static int igb_enet_mii_probe(struct net_device *netdev)
+{
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       struct phy_device *phy_dev = NULL;
+       int phy_id;
+
+       /* check for attached phy */
+       for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {
+               if (hw->mii_bus->phy_map[phy_id]) {
+                       phy_dev = hw->mii_bus->phy_map[phy_id];

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_82575.c

Quoted 3 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +3 -0

+                       break;
+               }
+       }

There is code that should have already figured this out in the driver. We just need to check the PHY addr after get_invarians has been called, assuming the EEPROM is correct. So you should be able to just get the phy.addr from the e1000_hw struct.

It is true that in igb_enet_mii_init() where the mii bus is registered with phylib that I set the phy_mask to include only the phy address from the eeprom. In my case I put 0x10 in the EEPROM as the phy address as that is the first phy address that the switch responds to and so I use that as a way to detect the phy.

I can remove the loop and just set phy_dev =
hw->mii_bus->phy_map[hw->phy.addr] in this case.

Quoted 116 lines

Quoted 89 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +48 -0

+       if (!phy_dev) {
+               netdev_err(netdev, "no PHY found\n");
+               return -ENODEV;
+       }
+
+       hw->phy_interface = PHY_INTERFACE_MODE_RGMII;
+       phy_dev = phy_connect(netdev, dev_name(&phy_dev->dev),
+                             igb_enet_mii_link, hw->phy_interface);
+       if (IS_ERR(phy_dev)) {
+               netdev_err(netdev, "could not attach to PHY\n");
+               return PTR_ERR(phy_dev);
+       }
+
+       hw->phy_dev = phy_dev;
+       netdev_info(netdev, "igb PHY driver [%s] (mii_bus:phy_addr=%s)\n",
+                   hw->phy_dev->drv->name, dev_name(&hw->phy_dev->dev));
+
+       return 0;
+}
+
+/* Create and register mdio bus */
+static int igb_enet_mii_init(struct pci_dev *pdev)
+{
+       struct mii_bus *mii_bus;
+       struct net_device *netdev = pci_get_drvdata(pdev);
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       int err;
+
+       mii_bus = mdiobus_alloc();
+       if (!mii_bus) {
+               err = -ENOMEM;
+               goto err_out;
+       }
+
+       mii_bus->name = "igb_enet_mii_bus";
+       mii_bus->read = igb_enet_mdio_read;
+       mii_bus->write = igb_enet_mdio_write;
+       mii_bus->reset = igb_enet_mdio_reset;
+       snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
+                pci_name(pdev), hw->device_id + 1);
+       mii_bus->priv = hw;
+       mii_bus->parent = &pdev->dev;
+       mii_bus->phy_mask = ~(1 << hw->phy.addr);
+
+       err = mdiobus_register(mii_bus);
+       if (err) {
+               netdev_err(netdev, "failed to register mii_bus: %d\n",

err);

drivers/net/ethernet/intel/igb/e1000_82575.c +28 -0

+               goto err_out_free_mdiobus;
+       }
+       hw->mii_bus = mii_bus;
+
+       return 0;
+
+err_out_free_mdiobus:
+       mdiobus_free(mii_bus);
+err_out:
+       return err;
+}
+
+static void igb_enet_mii_remove(struct e1000_hw *hw)
+{
+       if (hw->mii_bus) {
+               mdiobus_unregister(hw->mii_bus);
+               mdiobus_free(hw->mii_bus);
+       }
+}
+#endif /* CONFIG_PHYLIB */
+
   /**
    *  igb_probe - Device Initialization Routine
    *  @pdev: PCI device information struct

@@ -2645,6 +2761,13 @@ static int igb_probe(struct pci_dev *pdev, const
 struct pci_device_id *ent)
                 }
         }
         pm_runtime_put_noidle(&pdev->dev);
+
+#ifdef CONFIG_PHYLIB
+       /* create and register the mdio bus if using ext phy */
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_init(pdev);
+#endif
+
         return 0;
 

Quoted 1 line

err_register:

The MDICNFG register doesn't exist for all devices as I recall. What you may want to do is make enabling the mii_bus optional dependent on get_invariants returning an error that the PHY is not recognized.

drivers/net/ethernet/intel/igb/e1000_82575.c static void igb_remove(struct pci_dev *pdev) · line 2920

Quoted 11 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +4 -0

@@ -2788,6 +2911,10 @@ static void igb_remove(struct pci_dev *pdev)
         struct e1000_hw *hw = &adapter->hw;
 
         pm_runtime_get_noresume(&pdev->dev);
+#ifdef CONFIG_PHYLIB
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_remove(hw);
+#endif
   #ifdef CONFIG_IGB_HWMON
         igb_sysfs_exit(adapter);
   #endif

Same thing here. I would say what you could do is just check to see if a mii_bus is allocated and if so remove it. You could probably push the check inside of igb_mii_remove.

Quoted 5 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +0 -0

@@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev,
 bool resuming)
         if (!resuming)
                 pm_runtime_put(&pdev->dev);
 

Quoted 12 lines

Quoted 9 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +6 -0

+#ifdef CONFIG_PHYLIB
+       /* Probe and connect to PHY if using ext phy */
+       if (rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO)
+               igb_enet_mii_probe(netdev);
+#endif
+
         /* start the watchdog. */
         hw->mac.get_link_status = 1;

         schedule_work(&adapter->watchdog_task);

Same here, use the existance of a mii_bus, not the register check.

This goes back to the discussion on how to decide if we should register with phylib and will depend on how that discussion plays out. There are about 4 places I need to know if we're using phylib and whatever mechanism is agree'd upon can be used in all these cases:
igb_probe() - register the mii bus with phylib igb_remove() - remove the mii bus
igb_open() - to call the mii probe
igb_mii_ioctl() - to know to use the mdio read/write wrappers

Quoted 48 lines

Quoted 48 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +15 -5

@@ -7127,21 +7260,41 @@ void igb_alloc_rx_buffers(struct igb_ring
 *rx_ring, u16 cleaned_count)
   static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
 int cmd)
   {
         struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
         struct mii_ioctl_data *data = if_mii(ifr);
 
-       if (adapter->hw.phy.media_type != e1000_media_type_copper)
+       if (adapter->hw.phy.media_type != e1000_media_type_copper &&
+           !(rd32(E1000_MDICNFG) & E1000_MDICNFG_EXT_MDIO))
                 return -EOPNOTSUPP;
 
         switch (cmd) {
         case SIOCGMIIPHY:
-               data->phy_id = adapter->hw.phy.addr;
+               data->phy_id = hw->phy.addr;
                 break;
         case SIOCGMIIREG:
-               if (igb_read_phy_reg(&adapter->hw, data->reg_num & 0x1F,
-                                    &data->val_out))
-                       return -EIO;
+               if (hw->mac.type == e1000_i210 || hw->mac.type ==
 e1000_i211) {
+                       if (igb_read_reg_gs40g(&adapter->hw, data->phy_id,
+                                              data->reg_num & 0x1F,
+                                              &data->val_out))
+                               return -EIO;
+               } else {
+                       if (igb_read_phy_reg(&adapter->hw, data->reg_num &
 0x1F,
+                                            &data->val_out))
+                               return -EIO;
+               }
                 break;
         case SIOCSMIIREG:
+               if (hw->mac.type == e1000_i210 || hw->mac.type ==

e1000_i211) {

drivers/net/ethernet/intel/igb/e1000_82575.c +9 -0

+                       if (igb_write_reg_gs40g(hw, data->phy_id,
+                                               data->reg_num & 0x1F,
+                                               data->val_in))
+                               return -EIO;
+               } else {
+                       if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
+                                             data->val_in))
+                               return -EIO;
+               }

Quoted 8 lines

Quoted 5 lines

+               break;
         default:
                 return -EOPNOTSUPP;
         }

These changes just shouldn't be needed. I'd say they could be dropped. The phy_id should be static and configured by the hardware before the driver is even loaded.

The above also adds SIOCGMIIREG which previously was not supported, but I also need to allow multiple phy addresses here and thus can't just default to igb_write_phy_reg() which uses the single phy addr from the EEPROM (unless I move multi-addr support there).

Tim

Quoted 11 lines · Tim Harvey

Quoted 8 lines · Alexander Duyck

Quoted 6 lines · Tim Harvey

If an i210 is configured for 1000BASE-BX link_mode and has an external phy specified, then register an mii bus using the external phy address as
a mask.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something that

Quoted 20 lines

Quoted 8 lines

Quoted 5 lines

on the mdio bus but is not a standard phy, such as a switch. In this case
by registering an mdio bus a phy driver can manage the device.

Signed-off-by Tim Harvey <[email protected]>

Quoted 7 lines

So I think I am staring to see a pattern here between the i210 and the i354
it looks like the EEPROMs for these interfaces are not getting set up correctly. Unfortunately for the i210 you cannot change the EEPROM from ethtool so you would need to work out with your manufacturer how to get that
fixed for your device.

Alexander,

Thanks for the review!

Quoted 9 lines

I think much of my patchset is more clear if I give a bit more background of how we use the i210 on our GW16083
(http://www.gateworks.com/product/item/ventana-gw16083-mezzanine).
This board is an i210 connected to a Marvell MV88E6176 6-port switch via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and instead of having a register interface that utilizes a single phy address, it uses several addresses between 0x10 and 0x1d. The link between the two chips is thus 1gbps full-duplex without the need of link management (why I consider it phy-less).

Okay, so this is likely much more complicated then your original patch let on. What you are actually doing is enabling phylib so that you can

DSA switch on top of the i210 interface, do I have that right?

The 1000BASE-KX is really meant for pure SerDes configuration where there will be no PHY attached at all. So for example if you were going to

directly to another 1000BASE-KX port on either a backplane switch or

adapter. It is implied that by using this you aren't going to use the

interface at all. There are essentially 3 modes that do external 1gbs 'phy-less' and the problem is that KX implies a special mode that is typically used for backplane w/o any link negotiation. Since there is a

like entity on the other end you would want SGMII, and in this case the variant that uses the MDIO registers for configuration.

The 1000Base-BX was probably closer to what you had envisioned. That is meant to be used for pure SerDes with some sort of transceiver in between. As such it is normally used for configurations such as fiber since it supports standard 1Gb/s Ethernet over fiber.

Really if you were planning to connect to an external switch or PHY that required MDIO configuration you should have gone with SGMII as it is essentially the same as the 1000Base-BX however it includes the configuration bits for managing an external PHY over either I2C or the

interfaces.

The problem is the link mode doesn't tell you anything other than the type of interface being used. So in your case you want to advertise the interface as SGMII because you will want SerDes and to perform

via the external MDIO interface.

Quoted 23 lines

Quoted 23 lines

Quoted 21 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +0 -0

@@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct

e1000_hw
*hw)

         switch (link_mode) {
         case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX:
                 hw->phy.media_type = e1000_media_type_internal_serdes;

drivers/net/ethernet/intel/igb/e1000_82575.c +5 -0

+               if (igb_sgmii_uses_mdio_82575(hw)) {
+                       u32 mdicnfg = rd32(E1000_MDICNFG);
+
+                       mdicnfg &= E1000_MDICNFG_PHY_MASK;
+                       hw->phy.addr = mdicnfg >>

E1000_MDICNFG_PHY_SHIFT;
+ hw_dbg("1000BASE_KX w/ external MDIO device at
0x%x\n",

drivers/net/ethernet/intel/igb/e1000_82575.c +4 -0

+                              hw->phy.addr);
+               } else {
+                       hw_dbg("1000BASE_KX");
+               }
                 break;

The problem is KX has a very specific meaning, and is supposed to be reserved for backplane Ethernet connections between MACs. It makes assumptions about things that could have an impact on link negotiation. Odds are if it is working for you it is because it is "good enough"

it isn't really meant for the type of connection you are using it for.

You might try working things out with Intel to see if you get get either a driver quirk directly added to igb, or possibly have something added to

PCI quirks in the Linux kernel. All that really needs to happen is to update the KX go SGMII at driver load, or during PCI bus probe as the

should be persistent through reboots. In your EEPROM implementation did

do anything that could be used to uniquely identify your device. For example, is there a sub-device or sub-vendor ID registered in the PCI config?

Quoted 6 lines

We have been shipping boards for a year supported by a phylib driver and it would not be easy to field update these because the i210 nvram is only supported via the non-redistributable licensed eepromARMtool (yuck!). One of the key U-Boot developers tried to get Intel to allow him to add source to U-Boot for EEPROM programming and was denied. (again... yuck!).

Yeah, very yuck indeed. Unfortunately you are adding PHY support to a

mode that doesn't support external PHYs. The likelihood of that causing regressions for other implementations is very high. That is why if nothing else it would be better to add a specific quirk for your i210 w/ KX than something that just takes all KX and converts it into something like

Okay, so in the case of your switch the phy_mask applies to probe only

I was thinking of traditional PHYs where there is usually only one address that applies to the PHY belonging to a given MAC.

What you may want to do is look at adding your own igb_mii_write/read functions based off of something like the igb_(write/read)_phy_reg_82580 code. The gs40g has some very specific logic built into it to convert a

register into a page and offset which you likely don't need. The only change you would need from the 82580 functions would be to add a few lines for configuring the MDICNFG address.

I wouldn't bother with restoring the address after you have performed your operation. There is always a possible scenario where something gets hung

crashes in the middle of your operation and that will foul up the MDICNFG register. Instead I would recommend looking at something like modifying igb_reset_mdicnfg_82580 as it might be preferable to have it apply to all hardware 82580 and newer instead of just limiting it to 82580. That way

value is restored on reset or driver load so that as long as the EEPROM is there you will always read the correct value.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +10 -0

+/* Probe the mdio bus for phys and connect them */
+static int igb_enet_mii_probe(struct net_device *netdev)
+{
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       struct phy_device *phy_dev = NULL;
+       int phy_id;
+
+       /* check for attached phy */
+       for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {

Quoted 8 lines

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_82575.c

Quoted 5 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +5 -0

+               if (hw->mii_bus->phy_map[phy_id]) {
+                       phy_dev = hw->mii_bus->phy_map[phy_id];
+                       break;
+               }
+       }

There is code that should have already figured this out in the driver.

Quoted 10 lines

Quoted 3 lines

just need to check the PHY addr after get_invarians has been called, assuming the EEPROM is correct. So you should be able to just get the phy.addr from the e1000_hw struct.

It is true that in igb_enet_mii_init() where the mii bus is registered with phylib that I set the phy_mask to include only the phy address from the eeprom. In my case I put 0x10 in the EEPROM as the phy address as that is the first phy address that the switch responds to and so I use that as a way to detect the phy.

Quoted 2 lines

I can remove the loop and just set phy_dev =
hw->mii_bus->phy_map[hw->phy.addr] in this case.

Yes, that probably would be cleaner.

I really think you would be better off just performing any kind of check

igb_probe. Specifically if the link_mode is SGMII, and there is a PHY,

it is unrecognized then we should be using phylib and should setup an mii_bus. That way it should play well with other implementations such as the stuff the Cumulus guys will need. Otherwise the mii_bus should not be configured. Then all of the other calls can simply check to see if the mii_bus exists and queue off of that for the phylib calls.

The problem with SIOCSMIIREG is that you are basically opening the door

a user-space driver of some sort to manage the PHY, or at least that is

I would assume. The Cumulus guys had a similar patch that they have withdrawn as it was mostly just for debugging. You might want to pull this block out and place it in some sort of separate patch. You would need to justify why you need to expose this functionality.

Quoted 8 lines · On Mon, May 11, 2015 at 1:44 PM, Alexander Duyck <alexander.h.duyck at…

Quoted 7 lines · Tim Harvey

Quoted 4 lines · On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck <alexander.duyck at gmail.com…

Quoted 2 lines · Tim Harvey

If an i210 is configured for 1000BASE-BX link_mode and has an

external

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

phy specified, then register an mii bus using the external phy

address

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

as
a mask.

An i210 hooked to an external standard phy will be configured with a link_mode of SGMII in which case phy ops will be configured and used internal in the igb driver for link status. However, in certain cases one might be using a backplane SerDes connection to something

that talks

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

on the mdio bus but is not a standard phy, such as a switch. In this case
by registering an mdio bus a phy driver can manage the device.

Signed-off-by Tim Harvey <[email protected]

<mailto:[email protected]>>

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

So I think I am staring to see a pattern here between the i210 and the i354
it looks like the EEPROMs for these interfaces are not getting set up correctly. Unfortunately for the i210 you cannot change the

EEPROM from

Quoted 1 line

Quoted 1 line

Quoted 1 line

ethtool so you would need to work out with your manufacturer how

to get

Quoted 18 lines

Quoted 16 lines

Quoted 2 lines

that
fixed for your device.

Alexander,

Thanks for the review!

I think much of my patchset is more clear if I give a bit more background of how we use the i210 on our GW16083
(http://www.gateworks.com/product/item/ventana-gw16083-mezzanine).
This board is an i210 connected to a Marvell MV88E6176 6-port switch via SGMII and MDIO. The MV88E6176 uses MDIO for its control access and instead of having a register interface that utilizes a single phy address, it uses several addresses between 0x10 and 0x1d. The link between the two chips is thus 1gbps full-duplex without the need of link management (why I consider it phy-less).

Quoted 1 line

Okay, so this is likely much more complicated then your original

patch let

Quoted 1 line

on. What you are actually doing is enabling phylib so that you can

enable a

Quoted 1 line

DSA switch on top of the i210 interface, do I have that right?

correct but note that the MV88E6176 data interface to the i210 is SERDES, not SGMII.

Quoted 1 line

<snip>

Quoted 2 lines

The 1000BASE-KX is really meant for pure SerDes configuration where

there

Quoted 1 line

will be no PHY attached at all. So for example if you were going to

connect

Quoted 1 line

directly to another 1000BASE-KX port on either a backplane switch or

another

Quoted 1 line

adapter. It is implied that by using this you aren't going to use

the MDIO

Quoted 3 lines

interface at all. There are essentially 3 modes that do external 1gbs 'phy-less' and the problem is that KX implies a special mode that is typically used for backplane w/o any link negotiation. Since there

is a PHY

Quoted 5 lines

like entity on the other end you would want SGMII, and in this case the variant that uses the MDIO registers for configuration.

The 1000Base-BX was probably closer to what you had envisioned. That is meant to be used for pure SerDes with some sort of transceiver in

between.

Quoted 7 lines

As such it is normally used for configurations such as fiber since it supports standard 1Gb/s Ethernet over fiber.

Really if you were planning to connect to an external switch or PHY that required MDIO configuration you should have gone with SGMII as it is essentially the same as the 1000Base-BX however it includes the configuration bits for managing an external PHY over either I2C or

the MDIO

Quoted 2 lines

interfaces.

<snip>

Quoted 2 lines

The problem is the link mode doesn't tell you anything other than

the type

Quoted 2 lines

of interface being used. So in your case you want to advertise the interface as SGMII because you will want SerDes and to perform

configuration

Quoted 3 lines

via the external MDIO interface.

<snip

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +0 -0

@@ -598,13 +598,26 @@ static s32 igb_get_invariants_82575(struct

e1000_hw
*hw)

         switch (link_mode) {
         case E1000_CTRL_EXT_LINK_MODE_1000BASE_KX:
                 hw->phy.media_type = 

e1000_media_type_internal_serdes;

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +5 -0

+               if (igb_sgmii_uses_mdio_82575(hw)) {
+                       u32 mdicnfg = rd32(E1000_MDICNFG);
+
+                       mdicnfg &= E1000_MDICNFG_PHY_MASK;
+                       hw->phy.addr = mdicnfg >>

E1000_MDICNFG_PHY_SHIFT;
+ hw_dbg("1000BASE_KX w/ external MDIO

device at

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

0x%x\n",

drivers/net/ethernet/intel/igb/e1000_82575.c +4 -0

+  hw->phy.addr);
+               } else {
+                       hw_dbg("1000BASE_KX");
+               }
                 break;

<snip>

Quoted 5 lines

The problem is KX has a very specific meaning, and is supposed to be reserved for backplane Ethernet connections between MACs. It makes assumptions about things that could have an impact on link negotiation. Odds are if it is working for you it is because it is "good enough"

however

Quoted 1 line

it isn't really meant for the type of connection you are using it for.

For clarity, our whole discussion about EEPROM configuration (link-mode, external MDIO, and phy-address) revolves around the patch above.

I initially did try to go with link-mode of SGMII but I find that hw->phy.media_type = e1000_media_type_copper just plain doesn't work for my configuration and I need hw->phy.media_type = e1000_media_type_internal_serdes. That's when I realized that SERDES != RGMII and that the i210 SerDes interface needs to be powered up instead of setting up the internal phy.ops.read/write/reset media-detect functions. Even though the MV88E6176 has a phy-like interface it uses SerDes wired on the PCB instead of link negotiation.

The MAC<->MAC case you describe 1000BASE_KX being intended for is truely what I have but I do want an external MDIO for the purpose of phylib to the switch. I don't know who is currently using 1000BASE_KX but if they are not using external MDIO then my changes should not affect them and the use of MDICNFG.Destination (what igb_sgmii_uses_mdio_82575 uses for i210) makes sense for enabling phylib.

The way I read the use of link_mode in igb_get_invariants_82575 before my patch is:
- 1000BASE_KX: uses internal serdes instead of copper with phy read/write/reset hooks
- SGMII: uses copper with phy read/write/reset hooks and phy negotiation and can be internal or external MDIO

- SERDES - reads SFP info via i2c to determine media type

I still can see the logic in using a return of -E1000_PHY_ERR from get_invariants also as a condition for enabling phylib support and that my be what Johnathan needs for his phy.

Quoted 3 lines

You might try working things out with Intel to see if you get get

either a

Quoted 1 line

driver quirk directly added to igb, or possibly have something added

to the

Quoted 2 lines

PCI quirks in the Linux kernel. All that really needs to happen is to update the KX go SGMII at driver load, or during PCI bus probe as

the value

Quoted 1 line

should be persistent through reboots. In your EEPROM implementation

did you

Quoted 12 lines

do anything that could be used to uniquely identify your device. For example, is there a sub-device or sub-vendor ID registered in the PCI config?

Quoted 6 lines

We have been shipping boards for a year supported by a phylib driver and it would not be easy to field update these because the i210 nvram is only supported via the non-redistributable licensed eepromARMtool (yuck!). One of the key U-Boot developers tried to get Intel to allow him to add source to U-Boot for EEPROM programming and was denied. (again... yuck!).

Quoted 1 line

Yeah, very yuck indeed. Unfortunately you are adding PHY support to

a link

Quoted 2 lines

mode that doesn't support external PHYs. The likelihood of that causing regressions for other implementations is very high. That is why if

nothing

Quoted 2 lines

else it would be better to add a specific quirk for your i210 w/ KX than something that just takes all KX and converts it into something like

SGMII.

Quoted 1 line

Assuming you are able to convince me that my EEPROM/NVRAM config 'is' wrong and we somehow figure out how to make it work with SGMII and external PHY support, I can see either a device-tree binding link-mode property being added that overrides EEPROM/NVRAM or a PCI quirk/fixup working.

Quoted 2 lines

<snip>

Quoted 2 lines

Okay, so in the case of your switch the phy_mask applies to probe

only then.

Quoted 1 line

I was thinking of traditional PHYs where there is usually only one

address

Quoted 5 lines

that applies to the PHY belonging to a given MAC.

What you may want to do is look at adding your own igb_mii_write/read functions based off of something like the igb_(write/read)_phy_reg_82580 code. The gs40g has some very specific logic built into it to

convert a 32b

Quoted 2 lines

register into a page and offset which you likely don't need. The only change you would need from the 82580 functions would be to add a few

lines

Quoted 1 line

for configuring the MDICNFG address.

Yes this is where I need advise from people like you that know the various chipsets supported by igb. I have no idea what is old, what is new, what is fragile and shouldn't be touched. The igb driver is a big mysterious (hate to say mess) of chip identifiers and its difficult to understand exactly what Intel chipsets are supported (would love to see a list/table in Kconfig or source!).

So I patched the generic igb_{read,write}_phy_reg_mdic to accept an addr which are called by both igb_read_phy_reg_82580 and igb_read_phy_reg_gs40g. I have no idea what the gs40g is, but note that igb_{read,write}_phy_reg_gs40g are used for i210/i211 for copper media mode phy negotiation. What enum e1000_mac_type values are capable of MDIO and applicable for phylib? Do some/all of these have a register bit such as the i210 MDICNFG.Destination that indicates external MDIO vs internal that can be used?

What is the difference between an 82580 and 82585 and how do i350/i354 and i210/i211 relate to them?

Quoted 3 lines

I wouldn't bother with restoring the address after you have

performed your

Quoted 1 line

operation. There is always a possible scenario where something gets

hung or

Quoted 1 line

crashes in the middle of your operation and that will foul up the

MDICNFG

Quoted 2 lines

register. Instead I would recommend looking at something like modifying igb_reset_mdicnfg_82580 as it might be preferable to have it apply

to all

Quoted 1 line

hardware 82580 and newer instead of just limiting it to 82580. That

way the

Quoted 1 line

value is restored on reset or driver load so that as long as the

EEPROM is

Quoted 1 line

there you will always read the correct value.

Makes sense, but this would involve the chip information that I'm asking for above - I only know the i210.

When you say '82580 and newer' I don't know what that means. It sounds like your talking about various enum e1000_mac_type values.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +10 -0

+/* Probe the mdio bus for phys and connect them */
+static int igb_enet_mii_probe(struct net_device *netdev)
+{
+       struct igb_adapter *adapter = netdev_priv(netdev);
+       struct e1000_hw *hw = &adapter->hw;
+       struct phy_device *phy_dev = NULL;
+       int phy_id;
+
+       /* check for attached phy */
+       for (phy_id = 0; (phy_id < PHY_MAX_ADDR); phy_id++) {

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/net/ethernet/intel/igb/e1000_82575.c

Quoted 5 lines

drivers/net/ethernet/intel/igb/e1000_82575.c +5 -0

+               if (hw->mii_bus->phy_map[phy_id]) {
+                       phy_dev = hw->mii_bus->phy_map[phy_id];
+                       break;
+               }
+       }

There is code that should have already figured this out in the

driver. We

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

just need to check the PHY addr after get_invarians has been called, assuming the EEPROM is correct. So you should be able to just get the phy.addr from the e1000_hw struct.

It is true that in igb_enet_mii_init() where the mii bus is registered with phylib that I set the phy_mask to include only the phy address from the eeprom. In my case I put 0x10 in the EEPROM as the phy address as that is the first phy address that the switch responds to and so I use that as a way to detect the phy.

Quoted 5 lines

Quoted 2 lines

I can remove the loop and just set phy_dev =
hw->mii_bus->phy_map[hw->phy.addr] in this case.

Yes, that probably would be cleaner.

this is in my pending v2 changes

Quoted 3 lines

<snip>

Quoted 2 lines

I really think you would be better off just performing any kind of

check in

Quoted 1 line

igb_probe. Specifically if the link_mode is SGMII, and there is a

PHY, but

Quoted 2 lines

it is unrecognized then we should be using phylib and should setup an mii_bus. That way it should play well with other implementations

such as

Quoted 1 line

the stuff the Cumulus guys will need. Otherwise the mii_bus should

not be

Quoted 3 lines

configured. Then all of the other calls can simply check to see if the mii_bus exists and queue off of that for the phylib calls.

agreed - this is in my pending v2 changes to make the decision to register the MDIO bus in igb_probe (as a field in struct igb_adapter now) of the and the other places that operate on it just check for a non-null adapter->mii_bus.

Quoted 1 line

<snip>

Quoted 2 lines

The problem with SIOCSMIIREG is that you are basically opening the

door for

Quoted 1 line

a user-space driver of some sort to manage the PHY, or at least that

is what

Quoted 2 lines

I would assume. The Cumulus guys had a similar patch that they have withdrawn as it was mostly just for debugging. You might want to

pull this

Quoted 1 line

block out and place it in some sort of separate patch. You would

need to

Quoted 1 line

justify why you need to expose this functionality.

agreed - I will separate this into a different patch. I think SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace tools like ethtool and others that allow direct mii register access. I'm not clear what the general consusus is but there do seem to be many ethernet drivers that support SIOCMIIREG including the phylib default ioctl handler.

agreed - I will separate this into a different patch. I think SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace tools like ethtool and others that allow direct mii register access. I'm not clear what the general consusus is but there do seem to be many ethernet drivers that support SIOCMIIREG including the phylib default ioctl handler.

Quoted 8 lines · Tim Harvey

agreed - I will separate this into a different patch. I think SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace tools like ethtool and others that allow direct mii register access. I'm not clear what the general consusus is but there do seem to be many ethernet drivers that support SIOCMIIREG including the phylib default ioctl handler.

I am on holiday, will get a chance to read over the thread this weekend.

Regards,
-Jon

Quoted 11 lines · Jonathan Toppins

Quoted 8 lines · Tim Harvey

agreed - I will separate this into a different patch. I think SIOCGMIIREG/SIOCSMIIREG are useful for debugging and various userspace tools like ethtool and others that allow direct mii register access. I'm not clear what the general consusus is but there do seem to be many ethernet drivers that support SIOCMIIREG including the phylib default ioctl handler.

Quoted 4 lines

I am on holiday, will get a chance to read over the thread this weekend.

Regards,
-Jon

Jon,

I will be posting a followup patch very soon (hopefully in the next couple of days) for adding phylib support to igb hopefully addressing Alexander's comments and concerns noted in this thread. My phylib phy driver is an un-posted work-in-progress and because my phy operates likely different from yours, you will need to write a phylib phy driver to test the patch when its posted.

Tim

On 5/20/15 11:46 AM, Tim Harvey wrote:

Thanks for the update Tim. I have not had time to look at this for now but also have not seen your v2 series either, did I miss that?

-Jon

The i210/i211 can use an external phy and there is support built into igb
for several. However if you instead want to use the Linux phydev driver API
the igb driver needs to register an MDIO bus with Linux.

This patch series fixes mdio read/write capability for i210/i211 and registers an MDIO bus if the EEPROM is configured with an external phy.

This has only been tested on an i210 with an external phy as that is all I
have access to. Testing is needed.

Signed-off-by Tim Harvey <[email protected]>

Tim Harvey (3):
net: igb: add i210/i211 support for phy read/write
net: igb: add phy read/write functions that accept phy addr
net: igb: register mii_bus for SerDes w/ external phy

 drivers/net/ethernet/intel/igb/e1000_82575.c |  20 +++-
 drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
 drivers/net/ethernet/intel/igb/e1000_phy.c   | 137
+++++++++++++++++-----
 drivers/net/ethernet/intel/igb/e1000_phy.h   |   6 +-
 drivers/net/ethernet/intel/igb/igb_main.c    | 163
++++++++++++++++++++++++++-
 5 files changed, 297 insertions(+), 36 deletions(-)

Quoted 14 lines · Tim Harvey

The i210/i211 can use an external phy and there is support built into igb
for several. However if you instead want to use the Linux phydev driver API
the igb driver needs to register an MDIO bus with Linux.

This patch series fixes mdio read/write capability for i210/i211 and registers an MDIO bus if the EEPROM is configured with an external phy.

This has only been tested on an i210 with an external phy as that is all I
have access to. Testing is needed.

Quoted 1 line

Signed-off-by Tim Harvey <[email protected]>

Tried to apply your patch series and it does not apply cleanly to my next-queue tree (dev-queue branch). Please update your series to my latest tree and re-submit.

Quoted 13 lines

Tim Harvey (3):
net: igb: add i210/i211 support for phy read/write
net: igb: add phy read/write functions that accept phy addr
net: igb: register mii_bus for SerDes w/ external phy

 drivers/net/ethernet/intel/igb/e1000_82575.c |  20 +++-
 drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
 drivers/net/ethernet/intel/igb/e1000_phy.c   | 137
+++++++++++++++++-----
 drivers/net/ethernet/intel/igb/e1000_phy.h   |   6 +-
 drivers/net/ethernet/intel/igb/igb_main.c    | 163
++++++++++++++++++++++++++-
 5 files changed, 297 insertions(+), 36 deletions(-)

Quoted 2 lines · Jeff Kirsher

Quoted 1 line · Tim Harvey

The i210/i211 can use an external phy and there is support built

into

Quoted 6 lines

Quoted 6 lines

igb
for several. However if you instead want to use the Linux phydev driver API
the igb driver needs to register an MDIO bus with Linux.

This patch series fixes mdio read/write capability for i210/i211

and

Quoted 4 lines

Quoted 4 lines

registers an MDIO bus if the EEPROM is configured with an external phy.

This has only been tested on an i210 with an external phy as that

is

Quoted 24 lines

Quoted 4 lines

all I
have access to. Testing is needed.

Signed-off-by Tim Harvey <[email protected]>

Tried to apply your patch series and it does not apply cleanly to my next-queue tree (dev-queue branch). Please update your series to my latest tree and re-submit.

Quoted 13 lines

Tim Harvey (3):
net: igb: add i210/i211 support for phy read/write
net: igb: add phy read/write functions that accept phy addr
net: igb: register mii_bus for SerDes w/ external phy

 drivers/net/ethernet/intel/igb/e1000_82575.c |  20 +++-
 drivers/net/ethernet/intel/igb/e1000_hw.h    |   7 ++
 drivers/net/ethernet/intel/igb/e1000_phy.c   | 137
+++++++++++++++++-----
 drivers/net/ethernet/intel/igb/e1000_phy.h   |   6 +-
 drivers/net/ethernet/intel/igb/igb_main.c    | 163
++++++++++++++++++++++++++-
 5 files changed, 297 insertions(+), 36 deletions(-)

Jeff,

The first patch was to add i210/i211 support for phy read/write (when using i210/i211 with an external phy) which I should split out from the other two which are to add phylib support to igb. This first patch however is colliding with 'igb: add PHY support for Broadcom 5461S'
[1] which I believe you just nak'd. I guess patch submissions automatically get applied to next-queue/dev-queue? What should I rebase against?