Re: [Intel-wired-lan] [PATCH iwl-net v2 1/1] e1000e: fix force smbus during suspend flow

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

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