Message ID | 20240211151245.811320-1-vitaly.lifshits@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [iwl-net,v1,1/1] e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue | expand |
Dear Vitaly, Thank you for your patch. Am 11.02.24 um 16:12 schrieb Vitaly Lifshits: > Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on > some systems. Please list the systems you know of. > It is suspected to be caused by initiating PHY transactions before the > the interface settles. No new paragraph is needed here. Please do not break the line, just because a sentence ends. (If you use paragraphs, please separate them by a blank line. > Separating this configuration from the ULP enabling flow avoids this > issue. > > Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in Sx state") > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> > Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com> > Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 ------------------- > drivers/net/ethernet/intel/e1000e/netdev.c | 18 ++++++++++++++++++ > 2 files changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index 717c52913e84..4d83c9a0c023 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx) > if (ret_val) > goto out; > > - /* 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) > - goto release; > - 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); > - > /* Si workaround for ULP entry flow on i127/rev6 h/w. Enable > * LPLU and disable Gig speed when entering ULP > */ > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index af5d9d97a0d6..8fcf8f11f5a4 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) > struct e1000_adapter *adapter = netdev_priv(netdev); > struct e1000_hw *hw = &adapter->hw; > u32 ctrl, ctrl_ext, rctl, status, wufc; > + u16 smb_ctrl; > int retval = 0; > > /* Runtime suspend should only enable wakeup for link changes */ > @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) > > if (retval) > return retval; > + > + /* Force SMBUS to allow WOL */ > + /* Switching PHY interface always returns MDI error > + * so disable retry mechanism to avoid wasting time > + */ > + e1000e_disable_phy_retry(hw); > + > + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); > + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; > + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); > + > + e1000e_enable_phy_retry(hw); > + > + /* Force SMBus mode in MAC */ > + ctrl_ext = er32(CTRL_EXT); > + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; > + ew32(CTRL_EXT, ctrl_ext); > } > > /* Ensure that the appropriate bits are set in LPI_CTRL Why is `__e1000_shutdown()` the correct place, where it was in `e1000_enable_ulp_lpt_lp()` before? Kind regards, Paul
On 2/12/2024 11:00 AM, Paul Menzel wrote: > Dear Vitaly, > > > Thank you for your patch. > > Am 11.02.24 um 16:12 schrieb Vitaly Lifshits: >> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on >> some systems. > > Please list the systems you know of. On some Meteor-lake systems, we also suspect that on some legacy platforms this issue happened with some low probability. > >> It is suspected to be caused by initiating PHY transactions before the >> the interface settles. > > No new paragraph is needed here. Please do not break the line, just > because a sentence ends. (If you use paragraphs, please separate them by > a blank line. Will be fixed in a v2. > >> Separating this configuration from the ULP enabling flow avoids this >> issue. >> >> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in >> Sx state") >> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >> --- >> drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 ------------------- >> drivers/net/ethernet/intel/e1000e/netdev.c | 18 ++++++++++++++++++ >> 2 files changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c >> b/drivers/net/ethernet/intel/e1000e/ich8lan.c >> index 717c52913e84..4d83c9a0c023 100644 >> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c >> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c >> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw >> *hw, bool to_sx) >> if (ret_val) >> goto out; >> - /* 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) >> - goto release; >> - 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); >> - >> /* Si workaround for ULP entry flow on i127/rev6 h/w. Enable >> * LPLU and disable Gig speed when entering ULP >> */ >> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c >> b/drivers/net/ethernet/intel/e1000e/netdev.c >> index af5d9d97a0d6..8fcf8f11f5a4 100644 >> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev >> *pdev, bool runtime) >> struct e1000_adapter *adapter = netdev_priv(netdev); >> struct e1000_hw *hw = &adapter->hw; >> u32 ctrl, ctrl_ext, rctl, status, wufc; >> + u16 smb_ctrl; >> int retval = 0; >> /* Runtime suspend should only enable wakeup for link changes */ >> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev >> *pdev, bool runtime) >> if (retval) >> return retval; >> + >> + /* Force SMBUS to allow WOL */ >> + /* Switching PHY interface always returns MDI error >> + * so disable retry mechanism to avoid wasting time >> + */ >> + e1000e_disable_phy_retry(hw); >> + >> + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); >> + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; >> + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); >> + >> + e1000e_enable_phy_retry(hw); >> + >> + /* Force SMBus mode in MAC */ >> + ctrl_ext = er32(CTRL_EXT); >> + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; >> + ew32(CTRL_EXT, ctrl_ext); >> } >> /* Ensure that the appropriate bits are set in LPI_CTRL > > Why is `__e1000_shutdown()` the correct place, where it was in > `e1000_enable_ulp_lpt_lp()` before? First of all because forcing the SMBUS is not related to ULP enabling flow, but rather a configuration that is related to shutdown. Secondly, moving the function to shutdown function allows enough time for the interface to settle and avoids adding a delay. > > > Kind regards, > > Paul
Dear Vitaly, Thank you for your reply. Am 29.02.24 um 11:34 schrieb Lifshits, Vitaly: > On 2/12/2024 11:00 AM, Paul Menzel wrote: >> Am 11.02.24 um 16:12 schrieb Vitaly Lifshits: >>> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY loss on >>> some systems. >> >> Please list the systems you know of. > On some Meteor-lake systems, we also suspect that on some legacy > platforms this issue happened with some low probability. It’d be great if you could be more specific about the Meteor Lake systems. What model and what firmware versions? >>> It is suspected to be caused by initiating PHY transactions before the >>> the interface settles. >> >> No new paragraph is needed here. Please do not break the line, just >> because a sentence ends. (If you use paragraphs, please separate them >> by a blank line. > Will be fixed in a v2. Thank you. >>> Separating this configuration from the ULP enabling flow avoids this >>> issue. >>> >>> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE in Sx state") >>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >>> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >>> --- >>> drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 ------------------- >>> drivers/net/ethernet/intel/e1000e/netdev.c | 18 ++++++++++++++++++ >>> 2 files changed, 18 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> index 717c52913e84..4d83c9a0c023 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx) >>> if (ret_val) >>> goto out; >>> - /* 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) >>> - goto release; >>> - 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); >>> - >>> /* Si workaround for ULP entry flow on i127/rev6 h/w. Enable >>> * LPLU and disable Gig speed when entering ULP >>> */ >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c >>> b/drivers/net/ethernet/intel/e1000e/netdev.c >>> index af5d9d97a0d6..8fcf8f11f5a4 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >>> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev >>> *pdev, bool runtime) >>> struct e1000_adapter *adapter = netdev_priv(netdev); >>> struct e1000_hw *hw = &adapter->hw; >>> u32 ctrl, ctrl_ext, rctl, status, wufc; >>> + u16 smb_ctrl; >>> int retval = 0; >>> /* Runtime suspend should only enable wakeup for link changes */ >>> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) >>> if (retval) >>> return retval; >>> + >>> + /* Force SMBUS to allow WOL */ >>> + /* Switching PHY interface always returns MDI error >>> + * so disable retry mechanism to avoid wasting time >>> + */ >>> + e1000e_disable_phy_retry(hw); >>> + >>> + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); >>> + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; >>> + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); >>> + >>> + e1000e_enable_phy_retry(hw); >>> + >>> + /* Force SMBus mode in MAC */ >>> + ctrl_ext = er32(CTRL_EXT); >>> + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; >>> + ew32(CTRL_EXT, ctrl_ext); >>> } >>> /* Ensure that the appropriate bits are set in LPI_CTRL >> >> Why is `__e1000_shutdown()` the correct place, where it was in >> `e1000_enable_ulp_lpt_lp()` before? > First of all because forcing the SMBUS is not related to ULP enabling > flow, but rather a configuration that is related to shutdown. Secondly, > moving the function to shutdown function allows enough time for the > interface to settle and avoids adding a delay. Please add that clarification to the commit message. Kind regards, Paul
On 2/29/2024 4:31 PM, Paul Menzel wrote: > Dear Vitaly, > > > Thank you for your reply. > > > Am 29.02.24 um 11:34 schrieb Lifshits, Vitaly: > >> On 2/12/2024 11:00 AM, Paul Menzel wrote: > >>> Am 11.02.24 um 16:12 schrieb Vitaly Lifshits: >>>> Forcing SMBUS inside the ULP enabling flow leads to sporadic PHY >>>> loss on >>>> some systems. >>> >>> Please list the systems you know of. >> On some Meteor-lake systems, we also suspect that on some legacy >> platforms this issue happened with some low probability. > > It’d be great if you could be more specific about the Meteor Lake > systems. What model and what firmware versions? I do not have this kind of detailed information. > >>>> It is suspected to be caused by initiating PHY transactions before the >>>> the interface settles. >>> >>> No new paragraph is needed here. Please do not break the line, just >>> because a sentence ends. (If you use paragraphs, please separate them >>> by a blank line. >> Will be fixed in a v2. > > Thank you. > >>>> Separating this configuration from the ULP enabling flow avoids this >>>> issue. >>>> >>>> Fixes: 6607c99e7034 ("e1000e: i219 - fix to enable both ULP and EEE >>>> in Sx state") >>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >>>> Co-developed-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >>>> Signed-off-by: Dima Ruinskiy <dima.ruinskiy@intel.com> >>>> --- >>>> drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 ------------------- >>>> drivers/net/ethernet/intel/e1000e/netdev.c | 18 ++++++++++++++++++ >>>> 2 files changed, 18 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>>> index 717c52913e84..4d83c9a0c023 100644 >>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c >>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c >>>> @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw >>>> *hw, bool to_sx) >>>> if (ret_val) >>>> goto out; >>>> - /* 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) >>>> - goto release; >>>> - 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); >>>> - >>>> /* Si workaround for ULP entry flow on i127/rev6 h/w. Enable >>>> * LPLU and disable Gig speed when entering ULP >>>> */ >>>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c >>>> b/drivers/net/ethernet/intel/e1000e/netdev.c >>>> index af5d9d97a0d6..8fcf8f11f5a4 100644 >>>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >>>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >>>> @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev >>>> *pdev, bool runtime) >>>> struct e1000_adapter *adapter = netdev_priv(netdev); >>>> struct e1000_hw *hw = &adapter->hw; >>>> u32 ctrl, ctrl_ext, rctl, status, wufc; >>>> + u16 smb_ctrl; >>>> int retval = 0; >>>> /* Runtime suspend should only enable wakeup for link changes */ >>>> @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev >>>> *pdev, bool runtime) >>>> if (retval) >>>> return retval; >>>> + >>>> + /* Force SMBUS to allow WOL */ >>>> + /* Switching PHY interface always returns MDI error >>>> + * so disable retry mechanism to avoid wasting time >>>> + */ >>>> + e1000e_disable_phy_retry(hw); >>>> + >>>> + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); >>>> + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; >>>> + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); >>>> + >>>> + e1000e_enable_phy_retry(hw); >>>> + >>>> + /* Force SMBus mode in MAC */ >>>> + ctrl_ext = er32(CTRL_EXT); >>>> + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; >>>> + ew32(CTRL_EXT, ctrl_ext); >>>> } >>>> /* Ensure that the appropriate bits are set in LPI_CTRL >>> >>> Why is `__e1000_shutdown()` the correct place, where it was in >>> `e1000_enable_ulp_lpt_lp()` before? >> First of all because forcing the SMBUS is not related to ULP enabling >> flow, but rather a configuration that is related to shutdown. >> Secondly, moving the function to shutdown function allows enough time >> for the interface to settle and avoids adding a delay. > > Please add that clarification to the commit message. Thanks, I'll take care of it in v2. > > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 717c52913e84..4d83c9a0c023 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1165,25 +1165,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx) if (ret_val) goto out; - /* 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) - goto release; - 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); - /* Si workaround for ULP entry flow on i127/rev6 h/w. Enable * LPLU and disable Gig speed when entering ULP */ diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index af5d9d97a0d6..8fcf8f11f5a4 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -6622,6 +6622,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) struct e1000_adapter *adapter = netdev_priv(netdev); struct e1000_hw *hw = &adapter->hw; u32 ctrl, ctrl_ext, rctl, status, wufc; + u16 smb_ctrl; int retval = 0; /* Runtime suspend should only enable wakeup for link changes */ @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) if (retval) return retval; + + /* Force SMBUS to allow WOL */ + /* Switching PHY interface always returns MDI error + * so disable retry mechanism to avoid wasting time + */ + e1000e_disable_phy_retry(hw); + + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); + + e1000e_enable_phy_retry(hw); + + /* Force SMBus mode in MAC */ + ctrl_ext = er32(CTRL_EXT); + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; + ew32(CTRL_EXT, ctrl_ext); } /* Ensure that the appropriate bits are set in LPI_CTRL