| Message ID | 20230921121946.3025771-3-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TSN auto negotiation between 1G and 2.5G | expand |
On Thu, Sep 21, 2023 at 08:19:43PM +0800, Choong Yong Liang wrote: > From: "Tan, Tee Min" <tee.min.tan@linux.intel.com> > > This commit introduces xpcs_sgmii_2500basex_features[] that combine > xpcs_sgmii_features[] and xpcs_2500basex_features[] for Intel mGbE > controller that desire to interchange the speed mode of > 10/100/1000/2500Mbps at runtime. > > Also, we introduce xpcs_config_aneg_c37_sgmii_2500basex() function Clause 37... SGMII? 2500base-X? Technically, clause 37 doesn't cover 2500base-X. > which is called by the xpcs_do_config() with the new AN mode: > DW_SGMII_2500BASEX, and this new function will proceed next-level > calling to perform C37 SGMII AN/2500BASEX configuration based on > the PHY interface updated by PHY driver. > > Signed-off-by: Tan, Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > --- > drivers/net/pcs/pcs-xpcs.c | 72 ++++++++++++++++++++++++++++++------ > include/linux/pcs/pcs-xpcs.h | 1 + > 2 files changed, 62 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 4dbc21f604f2..60d90191677d 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -104,6 +104,21 @@ static const int xpcs_2500basex_features[] = { > __ETHTOOL_LINK_MODE_MASK_NBITS, > }; > > +static const int xpcs_sgmii_2500basex_features[] = { > + ETHTOOL_LINK_MODE_Pause_BIT, > + ETHTOOL_LINK_MODE_Asym_Pause_BIT, > + ETHTOOL_LINK_MODE_Autoneg_BIT, > + ETHTOOL_LINK_MODE_10baseT_Half_BIT, > + ETHTOOL_LINK_MODE_10baseT_Full_BIT, > + ETHTOOL_LINK_MODE_100baseT_Half_BIT, > + ETHTOOL_LINK_MODE_100baseT_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseT_Half_BIT, > + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, The connected PHY could be one that supports 1000baseX as well. > + ETHTOOL_LINK_MODE_2500baseX_Full_BIT, > + ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > + __ETHTOOL_LINK_MODE_MASK_NBITS, > +}; > + > static const phy_interface_t xpcs_usxgmii_interfaces[] = { > PHY_INTERFACE_MODE_USXGMII, > }; > @@ -133,6 +148,12 @@ static const phy_interface_t xpcs_2500basex_interfaces[] = { > PHY_INTERFACE_MODE_MAX, > }; > > +static const phy_interface_t xpcs_sgmii_2500basex_interfaces[] = { > + PHY_INTERFACE_MODE_SGMII, > + PHY_INTERFACE_MODE_2500BASEX, > + PHY_INTERFACE_MODE_MAX, > +}; > + > enum { > DW_XPCS_USXGMII, > DW_XPCS_10GKR, > @@ -141,6 +162,7 @@ enum { > DW_XPCS_SGMII, > DW_XPCS_1000BASEX, > DW_XPCS_2500BASEX, > + DW_XPCS_SGMII_2500BASEX, > DW_XPCS_INTERFACE_MAX, > }; > > @@ -290,6 +312,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs, > case DW_AN_C37_SGMII: > case DW_2500BASEX: > case DW_AN_C37_1000BASEX: > + case DW_SGMII_2500BASEX: > dev = MDIO_MMD_VEND2; > break; > default: > @@ -748,6 +771,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, > if (xpcs->dev_flag == DW_DEV_TXGBE) > ret |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL; > > + /* Disable 2.5G GMII for SGMII C37 mode */ > + ret &= ~DW_VR_MII_DIG_CTRL1_2G5_EN; Do you know that this is correct for every user of this function? > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > if (ret < 0) > return ret; > @@ -848,6 +873,26 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) > return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret); > } > > +static int xpcs_config_aneg_c37_sgmii_2500basex(struct dw_xpcs *xpcs, > + unsigned int neg_mode, > + phy_interface_t interface) > +{ > + int ret = -EOPNOTSUPP; > + > + switch (interface) { > + case PHY_INTERFACE_MODE_SGMII: > + ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); > + break; > + case PHY_INTERFACE_MODE_2500BASEX: > + ret = xpcs_config_2500basex(xpcs); > + break; > + default: > + break; > + } > + > + return ret; > +} > + > int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, > const unsigned long *advertising, unsigned int neg_mode) > { > @@ -890,6 +935,12 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, > if (ret) > return ret; > break; > + case DW_SGMII_2500BASEX: > + ret = xpcs_config_aneg_c37_sgmii_2500basex(xpcs, neg_mode, > + interface); > + if (ret) > + return ret; > + break; > default: > return -1; > } > @@ -1114,6 +1165,11 @@ static void xpcs_get_state(struct phylink_pcs *pcs, > } > break; > case DW_AN_C37_SGMII: > + case DW_SGMII_2500BASEX: > + /* 2500BASEX is not supported for in-band AN mode. */ > + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) > + break; > + > ret = xpcs_get_state_c37_sgmii(xpcs, state); > if (ret) { > pr_err("xpcs_get_state_c37_sgmii returned %pe\n", > @@ -1266,23 +1322,17 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = { > .num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces), > .an_mode = DW_10GBASER, > }, > - [DW_XPCS_SGMII] = { > - .supported = xpcs_sgmii_features, > - .interface = xpcs_sgmii_interfaces, > - .num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces), > - .an_mode = DW_AN_C37_SGMII, > - }, Doesn't this break SGMII-only support (those using DW_XPCS_SGMII) ? > [DW_XPCS_1000BASEX] = { > .supported = xpcs_1000basex_features, > .interface = xpcs_1000basex_interfaces, > .num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces), > .an_mode = DW_AN_C37_1000BASEX, > }, > - [DW_XPCS_2500BASEX] = { > - .supported = xpcs_2500basex_features, > - .interface = xpcs_2500basex_interfaces, > - .num_interfaces = ARRAY_SIZE(xpcs_2500basex_interfaces), > - .an_mode = DW_2500BASEX, Doesn't this break 2500base-X only support (those using DW_XPCS_2500BASEX)? > + [DW_XPCS_SGMII_2500BASEX] = { > + .supported = xpcs_sgmii_2500basex_features, > + .interface = xpcs_sgmii_2500basex_interfaces, > + .num_interfaces = ARRAY_SIZE(xpcs_sgmii_2500basex_features), > + .an_mode = DW_SGMII_2500BASEX, > }, > }; > > diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h > index da3a6c30f6d2..f075d2fca54a 100644 > --- a/include/linux/pcs/pcs-xpcs.h > +++ b/include/linux/pcs/pcs-xpcs.h > @@ -19,6 +19,7 @@ > #define DW_2500BASEX 3 > #define DW_AN_C37_1000BASEX 4 > #define DW_10GBASER 5 > +#define DW_SGMII_2500BASEX 6 > > /* device vendor OUI */ > #define DW_OUI_WX 0x0018fc80 > -- > 2.25.1 > >
Hi Choong On Thu, Sep 21, 2023 at 08:19:43PM +0800, Choong Yong Liang wrote: > From: "Tan, Tee Min" <tee.min.tan@linux.intel.com> > > This commit introduces xpcs_sgmii_2500basex_features[] that combine > xpcs_sgmii_features[] and xpcs_2500basex_features[] for Intel mGbE > controller that desire to interchange the speed mode of > 10/100/1000/2500Mbps at runtime. > > Also, we introduce xpcs_config_aneg_c37_sgmii_2500basex() function > which is called by the xpcs_do_config() with the new AN mode: > DW_SGMII_2500BASEX, and this new function will proceed next-level > calling to perform C37 SGMII AN/2500BASEX configuration based on > the PHY interface updated by PHY driver. Why do you even need all of those changes? Please thoroughly justify because ... (see below) > > Signed-off-by: Tan, Tee Min <tee.min.tan@linux.intel.com> > Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > --- > drivers/net/pcs/pcs-xpcs.c | 72 ++++++++++++++++++++++++++++++------ > include/linux/pcs/pcs-xpcs.h | 1 + > 2 files changed, 62 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 4dbc21f604f2..60d90191677d 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -104,6 +104,21 @@ static const int xpcs_2500basex_features[] = { > __ETHTOOL_LINK_MODE_MASK_NBITS, > }; > > +static const int xpcs_sgmii_2500basex_features[] = { > + ETHTOOL_LINK_MODE_Pause_BIT, > + ETHTOOL_LINK_MODE_Asym_Pause_BIT, > + ETHTOOL_LINK_MODE_Autoneg_BIT, > + ETHTOOL_LINK_MODE_10baseT_Half_BIT, > + ETHTOOL_LINK_MODE_10baseT_Full_BIT, > + ETHTOOL_LINK_MODE_100baseT_Half_BIT, > + ETHTOOL_LINK_MODE_100baseT_Full_BIT, > + ETHTOOL_LINK_MODE_1000baseT_Half_BIT, > + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, > + ETHTOOL_LINK_MODE_2500baseX_Full_BIT, > + ETHTOOL_LINK_MODE_2500baseT_Full_BIT, > + __ETHTOOL_LINK_MODE_MASK_NBITS, > +}; > + > static const phy_interface_t xpcs_usxgmii_interfaces[] = { > PHY_INTERFACE_MODE_USXGMII, > }; > @@ -133,6 +148,12 @@ static const phy_interface_t xpcs_2500basex_interfaces[] = { > PHY_INTERFACE_MODE_MAX, > }; > > +static const phy_interface_t xpcs_sgmii_2500basex_interfaces[] = { > + PHY_INTERFACE_MODE_SGMII, > + PHY_INTERFACE_MODE_2500BASEX, > + PHY_INTERFACE_MODE_MAX, > +}; > + ... these are just a combination of the xpcs_sgmii_features[]/xpcs_sgmii_interfaces[] and xpcs_2500basex_features[]/xpcs_2500basex_interfaces[] data which are already supported by the generic DW XPCS code. All of these features and interfaces are checked in the xpcs_create() method and then get to be combined in the framework of the xpcs_validate() and xpcs_get_interfaces() functions. And ... > enum { > DW_XPCS_USXGMII, > DW_XPCS_10GKR, > @@ -141,6 +162,7 @@ enum { > DW_XPCS_SGMII, > DW_XPCS_1000BASEX, > DW_XPCS_2500BASEX, > + DW_XPCS_SGMII_2500BASEX, > DW_XPCS_INTERFACE_MAX, > }; > > @@ -290,6 +312,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs, > case DW_AN_C37_SGMII: > case DW_2500BASEX: > case DW_AN_C37_1000BASEX: > + case DW_SGMII_2500BASEX: > dev = MDIO_MMD_VEND2; > break; > default: > @@ -748,6 +771,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, > if (xpcs->dev_flag == DW_DEV_TXGBE) > ret |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL; > > + /* Disable 2.5G GMII for SGMII C37 mode */ > + ret &= ~DW_VR_MII_DIG_CTRL1_2G5_EN; * This is the only specific change in this patch. But it can be * applied independently from the rest of the changes. Although I agree * with Russel, it must be double checked since may cause regressions * on the other platforms. > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); > if (ret < 0) > return ret; > @@ -848,6 +873,26 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) > return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret); > } > > +static int xpcs_config_aneg_c37_sgmii_2500basex(struct dw_xpcs *xpcs, > + unsigned int neg_mode, > + phy_interface_t interface) > +{ > + int ret = -EOPNOTSUPP; > + > + switch (interface) { > + case PHY_INTERFACE_MODE_SGMII: > + ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); > + break; > + case PHY_INTERFACE_MODE_2500BASEX: > + ret = xpcs_config_2500basex(xpcs); > + break; > + default: > + break; > + } > + > + return ret; > +} > + ... this is just a copy of the code which is already available in xpcs_do_config(): < compat = xpcs_find_compat(xpcs->id, interface); < if (!compat) < return -ENODEV; < < switch (compat->an_mode) { < ... < case DW_AN_C37_SGMII: < ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); < if (ret) < return ret; < break; < ... < case DW_2500BASEX: < ret = xpcs_config_2500basex(xpcs); < if (ret) < return ret; < break; So based on the passed interface xpcs_find_compat() will find a proper compat descriptor, which an_mode field will be then utilized to call the respective config method. Thus, unless I miss something, basically you won't need any of the changes below and the most of the changes above reducing the patch to just a few lines. -Serge(y) > int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, > const unsigned long *advertising, unsigned int neg_mode) > { > @@ -890,6 +935,12 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, > if (ret) > return ret; > break; > + case DW_SGMII_2500BASEX: > + ret = xpcs_config_aneg_c37_sgmii_2500basex(xpcs, neg_mode, > + interface); > + if (ret) > + return ret; > + break; > default: > return -1; > } > @@ -1114,6 +1165,11 @@ static void xpcs_get_state(struct phylink_pcs *pcs, > } > break; > case DW_AN_C37_SGMII: > + case DW_SGMII_2500BASEX: > + /* 2500BASEX is not supported for in-band AN mode. */ > + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) > + break; > + > ret = xpcs_get_state_c37_sgmii(xpcs, state); > if (ret) { > pr_err("xpcs_get_state_c37_sgmii returned %pe\n", > @@ -1266,23 +1322,17 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = { > .num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces), > .an_mode = DW_10GBASER, > }, > - [DW_XPCS_SGMII] = { > - .supported = xpcs_sgmii_features, > - .interface = xpcs_sgmii_interfaces, > - .num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces), > - .an_mode = DW_AN_C37_SGMII, > - }, > [DW_XPCS_1000BASEX] = { > .supported = xpcs_1000basex_features, > .interface = xpcs_1000basex_interfaces, > .num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces), > .an_mode = DW_AN_C37_1000BASEX, > }, > - [DW_XPCS_2500BASEX] = { > - .supported = xpcs_2500basex_features, > - .interface = xpcs_2500basex_interfaces, > - .num_interfaces = ARRAY_SIZE(xpcs_2500basex_interfaces), > - .an_mode = DW_2500BASEX, > + [DW_XPCS_SGMII_2500BASEX] = { > + .supported = xpcs_sgmii_2500basex_features, > + .interface = xpcs_sgmii_2500basex_interfaces, > + .num_interfaces = ARRAY_SIZE(xpcs_sgmii_2500basex_features), > + .an_mode = DW_SGMII_2500BASEX, > }, > }; > > diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h > index da3a6c30f6d2..f075d2fca54a 100644 > --- a/include/linux/pcs/pcs-xpcs.h > +++ b/include/linux/pcs/pcs-xpcs.h > @@ -19,6 +19,7 @@ > #define DW_2500BASEX 3 > #define DW_AN_C37_1000BASEX 4 > #define DW_10GBASER 5 > +#define DW_SGMII_2500BASEX 6 > > /* device vendor OUI */ > #define DW_OUI_WX 0x0018fc80 > -- > 2.25.1 > >
On 26/9/2023 6:51 pm, Serge Semin wrote: > Hi Choong > > On Thu, Sep 21, 2023 at 08:19:43PM +0800, Choong Yong Liang wrote: >> From: "Tan, Tee Min" <tee.min.tan@linux.intel.com> >> >> This commit introduces xpcs_sgmii_2500basex_features[] that combine >> xpcs_sgmii_features[] and xpcs_2500basex_features[] for Intel mGbE >> controller that desire to interchange the speed mode of >> 10/100/1000/2500Mbps at runtime. >> >> Also, we introduce xpcs_config_aneg_c37_sgmii_2500basex() function >> which is called by the xpcs_do_config() with the new AN mode: >> DW_SGMII_2500BASEX, and this new function will proceed next-level >> calling to perform C37 SGMII AN/2500BASEX configuration based on >> the PHY interface updated by PHY driver. > > Why do you even need all of those changes? Please thoroughly justify > because ... (see below) > >> >> Signed-off-by: Tan, Tee Min <tee.min.tan@linux.intel.com> >> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> >> --- >> drivers/net/pcs/pcs-xpcs.c | 72 ++++++++++++++++++++++++++++++------ >> include/linux/pcs/pcs-xpcs.h | 1 + >> 2 files changed, 62 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c >> index 4dbc21f604f2..60d90191677d 100644 >> --- a/drivers/net/pcs/pcs-xpcs.c >> +++ b/drivers/net/pcs/pcs-xpcs.c >> @@ -104,6 +104,21 @@ static const int xpcs_2500basex_features[] = { >> __ETHTOOL_LINK_MODE_MASK_NBITS, >> }; >> > >> +static const int xpcs_sgmii_2500basex_features[] = { >> + ETHTOOL_LINK_MODE_Pause_BIT, >> + ETHTOOL_LINK_MODE_Asym_Pause_BIT, >> + ETHTOOL_LINK_MODE_Autoneg_BIT, >> + ETHTOOL_LINK_MODE_10baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_10baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_100baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_100baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >> + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >> + ETHTOOL_LINK_MODE_2500baseX_Full_BIT, >> + ETHTOOL_LINK_MODE_2500baseT_Full_BIT, >> + __ETHTOOL_LINK_MODE_MASK_NBITS, >> +}; >> + >> static const phy_interface_t xpcs_usxgmii_interfaces[] = { >> PHY_INTERFACE_MODE_USXGMII, >> }; >> @@ -133,6 +148,12 @@ static const phy_interface_t xpcs_2500basex_interfaces[] = { >> PHY_INTERFACE_MODE_MAX, >> }; >> >> +static const phy_interface_t xpcs_sgmii_2500basex_interfaces[] = { >> + PHY_INTERFACE_MODE_SGMII, >> + PHY_INTERFACE_MODE_2500BASEX, >> + PHY_INTERFACE_MODE_MAX, >> +}; >> + > > ... these are just a combination of the > xpcs_sgmii_features[]/xpcs_sgmii_interfaces[] and > xpcs_2500basex_features[]/xpcs_2500basex_interfaces[] data which are > already supported by the generic DW XPCS code. All of these features > and interfaces are checked in the xpcs_create() method and then get to > be combined in the framework of the xpcs_validate() and > xpcs_get_interfaces() functions. And ... > >> enum { >> DW_XPCS_USXGMII, >> DW_XPCS_10GKR, >> @@ -141,6 +162,7 @@ enum { >> DW_XPCS_SGMII, >> DW_XPCS_1000BASEX, >> DW_XPCS_2500BASEX, >> + DW_XPCS_SGMII_2500BASEX, >> DW_XPCS_INTERFACE_MAX, >> }; >> >> @@ -290,6 +312,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs, >> case DW_AN_C37_SGMII: >> case DW_2500BASEX: >> case DW_AN_C37_1000BASEX: >> + case DW_SGMII_2500BASEX: >> dev = MDIO_MMD_VEND2; >> break; >> default: >> @@ -748,6 +771,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, >> if (xpcs->dev_flag == DW_DEV_TXGBE) >> ret |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL; >> > >> + /* Disable 2.5G GMII for SGMII C37 mode */ >> + ret &= ~DW_VR_MII_DIG_CTRL1_2G5_EN; > > * This is the only specific change in this patch. But it can be > * applied independently from the rest of the changes. Although I agree > * with Russel, it must be double checked since may cause regressions > * on the other platforms. > >> ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); >> if (ret < 0) >> return ret; >> @@ -848,6 +873,26 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) >> return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret); >> } >> > >> +static int xpcs_config_aneg_c37_sgmii_2500basex(struct dw_xpcs *xpcs, >> + unsigned int neg_mode, >> + phy_interface_t interface) >> +{ >> + int ret = -EOPNOTSUPP; >> + >> + switch (interface) { >> + case PHY_INTERFACE_MODE_SGMII: >> + ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); >> + break; >> + case PHY_INTERFACE_MODE_2500BASEX: >> + ret = xpcs_config_2500basex(xpcs); >> + break; >> + default: >> + break; >> + } >> + >> + return ret; >> +} >> + > > ... this is just a copy of the code which is already available in xpcs_do_config(): > > < compat = xpcs_find_compat(xpcs->id, interface); > < if (!compat) > < return -ENODEV; > < > < switch (compat->an_mode) { > < ... > < case DW_AN_C37_SGMII: > < ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); > < if (ret) > < return ret; > < break; > < ... > < case DW_2500BASEX: > < ret = xpcs_config_2500basex(xpcs); > < if (ret) > < return ret; > < break; > > So based on the passed interface xpcs_find_compat() will find a proper > compat descriptor, which an_mode field will be then utilized to call > the respective config method. Thus, unless I miss something, basically > you won't need any of the changes below and the most of the changes > above reducing the patch to just a few lines. > > -Serge(y) > >> int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, >> const unsigned long *advertising, unsigned int neg_mode) >> { >> @@ -890,6 +935,12 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, >> if (ret) >> return ret; >> break; >> + case DW_SGMII_2500BASEX: >> + ret = xpcs_config_aneg_c37_sgmii_2500basex(xpcs, neg_mode, >> + interface); >> + if (ret) >> + return ret; >> + break; >> default: >> return -1; >> } >> @@ -1114,6 +1165,11 @@ static void xpcs_get_state(struct phylink_pcs *pcs, >> } >> break; >> case DW_AN_C37_SGMII: >> + case DW_SGMII_2500BASEX: >> + /* 2500BASEX is not supported for in-band AN mode. */ >> + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) >> + break; >> + >> ret = xpcs_get_state_c37_sgmii(xpcs, state); >> if (ret) { >> pr_err("xpcs_get_state_c37_sgmii returned %pe\n", >> @@ -1266,23 +1322,17 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = { >> .num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces), >> .an_mode = DW_10GBASER, >> }, >> - [DW_XPCS_SGMII] = { >> - .supported = xpcs_sgmii_features, >> - .interface = xpcs_sgmii_interfaces, >> - .num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces), >> - .an_mode = DW_AN_C37_SGMII, >> - }, >> [DW_XPCS_1000BASEX] = { >> .supported = xpcs_1000basex_features, >> .interface = xpcs_1000basex_interfaces, >> .num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces), >> .an_mode = DW_AN_C37_1000BASEX, >> }, >> - [DW_XPCS_2500BASEX] = { >> - .supported = xpcs_2500basex_features, >> - .interface = xpcs_2500basex_interfaces, >> - .num_interfaces = ARRAY_SIZE(xpcs_2500basex_interfaces), >> - .an_mode = DW_2500BASEX, >> + [DW_XPCS_SGMII_2500BASEX] = { >> + .supported = xpcs_sgmii_2500basex_features, >> + .interface = xpcs_sgmii_2500basex_interfaces, >> + .num_interfaces = ARRAY_SIZE(xpcs_sgmii_2500basex_features), >> + .an_mode = DW_SGMII_2500BASEX, >> }, >> }; >> >> diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h >> index da3a6c30f6d2..f075d2fca54a 100644 >> --- a/include/linux/pcs/pcs-xpcs.h >> +++ b/include/linux/pcs/pcs-xpcs.h >> @@ -19,6 +19,7 @@ >> #define DW_2500BASEX 3 >> #define DW_AN_C37_1000BASEX 4 >> #define DW_10GBASER 5 >> +#define DW_SGMII_2500BASEX 6 >> >> /* device vendor OUI */ >> #define DW_OUI_WX 0x0018fc80 >> -- >> 2.25.1 >> >> Hi Serge and Russell This patch does not serve the purpose correctly, I did remove this patch and handle it properly in the new patch series.
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 4dbc21f604f2..60d90191677d 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -104,6 +104,21 @@ static const int xpcs_2500basex_features[] = { __ETHTOOL_LINK_MODE_MASK_NBITS, }; +static const int xpcs_sgmii_2500basex_features[] = { + ETHTOOL_LINK_MODE_Pause_BIT, + ETHTOOL_LINK_MODE_Asym_Pause_BIT, + ETHTOOL_LINK_MODE_Autoneg_BIT, + ETHTOOL_LINK_MODE_10baseT_Half_BIT, + ETHTOOL_LINK_MODE_10baseT_Full_BIT, + ETHTOOL_LINK_MODE_100baseT_Half_BIT, + ETHTOOL_LINK_MODE_100baseT_Full_BIT, + ETHTOOL_LINK_MODE_1000baseT_Half_BIT, + ETHTOOL_LINK_MODE_1000baseT_Full_BIT, + ETHTOOL_LINK_MODE_2500baseX_Full_BIT, + ETHTOOL_LINK_MODE_2500baseT_Full_BIT, + __ETHTOOL_LINK_MODE_MASK_NBITS, +}; + static const phy_interface_t xpcs_usxgmii_interfaces[] = { PHY_INTERFACE_MODE_USXGMII, }; @@ -133,6 +148,12 @@ static const phy_interface_t xpcs_2500basex_interfaces[] = { PHY_INTERFACE_MODE_MAX, }; +static const phy_interface_t xpcs_sgmii_2500basex_interfaces[] = { + PHY_INTERFACE_MODE_SGMII, + PHY_INTERFACE_MODE_2500BASEX, + PHY_INTERFACE_MODE_MAX, +}; + enum { DW_XPCS_USXGMII, DW_XPCS_10GKR, @@ -141,6 +162,7 @@ enum { DW_XPCS_SGMII, DW_XPCS_1000BASEX, DW_XPCS_2500BASEX, + DW_XPCS_SGMII_2500BASEX, DW_XPCS_INTERFACE_MAX, }; @@ -290,6 +312,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs, case DW_AN_C37_SGMII: case DW_2500BASEX: case DW_AN_C37_1000BASEX: + case DW_SGMII_2500BASEX: dev = MDIO_MMD_VEND2; break; default: @@ -748,6 +771,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, if (xpcs->dev_flag == DW_DEV_TXGBE) ret |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL; + /* Disable 2.5G GMII for SGMII C37 mode */ + ret &= ~DW_VR_MII_DIG_CTRL1_2G5_EN; ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret); if (ret < 0) return ret; @@ -848,6 +873,26 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs) return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, ret); } +static int xpcs_config_aneg_c37_sgmii_2500basex(struct dw_xpcs *xpcs, + unsigned int neg_mode, + phy_interface_t interface) +{ + int ret = -EOPNOTSUPP; + + switch (interface) { + case PHY_INTERFACE_MODE_SGMII: + ret = xpcs_config_aneg_c37_sgmii(xpcs, neg_mode); + break; + case PHY_INTERFACE_MODE_2500BASEX: + ret = xpcs_config_2500basex(xpcs); + break; + default: + break; + } + + return ret; +} + int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, const unsigned long *advertising, unsigned int neg_mode) { @@ -890,6 +935,12 @@ int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface, if (ret) return ret; break; + case DW_SGMII_2500BASEX: + ret = xpcs_config_aneg_c37_sgmii_2500basex(xpcs, neg_mode, + interface); + if (ret) + return ret; + break; default: return -1; } @@ -1114,6 +1165,11 @@ static void xpcs_get_state(struct phylink_pcs *pcs, } break; case DW_AN_C37_SGMII: + case DW_SGMII_2500BASEX: + /* 2500BASEX is not supported for in-band AN mode. */ + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) + break; + ret = xpcs_get_state_c37_sgmii(xpcs, state); if (ret) { pr_err("xpcs_get_state_c37_sgmii returned %pe\n", @@ -1266,23 +1322,17 @@ static const struct xpcs_compat synopsys_xpcs_compat[DW_XPCS_INTERFACE_MAX] = { .num_interfaces = ARRAY_SIZE(xpcs_10gbaser_interfaces), .an_mode = DW_10GBASER, }, - [DW_XPCS_SGMII] = { - .supported = xpcs_sgmii_features, - .interface = xpcs_sgmii_interfaces, - .num_interfaces = ARRAY_SIZE(xpcs_sgmii_interfaces), - .an_mode = DW_AN_C37_SGMII, - }, [DW_XPCS_1000BASEX] = { .supported = xpcs_1000basex_features, .interface = xpcs_1000basex_interfaces, .num_interfaces = ARRAY_SIZE(xpcs_1000basex_interfaces), .an_mode = DW_AN_C37_1000BASEX, }, - [DW_XPCS_2500BASEX] = { - .supported = xpcs_2500basex_features, - .interface = xpcs_2500basex_interfaces, - .num_interfaces = ARRAY_SIZE(xpcs_2500basex_interfaces), - .an_mode = DW_2500BASEX, + [DW_XPCS_SGMII_2500BASEX] = { + .supported = xpcs_sgmii_2500basex_features, + .interface = xpcs_sgmii_2500basex_interfaces, + .num_interfaces = ARRAY_SIZE(xpcs_sgmii_2500basex_features), + .an_mode = DW_SGMII_2500BASEX, }, }; diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h index da3a6c30f6d2..f075d2fca54a 100644 --- a/include/linux/pcs/pcs-xpcs.h +++ b/include/linux/pcs/pcs-xpcs.h @@ -19,6 +19,7 @@ #define DW_2500BASEX 3 #define DW_AN_C37_1000BASEX 4 #define DW_10GBASER 5 +#define DW_SGMII_2500BASEX 6 /* device vendor OUI */ #define DW_OUI_WX 0x0018fc80