| Message ID | 20250206131859.2960543-3-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable SGMII and 2500BASEX interface mode switching for Intel platforms | expand |
On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote: > 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. Hmm. Given that description, taking it literally, claus 37 auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE 802.3 standard.) Are you absolutely sure that this applies to Cisco SGMII? If the reset is required when switching to SGMII, should it be done before or after configuring the XPCS for SGMII? Also, if the reset is required, what happens if we're already using SGMII, but AN has been disabled temporarily and is then re-enabled? Is a reset required? What about 1000BASE-X when AN is enabled or disabled and then switching to SGMII? > +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat, > + struct dw_xpcs *xpcs, > + unsigned int neg_mode) > +{ > + bool an_c37_enabled; > + int ret, mdio_ctrl; > + > + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { > + mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR); > + if (mdio_ctrl < 0) > + return mdio_ctrl; > + > + an_c37_enabled = mdio_ctrl & BMCR_ANENABLE; > + if (!an_c37_enabled) { I don't think that we need "an_c37_enabled" here, I think simply: if (!(mdio_ctrl & BMCR_ANENABLE)) { would be sufficient. > + //Perform soft reset to initiate C37 auto-negotiation > + ret = xpcs_soft_reset(xpcs, compat); > + if (ret) > + return ret; > + } > + } > + return 0; I'm also wondering (as above) whether this soft reset needs to happen _after_ xpcs_config_aneg_c37_sgmii() has done its work - this function temporarily disables AN while it's doing its work. I'm also wondering whether AN being disabled is really a deciding factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a reset required?)
On 6/2/2025 11:30 pm, Russell King (Oracle) wrote: > On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote: >> 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. > > Hmm. Given that description, taking it literally, claus 37 > auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE > 802.3 standard.) Are you absolutely sure that this applies to Cisco > SGMII? > Hi Russell, Yes, you are correct that Clause 37 auto-negotiation is for 1000BASE-X. However, I do not believe it applies to Cisco SGMII. The XPCS implements Clause 37 auto-negotiation for both 1000BASE-X and SGMII. > If the reset is required when switching to SGMII, should it be done > before or after configuring the XPCS for SGMII? > A soft reset is required before configuring the XPCS for SGMII. Based on the existing XPCS handling in the initial state, the xpcs_create() function will be called, and then xpcs->need_reset will be set to true. Later on, phylink_major_config() will call xpcs_pre_config() to perform the xpcs_soft_reset(), and then it will continue with xpcs_config(). I apologize for missing this patch: https://patchwork.kernel.org/project/netdevbpf/patch/E1svfMA-005ZI3-Va@rmk-PC.armlinux.org.uk/ I think I should move xpcs_switch_interface_mode() to xpcs_pre_config() and just update xpcs->need_reset instead of implementing my own method for calling xpcs_soft_reset(). > Also, if the reset is required, what happens if we're already using > SGMII, but AN has been disabled temporarily and is then re-enabled? > Is a reset required? > Good point. I cannot find this scenario in the datasheet. Please allow me some time to test this scenario. I will update you with the results. > What about 1000BASE-X when AN is enabled or disabled and then switching > to SGMII? > According to the datasheet, a soft reset is required. >> +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat, >> + struct dw_xpcs *xpcs, >> + unsigned int neg_mode) >> +{ >> + bool an_c37_enabled; >> + int ret, mdio_ctrl; >> + >> + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { >> + mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR); >> + if (mdio_ctrl < 0) >> + return mdio_ctrl; >> + >> + an_c37_enabled = mdio_ctrl & BMCR_ANENABLE; >> + if (!an_c37_enabled) { > > I don't think that we need "an_c37_enabled" here, I think simply: > > if (!(mdio_ctrl & BMCR_ANENABLE)) { > > would be sufficient. > >> + //Perform soft reset to initiate C37 auto-negotiation >> + ret = xpcs_soft_reset(xpcs, compat); >> + if (ret) >> + return ret; >> + } >> + } >> + return 0; > > I'm also wondering (as above) whether this soft reset needs to happen > _after_ xpcs_config_aneg_c37_sgmii() has done its work - this function > temporarily disables AN while it's doing its work. > Based on the programming sequence in the datasheet, it is not necessary to perform a soft reset after xpcs_config_aneg_c37_sgmii() has completed its work. > I'm also wondering whether AN being disabled is really a deciding > factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a > reset required?) > Thank you for pointing this out. The datasheet only mentions performing a soft reset when switching to the 1000BASE-X and SGMII interfaces, and it does not specify whether AN needs to be enabled or disabled. I thought adding a check would reduce the calls to the soft reset. However, I did not consider the scenario of switching from 1000BASE-X with AN enabled to SGMII with AN enabled. This scenario might cause regression. I will remove all the checks and just perform a soft reset when switching to the SGMII interface.
> Good point. I cannot find this scenario in the datasheet. Please allow me > some time to test this scenario. I will update you with the results. By data sheet, do you mean documentation from Synopsis, or is this an internal document? Assuming the hardware engineers have not hacked up the Synopsis IP too much, the Synopsis documentation is probably the most accurate you have. > > What about 1000BASE-X when AN is enabled or disabled and then switching > > to SGMII? > > > According to the datasheet, a soft reset is required. Do you know if this is specific to Intels integration of the Synopsis IP, or this is part of the core licensed IP? We need to understand when we need a quirk because intel did something odd, or it is part of the licensed IP and should happen for all devices using the IP. Andrew
On 7/2/2025 9:32 pm, Andrew Lunn wrote: >> Good point. I cannot find this scenario in the datasheet. Please allow me >> some time to test this scenario. I will update you with the results. > > By data sheet, do you mean documentation from Synopsis, or is this an > internal document? Assuming the hardware engineers have not hacked up > the Synopsis IP too much, the Synopsis documentation is probably the > most accurate you have. > >>> What about 1000BASE-X when AN is enabled or disabled and then switching >>> to SGMII? >>> >> According to the datasheet, a soft reset is required. > > Do you know if this is specific to Intels integration of the Synopsis > IP, or this is part of the core licensed IP? > > We need to understand when we need a quirk because intel did something > odd, or it is part of the licensed IP and should happen for all > devices using the IP. > > Andrew > Hi Andrew, Thank you for your questions. Just to clarify, when I mention the "datasheet," I am referring to the documentation from "Synopsis - DesignWare Cores Ethernet PCS" and not specific to Intel's documentation.
On 6/2/2025 11:30 pm, Russell King (Oracle) wrote: > Also, if the reset is required, what happens if we're already using > SGMII, but AN has been disabled temporarily and is then re-enabled? > Is a reset required? > Hi Russell, When we use SGMII with XPCS, the AN is disabled and then re-enabled. This process does not involve any PCS configuration, so a reset is not required. I tested it with stmmac (dwmac5) + xpcs + Marvell (88E1510) PHY, and it works fine when AN is temporarily disabled and then re-enabled without a reset.
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 1faa37f0e7b9..fb3d1548a8e0 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -817,6 +817,58 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) BMCR_SPEED1000); } +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat, + struct dw_xpcs *xpcs, + unsigned int neg_mode) +{ + bool an_c37_enabled; + int ret, mdio_ctrl; + + if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) { + mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR); + if (mdio_ctrl < 0) + return mdio_ctrl; + + an_c37_enabled = mdio_ctrl & BMCR_ANENABLE; + if (!an_c37_enabled) { + //Perform soft reset to initiate C37 auto-negotiation + ret = xpcs_soft_reset(xpcs, compat); + if (ret) + return ret; + } + } + return 0; +} + +static int xpcs_switch_interface_mode(const struct dw_xpcs_compat *compat, + struct dw_xpcs *xpcs, + phy_interface_t interface, + unsigned int neg_mode) +{ + int ret = 0; + + if (xpcs->interface != interface) { + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { + ret = txgbe_xpcs_switch_mode(xpcs, interface); + } else { + switch (compat->an_mode) { + case DW_AN_C37_SGMII: + ret = xpcs_switch_to_aneg_c37_sgmii(compat, xpcs, neg_mode); + break; + default: + break; + } + } + + if (ret) + return ret; + + xpcs->interface = interface; + } + + return 0; +} + static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, const unsigned long *advertising, unsigned int neg_mode) @@ -828,11 +880,11 @@ static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, if (!compat) return -ENODEV; - if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { - ret = txgbe_xpcs_switch_mode(xpcs, interface); - if (ret) - return ret; + ret = xpcs_switch_interface_mode(compat, xpcs, interface, neg_mode); + if (ret) + return ret; + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { /* 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 is set to 2500BASE-X, Clause 37 auto-negotiation is turned off. Subsequently, 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 | 60 ++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-)