[Intel-wired-lan] [PATCH 0/3] igb: add i210/i211 external phy support

[Intel-wired-lan] [PATCH 0/3] igb: add i210/i211 external phy support

[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.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>

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(-)

-- 
1.9.1

[Intel-wired-lan] [PATCH 1/3] net: igb: add i210/i211 support for phy read/write

[Tim Harvey]

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

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c | 71 ++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index c1bb64d..2307ac6 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -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) |
+			(phy->addr << E1000_MDIC_PHY_SHIFT) |
+			(E1000_MDIC_OP_READ));
+		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
@@ -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) {
+	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 = (((u32)data) |
+			(offset << E1000_MDIC_REG_SHIFT) |
+			(E1000_MDIC_OP_WRITE));
+		break;
+	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
@@ -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:
+		break;
+	}
 	return ret_val;
 }
 
-- 
1.9.1

[Intel-wired-lan] [PATCH 1/3] net: igb: add i210/i211 support for phy read/write

[Alexander Duyck]

On 04/30/2015 11:19 AM, Tim Harvey wrote:
> The i210/i211 uses the MDICNFG register for the phy address instead
> of the MDIC register.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

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.

> ---
>   drivers/net/ethernet/intel/igb/e1000_phy.c | 71 ++++++++++++++++++++++++++----
>   1 file changed, 62 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
> index c1bb64d..2307ac6 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -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) |
> +			(phy->addr << E1000_MDIC_PHY_SHIFT) |
> +			(E1000_MDIC_OP_READ));
> +		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

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.

> @@ -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) {
> +	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 = (((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.

> +	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.

> @@ -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:
> +		break;
> +	}
>   	return ret_val;
>   }
>
>

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

[Intel-wired-lan] [PATCH 1/3] net: igb: add i210/i211 support for phy read/write

[Tim Harvey]

On Fri, May 8, 2015 at 6:06 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>
>> The i210/i211 uses the MDICNFG register for the phy address instead
>> of the MDIC register.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
>
> 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).

>
>> ---
>>   drivers/net/ethernet/intel/igb/e1000_phy.c | 71
>> ++++++++++++++++++++++++++----
>>   1 file changed, 62 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c
>> b/drivers/net/ethernet/intel/igb/e1000_phy.c
>> index c1bb64d..2307ac6 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
>> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
>> @@ -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) |
>> +                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
>> +                       (E1000_MDIC_OP_READ));
>> +               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
>
>
> 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.

