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