On 2/24/2025 3:53 PM, Paul Menzel wrote:
Dear Vitaly,
Thank you for your patch.
Am 24.02.25 um 11:12 schrieb Vitaly Lifshits:
LAN devices starting from Meteorlake the interface between the MAC and
Meteor Lake
the PHY has a different frequency.
The sentences reads a little strange. Maybe:
Starting with Meteor Lake, the frequency of MAC and PHY of the LAN
devices differ, cf. datasheet X section Y.
Will be fixed in v2.
Also, I’d add a blank between paragraphs.
This caused sporadic MDI errors when accessing the PHY and a rare case
cause*s*
of packets corruption. To overcome this introduce a PHY K1 exit timeout
Does Linux log these errors and corruptions?
In some cases yes. When there is packet corruption the user encounters
packet loss or re-transmissions on the wire. Both result in a lower
performance.
For MDI errors, yes there are be error prints to the log.
reconfiguration in the init flow. The exit timeout is reverted during
the hardware reset, thus, it is required to be called in a few places.
Excuse my ignorance, but what do different frequencies have to do with
the exit timeout?
I'll explain this better in the v2 version.
How did you test this?
Ran the failed flows and made sure that the issues don't reproduce.
Fixes: cc23f4f0b6b9 ("e1000e: Add support for Meteor Lake")
Signed-off-by: Vitaly Lifshits <[email protected]>
---
drivers/net/ethernet/intel/e1000e/ich8lan.c | 78 +++++++++++++++++++--
drivers/net/ethernet/intel/e1000e/ich8lan.h | 4 ++
2 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/
net/ethernet/intel/e1000e/ich8lan.c
index 2f9655cf5dd9..d3636433938e 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -285,6 +285,46 @@ static void e1000_toggle_lanphypc_pch_lpt(struct
e1000_hw *hw)
}
}
+/**
+ * e1000_reconfigure_k1_exit_timeout - reconfigure K1 exit timeout to
+ * align to MTP and later platform requirements.
+ * @hw: pointer to the HW structure
+ *
+ * Assuming that PHY semaphore is taken prior to this function call.
+ *
+ * Return: 0 on success, negative on failure
+ */
+static s32 e1000_reconfigure_k1_exit_timeout(struct e1000_hw *hw)
Why limit the variable length?
+{
+ u16 phy_timeout;
Please add the unit. >
Why limit the variable length?
I am not sure that I understood your question here.
phy_timeout will hold the data from a PHY register, that has a size of
uint16.
+ u32 fextnvm12;
+ s32 ret_val;
+
+ if (hw->mac.type < e1000_pch_mtp)
+ return 0;
+
+ /* Change Kumeran K1 power down state from P0s to P1*/
What is Kumeran?
It is the interface between the MAC and the PHY.
+ fextnvm12 = er32(FEXTNVM12);
+ fextnvm12 |= BIT(23);
+ fextnvm12 &= ~BIT(22);
+ ew32(FEXTNVM12, fextnvm12);
+
+ /* Wait for the interface the settle */
+ msleep(1);
Any chance, of polling stuff instead of 1 ms sleep?
No, there is no indication for this.
+
+ /* Change K1 exit timeout */
+ ret_val = e1e_rphy_locked(hw, E1000_PHY_TIMEOUTS_REG,
+ &phy_timeout);
+ if (ret_val)
+ return ret_val;
+
+ phy_timeout &= ~E1000_PHY_TIMEOUTS_K1_EXIT_TO_MASK;
+ phy_timeout |= 0xF00;
+
+ return e1e_wphy_locked(hw, E1000_PHY_TIMEOUTS_REG,
+ phy_timeout);
+}
+
/**
* e1000_init_phy_workarounds_pchlan - PHY initialization workarounds
* @hw: pointer to the HW structure
@@ -327,15 +367,23 @@ static s32
e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
* LANPHYPC Value bit to force the interconnect to PCIe mode.
*/
switch (hw->mac.type) {
+ case e1000_pch_mtp:
+ case e1000_pch_lnp:
+ case e1000_pch_ptp:
+ case e1000_pch_nvp:
+ /* At this point the PHY might be inaccessible so don't
+ * propagate the failure
+ */
+ if (e1000_reconfigure_k1_exit_timeout(hw))
+ break;
+
+ fallthrough;
case e1000_pch_lpt:
case e1000_pch_spt:
case e1000_pch_cnp:
case e1000_pch_tgp:
case e1000_pch_adp:
- case e1000_pch_mtp:
- case e1000_pch_lnp:
- case e1000_pch_ptp:
- case e1000_pch_nvp:
+
if (e1000_phy_is_accessible_pchlan(hw))
break;
@@ -421,6 +469,16 @@ static s32
e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
ret_val = hw->phy.ops.check_reset_block(hw);
if (ret_val)
e_err("ME blocked access to PHY after reset\n");
+
+ if (hw->mac.type >= e1000_pch_mtp) {
+ ret_val = hw->phy.ops.acquire(hw);
+ if (ret_val) {
+ e_dbg("Failed to reconfigure K1 exit timeout\n");
+ goto out;
+ }
+ ret_val = e1000_reconfigure_k1_exit_timeout(hw);
+ hw->phy.ops.release(hw);
+ }
}
out:
@@ -4888,6 +4946,18 @@ static s32 e1000_init_hw_ich8lan(struct
e1000_hw *hw)
u16 i;
e1000_initialize_hw_bits_ich8lan(hw);
+ if (hw->mac.type >= e1000_pch_mtp) {
+ ret_val = hw->phy.ops.acquire(hw);
+ if (ret_val)
+ return ret_val;
+
+ ret_val = e1000_reconfigure_k1_exit_timeout(hw);
+ if (ret_val) {
+ e_dbg("Error failed to reconfigure K1 exit timeout\n");
This differs from the log above. Use consistent error messages?
I used here the same convention as in this whole function.
In addition, in the v2 version I changed the e_dbg to e_err so it will
be printed as an error anyway.
+ return ret_val;
+ }
+ hw->phy.ops.release(hw);
+ }
/* Initialize identification LED */
ret_val = mac->ops.id_led_init(hw);
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h b/drivers/
net/ethernet/intel/e1000e/ich8lan.h
index 2504b11c3169..dffc63e89ee2 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.h
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h
@@ -219,6 +219,10 @@
#define I217_PLL_CLOCK_GATE_REG PHY_REG(772, 28)
#define I217_PLL_CLOCK_GATE_MASK 0x07FF
+/* PHY Timeouts */
+#define E1000_PHY_TIMEOUTS_REG PHY_REG(770, 21)
+#define E1000_PHY_TIMEOUTS_K1_EXIT_TO_MASK 0x0FC0
+
#define SW_FLAG_TIMEOUT 1000 /* SW Semaphore flag timeout
in ms */
/* Inband Control */
Kind regards,
Paul
