[Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Vitaly Lifshits]

Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function
to avoid PHY loss issue") resolved a PHY access loss during suspend on
Meteor Lake consumer platforms, but it affected corporate systems
incorrectly.

A better fix, working for both consumer and corporate systems, was
proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end
of enable_ulp function"). 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 to Meteor Lake platforms only.

Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
Reported-by: Todd Brandt <todd.e.brandt@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
--
v2: enhance the function description and address community comments
v1: initial version
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
 1 file changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 	return 0;
 }
 
+/**
+ *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
+ *  @hw: pointer to the HW structure
+ *
+ *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
+ *  acquired.
+ *
+ * Return: 0 on success, negative errno on failure.
+ **/
+static s32 e1000e_force_smbus(struct e1000_hw *hw)
+{
+	u16 smb_ctrl = 0;
+	u32 ctrl_ext;
+	s32 ret_val;
+
+	/* 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) {
+		e1000e_enable_phy_retry(hw);
+		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 +1206,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 release;
+		}
+	}
+
 	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
 	 * LPLU and disable Gig speed when entering ULP
 	 */
@@ -1225,27 +1274,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 over MTL system: %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:
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Paul Menzel]

Dear Vitaly,


Thank you for your patch. For the summary, it’d be great if you could be 
more specific by saying it’s about limiting the scope.

Am 20.06.24 um 08:36 schrieb Vitaly Lifshits:
> Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function
> to avoid PHY loss issue") resolved a PHY access loss during suspend on
> Meteor Lake consumer platforms, but it affected corporate systems
> incorrectly.
> 
> A better fix, working for both consumer and corporate systems, was
> proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end

SMBus

> of enable_ulp function"). However, it introduced a regression on older
> devices, such as [8086:15B8], [8086:15F9], [8086:15BE].

Sort the ids? Also, I do not have the ids memorized. Maybe also describe 
them.

Please summarize the regression, and maybe make it clear, that’s is the 
Bugzilla reports in the tags.

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

So what makes Meteor Lake special? Why is it not needed for predecessors 
and successors?

As now three commits are involved, it’d be really nice to have an 
elaborate summary and also test matrix in the commit message.


Kind regards,

Paul


> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
> Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> --
> v2: enhance the function description and address community comments
> v1: initial version
> ---
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
>   1 file changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
>   	return 0;
>   }
>   
> +/**
> + *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
> + *  @hw: pointer to the HW structure
> + *
> + *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
> + *  acquired.
> + *
> + * Return: 0 on success, negative errno on failure.
> + **/
> +static s32 e1000e_force_smbus(struct e1000_hw *hw)
> +{
> +	u16 smb_ctrl = 0;
> +	u32 ctrl_ext;
> +	s32 ret_val;
> +
> +	/* 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) {
> +		e1000e_enable_phy_retry(hw);
> +		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 +1206,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 release;
> +		}
> +	}
> +
>   	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>   	 * LPLU and disable Gig speed when entering ULP
>   	 */
> @@ -1225,27 +1274,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 over MTL system: %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:

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Lifshits, Vitaly]

On 6/20/2024 9:47 AM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for your patch. For the summary, it’d be great if you could be 
> more specific by saying it’s about limiting the scope.
> 
> Am 20.06.24 um 08:36 schrieb Vitaly Lifshits:
>> Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function
>> to avoid PHY loss issue") resolved a PHY access loss during suspend on
>> Meteor Lake consumer platforms, but it affected corporate systems
>> incorrectly.
>>
>> A better fix, working for both consumer and corporate systems, was
>> proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end
> 
> SMBus

I copied the title of that commit, I don't agree that we need to change it.

> 
>> of enable_ulp function"). However, it introduced a regression on older
>> devices, such as [8086:15B8], [8086:15F9], [8086:15BE].
> 
> Sort the ids? Also, I do not have the ids memorized. Maybe also describe 
> them.

I took the device IDs from the Bugzilla report, why do they need to be 
sorted to projects? This is a bug fix for a reported bug.

> 
> Please summarize the regression, and maybe make it clear, that’s is the 
> Bugzilla reports in the tags.

Why does it matter that it is a Bugzilla report? it is clear that it was 
reported in Bugzilla the Link tag.

