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

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/28/2024 3:43 AM, Paul Menzel wrote:
> Dear Vitaly,
> 
> 
> Thank you for the patch.
> 
> Am 28.05.24 um 12:33 schrieb Vitaly Lifshits:
>> From: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>
>> On vPro systems,the  configuration of the I219-LM to achieve power
> 
> s/,the  /, the /
> 
>> 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?
> 
> Any URL to the reports?
> 
>> 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?
> 
> Please add more information about the affected systems, and the test 
> environment (firmware versions, …).
> 
>> Fixes: cc23f4f0b6b9 ("e1000e: Add support for Meteor Lake")
>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>> Signed-off-by: Vitaly.Lifshits <vitaly.lifshits@intel.com>
> 
> The line above with the dot can be removed.
> 
>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> ---

I would also appreciate clarifications so we can understand the fix.

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

I would like an explanation of this as well.

Thanks,
Jake

> Kind regards,
> 
> Paul