diff mbox series

[iwl-net,v1,1/1] e1000e: fix force smbus during suspend flow

Message ID 20240613120134.224585-1-vitaly.lifshits@intel.com
State Superseded
Headers show
Series [iwl-net,v1,1/1] e1000e: fix force smbus during suspend flow | expand

Commit Message

Lifshits, Vitaly June 13, 2024, 12:01 p.m. UTC
Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of
enable_ulp function") fixed an issue with loss of PHY access during
suspend on Meteor Lake systems. However, it introduced a regression
on older devices, such as [8086:15B8], [8086:15F9], [8086:15BE].

This patch aims to fix the secondary regression, by limiting the scope
of the changes in that commit to Meteor Lake platforms only.

Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218940
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 68 +++++++++++++++------
 1 file changed, 48 insertions(+), 20 deletions(-)

Comments

Hui Wang June 14, 2024, 1:53 a.m. UTC | #1
Hi Vitaly

I tested the patch on a laptop with the ethernet card [8086:550A], the 
system suspend and resume worked well with the cable plugged or unplugged.

But I still think that reverting 2 regression commits is a better solution.

Thanks.

And some comment below:

On 6/13/24 20:01, Vitaly Lifshits wrote:
> Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of
> enable_ulp function") fixed an issue with loss of PHY access during
> suspend on Meteor Lake systems. However, it introduced a regression
> on older devices, such as [8086:15B8], [8086:15F9], [8086:15BE].
>
> This patch aims to fix the secondary regression, by limiting the scope
> of the changes in that commit to Meteor Lake platforms only.
>
> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218940
> ---
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 68 +++++++++++++++------
>   1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 2e98a2a0bead..7b1da97e82bf 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -137,6 +137,7 @@ static void e1000_gate_hw_phy_config_ich8lan(struct e1000_hw *hw, bool gate);
>   static s32 e1000_disable_ulp_lpt_lp(struct e1000_hw *hw, bool force);
>   static s32 e1000_setup_copper_link_pch_lpt(struct e1000_hw *hw);
>   static s32 e1000_oem_bits_config_ich8lan(struct e1000_hw *hw, bool d0_state);
> +static s32 e1000e_force_smbus(struct e1000_hw *hw);
>   
>   static inline u16 __er16flash(struct e1000_hw *hw, unsigned long reg)
>   {
> @@ -1108,6 +1109,41 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
>   	return 0;
>   }
>   
> +/**
> + *  e1000e_force_smbus
> + *  @hw: pointer to the HW structure
> + *
> + *  Force the MAC and the PHY to SMBUS mode.
> + **/
> +static s32 e1000e_force_smbus(struct e1000_hw *hw)
> +{
> +	u16 smb_ctrl = 0;
> +	u32 ctrl_ext = 0;
> +	s32 ret_val = 0;
> +
> +	/* Switching PHY interface always returns MDI error
> +	 * so disable retry mechanism to avoid wasting time
> +	 */
> +	e1000e_disable_phy_retry(hw);
> +
> +	/* Force SMBus mode in the PHY */
> +	ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &smb_ctrl);
> +	if (ret_val)
Does the e1000e_enable_phy_retry(hw) need to be called here?
> +		return ret_val;
> +
> +	smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
> +	e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, smb_ctrl);
> +
> +	e1000e_enable_phy_retry(hw);
> +
> +	/* Force SMBus mode in the MAC */
> +	ctrl_ext = er32(CTRL_EXT);
> +	ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
> +	ew32(CTRL_EXT, ctrl_ext);
> +
> +	return 0;
> +}
> +
>   /**
>    *  e1000_enable_ulp_lpt_lp - configure Ultra Low Power mode for LynxPoint-LP
>    *  @hw: pointer to the HW structure
> @@ -1165,6 +1201,14 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
>   	if (ret_val)
>   		goto out;
>   
> +	if (hw->mac.type != e1000_pch_mtp) {
> +		ret_val = e1000e_force_smbus(hw);
> +		if (ret_val) {
> +			e_dbg("Failed to force SMBUS: %d\n", ret_val);
Does the hw->phy.ops.release(hw) need to be called here or "goto 
release" instead of "goto out"?
> +			goto out;
> +		}
> +	}
> +
>   	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>   	 * LPLU and disable Gig speed when entering ULP
>   	 */
> @@ -1225,27 +1269,11 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
>   	}
>   
>   release:
> -	/* 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) {
> -		e1000e_enable_phy_retry(hw);
> -		hw->phy.ops.release(hw);
> -		goto out;
> +	if (hw->mac.type == e1000_pch_mtp) {

Maybe we should also check ret_val like below:

if (!ret_val && hw->mac.type == e1000_pch_mtp) {

> +		ret_val = e1000e_force_smbus(hw);
> +		if (ret_val)
> +			e_dbg("Failed to force SMBUS: %d\n", ret_val);
>   	}
> -	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);
>   
>   	hw->phy.ops.release(hw);
>   out:
Lifshits, Vitaly June 19, 2024, 12:17 p.m. UTC | #2
Hi Hui,

On 6/14/2024 4:53 AM, Hui Wang wrote:
> Hi Vitaly
> 
> I tested the patch on a laptop with the ethernet card [8086:550A], the 
> system suspend and resume worked well with the cable plugged or unplugged.
> 
> But I still think that reverting 2 regression commits is a better solution.
Reverting the patches will bring us back to the starting point where we 
had a PHY access loss issue during suspend flows on MTL systems. I don't 
think that it is what we want to accomplish.
> 
> Thanks.
> 
> And some comment below:
> 
> On 6/13/24 20:01, Vitaly Lifshits wrote:
>> Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of
>> enable_ulp function") fixed an issue with loss of PHY access during
>> suspend on Meteor Lake systems. However, it introduced a regression
>> on older devices, such as [8086:15B8], [8086:15F9], [8086:15BE].
>>
>> This patch aims to fix the secondary regression, by limiting the scope
>> of the changes in that commit to Meteor Lake platforms only.
>>
>> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of 
>> enable_ulp function")
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218940
>> ---
>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 68 +++++++++++++++------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index 2e98a2a0bead..7b1da97e82bf 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> @@ -137,6 +137,7 @@ static void 
>> e1000_gate_hw_phy_config_ich8lan(struct e1000_hw *hw, bool gate);
>>   static s32 e1000_disable_ulp_lpt_lp(struct e1000_hw *hw, bool force);
>>   static s32 e1000_setup_copper_link_pch_lpt(struct e1000_hw *hw);
>>   static s32 e1000_oem_bits_config_ich8lan(struct e1000_hw *hw, bool 
>> d0_state);
>> +static s32 e1000e_force_smbus(struct e1000_hw *hw);
>>   static inline u16 __er16flash(struct e1000_hw *hw, unsigned long reg)
>>   {
>> @@ -1108,6 +1109,41 @@ static s32 e1000_platform_pm_pch_lpt(struct 
>> e1000_hw *hw, bool link)
>>       return 0;
>>   }
>> +/**
>> + *  e1000e_force_smbus
>> + *  @hw: pointer to the HW structure
>> + *
>> + *  Force the MAC and the PHY to SMBUS mode.
>> + **/
>> +static s32 e1000e_force_smbus(struct e1000_hw *hw)
>> +{
>> +    u16 smb_ctrl = 0;
>> +    u32 ctrl_ext = 0;
>> +    s32 ret_val = 0;
>> +
>> +    /* Switching PHY interface always returns MDI error
>> +     * so disable retry mechanism to avoid wasting time
>> +     */
>> +    e1000e_disable_phy_retry(hw);
>> +
>> +    /* Force SMBus mode in the PHY */
>> +    ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &smb_ctrl);
>> +    if (ret_val)
> Does the e1000e_enable_phy_retry(hw) need to be called here?
Yes, you are correct. Thank you, I'll fix it in the v2.
>> +        return ret_val;
>> +
>> +    smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
>> +    e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, smb_ctrl);
>> +
>> +    e1000e_enable_phy_retry(hw);
>> +
>> +    /* Force SMBus mode in the MAC */
>> +    ctrl_ext = er32(CTRL_EXT);
>> +    ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
>> +    ew32(CTRL_EXT, ctrl_ext);
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    *  e1000_enable_ulp_lpt_lp - configure Ultra Low Power mode for 
>> LynxPoint-LP
>>    *  @hw: pointer to the HW structure
>> @@ -1165,6 +1201,14 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw 
>> *hw, bool to_sx)
>>       if (ret_val)
>>           goto out;
>> +    if (hw->mac.type != e1000_pch_mtp) {
>> +        ret_val = e1000e_force_smbus(hw);
>> +        if (ret_val) {
>> +            e_dbg("Failed to force SMBUS: %d\n", ret_val);
> Does the hw->phy.ops.release(hw) need to be called here or "goto 
> release" instead of "goto out"?
Yes, you are correct. Thanks for noticing.
>> +            goto out;
>> +        }
>> +    }
>> +
>>       /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>>        * LPLU and disable Gig speed when entering ULP
>>        */
>> @@ -1225,27 +1269,11 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw 
>> *hw, bool to_sx)
>>       }
>>   release:
>> -    /* 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) {
>> -        e1000e_enable_phy_retry(hw);
>> -        hw->phy.ops.release(hw);
>> -        goto out;
>> +    if (hw->mac.type == e1000_pch_mtp) {
> 
> Maybe we should also check ret_val like below:
No, because we want to unconditionally force the MAC and the PHY to 
transition to SMBUS mode. This is how it used to work previously before 
moving this part of the code to the end of the function.
> 
> if (!ret_val && hw->mac.type == e1000_pch_mtp) {
> 
>> +        ret_val = e1000e_force_smbus(hw);
>> +        if (ret_val)
>> +            e_dbg("Failed to force SMBUS: %d\n", ret_val);
>>       }
>> -    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);
>>       hw->phy.ops.release(hw);
>>   out:
Hui Wang June 19, 2024, 12:29 p.m. UTC | #3
On 6/19/24 20:17, Lifshits, Vitaly wrote:
>
> Hi Hui,
>
> On 6/14/2024 4:53 AM, Hui Wang wrote:
>> Hi Vitaly
>>
>> I tested the patch on a laptop with the ethernet card [8086:550A], 
>> the system suspend and resume worked well with the cable plugged or 
>> unplugged.
>>
>> But I still think that reverting 2 regression commits is a better 
>> solution.
> Reverting the patches will bring us back to the starting point where 
> we had a PHY access loss issue during suspend flows on MTL systems. I 
> don't think that it is what we want to accomplish.
OK, got it.
>>
>> Thanks.
...
>> +    if (hw->mac.type == e1000_pch_mtp) {
>>
>> Maybe we should also check ret_val like below:
> No, because we want to unconditionally force the MAC and the PHY to 
> transition to SMBUS mode. This is how it used to work previously 
> before moving this part of the code to the end of the function.

OK, got it.

Thanks.

>>
>> if (!ret_val && hw->mac.type == e1000_pch_mtp) {
>>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 2e98a2a0bead..7b1da97e82bf 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -137,6 +137,7 @@  static void e1000_gate_hw_phy_config_ich8lan(struct e1000_hw *hw, bool gate);
 static s32 e1000_disable_ulp_lpt_lp(struct e1000_hw *hw, bool force);
 static s32 e1000_setup_copper_link_pch_lpt(struct e1000_hw *hw);
 static s32 e1000_oem_bits_config_ich8lan(struct e1000_hw *hw, bool d0_state);
+static s32 e1000e_force_smbus(struct e1000_hw *hw);
 
 static inline u16 __er16flash(struct e1000_hw *hw, unsigned long reg)
 {
@@ -1108,6 +1109,41 @@  static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 	return 0;
 }
 
+/**
+ *  e1000e_force_smbus
+ *  @hw: pointer to the HW structure
+ *
+ *  Force the MAC and the PHY to SMBUS mode.
+ **/
+static s32 e1000e_force_smbus(struct e1000_hw *hw)
+{
+	u16 smb_ctrl = 0;
+	u32 ctrl_ext = 0;
+	s32 ret_val = 0;
+
+	/* Switching PHY interface always returns MDI error
+	 * so disable retry mechanism to avoid wasting time
+	 */
+	e1000e_disable_phy_retry(hw);
+
+	/* Force SMBus mode in the PHY */
+	ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &smb_ctrl);
+	if (ret_val)
+		return ret_val;
+
+	smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS;
+	e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, smb_ctrl);
+
+	e1000e_enable_phy_retry(hw);
+
+	/* Force SMBus mode in the MAC */
+	ctrl_ext = er32(CTRL_EXT);
+	ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS;
+	ew32(CTRL_EXT, ctrl_ext);
+
+	return 0;
+}
+
 /**
  *  e1000_enable_ulp_lpt_lp - configure Ultra Low Power mode for LynxPoint-LP
  *  @hw: pointer to the HW structure
@@ -1165,6 +1201,14 @@  s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
 	if (ret_val)
 		goto out;
 
+	if (hw->mac.type != e1000_pch_mtp) {
+		ret_val = e1000e_force_smbus(hw);
+		if (ret_val) {
+			e_dbg("Failed to force SMBUS: %d\n", ret_val);
+			goto out;
+		}
+	}
+
 	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
 	 * LPLU and disable Gig speed when entering ULP
 	 */
@@ -1225,27 +1269,11 @@  s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx)
 	}
 
 release:
-	/* 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) {
-		e1000e_enable_phy_retry(hw);
-		hw->phy.ops.release(hw);
-		goto out;
+	if (hw->mac.type == e1000_pch_mtp) {
+		ret_val = e1000e_force_smbus(hw);
+		if (ret_val)
+			e_dbg("Failed to force SMBUS: %d\n", ret_val);
 	}
-	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);
 
 	hw->phy.ops.release(hw);
 out: