Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: Fix S0ix residency on Meteor Lake corporate systems

[Paul Menzel]

Dear Vitaly,

Thank you for your reply.

Am 29.05.24 um 13:13 schrieb Lifshits, Vitaly:

On 5/28/2024 1:43 PM, Paul Menzel wrote:

Am 28.05.24 um 12:33 schrieb Vitaly Lifshits:
From: Dima Ruinskiy <dima.ruins...@intel.com>

On vPro systems,the  configuration of the I219-LM to achieve power

s/,the  /, the /
Thank you for noticing it.
I will fix it in a v2.

gating and S0ix residency is split between the driver and the CSME FW.
It was discovered that in some scenarios, where the network cable is
connected and then disconnected, S0ix residency is not always reached.

Disconnected at any point, or just during suspend?
Yes, at any point.

Any URL to the reports?
Yes, https://bugs.launchpad.net/sutton/+bug/2054657

This page does not exist.

    $ curl -I https://bugs.launchpad.net/sutton/+bug/2054657
    HTTP/1.1 404 Not Found
    Date: Wed, 29 May 2024 11:20:52 GMT
    Server: gunicorn
    […]

Please reference the (working) URL in a Link: tag in the footer.

This was root-caused to a subset of I219-LM register writes that are not
performed by the CSME FW. Therefore, the driver should perform these
register writes on corporate setups, regardless of the CSME FW state.

Is that documented somewhere?
Only in an internal documentation.

If you can, it’d be great if you summarized why this is triggered by 
unplugging the network cable.

Please add more information about the affected systems, and the test 
environment (firmware versions, …).
It is mentioned at the beginning of the commit, Meteorlake vPro systems.

Please be more specific. If there is a Meteor Lake vPro system, where 
this can *not* be reproduced, it’d be great to know the exact system you 
tested this with. Meteor Lake has been released a long time ago, and not 
remembering a lot of bug reports, I assume, it’s not always reproducible.

(You missed to comment on my previous comments at the end.)

Fixes: cc23f4f0b6b9 ("e1000e: Add support for Meteor Lake")
Signed-off-by: Dima Ruinskiy <dima.ruins...@intel.com>
Signed-off-by: Vitaly.Lifshits <vitaly.lifsh...@intel.com>

The line above with the dot can be removed.
Will be fixed in a v2.

Signed-off-by: Vitaly Lifshits <vitaly.lifsh...@intel.com>
---
  drivers/net/ethernet/intel/e1000e/netdev.c | 132 ++++++++++-----------
  1 file changed, 66 insertions(+), 66 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
b/drivers/net/ethernet/intel/e1000e/netdev.c
index da5c59daf8ba..3cd161c6672b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6363,49 +6363,49 @@ static void e1000e_s0ix_entry_flow(struct 
e1000_adapter *adapter)
          mac_data |= E1000_EXTCNF_CTRL_GATE_PHY_CFG;
          ew32(EXTCNF_CTRL, mac_data);
-        /* Enable the Dynamic Power Gating in the MAC */
-        mac_data = er32(FEXTNVM7);
-        mac_data |= BIT(22);
-        ew32(FEXTNVM7, mac_data);
-
          /* Disable disconnected cable conditioning for Power Gating */
          mac_data = er32(DPGFR);
          mac_data |= BIT(2);
          ew32(DPGFR, mac_data);
-        /* Don't wake from dynamic Power Gating with clock request */
-        mac_data = er32(FEXTNVM12);
-        mac_data |= BIT(12);
-        ew32(FEXTNVM12, mac_data);
-
-        /* Ungate PGCB clock */
-        mac_data = er32(FEXTNVM9);
-        mac_data &= ~BIT(28);
-        ew32(FEXTNVM9, mac_data);
-
-        /* Enable K1 off to enable mPHY Power Gating */
-        mac_data = er32(FEXTNVM6);
-        mac_data |= BIT(31);
-        ew32(FEXTNVM6, mac_data);
-
-        /* Enable mPHY power gating for any link and speed */
-        mac_data = er32(FEXTNVM8);
-        mac_data |= BIT(9);
-        ew32(FEXTNVM8, mac_data);
-
          /* Enable the Dynamic Clock Gating in the DMA and MAC */
          mac_data = er32(CTRL_EXT);
          mac_data |= E1000_CTRL_EXT_DMA_DYN_CLK_EN;
          ew32(CTRL_EXT, mac_data);
-
-        /* No MAC DPG gating SLP_S0 in modern standby
-         * Switch the logic of the lanphypc to use PMC counter
-         */
-        mac_data = er32(FEXTNVM5);
-        mac_data |= BIT(7);
-        ew32(FEXTNVM5, mac_data);
      }