> 
>> This patch aims to fix the secondary regression, by limiting the scope of
>> the changes to Meteor Lake platforms only.
> 
> So what makes Meteor Lake special? Why is it not needed for predecessors 
> and successors?

It is still under debug.

> 
> As now three commits are involved, it’d be really nice to have an 
> elaborate summary and also test matrix in the commit message.

I mentioned the devices that were hit by the regression as well as the 
devices that had the initial issue (Meteor lake systems). The commit 
message summarizes all the history for this commit.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
>> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of 
>> enable_ulp function")
>> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
>> Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> -- 
>> v2: enhance the function description and address community comments
>> v1: initial version
>> ---
>>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
>>   1 file changed, 53 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct 
>> e1000_hw *hw, bool link)
>>       return 0;
>>   }
>> +/**
>> + *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
>> + *  @hw: pointer to the HW structure
>> + *
>> + *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
>> + *  acquired.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + **/
>> +static s32 e1000e_force_smbus(struct e1000_hw *hw)
>> +{
>> +    u16 smb_ctrl = 0;
>> +    u32 ctrl_ext;
>> +    s32 ret_val;
>> +
>> +    /* 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) {
>> +        e1000e_enable_phy_retry(hw);
>> +        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 +1206,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 release;
>> +        }
>> +    }
>> +
>>       /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>>        * LPLU and disable Gig speed when entering ULP
>>        */
>> @@ -1225,27 +1274,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 over MTL system: %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:

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Brandt, Todd E]

I just built and tested your patch on the latest 6.10.0-rc3 tip. It seems to have fixed the issue on three of our machines, but the issue still occurs on our Meteor Lake SDV board (otcpl-mtl-s).

[  130.302511] e1000e: EEE TX LPI TIMER: 00000011
[  130.390014] e1000e 0000:80:1f.6: PM: pci_pm_suspend(): e1000e_pm_suspend [e1000e] returns -2
[  130.390033] e1000e 0000:80:1f.6: PM: dpm_run_callback(): pci_pm_suspend returns -2
[  130.390039] e1000e 0000:80:1f.6: PM: failed to suspend async: error -2
[  130.574807] PM: suspend of devices aborted after 293.955 msecs
[  130.574817] PM: start suspend of devices aborted after 376.596 msecs
[  130.574820] PM: Some devices failed to suspend, or early wake event detected

$> lspci -nn -s 80:1f.6
80:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550d]

-----Original Message-----
From: Lifshits, Vitaly <vitaly.lifshits@intel.com> 
Sent: Wednesday, June 19, 2024 11:37 PM
To: intel-wired-lan@osuosl.org
Cc: hui.wang@canonical.com; Lifshits, Vitaly <vitaly.lifshits@intel.com>; Brandt, Todd E <todd.e.brandt@intel.com>; Dieter Mummenschanz <dmummenschanz@web.de>
Subject: [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") resolved a PHY access loss during suspend on Meteor Lake consumer platforms, but it affected corporate systems incorrectly.

A better fix, working for both consumer and corporate systems, was proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function"). 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 to Meteor Lake platforms only.

Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
Reported-by: Todd Brandt <todd.e.brandt@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
--
v2: enhance the function description and address community comments
v1: initial version
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
 1 file changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 	return 0;
 }
 
+/**
+ *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
+ *  @hw: pointer to the HW structure
+ *
+ *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
+ *  acquired.
+ *
+ * Return: 0 on success, negative errno on failure.
+ **/
+static s32 e1000e_force_smbus(struct e1000_hw *hw) {
+	u16 smb_ctrl = 0;
+	u32 ctrl_ext;
+	s32 ret_val;
+
+	/* 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) {
+		e1000e_enable_phy_retry(hw);
+		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 +1206,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 release;
+		}
+	}
+
 	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
 	 * LPLU and disable Gig speed when entering ULP
 	 */
@@ -1225,27 +1274,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 over MTL system: %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:
--
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Lifshits, Vitaly]

On 6/21/2024 10:55 AM, Brandt, Todd E wrote:
> I just built and tested your patch on the latest 6.10.0-rc3 tip. It seems to have fixed the issue on three of our machines, but the issue still occurs on our Meteor Lake SDV board (otcpl-mtl-s).
> 
> [  130.302511] e1000e: EEE TX LPI TIMER: 00000011
> [  130.390014] e1000e 0000:80:1f.6: PM: pci_pm_suspend(): e1000e_pm_suspend [e1000e] returns -2
> [  130.390033] e1000e 0000:80:1f.6: PM: dpm_run_callback(): pci_pm_suspend returns -2
> [  130.390039] e1000e 0000:80:1f.6: PM: failed to suspend async: error -2
> [  130.574807] PM: suspend of devices aborted after 293.955 msecs
> [  130.574817] PM: start suspend of devices aborted after 376.596 msecs
> [  130.574820] PM: Some devices failed to suspend, or early wake event detected
> 
> $> lspci -nn -s 80:1f.6
> 80:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550d]

I see that the bus of your device is 80 and not 0 as usual, do you use 
virtualization features? If so, can you please disable them and retest?

> 
> -----Original Message-----
> From: Lifshits, Vitaly <vitaly.lifshits@intel.com>
> Sent: Wednesday, June 19, 2024 11:37 PM
> To: intel-wired-lan@osuosl.org
> Cc: hui.wang@canonical.com; Lifshits, Vitaly <vitaly.lifshits@intel.com>; Brandt, Todd E <todd.e.brandt@intel.com>; Dieter Mummenschanz <dmummenschanz@web.de>
> Subject: [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow
> 
> Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") resolved a PHY access loss during suspend on Meteor Lake consumer platforms, but it affected corporate systems incorrectly.
> 
> A better fix, working for both consumer and corporate systems, was proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function"). 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 to Meteor Lake platforms only.
> 
> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
> Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> --
> v2: enhance the function description and address community comments
> v1: initial version
> ---
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
>   1 file changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
>   return 0;
>   }
> 
> +/**
> + *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
> + *  @hw: pointer to the HW structure
> + *
> + *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
> + *  acquired.
> + *
> + * Return: 0 on success, negative errno on failure.
> + **/
> +static s32 e1000e_force_smbus(struct e1000_hw *hw) {
> +u16 smb_ctrl = 0;
> +u32 ctrl_ext;
> +s32 ret_val;
> +
> +/* 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) {
> +e1000e_enable_phy_retry(hw);
> +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 +1206,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 release;
> +}
> +}
> +
>   /* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>    * LPLU and disable Gig speed when entering ULP
>    */
> @@ -1225,27 +1274,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 over MTL system: %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:
> --
> 2.34.1
> 

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Dieter Mummenschanz]

[-- Attachment #1: Type: text/html, Size: 8735 bytes --]

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Przemek Kitszel]

On 6/28/24 09:48, Dieter Mummenschanz wrote:
> Hi,
> any chance we can upstream the patch before 6.10 goes final? At least it 
> would fix suspend on older devices (I219-V [8086:15bc] (rev 10)).
> Kind Regards,
> Dieter

would be great, do you want to add your Tested-by?

> 
> On 6/21/2024 10:55 AM, Brandt, Todd E wrote:
>  > I just built and tested your patch on the latest 6.10.0-rc3 tip. It 
> seems to have fixed the issue on three of our machines, but the issue 
> still occurs on our Meteor Lake SDV board (otcpl-mtl-s).

My understanding was that the very first regression was for Meteor Lake,
with subsequent fix (bfd546a552e1) and yet another now (this thread),
with both of the fixes targeted to help on prior platforms. According to
your comment, this patch seems to help with "three of our machines", I 
read this as those are the "older devices" that Vitaly refers to.

So just from your comment I would say that this patch reduces the scope
of affected platforms to a subset of what it was prior to the very first
fix (bfd546a552e1), is that correct?
if so, you could provide your Tested-by tag.

