Intel platforms’ integrated Gigabit Ethernet controllers support
2.5Gbps mode statically using BIOS programming. In the current
implementation, the BIOS menu provides an option to select between
10/100/1000Mbps and 2.5Gbps modes. Based on the selection, the BIOS
programs the Phase Lock Loop (PLL) registers. The BIOS also read the
TSN lane registers from Flexible I/O Adapter (FIA) block and provided
10/100/1000Mbps/2.5Gbps information to the stmmac driver. But
auto-negotiation between 10/100/1000Mbps and 2.5Gbps is not allowed.
The new proposal is to support auto-negotiation between 10/100/1000Mbps
and 2.5Gbps . Auto-negotiation between 10, 100, 1000Mbps will use
in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and
2.5Gbps will work as the following proposed flow, the stmmac driver reads
the PHY link status registers then identifies the negotiated speed.
Based on the speed stmmac driver will identify TSN lane registers from
FIA then send IPC command to the Power Management controller (PMC)
through PMC driver/API. PMC will act as a proxy to programs the
PLL registers.
changelog:
v1 -> v2:
- Add static to pmc_lpm_modes declaration
- Add cur_link_an_mode to the kernel doc
- Combine 2 commits i.e. "stmmac: intel: Separate driver_data of ADL-N
from TGL" and "net: stmmac: Add 1G/2.5G auto-negotiation
support for ADL-N" into 1 commit.
v2 -> v3:
- Create `pmc_ipc.c` file for `intel_pmc_ipc()` function and
allocate the file in `arch/x86/platform/intel/` directory.
- Update phylink's AN mode during phy interface change and
not exposing phylink's AN mode into phylib.
---
Choong Yong Liang (2):
net: phy: update in-band AN mode when changing interface by PHY driver
stmmac: intel: Add 1G/2.5G auto-negotiation support for ADL-N
David E. Box (1):
arch: x86: Add IPC mailbox accessor function and add SoC register
access
Tan, Tee Min (2):
net: pcs: xpcs: combine C37 SGMII AN and 2500BASEX for Intel mGbE
controller
net: stmmac: enable Intel mGbE 1G/2.5G auto-negotiation support
MAINTAINERS | 2 +
arch/x86/Kconfig | 9 +
arch/x86/platform/intel/Makefile | 1 +
arch/x86/platform/intel/pmc_ipc.c | 75 +++++++
drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 183 +++++++++++++++++-
.../net/ethernet/stmicro/stmmac/dwmac-intel.h | 81 ++++++++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 20 ++
drivers/net/pcs/pcs-xpcs.c | 72 +++++--
drivers/net/phy/phylink.c | 30 ++-
include/linux/pcs/pcs-xpcs.h | 1 +
.../linux/platform_data/x86/intel_pmc_ipc.h | 34 ++++
include/linux/stmmac.h | 1 +
13 files changed, 493 insertions(+), 17 deletions(-)
create mode 100644 arch/x86/platform/intel/pmc_ipc.c
create mode 100644 include/linux/platform_data/x86/intel_pmc_ipc.h
--
2.25.1
As there is a mechanism in PHY drivers to switch the PHY interface
between SGMII and 2500BaseX according to link speed. In this case,
the in-band AN mode should be switching based on the PHY interface
as well, if the PHY interface has been changed/updated by PHY driver.
For e.g., disable in-band AN in 2500BaseX mode, or enable in-band AN
back for SGMII mode (10/100/1000Mbps).
Signed-off-by: Choong Yong Liang <[email protected]>
---
drivers/net/phy/phylink.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0d7354955d62..5811c8086149 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1723,6 +1723,34 @@ bool phylink_expects_phy(struct phylink *pl)
}
EXPORT_SYMBOL_GPL(phylink_expects_phy);
+/**
+ * phylink_interface_change() - update both cfg_link_an_mode and
+ * cur_link_an_mode when there is a change in the interface.
+ * @phydev: pointer to &struct phy_device
+ *
+ * When the PHY interface switches between SGMII and 2500BaseX in
+ * accordance with the link speed, the in-band AN mode should also switch
+ * based on the PHY interface
+ */
+static void phylink_interface_change(struct phy_device *phydev)
+{
+ struct phylink *pl = phydev->phylink;
+
+ if (pl->phy_state.interface != phydev->interface) {
+ /* Fallback to the correct AN mode. */
+ if (phy_interface_mode_is_8023z(phydev->interface) &&
+ pl->cfg_link_an_mode == MLO_AN_INBAND) {
+ pl->cfg_link_an_mode = MLO_AN_PHY;
+ pl->cur_link_an_mode = MLO_AN_PHY;
+ } else if (pl->config->ovr_an_inband) {
+ pl->cfg_link_an_mode = MLO_AN_INBAND;
+ pl->cur_link_an_mode = MLO_AN_INBAND;
+ }
+
+ pl->phy_state.interface = phydev->interface;
+ }
+}
+
static void phylink_phy_change(struct phy_device *phydev, bool up)
{
struct phylink *pl = phydev->phylink;
@@ -1739,7 +1767,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
pl->phy_state.pause |= MLO_PAUSE_TX;
if (rx_pause)
pl->phy_state.pause |= MLO_PAUSE_RX;
- pl->phy_state.interface = phydev->interface;
+ phylink_interface_change(phydev);
pl->phy_state.link = up;
mutex_unlock(&pl->state_mutex);
--
2.25.1
From: "Tan, Tee Min" <[email protected]>
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.
Signed-off-by: Tan, Tee Min <[email protected]>
Signed-off-by: Choong Yong Liang <[email protected]>
---
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,
+};
+
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
--
2.25.1
> Auto-negotiation between 10, 100, 1000Mbps will use
> in-band auto negotiation. Auto-negotiation between 10/100/1000Mbps and
> 2.5Gbps will work as the following proposed flow, the stmmac driver reads
> the PHY link status registers then identifies the negotiated speed.
I don't think you replied to my comment.
in-band is just an optimisation. It in theory allows you to avoid a
software path, the PHY driver talking to the MAC driver about the PHY
status. As an optimisation, it is optional. Linux has the software
path and the MAC driver you are using basically has it implemented.
Why use this odd mix of in-band and out of band? It seems the change
will be simpler if you just use the out of band method all the time
and ignore in-band.
Andrew
Hi Choong
On Thu, Sep 21, 2023 at 08:19:43PM +0800, Choong Yong Liang wrote:
> From: "Tan, Tee Min" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Choong Yong Liang <[email protected]>
> ---
> 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" <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Choong Yong Liang <[email protected]>
>> ---
>> 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.