[Intel-wired-lan] [PATCH] igb: Remove superfluous reset to PHY and page 0 selection

[Intel-wired-lan] [PATCH] igb: Remove superfluous reset to PHY and page 0 selection

[Christian]

This patch reverts two previous applied patches to fix an issue
that appeared when using SGMII based SFP modules. In the current
state the driver will try to reset the PHY before obtaining the
phy_addr of the SGMII attached PHY. That leads to an error in
e1000_write_phy_reg_sgmii_82575. Causing the initialization to
fail:

    igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
    igb: Copyright (c) 2007-2014 Intel Corporation.
    igb: probe of ????:??:??.? failed with error -3

The patches being reverted are:

    commit 182785335447957409282ca745aa5bc3968facee
    Author: Aaron Sierra <asierra@xes-inc.com>
    Date:   Tue Nov 29 10:03:56 2016 -0600

        igb: reset the PHY before reading the PHY ID

    commit 440aeca4b9858248d8f16d724d9fa87a4f65fa33
    Author: Matwey V Kornilov <matwey@sai.msu.ru>
    Date:   Thu Nov 24 13:32:48 2016 +0300

         igb: Explicitly select page 0 at initialization

The first reverted patch directly causes the problem mentioned above.
In case of SGMII the phy_addr is not known at this point and will
only be obtained by 'igb_get_phy_id_82575' further down in the code.
The second removed patch selects forces selection of page 0 in the
PHY. Something that the reset tries to address as well.

As pointed out by Alexander Duzck, the patch below fixes the same
issue but in the proper location:

    commit 4e684f59d760a2c7c716bb60190783546e2d08a1
    Author: Chris J Arges <christopherarges@gmail.com>
    Date:   Wed Nov 2 09:13:42 2016 -0500

        igb: Workaround for igb i210 firmware issue

Reverts: 440aeca4b9858248d8f16d724d9fa87a4f65fa33.
Reverts: 182785335447957409282ca745aa5bc3968facee.

Signed-off-by: Christian Gr?nke <c.groenke@infodas.de>
---
 e1000_82575.c |   12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index b13b42e..a795c07 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -225,19 +225,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
 	hw->bus.func = (rd32(E1000_STATUS) & E1000_STATUS_FUNC_MASK) >>
 			E1000_STATUS_FUNC_SHIFT;
 
-	/* Make sure the PHY is in a good state. Several people have reported
-	 * firmware leaving the PHY's page select register set to something
-	 * other than the default of zero, which causes the PHY ID read to
-	 * access something other than the intended register.
-	 */
-	ret_val = hw->phy.ops.reset(hw);
-	if (ret_val) {
-		hw_dbg("Error resetting the PHY.\n");
-		goto out;
-	}
-
 	/* Set phy->phy_addr and phy->id. */
-	igb_write_phy_reg_82580(hw, I347AT4_PAGE_SELECT, 0);
 	ret_val = igb_get_phy_id_82575(hw);
 	if (ret_val)
 		return ret_val;

[Intel-wired-lan] [PATCH] igb: Remove superfluous reset to PHY and page 0 selection

[Alexander Duyck]

On Tue, Jun 26, 2018 at 3:12 AM, Gr?nke, Christian <C.Groenke@infodas.de> wrote:
> This patch reverts two previous applied patches to fix an issue
> that appeared when using SGMII based SFP modules. In the current
> state the driver will try to reset the PHY before obtaining the
> phy_addr of the SGMII attached PHY. That leads to an error in
> e1000_write_phy_reg_sgmii_82575. Causing the initialization to
> fail:
>
>     igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
>     igb: Copyright (c) 2007-2014 Intel Corporation.
>     igb: probe of ????:??:??.? failed with error -3
>
> The patches being reverted are:
>
>     commit 182785335447957409282ca745aa5bc3968facee
>     Author: Aaron Sierra <asierra@xes-inc.com>
>     Date:   Tue Nov 29 10:03:56 2016 -0600
>
>         igb: reset the PHY before reading the PHY ID
>
>     commit 440aeca4b9858248d8f16d724d9fa87a4f65fa33
>     Author: Matwey V Kornilov <matwey@sai.msu.ru>
>     Date:   Thu Nov 24 13:32:48 2016 +0300
>
>          igb: Explicitly select page 0 at initialization
>
> The first reverted patch directly causes the problem mentioned above.
> In case of SGMII the phy_addr is not known at this point and will
> only be obtained by 'igb_get_phy_id_82575' further down in the code.
> The second removed patch selects forces selection of page 0 in the
> PHY. Something that the reset tries to address as well.
>
> As pointed out by Alexander Duzck, the patch below fixes the same
> issue but in the proper location:
>
>     commit 4e684f59d760a2c7c716bb60190783546e2d08a1
>     Author: Chris J Arges <christopherarges@gmail.com>
>     Date:   Wed Nov 2 09:13:42 2016 -0500
>
>         igb: Workaround for igb i210 firmware issue
>
> Reverts: 440aeca4b9858248d8f16d724d9fa87a4f65fa33.
> Reverts: 182785335447957409282ca745aa5bc3968facee.
>
> Signed-off-by: Christian Gr?nke <c.groenke@infodas.de>

