| Message ID | 20250226074837.1679988-3-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Enable SGMII and 2500BASEX interface mode switching for Intel platforms | expand |
On Wed, Feb 26, 2025 at 03:48:33PM +0800, Choong Yong Liang wrote: > diff --git a/drivers/net/pcs/pcs-xpcs-wx.c b/drivers/net/pcs/pcs-xpcs-wx.c > index fc52f7aa5f59..f73ab04d09f0 100644 > --- a/drivers/net/pcs/pcs-xpcs-wx.c > +++ b/drivers/net/pcs/pcs-xpcs-wx.c > @@ -172,11 +172,9 @@ int txgbe_xpcs_switch_mode(struct dw_xpcs *xpcs, phy_interface_t interface) > return 0; > } > > - if (xpcs->interface == interface && !txgbe_xpcs_mode_quirk(xpcs)) > + if (!txgbe_xpcs_mode_quirk(xpcs)) > return 0; > > - xpcs->interface = interface; > - ... > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -602,12 +602,37 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces) > __set_bit(compat->interface, interfaces); > } > > +static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs, > + phy_interface_t interface) > +{ > + int ret = 0; > + > + if (xpcs->interface != interface) { > + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { > + ret = txgbe_xpcs_switch_mode(xpcs, interface); > + if (ret) > + return ret; The above modification changes the functionality. In the old code, txgbe_xpcs_switch_mode() does its work when xpcs->interface is not the same as interface OR txgbe_xpcs_mode_quirk() is true. Your replacement code calls txgbe_xpcs_switch_mode() when xpcs->interface is not the same as interface, *and* it can do its work when txgbe_xpcs_mode_quirk() returns true. So, e.g. when txgbe_xpcs_mode_quirk() returns false, but the interface changes, txgbe_xpcs_mode_quirk() used to do its work, but as a result if your changes, it becomes a no-op. The point of txgbe_xpcs_mode_quirk() is to always do the work if it returns true, even if the interface mode doesn't change. Therefore, this patch is logically incorrect, and likely breaks TXGBE.
On 26/2/2025 11:40 pm, Russell King (Oracle) wrote: > On Wed, Feb 26, 2025 at 03:48:33PM +0800, Choong Yong Liang wrote: >> diff --git a/drivers/net/pcs/pcs-xpcs-wx.c b/drivers/net/pcs/pcs-xpcs-wx.c >> index fc52f7aa5f59..f73ab04d09f0 100644 >> --- a/drivers/net/pcs/pcs-xpcs-wx.c >> +++ b/drivers/net/pcs/pcs-xpcs-wx.c >> @@ -172,11 +172,9 @@ int txgbe_xpcs_switch_mode(struct dw_xpcs *xpcs, phy_interface_t interface) >> return 0; >> } >> >> - if (xpcs->interface == interface && !txgbe_xpcs_mode_quirk(xpcs)) >> + if (!txgbe_xpcs_mode_quirk(xpcs)) >> return 0; >> >> - xpcs->interface = interface; >> - > > ... > >> --- a/drivers/net/pcs/pcs-xpcs.c >> +++ b/drivers/net/pcs/pcs-xpcs.c >> @@ -602,12 +602,37 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces) >> __set_bit(compat->interface, interfaces); >> } >> >> +static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs, >> + phy_interface_t interface) >> +{ >> + int ret = 0; >> + >> + if (xpcs->interface != interface) { >> + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { >> + ret = txgbe_xpcs_switch_mode(xpcs, interface); >> + if (ret) >> + return ret; > > The above modification changes the functionality. > > In the old code, txgbe_xpcs_switch_mode() does its work when > xpcs->interface is not the same as interface OR txgbe_xpcs_mode_quirk() > is true. > > Your replacement code calls txgbe_xpcs_switch_mode() when > xpcs->interface is not the same as interface, *and* it can do its > work when txgbe_xpcs_mode_quirk() returns true. > > So, e.g. when txgbe_xpcs_mode_quirk() returns false, but the interface > changes, txgbe_xpcs_mode_quirk() used to do its work, but as a result > if your changes, it becomes a no-op. > > The point of txgbe_xpcs_mode_quirk() is to always do the work if it > returns true, even if the interface mode doesn't change. > > Therefore, this patch is logically incorrect, and likely breaks TXGBE. > Thank you for pointing out the oversight. I will review the logic and make the necessary corrections to ensure the patch does not disrupt the expected behavior.
diff --git a/drivers/net/pcs/pcs-xpcs-wx.c b/drivers/net/pcs/pcs-xpcs-wx.c index fc52f7aa5f59..f73ab04d09f0 100644 --- a/drivers/net/pcs/pcs-xpcs-wx.c +++ b/drivers/net/pcs/pcs-xpcs-wx.c @@ -172,11 +172,9 @@ int txgbe_xpcs_switch_mode(struct dw_xpcs *xpcs, phy_interface_t interface) return 0; } - if (xpcs->interface == interface && !txgbe_xpcs_mode_quirk(xpcs)) + if (!txgbe_xpcs_mode_quirk(xpcs)) return 0; - xpcs->interface = interface; - ret = txgbe_pcs_poll_power_up(xpcs); if (ret < 0) return ret; diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index e32dec4b812e..c506797f9b63 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -602,12 +602,37 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces) __set_bit(compat->interface, interfaces); } +static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs, + phy_interface_t interface) +{ + int ret = 0; + + if (xpcs->interface != interface) { + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { + ret = txgbe_xpcs_switch_mode(xpcs, interface); + if (ret) + return ret; + } else if (interface == PHY_INTERFACE_MODE_SGMII) { + xpcs->need_reset = true; + } + + xpcs->interface = interface; + } + + return 0; +} + static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface) { struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs); const struct dw_xpcs_compat *compat; int ret; + ret = xpcs_switch_interface_mode(xpcs, interface); + if (ret) + dev_err(&xpcs->mdiodev->dev, "switch interface failed: %pe\n", + ERR_PTR(ret)); + if (!xpcs->need_reset) return; @@ -799,10 +824,6 @@ static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, return -ENODEV; if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { - ret = txgbe_xpcs_switch_mode(xpcs, interface); - if (ret) - return ret; - /* Wangxun devices need backplane CL37 AN enabled for * SGMII and 1000base-X */
The xpcs_switch_interface_mode function was introduced to handle interface switching. According to the XPCS datasheet, a soft reset is required to initiate Clause 37 auto-negotiation when the XPCS switches interface modes. When the interface mode switches from 2500BASE-X to SGMII, re-initiating Clause 37 auto-negotiation is required for the SGMII interface mode to function properly. Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> --- drivers/net/pcs/pcs-xpcs-wx.c | 4 +--- drivers/net/pcs/pcs-xpcs.c | 29 +++++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-)