+    /* Enable the Dynamic Power Gating in the MAC */
+    mac_data = er32(FEXTNVM7);
+    mac_data |= BIT(22);
+    ew32(FEXTNVM7, mac_data);
+
+    /* Don't wake from dynamic Power Gating with clock request */
+    mac_data = er32(FEXTNVM12);
+    mac_data |= BIT(12);
+    ew32(FEXTNVM12, mac_data);
+
+    /* Ungate PGCB clock */
+    mac_data = er32(FEXTNVM9);
+    mac_data &= ~BIT(28);
+    ew32(FEXTNVM9, mac_data);
+
+    /* Enable K1 off to enable mPHY Power Gating */
+    mac_data = er32(FEXTNVM6);
+    mac_data |= BIT(31);
+    ew32(FEXTNVM6, mac_data);
+
+    /* Enable mPHY power gating for any link and speed */
+    mac_data = er32(FEXTNVM8);
+    mac_data |= BIT(9);
+    ew32(FEXTNVM8, mac_data);
+
+    /* No MAC DPG gating SLP_S0 in modern standby
+     * Switch the logic of the lanphypc to use PMC counter
+     */
+    mac_data = er32(FEXTNVM5);
+    mac_data |= BIT(7);
+    ew32(FEXTNVM5, mac_data);
+
      /* Disable the time synchronization clock */
      mac_data = er32(FEXTNVM7);
      mac_data |= BIT(31);
@@ -6498,33 +6498,6 @@ static void e1000e_s0ix_exit_flow(struct 
e1000_adapter *adapter)
      } else {
          /* Request driver unconfigure the device from S0ix */
-        /* Disable the Dynamic Power Gating in the MAC */
-        mac_data = er32(FEXTNVM7);
-        mac_data &= 0xFFBFFFFF;
-        ew32(FEXTNVM7, mac_data);
-
-        /* Disable mPHY power gating for any link and speed */
-        mac_data = er32(FEXTNVM8);
-        mac_data &= ~BIT(9);
-        ew32(FEXTNVM8, mac_data);
-
-        /* Disable K1 off */
-        mac_data = er32(FEXTNVM6);
-        mac_data &= ~BIT(31);
-        ew32(FEXTNVM6, mac_data);
-
-        /* Disable Ungate PGCB clock */
-        mac_data = er32(FEXTNVM9);
-        mac_data |= BIT(28);
-        ew32(FEXTNVM9, mac_data);
-
-        /* Cancel not waking from dynamic
-         * Power Gating with clock request
-         */
-        mac_data = er32(FEXTNVM12);
-        mac_data &= ~BIT(12);
-        ew32(FEXTNVM12, mac_data);
-
          /* Cancel disable disconnected cable conditioning
           * for Power Gating
           */
@@ -6537,13 +6510,6 @@ static void e1000e_s0ix_exit_flow(struct 
e1000_adapter *adapter)
          mac_data &= 0xFFF7FFFF;
          ew32(CTRL_EXT, mac_data);
-        /* Revert the lanphypc logic to use the internal Gbe counter
-         * and not the PMC counter
-         */
-        mac_data = er32(FEXTNVM5);
-        mac_data &= 0xFFFFFF7F;
-        ew32(FEXTNVM5, mac_data);
-
          /* Enable the periodic inband message,
           * Request PCIe clock in K1 page770_17[10:9] =01b
           */
@@ -6581,6 +6547,40 @@ static void e1000e_s0ix_exit_flow(struct 
e1000_adapter *adapter)
      mac_data &= ~BIT(31);
      mac_data |= BIT(0);
      ew32(FEXTNVM7, mac_data);
+
+    /* Disable the Dynamic Power Gating in the MAC */
+    mac_data = er32(FEXTNVM7);
+    mac_data &= 0xFFBFFFFF;
+    ew32(FEXTNVM7, mac_data);
+
+    /* Disable mPHY power gating for any link and speed */
+    mac_data = er32(FEXTNVM8);
+    mac_data &= ~BIT(9);
+    ew32(FEXTNVM8, mac_data);
+
+    /* Disable K1 off */
+    mac_data = er32(FEXTNVM6);
+    mac_data &= ~BIT(31);
+    ew32(FEXTNVM6, mac_data);
+
+    /* Disable Ungate PGCB clock */
+    mac_data = er32(FEXTNVM9);
+    mac_data |= BIT(28);
+    ew32(FEXTNVM9, mac_data);
+
+    /* Cancel not waking from dynamic
+     * Power Gating with clock request
+     */
+    mac_data = er32(FEXTNVM12);
+    mac_data &= ~BIT(12);
+    ew32(FEXTNVM12, mac_data);
+
+    /* Revert the lanphypc logic to use the internal Gbe counter
+     * and not the PMC counter
+     */
+    mac_data = er32(FEXTNVM5);
+    mac_data &= 0xFFFFFF7F;
+    ew32(FEXTNVM5, mac_data);
  }
  static int e1000e_pm_freeze(struct device *dev)

Doesn’t moving this out of the branch and running it unconditionally 
affect a lot more systems than Meteor Lake?


Kind regards,

Paul