Looks good to me.

Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>

> ---
>  e1000_82575.c |   12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
> index b13b42e..a795c07 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> @@ -225,19 +225,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
>         hw->bus.func = (rd32(E1000_STATUS) & E1000_STATUS_FUNC_MASK) >>
>                         E1000_STATUS_FUNC_SHIFT;
>
> -       /* Make sure the PHY is in a good state. Several people have reported
> -        * firmware leaving the PHY's page select register set to something
> -        * other than the default of zero, which causes the PHY ID read to
> -        * access something other than the intended register.
> -        */
> -       ret_val = hw->phy.ops.reset(hw);
> -       if (ret_val) {
> -               hw_dbg("Error resetting the PHY.\n");
> -               goto out;
> -       }
> -
>         /* Set phy->phy_addr and phy->id. */
> -       igb_write_phy_reg_82580(hw, I347AT4_PAGE_SELECT, 0);
>         ret_val = igb_get_phy_id_82575(hw);
>         if (ret_val)
>                 return ret_val;

[Intel-wired-lan] [PATCH] igb: Remove superfluous reset to PHY and page 0 selection

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf
> Of Gr?nke, Christian
> Sent: Tuesday, June 26, 2018 3:12 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Chris J Arges <christopherarges@gmail.com>
> Subject: [Intel-wired-lan] [PATCH] igb: Remove superfluous reset to PHY and
> page 0 selection
> 
> This patch reverts two previous applied patches to fix an issue
> that appeared when using SGMII based SFP modules. In the current
> state the driver will try to reset the PHY before obtaining the
> phy_addr of the SGMII attached PHY. That leads to an error in
> e1000_write_phy_reg_sgmii_82575. Causing the initialization to
> fail:
> 
>     igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
>     igb: Copyright (c) 2007-2014 Intel Corporation.
>     igb: probe of ????:??:??.? failed with error -3
> 
> The patches being reverted are:
> 
>     commit 182785335447957409282ca745aa5bc3968facee
>     Author: Aaron Sierra <asierra@xes-inc.com>
>     Date:   Tue Nov 29 10:03:56 2016 -0600
> 
>         igb: reset the PHY before reading the PHY ID
> 
>     commit 440aeca4b9858248d8f16d724d9fa87a4f65fa33
>     Author: Matwey V Kornilov <matwey@sai.msu.ru>
>     Date:   Thu Nov 24 13:32:48 2016 +0300
> 
>          igb: Explicitly select page 0 at initialization
> 
> The first reverted patch directly causes the problem mentioned above.
> In case of SGMII the phy_addr is not known at this point and will
> only be obtained by 'igb_get_phy_id_82575' further down in the code.
> The second removed patch selects forces selection of page 0 in the
> PHY. Something that the reset tries to address as well.
> 
> As pointed out by Alexander Duzck, the patch below fixes the same
> issue but in the proper location:
> 
>     commit 4e684f59d760a2c7c716bb60190783546e2d08a1
>     Author: Chris J Arges <christopherarges@gmail.com>
>     Date:   Wed Nov 2 09:13:42 2016 -0500
> 
>         igb: Workaround for igb i210 firmware issue
> 
> Reverts: 440aeca4b9858248d8f16d724d9fa87a4f65fa33.
> Reverts: 182785335447957409282ca745aa5bc3968facee.
> 
> Signed-off-by: Christian Gr?nke <c.groenke@infodas.de>
> ---
>  e1000_82575.c |   12 ------------
>  1 file changed, 12 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>