>
>
>> @@ -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) {
>> +       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 = (((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.

>
>> +       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.
>
>> @@ -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:
>> +               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

[Intel-wired-lan] [PATCH 1/3] net: igb: add i210/i211 support for phy read/write

[Alexander Duyck]

On 05/11/2015 08:26 AM, Tim Harvey wrote:
> On Fri, May 8, 2015 at 6:06 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>> The i210/i211 uses the MDICNFG register for the phy address instead
>>> of the MDIC register.
>>>
>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>
>> 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).

I think you might be interpreting things a bit to literally.  If you are 
passing only one phy address into the PHY mask which I believe you are 
via the ~(1 << phy.addr) call in your third patch there isn't much point 
in writing the PHY address since you are already locked the phylib into 
only supporting one PHY address.

>>> ---
>>>    drivers/net/ethernet/intel/igb/e1000_phy.c | 71
>>> ++++++++++++++++++++++++++----
>>>    1 file changed, 62 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c
>>> b/drivers/net/ethernet/intel/igb/e1000_phy.c
>>> index c1bb64d..2307ac6 100644
>>> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
>>> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
>>> @@ -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) |
>>> +                       (phy->addr << E1000_MDIC_PHY_SHIFT) |
>>> +                       (E1000_MDIC_OP_READ));
>>> +               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
>>
>> 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.

This code was kind of an undocumented hack.  Basically with the phy addr 
bits of the mdic register being reserved zero it didn't actually hurt 
things to write to them as the hardware has ignored them since the 82850 
if I recall correctly.

>>
>>> @@ -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) {
>>> +       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 = (((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.

Yes, int this case the "should" is not a must, and writing something to 
it has no effect.  That is why the legacy code was left in place to 
write to the register.

> 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.

The fact is this was how it was done before you modified the code. If 
anything changing it at this point risks introducing regressions.  If 
you look in the documentation the MDICNFG register has been around since 
the 82580.  There was one errata against the 82580 that required 
rewriting the register after reset, but the implementations of hardware 
since then have wanted to leave this register value static.

>>> +       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.
>>
>>> @@ -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:
>>> +               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

What I was trying to get at is in the 3rd patch you pretty much locked 
down the PHY address by setting a mask that excludes everything but the 
address contained in the MDICNFG register.  So instead of rewriting the 
register every time you access the PHY you could probably just skip 
these first two patches, and ignore the PHY address passed to you from 
phylib.

- Alex

[Intel-wired-lan] [PATCH 2/3] net: igb: add phy read/write functions that accept phy addr

[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 <tharvey@gateworks.com>
---
 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(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 0f69ef8..d2afd7b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -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);
 
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 2307ac6..66c2a09 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -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@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
  *
diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
index 7af4ffa..6256e76 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.h
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
@@ -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 */
-- 
1.9.1

[Intel-wired-lan] [PATCH 2/3] net: igb: add phy read/write functions that accept phy addr

[Alexander Duyck]

On 04/30/2015 11:19 AM, Tim Harvey wrote:
> 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 <tharvey@gateworks.com>

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

> ---
>   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(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index 0f69ef8..d2afd7b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -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);
>   
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
> index 2307ac6..66c2a09 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
> @@ -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
>    *
> diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.h b/drivers/net/ethernet/intel/igb/e1000_phy.h
> index 7af4ffa..6256e76 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_phy.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_phy.h
> @@ -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 */

[Intel-wired-lan] [PATCH 2/3] net: igb: add phy read/write functions that accept phy addr

[Tim Harvey]

On Fri, May 8, 2015 at 6:07 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>
>> 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 <tharvey@gateworks.com>
>
>
> 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

[Intel-wired-lan] [PATCH 2/3] net: igb: add phy read/write functions that accept phy addr

[Alexander Duyck]

On 05/11/2015 08:27 AM, Tim Harvey wrote:
> On Fri, May 8, 2015 at 6:07 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>> 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 <tharvey@gateworks.com>
>>
>> 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

Tim,

You only require PHY address support if you support multiple addresses.  
From what I can tell you don't.  You lock it down to only one address 
via the mask field when you are initializing the phylib structures.

- Alex

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[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.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 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(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index d2afd7b..e80617b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -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);
+
+			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;
 	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");
 		}
 		/* fall through for I2C based SGMII */
 	case E1000_CTRL_EXT_LINK_MODE_PCIE_SERDES:
@@ -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");
 		}
 
 		/* do not change link mode for 100BaseFX */
diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 2003b37..2864779 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -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;
 
+#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;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f366b3b..9b5f538 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -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)
+		return err;
+	return out;
+}
+
+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);
+}
+
+static int igb_enet_mdio_reset(struct mii_bus *bus)
+{
+	usleep_range(1000, 2000);
+	return 0;
+}
+
+static void igb_enet_mii_link(struct net_device *netdev)
+{
+}
+
+/* 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;
+		}
+	}
+	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);
+
+#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;
 
 err_register:
@@ -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
@@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, bool resuming)
 	if (!resuming)
 		pm_runtime_put(&pdev->dev);
 
+#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);
@@ -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))
+				return -EIO;
+		} else {
+			if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
+					      data->val_in))
+				return -EIO;
+		}
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
1.9.1

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Alexander Duyck]

On 04/30/2015 11:19 AM, Tim Harvey wrote:
> 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 <tharvey@gateworks.com>


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.

> ---
>   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(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index d2afd7b..e80617b 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -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);
> +
> +			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.

>   	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");
>   		}
>   		/* 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.

> @@ -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");
>   		}
>
>   		/* do not change link mode for 100BaseFX */

Same for these.

> diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
> index 2003b37..2864779 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
> @@ -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;
>
> +#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.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index f366b3b..9b5f538 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -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)
> +		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..?

> +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.

> +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.

> +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.

> +/* 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;
> +		}
> +	}

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.

> +	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);
> +
> +#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;
>
>   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.

> @@ -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.

> @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev, bool resuming)
>   	if (!resuming)
>   		pm_runtime_put(&pdev->dev);
>
> +#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.

> @@ -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))
> +				return -EIO;
> +		} else {
> +			if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
> +					      data->val_in))
> +				return -EIO;
> +		}
> +		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.

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Tim Harvey]

On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>
>> 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 <tharvey@gateworks.com>
>
>
>
> 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'.

>
> 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.

>
> 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.

sure - I saw your comment about getting rid of the igb_enet notation
and I can do this in a next-rev patch.

>
>> ---
>>   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(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c
>> b/drivers/net/ethernet/intel/igb/e1000_82575.c
>> index d2afd7b..e80617b 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
>> @@ -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);
>> +
>> +                       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.

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!).

>
> 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.
>
>>         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");
>>                 }
>>                 /* 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.
>
>> @@ -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");
>>                 }
>>
>>                 /* do not change link mode for 100BaseFX */
>
>
> Same for these.

ok

>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h
>> b/drivers/net/ethernet/intel/igb/e1000_hw.h
>> index 2003b37..2864779 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_hw.h
>> +++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
>> @@ -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;
>>
>> +#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.

ok

>
>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>> b/drivers/net/ethernet/intel/igb/igb_main.c
>> index f366b3b..9b5f538 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -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)
>> +               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..?

