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

["Lifshits, Vitaly" <vitaly.lifshits@intel.com>]

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