>  >
>  > [ 130.302511] e1000e: EEE TX LPI TIMER: 00000011
>  > [ 130.390014] e1000e 0000:80:1f.6: PM: pci_pm_suspend(): 
> e1000e_pm_suspend [e1000e] returns -2
>  > [ 130.390033] e1000e 0000:80:1f.6: PM: dpm_run_callback(): 
> pci_pm_suspend returns -2
>  > [ 130.390039] e1000e 0000:80:1f.6: PM: failed to suspend async: error -2
>  > [ 130.574807] PM: suspend of devices aborted after 293.955 msecs
>  > [ 130.574817] PM: start suspend of devices aborted after 376.596 msecs
>  > [ 130.574820] PM: Some devices failed to suspend, or early wake event 
> detected
>  >
>  > $> lspci -nn -s 80:1f.6
>  > 80:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550d]
> 
> I see that the bus of your device is 80 and not 0 as usual, do you use
> virtualization features? If so, can you please disable them and retest?
> 

Would be good to know more to aid future debug, or perhaps it should be
a blocker here? (IOW what's the scope)

>  >
>  > -----Original Message-----
>  > From: Lifshits, Vitaly <vitaly.lifshits@intel.com>
>  > Sent: Wednesday, June 19, 2024 11:37 PM
>  > To: intel-wired-lan@osuosl.org
>  > Cc: hui.wang@canonical.com; Lifshits, Vitaly 
> <vitaly.lifshits@intel.com>; Brandt, Todd E <todd.e.brandt@intel.com>; 
> Dieter Mummenschanz <dmummenschanz@web.de>
>  > Subject: [PATCH iwl-net v2 1/1] e1000e: fix force smbus during 
> suspend flow
>  >
>  > Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp 
> function to avoid PHY loss issue") resolved a PHY access loss during 
> suspend on Meteor Lake consumer platforms, but it affected corporate 
> systems incorrectly.
>  >
>  > A better fix, working for both consumer and corporate systems, was 
> proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end 
> of enable_ulp function"). However, it introduced a regression on older 
> devices, such as [8086:15B8], [8086:15F9], [8086:15BE].

Paul was right that such lists are best provided in sorted form, but
for free-form text with just three items, does not matter that much.

>  >
>  > This patch aims to fix the secondary regression, by limiting the 
> scope of the changes to Meteor Lake platforms only.
>  >
>  > Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of 
> enable_ulp function")
>  > Reported-by: Todd Brandt <todd.e.brandt@intel.com>
>  > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940 
> <https://bugzilla.kernel.org/show_bug.cgi?id=218940>
>  > Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
>  > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936 
> <https://bugzilla.kernel.org/show_bug.cgi?id=218936>
>  > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

[Mor Bar-Gabay]

On 20/06/2024 9:36, Vitaly Lifshits wrote:
> Commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function
> to avoid PHY loss issue") resolved a PHY access loss during suspend on
> Meteor Lake consumer platforms, but it affected corporate systems
> incorrectly.
> 
> A better fix, working for both consumer and corporate systems, was
> proposed in commit bfd546a552e1 ("e1000e: move force SMBUS near the end
> of enable_ulp function"). 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 to Meteor Lake platforms only.
> 
> Fixes: bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function")
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
> Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Reported-by: Todd Brandt <todd.e.brandt@intel.com>
> Reported-by: Dieter Mummenschanz <dmummenschanz@web.de>
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> ---
> v2: enhance the function description and address community comments
> v1: initial version
> ---
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 73 +++++++++++++++------
>   1 file changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index 2e98a2a0bead..86d4ae95b45a 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,46 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
>   	return 0;
>   }
>   
> +/**
> + *  e1000e_force_smbus - Force interfaces to transition to SMBUS mode.
> + *  @hw: pointer to the HW structure
> + *
> + *  Force the MAC and the PHY to SMBUS mode. Assumes semaphore already
> + *  acquired.
> + *
> + * Return: 0 on success, negative errno on failure.
> + **/
> +static s32 e1000e_force_smbus(struct e1000_hw *hw)
> +{
> +	u16 smb_ctrl = 0;
> +	u32 ctrl_ext;
> +	s32 ret_val;
> +
> +	/* 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) {
> +		e1000e_enable_phy_retry(hw);
> +		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 +1206,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 release;
> +		}
> +	}
> +
>   	/* Si workaround for ULP entry flow on i127/rev6 h/w.  Enable
>   	 * LPLU and disable Gig speed when entering ULP
>   	 */
> @@ -1225,27 +1274,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 over MTL system: %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:

Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com> (A Contingent worker 
at Intel)