[Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: change k1 exit timeout on MTP and later platforms

[Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: change k1 exit timeout on MTP and later platforms

[Vitaly Lifshits]

LAN devices starting from Meteorlake the interface between the MAC and
the PHY has a different frequency.
This caused sporadic MDI errors when accessing the PHY and a rare case
of packets corruption. To overcome this introduce a PHY K1 exit timeout
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.

Fixes: cc23f4f0b6b9 ("e1000e: Add support for Meteor Lake")
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
---
 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)
+{
+	u16 phy_timeout;
+	u32 fextnvm12;
+	s32 ret_val;
+
+	if (hw->mac.type < e1000_pch_mtp)
+		return 0;
+
+	/* Change Kumeran K1 power down state from P0s to P1 */
+	fextnvm12 = er32(FEXTNVM12);
+	fextnvm12 |= BIT(23);
+	fextnvm12 &= ~BIT(22);
+	ew32(FEXTNVM12, fextnvm12);
+
+	/* Wait for the interface the settle */
+	msleep(1);
+
+	/* 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");
+			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 */
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: change k1 exit timeout on MTP and later platforms

[Paul Menzel]

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.

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?

> 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?

How did you test this?

> Fixes: cc23f4f0b6b9 ("e1000e: Add support for Meteor Lake")
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> ---
>   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?

> +	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?

> +	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?

> +
> +	/* 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?

> +			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

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: change k1 exit timeout on MTP and later platforms

[Lifshits, Vitaly]

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 <vitaly.lifshits@intel.com>
>> ---
>>   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