diff mbox series

[iwl-net,v1,1/1] e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue

Message ID 20240211151245.811320-1-vitaly.lifshits@intel.com
State Changes Requested
Headers show
Series [iwl-net,v1,1/1] e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue | expand

Commit Message

Vitaly Lifshits Feb. 11, 2024, 3:12 p.m. UTC
Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on
some systems.
It is suspected to be caused by initiating PHY transactions before the
the interface settles.

Separating this configuration from the ULP enabling flow avoids this
issue.

Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in Sx state")
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 -------------------
 drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 19 deletions(-)

Comments

Paul Menzel Feb. 12, 2024, 9 a.m. UTC | #1
Dear Vitaly,


Thank you for your patch.

Am 11.02.24 um 16:12 schrieb Vitaly Lifshits:
> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on
> some systems.

Please list the systems you know of.

> It is suspected to be caused by initiating PHY transactions before the
> the interface settles.

No new paragraph is needed here. Please do not break the line, just 
because a sentence ends. (If you use paragraphs, please separate them by 
a blank line.

> Separating this configuration from the ULP enabling flow avoids this
> issue.
> 
> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in Sx state")
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> ---
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 -------------------
>   drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++++++++++++
>   2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 717c52913e84..4d83c9a0c023 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
>   	if (ret_val)
>   		goto out;
>   
> -	/* Switching PHY interface always returns MDI error
> -	 * so disable retry mechanism to avoid wasting time
> -	 */
> -	e1000e_disable_phy_retry(hw);
> -
> -	/* Force SMBus mode in PHY */
> -	ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg);
> -	if (ret_val)
> -		goto release;
> -	phy_reg |= CV_SMB_CTRL_FORCE_SMBUS;
> -	e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg);
> -
> -	e1000e_enable_phy_retry(hw);
> -
> -	/* Force SMBus mode in MAC */
> -	mac_reg = er32(CTRL_EXT);
> -	mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS;
> -	ew32(CTRL_EXT, mac_reg);
> -
>   	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>   	 * LPLU and disable Gig speed when entering ULP
>   	 */
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index af5d9d97a0d6..8fcf8f11f5a4 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   	struct e1000_adapter *adapter = netdev_priv(netdev);
>   	struct e1000_hw *hw = &adapter->hw;
>   	u32 ctrl, ctrl_ext, rctl, status, wufc;
> +	u16 smb_ctrl;
>   	int retval = 0;
>   
>   	/* Runtime suspend should only enable wakeup for link changes */
> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   
>   		if (retval)
>   			return retval;
> +
> +		/* Force SMBUS to allow WOL */
> +		/* Switching PHY interface always returns MDI error
> +		 * so disable retry mechanism to avoid wasting time
> +		 */
> +		e1000e_disable_phy_retry(hw);
> +
> +		e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl);
> +		smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
> +		e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl);
> +
> +		e1000e_enable_phy_retry(hw);
> +
> +		/* Force SMBus mode in MAC */
> +		ctrl_ext = er32(CTRL_EXT);
> +		ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
> +		ew32(CTRL_EXT, ctrl_ext);
>   	}
>   
>   	/* Ensure that the appropriate bits are set in LPI_CTRL

Why is `__e1000_shutdown()` the correct place, where it was in 
`e1000_enable_ulp_lpt_lp()` before?


Kind regards,

Paul
Vitaly Lifshits Feb. 29, 2024, 10:34 a.m. UTC | #2
On 2/12/2024 11:00 AM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for your patch.
> 
> Am 11.02.24 um 16:12 schrieb Vitaly Lifshits:
>> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on
>> some systems.
> 
> Please list the systems you know of.
On some Meteor-lake systems, we also suspect that on some legacy 
platforms this issue happened with some low probability.
> 
>> It is suspected to be caused by initiating PHY transactions before the
>> the interface settles.
> 
> No new paragraph is needed here. Please do not break the line, just 
> because a sentence ends. (If you use paragraphs, please separate them by 
> a blank line.
Will be fixed in a v2.
> 
>> Separating this configuration from the ULP enabling flow avoids this
>> issue.
>>
>> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in 
>> Sx state")
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 -------------------
>>   drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++++++++++++
>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index 717c52913e84..4d83c9a0c023 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw 
>> *hw, bool to_sx)
>>       if (ret_val)
>>           goto out;
>> -    /* Switching PHY interface always returns MDI error
>> -     * so disable retry mechanism to avoid wasting time
>> -     */
>> -    e1000e_disable_phy_retry(hw);
>> -
>> -    /* Force SMBus mode in PHY */
>> -    ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg);
>> -    if (ret_val)
>> -        goto release;
>> -    phy_reg |= CV_SMB_CTRL_FORCE_SMBUS;
>> -    e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg);
>> -
>> -    e1000e_enable_phy_retry(hw);
>> -
>> -    /* Force SMBus mode in MAC */
>> -    mac_reg = er32(CTRL_EXT);
>> -    mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS;
>> -    ew32(CTRL_EXT, mac_reg);
>> -
>>       /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>>        * LPLU and disable Gig speed when entering ULP
>>        */
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index af5d9d97a0d6..8fcf8f11f5a4 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev 
>> *pdev, bool runtime)
>>       struct e1000_adapter *adapter = netdev_priv(netdev);
>>       struct e1000_hw *hw = &adapter->hw;
>>       u32 ctrl, ctrl_ext, rctl, status, wufc;
>> +    u16 smb_ctrl;
>>       int retval = 0;
>>       /* Runtime suspend should only enable wakeup for link changes */
>> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev 
>> *pdev, bool runtime)
>>           if (retval)
>>               return retval;
>> +
>> +        /* Force SMBUS to allow WOL */
>> +        /* Switching PHY interface always returns MDI error
>> +         * so disable retry mechanism to avoid wasting time
>> +         */
>> +        e1000e_disable_phy_retry(hw);
>> +
>> +        e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl);
>> +        smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
>> +        e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl);
>> +
>> +        e1000e_enable_phy_retry(hw);
>> +
>> +        /* Force SMBus mode in MAC */
>> +        ctrl_ext = er32(CTRL_EXT);
>> +        ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
>> +        ew32(CTRL_EXT, ctrl_ext);
>>       }
>>       /* Ensure that the appropriate bits are set in LPI_CTRL
> 
> Why is `__e1000_shutdown()` the correct place, where it was in 
> `e1000_enable_ulp_lpt_lp()` before?
First of all because forcing the SMBUS is not related to ULP enabling 
flow, but rather a configuration that is related to shutdown. Secondly, 
moving the function to shutdown function allows enough time for the 
interface to settle and avoids adding a delay.
> 
> 
> Kind regards,
> 
> Paul
Paul Menzel Feb. 29, 2024, 2:31 p.m. UTC | #3
Dear Vitaly,


Thank you for your reply.


Am 29.02.24 um 11:34 schrieb Lifshits, Vitaly:

> On 2/12/2024 11:00 AM, Paul Menzel wrote:

>> Am 11.02.24 um 16:12 schrieb Vitaly Lifshits:
>>> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on
>>> some systems.
>>
>> Please list the systems you know of.
> On some Meteor-lake systems, we also suspect that on some legacy 
> platforms this issue happened with some low probability.

It’d be great if you could be more specific about the Meteor Lake 
systems. What model and what firmware versions?

>>> It is suspected to be caused by initiating PHY transactions before the
>>> the interface settles.
>>
>> No new paragraph is needed here. Please do not break the line, just 
>> because a sentence ends. (If you use paragraphs, please separate them 
>> by a blank line.
> Will be fixed in a v2.

Thank you.

>>> Separating this configuration from the ULP enabling flow avoids this
>>> issue.
>>>
>>> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in Sx state")
>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 -------------------
>>>   drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++++++++++++
>>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> index 717c52913e84..4d83c9a0c023 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
>>>       if (ret_val)
>>>           goto out;
>>> -    /* Switching PHY interface always returns MDI error
>>> -     * so disable retry mechanism to avoid wasting time
>>> -     */
>>> -    e1000e_disable_phy_retry(hw);
>>> -
>>> -    /* Force SMBus mode in PHY */
>>> -    ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg);
>>> -    if (ret_val)
>>> -        goto release;
>>> -    phy_reg |= CV_SMB_CTRL_FORCE_SMBUS;
>>> -    e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg);
>>> -
>>> -    e1000e_enable_phy_retry(hw);
>>> -
>>> -    /* Force SMBus mode in MAC */
>>> -    mac_reg = er32(CTRL_EXT);
>>> -    mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS;
>>> -    ew32(CTRL_EXT, mac_reg);
>>> -
>>>       /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>>>        * LPLU and disable Gig speed when entering ULP
>>>        */
>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
>>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> index af5d9d97a0d6..8fcf8f11f5a4 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev 
>>> *pdev, bool runtime)
>>>       struct e1000_adapter *adapter = netdev_priv(netdev);
>>>       struct e1000_hw *hw = &adapter->hw;
>>>       u32 ctrl, ctrl_ext, rctl, status, wufc;
>>> +    u16 smb_ctrl;
>>>       int retval = 0;
>>>       /* Runtime suspend should only enable wakeup for link changes */
>>> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>>>           if (retval)
>>>               return retval;
>>> +
>>> +        /* Force SMBUS to allow WOL */
>>> +        /* Switching PHY interface always returns MDI error
>>> +         * so disable retry mechanism to avoid wasting time
>>> +         */
>>> +        e1000e_disable_phy_retry(hw);
>>> +
>>> +        e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl);
>>> +        smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
>>> +        e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl);
>>> +
>>> +        e1000e_enable_phy_retry(hw);
>>> +
>>> +        /* Force SMBus mode in MAC */
>>> +        ctrl_ext = er32(CTRL_EXT);
>>> +        ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
>>> +        ew32(CTRL_EXT, ctrl_ext);
>>>       }
>>>       /* Ensure that the appropriate bits are set in LPI_CTRL
>>
>> Why is `__e1000_shutdown()` the correct place, where it was in 
>> `e1000_enable_ulp_lpt_lp()` before?
> First of all because forcing the SMBUS is not related to ULP enabling 
> flow, but rather a configuration that is related to shutdown. Secondly, 
> moving the function to shutdown function allows enough time for the 
> interface to settle and avoids adding a delay.

Please add that clarification to the commit message.


Kind regards,

Paul
Vitaly Lifshits Feb. 29, 2024, 4:16 p.m. UTC | #4
On 2/29/2024 4:31 PM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for your reply.
> 
> 
> Am 29.02.24 um 11:34 schrieb Lifshits, Vitaly:
> 
>> On 2/12/2024 11:00 AM, Paul Menzel wrote:
> 
>>> Am 11.02.24 um 16:12 schrieb Vitaly Lifshits:
>>>> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY 
>>>> loss on
>>>> some systems.
>>>
>>> Please list the systems you know of.
>> On some Meteor-lake systems, we also suspect that on some legacy 
>> platforms this issue happened with some low probability.
> 
> It’d be great if you could be more specific about the Meteor Lake 
> systems. What model and what firmware versions?
I do not have this kind of detailed information.
> 
>>>> It is suspected to be caused by initiating PHY transactions before the
>>>> the interface settles.
>>>
>>> No new paragraph is needed here. Please do not break the line, just 
>>> because a sentence ends. (If you use paragraphs, please separate them 
>>> by a blank line.
>> Will be fixed in a v2.
> 
> Thank you.
> 
>>>> Separating this configuration from the ULP enabling flow avoids this
>>>> issue.
>>>>
>>>> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE 
>>>> in Sx state")
>>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>>> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>>> ---
>>>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 -------------------
>>>>   drivers/net/ethernet/intel/e1000e/netdev.c  | 18 ++++++++++++++++++
>>>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> index 717c52913e84..4d83c9a0c023 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw 
>>>> *hw, bool to_sx)
>>>>       if (ret_val)
>>>>           goto out;
>>>> -    /* Switching PHY interface always returns MDI error
>>>> -     * so disable retry mechanism to avoid wasting time
>>>> -     */
>>>> -    e1000e_disable_phy_retry(hw);
>>>> -
>>>> -    /* Force SMBus mode in PHY */
>>>> -    ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg);
>>>> -    if (ret_val)
>>>> -        goto release;
>>>> -    phy_reg |= CV_SMB_CTRL_FORCE_SMBUS;
>>>> -    e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg);
>>>> -
>>>> -    e1000e_enable_phy_retry(hw);
>>>> -
>>>> -    /* Force SMBus mode in MAC */
>>>> -    mac_reg = er32(CTRL_EXT);
>>>> -    mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS;
>>>> -    ew32(CTRL_EXT, mac_reg);
>>>> -
>>>>       /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>>>>        * LPLU and disable Gig speed when entering ULP
>>>>        */
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
>>>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> index af5d9d97a0d6..8fcf8f11f5a4 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>>>> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev 
>>>> *pdev, bool runtime)
>>>>       struct e1000_adapter *adapter = netdev_priv(netdev);
>>>>       struct e1000_hw *hw = &adapter->hw;
>>>>       u32 ctrl, ctrl_ext, rctl, status, wufc;
>>>> +    u16 smb_ctrl;
>>>>       int retval = 0;
>>>>       /* Runtime suspend should only enable wakeup for link changes */
>>>> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev 
>>>> *pdev, bool runtime)
>>>>           if (retval)
>>>>               return retval;
>>>> +
>>>> +        /* Force SMBUS to allow WOL */
>>>> +        /* Switching PHY interface always returns MDI error
>>>> +         * so disable retry mechanism to avoid wasting time
>>>> +         */
>>>> +        e1000e_disable_phy_retry(hw);
>>>> +
>>>> +        e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl);
>>>> +        smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
>>>> +        e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl);
>>>> +
>>>> +        e1000e_enable_phy_retry(hw);
>>>> +
>>>> +        /* Force SMBus mode in MAC */
>>>> +        ctrl_ext = er32(CTRL_EXT);
>>>> +        ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
>>>> +        ew32(CTRL_EXT, ctrl_ext);
>>>>       }
>>>>       /* Ensure that the appropriate bits are set in LPI_CTRL
>>>
>>> Why is `__e1000_shutdown()` the correct place, where it was in 
>>> `e1000_enable_ulp_lpt_lp()` before?
>> First of all because forcing the SMBUS is not related to ULP enabling 
>> flow, but rather a configuration that is related to shutdown. 
>> Secondly, moving the function to shutdown function allows enough time 
>> for the interface to settle and avoids adding a delay.
> 
> Please add that clarification to the commit message.
Thanks, I'll take care of it in v2.
> 
> 
> Kind regards,
> 
> Paul
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 717c52913e84..4d83c9a0c023 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1165,25 +1165,6 @@  s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
 	if (ret_val)
 		goto out;
 
-	/* Switching PHY interface always returns MDI error
-	 * so disable retry mechanism to avoid wasting time
-	 */
-	e1000e_disable_phy_retry(hw);
-
-	/* Force SMBus mode in PHY */
-	ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg);
-	if (ret_val)
-		goto release;
-	phy_reg |= CV_SMB_CTRL_FORCE_SMBUS;
-	e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg);
-
-	e1000e_enable_phy_retry(hw);
-
-	/* Force SMBus mode in MAC */
-	mac_reg = er32(CTRL_EXT);
-	mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS;
-	ew32(CTRL_EXT, mac_reg);
-
 	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
 	 * LPLU and disable Gig speed when entering ULP
 	 */
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index af5d9d97a0d6..8fcf8f11f5a4 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6622,6 +6622,7 @@  static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 	u32 ctrl, ctrl_ext, rctl, status, wufc;
+	u16 smb_ctrl;
 	int retval = 0;
 
 	/* Runtime suspend should only enable wakeup for link changes */
@@ -6696,6 +6697,23 @@  static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 
 		if (retval)
 			return retval;
+
+		/* Force SMBUS to allow WOL */
+		/* Switching PHY interface always returns MDI error
+		 * so disable retry mechanism to avoid wasting time
+		 */
+		e1000e_disable_phy_retry(hw);
+
+		e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl);
+		smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
+		e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl);
+
+		e1000e_enable_phy_retry(hw);
+
+		/* Force SMBus mode in MAC */
+		ctrl_ext = er32(CTRL_EXT);
+		ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
+		ew32(CTRL_EXT, ctrl_ext);
 	}
 
 	/* Ensure that the appropriate bits are set in LPI_CTRL