sure, I can rename them.

>
>> +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.

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).

>
>> +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.

I can drop this - I think I may have been trying to meet some delay
spec after reset but since as you say this doesn't actually reset
something it shouldn't go here anyway.

>
>> +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.

I'm not sure either. Perhaps someone with more knowledge of phylib can comment.

>
>> +/* 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;
>> +               }
>> +       }
>
>
> 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.

>
>
>> +       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);
>> +
>> +#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;
>>
>>   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.
>
>> @@ -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.
>
>> @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev,
>> bool resuming)
>>         if (!resuming)
>>                 pm_runtime_put(&pdev->dev);
>>
>> +#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


>
>> @@ -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))
>> +                               return -EIO;
>> +               } else {
>> +                       if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
>> +                                             data->val_in))
>> +                               return -EIO;
>> +               }
>> +               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

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Alexander Duyck]

On 05/11/2015 11:42 AM, Tim Harvey wrote:
> On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>> 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 <tharvey@gateworks.com>
>>
>>
>> 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).

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 
enable a DSA switch on top of the i210 interface, do I have that right?

> 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.

It looks like you are learning the same lesson many have when it comes 
to this kind of stuff.  Upstream first is usually a much better policy 
because then you figure out all of these issues much earlier and you 
aren't stuck supporting something that has implementation issues you 
weren't aware of.

> 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'.

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 connect directly to another 1000BASE-KX port on either a backplane 
switch or another adapter.  It is implied that by using this you aren't 
going to use the MDIO 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 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 
MDIO interfaces.

>> 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.

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 
configuration via the external MDIO interface.

>>> ---
>>>    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(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c
>>> b/drivers/net/ethernet/intel/igb/e1000_82575.c
>>> index d2afd7b..e80617b 100644
>>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
>>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
>>> @@ -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);
>>> +
>>> +                       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.
> 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).

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 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 the 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 should be persistent through reboots.  In your EEPROM 
implementation did you 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?

> 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 
link 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 SGMII.

>>> +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.
> 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).

Okay, so in the case of your switch the phy_mask applies to probe only 
then.  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 
32b 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 or 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 the value is restored on reset or driver load so that 
as long as the EEPROM is there you will always read the correct value.

>>> +/* 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;
>>> +               }
>>> +       }
>>
>> 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.

Yes, that probably would be cleaner.

>
>>
>>> +       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);
>>> +
>>> +#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;
>>>
>>>    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.
>>
>>> @@ -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.
>>
>>> @@ -3093,6 +3220,12 @@ static int __igb_open(struct net_device *netdev,
>>> bool resuming)
>>>          if (!resuming)
>>>                  pm_runtime_put(&pdev->dev);
>>>
>>> +#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

I really think you would be better off just performing any kind of check 
in igb_probe.  Specifically if the link_mode is SGMII, and there is a 
PHY, but 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.

>>> @@ -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))
>>> +                               return -EIO;
>>> +               } else {
>>> +                       if (igb_write_phy_reg(hw, data->reg_num & 0x1F,
>>> +                                             data->val_in))
>>> +                               return -EIO;
>>> +               }
>>> +               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

The problem with SIOCSMIIREG is that you are basically opening the door 
for a user-space driver of some sort to manage the PHY, or at least that 
is what 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.

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Tim Harvey]

On Mon, May 11, 2015 at 1:44 PM, Alexander Duyck <
alexander.h.duyck@redhat.com> wrote:
> On 05/11/2015 11:42 AM, Tim Harvey wrote:
>>
>> On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>>
>>> On 04/30/2015 11:19 AM, Tim Harvey wrote:
>>>>
>>>> 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 <tharvey@gateworks.com>
>>>
>>>
>>>
>>> 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).
>
>
> 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
enable a
> 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.

>
<snip>
>
> 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
connect
> directly to another 1000BASE-KX port on either a backplane switch or
another
> adapter.  It is implied that by using this you aren't going to use the
MDIO
> 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
> 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
MDIO
> interfaces.
>
<snip>
>
> 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
configuration
> via the external MDIO interface.
>
>
<snip
>>>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
>>>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
>>>> @@ -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);
>>>> +
>>>> +                       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;
>>>
>>>
<snip>
>
> 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
> 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.

>
> 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
the
> 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
> should be persistent through reboots.  In your EEPROM implementation did
you
> 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?
>
>> 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
link
> 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
SGMII.
>

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.

>
<snip>
>
> Okay, so in the case of your switch the phy_mask applies to probe only
then.
> 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
32b
> 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.

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?

>
> I wouldn't bother with restoring the address after you have performed your
> operation.  There is always a possible scenario where something gets hung
or
> 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
the
> value is restored on reset or driver load so that as long as the EEPROM is
> 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.

>
>
>>>> +/* 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;
>>>> +               }
>>>> +       }
>>>
>>>
>>> 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.
>
>
> Yes, that probably would be cleaner.

this is in my pending v2 changes

>
>
<snip>
>
> I really think you would be better off just performing any kind of check
in
> igb_probe.  Specifically if the link_mode is SGMII, and there is a PHY,
but
> 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.
>

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.

>
<snip>
>
> The problem with SIOCSMIIREG is that you are basically opening the door
for
> a user-space driver of some sort to manage the PHY, or at least that is
what
> 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.

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.

Regards,

Tim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150512/8c1f0dc0/attachment-0001.html>

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Alexander Duyck]

On 05/12/2015 03:37 PM, Tim Harvey wrote:
> On Mon, May 11, 2015 at 1:44 PM, Alexander Duyck 
> <alexander.h.duyck at redhat.com <mailto:alexander.h.duyck@redhat.com>> 
> wrote:
> > On 05/11/2015 11:42 AM, Tim Harvey wrote:
> >>
> >> On Fri, May 8, 2015 at 6:05 PM, Alexander Duyck
> >> <alexander.duyck at gmail.com <mailto:alexander.duyck@gmail.com>> wrote:
> >>>
> >>> On 04/30/2015 11:19 AM, Tim Harvey wrote:
> >>>>
> >>>> 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 <tharvey@gateworks.com 
> <mailto:tharvey@gateworks.com>>
> >>>
> >>>
> >>>
> >>> 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).
> >
> >
> > 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 
> enable a
> > 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.
>

It is essentially the same thing.  Either of those two modes should work 
if I recall correctly.  The only problem is the expectations on the PHY 
in the case of SGMII.

> >
> <snip>
> >
> > 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 
> connect
> > directly to another 1000BASE-KX port on either a backplane switch or 
> another
> > adapter.  It is implied that by using this you aren't going to use 
> the MDIO
> > 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
> > 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 MDIO
> > interfaces.
> >
> <snip>
> >
> > 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 
> configuration
> > via the external MDIO interface.
> >
> >
> <snip
> >>>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> >>>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> >>>> @@ -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);
> >>>> +
> >>>> +                       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;
> >>>
> >>>
> <snip>
> >
> > 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
> > 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.

Yes.  KX has a very specific meaning in terms of what is present on the 
adapter.

> 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.

So does SGMII.  SGMII is essentially just GMII over SerDes.  If you look 
at the code for SGMII it is going through and powering on the SerDes 
link and then trying to send commands across the MII interface to the 
PHY, if that fails it tries to get the info over I2C.

The problems you ran into are likely due to the fact that the switch was 
being treated as an unsupported PHY.  From what I can tell there would 
be a couple of spots that you need to skip over code if you have an 
"unknown phy" that is being handled by phylib.

>
> 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 problem is what you have isn't MAC<->MAC.  If it was we wouldn't 
need the MDIO interface and that is where things get messy.  It isn't 
clearly documented what the behavior is if you try to read I2C/MDIO pins 
that aren't connected to anything and in the KX case that is exactly 
what we could be looking at.  The other problem is we have no way to 
verify what bits are or are not set in any of the number of KX 
interfaces out there.  This could very well introduce some very odd  
issues if floating pins are read/written, or unused EEPROM bits are now 
read.

>
> 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

SGMII uses SerDes as well.  So for all intents and purposes it is SerDes 
that includes supports reading a PHY over either I2C or MDIO.

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

This probably would have been a better match for your setup than the KX, 
the difference between the two is that SerDes is technically 1000Base-BX 
which is your standard 1Gb/s over SerDes, and you are expected to have 
the I2C pins connected to something either MDIO or I2C based.

>
> 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.

Yes you would probably need to return something like 
-E1000_ERR_UNKNOWN_PHY (error value TBD) and then you might also need to 
modify the code in a few spots so that if the phy_type values is 
phy_unknown you don't poke any of the registers and just leave it to 
phylib.  Either that or you could have the media type changed back to 
serdes from copper when you detect an unknown PHY.

We will also should audit the driver code to clean out all of the spots 
that think that the only external PHY supported is a Marvell such as 
igb_phy_hw_reset_sgmii_82575().

>
> >
> > 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 the
> > 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
> > should be persistent through reboots.  In your EEPROM implementation 
> did you
> > 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?
> >
> >> 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 link
> > 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 
> SGMII.
> >
>
> 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.

That works.  All you really need is something that will overwrite the 
link mode bits in the CTRL_EXT register before the driver is loaded.  If 
I recall those bits are not cleared in reset so you would only need to 
do it once.

The problem is we have to make multiple scenarios work with the phylib 
logic and SGMII would be the preferred type to have reported since you 
aren't using I2C.  As I said though if we can workaround it in software 
it would be best to have something that makes it clear this is a quirk 
so it doesn't become the way things are done as otherwise it will just 
make stuff messier.

>
> >
> <snip>
> >
> > Okay, so in the case of your switch the phy_mask applies to probe 
> only then.
> > 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 32b
> > 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.
>
> 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?

The 82580 is basically the head of family for the i350/i354/i210/i211 
parts.  The reason why I suggested using it is because it is a 
simplified version of things if I recall correctly as the PHY only had 
the standard registers and didn't go into the pages like the other parts 
did to support things like energy efficient Ethernet.  The extra 
overhead for the other read/write_phy calls is from the fact that they 
encoded the page number into the PHY offsets and had to update the page 
number on each read/write to a register to make sure they accessed the 
correct page in the PHY.

>
> >
> > I wouldn't bother with restoring the address after you have 
> performed your
> > operation.  There is always a possible scenario where something gets 
> hung or
> > 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 the
> > value is restored on reset or driver load so that as long as the 
> EEPROM is
> > 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.

Well I worked on the 82580 and i350 back in the day.  The i350/i354 
should be able to take the same workaround with no side effects. All you 
are doing is restoring bits that would have been written from the EEPROM 
at power on.

Although I just realized that the workaround doesn't cover the PHY 
address itself.  For that you would have to read NVM_INIT_CTRL_4. The 
register layout is defined in the i210 data sheet, and it should be the 
same for every part since the 82580.

>
> 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.
>

Yes, if you look into igb_reset_mdicnfg_82580 there is a bit of logic 
that says hw->mac.type == e1000_mac_82580, what I am suggesting is you 
replace the == with >=.

> >
> >
> >>>> +/* 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;
> >>>> +               }
> >>>> +       }
> >>>
> >>>
> >>> 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.
> >
> >
> > Yes, that probably would be cleaner.
>
> this is in my pending v2 changes

Okay.

>
> >
> >
> <snip>
> >
> > I really think you would be better off just performing any kind of 
> check in
> > igb_probe.  Specifically if the link_mode is SGMII, and there is a 
> PHY, but
> > 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.
> >
>
> 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.
>

Okay.

> >
> <snip>
> >
> > The problem with SIOCSMIIREG is that you are basically opening the 
> door for
> > a user-space driver of some sort to manage the PHY, or at least that 
> is what
> > 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.
>
> 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.

My advice would be to leave the SIOCGMIIREG as is but add the ability to 
read any phy address/offset pair for e1000_phy_unknown. I would say you 
could probably make SIOCSMIIREG only work w/ e1000_phy_unknown, as that 
way you allow managing a PHY that cannot be managed by the driver.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150512/4f128de5/attachment-0001.html>

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Jonathan Toppins]

On 5/12/15 6:37 PM, Tim Harvey wrote:
>
> 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

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Tim Harvey]

On Thu, May 14, 2015 at 9:08 PM, Jonathan Toppins
<jtoppins@cumulusnetworks.com> wrote:
> On 5/12/15 6:37 PM, Tim Harvey wrote:
>>
>>
>> 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

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

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Jonathan Toppins]

On 5/20/15 11:46 AM, Tim Harvey wrote:
> On Thu, May 14, 2015 at 9:08 PM, Jonathan Toppins
> <jtoppins@cumulusnetworks.com> wrote:
>> On 5/12/15 6:37 PM, Tim Harvey wrote:
>>>
>>>
>>> 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
>
> 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
>

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

[Intel-wired-lan] [PATCH 3/3] net: igb: register mii_bus for SerDes w/ external phy

[Tim Harvey]

On Fri, May 29, 2015 at 8:26 AM, Jonathan Toppins
<jtoppins@cumulusnetworks.com> wrote:
> On 5/20/15 11:46 AM, Tim Harvey wrote:
<snip>
>
> 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

Jon,

You haven't missed it - I've gotten pulled off onto some other things
and haven't gotten back to it. Where it stands right now I have a
patch that should be completely 'safe' and eliminate any backwards
compatibility concerns however I'm not convinced I'm skipping enough
phy related code when phylib mode is used. Arguably those could come
later as issues are found with phylib phy drivers, but I feel like if
I can just get another half day into it I could possibly work out
those issues as well.

Tim

[Intel-wired-lan] [PATCH 0/3] igb: add i210/i211 external phy support

[Jeff Kirsher]

On Thu, 2015-04-30 at 11:19 -0700, Tim Harvey wrote:
> 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 <tharvey@gateworks.com>

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.

> 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(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150504/50ff339a/attachment.asc>

[Intel-wired-lan] [PATCH 0/3] igb: add i210/i211 external phy support

[Tim Harvey]

On Mon, May 4, 2015 at 7:00 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Thu, 2015-04-30 at 11:19 -0700, Tim Harvey wrote:
>> 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 <tharvey@gateworks.com>
>
> 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.
>
>> 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?

Regards,

Tim

[1] http://patchwork.ozlabs.org/patch/462223/

[Intel-wired-lan] [PATCH 0/3] igb: add i210/i211 external phy support

[Jeff Kirsher]

On Thu, 2015-05-07 at 09:40 -0700, Tim Harvey wrote:
> On Mon, May 4, 2015 at 7:00 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > On Thu, 2015-04-30 at 11:19 -0700, Tim Harvey wrote:
> >> 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 <tharvey@gateworks.com>
> >
> > 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.
> >
> >> 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?

I applied it so that Aaron could do regression testing on it, while we
were reviewing the changes.  I popped off that last series from
Jonathan, so you should be able to rebase your patches on my dev-queue
branch now.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150507/6d9f8f7f/attachment.asc>