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.bra...@intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
Reported-by: Dieter Mummenschanz <dmummensch...@web.de>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
Signed-off-by: Vitaly Lifshits <vitaly.lifsh...@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: